Message-ID: <aIDn8OFzUbVXQVTF@tardis.local>
Date: Wed, 23 Jul 2025 06:49:13 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: 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, Tamir Duberstein <tamird@...il.com>
Subject: Re: [PATCH 2/2] rust: sync: clean up LockClassKey and its docs

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

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