Message-ID: <10453342-d269-4b78-8962-821ef53d3cb5@proton.me>
Date: Thu, 29 Aug 2024 18:16:55 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Alice Ryhl <aliceryhl@...gle.com>, Miguel Ojeda <ojeda@...nel.org>
Cc: 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 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?

> 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.

> 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.

> 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`?

> 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 

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

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

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

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

> +        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`?

---
Cheers,
Benno

> +        self: Pin<&Self>,
> +        name: &'static CStr,
> +        key: &'static LockClassKey,
> +    ) {
> +        // SAFETY: The pointer to `state` is valid for the duration of this call, and both `name`
> +        // and `key` are valid indefinitely.
> +        unsafe { B::init(self.state.get(), name.as_char_ptr(), key.as_ptr()) }
> +    }
>  }
> 
>  impl<T: ?Sized, B: Backend> Lock<T, B> {
> 
> ---
> base-commit: b204bbc53f958fc3119d63bf2cda5a526e7267a4
> change-id: 20240826-static-mutex-a4b228e0e6aa
> 
> Best regards,
> --
> Alice Ryhl <aliceryhl@...gle.com>
> 

Powered by blists - more mailing lists