[PATCH v2] rust: add global lock support

[PATCH v2] rust: add global lock support

[Alice Ryhl]

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.

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.

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.

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.

Signed-off-by: Alice Ryhl <aliceryhl@google.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.
+    ///
+    /// 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
+    ///
+    /// ```
+    /// 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.
+    /// 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)
+    ///             .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 {
+        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(
+        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@google.com>

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

[Benno Lossin]

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@google.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@google.com>
> 

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

[Alice Ryhl]

On Thu, Aug 29, 2024 at 8:17 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> 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?

Not sure what you mean by "putting a Mutex" but the workaround is
really gross and involves defining a whole struct to make types Sync
and so on. Unlike binder, this patch has access to private fields of
Lock, so it can do it in a more nice way. You can find it in the
Binder RFC if you search for "context_global".
https://lore.kernel.org/rust-for-linux/20231101-rust-binder-v1-2-08ba9197f637@google.com/#Z31drivers:android:context.rs

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

I don't think traits work with const today, so pin-init would need an
entirely different mechanism? If you're talking about using
CONSTRUCTORS, then I think it's an undesirable solution. C code can
define static mutexes without load-time initialization hooks. We
should be able to do the same.

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

You mean put the logic here instead in the downstream user? I agree.

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

No, I mean we would have `unsafe_const_new` directly set `state` to
the right value and let `unsafe_const_init` be a no-op.

> > Signed-off-by: Alice Ryhl <aliceryhl@google.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

Explains that what?

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

I don't love that wording. How about something along these lines?
"This example uses a Mutex, but this works with any other lock
including spin locks."

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

Can I put a `module!` macro inside a kunit test? I assumed that I couldn't.

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

Ok.

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

Hmm. Any other ideas? const_new_need_manual_init?

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

That could work.

Alice

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

[Benno Lossin]

On 30.08.24 07:34, Alice Ryhl wrote:
> On Thu, Aug 29, 2024 at 8:17 PM Benno Lossin <benno.lossin@proton.me> wrote:
>>
>> 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?
> 
> Not sure what you mean by "putting a Mutex" but the workaround is

Oh sorry, seems like I forgot to write the rest of that... Let me try
again: If your use-case is covered by putting a `Mutex` inside of the
type that implements `Module`, then I think you should do that instead
of using a global. (you need the inplace module patch for that)

> really gross and involves defining a whole struct to make types Sync
> and so on. Unlike binder, this patch has access to private fields of
> Lock, so it can do it in a more nice way. You can find it in the
> Binder RFC if you search for "context_global".
> https://lore.kernel.org/rust-for-linux/20231101-rust-binder-v1-2-08ba9197f637@google.com/#Z31drivers:android:context.rs

Oh I remember this... Yeah I agree that is ugly, but it is not the
workaround that I imagined when you wrote "initializing the mutex in the
module's init function". There I was thinking of what I wrote above.

This might just be me misunderstanding that, but if you want to improve
it, then you could mention that the mutex is still a static.

>>> 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.
> 
> I don't think traits work with const today, so pin-init would need an
> entirely different mechanism? If you're talking about using

Oh yeah I forgot that that got scratched some time ago.

> CONSTRUCTORS, then I think it's an undesirable solution. C code can

No, I was thinking that the initializer is run at const eval and then
the result is put into the binary.

> define static mutexes without load-time initialization hooks. We
> should be able to do the same.

Agreed.

>>> 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.
> 
> You mean put the logic here instead in the downstream user? I agree.

I meant that

    #[cfg(ZERO_LOCK_INIT)]
    static MY_MUTEX: Mutex<()> = Mutex::new_zeroed();

    #[cfg(not(ZERO_LOCK_INIT))]
    // SAFETY: ...
    static MY_MUTEX: Mutex<()> = unsafe { Mutex::unsafe_const_new() };


    module_init() {
        #[cfg(not(ZERO_LOCK_INIT))]
        {
            // SAFETY: ...
            unsafe { MY_MUTEX.unsafe_const_init() };
        }
    }

is significantly worse compared to just

    // SAFETY: ...
    static MY_MUTEX: Mutex<()> = unsafe { Mutex::unsafe_const_new() };


    module_init() {
        // SAFETY: ...
        unsafe { MY_MUTEX.unsafe_const_init() };
        //                ^^^^^^^^^^^^^^^^^
        //                if ZERO_LOCK_INIT, then this is a no-op.
    }

>>> 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`?
> 
> No, I mean we would have `unsafe_const_new` directly set `state` to
> the right value and let `unsafe_const_init` be a no-op.

But how do you set the right value of a list_head? The value will be
moved.

>>> Signed-off-by: Alice Ryhl <aliceryhl@google.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
> 
> Explains that what?

... this is not the usual way to create a `Lock`, use this only when
creating a global, `static` lock. For all other purposes, use
`new_<lock-type>`.

>>> +    /// 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.".
> 
> I don't love that wording. How about something along these lines?
> "This example uses a Mutex, but this works with any other lock
> including spin locks."

Sure.

>>> +    /// ```
>>> +    /// 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?
> 
> Can I put a `module!` macro inside a kunit test? I assumed that I couldn't.

I think if you wrap it in another `mod`, then it should work, but I
might be wrong.

>>> +    /// 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.
> 
> Ok.
> 
>>> +    ///             .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...
> 
> Hmm. Any other ideas? const_new_need_manual_init?

Hmm that seems too long... `new_static_uninit`? I don't think that
`const` belongs in the name either, since you wouldn't use it in a
`const` (but sure it is used in const contexts, but I find putting it in
the name confusing).

---
Cheers,
Benno

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

[Alice Ryhl]

On Fri, Aug 30, 2024 at 3:22 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 30.08.24 07:34, Alice Ryhl wrote:
> > On Thu, Aug 29, 2024 at 8:17 PM Benno Lossin <benno.lossin@proton.me> wrote:
> >>
> >> 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?
> >
> > Not sure what you mean by "putting a Mutex" but the workaround is
>
> Oh sorry, seems like I forgot to write the rest of that... Let me try
> again: If your use-case is covered by putting a `Mutex` inside of the
> type that implements `Module`, then I think you should do that instead
> of using a global. (you need the inplace module patch for that)

I don't think it's possible to access the `Module` struct after `init`
returns? Even with in-place init.

> > really gross and involves defining a whole struct to make types Sync
> > and so on. Unlike binder, this patch has access to private fields of
> > Lock, so it can do it in a more nice way. You can find it in the
> > Binder RFC if you search for "context_global".
> > https://lore.kernel.org/rust-for-linux/20231101-rust-binder-v1-2-08ba9197f637@google.com/#Z31drivers:android:context.rs
>
> Oh I remember this... Yeah I agree that is ugly, but it is not the
> workaround that I imagined when you wrote "initializing the mutex in the
> module's init function". There I was thinking of what I wrote above.
>
> This might just be me misunderstanding that, but if you want to improve
> it, then you could mention that the mutex is still a static.
>
> >>> 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.
> >
> > I don't think traits work with const today, so pin-init would need an
> > entirely different mechanism? If you're talking about using
>
> Oh yeah I forgot that that got scratched some time ago.
>
> > CONSTRUCTORS, then I think it's an undesirable solution. C code can
>
> No, I was thinking that the initializer is run at const eval and then
> the result is put into the binary.
>
> > define static mutexes without load-time initialization hooks. We
> > should be able to do the same.
>
> Agreed.
>
> >>> 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.
> >
> > You mean put the logic here instead in the downstream user? I agree.
>
> I meant that
>
>     #[cfg(ZERO_LOCK_INIT)]
>     static MY_MUTEX: Mutex<()> = Mutex::new_zeroed();
>
>     #[cfg(not(ZERO_LOCK_INIT))]
>     // SAFETY: ...
>     static MY_MUTEX: Mutex<()> = unsafe { Mutex::unsafe_const_new() };
>
>
>     module_init() {
>         #[cfg(not(ZERO_LOCK_INIT))]
>         {
>             // SAFETY: ...
>             unsafe { MY_MUTEX.unsafe_const_init() };
>         }
>     }
>
> is significantly worse compared to just
>
>     // SAFETY: ...
>     static MY_MUTEX: Mutex<()> = unsafe { Mutex::unsafe_const_new() };
>
>
>     module_init() {
>         // SAFETY: ...
>         unsafe { MY_MUTEX.unsafe_const_init() };
>         //                ^^^^^^^^^^^^^^^^^
>         //                if ZERO_LOCK_INIT, then this is a no-op.
>     }

Agree.

> >>> 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`?
> >
> > No, I mean we would have `unsafe_const_new` directly set `state` to
> > the right value and let `unsafe_const_init` be a no-op.
>
> But how do you set the right value of a list_head? The value will be
> moved.

Right ... we probably can't get around needing a macro. Can statics
even reference themselves?

> >>> Signed-off-by: Alice Ryhl <aliceryhl@google.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
> >
> > Explains that what?
>
> ... this is not the usual way to create a `Lock`, use this only when
> creating a global, `static` lock. For all other purposes, use
> `new_<lock-type>`.

Ok.

> >>> +    /// 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.".
> >
> > I don't love that wording. How about something along these lines?
> > "This example uses a Mutex, but this works with any other lock
> > including spin locks."
>
> Sure.
>
> >>> +    /// ```
> >>> +    /// 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?
> >
> > Can I put a `module!` macro inside a kunit test? I assumed that I couldn't.
>
> I think if you wrap it in another `mod`, then it should work, but I
> might be wrong.

I guess I can implement the Module trait without using the module! macro.

> >>> +    ///             .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...
> >
> > Hmm. Any other ideas? const_new_need_manual_init?
>
> Hmm that seems too long... `new_static_uninit`? I don't think that
> `const` belongs in the name either, since you wouldn't use it in a
> `const` (but sure it is used in const contexts, but I find putting it in
> the name confusing).

Works for me.

Alice

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

[Alice Ryhl]

On Mon, Sep 2, 2024 at 1:37 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Fri, Aug 30, 2024 at 3:22 PM Benno Lossin <benno.lossin@proton.me> wrote:
> >
> > On 30.08.24 07:34, Alice Ryhl wrote:
> > > On Thu, Aug 29, 2024 at 8:17 PM Benno Lossin <benno.lossin@proton.me> wrote:
> > >>
> > >> On 27.08.24 10:41, Alice Ryhl wrote:
> > >>> 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`?
> > >
> > > No, I mean we would have `unsafe_const_new` directly set `state` to
> > > the right value and let `unsafe_const_init` be a no-op.
> >
> > But how do you set the right value of a list_head? The value will be
> > moved.
>
> Right ... we probably can't get around needing a macro. Can statics
> even reference themselves?

Looks like they can:

use std::ptr::addr_of;

struct MyStruct {
    ptr: *const MyStruct,
}

static mut MY_STRUCT: MyStruct = MyStruct {
    ptr: addr_of!(MY_STRUCT),
};

Alice

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

[Boqun Feng]

On Mon, Sep 02, 2024 at 01:42:53PM +0200, Alice Ryhl wrote:
> On Mon, Sep 2, 2024 at 1:37 PM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > On Fri, Aug 30, 2024 at 3:22 PM Benno Lossin <benno.lossin@proton.me> wrote:
> > >
> > > On 30.08.24 07:34, Alice Ryhl wrote:
> > > > On Thu, Aug 29, 2024 at 8:17 PM Benno Lossin <benno.lossin@proton.me> wrote:
> > > >>
> > > >> On 27.08.24 10:41, Alice Ryhl wrote:
> > > >>> 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`?
> > > >
> > > > No, I mean we would have `unsafe_const_new` directly set `state` to
> > > > the right value and let `unsafe_const_init` be a no-op.
> > >
> > > But how do you set the right value of a list_head? The value will be
> > > moved.
> >
> > Right ... we probably can't get around needing a macro. Can statics
> > even reference themselves?
> 
> Looks like they can:
> 
> use std::ptr::addr_of;
> 
> struct MyStruct {
>     ptr: *const MyStruct,
> }
> 
> static mut MY_STRUCT: MyStruct = MyStruct {
>     ptr: addr_of!(MY_STRUCT),

I'm guessing you're using nightly or new enough rustc, in the current
stable (1.80), this would complain using static mut without unsafe:

	https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=2954daab193caf14d1fb91492dcf325a

, which gets changed recently:

	https://github.com/rust-lang/rust/pull/125834

Regards,
Boqun

> };
> 
> Alice

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

[Alice Ryhl]

On Mon, Sep 2, 2024 at 4:18 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Mon, Sep 02, 2024 at 01:42:53PM +0200, Alice Ryhl wrote:
> > On Mon, Sep 2, 2024 at 1:37 PM Alice Ryhl <aliceryhl@google.com> wrote:
> > >
> > > On Fri, Aug 30, 2024 at 3:22 PM Benno Lossin <benno.lossin@proton.me> wrote:
> > > >
> > > > On 30.08.24 07:34, Alice Ryhl wrote:
> > > > > On Thu, Aug 29, 2024 at 8:17 PM Benno Lossin <benno.lossin@proton.me> wrote:
> > > > >>
> > > > >> On 27.08.24 10:41, Alice Ryhl wrote:
> > > > >>> 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`?
> > > > >
> > > > > No, I mean we would have `unsafe_const_new` directly set `state` to
> > > > > the right value and let `unsafe_const_init` be a no-op.
> > > >
> > > > But how do you set the right value of a list_head? The value will be
> > > > moved.
> > >
> > > Right ... we probably can't get around needing a macro. Can statics
> > > even reference themselves?
> >
> > Looks like they can:
> >
> > use std::ptr::addr_of;
> >
> > struct MyStruct {
> >     ptr: *const MyStruct,
> > }
> >
> > static mut MY_STRUCT: MyStruct = MyStruct {
> >     ptr: addr_of!(MY_STRUCT),
>
> I'm guessing you're using nightly or new enough rustc, in the current
> stable (1.80), this would complain using static mut without unsafe:
>
>         https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=2954daab193caf14d1fb91492dcf325a
>
> , which gets changed recently:
>
>         https://github.com/rust-lang/rust/pull/125834

That's a trivial matter. Adding the unsafe block makes it work on stable.

Alice

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

[Benno Lossin]

On 02.09.24 13:42, Alice Ryhl wrote:
> On Mon, Sep 2, 2024 at 1:37 PM Alice Ryhl <aliceryhl@google.com> wrote:
>>
>> On Fri, Aug 30, 2024 at 3:22 PM Benno Lossin <benno.lossin@proton.me> wrote:
>>>
>>> On 30.08.24 07:34, Alice Ryhl wrote:
>>>> On Thu, Aug 29, 2024 at 8:17 PM Benno Lossin <benno.lossin@proton.me> wrote:
>>>>>
>>>>> On 27.08.24 10:41, Alice Ryhl wrote:
>>>>>> 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`?
>>>>
>>>> No, I mean we would have `unsafe_const_new` directly set `state` to
>>>> the right value and let `unsafe_const_init` be a no-op.
>>>
>>> But how do you set the right value of a list_head? The value will be
>>> moved.
>>
>> Right ... we probably can't get around needing a macro. Can statics
>> even reference themselves?
> 
> Looks like they can:
> 
> use std::ptr::addr_of;
> 
> struct MyStruct {
>     ptr: *const MyStruct,
> }
> 
> static mut MY_STRUCT: MyStruct = MyStruct {
>     ptr: addr_of!(MY_STRUCT),
> };

That's useful to know...
But I don't see a way to get pinned-init to work with this. I would need
a lot of currently experimental features (const closures, const traits)
and a way to initialize a static without providing a direct value, since
I can't just do

    static mut MY_STRUCT: MyStruct = {
        unsafe { __pinned_init(addr_of_mut!(MY_STRUCT), /* initializer */) };
        unsafe { addr_of!(MY_STRUCT).read() }
    };

It (rightfully) complains that I am initializing the static with itself.

We /might/ be able to do something special for `Mutex`/ other locks, but
I haven't tried yet. So the unsafe approach seems the best at the moment.

---
Cheers,
Benno

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

[Alice Ryhl]

On Tue, Sep 3, 2024 at 12:17 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 02.09.24 13:42, Alice Ryhl wrote:
> > On Mon, Sep 2, 2024 at 1:37 PM Alice Ryhl <aliceryhl@google.com> wrote:
> >>
> >> On Fri, Aug 30, 2024 at 3:22 PM Benno Lossin <benno.lossin@proton.me> wrote:
> >>>
> >>> On 30.08.24 07:34, Alice Ryhl wrote:
> >>>> On Thu, Aug 29, 2024 at 8:17 PM Benno Lossin <benno.lossin@proton.me> wrote:
> >>>>>
> >>>>> On 27.08.24 10:41, Alice Ryhl wrote:
> >>>>>> 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`?
> >>>>
> >>>> No, I mean we would have `unsafe_const_new` directly set `state` to
> >>>> the right value and let `unsafe_const_init` be a no-op.
> >>>
> >>> But how do you set the right value of a list_head? The value will be
> >>> moved.
> >>
> >> Right ... we probably can't get around needing a macro. Can statics
> >> even reference themselves?
> >
> > Looks like they can:
> >
> > use std::ptr::addr_of;
> >
> > struct MyStruct {
> >     ptr: *const MyStruct,
> > }
> >
> > static mut MY_STRUCT: MyStruct = MyStruct {
> >     ptr: addr_of!(MY_STRUCT),
> > };
>
> That's useful to know...
> But I don't see a way to get pinned-init to work with this. I would need
> a lot of currently experimental features (const closures, const traits)
> and a way to initialize a static without providing a direct value, since
> I can't just do
>
>     static mut MY_STRUCT: MyStruct = {
>         unsafe { __pinned_init(addr_of_mut!(MY_STRUCT), /* initializer */) };
>         unsafe { addr_of!(MY_STRUCT).read() }
>     };
>
> It (rightfully) complains that I am initializing the static with itself.
>
> We /might/ be able to do something special for `Mutex`/ other locks, but
> I haven't tried yet. So the unsafe approach seems the best at the moment.

It sounds like we'll just want a macro that generates a global wrapped
in a Mutex/SpinLock for now ...

If we're going to do that, we could take the extra step and have it
generate special Guard and LockedBy types so that you can have a
LockedBy that doesn't need to make any runtime checks.

Alice

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

[Benno Lossin]

On 04.09.24 12:32, Alice Ryhl wrote:
> On Tue, Sep 3, 2024 at 12:17 AM Benno Lossin <benno.lossin@proton.me> wrote:
>>
>> On 02.09.24 13:42, Alice Ryhl wrote:
>>> On Mon, Sep 2, 2024 at 1:37 PM Alice Ryhl <aliceryhl@google.com> wrote:
>>>>
>>>> On Fri, Aug 30, 2024 at 3:22 PM Benno Lossin <benno.lossin@proton.me> wrote:
>>>>>
>>>>> On 30.08.24 07:34, Alice Ryhl wrote:
>>>>>> On Thu, Aug 29, 2024 at 8:17 PM Benno Lossin <benno.lossin@proton.me> wrote:
>>>>>>>
>>>>>>> On 27.08.24 10:41, Alice Ryhl wrote:
>>>>>>>> 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`?
>>>>>>
>>>>>> No, I mean we would have `unsafe_const_new` directly set `state` to
>>>>>> the right value and let `unsafe_const_init` be a no-op.
>>>>>
>>>>> But how do you set the right value of a list_head? The value will be
>>>>> moved.
>>>>
>>>> Right ... we probably can't get around needing a macro. Can statics
>>>> even reference themselves?
>>>
>>> Looks like they can:
>>>
>>> use std::ptr::addr_of;
>>>
>>> struct MyStruct {
>>>     ptr: *const MyStruct,
>>> }
>>>
>>> static mut MY_STRUCT: MyStruct = MyStruct {
>>>     ptr: addr_of!(MY_STRUCT),
>>> };
>>
>> That's useful to know...
>> But I don't see a way to get pinned-init to work with this. I would need
>> a lot of currently experimental features (const closures, const traits)
>> and a way to initialize a static without providing a direct value, since
>> I can't just do
>>
>>     static mut MY_STRUCT: MyStruct = {
>>         unsafe { __pinned_init(addr_of_mut!(MY_STRUCT), /* initializer */) };
>>         unsafe { addr_of!(MY_STRUCT).read() }
>>     };
>>
>> It (rightfully) complains that I am initializing the static with itself.
>>
>> We /might/ be able to do something special for `Mutex`/ other locks, but
>> I haven't tried yet. So the unsafe approach seems the best at the moment.
> 
> It sounds like we'll just want a macro that generates a global wrapped
> in a Mutex/SpinLock for now ...

Yeah.

> If we're going to do that, we could take the extra step and have it
> generate special Guard and LockedBy types so that you can have a
> LockedBy that doesn't need to make any runtime checks.

Oh that is a good idea!

---
Cheers,
Benno

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

[Benno Lossin]

On 02.09.24 13:37, Alice Ryhl wrote:
> On Fri, Aug 30, 2024 at 3:22 PM Benno Lossin <benno.lossin@proton.me> wrote:
>>
>> On 30.08.24 07:34, Alice Ryhl wrote:
>>> On Thu, Aug 29, 2024 at 8:17 PM Benno Lossin <benno.lossin@proton.me> wrote:
>>>>
>>>> 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?
>>>
>>> Not sure what you mean by "putting a Mutex" but the workaround is
>>
>> Oh sorry, seems like I forgot to write the rest of that... Let me try
>> again: If your use-case is covered by putting a `Mutex` inside of the
>> type that implements `Module`, then I think you should do that instead
>> of using a global. (you need the inplace module patch for that)
> 
> I don't think it's possible to access the `Module` struct after `init`
> returns? Even with in-place init.

Oh I see... Maybe if you create it inside of an `Arc` or `Box` and then
pass it to something inside of your module that will then be called by
some other mechanism?

---
Cheers,
Benno

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

[Gary Guo]

On Fri, 30 Aug 2024 07:34:00 +0200
Alice Ryhl <aliceryhl@google.com> wrote:

> > > 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.  
> 
> I don't think traits work with const today, so pin-init would need an
> entirely different mechanism? If you're talking about using
> CONSTRUCTORS, then I think it's an undesirable solution. C code can
> define static mutexes without load-time initialization hooks. We
> should be able to do the same.

I think I actually prefer using constructors to unsafe.

Best,
Gary

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

[Alice Ryhl]

On Fri, Aug 30, 2024 at 5:10 PM Gary Guo <gary@garyguo.net> wrote:
>
> On Fri, 30 Aug 2024 07:34:00 +0200
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> > > > 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.
> >
> > I don't think traits work with const today, so pin-init would need an
> > entirely different mechanism? If you're talking about using
> > CONSTRUCTORS, then I think it's an undesirable solution. C code can
> > define static mutexes without load-time initialization hooks. We
> > should be able to do the same.
>
> I think I actually prefer using constructors to unsafe.

Constructors are used pretty rarely. They're not enabled in the
Android build right now at all. I could ask to enable the option, but
I think "global mutexes" are a rather weak reason.

I also think they're a can of worms safety-wise. You can't run
arbitrary code in them since otherwise one constructor could use
another global before it gets initialized, so we will still need
global-mutex/spinlock-specific functionality even with constructors.
At that point, I think it would be better to just strive for real
const mutexes.

Alice