Re: [PATCH v4] rust: add global lock support

[Andreas Hindborg]

Hi Alice,

Alice Ryhl <aliceryhl@xxxxxxxxxx> writes:

> Add support for creating global variables that are wrapped in a mutex or
> spinlock. Optionally, the macro can generate a special LockedBy type
> that does not require a runtime check.
>
> The implementation here is intended to replace the global mutex
> workaround found in the Rust Binder RFC [1]. In both cases, the global
> lock must be initialized before first use. The macro is unsafe to use
> for the same reason.
>
> The separate initialization step is required because it is tricky to
> access the value of __ARCH_SPIN_LOCK_UNLOCKED from Rust. Doing so will
> require changes to the C side. That change will happen as a follow-up to
> this patch.

Why is this a challenge? It seems to work with locks that are not
global.

[...]

> diff --git a/rust/kernel/sync/lock/global.rs b/rust/kernel/sync/lock/global.rs
> new file mode 100644
> index 000000000000..fc02fac864f6
> --- /dev/null
> +++ b/rust/kernel/sync/lock/global.rs
> @@ -0,0 +1,260 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2024 Google LLC.
> +
> +//! Support for defining statics containing locks.
> +
> +/// Defines a global lock.
> +///
> +/// Supports the following options:
> +///
> +/// * `value` specifies the initial value in the global lock.
> +/// * `wrapper` specifies the name of the wrapper struct.

Could you add an example to demonstrate when using `wrapper` option
would be useful?

> +/// * `guard` specifies the name of the guard type.
> +/// * `locked_by` specifies the name of the `LockedBy` type.
> +///
> +/// # Examples
> +///
> +/// A global counter.
> +///
> +/// ```
> +/// # mod ex {
> +/// # use kernel::prelude::*;
> +/// kernel::sync::global_lock! {
> +///     // SAFETY: Initialized in module initializer before first use.
> +///     static MY_COUNTER: Mutex<u32> = unsafe { uninit };
> +///     value: 0;
> +/// }
> +///
> +/// fn increment_counter() -> u32 {
> +///     let mut guard = MY_COUNTER.lock();
> +///     *guard += 1;
> +///     *guard
> +/// }
> +///
> +/// impl kernel::Module for MyModule {
> +///     fn init(_module: &'static ThisModule) -> Result<Self> {
> +///         // SAFETY: called exactly once
> +///         unsafe { MY_COUNTER.init() };
> +///
> +///         Ok(MyModule {})
> +///     }
> +/// }
> +/// # struct MyModule {}
> +/// # }
> +/// ```
> +///
> +/// A global mutex used to protect all instances of a given struct.
> +///
> +/// ```
> +/// # mod ex {
> +/// # use kernel::prelude::*;
> +/// kernel::sync::global_lock! {
> +///     // SAFETY: Initialized in module initializer before first use.
> +///     static MY_MUTEX: Mutex<()> = unsafe { uninit };
> +///     value: ();
> +///     guard: MyGuard;
> +///     locked_by: LockedByMyMutex;
> +/// }
> +///
> +/// /// All instances of this struct are protected by `MY_MUTEX`.
> +/// struct MyStruct {
> +///     my_counter: LockedByMyMutex<u32>,
> +/// }
> +///
> +/// impl MyStruct {
> +///     /// Increment the counter in this instance.
> +///     ///
> +///     /// The caller must hold the `MY_MUTEX` mutex.
> +///     fn increment(&self, guard: &mut MyGuard) -> u32 {
> +///         let my_counter = self.my_counter.as_mut(guard);
> +///         *my_counter += 1;
> +///         *my_counter
> +///     }
> +/// }
> +///
> +/// impl kernel::Module for MyModule {
> +///     fn init(_module: &'static ThisModule) -> Result<Self> {
> +///         // SAFETY: called exactly once
> +///         unsafe { MY_MUTEX.init() };
> +///
> +///         Ok(MyModule {})
> +///     }
> +/// }
> +/// # struct MyModule {}
> +/// # }
> +/// ```
> +#[macro_export]
> +macro_rules! global_lock {
> +    {
> +        $(#[$meta:meta])* $pub:vis static $name:ident: $kind:ident<$valuety:ty> = unsafe { uninit };
> +        value: $value:expr;
> +        wrapper: $wrapper:ident;
> +        $( name: $lname:literal; )?
> +        $(
> +            guard: $guard:ident;
> +            locked_by: $locked_by:ident;
> +        )?
> +    } => {
> +        $crate::macros::paste! {
> +            type [< __static_lock_ty_ $name >] = $valuety;
> +            const [< __static_lock_init_ $name >]: [< __static_lock_ty_ $name >] = $value;
> +
> +            #[allow(unused_pub)]
> +            mod [< __static_lock_mod_ $name >] {
> +                use super::[< __static_lock_ty_ $name >] as Val;
> +                use super::[< __static_lock_init_ $name >] as INIT;
> +                type Backend = $crate::global_lock_inner!(backend $kind);
> +                type GuardTyp = $crate::global_lock_inner!(guard $kind, Val $(, $guard)?);
> +
> +                /// # Safety
> +                ///
> +                /// Must be used to initialize `super::$name`.
> +                pub(super) const unsafe fn new() -> $wrapper {
> +                    let state = $crate::types::Opaque::uninit();
> +                    $wrapper {
> +                        // SAFETY: The user of this macro promises to call `init` before calling
> +                        // `lock`.
> +                        inner: unsafe {
> +                            $crate::sync::lock::Lock::global_lock_helper_new(state, INIT)
> +                        }
> +                    }
> +                }
> +
> +                /// Wrapper type for a global lock.
> +                pub(crate) struct $wrapper {
> +                    inner: $crate::sync::lock::Lock<Val, Backend>,
> +                }
> +
> +                impl $wrapper {
> +                    /// Initialize the global lock.
> +                    ///
> +                    /// # Safety
> +                    ///
> +                    /// This method must not be called more than once.
> +                    pub(crate) unsafe fn init(&'static self) {
> +                        // SAFETY:
> +                        // * This type can only be created by `new`.
> +                        // * Caller promises to not call this method more than once.
> +                        unsafe {
> +                            $crate::sync::lock::Lock::global_lock_helper_init(
> +                                ::core::pin::Pin::static_ref(&self.inner),
> +                                $crate::optional_name!($($lname)?),
> +                                $crate::static_lock_class!(),
> +                            );
> +                        }
> +                    }
> +
> +                    /// Lock this global lock.
> +                    pub(crate) fn lock(&'static self) -> GuardTyp {
> +                        $crate::global_lock_inner!(new_guard $($guard)? {
> +                            self.inner.lock()
> +                        })
> +                    }
> +
> +                    /// Lock this global lock.

"Try to lock..." ?

Best regards,
Andreas