Message-ID: <CAH5fLggE-fWDuZXH_F3ixDSo7sQEFwnUV3cd+9_rpFH+XmnA2Q@mail.gmail.com>
Date: Mon, 2 Sep 2024 13:37:48 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Wedson Almeida Filho <wedsonaf@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Andreas Hindborg <a.hindborg@...sung.com>, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] rust: add global lock support

On Fri, Aug 30, 2024 at 3:22 PM Benno Lossin <benno.lossin@...ton.me> wrote:
>
> On 30.08.24 07:34, Alice Ryhl wrote:
> > On Thu, Aug 29, 2024 at 8:17 PM Benno Lossin <benno.lossin@...ton.me> wrote:
> >>
> >> On 27.08.24 10:41, Alice Ryhl wrote:
> >>> We don't currently have any support for global locks in Rust, however
> >>> they are very useful and I have needed to work around this limitation
> >>> several times. My workarounds generally involve initializing the mutex
> >>> in the module's init function, and this workaround is reflected here.
> >>
> >> I would not exactly call this a "workaround". If your use-case is
> >> covered by putting a `Mutex`, then I would prefer it. Or did you need
> >> additional ugly code to access it?
> >
> > Not sure what you mean by "putting a Mutex" but the workaround is
>
> Oh sorry, seems like I forgot to write the rest of that... Let me try
> again: If your use-case is covered by putting a `Mutex` inside of the
> type that implements `Module`, then I think you should do that instead
> of using a global. (you need the inplace module patch for that)

I don't think it's possible to access the `Module` struct after `init`
returns? Even with in-place init.

> > really gross and involves defining a whole struct to make types Sync
> > and so on. Unlike binder, this patch has access to private fields of
> > Lock, so it can do it in a more nice way. You can find it in the
> > Binder RFC if you search for "context_global".
> > https://lore.kernel.org/rust-for-linux/20231101-rust-binder-v1-2-08ba9197f637@google.com/#Z31drivers:android:context.rs
>
> Oh I remember this... Yeah I agree that is ugly, but it is not the
> workaround that I imagined when you wrote "initializing the mutex in the
> module's init function". There I was thinking of what I wrote above.
>
> This might just be me misunderstanding that, but if you want to improve
> it, then you could mention that the mutex is still a static.
>
> >>> Due to the initialization requirement, constructing a global mutex is
> >>> unsafe with the current approach. In the future, it would be really nice
> >>> to support global mutexes that don't need to be initialized, which would
> >>> make them safe. Unfortunately, this is not possible today because
> >>> bindgen refuses to expose __ARCH_SPIN_LOCK_UNLOCKED to Rust as a
> >>> compile-time constant. It just generates an `extern "C"` global
> >>> reference instead.
> >>
> >> Ideally, we would have support for static initialization in pinned-init.
> >
> > I don't think traits work with const today, so pin-init would need an
> > entirely different mechanism? If you're talking about using
>
> Oh yeah I forgot that that got scratched some time ago.
>
> > CONSTRUCTORS, then I think it's an undesirable solution. C code can
>
> No, I was thinking that the initializer is run at const eval and then
> the result is put into the binary.
>
> > define static mutexes without load-time initialization hooks. We
> > should be able to do the same.
>
> Agreed.
>
> >>> On most architectures, we could initialize the lock to just contain all
> >>> zeros. A possible improvement would be to create a Kconfig constant
> >>> that is set whenever the current architecture uses all zeros for the
> >>> initializer and have `unsafe_const_init` be a no-op on those
> >>> architectures. We could also provide a safe const initializer that is
> >>> only available when that Kconfig option is set.
> >>
> >> I am not sure if the two branches (depending on the config) will be a
> >> good idea. We don't save on `unsafe` and only increase code complexity.
> >> The no-op sounds like a better idea to me.
> >
> > You mean put the logic here instead in the downstream user? I agree.
>
> I meant that
>
>     #[cfg(ZERO_LOCK_INIT)]
>     static MY_MUTEX: Mutex<()> = Mutex::new_zeroed();
>
>     #[cfg(not(ZERO_LOCK_INIT))]
>     // SAFETY: ...
>     static MY_MUTEX: Mutex<()> = unsafe { Mutex::unsafe_const_new() };
>
>
>     module_init() {
>         #[cfg(not(ZERO_LOCK_INIT))]
>         {
>             // SAFETY: ...
>             unsafe { MY_MUTEX.unsafe_const_init() };
>         }
>     }
>
> is significantly worse compared to just
>
>     // SAFETY: ...
>     static MY_MUTEX: Mutex<()> = unsafe { Mutex::unsafe_const_new() };
>
>
>     module_init() {
>         // SAFETY: ...
>         unsafe { MY_MUTEX.unsafe_const_init() };
>         //                ^^^^^^^^^^^^^^^^^
>         //                if ZERO_LOCK_INIT, then this is a no-op.
>     }

Agree.

> >>> For architectures that don't use all-zeros for the unlocked case, we
> >>> will most likely have to hard-code the correct representation on the
> >>> Rust side.
> >>
> >> You mean in `unsafe_const_init`?
> >
> > No, I mean we would have `unsafe_const_new` directly set `state` to
> > the right value and let `unsafe_const_init` be a no-op.
>
> But how do you set the right value of a list_head? The value will be
> moved.

Right ... we probably can't get around needing a macro. Can statics
even reference themselves?

> >>> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> >>> ---
> >>> Changes in v2:
> >>> - Require `self: Pin<&Self>` and recommend `Pin::static_ref`.
> >>> - Other doc improvements including new example.
> >>> - Link to v1: https://lore.kernel.org/r/20240826-static-mutex-v1-1-a14ee71561f3@google.com
> >>> ---
> >>>  rust/kernel/sync/lock.rs | 64 +++++++++++++++++++++++++++++++++++++++++++++++-
> >>>  1 file changed, 63 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> >>> index f6c34ca4d819..cfc5e160d78c 100644
> >>> --- a/rust/kernel/sync/lock.rs
> >>> +++ b/rust/kernel/sync/lock.rs
> >>> @@ -7,7 +7,7 @@
> >>>
> >>>  use super::LockClassKey;
> >>>  use crate::{init::PinInit, pin_init, str::CStr, types::Opaque, types::ScopeGuard};
> >>> -use core::{cell::UnsafeCell, marker::PhantomData, marker::PhantomPinned};
> >>> +use core::{cell::UnsafeCell, marker::PhantomData, marker::PhantomPinned, pin::Pin};
> >>>  use macros::pin_data;
> >>>
> >>>  pub mod mutex;
> >>> @@ -117,6 +117,68 @@ pub fn new(t: T, name: &'static CStr, key: &'static LockClassKey) -> impl PinIni
> >>>              }),
> >>>          })
> >>>      }
> >>> +
> >>> +    /// Create a global lock that has not yet been initialized.
> >>> +    ///
> >>
> >> Could you add a paragraph that explains that
> >
> > Explains that what?
>
> ... this is not the usual way to create a `Lock`, use this only when
> creating a global, `static` lock. For all other purposes, use
> `new_<lock-type>`.

Ok.

> >>> +    /// Since global locks is not yet fully supported, this method implements global locks by
> >>> +    /// requiring you to initialize them before you start using it. Usually this is best done in
> >>> +    /// the module's init function.
> >>> +    ///
> >>> +    /// # Examples
> >>> +    ///
> >>
> >> I would preface this example with "Instead of [`Mutex<T>`], you can use
> >> any other lock.".
> >
> > I don't love that wording. How about something along these lines?
> > "This example uses a Mutex, but this works with any other lock
> > including spin locks."
>
> Sure.
>
> >>> +    /// ```
> >>> +    /// use kernel::sync::Mutex;
> >>> +    ///
> >>> +    /// // SAFETY: We initialize the mutex before first use.
> >>> +    /// static MY_MUTEX: Mutex<()> = unsafe { Mutex::unsafe_const_new(()) };
> >>> +    ///
> >>> +    /// // For the sake of this example, assume that this is the module initializer.
> >>
> >> Why not actually provide a module initializer?
> >
> > Can I put a `module!` macro inside a kunit test? I assumed that I couldn't.
>
> I think if you wrap it in another `mod`, then it should work, but I
> might be wrong.

I guess I can implement the Module trait without using the module! macro.

> >>> +    ///             .unsafe_const_init(kernel::c_str!("MY_MUTEX"), kernel::static_lock_class!())
> >>> +    ///     };
> >>> +    /// }
> >>> +    /// ```
> >>> +    ///
> >>> +    /// # Safety
> >>> +    ///
> >>> +    /// You must call [`unsafe_const_init`] before calling any other method on this lock.
> >>> +    ///
> >>> +    /// [`unsafe_const_init`]: Self::unsafe_const_init
> >>> +    pub const unsafe fn unsafe_const_new(t: T) -> Self {
> >>
> >> I am not sure on this name, I don't think we have any functions with
> >> `unsafe` in it (and `std` also doesn't). How about `new_uninitialized`?
> >>
> >> Although that might be confusing, since it does actually take a value...
> >
> > Hmm. Any other ideas? const_new_need_manual_init?
>
> Hmm that seems too long... `new_static_uninit`? I don't think that
> `const` belongs in the name either, since you wouldn't use it in a
> `const` (but sure it is used in const contexts, but I find putting it in
> the name confusing).

Works for me.

Alice

Powered by blists - more mailing lists