Message-ID: <CAJ-ks9mQJ5Ed-zd_p9aNAxACn7m-DhcacEpSCwtyixdjtG3gxg@mail.gmail.com>
Date: Thu, 24 Jul 2025 14:14:59 -0400
From: Tamir Duberstein <tamird@...il.com>
To: Boqun Feng <boqun.feng@...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 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.

>
> > +///         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