* [PATCH v2 0/2] rust: sync: Introduce LazyInit
@ 2026-05-29 17:30 Lyude Paul
2026-05-29 17:30 ` [PATCH v2 1/2] rust: sync: lock: Add Lock::get_mut_pinned() Lyude Paul
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Lyude Paul @ 2026-05-29 17:30 UTC (permalink / raw)
To: dri-devel, linux-kernel, rust-for-linux
Cc: Gary Guo, Ingo Molnar, Miguel Ojeda, Alice Ryhl, Tamir Duberstein,
Boqun Feng, Peter Zijlstra, Benno Lossin, Will Deacon, Lyude Paul
While trying to write up some SGTable bindings for the GEM shmem
helpers, I discovered that SetOnce is actually quite difficult to make
use of in a number of situations.
For one: there is (upstream) currently no way to use it with fallible
initializers. Even with some of the work being done to add support for
this from Gary Guo and Alvin Sun:
https://lore.kernel.org/rust-for-linux/20260326-b4-tyr-debugfs-v1-1-074badd18716@linux.dev/
We're still left with the issue that if two callers race on trying to
call init(), there's no actual way for either of them to block until the
thread they raced with is done initializing the contents of the
container.
LazyInit is my proposed solution to this issue. It provides a container
which protects the initialization of its contents, without protecting
access to its contents (implying of course, that its contents have to
provide their own thread synchronization).
Lyude Paul (2):
rust: sync: lock: Add Lock::get_mut_pinned()
rust: sync: Introduce LazyInit
rust/kernel/sync.rs | 2 +
rust/kernel/sync/lazy_init.rs | 352 ++++++++++++++++++++++++++++++++++
rust/kernel/sync/lock.rs | 11 ++
3 files changed, 365 insertions(+)
create mode 100644 rust/kernel/sync/lazy_init.rs
base-commit: 0e42ec83d46ab8877d38d37493328ed7d1a24de8
--
2.54.0
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v2 1/2] rust: sync: lock: Add Lock::get_mut_pinned() 2026-05-29 17:30 [PATCH v2 0/2] rust: sync: Introduce LazyInit Lyude Paul @ 2026-05-29 17:30 ` Lyude Paul 2026-06-01 8:28 ` Alice Ryhl 2026-06-04 6:11 ` Claude review: " Claude Code Review Bot 2026-05-29 17:30 ` [PATCH v2 2/2] rust: sync: Introduce LazyInit Lyude Paul 2026-06-04 6:11 ` Claude Code Review Bot 2 siblings, 2 replies; 9+ messages in thread From: Lyude Paul @ 2026-05-29 17:30 UTC (permalink / raw) To: dri-devel, linux-kernel, rust-for-linux Cc: Gary Guo, Ingo Molnar, Miguel Ojeda, Alice Ryhl, Tamir Duberstein, Boqun Feng, Peter Zijlstra, Benno Lossin, Will Deacon, Lyude Paul These two functions are inspired by the Rust stdlib equivalent: https://doc.rust-lang.org/std/sync/struct.Mutex.html#method.get_mut() The idea here is very simple - if the user has access to a Pin<&mut Mutex<…>>, we can guarantee that no one else can look at the data protected by the lock. Thus in such situations, locking the mutex isn't necessary to access its contents. This can be useful in situations like `Drop` implementations, where we may want to access the contents of a Mutex within a struct before dropping it. So to do this, we add `get_mut_pinned()` to `Lock` - which provides a function to access the inner contents of a Mutex provided a Pin<&mut …>. Signed-off-by: Lyude Paul <lyude@redhat.com> --- rust/kernel/sync/lock.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs index 10b6b5e9b024f..5ca36baed34f5 100644 --- a/rust/kernel/sync/lock.rs +++ b/rust/kernel/sync/lock.rs @@ -190,6 +190,17 @@ pub fn try_lock(&self) -> Option<Guard<'_, T, B>> { // that `init` was called. unsafe { B::try_lock(self.state.get()).map(|state| Guard::new(self, state)) } } + + /// Returns a pinned mutable reference to the underlying data. + /// + /// Because this borrows the lock mutably, no actual locking needs to take place - as the + /// mutable borrow statically guarantees no new locks can be acquired while this reference + /// exists. + #[inline(always)] + pub fn get_mut_pinned(self: Pin<&mut Self>) -> Pin<&mut T> { + // SAFETY: We return a pinned T, ensuring we don't move T. + unsafe { self.map_unchecked_mut(|data| data.data.get_mut()) } + } } /// A lock guard. -- 2.54.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] rust: sync: lock: Add Lock::get_mut_pinned() 2026-05-29 17:30 ` [PATCH v2 1/2] rust: sync: lock: Add Lock::get_mut_pinned() Lyude Paul @ 2026-06-01 8:28 ` Alice Ryhl 2026-06-04 6:11 ` Claude review: " Claude Code Review Bot 1 sibling, 0 replies; 9+ messages in thread From: Alice Ryhl @ 2026-06-01 8:28 UTC (permalink / raw) To: Lyude Paul Cc: dri-devel, linux-kernel, rust-for-linux, Gary Guo, Ingo Molnar, Miguel Ojeda, Tamir Duberstein, Boqun Feng, Peter Zijlstra, Benno Lossin, Will Deacon On Fri, May 29, 2026 at 01:30:41PM -0400, Lyude Paul wrote: > These two functions are inspired by the Rust stdlib equivalent: > > https://doc.rust-lang.org/std/sync/struct.Mutex.html#method.get_mut() > > The idea here is very simple - if the user has access to a Pin<&mut > Mutex<…>>, we can guarantee that no one else can look at the data protected > by the lock. Thus in such situations, locking the mutex isn't necessary to > access its contents. This can be useful in situations like `Drop` > implementations, where we may want to access the contents of a Mutex within > a struct before dropping it. > > So to do this, we add `get_mut_pinned()` to `Lock` - which provides a > function to access the inner contents of a Mutex provided a Pin<&mut …>. > > Signed-off-by: Lyude Paul <lyude@redhat.com> Reviewed-by: Alice Ryhl <aliceryhl@google.com> > rust/kernel/sync/lock.rs | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs > index 10b6b5e9b024f..5ca36baed34f5 100644 > --- a/rust/kernel/sync/lock.rs > +++ b/rust/kernel/sync/lock.rs > @@ -190,6 +190,17 @@ pub fn try_lock(&self) -> Option<Guard<'_, T, B>> { > // that `init` was called. > unsafe { B::try_lock(self.state.get()).map(|state| Guard::new(self, state)) } > } > + > + /// Returns a pinned mutable reference to the underlying data. > + /// > + /// Because this borrows the lock mutably, no actual locking needs to take place - as the > + /// mutable borrow statically guarantees no new locks can be acquired while this reference > + /// exists. > + #[inline(always)] Normal #[inline] is sufficient. > + pub fn get_mut_pinned(self: Pin<&mut Self>) -> Pin<&mut T> { > + // SAFETY: We return a pinned T, ensuring we don't move T. > + unsafe { self.map_unchecked_mut(|data| data.data.get_mut()) } > + } > } > > /// A lock guard. > -- > 2.54.0 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Claude review: rust: sync: lock: Add Lock::get_mut_pinned() 2026-05-29 17:30 ` [PATCH v2 1/2] rust: sync: lock: Add Lock::get_mut_pinned() Lyude Paul 2026-06-01 8:28 ` Alice Ryhl @ 2026-06-04 6:11 ` Claude Code Review Bot 1 sibling, 0 replies; 9+ messages in thread From: Claude Code Review Bot @ 2026-06-04 6:11 UTC (permalink / raw) To: dri-devel-reviews Patch Review **Correctness: Good.** The implementation is sound. The method provides `Pin<&mut T>` from `Pin<&mut Lock<T, B>>` without acquiring the lock, which is safe because `&mut` guarantees exclusive access. This mirrors `std::sync::Mutex::get_mut()` from the Rust stdlib. ```rust pub fn get_mut_pinned(self: Pin<&mut Self>) -> Pin<&mut T> { // SAFETY: We return a pinned T, ensuring we don't move T. unsafe { self.map_unchecked_mut(|data| data.data.get_mut()) } } ``` The safety of `map_unchecked_mut` relies on the returned reference pointing to structurally pinned data. Since `data: UnsafeCell<T>` is `#[pin]` in the `Lock` struct definition (line 119 of `lock.rs`), this is correct. **Minor issues:** 1. **Commit message says "These two functions"** but only one function (`get_mut_pinned`) is added. Likely a leftover from an earlier revision: > These two functions are inspired by the Rust stdlib equivalent 2. **`#[inline(always)]` is arguably too strong.** The method is trivial and the compiler will almost certainly inline it without the hint. `#[inline]` would suffice and avoids forcing inlining in debug builds. Very minor nit. 3. The method is placed in the `impl<T: ?Sized, B: Backend> Lock<T, B>` block, which is correct — it works for unsized types and any backend. --- --- Generated by Claude Code Patch Reviewer ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] rust: sync: Introduce LazyInit 2026-05-29 17:30 [PATCH v2 0/2] rust: sync: Introduce LazyInit Lyude Paul 2026-05-29 17:30 ` [PATCH v2 1/2] rust: sync: lock: Add Lock::get_mut_pinned() Lyude Paul @ 2026-05-29 17:30 ` Lyude Paul 2026-06-01 8:27 ` Alice Ryhl 2026-06-04 6:11 ` Claude review: " Claude Code Review Bot 2026-06-04 6:11 ` Claude Code Review Bot 2 siblings, 2 replies; 9+ messages in thread From: Lyude Paul @ 2026-05-29 17:30 UTC (permalink / raw) To: dri-devel, linux-kernel, rust-for-linux Cc: Gary Guo, Ingo Molnar, Miguel Ojeda, Alice Ryhl, Tamir Duberstein, Boqun Feng, Peter Zijlstra, Viresh Kumar, Benno Lossin, Will Deacon, Lyude Paul SetOnce is nice, but it does have one problem - you can't use it with fallible initializers. While we will be adding support for doing that with SetOnce, this leads into another problem: There's no way for racing callers to actually block on the initialization of SetOnce, which makes it a difficult type to use safely for situations where we want to initialize data fallibly once, and then provide access to it to multiple users at once until drop. So to solve this, introduce a new type: LazyInit. LazyInit is like SetOnce with a couple of important differences: * It can't be used in const context * It can handle in-place fallible initializers. * It uses a Mutex for synchronization instead of an atomic, allowing callers to block on initialization. * It requires its contents already be Send + Sync, since the Mutex protects initializing data and not the data itself. Signed-off-by: Lyude Paul <lyude@redhat.com> --- V2: * Get rid of references to populate() in safety comments * Drop unused import in kunit tests * s/r @ _/r/ in unit tests * Fix many safety comments, a few of them did not get updated by me by mistake before submitting the first version of this series. * Pin-project harder, since the last time I looked at this I've got a better understanding of pin projection - so let's take advantage of that to cut down on some unsafe blocks. * Improve root doc comment a little bit * Improve INVARIANT comments rust/kernel/sync.rs | 2 + rust/kernel/sync/lazy_init.rs | 352 ++++++++++++++++++++++++++++++++++ 2 files changed, 354 insertions(+) create mode 100644 rust/kernel/sync/lazy_init.rs diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs index 993dbf2caa0e3..d18c26c56c4d2 100644 --- a/rust/kernel/sync.rs +++ b/rust/kernel/sync.rs @@ -15,6 +15,7 @@ pub mod barrier; pub mod completion; mod condvar; +mod lazy_init; pub mod lock; mod locked_by; pub mod poll; @@ -25,6 +26,7 @@ pub use arc::{Arc, ArcBorrow, UniqueArc}; pub use completion::Completion; pub use condvar::{new_condvar, CondVar, CondVarTimeoutResult}; +pub use lazy_init::*; pub use lock::global::{global_lock, GlobalGuard, GlobalLock, GlobalLockBackend, GlobalLockedBy}; pub use lock::mutex::{new_mutex, Mutex, MutexGuard}; pub use lock::spinlock::{new_spinlock, SpinLock, SpinLockGuard}; diff --git a/rust/kernel/sync/lazy_init.rs b/rust/kernel/sync/lazy_init.rs new file mode 100644 index 0000000000000..3f3126f4b153b --- /dev/null +++ b/rust/kernel/sync/lazy_init.rs @@ -0,0 +1,352 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Thread-safe lazy-initialization containers for use with [`Init`] and [`PinInit`]. +use crate::{ + prelude::*, + sync::{ + LockClassKey, + Mutex, + MutexGuard, // + }, +}; +use core::{ + mem::{ + self, + MaybeUninit, // + }, + ptr, +}; +use pin_init::{ + pin_init, + PinInit, // +}; + +/// # Invariants +/// +/// - `data` is guaranteed to be initialized if `is_init` is `true`. +/// - There is only one location where `data` is written to through an immutable shared reference, +/// `LazyInit::init()`. All other places where `data` is written to through this container require +/// exclusive mutable references. This means `data` cannot be deinitialized so long as shared +/// references to it exist. +#[pin_data(PinnedDrop)] +struct Inner<T: Send + Sync> { + #[pin] + data: MaybeUninit<T>, + is_init: bool, +} + +#[pinned_drop] +impl<T: Send + Sync> PinnedDrop for Inner<T> { + fn drop(self: Pin<&mut Self>) { + if self.is_init { + let project = self.project(); + + // SAFETY: + // - We drop this in place, ensuring we don't violate pinning invariants. + // - `is_init` is true, ensuring `data` is initialized via our type invariants. + unsafe { project.data.get_unchecked_mut().assume_init_drop() }; + + *project.is_init = false; + } + } +} + +/// Errors that can occur during [`LazyInit::new`]. +#[derive(Debug)] +pub enum LazyInitError<'a, T: Send + Sync, E = Error> { + /// The [`LazyInit`] has already been initialized. + /// + /// `self.0` contains a reference to the previously initialized value. + AlreadyInit(&'a T), + /// An error occurred during initialization. + DuringInit(E), +} + +impl<'a, T: Send + Sync, E> From<E> for LazyInitError<'a, T, E> { + fn from(value: E) -> Self { + Self::DuringInit(value) + } +} + +/// Creates a [`LazyInit`] initialiser with the given name and newly-created lock class. +/// +/// It uses the name if one is given, otherwise it generates one based on the file name and line +/// number. +#[macro_export] +macro_rules! new_lazy_init { + ($( $name:literal )?) => { + $crate::sync::LazyInit::new($crate::optional_name!($($name)?), $crate::static_lock_class!()) + }; +} +pub use new_lazy_init; + +/// A thread-safe container that allows thread-safe single-time initialization of its contents, +/// which can then have shared references taken to its contents. It is similar to [`SetOnce`] except +/// that it uses a [`Mutex`], it can be populated using fallible [`PinInit`] initializers, and +/// multiple callers attempting to initialize at the same time will block until the conetents of the +/// [`LazyInit`] is finished initializing. +/// +/// This type cannot be used in `const` contexts, however, unlike [`SetOnce`] users of this type are +/// able to block if another thread is busy populating the [`SetOnce`]. This also allows the use of +/// fallible initializers for population. +/// +/// This type internally uses a [`Mutex`] for synchronizing creation of `T`, but allows access to +/// `T` outside of lock post-init. This means that only the initialization of the value is protected +/// by the lock, and thus `T` must provide it's own synchronization by implementing `Send` + `Sync`. +/// +/// # Examples +/// +/// ``` +/// use kernel::sync::{ +/// LazyInit, +/// LazyInitError, +/// Arc, +/// new_lazy_init, // +/// }; +/// +/// struct Inner { +/// a: u8, +/// } +/// +/// #[pin_data] +/// struct Example { +/// #[pin] +/// lazy: LazyInit<Arc<Inner>>, +/// } +/// +/// impl Example { +/// fn new() -> impl PinInit<Self> { +/// pin_init!(Self { +/// lazy <- new_lazy_init!(), +/// }) +/// } +/// } +/// +/// // Allocate a boxed `Example`. +/// let e = KBox::pin_init(Example::new(), GFP_KERNEL)?; +/// +/// assert!(e.lazy.init(Arc::init(init!(Inner { a: 42 }), GFP_KERNEL)).is_ok()); +/// +/// // `as_ref()` can be used to get a reference to the contents of a `LazyInit` if its initialized. +/// assert_eq!(e.lazy.as_ref().unwrap().a, 42); +/// +/// // If `LazyInit::init()` fails due to the `LazyInit` already being initialized, a reference to +/// // the initialized data can still be retrieved from the error. +/// match e.lazy.init(Arc::init(init!(Inner { a: 54 }), GFP_KERNEL)) { +/// Err(LazyInitError::AlreadyInit(ret)) => assert_eq!(ret.a, 42), +/// _ => unreachable!(), +/// } +/// # Ok::<(), Error>(()) +/// ``` +/// +/// [`SetOnce`]: super::SetOnce +#[pin_data] +pub struct LazyInit<T: Send + Sync> { + #[pin] + inner: Mutex<Inner<T>>, +} + +impl<T: Send + Sync> LazyInit<T> { + /// Create a new initializer for an empty [`LazyInit`]. + pub fn new(name: &'static CStr, key: Pin<&'static LockClassKey>) -> impl PinInit<Self> { + pin_init!(Self { + inner <- Mutex::new( + pin_init!(Inner { + data <- MaybeUninit::uninit(), + is_init: false, + }), + name, + key + ) + }) + } + + /// Retrieve the contents of `inner.data` and extend their lifetime. + /// + /// # Safety + /// + /// The caller guarantees that `self.inner.data` has been previously initialized. + #[inline(always)] + unsafe fn data<'a>(&'a self, inner: &MutexGuard<'_, Inner<T>>) -> &'a T { + // SAFETY: + // - Our safety contract guarantees `inner.data` is initialized. + // - `T` is `Send` + `Sync`, and thus does not need the `Mutex` for synchronization, making + // it safe to hold onto outside of the lock. + // - We're guaranteed via `Inner`'s type invariants that so long as immutable references to + // self exist, `data` cannot be uninitialized - ensuring it lives throughout the lifetime + // of A. + // - We're guaranteed the container of `T` will not be written to via `Inner`s type + // invariants until `Drop`, ensuring it remains populated for the lifetime of 'a. + unsafe { mem::transmute::<&_, &'a _>(inner.data.assume_init_ref()) } + } + + /// Attempt to init the contents of [`LazyInit`] and return a reference to its contents. + /// + /// If the contents of the [`LazyInit`] were already initialized, they will not be + /// re-initialized. + /// + /// Returns: + /// + /// - `Ok(&T)` if the container was initialized successfully, or if it was already initialized + /// by another user. + /// - [`Err(LazyInitError::AlreadyInit)`](LazyInitError::AlreadyInit) if the container was + /// already initialized. This error contains a reference to the contents of the [`LazyInit`]. + /// - [`Err(LazyInitError::DuringInit)`](LazyInitError::DuringInit) if an error was encountered + /// while trying to initialize this [`LazyInit`]. + pub fn init<'a, E>( + &'a self, + init: impl PinInit<T, E>, + ) -> Result<&'a T, LazyInitError<'a, T, E>> { + let mut inner = self.inner.lock(); + let do_init = !inner.is_init; + + if do_init { + let inner = inner.as_mut().project(); + + // INVARIANT: This is the only place we can write to `data` through an immutable shared + // reference, and because we check `do_init` before writing - it can only be written to + // once. + // + // SAFETY: + // - We drop `inner.data` in place, ensuring we don't violate pinning invariants. + // - Via Inner's invariants, since we checked `is_init` we're guaranteed `data` is + // uninitialized. + // - We do not touch `data` at any point after this if we fail. + // - This does not move `data`. + unsafe { init.__pinned_init(inner.data.get_unchecked_mut().as_mut_ptr()) }?; + + // INVARIANT: Now that we've successfully initialized `inner.data`, we update + // `inner.is_init` to reflect this - fulfilling the relevant Inner type invariant. + *inner.is_init = true; + } + + // SAFETY: We confirmed `data` is initialized above. + let ret = unsafe { self.data(&inner) }; + + match do_init { + true => Ok(ret), + false => Err(LazyInitError::AlreadyInit(ret)), + } + } + + /// Get a reference to the contained object. + /// + /// Returns [`None`] if this [`LazyInit`] is empty. + pub fn as_ref<'a>(&'a self) -> Option<&'a T> { + let inner = self.inner.lock(); + + inner.is_init.then(|| { + // SAFETY: This closure can only execute if `inner.is_init` is true, guaranteeing + // `inner.data` is initialized via `Inner`s type invariants. + unsafe { self.data(&inner) } + }) + } + + /// Release the contents of the [`LazyInit`]. + /// + /// Since this call borrows the [`LazyInit`] mutably, no locking actually needs to take place - + /// as parallel lock acquisitions are prevented via the compiler enforcing Rust's borrowing + /// rules. + /// + /// Returns [`true`] if `self` was initialized before calling this function. + pub fn reset(self: Pin<&mut Self>) -> bool { + let inner = self.project().inner.get_mut_pinned(); + let was_init = inner.is_init; + + // SAFETY: + // - We drop in place, and therefore do not move `inner`. + // - The `PinnedDrop` implementation of `Inner` leaves it in a well-defined + // state, so we do not need to worry about UB from further usage. + unsafe { ptr::drop_in_place(inner.get_unchecked_mut()) }; + + was_init + } +} + +#[kunit_tests(rust_lazy_init)] +mod tests { + use super::*; + use pin_init::{ + init_from_closure, + stack_pin_init, // + }; + + #[derive(Debug)] + #[pin_data] + struct LazyInitTest {} + + impl LazyInitTest { + fn init_ok() -> impl Init<Self> { + init!(LazyInitTest {}) + } + + fn init_err(e: Error) -> impl Init<Self, Error> { + // SAFETY: + // This initializer is intended to fail for testing purposes, and does not actually + // initialize any data meaning: + // + // - We don't need to worry about returning Ok(()). + // - We have nothing to actually clean in the slot. + // - Since there is no data in the slot, nothing can meaningfully move and we have no + // pinning invariants. + unsafe { init_from_closure(move |_| Err(e)) } + } + } + + fn try_lazy_init(once: &LazyInit<LazyInitTest>) -> Result { + // It should start as being empty. + assert!((*once).as_ref().is_none()); + + // Try populating it a single time, this should succeed. + let res = once.init(LazyInitTest::init_ok()).map_err(|_| EINVAL)?; + assert!((*once).as_ref().is_some()); + + // Populating a second time should fail and return the contents. + match once.init(LazyInitTest::init_ok()) { + Err(LazyInitError::AlreadyInit(data)) => assert!(core::ptr::eq(data, res)), + r => panic!("Calling once.init() twice returned unexpected value: {r:?}"), + } + + // And it should still hold a value + assert!((*once).as_ref().is_some()); + + Ok(()) + } + + #[test] + fn lazy_init() -> Result { + stack_pin_init!(let once: LazyInit<LazyInitTest> = new_lazy_init!()); + + try_lazy_init(&once)?; + Ok(()) + } + + #[test] + fn lazy_init_error() -> Result { + stack_pin_init!(let once: LazyInit<LazyInitTest> = new_lazy_init!()); + + // Make sure it starts as empty. + assert!((*once).as_ref().is_none()); + + // Try populating it with a initializer that fails. + match once.init(LazyInitTest::init_err(EJUKEBOX)) { + Err(LazyInitError::DuringInit(e)) => assert_eq!(e, EJUKEBOX), + r => panic!( + "Calling once.init() with a failing initializer returned an unexpected value: {r:?}" + ), + } + + try_lazy_init(&once)?; + Ok(()) + } + + #[test] + fn lazy_init_reset() -> Result { + stack_pin_init!(let once: LazyInit<LazyInitTest> = new_lazy_init!()); + + try_lazy_init(&once)?; + assert_eq!(once.as_mut().reset(), true); + assert_eq!(once.as_mut().reset(), false); + try_lazy_init(&once) + } +} -- 2.54.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] rust: sync: Introduce LazyInit 2026-05-29 17:30 ` [PATCH v2 2/2] rust: sync: Introduce LazyInit Lyude Paul @ 2026-06-01 8:27 ` Alice Ryhl 2026-06-01 22:26 ` lyude 2026-06-04 6:11 ` Claude review: " Claude Code Review Bot 1 sibling, 1 reply; 9+ messages in thread From: Alice Ryhl @ 2026-06-01 8:27 UTC (permalink / raw) To: Lyude Paul Cc: dri-devel, linux-kernel, rust-for-linux, Gary Guo, Ingo Molnar, Miguel Ojeda, Tamir Duberstein, Boqun Feng, Peter Zijlstra, Viresh Kumar, Benno Lossin, Will Deacon On Fri, May 29, 2026 at 01:30:42PM -0400, Lyude Paul wrote: > SetOnce is nice, but it does have one problem - you can't use it with > fallible initializers. While we will be adding support for doing that with > SetOnce, this leads into another problem: There's no way for racing callers > to actually block on the initialization of SetOnce, which makes it a > difficult type to use safely for situations where we want to initialize > data fallibly once, and then provide access to it to multiple users at once > until drop. > > So to solve this, introduce a new type: LazyInit. LazyInit is like SetOnce > with a couple of important differences: > > * It can't be used in const context > * It can handle in-place fallible initializers. > * It uses a Mutex for synchronization instead of an atomic, allowing > callers to block on initialization. > * It requires its contents already be Send + Sync, since the Mutex protects > initializing data and not the data itself. > > Signed-off-by: Lyude Paul <lyude@redhat.com> > + /// Retrieve the contents of `inner.data` and extend their lifetime. > + /// > + /// # Safety > + /// > + /// The caller guarantees that `self.inner.data` has been previously initialized. > + #[inline(always)] > + unsafe fn data<'a>(&'a self, inner: &MutexGuard<'_, Inner<T>>) -> &'a T { > + // SAFETY: > + // - Our safety contract guarantees `inner.data` is initialized. > + // - `T` is `Send` + `Sync`, and thus does not need the `Mutex` for synchronization, making > + // it safe to hold onto outside of the lock. > + // - We're guaranteed via `Inner`'s type invariants that so long as immutable references to > + // self exist, `data` cannot be uninitialized - ensuring it lives throughout the lifetime > + // of A. > + // - We're guaranteed the container of `T` will not be written to via `Inner`s type > + // invariants until `Drop`, ensuring it remains populated for the lifetime of 'a. > + unsafe { mem::transmute::<&_, &'a _>(inner.data.assume_init_ref()) } I'm not a big fan of using these kinds of tricks to access the contents of a mutex after unlocking it. Could we instead use a struct like this: struct LazyInit<T> { data: UnsafeCell<MaybeUninit<T>>, set: AtomicBool, lock: Mutex<()>, } or even: struct LazyInit<T> { data: SetOnce<T>, lock: Mutex<()>, } I think this logic will be simpler for everyone. By the way, another option is to use a similar strategy to https://lore.kernel.org/rust-for-linux/20260523-upgrade-poll-v4-2-f5b4c747eac2@google.com/ where you just use SetOnce and protect calls to `populate` by another mutex in the structure. Then you don't need a separate LazyInit. Alice ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] rust: sync: Introduce LazyInit 2026-06-01 8:27 ` Alice Ryhl @ 2026-06-01 22:26 ` lyude 0 siblings, 0 replies; 9+ messages in thread From: lyude @ 2026-06-01 22:26 UTC (permalink / raw) To: Alice Ryhl Cc: dri-devel, linux-kernel, rust-for-linux, Gary Guo, Ingo Molnar, Miguel Ojeda, Tamir Duberstein, Boqun Feng, Peter Zijlstra, Viresh Kumar, Benno Lossin, Will Deacon On Mon, 2026-06-01 at 08:27 +0000, Alice Ryhl wrote: > I'm not a big fan of using these kinds of tricks to access the > contents > of a mutex after unlocking it. Could we instead use a struct like > this: > > struct LazyInit<T> { > data: UnsafeCell<MaybeUninit<T>>, > set: AtomicBool, > lock: Mutex<()>, > } > > or even: > > struct LazyInit<T> { > data: SetOnce<T>, > lock: Mutex<()>, > } > > I think this logic will be simpler for everyone. > By the way, another option is to use a similar strategy to > https://lore.kernel.org/rust-for-linux/20260523-upgrade-poll-v4-2-f5b4c747eac2@google.com/ > where you just use SetOnce and protect calls to `populate` by another > mutex in the structure. Then you don't need a separate LazyInit. ooo - yeah, that definitely works for me and would be preferable. I still will like to make one change to SetOnce that I think isn't too crazy: writing up a `reset()` function for SetOnce that works through a &mut like what I had here for LazyInit, since that would be quite helpful to have in the gem shmem free_callback() function. > > Alice ^ permalink raw reply [flat|nested] 9+ messages in thread
* Claude review: rust: sync: Introduce LazyInit 2026-05-29 17:30 ` [PATCH v2 2/2] rust: sync: Introduce LazyInit Lyude Paul 2026-06-01 8:27 ` Alice Ryhl @ 2026-06-04 6:11 ` Claude Code Review Bot 1 sibling, 0 replies; 9+ messages in thread From: Claude Code Review Bot @ 2026-06-04 6:11 UTC (permalink / raw) To: dri-devel-reviews Patch Review **Correctness: Sound overall.** The core design is correct. The lifetime extension, initialization synchronization, and reset semantics are all well-reasoned. Issues found are documentation/comment bugs, not logic bugs. **Issue 1 — Contradictory doc comment on `init()` (bug):** The doc comment says `Ok` is returned when already initialized, but the code returns `Err(AlreadyInit)`: ```rust /// - `Ok(&T)` if the container was initialized successfully, or if it was already initialized /// by another user. /// - [`Err(LazyInitError::AlreadyInit)`](LazyInitError::AlreadyInit) if the container was /// already initialized. ``` The first bullet includes "or if it was already initialized by another user" — this contradicts the third bullet and the actual code: ```rust match do_init { true => Ok(ret), false => Err(LazyInitError::AlreadyInit(ret)), } ``` The first bullet should read: `Ok(&T)` if the container was initialized successfully by this call. **Issue 2 — Typo "conetents":** ```rust /// multiple callers attempting to initialize at the same time will block until the conetents of the ``` Should be "contents". **Issue 3 — "it's" vs "its":** ```rust /// `T` must provide it's own synchronization by implementing `Send` + `Sync`. ``` Should be "its own synchronization" (possessive, not contraction). **Issue 4 — SAFETY comment references "lifetime of A":** ```rust /// - We're guaranteed via `Inner`'s type invariants that so long as immutable references to /// self exist, `data` cannot be uninitialized - ensuring it lives throughout the lifetime /// of A. ``` Should be "lifetime of `'a`" — referring to the generic lifetime parameter. **Issue 5 — `reset()` correctness is subtle but sound:** ```rust pub fn reset(self: Pin<&mut Self>) -> bool { let inner = self.project().inner.get_mut_pinned(); let was_init = inner.is_init; unsafe { ptr::drop_in_place(inner.get_unchecked_mut()) }; was_init } ``` This calls `ptr::drop_in_place` on `Inner`, which runs `PinnedDrop::drop`. That implementation sets `is_init = false` and drops the value. When the enclosing `Mutex<Inner<T>>` is later dropped (at `LazyInit` destruction), `PinnedDrop::drop` runs again on `Inner`, sees `is_init == false`, and is a no-op. This is safe because `MaybeUninit<T>` and `bool` have no drop glue, so the "double drop" of `Inner` is benign. The SAFETY comment could be more explicit about why re-running `PinnedDrop` later is safe: ```rust // SAFETY: // - We drop in place, and therefore do not move `inner`. // - The `PinnedDrop` implementation of `Inner` leaves it in a well-defined // state, so we do not need to worry about UB from further usage. ``` It would be worth adding: "When `Inner` is dropped again during `LazyInit`'s destruction, `is_init` will be `false`, making the second `PinnedDrop` a no-op. `Inner`'s fields (`MaybeUninit<T>`, `bool`) have no implicit drop glue." **Issue 6 — `pub use lazy_init::*` is a glob re-export:** ```rust pub use lazy_init::*; ``` This exports everything public from the module. Currently that's `LazyInit`, `LazyInitError`, and `new_lazy_init`. This works but is inconsistent with how other types in `sync.rs` are re-exported (explicit names). A named re-export would be more conventional for the kernel codebase: ```rust pub use lazy_init::{LazyInit, LazyInitError, new_lazy_init}; ``` **Issue 7 — The `data()` helper's `inner` parameter type is slightly unusual:** ```rust unsafe fn data<'a>(&'a self, inner: &MutexGuard<'_, Inner<T>>) -> &'a T { unsafe { mem::transmute::<&_, &'a _>(inner.data.assume_init_ref()) } } ``` The `MutexGuard` parameter serves as a witness that the lock is held, but it's only used to access the data through `Deref`. The lifetime extension via `transmute` from the guard's borrow to `'a` (tied to `self`) is the key unsafe operation. The safety argument is sound: once initialized, `T` can't be deinitialized until `Drop` (which requires `&mut self`, incompatible with the `'a` borrow from `&'a self`), and `T: Send + Sync` makes lock-free access safe. **Issue 8 — Tests are good.** The three kunit tests cover: - Basic init + double-init (`lazy_init`) - Failed init followed by successful init (`lazy_init_error`) - Init + reset + re-init (`lazy_init_reset`) These exercise the key state transitions well. --- Generated by Claude Code Patch Reviewer ^ permalink raw reply [flat|nested] 9+ messages in thread
* Claude review: rust: sync: Introduce LazyInit 2026-05-29 17:30 [PATCH v2 0/2] rust: sync: Introduce LazyInit Lyude Paul 2026-05-29 17:30 ` [PATCH v2 1/2] rust: sync: lock: Add Lock::get_mut_pinned() Lyude Paul 2026-05-29 17:30 ` [PATCH v2 2/2] rust: sync: Introduce LazyInit Lyude Paul @ 2026-06-04 6:11 ` Claude Code Review Bot 2 siblings, 0 replies; 9+ messages in thread From: Claude Code Review Bot @ 2026-06-04 6:11 UTC (permalink / raw) To: dri-devel-reviews Overall Series Review Subject: rust: sync: Introduce LazyInit Author: Lyude Paul <lyude@redhat.com> Patches: 6 Reviewed: 2026-06-04T16:11:03.339061 --- This is a well-structured 2-patch series from Lyude Paul introducing `LazyInit<T>`, a thread-safe lazy-initialization container for the kernel's Rust abstractions. The motivation is clear: the existing `SetOnce` cannot handle fallible initializers and doesn't allow racing callers to block until initialization completes. `LazyInit` solves both by using a `Mutex` for synchronization. **Patch 1** adds a small utility method `Lock::get_mut_pinned()` that Patch 2 depends on. **Patch 2** is the bulk of the work — a new `LazyInit<T>` type with Mutex-based synchronization, fallible `PinInit` support, `reset()` capability, and thorough kunit tests. The design is sound: the Mutex protects only initialization, while post-init access is lock-free (relying on `T: Send + Sync`). The lifetime extension pattern via `mem::transmute` in `data()` is the riskiest construct but is correctly justified. The `reset()` implementation using `ptr::drop_in_place` on `Inner` is subtle but correct given that `PinnedDrop` leaves `Inner` in a well-defined uninitialized state and the fields have no drop glue of their own. There are a few documentation bugs and minor issues detailed below, but no correctness issues found. --- --- Generated by Claude Code Patch Reviewer ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-06-04 6:11 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-05-29 17:30 [PATCH v2 0/2] rust: sync: Introduce LazyInit Lyude Paul 2026-05-29 17:30 ` [PATCH v2 1/2] rust: sync: lock: Add Lock::get_mut_pinned() Lyude Paul 2026-06-01 8:28 ` Alice Ryhl 2026-06-04 6:11 ` Claude review: " Claude Code Review Bot 2026-05-29 17:30 ` [PATCH v2 2/2] rust: sync: Introduce LazyInit Lyude Paul 2026-06-01 8:27 ` Alice Ryhl 2026-06-01 22:26 ` lyude 2026-06-04 6:11 ` Claude review: " Claude Code Review Bot 2026-06-04 6:11 ` Claude Code Review Bot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox