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

[Benno Lossin]

On 10.10.24 12:53, Alice Ryhl wrote:
> On Thu, Oct 10, 2024 at 12:39 PM Benno Lossin <benno.lossin@xxxxxxxxx> 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.

Right... That's rather annoying...

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

Maybe you mean `unreachable_pub`?

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

I just put a `global_lock!` invocation into `lib.rs` and didn't use any
of the functions. But maybe we want that to error?

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

Ah I missed the one on try_lock. It's fine to keep them.

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

Maybe the `unreachable_pub` lint can help?

---
Cheers,
Benno