rust/kernel/sync.rs | 1 + rust/kernel/sync/lock.rs | 31 ++++- rust/kernel/sync/lock/global.rs | 260 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 291 insertions(+), 1 deletion(-)
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.
Link: https://lore.kernel.org/rust-for-linux/20231101-rust-binder-v1-2-08ba9197f637@google.com/#Z31drivers:android:context.rs [1]
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
This patch is based on top of v6.12-rc1 with:
* https://lore.kernel.org/r/BL0PR02MB4914579914884B5D7473B3D6E96A2@BL0PR02MB4914.namprd02.prod.outlook.com
---
Changes in v4:
- Evaluate `$value` in the surrounding scope.
- Make types `pub(crate)` to avoid "private type in public interface"
errors when using the macro.
- Add trylock method.
- using https://lore.kernel.org/r/BL0PR02MB4914579914884B5D7473B3D6E96A2@BL0PR02MB4914.namprd02.prod.outlook.com
- Add Send/Sync implementations of LockedBy.
- Fix examples so they compile.
- Link to v3: https://lore.kernel.org/r/20240910-static-mutex-v3-1-5bebd11bdf3b@google.com
Changes in v3:
- Rewrite to provide a macro instead.
- Link to v2: https://lore.kernel.org/r/20240827-static-mutex-v2-1-17fc32b20332@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.rs | 1 +
rust/kernel/sync/lock.rs | 31 ++++-
rust/kernel/sync/lock/global.rs | 260 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 291 insertions(+), 1 deletion(-)
diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index 0ab20975a3b5..2e97e22715db 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -14,6 +14,7 @@
pub use arc::{Arc, ArcBorrow, UniqueArc};
pub use condvar::{new_condvar, CondVar, CondVarTimeoutResult};
+pub use lock::global::global_lock;
pub use lock::mutex::{new_mutex, Mutex};
pub use lock::spinlock::{new_spinlock, SpinLock};
pub use locked_by::LockedBy;
diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index 360d06e9c57a..528e885ee535 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -7,12 +7,14 @@
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;
pub mod spinlock;
+pub(super) mod global;
+
/// The "backend" of a lock.
///
/// It is the actual implementation of the lock, without the need to repeat patterns used in all
@@ -124,6 +126,33 @@ pub fn new(t: T, name: &'static CStr, key: &'static LockClassKey) -> impl PinIni
}),
})
}
+
+ /// # Safety
+ ///
+ /// Before any other method on this lock is called, `global_lock_helper_init` must be called.
+ #[doc(hidden)]
+ pub const unsafe fn global_lock_helper_new(state: Opaque<B::State>, data: T) -> Self {
+ Self {
+ state,
+ data: UnsafeCell::new(data),
+ _pin: PhantomPinned,
+ }
+ }
+
+ /// # Safety
+ ///
+ /// * This lock must have been created using `global_lock_helper_new`.
+ /// * Must only be called once for each lock.
+ #[doc(hidden)]
+ pub unsafe fn global_lock_helper_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> {
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 {}
+/// # }
+/// ```
+#[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.
+ pub(crate) fn try_lock(&'static self) -> Option<GuardTyp> {
+ Some($crate::global_lock_inner!(new_guard $($guard)? {
+ self.inner.try_lock()?
+ }))
+ }
+ }
+
+ $(
+ pub(crate) struct $guard($crate::sync::lock::Guard<'static, Val, Backend>);
+
+ impl ::core::ops::Deref for $guard {
+ type Target = Val;
+ fn deref(&self) -> &Val {
+ &self.0
+ }
+ }
+
+ impl ::core::ops::DerefMut for $guard {
+ fn deref_mut(&mut self) -> &mut Val {
+ &mut self.0
+ }
+ }
+
+ pub(crate) struct $locked_by<T: ?Sized>(::core::cell::UnsafeCell<T>);
+
+ // SAFETY: `LockedBy` can be transferred across thread boundaries iff the data it
+ // protects can.
+ unsafe impl<T: ?Sized + Send> Send for $locked_by<T> {}
+
+ // SAFETY: `LockedBy` serialises the interior mutability it provides, so it is `Sync` as long as the
+ // data it protects is `Send`.
+ unsafe impl<T: ?Sized + Send> Sync for $locked_by<T> {}
+
+ impl<T> $locked_by<T> {
+ pub(crate) fn new(val: T) -> Self {
+ Self(::core::cell::UnsafeCell::new(val))
+ }
+ }
+
+ impl<T: ?Sized> $locked_by<T> {
+ pub(crate) fn as_ref<'a>(&'a self, _guard: &'a $guard) -> &'a T {
+ // SAFETY: The lock is globally unique, so there can only be one guard.
+ unsafe { &*self.0.get() }
+ }
+
+ pub(crate) fn as_mut<'a>(&'a self, _guard: &'a mut $guard) -> &'a mut T {
+ // SAFETY: The lock is globally unique, so there can only be one guard.
+ unsafe { &mut *self.0.get() }
+ }
+
+ pub(crate) fn get_mut(&mut self) -> &mut T {
+ self.0.get_mut()
+ }
+ }
+ )?
+ }
+
+ use [< __static_lock_mod_ $name >]::$wrapper;
+ $( use [< __static_lock_mod_ $name >]::{$guard, $locked_by}; )?
+
+ $(#[$meta])*
+ $pub static $name: $wrapper = {
+ // SAFETY: We are using this to initialize $name.
+ unsafe { [< __static_lock_mod_ $name >]::new() }
+ };
+ }
+ };
+
+ {
+ $(#[$meta:meta])* $pub:vis static $name:ident: $kind:ident<$valuety:ty> = unsafe { uninit };
+ value: $value:expr;
+ $( name: $lname:literal; )?
+ $(
+ guard: $guard:ident;
+ locked_by: $locked_by:ident;
+ )?
+ } => {
+ $crate::macros::paste! {
+ $crate::global_lock! {
+ $(#[$meta])* $pub static $name: $kind<$valuety> = unsafe { uninit };
+ value: $value;
+ wrapper: [< __static_lock_wrapper_ $name >];
+ $( name: $lname; )?
+ $( guard: $guard; locked_by: $locked_by; )?
+ }
+ }
+ }
+}
+pub use global_lock;
+
+#[doc(hidden)]
+#[macro_export]
+macro_rules! global_lock_inner {
+ (backend Mutex) => { $crate::sync::lock::mutex::MutexBackend };
+ (backend SpinLock) => { $crate::sync::lock::spinlock::SpinLockBackend };
+ (guard Mutex, $val:ty) => {
+ $crate::sync::lock::Guard<'static, $val, $crate::sync::lock::mutex::MutexBackend>
+ };
+ (guard SpinLock, $val:ty) => {
+ $crate::sync::lock::Guard<'static, $val, $crate::sync::lock::spinlock::SpinLockBackend>
+ };
+ (guard $kind:ident, $val:ty, $name:ident) => { $name };
+ (new_guard { $val:expr }) => { $val };
+ (new_guard $name:ident { $val:expr }) => { $name($val) };
+}
---
base-commit: e2bde5e2cb2e5b8dca87adce3a9eb605133518ea
change-id: 20240826-static-mutex-a4b228e0e6aa
Best regards,
--
Alice Ryhl <aliceryhl@google.com>Hi Alice,
Alice Ryhl <aliceryhl@google.com> 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
On Thu, Oct 10, 2024 at 3:57 PM Andreas Hindborg <a.hindborg@kernel.org> wrote: > > Hi Alice, > > Alice Ryhl <aliceryhl@google.com> 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. Because normal locks are not initialized in a const expression. > > 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? Probably only guard and locked_by are useful, but I think you need to give the wrapper a name to reasonably use guard/locked_by. > > + /// Lock this global lock. > > "Try to lock..." ? Thanks. Good point. Alice
Alice Ryhl <aliceryhl@google.com> writes: > On Thu, Oct 10, 2024 at 3:57 PM Andreas Hindborg <a.hindborg@kernel.org> wrote: >> >> Hi Alice, >> >> Alice Ryhl <aliceryhl@google.com> 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. > > Because normal locks are not initialized in a const expression. > >> > 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? > > Probably only guard and locked_by are useful, but I think you need to > give the wrapper a name to reasonably use guard/locked_by. Could you expand the example to show this, or would it become too long? BR Andreas
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>,
> + }
> +
> +
"Benno Lossin" <benno.lossin@proton.me> writes:
>
> 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`
>
How did you manage to get these errors? The only thing I get from clippy
is this:
CC rust/doctests_kernel_generated_kunit.o
warning: question mark operator is useless here
--> rust/doctests_kernel_generated.rs:4744:1
|
4744 | / kernel::sync::global_lock! {
4745 | | // SAFETY: Initialized in module initializer before first use.
4746 | | static MY_COUNTER: Mutex<u32> = unsafe { uninit };
4747 | | value: 0;
4748 | | }
| |_^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_question_mark
= note: `-W clippy::needless-question-mark` implied by `-W clippy::all`
= help: to override `-W clippy::all` add `#[allow(clippy::needless_question_mark)]`
= note: this warning originates in the macro `$crate::global_lock` which comes from the expansion of the macro `kernel::sync::global_lock` (in Nightly builds, run with -Z macro-backtrace for more info)
warning: 1 warning emitted
BR Andreas
On 10.10.24 16:01, Andreas Hindborg wrote: > "Benno Lossin" <benno.lossin@proton.me> writes: > >> >> 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` >> > > How did you manage to get these errors? I added a `global_lock!` invocation to `lib.rs`. Otherwise I also don't get them (they might also be emitted from the examples, but I didn't compile those). --- Cheers, Benno
"Benno Lossin" <benno.lossin@proton.me> writes: > On 10.10.24 16:01, Andreas Hindborg wrote: >> "Benno Lossin" <benno.lossin@proton.me> writes: >> >>> >>> 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` >>> >> >> How did you manage to get these errors? > > I added a `global_lock!` invocation to `lib.rs`. Otherwise I also don't > get them (they might also be emitted from the examples, but I didn't > compile those). I build the examples for kunit, but I did not get any warnings. Interesting. BR Andreas
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
On 10.10.24 12:53, Alice Ryhl wrote:
> 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.
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
On Thu, Oct 10, 2024 at 12:53:00PM +0200, Alice Ryhl wrote:
[...]
> > > +#[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.
>
While we are at it, how about we make the syntax:
global_lock!{
static MY_LOCK: Mutex<u32> = unsafe { 0 };
}
or
global_lock!{
static MY_LOCK: Mutex<u32> = unsafe { uninit { 0 } };
}
?
i.e. instead of a "value" field, we put it in the "initialization
expression". To me, this make it more clear that "value" is the
initialized value protected by the lock. Thoughts?
Besides, instead of a "guard" type name, could you make a
generic guard type over the "locked_by" type? E.g.
struct GlobalGuard<L: GlobalLockedBy>(Guard<...>, PhantomData<*mut L>);
I feel like this could make the relationship between the guard type and
the locked_by type more obvious. But maybe there's something I'm
missing?
Regards,
Boqun
[...]
On Thu, Oct 10, 2024 at 3:55 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Thu, Oct 10, 2024 at 12:53:00PM +0200, Alice Ryhl wrote:
> [...]
> > > > +#[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.
> >
>
> While we are at it, how about we make the syntax:
>
> global_lock!{
> static MY_LOCK: Mutex<u32> = unsafe { 0 };
> }
>
> or
>
> global_lock!{
> static MY_LOCK: Mutex<u32> = unsafe { uninit { 0 } };
> }
>
> ?
>
> i.e. instead of a "value" field, we put it in the "initialization
> expression". To me, this make it more clear that "value" is the
> initialized value protected by the lock. Thoughts?
`uninit { 0 }` looks pretty terrible IMO. Can we come up with something better?
> Besides, instead of a "guard" type name, could you make a
> generic guard type over the "locked_by" type? E.g.
>
> struct GlobalGuard<L: GlobalLockedBy>(Guard<...>, PhantomData<*mut L>);
>
> I feel like this could make the relationship between the guard type and
> the locked_by type more obvious. But maybe there's something I'm
> missing?
Sorry, I don't understand this. Why is the LockedBy type relevant to the guard?
Alice
On Thu, Oct 10, 2024 at 03:58:07PM +0200, Alice Ryhl wrote:
> On Thu, Oct 10, 2024 at 3:55 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > On Thu, Oct 10, 2024 at 12:53:00PM +0200, Alice Ryhl wrote:
> > [...]
> > > > > +#[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.
> > >
> >
> > While we are at it, how about we make the syntax:
> >
> > global_lock!{
> > static MY_LOCK: Mutex<u32> = unsafe { 0 };
> > }
> >
> > or
> >
> > global_lock!{
> > static MY_LOCK: Mutex<u32> = unsafe { uninit { 0 } };
> > }
> >
> > ?
> >
> > i.e. instead of a "value" field, we put it in the "initialization
> > expression". To me, this make it more clear that "value" is the
> > initialized value protected by the lock. Thoughts?
>
> `uninit { 0 }` looks pretty terrible IMO. Can we come up with something better?
>
I'm trying, but in the meanwhile, another idea comes to me, can we just
avoid using the `global_lock!` macro if "locked_by" is not needed? I.e.
as the global counter in your examples, we probably need a new lock type
in these cases but we don't use macros. I feel like macros don't bring
us anything in these usages. As a result, we make `global_lock!`
dedicated for locked_by usages, where values should usually be a `()`.
> > Besides, instead of a "guard" type name, could you make a
> > generic guard type over the "locked_by" type? E.g.
> >
> > struct GlobalGuard<L: GlobalLockedBy>(Guard<...>, PhantomData<*mut L>);
> >
> > I feel like this could make the relationship between the guard type and
> > the locked_by type more obvious. But maybe there's something I'm
> > missing?
>
> Sorry, I don't understand this. Why is the LockedBy type relevant to the guard?
>
well, $locked_by's `as_ref()` and `as_mut()` both need a $guard
reference, instead of asking users to provide an arbitrary $guard name,
I thought it's better to just make $guard generic over $locked_by.
However, I realise that $locked_by is a generic type over T, so my
previous proposal doesn't work. How about another way around: making
$locked_by type generic over $guard:
pub(crate) struct GlobalLockedBy<T: ?Sized, G>(::core::cell::UnsafeCell<T>, ...);
(G is a $guard type)
and if we do
global_lock!{
static MY_LOCK ...;
guard: MyGuard;
}
we can have a struct:
struct MyStruct {
inner: GlobalLockedBy<u32, MyGuard>
}
instead of asking users to provide a name of the $locked_by type.
Does this makes sense?
Regards,
Boqun
> Alice
On Thu, Oct 10, 2024 at 07:29:32AM -0700, Boqun Feng wrote:
> On Thu, Oct 10, 2024 at 03:58:07PM +0200, Alice Ryhl wrote:
> > On Thu, Oct 10, 2024 at 3:55 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> > >
> > > On Thu, Oct 10, 2024 at 12:53:00PM +0200, Alice Ryhl wrote:
> > > [...]
> > > > > > +#[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.
> > > >
> > >
> > > While we are at it, how about we make the syntax:
> > >
> > > global_lock!{
> > > static MY_LOCK: Mutex<u32> = unsafe { 0 };
> > > }
> > >
> > > or
> > >
> > > global_lock!{
> > > static MY_LOCK: Mutex<u32> = unsafe { uninit { 0 } };
> > > }
> > >
> > > ?
> > >
> > > i.e. instead of a "value" field, we put it in the "initialization
> > > expression". To me, this make it more clear that "value" is the
> > > initialized value protected by the lock. Thoughts?
> >
> > `uninit { 0 }` looks pretty terrible IMO. Can we come up with something better?
> >
>
how about:
global_lock!{
static MY_LOCK: Mutex<u32> = unsafe { data: 0 };
}
?
"data: " will make it clear that the value is not for the lock state.
"uninit" is dropped because the "unsafe" already requires the global
variable to be initialised first. Or "unsafe { uninit, data: 0 }" if you
want to keep the "uninit" part?
Regards,
Boqun
[...]
On 10.10.24 18:33, Boqun Feng wrote:
> On Thu, Oct 10, 2024 at 07:29:32AM -0700, Boqun Feng wrote:
>> On Thu, Oct 10, 2024 at 03:58:07PM +0200, Alice Ryhl wrote:
>>> On Thu, Oct 10, 2024 at 3:55 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>>>>
>>>> On Thu, Oct 10, 2024 at 12:53:00PM +0200, Alice Ryhl wrote:
>>>> [...]
>>>>>>> +#[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.
>>>>>
>>>>
>>>> While we are at it, how about we make the syntax:
>>>>
>>>> global_lock!{
>>>> static MY_LOCK: Mutex<u32> = unsafe { 0 };
>>>> }
>>>>
>>>> or
>>>>
>>>> global_lock!{
>>>> static MY_LOCK: Mutex<u32> = unsafe { uninit { 0 } };
>>>> }
>>>>
>>>> ?
>>>>
>>>> i.e. instead of a "value" field, we put it in the "initialization
>>>> expression". To me, this make it more clear that "value" is the
>>>> initialized value protected by the lock. Thoughts?
>>>
>>> `uninit { 0 }` looks pretty terrible IMO. Can we come up with something better?
>>>
>>
>
> how about:
>
> global_lock!{
> static MY_LOCK: Mutex<u32> = unsafe { data: 0 };
I dislike this, since there is no `uninit` anywhere, but the mutex needs
to be initialized.
> }
>
> ?
>
> "data: " will make it clear that the value is not for the lock state.
> "uninit" is dropped because the "unsafe" already requires the global
> variable to be initialised first. Or "unsafe { uninit, data: 0 }" if you
> want to keep the "uninit" part?
That also looks weird to me...
But I haven't come up with a good alternative
---
Cheers,
Benno
On Thu, Oct 10, 2024 at 10:21:41PM +0000, Benno Lossin wrote:
> On 10.10.24 18:33, Boqun Feng wrote:
> > On Thu, Oct 10, 2024 at 07:29:32AM -0700, Boqun Feng wrote:
> >> On Thu, Oct 10, 2024 at 03:58:07PM +0200, Alice Ryhl wrote:
> >>> On Thu, Oct 10, 2024 at 3:55 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >>>>
> >>>> On Thu, Oct 10, 2024 at 12:53:00PM +0200, Alice Ryhl wrote:
> >>>> [...]
> >>>>>>> +#[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.
> >>>>>
> >>>>
> >>>> While we are at it, how about we make the syntax:
> >>>>
> >>>> global_lock!{
> >>>> static MY_LOCK: Mutex<u32> = unsafe { 0 };
> >>>> }
> >>>>
> >>>> or
> >>>>
> >>>> global_lock!{
> >>>> static MY_LOCK: Mutex<u32> = unsafe { uninit { 0 } };
> >>>> }
> >>>>
> >>>> ?
> >>>>
> >>>> i.e. instead of a "value" field, we put it in the "initialization
> >>>> expression". To me, this make it more clear that "value" is the
> >>>> initialized value protected by the lock. Thoughts?
> >>>
> >>> `uninit { 0 }` looks pretty terrible IMO. Can we come up with something better?
> >>>
> >>
> >
> > how about:
> >
> > global_lock!{
> > static MY_LOCK: Mutex<u32> = unsafe { data: 0 };
>
> I dislike this, since there is no `uninit` anywhere, but the mutex needs
> to be initialized.
>
> > }
> >
> > ?
> >
> > "data: " will make it clear that the value is not for the lock state.
> > "uninit" is dropped because the "unsafe" already requires the global
> > variable to be initialised first. Or "unsafe { uninit, data: 0 }" if you
> > want to keep the "uninit" part?
>
> That also looks weird to me...
>
> But I haven't come up with a good alternative
>
How about a "fake" MaybyUninit:
global_lock!{
static MY_LOCK: Mutex<u32> = unsafe { MaybeUninit::new(0).assume_init() };
}
?
I feel like we need to put the data in the initialization expression
because if we resolve the initialization issues and can skip the extra
init step, we pretty much want to use the macro like:
global_lock!{
static MY_LOCK: Mutex<u32> = { data: 0 };
// maybe even
// static MY_LOCK: Mutex<u32> = { 0 };
}
instead of
global_lock!{
static MY_LOCK: Mutex<u32> = init;
value = 0;
}
, right?
So we need to think about providing a smooth way for users to transfer.
Not just adjust the changes (which I believe is a good practice for
coccinelle), but also the conceptual model "oh now I don't need to
provide a 'value=' field?". Hence even though the above proposals may
look weird, but I think that's still better?
Regards,
Boqun
> ---
> Cheers,
> Benno
>
On 11.10.24 01:06, Boqun Feng wrote:
> On Thu, Oct 10, 2024 at 10:21:41PM +0000, Benno Lossin wrote:
>> On 10.10.24 18:33, Boqun Feng wrote:
>>> On Thu, Oct 10, 2024 at 07:29:32AM -0700, Boqun Feng wrote:
>>>> On Thu, Oct 10, 2024 at 03:58:07PM +0200, Alice Ryhl wrote:
>>>>> On Thu, Oct 10, 2024 at 3:55 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>>>>>>
>>>>>> On Thu, Oct 10, 2024 at 12:53:00PM +0200, Alice Ryhl wrote:
>>>>>> [...]
>>>>>>>>> +#[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.
>>>>>>>
>>>>>>
>>>>>> While we are at it, how about we make the syntax:
>>>>>>
>>>>>> global_lock!{
>>>>>> static MY_LOCK: Mutex<u32> = unsafe { 0 };
>>>>>> }
>>>>>>
>>>>>> or
>>>>>>
>>>>>> global_lock!{
>>>>>> static MY_LOCK: Mutex<u32> = unsafe { uninit { 0 } };
>>>>>> }
>>>>>>
>>>>>> ?
>>>>>>
>>>>>> i.e. instead of a "value" field, we put it in the "initialization
>>>>>> expression". To me, this make it more clear that "value" is the
>>>>>> initialized value protected by the lock. Thoughts?
>>>>>
>>>>> `uninit { 0 }` looks pretty terrible IMO. Can we come up with something better?
>>>>>
>>>>
>>>
>>> how about:
>>>
>>> global_lock!{
>>> static MY_LOCK: Mutex<u32> = unsafe { data: 0 };
>>
>> I dislike this, since there is no `uninit` anywhere, but the mutex needs
>> to be initialized.
>>
>>> }
>>>
>>> ?
>>>
>>> "data: " will make it clear that the value is not for the lock state.
>>> "uninit" is dropped because the "unsafe" already requires the global
>>> variable to be initialised first. Or "unsafe { uninit, data: 0 }" if you
>>> want to keep the "uninit" part?
>>
>> That also looks weird to me...
>>
>> But I haven't come up with a good alternative
>>
>
> How about a "fake" MaybyUninit:
>
> global_lock!{
> static MY_LOCK: Mutex<u32> = unsafe { MaybeUninit::new(0).assume_init() };
> }
>
> ?
That still suggests to the user, that the contents are initialized.
> I feel like we need to put the data in the initialization expression
> because if we resolve the initialization issues and can skip the extra
> init step, we pretty much want to use the macro like:
>
> global_lock!{
> static MY_LOCK: Mutex<u32> = { data: 0 };
> // maybe even
> // static MY_LOCK: Mutex<u32> = { 0 };
> }
>
> instead of
>
> global_lock!{
> static MY_LOCK: Mutex<u32> = init;
> value = 0;
> }
>
> , right?
>
> So we need to think about providing a smooth way for users to transfer.
> Not just adjust the changes (which I believe is a good practice for
> coccinelle), but also the conceptual model "oh now I don't need to
> provide a 'value=' field?".
I think we can just use a multiline regex to find `global_lock!` and
then change the current way. It shouldn't be that difficult to change.
> Hence even though the above proposals may look weird, but I think
> that's still better?
Do you think that we will have 1000s of users by the time we change it?
I don't think so, but I don't know how often drivers use global locks. I
think we should discourage them. People still can have a "global" lock
by putting it inside of their Module struct (they need access to the
module, but maybe we should have a function that gives them the module
for the `ThisModule` pointer?)
---
Cheers,
Benno
On Fri, Oct 11, 2024 at 07:01:09AM +0000, Benno Lossin wrote:
> On 11.10.24 01:06, Boqun Feng wrote:
> > On Thu, Oct 10, 2024 at 10:21:41PM +0000, Benno Lossin wrote:
> >> On 10.10.24 18:33, Boqun Feng wrote:
> >>> On Thu, Oct 10, 2024 at 07:29:32AM -0700, Boqun Feng wrote:
> >>>> On Thu, Oct 10, 2024 at 03:58:07PM +0200, Alice Ryhl wrote:
> >>>>> On Thu, Oct 10, 2024 at 3:55 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >>>>>>
> >>>>>> On Thu, Oct 10, 2024 at 12:53:00PM +0200, Alice Ryhl wrote:
> >>>>>> [...]
> >>>>>>>>> +#[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.
> >>>>>>>
> >>>>>>
> >>>>>> While we are at it, how about we make the syntax:
> >>>>>>
> >>>>>> global_lock!{
> >>>>>> static MY_LOCK: Mutex<u32> = unsafe { 0 };
> >>>>>> }
> >>>>>>
> >>>>>> or
> >>>>>>
> >>>>>> global_lock!{
> >>>>>> static MY_LOCK: Mutex<u32> = unsafe { uninit { 0 } };
> >>>>>> }
> >>>>>>
> >>>>>> ?
> >>>>>>
> >>>>>> i.e. instead of a "value" field, we put it in the "initialization
> >>>>>> expression". To me, this make it more clear that "value" is the
> >>>>>> initialized value protected by the lock. Thoughts?
> >>>>>
> >>>>> `uninit { 0 }` looks pretty terrible IMO. Can we come up with something better?
> >>>>>
> >>>>
> >>>
> >>> how about:
> >>>
> >>> global_lock!{
> >>> static MY_LOCK: Mutex<u32> = unsafe { data: 0 };
> >>
> >> I dislike this, since there is no `uninit` anywhere, but the mutex needs
> >> to be initialized.
> >>
> >>> }
> >>>
> >>> ?
> >>>
> >>> "data: " will make it clear that the value is not for the lock state.
> >>> "uninit" is dropped because the "unsafe" already requires the global
> >>> variable to be initialised first. Or "unsafe { uninit, data: 0 }" if you
> >>> want to keep the "uninit" part?
> >>
> >> That also looks weird to me...
> >>
> >> But I haven't come up with a good alternative
> >>
> >
> > How about a "fake" MaybyUninit:
> >
> > global_lock!{
> > static MY_LOCK: Mutex<u32> = unsafe { MaybeUninit::new(0).assume_init() };
> > }
> >
> > ?
>
> That still suggests to the user, that the contents are initialized.
>
> > I feel like we need to put the data in the initialization expression
> > because if we resolve the initialization issues and can skip the extra
> > init step, we pretty much want to use the macro like:
> >
> > global_lock!{
> > static MY_LOCK: Mutex<u32> = { data: 0 };
> > // maybe even
> > // static MY_LOCK: Mutex<u32> = { 0 };
> > }
> >
> > instead of
> >
> > global_lock!{
> > static MY_LOCK: Mutex<u32> = init;
> > value = 0;
> > }
> >
> > , right?
> >
> > So we need to think about providing a smooth way for users to transfer.
> > Not just adjust the changes (which I believe is a good practice for
> > coccinelle), but also the conceptual model "oh now I don't need to
> > provide a 'value=' field?".
>
> I think we can just use a multiline regex to find `global_lock!` and
> then change the current way. It shouldn't be that difficult to change.
>
This only solves the "not just" part of the problem, it still confuses
the users why "value=" used to be a field but then becomes in the
initialization expression. In fact, after I take a deep look, I think we
should also move the optional "name=" field (the name for lockdep class)
to the static-item-like statement, i.e.
global_lock!{
// I use a new format here, which I think it's better than
// previous proposed formats.
static MY_LOCK: Mutex<u32> = { unsafe { uninit }, data: 0, name: "my_lock" };
}
I think it makes more sense than making "value=" and "name=" as fields.
Think about this: we choose to use a statement in global_lock!() macro
that is similar to a static item because we want to let users know what
the macro does is similar as defining a static item, right? If so,
wouldn't it make more sense to have the initialization inputs in the
static-item-like statement instead of a separate field? This will
provide a clear picture to users that which macro parameter participates
the initialization of the static variable.
Besides, for guard/locked_by, the current format will be something like:
global_lock!{
static MY_LOCK: Mutex<u32> = unsafe { uninit };
value=0;
guard=MyGuard;
locked_by=MyLockedBy;
}
"value=" is the value used to initialize MY_LOCK, and "guard=" and
"locked_by=" are the types associated with the static variable, making
these three parallel in the same level doesn't really make logical sense
to me. I cannot find any reason this is better than making "value=" part
of the initialization expression.
Your argument seems to be that will be weird, however, as you mentioned
below, you don't think there will be many users, so being weird but
accurate on what are used to initialize the static variable is fine. ;-)
Plus, there is no implementation difficulty, so let do it right.
Regards,
Boqun
> > Hence even though the above proposals may look weird, but I think
> > that's still better?
>
> Do you think that we will have 1000s of users by the time we change it?
> I don't think so, but I don't know how often drivers use global locks. I
> think we should discourage them. People still can have a "global" lock
> by putting it inside of their Module struct (they need access to the
> module, but maybe we should have a function that gives them the module
> for the `ThisModule` pointer?)
>
> ---
> Cheers,
> Benno
>
© 2016 - 2024 Red Hat, Inc.