Message-ID: <87cya8jqsq.fsf@kernel.org>
Date: Thu, 10 Jul 2025 12:14:45 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Benno Lossin" <lossin@...nel.org>
Cc: "Miguel Ojeda" <ojeda@...nel.org>,  "Alex Gaynor"
 <alex.gaynor@...il.com>,  "Boqun Feng" <boqun.feng@...il.com>,  "Gary Guo"
 <gary@...yguo.net>,  Björn Roy Baron
 <bjorn3_gh@...tonmail.com>,  "Alice
 Ryhl" <aliceryhl@...gle.com>,  "Masahiro Yamada" <masahiroy@...nel.org>,
  "Nathan Chancellor" <nathan@...nel.org>,  "Luis Chamberlain"
 <mcgrof@...nel.org>,  "Danilo Krummrich" <dakr@...nel.org>,  "Nicolas
 Schier" <nicolas.schier@...ux.dev>,  "Trevor Gross" <tmgross@...ch.edu>,
  "Adam Bratschi-Kaye" <ark.email@...il.com>,
  <rust-for-linux@...r.kernel.org>,  <linux-kernel@...r.kernel.org>,
  <linux-kbuild@...r.kernel.org>,  "Petr Pavlu" <petr.pavlu@...e.com>,
  "Sami Tolvanen" <samitolvanen@...gle.com>,  "Daniel Gomez"
 <da.gomez@...sung.com>,  "Simona Vetter" <simona.vetter@...ll.ch>,  "Greg
 KH" <gregkh@...uxfoundation.org>,  "Fiona Behrens" <me@...enk.dev>,
  "Daniel Almeida" <daniel.almeida@...labora.com>,
  <linux-modules@...r.kernel.org>
Subject: Re: [PATCH v16 1/7] rust: sync: add `SetOnce`

"Benno Lossin" <lossin@...nel.org> writes:

> On Wed Jul 9, 2025 at 7:52 PM CEST, Andreas Hindborg wrote:
>> Introduce the `SetOnce` type, a container that can only be written once.
>> The container uses an internal atomic to synchronize writes to the internal
>> value.
>>
>> Reviewed-by: Alice Ryhl <aliceryhl@...gle.com>
>> Signed-off-by: Andreas Hindborg <a.hindborg@...nel.org>
>
> A couple notes on safety documentation below. Also one pretty subtle
> functionality change from last version. With everything fixed:
>
> Reviewed-by: Benno Lossin <lossin@...nel.org>
>
>> ---
>>  rust/kernel/sync.rs          |   2 +
>>  rust/kernel/sync/set_once.rs | 122 +++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 124 insertions(+)
>>
>> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
>> index 81e3a806e57e2..13e6bc7fa87ac 100644
>> --- a/rust/kernel/sync.rs
>> +++ b/rust/kernel/sync.rs
>> @@ -18,6 +18,7 @@
>>  mod locked_by;
>>  pub mod poll;
>>  pub mod rcu;
>> +mod set_once;
>>
>>  pub use arc::{Arc, ArcBorrow, UniqueArc};
>>  pub use completion::Completion;
>> @@ -26,6 +27,7 @@
>>  pub use lock::mutex::{new_mutex, Mutex, MutexGuard};
>>  pub use lock::spinlock::{new_spinlock, SpinLock, SpinLockGuard};
>>  pub use locked_by::LockedBy;
>> +pub use set_once::SetOnce;
>>
>>  /// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
>>  #[repr(transparent)]
>> diff --git a/rust/kernel/sync/set_once.rs b/rust/kernel/sync/set_once.rs
>> new file mode 100644
>> index 0000000000000..73706abfe9991
>> --- /dev/null
>> +++ b/rust/kernel/sync/set_once.rs
>> @@ -0,0 +1,122 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! A container that can be initialized at most once.
>> +
>> +use super::atomic::{
>> +    ordering::{Acquire, Relaxed, Release},
>> +    Atomic,
>> +};
>> +use core::{cell::UnsafeCell, mem::MaybeUninit, ptr::drop_in_place};
>> +
>> +/// A container that can be populated at most once. Thread safe.
>> +///
>> +/// Once the a [`SetOnce`] is populated, it remains populated by the same object for the
>> +/// lifetime `Self`.
>> +///
>> +/// # Invariants
>> +///
>> +/// - `init` may only increase in value.
>> +/// - `init` may only assume values in the range `0..=2`.
>> +/// - `init == 0` if and only if the container is empty.
>> +/// - `init == 1` if and only if being initialized.
>> +/// - `init == 2` if and only if the container is populated and valid for shared access.
>
> I think I have a better idea for the last three invariants:
>
> - `init == 0` if and only if `value` is uninitialized.
> - `init == 1` if and only if there is exactly one thread with exclusive
>   access to `self.value`.
> - `init == 2` if and only if `value` is initialized and valid for shared
>   access.

Sounds good to me.

>
>> +///
>> +/// # Example
>> +///
>> +/// ```
>> +/// # use kernel::sync::SetOnce;
>> +/// let value = SetOnce::new();
>> +/// assert_eq!(None, value.as_ref());
>> +///
>> +/// let status = value.populate(42u8);
>> +/// assert_eq!(true, status);
>> +/// assert_eq!(Some(&42u8), value.as_ref());
>> +/// assert_eq!(Some(42u8), value.copy());
>> +///
>> +/// let status = value.populate(101u8);
>> +/// assert_eq!(false, status);
>> +/// assert_eq!(Some(&42u8), value.as_ref());
>> +/// assert_eq!(Some(42u8), value.copy());
>> +/// ```
>> +pub struct SetOnce<T> {
>> +    init: Atomic<u32>,
>> +    value: UnsafeCell<MaybeUninit<T>>,
>> +}
>> +
>> +impl<T> Default for SetOnce<T> {
>> +    fn default() -> Self {
>> +        Self::new()
>> +    }
>> +}
>> +
>> +impl<T> SetOnce<T> {
>> +    /// Create a new [`SetOnce`].
>> +    ///
>> +    /// The returned instance will be empty.
>> +    pub const fn new() -> Self {
>> +        // INVARIANT: The container is empty and we initialize `init` to `0`.
>> +        Self {
>> +            value: UnsafeCell::new(MaybeUninit::uninit()),
>> +            init: Atomic::new(0),
>> +        }
>> +    }
>> +
>> +    /// Get a reference to the contained object.
>> +    ///
>> +    /// Returns [`None`] if this [`SetOnce`] is empty.
>> +    pub fn as_ref(&self) -> Option<&T> {
>> +        if self.init.load(Acquire) == 2 {
>> +            // SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
>> +            // contains a valid value.
>
> s/contains a valid value/is initialized and valid for shared access/

OK.

>
>> +            Some(unsafe { &*self.value.get().cast() })
>> +        } else {
>> +            None
>> +        }
>> +    }
>> +
>> +    /// Populate the [`SetOnce`].
>> +    ///
>> +    /// Returns `true` if the [`SetOnce`] was successfully populated.
>> +    pub fn populate(&self, value: T) -> bool {
>> +        // INVARIANT: If the swap succeeds:
>> +        //  - We increase `init`.
>> +        //  - We write the valid value `1` to `init`.
>> +        //  - Only one thread can succeed in this write, so we have exclusive access after the
>> +        //    write.
>> +        if let Ok(0) = self.init.cmpxchg(0, 1, Relaxed) {
>> +            // SAFETY: By the type invariants of `Self`, the fact that we succeeded in writing `1`
>> +            // to `self.init` means we obtained exclusive access to the contained object.
>
> s/to the contained object/to `self.value`/

OK.

>
>> +            unsafe { core::ptr::write(self.value.get().cast(), value) };
>> +            // INVARIANT:
>> +            //  - We increase `init`.
>> +            //  - We write the valid value `2` to `init`.
>> +            //  - We release our exclusive access to the contained object and the object is now
>> +            //    valid for shared access.
>> +            self.init.store(2, Release);
>> +            true
>> +        } else {
>> +            false
>> +        }
>> +    }
>> +
>> +    /// Get a copy of the contained object.
>> +    ///
>> +    /// Returns [`None`] if the [`SetOnce`] is empty.
>> +    pub fn copy(&self) -> Option<T>
>> +    where
>> +        T: Copy,
>> +    {
>> +        self.as_ref().copied()
>> +    }
>> +}
>> +
>> +impl<T> Drop for SetOnce<T> {
>> +    fn drop(&mut self) {
>> +        if *self.init.get_mut() == 2 {
>> +            // SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
>> +            // contains a valid value. We have exclusive access, as we hold a `mut` reference to
>> +            // `self`.
>> +            unsafe { drop_in_place(self.value.get()) };
>
> This is sadly doing the wrong thing now since you changed the type of
> `value`: `self.value.get()` is of type `MaybeUninit<T>` and dropping
> that has (obviously) no effect. So we probably need to do
>
>     let value = unsafe { &mut *self.value.get() };
>     unsafe { value.assume_init_drop() };
>
> I almost overlooked this :)

Oops.


Best regards,
Andreas Hindborg

Powered by blists - more mailing lists