From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id BA5982D9792; Thu, 10 Jul 2025 10:14:55 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1752142495; cv=none; b=BgPhQpKdxzqYEx4sQiUOWWsG5xDpVbWzcq0aIiVM+cXzSWNTAVYrB9Mf7erQTKSX3KOu1H4CAqSvlEWB1ENhobMiaGBEMODNTlA2jiWkFDnf/HV/PtBp4epxIJfGrASLcCe01YQ0MgXDBn9XsJhq5AKfKp8Lk3xoIVjnE3ZWjjY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1752142495; c=relaxed/simple; bh=88rsh88mpW5gcwA7FMx0qvwMnXGoQaLQuq1H0aQocyg=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=otdx7EO6yAUjHGpQH9QLprfhSq5+RS0Xgy++Askzt/OpVjF89+tA8g1Rtp9JUB1KvZt5og0aakHgGfEGUzxnw4bPyrA8xPM2XqhvN0t6Cv4FHcQeAfKEpaa7mrQqdKXccKeyoeW13cBjlRPAbrLcE3Y7d8+AdjuzV0WPGr5Qgz4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=SY6blPnP; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="SY6blPnP" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 7DACDC4CEF8; Thu, 10 Jul 2025 10:14:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1752142495; bh=88rsh88mpW5gcwA7FMx0qvwMnXGoQaLQuq1H0aQocyg=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=SY6blPnPbTmwB4sDAlK0OqG/Or1ix1Bk/GL9MoAJ5cD0inWZ7+8jWKq9WMaRgnktJ KXrtNH9VP/Rwup+Q4VeOlfFoaI6LUjDtNaavHZ+ixMUrbSp6VuZqXbqy56Gh5jgPLy FtPOwxtKtK+WXMc4mg88eaY+u5RQe8jKhYlNoeUttqkVA7N+Xo3+3ciYDxR0tPiPai IbHEaqd5Sa9AhVX1NWu1OP2PNjHKTMUAmXQwyHNJ2cvDB9/XCF4xdjpUG4nC/Ojzzh R1jQ1W26mtFx/727ckjEaQJFPtJVmORHvz0d0s8sag02MSUtpkc3HbKALCEMTxh2nb 8CuePCkWuVivg== From: Andreas Hindborg To: "Benno Lossin" Cc: "Miguel Ojeda" , "Alex Gaynor" , "Boqun Feng" , "Gary Guo" , =?utf-8?Q?Bj=C3=B6rn?= Roy Baron , "Alice Ryhl" , "Masahiro Yamada" , "Nathan Chancellor" , "Luis Chamberlain" , "Danilo Krummrich" , "Nicolas Schier" , "Trevor Gross" , "Adam Bratschi-Kaye" , , , , "Petr Pavlu" , "Sami Tolvanen" , "Daniel Gomez" , "Simona Vetter" , "Greg KH" , "Fiona Behrens" , "Daniel Almeida" , Subject: Re: [PATCH v16 1/7] rust: sync: add `SetOnce` In-Reply-To: (Benno Lossin's message of "Wed, 09 Jul 2025 22:16:24 +0200") References: <20250709-module-params-v3-v16-0-4f926bcccb50@kernel.org> <20250709-module-params-v3-v16-1-4f926bcccb50@kernel.org> <9xgynwGn-IAcmiUICPvPtOFa-TV5IRH8pu3WLdvQRLG56Q_DXBvxy0q_9Ykmgff1LbRXht1XoYaQq13av21T2A==@protonmail.internalid> User-Agent: mu4e 1.12.9; emacs 30.1 Date: Thu, 10 Jul 2025 12:14:45 +0200 Message-ID: <87cya8jqsq.fsf@kernel.org> Precedence: bulk X-Mailing-List: rust-for-linux@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain "Benno Lossin" 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 >> Signed-off-by: Andreas Hindborg > > A couple notes on safety documentation below. Also one pretty subtle > functionality change from last version. With everything fixed: > > Reviewed-by: Benno Lossin > >> --- >> 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 { >> + init: Atomic, >> + value: UnsafeCell>, >> +} >> + >> +impl Default for SetOnce { >> + fn default() -> Self { >> + Self::new() >> + } >> +} >> + >> +impl SetOnce { >> + /// 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 >> + where >> + T: Copy, >> + { >> + self.as_ref().copied() >> + } >> +} >> + >> +impl Drop for SetOnce { >> + 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` 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