[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAH5fLgh-DYvXobXQVaQ9txYS4Rx8QhjyVvfTphk6vvnUOGzPnw@mail.gmail.com>
Date: Fri, 30 Aug 2024 07:34:00 +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 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
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
> > 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
CONSTRUCTORS, then I think it's an undesirable solution. C code can
define static mutexes without load-time initialization hooks. We
should be able to do the same.
> > 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.
> > 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.
> > 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?
> > + /// 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."
> > + /// ```
> > + /// 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.
> > + /// fn module_init() {
> > + /// // SAFETY:
> > + /// // * `MY_MUTEX` was created using `unsafe_const_new`.
> > + /// // * This call is in the module initializer, which doesn't runs more than once.
> > + /// unsafe {
> > + /// core::pin::Pin::static_ref(&MY_MUTEX)
>
> I would put this into a let binding, that way the formatting will also
> be nicer.
Ok.
> > + /// .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?
> > + Self {
> > + data: UnsafeCell::new(t),
> > + state: Opaque::uninit(),
> > + _pin: PhantomPinned,
> > + }
> > + }
> > +
> > + /// Initialize a global lock.
> > + ///
> > + /// When using this to initialize a `static` lock, you can use [`Pin::static_ref`] to construct
> > + /// the pinned reference.
> > + ///
> > + /// See the docs for [`unsafe_const_new`] for examples.
> > + ///
> > + /// # Safety
> > + ///
> > + /// * This lock must have been created with [`unsafe_const_new`].
> > + /// * This method must not be called more than once on a given lock.
> > + ///
> > + /// [`unsafe_const_new`]: Self::unsafe_const_new
> > + pub unsafe fn unsafe_const_init(
>
> I know you are using `const` here to have symmetry with the function
> above, but I think it's a bit misleading, you can't call this from const
> context. Going with the theme of the suggestion from above, what about
> `manual_init`?
That could work.
Alice
Powered by blists - more mailing lists