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

Alice Ryhl
2024-10-10T10:53:15Z

On Thu, Oct 10, 2024 at 12:39 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 30.09.24 15:11, Alice Ryhl wrote:
> > 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.
> > +/// * `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 {}
> > +/// # }
> > +/// ```
>
> The docs here don't mention that you still need to call `.init()`
> manually (though the examples show it nicely). I don't know if we want
> macros to have a `# Safety` section.
>
> > +#[macro_export]
> > +macro_rules! global_lock {
> > + {
> > + $(#[$meta:meta])* $pub:vis static $name:ident: $kind:ident<$valuety:ty> = unsafe { uninit };
> > + value: $value:expr;
>
> I would find it more natural to use `=` instead of `:` here, since then
> it would read as a normal statement with the semicolon at the end.
> Another alternative would be to use `,` instead of `;`, but that doesn't
> work nicely with the static keyword above (although you could make the
> user write it in another {}, but that also isn't ideal...).
>
> Using `=` instead of `:` makes my editor put the correct amount of
> indentation there, `:` adds a lot of extra spaces.

That seems sensible.

> > + 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;
>
> Why are these two items outside of the `mod` below?
> Also why do you need to define the type alias? You could just use
> `$valuety`, right?

Because they might access things that are in scope here, but not in
scope inside the module.

> Also,
>
> error: type `__static_lock_ty_VALUE` should have an upper camel case name
> --> rust/kernel/sync/lock/global.rs:100:18
> |
> 100 | type [< __static_lock_ty_ $name >] = $valuety;
> | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: convert the identifier to upper camel case: `StaticLockTyValue`
>
> The same error affects the `wrapper` type forwarding below.
>
>
> > +
> > + #[allow(unused_pub)]
>
> error: unknown lint: `unused_pub`
> --> rust/kernel/sync/lock/global.rs:103:21
> |
> 103 | #[allow(unused_pub)]
> | ^^^^^^^^^^ help: did you mean: `unused_mut`

Uhhh. This is the lint for when you mark a function pub but don't
actually export it from the crate. But now I can't find the lint
anywhere ... I'm so confused.

> Though I also get
>
> error: methods `init` and `lock` are never used
> --> rust/kernel/sync/lock/global.rs:128:42
> |
> 122 | / impl $wrapper {
> 123 | | /// Initialize the global lock.
> 124 | | ///
> 125 | | /// # Safety
> ... |
> 128 | | pub(crate) unsafe fn init(&'static self) {
> | | ^^^^
> ... |
> 142 | | pub(crate) fn lock(&'static self) -> $crate::global_lock_inner!(guard $kind, $valuety $(, $guard)?) {
> | | ^^^^
> ... |
> 146 | | }
> 147 | | }
> | |_________________- methods in this implementation
>
> But that is governed by the `dead_code` lint.

I guess I have to look into the lints again. I did not get this lint.

> > + 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)?);
>
> `GuardTyp` is only used once, so you should be able to just inline it.
> `Backend` is used twice, but I don't know if we need a type alias for
> it.

They're both used twice. Inlining them makes the lines really long.

> > +
> > + /// # Safety
> > + ///
> > + /// Must be used to initialize `super::$name`.
> > + pub(super) const unsafe fn new() -> $wrapper {
>
> Why is this function not associated to `$wrapper`?
>
> > + let state = $crate::types::Opaque::uninit();
>
> Why not add
>
> const INIT: $valuety = $value;
>
> here instead of outside the mod.

Because it might reference things that are only in scope outside of the module.

> > + $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 {
>
> How can the wrapper struct be `pub(crate)` when the constant might be
> global `pub`?
>
> error: type `__static_lock_wrapper_INIT` is more private than the item `INIT`
> --> rust/kernel/sync/lock/global.rs:206:14
> |
> 206 | };
> | ^ static `INIT` is reachable at visibility `pub`
> |
>
> The functions should probably just be `pub`.

I used to do that, but got some errors about `pub` being unused. I'll
look into this again.

Alice