public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH 0/2] rust: sync: Introduce LazyInit
@ 2026-05-26 20:41 Lyude Paul
  2026-05-26 20:41 ` [PATCH 1/2] rust: sync: lock: Add Lock::get_mut_pinned() Lyude Paul
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Lyude Paul @ 2026-05-26 20:41 UTC (permalink / raw)
  To: dri-devel, rust-for-linux, nouveau
  Cc: Gary Guo, Ingo Molnar, Miguel Ojeda, Alice Ryhl, linux-kernel,
	Tamir Duberstein, Boqun Feng, Peter Zijlstra, Shankari Anand,
	Viresh Kumar, 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 | 354 ++++++++++++++++++++++++++++++++++
 rust/kernel/sync/lock.rs      |  11 ++
 3 files changed, 367 insertions(+)
 create mode 100644 rust/kernel/sync/lazy_init.rs

-- 
2.54.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] rust: sync: lock: Add Lock::get_mut_pinned()
  2026-05-26 20:41 [PATCH 0/2] rust: sync: Introduce LazyInit Lyude Paul
@ 2026-05-26 20:41 ` Lyude Paul
  2026-05-27  4:07   ` Claude review: " Claude Code Review Bot
  2026-05-26 20:41 ` [PATCH 2/2] rust: sync: Introduce LazyInit Lyude Paul
  2026-05-27  4:07 ` Claude Code Review Bot
  2 siblings, 1 reply; 6+ messages in thread
From: Lyude Paul @ 2026-05-26 20:41 UTC (permalink / raw)
  To: dri-devel, rust-for-linux, nouveau
  Cc: Gary Guo, Ingo Molnar, Miguel Ojeda, Alice Ryhl, linux-kernel,
	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] 6+ messages in thread

* [PATCH 2/2] rust: sync: Introduce LazyInit
  2026-05-26 20:41 [PATCH 0/2] rust: sync: Introduce LazyInit Lyude Paul
  2026-05-26 20:41 ` [PATCH 1/2] rust: sync: lock: Add Lock::get_mut_pinned() Lyude Paul
@ 2026-05-26 20:41 ` Lyude Paul
  2026-05-27  4:07   ` Claude review: " Claude Code Review Bot
  2026-05-27  4:07 ` Claude Code Review Bot
  2 siblings, 1 reply; 6+ messages in thread
From: Lyude Paul @ 2026-05-26 20:41 UTC (permalink / raw)
  To: dri-devel, rust-for-linux, nouveau
  Cc: Gary Guo, Ingo Molnar, Miguel Ojeda, Alice Ryhl, linux-kernel,
	Tamir Duberstein, Boqun Feng, Peter Zijlstra, Shankari Anand,
	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>
---
 rust/kernel/sync.rs           |   2 +
 rust/kernel/sync/lazy_init.rs | 354 ++++++++++++++++++++++++++++++++++
 2 files changed, 356 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..707af369e965d
--- /dev/null
+++ b/rust/kernel/sync/lazy_init.rs
@@ -0,0 +1,354 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Thread-safe Lazy-[`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`.
+/// - `data` is only written to once from `LazyInit::init()`, and once from `LazyInit::reset()`
+///   (which cannot race with `LazyInit::init()` due to requiring a `&mut` reference to the
+///   `LazyInit`).
+#[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 {
+            // SAFETY: The only contents from `this` that we move is `is_init`. `is_init` is Unpin,
+            // making this OK.
+            let this = unsafe { self.get_unchecked_mut() };
+
+            // INVARIANT: This is the only place other then `LazyInit::populate()` where we write to
+            // `data`, and this function requires &mut self - ensuring it cannot race with
+            // `LazyInit::populate()`.
+            // SAFETY: Since `is_init` is true, our type invariants guarantee `data` is initialized.
+            unsafe { this.data.assume_init_drop() };
+
+            this.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 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 {
+            // SAFETY: The only thing that we move is `is_init`, which is Unpin.
+            let inner = unsafe { inner.as_mut().get_unchecked_mut() };
+
+            // INVARIANT: This is the only place we can write to `data` before Drop, fulfilling
+            // `Inner`'s relevant type invariant.
+            //
+            // SAFETY:
+            // - 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.as_mut_ptr()) }?;
+
+            inner.is_init = true;
+        }
+
+        // INVARIANT: This code is only reachable if `data` was, either previously or just now,
+        // initialized with a valid instance of `T`. Otherwise we've never written to it and `data`
+        // is guaranteed to be uninitialized, fulfilling `Inner`s type invariants regarding
+        // `data` initialization state.
+
+        // 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 core::ops::Deref;
+    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<(), Error> {
+        // 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] 6+ messages in thread

* Claude review: rust: sync: Introduce LazyInit
  2026-05-26 20:41 [PATCH 0/2] rust: sync: Introduce LazyInit Lyude Paul
  2026-05-26 20:41 ` [PATCH 1/2] rust: sync: lock: Add Lock::get_mut_pinned() Lyude Paul
  2026-05-26 20:41 ` [PATCH 2/2] rust: sync: Introduce LazyInit Lyude Paul
@ 2026-05-27  4:07 ` Claude Code Review Bot
  2 siblings, 0 replies; 6+ messages in thread
From: Claude Code Review Bot @ 2026-05-27  4:07 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: rust: sync: Introduce LazyInit
Author: Lyude Paul <lyude@redhat.com>
Patches: 3
Reviewed: 2026-05-27T14:07:41.246615

---

This is a well-motivated 2-patch series from Lyude Paul introducing `LazyInit<T>`, a mutex-backed lazy-initialization container for the kernel Rust abstractions. The cover letter clearly explains the problem: `SetOnce` cannot handle fallible initializers, and racing callers have no way to block until initialization completes. `LazyInit` solves both by using a `Mutex` to serialize initialization while allowing lock-free access post-init (requiring `T: Send + Sync`).

The code is generally well-structured with appropriate safety comments and invariant documentation. The `unsafe` usage is mostly sound and well-justified. There are a few issues worth addressing, ranging from a potential soundness concern in `reset()` to minor documentation nits.

**Recommendation**: Needs minor revisions. One potential soundness issue in `reset()` and several smaller items.

---

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Claude review: rust: sync: lock: Add Lock::get_mut_pinned()
  2026-05-26 20:41 ` [PATCH 1/2] rust: sync: lock: Add Lock::get_mut_pinned() Lyude Paul
@ 2026-05-27  4:07   ` Claude Code Review Bot
  0 siblings, 0 replies; 6+ messages in thread
From: Claude Code Review Bot @ 2026-05-27  4:07 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

This patch adds a method to `Lock` that provides `Pin<&mut T>` access when you hold a `Pin<&mut Lock<...>>`, bypassing the actual locking since `&mut` guarantees exclusivity at compile time. This mirrors `std::sync::Mutex::get_mut()`.

**Correctness**: The implementation is sound. `Lock::data` is declared as `#[pin] pub(crate) data: UnsafeCell<T>` (lock.rs:118-119), so `T` is structurally pinned within the `Lock`. Having `Pin<&mut Self>` guarantees exclusive access, and returning `Pin<&mut T>` preserves the pin projection correctly. The `map_unchecked_mut` + `UnsafeCell::get_mut` path is the standard pattern.

**Minor items**:

1. The SAFETY comment is slightly sparse:
```rust
// SAFETY: We return a pinned T, ensuring we don't move T.
unsafe { self.map_unchecked_mut(|data| data.data.get_mut()) }
```
It would be more precise to note *why* this is a valid pin projection — i.e., that `data` is structurally pinned within `Lock` (via `#[pin]` on the field). The current comment explains the output constraint (we don't move T) but not the input justification (the field is structurally pinned). This is minor since the `#[pin]` attribute is visible on the struct definition.

2. The `#[inline(always)]` is arguably aggressive for a method that will likely only be called in `Drop` paths and similar low-frequency situations. `#[inline]` would be sufficient, consistent with the existing `lock()` and `try_lock()` methods in the same `impl` block.

3. The commit message references "These two functions" (plural), but only one function is added. Looks like the commit message may have been written when a non-pinned `get_mut()` was also planned.

**Verdict**: Good. The approach is correct and matches standard Rust practice.

---

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Claude review: rust: sync: Introduce LazyInit
  2026-05-26 20:41 ` [PATCH 2/2] rust: sync: Introduce LazyInit Lyude Paul
@ 2026-05-27  4:07   ` Claude Code Review Bot
  0 siblings, 0 replies; 6+ messages in thread
From: Claude Code Review Bot @ 2026-05-27  4:07 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

This is the main patch. A thorough review:

**1. Potential soundness issue in `reset()`** (lazy_init.rs:253-264):

```rust
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
}
```

After `drop_in_place`, the `Inner` is left in a dropped state. The `PinnedDrop` impl does set `is_init = false`, which means subsequent `init()` calls on the re-used `LazyInit` will try to re-initialize. However, the `Mutex` *itself* is **not** dropped or re-initialized here — only its contents (`Inner`) are dropped in place. The question is whether the `Mutex`'s internal C state remains valid after its protected data has been dropped-in-place and a new value written into it via a subsequent `init()` call.

Since the `Mutex` wraps `Inner` and the lock state is separate from the data, this should be fine for the mutex itself. But there's a subtler concern: `Inner` contains `MaybeUninit<T>` which after `PinnedDrop` runs `assume_init_drop()` will have dropped `T` but the `MaybeUninit` bytes are still there (just logically uninitialized). The `is_init` flag being set to `false` in `PinnedDrop` ensures the invariant is maintained. Then when `init()` is called again, `__pinned_init` writes to `data.as_mut_ptr()` which writes over the `MaybeUninit` — this is fine because `MaybeUninit` allows writing at any time.

The real concern: after `drop_in_place`, the `Inner` struct's own drop flag (if any) or internal state may be in an undefined state for further use. In Rust, using a value after `drop_in_place` is valid only if you ensure no further drop will occur or you re-initialize it properly. Here, `LazyInit` itself will still be dropped eventually, which drops the `Mutex`, which drops its `Inner`. But `Inner` was already dropped in place. If `PinnedDrop` is called again on the already-dropped `Inner`, this is double-drop UB.

Actually, looking more carefully: `reset()` drops `Inner` in place via `ptr::drop_in_place`. Then when the `LazyInit` goes out of scope, the `Mutex` is dropped, which will try to drop its `Inner` contents again. If `is_init` is `false` (set by `PinnedDrop`), the second drop will skip the `assume_init_drop` path. But the `PinnedDrop` trait itself will still be invoked a second time — is that safe? The `PinnedDrop` impl only conditionally drops based on `is_init`, so the second call will be a no-op on the data. The `is_init = false` assignment is idempotent. So this should be safe **provided** that calling `PinnedDrop` on an already-dropped `Inner` does nothing harmful beyond the `is_init` check.

However, this pattern of `drop_in_place` followed by re-use is fragile. Consider documenting this more explicitly, or using a more conventional approach like `mem::replace` or swapping in an uninitialized `Inner`.

**2. Lifetime extension in `data()` uses `mem::transmute`** (lazy_init.rs:172-179):

```rust
unsafe fn data<'a>(&'a self, inner: &MutexGuard<'_, Inner<T>>) -> &'a T {
    unsafe { mem::transmute::<&_, &'a _>(inner.data.assume_init_ref()) }
}
```

This transmute extends the lifetime of the reference from the `MutexGuard`'s lifetime to `'a` (the `LazyInit`'s lifetime). The safety argument is that once initialized, the data is never moved or dropped until `LazyInit` itself is dropped, and `T: Send + Sync` makes concurrent access safe.

The concern is that the `MutexGuard` is dropped (unlocking the mutex) while the extended-lifetime reference still exists. Then another thread could call `init()`, acquire the lock, and — well, `init()` checks `is_init` and short-circuits if already initialized, so the data won't be overwritten. The invariant holds. But if `reset()` were ever callable from `&self` rather than `Pin<&mut Self>`, this would be unsound. The `&mut` requirement on `reset()` is load-bearing for this safety argument — worth a comment on `data()` noting this dependency.

**3. Documentation typos/nits** (lazy_init.rs:88-89):

```
/// multiple callers attempting to initialize at the same time will block until the conetents of the
```
"conetents" → "contents"

(lazy_init.rs:97):
```
/// by the lock, and thus `T` must provide it's own synchronization
```
"it's" → "its" (possessive, not contraction)

**4. Stale reference to `LazyInit::populate()` in `PinnedDrop`** (lazy_init.rs:45-46):

```rust
// INVARIANT: This is the only place other then `LazyInit::populate()` where we write to
// `data`, and this function requires &mut self - ensuring it cannot race with
// `LazyInit::populate()`.
```

There is no `LazyInit::populate()` method. This should reference `LazyInit::init()`.

**5. The `LazyInitError` doc says "during `LazyInit::new`"** (lazy_init.rs:56):

```rust
/// Errors that can occur during [`LazyInit::new`].
```

This should be "during [`LazyInit::init`]" — `new()` returns an initializer and doesn't produce `LazyInitError`.

**6. Glob re-export** (sync.rs):

```rust
pub use lazy_init::*;
```

This is a glob re-export from a non-`pub` module. While it works, the kernel Rust style generally prefers explicit re-exports (as seen with all the other items in the same block: `Arc`, `CondVar`, `Mutex`, etc.). Consider `pub use lazy_init::{LazyInit, LazyInitError, new_lazy_init};` for consistency.

**7. Unused import in tests** (lazy_init.rs:269):

```rust
use core::ops::Deref;
```

`Deref` does not appear to be explicitly used in the test module. The `(*once).as_ref()` calls use the deref operator `*`, but that doesn't require an explicit `use core::ops::Deref` import.

**8. Match pattern style** (lazy_init.rs:309, 336):

```rust
r @ _ => panic!("..."),
```

`r @ _` is equivalent to just `r`. The `@ _` binding is unnecessary.

**9. The `init()` return type semantics are slightly unusual**: Returning `Err(AlreadyInit(&T))` when the value was already initialized makes the "already init" path an error, but the caller often doesn't care and just wants the reference. Users will frequently need `.unwrap_or_else(|e| match e { AlreadyInit(v) => v, DuringInit(e) => return Err(e) })`. Consider whether a method like `get_or_init()` that returns `Result<&T, E>` (mapping AlreadyInit to Ok) would be a more ergonomic primary API, with the current `init()` as an `init_unique()` or similar for callers that need to distinguish.

**Verdict**: Mostly sound with good safety documentation. The `populate()` reference is clearly a leftover from development, and the `LazyInit::new` doc on `LazyInitError` should be fixed. The `reset()` + double-drop pattern deserves closer examination but appears safe given the `PinnedDrop` implementation's idempotent behavior. The lifetime extension via transmute is the trickiest part of this patch but is correctly guarded by the `&mut Self` requirement on `reset()`.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-05-27  4:07 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-26 20:41 [PATCH 0/2] rust: sync: Introduce LazyInit Lyude Paul
2026-05-26 20:41 ` [PATCH 1/2] rust: sync: lock: Add Lock::get_mut_pinned() Lyude Paul
2026-05-27  4:07   ` Claude review: " Claude Code Review Bot
2026-05-26 20:41 ` [PATCH 2/2] rust: sync: Introduce LazyInit Lyude Paul
2026-05-27  4:07   ` Claude review: " Claude Code Review Bot
2026-05-27  4:07 ` 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