Message-ID: <CAJ-ks9k-bJ7ENDXcj++KHzeefGjZAKLvn1QsbpDU-p=M-XtJ7Q@mail.gmail.com>
Date: Thu, 24 Jul 2025 15:11:41 -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 Thu, Jul 24, 2025 at 2:46 PM Boqun Feng <boqun.feng@...il.com> wrote:
>
> 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.

The merge conflict would only arise at the very end of the migration,
when we're using C-string literals everywhere *and* the macro has been
renamed. I haven't even sent that final series yet. Regardless, the
resolution would be quite trivial, I think:
- remove the import that doesn't exist
- replace `c_str!("new_locked_int")` with `c"new_locked_int"`

In other words: yep, I'm fine with however this is taken.

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

This would be ideal, yep.

>
> [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