Message-ID: <aFBbTMMTHrgHwVFJ@Mac.home>
Date: Mon, 16 Jun 2025 10:58:36 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: onur-ozkan <work@...rozkan.dev>
Cc: linux-kernel@...r.kernel.org, thatslyude@...il.com,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <lossin@...nel.org>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Danilo Krummrich <dakr@...nel.org>, rust-for-linux@...r.kernel.org,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...nel.org>, Will Deacon <will@...nel.org>,
	Waiman Long <longman@...hat.com>
Subject: Re: [PATCH] Add `ww_mutex` support & abstraction for Rust tree

Hi,

Thanks for the patch.

[Add Rust and Locking]

Could you please cc more people and lists for the next version? You
can find that information via `scripts/get_maintainer.pl` or the
"LOCKING PRIMITIVES" and "RUST" sections in MAINTAINERS file in the
kernel source.

Some comments from a quick look:

On Mon, Jun 16, 2025 at 07:24:48PM +0300, onur-ozkan wrote:
[...]
> +/// Implementation of C side `ww_class`.
> +///
> +/// Represents a group of mutexes that can participate in deadlock avoidance together.
> +/// All mutexes that might be acquired together should use the same class.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// use kernel::sync::lock::ww_mutex::WwClass;
> +/// use kernel::c_str;
> +///
> +/// let _wait_die_class = unsafe { WwClass::new(c_str!("graphics_buffers"), true) };
> +/// let _wound_wait_class = unsafe { WwClass::new(c_str!("memory_pools"), false) };
> +///
> +/// # Ok::<(), Error>(())
> +/// ```
> +#[repr(transparent)]
> +pub struct WwClass(Opaque<bindings::ww_class>);
> +
> +// SAFETY: `WwClass` can be shared between threads.
> +unsafe impl Sync for WwClass {}
> +
> +impl WwClass {
> +    /// Creates `WwClass` that wraps C side `ww_class`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that the returned `WwClass` is not moved or freed

You can make WwClass #[pin_data] & !Unpin as well.

> +    /// while any `WwMutex` instances using this class exist.
> +    pub unsafe fn new(name: &'static CStr, is_wait_die: bool) -> Self {
> +        Self(Opaque::new(bindings::ww_class {
> +            stamp: bindings::atomic_long_t { counter: 0 },
> +            acquire_name: name.as_char_ptr(),
> +            mutex_name: name.as_char_ptr(),
> +            is_wait_die: is_wait_die as u32,
> +
> +            // `lock_class_key` doesn't have any value
> +            acquire_key: bindings::lock_class_key {},
> +            mutex_key: bindings::lock_class_key {},
> +        }))
> +    }
> +}
> +
> +/// Implementation of C side `ww_acquire_ctx`.
> +///
> +/// An acquire context is used to group multiple mutex acquisitions together
> +/// for deadlock avoidance. It must be used when acquiring multiple mutexes
> +/// of the same class.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// use kernel::sync::lock::ww_mutex::{WwClass, WwAcquireCtx, WwMutex};
> +/// use kernel::alloc::KBox;
> +/// use kernel::c_str;
> +///
> +/// let class = unsafe { WwClass::new(c_str!("my_class"), false) };
> +///
> +/// // Create mutexes
> +/// let mutex1 = unsafe { KBox::pin_init(WwMutex::new(1u32, &class), GFP_KERNEL).unwrap() };
> +/// let mutex2 = unsafe { KBox::pin_init(WwMutex::new(2u32, &class), GFP_KERNEL).unwrap() };
> +///
> +/// // Create acquire context for deadlock avoidance
> +/// let mut ctx = KBox::pin_init(
> +///     unsafe { WwAcquireCtx::new(&class) },
> +///     GFP_KERNEL
> +/// ).unwrap();
> +///
> +/// // Acquire multiple locks safely
> +/// let guard1 = mutex1.as_ref().lock(Some(&ctx)).unwrap();
> +/// let guard2 = mutex2.as_ref().lock(Some(&ctx)).unwrap();
> +///
> +/// // Mark acquisition phase as complete
> +/// ctx.as_mut().done();
> +///
> +/// # Ok::<(), Error>(())
> +/// ```
> +#[pin_data(PinnedDrop)]
> +pub struct WwAcquireCtx {
> +    #[pin]
> +    inner: Opaque<bindings::ww_acquire_ctx>,
> +    #[pin]
> +    _pin: PhantomPinned,
> +}
> +
> +// SAFETY: `WwAcquireCtx` is safe to send between threads when not in use.
> +unsafe impl Send for WwAcquireCtx {}
> +
> +impl WwAcquireCtx {
> +    /// Initializes `Self` with calling C side `ww_acquire_init` inside.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that the `ww_class` remains valid for the lifetime
> +    /// of this context.

You can make the type system check this for you by:

    pub struct WwAcquireCtx<'a> {
        #[pin]
        inner: Opaque<bindings::ww_acquire_ctx>,
        #[pin]
        _pin: PhantomPinned,
        _p: PhantomData<&'a WwClass>
    }

and

    impl<'ctx> WwAcquireCtx<'ctx> {
        pub fn new<'class: 'ctx>(ww_class: &'class WwClass) -> impl PinInit<Self> {
	    ...
	}
    }

the lifetime of the reference on WwClass should outlive the lifetime of
WwAcquireCtx.

Similar for WwMutex. BUT all the existing ww_classes are static,
alternatively, you can make Ww{AcquireCtx,Mutex}::new() take a `&'static
WwClass`.

Regards,
Boqun

> +    pub unsafe fn new(ww_class: &WwClass) -> impl PinInit<Self> {
> +        let raw_ptr = ww_class.0.get();
> +
> +        pin_init!(WwAcquireCtx {
> +            inner <- Opaque::ffi_init(|slot: *mut bindings::ww_acquire_ctx| {
> +                // SAFETY: The caller guarantees that `ww_class` remains valid.
> +                unsafe {
> +                    bindings::ww_acquire_init(slot, raw_ptr)
> +                }
> +            }),
> +            _pin: PhantomPinned,
> +        })
> +    }
> +
[...]

Powered by blists - more mailing lists