Message-ID: <20250621133237.5ccdfa3a@nimda.home>
Date: Sat, 21 Jun 2025 13:32:37 +0300
From: Onur <work@...rozkan.dev>
To: "Benno Lossin" <lossin@...nel.org>
Cc: <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <peterz@...radead.org>, <mingo@...hat.com>, <will@...nel.org>,
 <boqun.feng@...il.com>, <longman@...hat.com>, <ojeda@...nel.org>,
 <alex.gaynor@...il.com>, <gary@...yguo.net>, <bjorn3_gh@...tonmail.com>,
 <a.hindborg@...nel.org>, <aliceryhl@...gle.com>, <tmgross@...ch.edu>,
 <dakr@...nel.org>, <thatslyude@...il.com>
Subject: Re: [PATCH V3] implement `ww_mutex` abstraction for the Rust tree

On Thu, 19 Jun 2025 16:42:15 +0200
"Benno Lossin" <lossin@...nel.org> wrote:

> On Thu Jun 19, 2025 at 4:06 PM CEST, Onur Özkan wrote:
> > From: onur-ozkan <work@...rozkan.dev>
> 
> Can you double-check your name in your git config? This doesn't match
> the Signed-off-by below.

That's strange. It should be "Onur Özkan", gitconfig is the correct
one. I will re-check that on V4 patch.

> > <work@...rozkan.dev> ---
> >  rust/helpers/helpers.c            |   1 +
> >  rust/helpers/ww_mutex.c           |  39 +++
> >  rust/kernel/error.rs              |   1 +
> >  rust/kernel/sync/lock.rs          |   1 +
> >  rust/kernel/sync/lock/ww_mutex.rs | 556
> > ++++++++++++++++++++++++++++++ 5 files changed, 598 insertions(+)
> >  create mode 100644 rust/helpers/ww_mutex.c
> >  create mode 100644 rust/kernel/sync/lock/ww_mutex.rs
> 
> Can you split this patch into multiple smaller ones? For example all
> the tests can be done separately as well as the abstractions for
> `ww_class`, `ww_acquire_ctx` and `ww_mutex`.
> 
> Thanks.

I will try to separate them. It's my first big (relatively) patch-based
work. I am still tryin to get used to it :)


> > +/// ```
> > +/// use kernel::c_str;
> > +/// use kernel::define_ww_class;
> > +///
> > +/// define_ww_class!(WOUND_WAIT_GLOBAL_CLASS, wound_wait,
> > c_str!("wound_wait_global_class")); +///
> > define_ww_class!(WAIT_DIE_GLOBAL_CLASS, wait_die,
> > c_str!("wait_die_global_class")); +/// ``` +#[macro_export]
> > +macro_rules! define_ww_class {
> 
> What's the reason for this being a macro?

It's for creating global classes which was suggested in previous
reviews. A similar approach is used on the C side as well with
`DEFINE_WD_CLASS`.

> > +    ($name:ident, wait_die, $class_name:expr) => {
> > +        static $name: $crate::sync::lock::ww_mutex::WwClass = {
> > +            $crate::sync::lock::ww_mutex::WwClass {
> > +                inner:
> > $crate::types::Opaque::new($crate::bindings::ww_class {
> > +                    stamp: $crate::bindings::atomic_long_t {
> > counter: 0 },
> > +                    acquire_name: $class_name.as_char_ptr(),
> > +                    mutex_name: $class_name.as_char_ptr(),
> > +                    is_wait_die: 1,
> > +                    // TODO: Replace with
> > `bindings::lock_class_key::default()` once stabilized for `const`.
> > +                    //
> > +                    // SAFETY: This is always zero-initialized
> > when defined with `DEFINE_WD_CLASS`
> > +                    // globally on C side.
> > +                    //
> > +                    // Ref:
> > https://github.com/torvalds/linux/blob/master/include/linux/ww_mutex.h#L85-L89
> > +                    acquire_key: unsafe { core::mem::zeroed() },
> > +                    // TODO: Replace with
> > `bindings::lock_class_key::default()` once stabilized for `const`.
> > +                    //
> > +                    // SAFETY: This is always zero-initialized
> > when defined with `DEFINE_WD_CLASS`
> > +                    // globally on C side.
> > +                    //
> > +                    // Ref:
> > https://github.com/torvalds/linux/blob/master/include/linux/ww_mutex.h#L85-L89
> > +                    mutex_key: unsafe { core::mem::zeroed() },
> > +                }),
> > +            }
> > +        };
> > +    };
> > +}
> > +
> > +/// Implementation of C side `ww_class`.
> 
> This isn't informative at all. The names already match, so I wouldn't
> have thought otherwise.

I didn't want to duplicate the docs. I will update it (and others) on
V4.

> > +///
> > +/// 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;
> > +/// use pin_init::stack_pin_init;
> > +///
> > +/// stack_pin_init!(let _wait_die_class =
> > WwClass::new_wait_die(c_str!("graphics_buffers"))); +///
> > stack_pin_init!(let _wound_wait_class =
> > WwClass::new_wound_wait(c_str!("memory_pools"))); +/// +/// #
> > Ok::<(), Error>(()) +/// ```
> > +#[pin_data]
> > +pub struct WwClass {
> > +    /// Wrapper of the underlying C `ww_class`.
> > +    ///
> > +    /// You should not construct this type manually. Use the
> > `define_ww_class` macro
> > +    /// or call `WwClass::new_wait_die` or
> > `WwClass::new_wound_wait` instead.
> > +    #[pin]
> > +    pub inner: Opaque<bindings::ww_class>,
> 
> Why `pub`? Abstractions normally don't expose `Opaque` wrappers for
> bindings. Especially because this type is marked `#[pin_data]` this
> seems wrong, because this would allow people to construct it in a
> non-pinned state & also non-initialized state.

It was for `define_ww_class` macro. It obviously says you shouldn't do
that but sure, I can undo the `pub` and create a `const` function for
`define_ww_class`.


Regards,
Onur

Powered by blists - more mailing lists