Message-ID: <aIJ_bJ5khAivH4kH@tardis-2.local>
Date: Thu, 24 Jul 2025 11:46:04 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Tamir Duberstein <tamird@...il.com>
Cc: Alice Ryhl <aliceryhl@...gle.com>, Miguel Ojeda <ojeda@...nel.org>,
	Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <lossin@...nel.org>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Trevor Gross <tmgross@...ch.edu>,
	Danilo Krummrich <dakr@...nel.org>, rust-for-linux@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] rust: sync: clean up LockClassKey and its docs

On Thu, Jul 24, 2025 at 02:14:59PM -0400, Tamir Duberstein wrote:
> On Wed, Jul 23, 2025 at 9:49 AM Boqun Feng <boqun.feng@...il.com> wrote:
> >
> > On Wed, Jul 23, 2025 at 11:49:34AM +0000, Alice Ryhl wrote:
> > > Several aspects of the code and documentation for this type is
> > > incomplete. Also several things are hidden from the docs. Thus, clean it
> > > up and make it easier to read the rendered html docs.
> > >
> > > Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> > > ---
> >
> > This looks good to me. One thing below:
> >
> > >  rust/kernel/sync.rs | 55 ++++++++++++++++++++++++++++++++++++++---------------
> > >  1 file changed, 40 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> > > index 9545bedf47b67976ab8c22d8368991cf1f382e42..5019a0bc95446fe30bad02ce040a1cbbe6d9ad5b 100644
> > > --- a/rust/kernel/sync.rs
> > > +++ b/rust/kernel/sync.rs
> > > @@ -26,7 +26,9 @@
> > >  pub use lock::spinlock::{new_spinlock, SpinLock, SpinLockGuard};
> > >  pub use locked_by::LockedBy;
> > >
> > > -/// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
> > > +/// Represents a lockdep class.
> > > +///
> > > +/// Wraps the kernel's `struct lock_class_key`.
> > >  #[repr(transparent)]
> > >  #[pin_data(PinnedDrop)]
> > >  pub struct LockClassKey {
> > > @@ -34,6 +36,10 @@ pub struct LockClassKey {
> > >      inner: Opaque<bindings::lock_class_key>,
> > >  }
> > >
> > > +// SAFETY: Unregistering a lock class key from a different thread than where it was registered is
> > > +// allowed.
> > > +unsafe impl Send for LockClassKey {}
> > > +
> > >  // SAFETY: `bindings::lock_class_key` is designed to be used concurrently from multiple threads and
> > >  // provides its own synchronization.
> > >  unsafe impl Sync for LockClassKey {}
> > > @@ -41,28 +47,30 @@ unsafe impl Sync for LockClassKey {}
> > >  impl LockClassKey {
> > >      /// Initializes a statically allocated lock class key.
> > >      ///
> > > -    /// This is usually used indirectly through the [`static_lock_class!`] macro.
> > > +    /// This is usually used indirectly through the [`static_lock_class!`] macro. See its
> > > +    /// documentation for more information.
> > >      ///
> > >      /// # Safety
> > >      ///
> > >      /// The destructor must never run on the returned `LockClassKey`.
> > > -    #[doc(hidden)]
> > >      pub const unsafe fn new_static() -> Self {
> > >          LockClassKey {
> > >              inner: Opaque::uninit(),
> > >          }
> > >      }
> > >
> > > -    /// Initializes a dynamically allocated lock class key. In the common case of using a
> > > -    /// statically allocated lock class key, the static_lock_class! macro should be used instead.
> > > +    /// Initializes a dynamically allocated lock class key.
> > > +    ///
> > > +    /// In the common case of using a statically allocated lock class key, the
> > > +    /// [`static_lock_class!`] macro should be used instead.
> > >      ///
> > >      /// # Examples
> > > +    ///
> > >      /// ```
> > > -    /// # use kernel::c_str;
> > > -    /// # use kernel::alloc::KBox;
> > > -    /// # use kernel::types::ForeignOwnable;
> > > -    /// # use kernel::sync::{LockClassKey, SpinLock};
> > > -    /// # use pin_init::stack_pin_init;
> > > +    /// use kernel::c_str;
> >
> > We can probably change the use `optional_name!()` to make
> > core::ffi::CStr -> kernel::str::CStr more smooth.
> >
> > > +    /// use kernel::types::ForeignOwnable;
> > > +    /// use kernel::sync::{LockClassKey, SpinLock};
> > > +    /// use pin_init::stack_pin_init;
> > >      ///
> > >      /// let key = KBox::pin_init(LockClassKey::new_dynamic(), GFP_KERNEL)?;
> > >      /// let key_ptr = key.into_foreign();
> > > @@ -80,7 +88,6 @@ impl LockClassKey {
> > >      /// // SAFETY: We dropped `num`, the only use of the key, so the result of the previous
> > >      /// // `borrow` has also been dropped. Thus, it's safe to use from_foreign.
> > >      /// unsafe { drop(<Pin<KBox<LockClassKey>> as ForeignOwnable>::from_foreign(key_ptr)) };
> > > -    ///
> > >      /// # Ok::<(), Error>(())
> > >      /// ```
> > >      pub fn new_dynamic() -> impl PinInit<Self> {
> > > @@ -90,7 +97,10 @@ pub fn new_dynamic() -> impl PinInit<Self> {
> > >          })
> > >      }
> > >
> > > -    pub(crate) fn as_ptr(&self) -> *mut bindings::lock_class_key {
> > > +    /// Returns a raw pointer to the inner C struct.
> > > +    ///
> > > +    /// It is up to the caller to use the raw pointer correctly.
> > > +    pub fn as_ptr(&self) -> *mut bindings::lock_class_key {
> > >          self.inner.get()
> > >      }
> > >  }
> > > @@ -98,14 +108,28 @@ pub(crate) fn as_ptr(&self) -> *mut bindings::lock_class_key {
> > >  #[pinned_drop]
> > >  impl PinnedDrop for LockClassKey {
> > >      fn drop(self: Pin<&mut Self>) {
> > > -        // SAFETY: self.as_ptr was registered with lockdep and self is pinned, so the address
> > > -        // hasn't changed. Thus, it's safe to pass to unregister.
> > > +        // SAFETY: `self.as_ptr()` was registered with lockdep and `self` is pinned, so the address
> > > +        // hasn't changed. Thus, it's safe to pass it to unregister.
> > >          unsafe { bindings::lockdep_unregister_key(self.as_ptr()) }
> > >      }
> > >  }
> > >
> > >  /// Defines a new static lock class and returns a pointer to it.
> > > -#[doc(hidden)]
> > > +///
> > > +/// # Examples
> > > +///
> > > +/// ```
> > > +/// use kernel::c_str;
> > > +/// use kernel::sync::{static_lock_class, Arc, SpinLock};
> > > +///
> > > +/// fn new_locked_int() -> Result<Arc<SpinLock<u32>>> {
> > > +///     Arc::pin_init(SpinLock::new(
> > > +///         42,
> > > +///         c_str!("new_locked_int"),
> >
> > We could use `optional_name!()` here to avoid another usage of
> > `c_str!()`.
> >
> > That said, I'm not sure whether we should replace `c_str!()` in the
> > example of `new_dynamic()` right now in this series, I think that
> > depends on two things: 1) whether this series goes into tip or rust-next
> > for 6.18 and 2) when we are expecting to land the replacement of
> > `c_str!()`.
> >
> > Miguel and Tamir, any thought?
> >
> > Regards,
> > Boqun
> 
> I don't think this patch meaningfully changes the complexity of the
> cstr migration. The changes are just a few tokens.
> 

Ok, so you're fine if I or someone else take this patch as it is
(including the new user of `c_str!()`), and get it merged via the tip
tree [1] in v6.18 merge window? That means if we remove `c_str!()` in
v6.18 merge window, there would be a non-trivial merge resolution to do.

Of course, it'll be less problematic if this goes into rust tree instead
of tip.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/

Regards,
Boqun

> >
> > > +///         static_lock_class!(),
> > > +///     ), GFP_KERNEL)
> > > +/// }
> > > +/// ```
> > >  #[macro_export]
> > >  macro_rules! static_lock_class {
> > >      () => {{
> > > @@ -117,6 +141,7 @@ macro_rules! static_lock_class {
> > >          $crate::prelude::Pin::static_ref(&CLASS)
> > >      }};
> > >  }
> > > +pub use static_lock_class;
> > >
> > >  /// Returns the given string, if one is provided, otherwise generates one based on the source code
> > >  /// location.
> > >
> > > --
> > > 2.50.0.727.gbf7dc18ff4-goog
> > >

Powered by blists - more mailing lists