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

Benno Lossin
2024-10-10T10:39:35Z

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.

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

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`

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.

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

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

> + $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`.

---
Cheers,
Benno

> + inner: $crate::sync::lock::Lock<Val, Backend>,
> + }
> +
> +