* [PATCH v14 1/7] rust: sync: add `OnceLock`
2025-07-02 13:18 [PATCH v14 0/7] rust: extend `module!` macro with integer parameter support Andreas Hindborg
@ 2025-07-02 13:18 ` Andreas Hindborg
2025-07-02 13:32 ` Alice Ryhl
` (2 more replies)
2025-07-02 13:18 ` [PATCH v14 2/7] rust: str: add radix prefixed integer parsing functions Andreas Hindborg
` (5 subsequent siblings)
6 siblings, 3 replies; 34+ messages in thread
From: Andreas Hindborg @ 2025-07-02 13:18 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Benno Lossin, Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules, Andreas Hindborg
Introduce the `OnceLock` type, a container that can only be written once.
The container uses an internal atomic to synchronize writes to the internal
value.
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/sync.rs | 1 +
rust/kernel/sync/once_lock.rs | 104 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 105 insertions(+)
diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index c7c0e552bafe..f2ee07315091 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -15,6 +15,7 @@
mod condvar;
pub mod lock;
mod locked_by;
+pub mod once_lock;
pub mod poll;
pub mod rcu;
diff --git a/rust/kernel/sync/once_lock.rs b/rust/kernel/sync/once_lock.rs
new file mode 100644
index 000000000000..cd311bea3919
--- /dev/null
+++ b/rust/kernel/sync/once_lock.rs
@@ -0,0 +1,104 @@
+//! A container that can be initialized at most once.
+
+use super::atomic::ordering::Acquire;
+use super::atomic::ordering::Release;
+use super::atomic::Atomic;
+use kernel::types::Opaque;
+
+/// A container that can be populated at most once. Thread safe.
+///
+/// Once the a [`OnceLock`] is populated, it remains populated by the same object for the
+/// lifetime `Self`.
+///
+/// # Invariants
+///
+/// `init` tracks the state of the container:
+///
+/// - If the container is empty, `init` is `0`.
+/// - If the container is mutably accessed, `init` is `1`.
+/// - If the container is populated and ready for shared access, `init` is `2`.
+///
+/// # Example
+///
+/// ```
+/// # use kernel::sync::once_lock::OnceLock;
+/// let value = OnceLock::new();
+/// assert_eq!(None, value.as_ref());
+///
+/// let status = value.populate(42u8);
+/// assert_eq!(true, status);
+/// assert_eq!(Some(&42u8), value.as_ref());
+/// assert_eq!(Some(42u8), value.copy());
+///
+/// let status = value.populate(101u8);
+/// assert_eq!(false, status);
+/// assert_eq!(Some(&42u8), value.as_ref());
+/// assert_eq!(Some(42u8), value.copy());
+/// ```
+pub struct OnceLock<T> {
+ init: Atomic<u32>,
+ value: Opaque<T>,
+}
+
+impl<T> Default for OnceLock<T> {
+ fn default() -> Self {
+ Self::new()
+ }
+}
+
+impl<T> OnceLock<T> {
+ /// Create a new [`OnceLock`].
+ ///
+ /// The returned instance will be empty.
+ pub const fn new() -> Self {
+ // INVARIANT: The container is empty and we set `init` to `0`.
+ Self {
+ value: Opaque::uninit(),
+ init: Atomic::new(0),
+ }
+ }
+
+ /// Get a reference to the contained object.
+ ///
+ /// Returns [`None`] if this [`OnceLock`] is empty.
+ pub fn as_ref(&self) -> Option<&T> {
+ if self.init.load(Acquire) == 2 {
+ // SAFETY: As determined by the load above, the object is ready for shared access.
+ Some(unsafe { &*self.value.get() })
+ } else {
+ None
+ }
+ }
+
+ /// Populate the [`OnceLock`].
+ ///
+ /// Returns `true` if the [`OnceLock`] was successfully populated.
+ pub fn populate(&self, value: T) -> bool {
+ // INVARIANT: We obtain exclusive access to the contained allocation and write 1 to
+ // `init`.
+ if let Ok(0) = self.init.cmpxchg(0, 1, Acquire) {
+ // SAFETY: We obtained exclusive access to the contained object.
+ unsafe { core::ptr::write(self.value.get(), value) };
+ // INVARIANT: We release our exclusive access and transition the object to shared
+ // access.
+ self.init.store(2, Release);
+ true
+ } else {
+ false
+ }
+ }
+}
+
+impl<T: Copy> OnceLock<T> {
+ /// Get a copy of the contained object.
+ ///
+ /// Returns [`None`] if the [`OnceLock`] is empty.
+ pub fn copy(&self) -> Option<T> {
+ if self.init.load(Acquire) == 2 {
+ // SAFETY: As determined by the load above, the object is ready for shared access.
+ Some(unsafe { *self.value.get() })
+ } else {
+ None
+ }
+ }
+}
--
2.47.2
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v14 1/7] rust: sync: add `OnceLock`
2025-07-02 13:18 ` [PATCH v14 1/7] rust: sync: add `OnceLock` Andreas Hindborg
@ 2025-07-02 13:32 ` Alice Ryhl
2025-07-02 13:54 ` Andreas Hindborg
2025-07-02 15:07 ` Benno Lossin
2025-07-03 9:36 ` Wren Turkal
2 siblings, 1 reply; 34+ messages in thread
From: Alice Ryhl @ 2025-07-02 13:32 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Masahiro Yamada, Nathan Chancellor,
Luis Chamberlain, Danilo Krummrich, Benno Lossin, Nicolas Schier,
Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
On Wed, Jul 2, 2025 at 3:19 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Introduce the `OnceLock` type, a container that can only be written once.
> The container uses an internal atomic to synchronize writes to the internal
> value.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
This type provides no way to wait for initialization to finish if it's
ongoing. Do you not need that?
> ---
> rust/kernel/sync.rs | 1 +
> rust/kernel/sync/once_lock.rs | 104 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 105 insertions(+)
>
> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> index c7c0e552bafe..f2ee07315091 100644
> --- a/rust/kernel/sync.rs
> +++ b/rust/kernel/sync.rs
> @@ -15,6 +15,7 @@
> mod condvar;
> pub mod lock;
> mod locked_by;
> +pub mod once_lock;
I would add a re-export so that users can import this as kernel::sync::OnceLock.
> pub mod poll;
> pub mod rcu;
>
> diff --git a/rust/kernel/sync/once_lock.rs b/rust/kernel/sync/once_lock.rs
> new file mode 100644
> index 000000000000..cd311bea3919
> --- /dev/null
> +++ b/rust/kernel/sync/once_lock.rs
> @@ -0,0 +1,104 @@
> +//! A container that can be initialized at most once.
> +
> +use super::atomic::ordering::Acquire;
> +use super::atomic::ordering::Release;
> +use super::atomic::Atomic;
> +use kernel::types::Opaque;
> +
> +/// A container that can be populated at most once. Thread safe.
> +///
> +/// Once the a [`OnceLock`] is populated, it remains populated by the same object for the
> +/// lifetime `Self`.
> +///
> +/// # Invariants
> +///
> +/// `init` tracks the state of the container:
> +///
> +/// - If the container is empty, `init` is `0`.
> +/// - If the container is mutably accessed, `init` is `1`.
I would phrase this as "being initialized" instead of "mutably
accessed". I initially thought this was talking about someone calling
a &mut self method.
> +/// - If the container is populated and ready for shared access, `init` is `2`.
> +///
> +/// # Example
> +///
> +/// ```
> +/// # use kernel::sync::once_lock::OnceLock;
> +/// let value = OnceLock::new();
> +/// assert_eq!(None, value.as_ref());
> +///
> +/// let status = value.populate(42u8);
> +/// assert_eq!(true, status);
> +/// assert_eq!(Some(&42u8), value.as_ref());
> +/// assert_eq!(Some(42u8), value.copy());
> +///
> +/// let status = value.populate(101u8);
> +/// assert_eq!(false, status);
> +/// assert_eq!(Some(&42u8), value.as_ref());
> +/// assert_eq!(Some(42u8), value.copy());
> +/// ```
> +pub struct OnceLock<T> {
> + init: Atomic<u32>,
> + value: Opaque<T>,
Opaque does not destroy the inner value. You are missing a destructor.
> +}
> +
> +impl<T> Default for OnceLock<T> {
> + fn default() -> Self {
> + Self::new()
> + }
> +}
> +
> +impl<T> OnceLock<T> {
> + /// Create a new [`OnceLock`].
> + ///
> + /// The returned instance will be empty.
> + pub const fn new() -> Self {
> + // INVARIANT: The container is empty and we set `init` to `0`.
> + Self {
> + value: Opaque::uninit(),
> + init: Atomic::new(0),
> + }
> + }
> +
> + /// Get a reference to the contained object.
> + ///
> + /// Returns [`None`] if this [`OnceLock`] is empty.
> + pub fn as_ref(&self) -> Option<&T> {
> + if self.init.load(Acquire) == 2 {
> + // SAFETY: As determined by the load above, the object is ready for shared access.
> + Some(unsafe { &*self.value.get() })
> + } else {
> + None
> + }
> + }
> +
> + /// Populate the [`OnceLock`].
> + ///
> + /// Returns `true` if the [`OnceLock`] was successfully populated.
> + pub fn populate(&self, value: T) -> bool {
> + // INVARIANT: We obtain exclusive access to the contained allocation and write 1 to
> + // `init`.
> + if let Ok(0) = self.init.cmpxchg(0, 1, Acquire) {
This acquire can be Relaxed. All other accesses to self.value
synchronize with the release store below, so you do not need acquire
here to obtain exclusive access.
> + // SAFETY: We obtained exclusive access to the contained object.
> + unsafe { core::ptr::write(self.value.get(), value) };
> + // INVARIANT: We release our exclusive access and transition the object to shared
> + // access.
> + self.init.store(2, Release);
> + true
> + } else {
> + false
> + }
> + }
> +}
> +
> +impl<T: Copy> OnceLock<T> {
> + /// Get a copy of the contained object.
> + ///
> + /// Returns [`None`] if the [`OnceLock`] is empty.
> + pub fn copy(&self) -> Option<T> {
> + if self.init.load(Acquire) == 2 {
> + // SAFETY: As determined by the load above, the object is ready for shared access.
> + Some(unsafe { *self.value.get() })
> + } else {
> + None
> + }
> + }
> +}
>
> --
> 2.47.2
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v14 1/7] rust: sync: add `OnceLock`
2025-07-02 13:32 ` Alice Ryhl
@ 2025-07-02 13:54 ` Andreas Hindborg
2025-07-02 14:50 ` Alice Ryhl
0 siblings, 1 reply; 34+ messages in thread
From: Andreas Hindborg @ 2025-07-02 13:54 UTC (permalink / raw)
To: Alice Ryhl
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Masahiro Yamada, Nathan Chancellor,
Luis Chamberlain, Danilo Krummrich, Benno Lossin, Nicolas Schier,
Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
"Alice Ryhl" <aliceryhl@google.com> writes:
> On Wed, Jul 2, 2025 at 3:19 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Introduce the `OnceLock` type, a container that can only be written once.
>> The container uses an internal atomic to synchronize writes to the internal
>> value.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>
> This type provides no way to wait for initialization to finish if it's
> ongoing. Do you not need that?
I don't, and in my use case it would cause a deadlock to wait. Anyway,
it might be useful to others. Would you add it now, or wait for a user?
>
>> ---
>> rust/kernel/sync.rs | 1 +
>> rust/kernel/sync/once_lock.rs | 104 ++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 105 insertions(+)
>>
>> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
>> index c7c0e552bafe..f2ee07315091 100644
>> --- a/rust/kernel/sync.rs
>> +++ b/rust/kernel/sync.rs
>> @@ -15,6 +15,7 @@
>> mod condvar;
>> pub mod lock;
>> mod locked_by;
>> +pub mod once_lock;
>
> I would add a re-export so that users can import this as kernel::sync::OnceLock.
OK.
>
>> pub mod poll;
>> pub mod rcu;
>>
>> diff --git a/rust/kernel/sync/once_lock.rs b/rust/kernel/sync/once_lock.rs
>> new file mode 100644
>> index 000000000000..cd311bea3919
>> --- /dev/null
>> +++ b/rust/kernel/sync/once_lock.rs
>> @@ -0,0 +1,104 @@
>> +//! A container that can be initialized at most once.
>> +
>> +use super::atomic::ordering::Acquire;
>> +use super::atomic::ordering::Release;
>> +use super::atomic::Atomic;
>> +use kernel::types::Opaque;
>> +
>> +/// A container that can be populated at most once. Thread safe.
>> +///
>> +/// Once the a [`OnceLock`] is populated, it remains populated by the same object for the
>> +/// lifetime `Self`.
>> +///
>> +/// # Invariants
>> +///
>> +/// `init` tracks the state of the container:
>> +///
>> +/// - If the container is empty, `init` is `0`.
>> +/// - If the container is mutably accessed, `init` is `1`.
>
> I would phrase this as "being initialized" instead of "mutably
> accessed". I initially thought this was talking about someone calling
> a &mut self method.
Makes sense, I will change that.
>
>> +/// - If the container is populated and ready for shared access, `init` is `2`.
>> +///
>> +/// # Example
>> +///
>> +/// ```
>> +/// # use kernel::sync::once_lock::OnceLock;
>> +/// let value = OnceLock::new();
>> +/// assert_eq!(None, value.as_ref());
>> +///
>> +/// let status = value.populate(42u8);
>> +/// assert_eq!(true, status);
>> +/// assert_eq!(Some(&42u8), value.as_ref());
>> +/// assert_eq!(Some(42u8), value.copy());
>> +///
>> +/// let status = value.populate(101u8);
>> +/// assert_eq!(false, status);
>> +/// assert_eq!(Some(&42u8), value.as_ref());
>> +/// assert_eq!(Some(42u8), value.copy());
>> +/// ```
>> +pub struct OnceLock<T> {
>> + init: Atomic<u32>,
>> + value: Opaque<T>,
>
> Opaque does not destroy the inner value. You are missing a destructor.
Oops.
>
>> +}
>> +
>> +impl<T> Default for OnceLock<T> {
>> + fn default() -> Self {
>> + Self::new()
>> + }
>> +}
>> +
>> +impl<T> OnceLock<T> {
>> + /// Create a new [`OnceLock`].
>> + ///
>> + /// The returned instance will be empty.
>> + pub const fn new() -> Self {
>> + // INVARIANT: The container is empty and we set `init` to `0`.
>> + Self {
>> + value: Opaque::uninit(),
>> + init: Atomic::new(0),
>> + }
>> + }
>> +
>> + /// Get a reference to the contained object.
>> + ///
>> + /// Returns [`None`] if this [`OnceLock`] is empty.
>> + pub fn as_ref(&self) -> Option<&T> {
>> + if self.init.load(Acquire) == 2 {
>> + // SAFETY: As determined by the load above, the object is ready for shared access.
>> + Some(unsafe { &*self.value.get() })
>> + } else {
>> + None
>> + }
>> + }
>> +
>> + /// Populate the [`OnceLock`].
>> + ///
>> + /// Returns `true` if the [`OnceLock`] was successfully populated.
>> + pub fn populate(&self, value: T) -> bool {
>> + // INVARIANT: We obtain exclusive access to the contained allocation and write 1 to
>> + // `init`.
>> + if let Ok(0) = self.init.cmpxchg(0, 1, Acquire) {
>
> This acquire can be Relaxed. All other accesses to self.value
> synchronize with the release store below, so you do not need acquire
> here to obtain exclusive access.
Right, thanks.
Best regards,
Andreas Hindborg
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v14 1/7] rust: sync: add `OnceLock`
2025-07-02 13:54 ` Andreas Hindborg
@ 2025-07-02 14:50 ` Alice Ryhl
2025-07-03 7:51 ` Andreas Hindborg
0 siblings, 1 reply; 34+ messages in thread
From: Alice Ryhl @ 2025-07-02 14:50 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Masahiro Yamada, Nathan Chancellor,
Luis Chamberlain, Danilo Krummrich, Benno Lossin, Nicolas Schier,
Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
On Wed, Jul 2, 2025 at 3:54 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Alice Ryhl" <aliceryhl@google.com> writes:
>
> > On Wed, Jul 2, 2025 at 3:19 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>
> >> Introduce the `OnceLock` type, a container that can only be written once.
> >> The container uses an internal atomic to synchronize writes to the internal
> >> value.
> >>
> >> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> >
> > This type provides no way to wait for initialization to finish if it's
> > ongoing. Do you not need that?
>
> I don't, and in my use case it would cause a deadlock to wait. Anyway,
> it might be useful to others. Would you add it now, or wait for a user?
Waiting would require additional fields so it should probably be a
different type. It's more that we probably want the OnceLock name for
that other type for consistency with stdlib, so perhaps this should be
renamed? The name could be SetOnce or similar.
Alice
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v14 1/7] rust: sync: add `OnceLock`
2025-07-02 14:50 ` Alice Ryhl
@ 2025-07-03 7:51 ` Andreas Hindborg
0 siblings, 0 replies; 34+ messages in thread
From: Andreas Hindborg @ 2025-07-03 7:51 UTC (permalink / raw)
To: Alice Ryhl
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Masahiro Yamada, Nathan Chancellor,
Luis Chamberlain, Danilo Krummrich, Benno Lossin, Nicolas Schier,
Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
"Alice Ryhl" <aliceryhl@google.com> writes:
> On Wed, Jul 2, 2025 at 3:54 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> "Alice Ryhl" <aliceryhl@google.com> writes:
>>
>> > On Wed, Jul 2, 2025 at 3:19 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>> >>
>> >> Introduce the `OnceLock` type, a container that can only be written once.
>> >> The container uses an internal atomic to synchronize writes to the internal
>> >> value.
>> >>
>> >> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> >
>> > This type provides no way to wait for initialization to finish if it's
>> > ongoing. Do you not need that?
>>
>> I don't, and in my use case it would cause a deadlock to wait. Anyway,
>> it might be useful to others. Would you add it now, or wait for a user?
>
> Waiting would require additional fields so it should probably be a
> different type.
That depends on the kind of waiting. If we do unfair waiting, with
spinning, we can have unlimited waiters with this type. We can also use
the remaining 29 bits of the atomic to encode ordering to get fair
queuing for spinning waiters.
Putting waiters to sleep would require more fields.
> It's more that we probably want the OnceLock name for
> that other type for consistency with stdlib, so perhaps this should be
> renamed? The name could be SetOnce or similar.
The feature set is very similar to `std::sync::OnceLock`, that is why I
picked that name. We can expand this to allow resetting without too much
effort, and we can do fair waiting with spinning for a reasonable amount
of waiters.
But I am also OK with changing the name to `SetOnce` if we envision a
`OnceLock` with thread sleep blocking at some point.
Best regards,
Andreas Hindborg
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v14 1/7] rust: sync: add `OnceLock`
2025-07-02 13:18 ` [PATCH v14 1/7] rust: sync: add `OnceLock` Andreas Hindborg
2025-07-02 13:32 ` Alice Ryhl
@ 2025-07-02 15:07 ` Benno Lossin
2025-07-02 15:27 ` Alice Ryhl
2025-07-03 9:03 ` Andreas Hindborg
2025-07-03 9:36 ` Wren Turkal
2 siblings, 2 replies; 34+ messages in thread
From: Benno Lossin @ 2025-07-02 15:07 UTC (permalink / raw)
To: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
On Wed Jul 2, 2025 at 3:18 PM CEST, Andreas Hindborg wrote:
> Introduce the `OnceLock` type, a container that can only be written once.
> The container uses an internal atomic to synchronize writes to the internal
> value.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/sync.rs | 1 +
> rust/kernel/sync/once_lock.rs | 104 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 105 insertions(+)
>
> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> index c7c0e552bafe..f2ee07315091 100644
> --- a/rust/kernel/sync.rs
> +++ b/rust/kernel/sync.rs
> @@ -15,6 +15,7 @@
> mod condvar;
> pub mod lock;
> mod locked_by;
> +pub mod once_lock;
As Alice already said, we should reexport the type. And then make the
module private, no need to have `kernel::sync::OnceLock` and
`kernel::sync::once_lock::OnceLock`...
Also, I agree with the name change to `SetOnce` or something similar.
> pub mod poll;
> pub mod rcu;
>
> diff --git a/rust/kernel/sync/once_lock.rs b/rust/kernel/sync/once_lock.rs
> new file mode 100644
> index 000000000000..cd311bea3919
> --- /dev/null
> +++ b/rust/kernel/sync/once_lock.rs
> @@ -0,0 +1,104 @@
> +//! A container that can be initialized at most once.
> +
> +use super::atomic::ordering::Acquire;
> +use super::atomic::ordering::Release;
> +use super::atomic::Atomic;
> +use kernel::types::Opaque;
> +
> +/// A container that can be populated at most once. Thread safe.
> +///
> +/// Once the a [`OnceLock`] is populated, it remains populated by the same object for the
> +/// lifetime `Self`.
> +///
> +/// # Invariants
> +///
> +/// `init` tracks the state of the container:
> +///
> +/// - If the container is empty, `init` is `0`.
> +/// - If the container is mutably accessed, `init` is `1`.
I think we should swap the order and change the ifs to iffs:
- `init == 0` iff the container is empty.
- `init == 1` iff the container is being accessed mutably.
> +/// - If the container is populated and ready for shared access, `init` is `2`.
You also need that `init` is only increased and never decreases.
Otherwise you could read a `2` and then access the value, but `init`
changed under your nose to `0`.
Then the INVARIANT comments below also need to be updated.
> +///
> +/// # Example
> +///
> +/// ```
> +/// # use kernel::sync::once_lock::OnceLock;
> +/// let value = OnceLock::new();
> +/// assert_eq!(None, value.as_ref());
> +///
> +/// let status = value.populate(42u8);
> +/// assert_eq!(true, status);
> +/// assert_eq!(Some(&42u8), value.as_ref());
> +/// assert_eq!(Some(42u8), value.copy());
> +///
> +/// let status = value.populate(101u8);
> +/// assert_eq!(false, status);
> +/// assert_eq!(Some(&42u8), value.as_ref());
> +/// assert_eq!(Some(42u8), value.copy());
> +/// ```
> +pub struct OnceLock<T> {
> + init: Atomic<u32>,
> + value: Opaque<T>,
> +}
> +
> +impl<T> Default for OnceLock<T> {
> + fn default() -> Self {
> + Self::new()
> + }
> +}
> +
> +impl<T> OnceLock<T> {
> + /// Create a new [`OnceLock`].
> + ///
> + /// The returned instance will be empty.
> + pub const fn new() -> Self {
> + // INVARIANT: The container is empty and we set `init` to `0`.
> + Self {
> + value: Opaque::uninit(),
> + init: Atomic::new(0),
> + }
> + }
> +
> + /// Get a reference to the contained object.
> + ///
> + /// Returns [`None`] if this [`OnceLock`] is empty.
> + pub fn as_ref(&self) -> Option<&T> {
> + if self.init.load(Acquire) == 2 {
> + // SAFETY: As determined by the load above, the object is ready for shared access.
// SAFETY: By the safety requirements of `Self`, `self.init == 2` means that `self.value` contains
// a valid value.
> + Some(unsafe { &*self.value.get() })
> + } else {
> + None
> + }
> + }
> +
> + /// Populate the [`OnceLock`].
> + ///
> + /// Returns `true` if the [`OnceLock`] was successfully populated.
> + pub fn populate(&self, value: T) -> bool {
> + // INVARIANT: We obtain exclusive access to the contained allocation and write 1 to
> + // `init`.
> + if let Ok(0) = self.init.cmpxchg(0, 1, Acquire) {
> + // SAFETY: We obtained exclusive access to the contained object.
> + unsafe { core::ptr::write(self.value.get(), value) };
> + // INVARIANT: We release our exclusive access and transition the object to shared
> + // access.
> + self.init.store(2, Release);
> + true
> + } else {
> + false
> + }
> + }
> +}
> +
> +impl<T: Copy> OnceLock<T> {
> + /// Get a copy of the contained object.
> + ///
> + /// Returns [`None`] if the [`OnceLock`] is empty.
> + pub fn copy(&self) -> Option<T> {
> + if self.init.load(Acquire) == 2 {
> + // SAFETY: As determined by the load above, the object is ready for shared access.
> + Some(unsafe { *self.value.get() })
> + } else {
> + None
> + }
The impl can just be:
self.as_ref().copied()
Would it make sense for this function to take `self` instead & we make
the `OnceLock` also `Copy` if `T: Copy`? Maybe not...
> + }
> +}
You can move this method into the block above and just add `where T:
Copy` on the method.
---
Cheers,
Benno
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v14 1/7] rust: sync: add `OnceLock`
2025-07-02 15:07 ` Benno Lossin
@ 2025-07-02 15:27 ` Alice Ryhl
2025-07-02 15:40 ` Benno Lossin
2025-07-03 9:03 ` Andreas Hindborg
1 sibling, 1 reply; 34+ messages in thread
From: Alice Ryhl @ 2025-07-02 15:27 UTC (permalink / raw)
To: Benno Lossin
Cc: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Masahiro Yamada, Nathan Chancellor,
Luis Chamberlain, Danilo Krummrich, Nicolas Schier, Trevor Gross,
Adam Bratschi-Kaye, rust-for-linux, linux-kernel, linux-kbuild,
Petr Pavlu, Sami Tolvanen, Daniel Gomez, Simona Vetter, Greg KH,
Fiona Behrens, Daniel Almeida, linux-modules
On Wed, Jul 2, 2025 at 5:07 PM Benno Lossin <lossin@kernel.org> wrote:
>
> On Wed Jul 2, 2025 at 3:18 PM CEST, Andreas Hindborg wrote:
> > +impl<T: Copy> OnceLock<T> {
> > + /// Get a copy of the contained object.
> > + ///
> > + /// Returns [`None`] if the [`OnceLock`] is empty.
> > + pub fn copy(&self) -> Option<T> {
> > + if self.init.load(Acquire) == 2 {
> > + // SAFETY: As determined by the load above, the object is ready for shared access.
> > + Some(unsafe { *self.value.get() })
> > + } else {
> > + None
> > + }
>
> The impl can just be:
>
> self.as_ref().copied()
>
> Would it make sense for this function to take `self` instead & we make
> the `OnceLock` also `Copy` if `T: Copy`? Maybe not...
Atomics are not Copy.
Alice
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v14 1/7] rust: sync: add `OnceLock`
2025-07-02 15:27 ` Alice Ryhl
@ 2025-07-02 15:40 ` Benno Lossin
0 siblings, 0 replies; 34+ messages in thread
From: Benno Lossin @ 2025-07-02 15:40 UTC (permalink / raw)
To: Alice Ryhl
Cc: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Masahiro Yamada, Nathan Chancellor,
Luis Chamberlain, Danilo Krummrich, Nicolas Schier, Trevor Gross,
Adam Bratschi-Kaye, rust-for-linux, linux-kernel, linux-kbuild,
Petr Pavlu, Sami Tolvanen, Daniel Gomez, Simona Vetter, Greg KH,
Fiona Behrens, Daniel Almeida, linux-modules
On Wed Jul 2, 2025 at 5:27 PM CEST, Alice Ryhl wrote:
> On Wed, Jul 2, 2025 at 5:07 PM Benno Lossin <lossin@kernel.org> wrote:
>> On Wed Jul 2, 2025 at 3:18 PM CEST, Andreas Hindborg wrote:
>> > +impl<T: Copy> OnceLock<T> {
>> > + /// Get a copy of the contained object.
>> > + ///
>> > + /// Returns [`None`] if the [`OnceLock`] is empty.
>> > + pub fn copy(&self) -> Option<T> {
>> > + if self.init.load(Acquire) == 2 {
>> > + // SAFETY: As determined by the load above, the object is ready for shared access.
>> > + Some(unsafe { *self.value.get() })
>> > + } else {
>> > + None
>> > + }
>>
>> The impl can just be:
>>
>> self.as_ref().copied()
>>
>> Would it make sense for this function to take `self` instead & we make
>> the `OnceLock` also `Copy` if `T: Copy`? Maybe not...
>
> Atomics are not Copy.
Ah right... Yeah it probably also isn't useful.
---
Cheers,
Benno
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v14 1/7] rust: sync: add `OnceLock`
2025-07-02 15:07 ` Benno Lossin
2025-07-02 15:27 ` Alice Ryhl
@ 2025-07-03 9:03 ` Andreas Hindborg
2025-07-03 9:42 ` Benno Lossin
1 sibling, 1 reply; 34+ messages in thread
From: Andreas Hindborg @ 2025-07-03 9:03 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
"Benno Lossin" <lossin@kernel.org> writes:
> On Wed Jul 2, 2025 at 3:18 PM CEST, Andreas Hindborg wrote:
>> Introduce the `OnceLock` type, a container that can only be written once.
>> The container uses an internal atomic to synchronize writes to the internal
>> value.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>> rust/kernel/sync.rs | 1 +
>> rust/kernel/sync/once_lock.rs | 104 ++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 105 insertions(+)
>>
>> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
>> index c7c0e552bafe..f2ee07315091 100644
>> --- a/rust/kernel/sync.rs
>> +++ b/rust/kernel/sync.rs
>> @@ -15,6 +15,7 @@
>> mod condvar;
>> pub mod lock;
>> mod locked_by;
>> +pub mod once_lock;
>
> As Alice already said, we should reexport the type. And then make the
> module private, no need to have `kernel::sync::OnceLock` and
> `kernel::sync::once_lock::OnceLock`...
Will do.
>
> Also, I agree with the name change to `SetOnce` or something similar.
I'm OK with that, but please see my comments on Alice suggestion.
>
>> pub mod poll;
>> pub mod rcu;
>>
>> diff --git a/rust/kernel/sync/once_lock.rs b/rust/kernel/sync/once_lock.rs
>> new file mode 100644
>> index 000000000000..cd311bea3919
>> --- /dev/null
>> +++ b/rust/kernel/sync/once_lock.rs
>> @@ -0,0 +1,104 @@
>> +//! A container that can be initialized at most once.
>> +
>> +use super::atomic::ordering::Acquire;
>> +use super::atomic::ordering::Release;
>> +use super::atomic::Atomic;
>> +use kernel::types::Opaque;
>> +
>> +/// A container that can be populated at most once. Thread safe.
>> +///
>> +/// Once the a [`OnceLock`] is populated, it remains populated by the same object for the
>> +/// lifetime `Self`.
>> +///
>> +/// # Invariants
>> +///
>> +/// `init` tracks the state of the container:
>> +///
>> +/// - If the container is empty, `init` is `0`.
>> +/// - If the container is mutably accessed, `init` is `1`.
>
> I think we should swap the order and change the ifs to iffs:
>
> - `init == 0` iff the container is empty.
> - `init == 1` iff the container is being accessed mutably.
Right, that is better, but I will expand "iff".
>
>> +/// - If the container is populated and ready for shared access, `init` is `2`.
>
> You also need that `init` is only increased and never decreases.
> Otherwise you could read a `2` and then access the value, but `init`
> changed under your nose to `0`.
>
> Then the INVARIANT comments below also need to be updated.
OK.
>
>> +///
>> +/// # Example
>> +///
>> +/// ```
>> +/// # use kernel::sync::once_lock::OnceLock;
>> +/// let value = OnceLock::new();
>> +/// assert_eq!(None, value.as_ref());
>> +///
>> +/// let status = value.populate(42u8);
>> +/// assert_eq!(true, status);
>> +/// assert_eq!(Some(&42u8), value.as_ref());
>> +/// assert_eq!(Some(42u8), value.copy());
>> +///
>> +/// let status = value.populate(101u8);
>> +/// assert_eq!(false, status);
>> +/// assert_eq!(Some(&42u8), value.as_ref());
>> +/// assert_eq!(Some(42u8), value.copy());
>> +/// ```
>> +pub struct OnceLock<T> {
>> + init: Atomic<u32>,
>> + value: Opaque<T>,
>> +}
>> +
>> +impl<T> Default for OnceLock<T> {
>> + fn default() -> Self {
>> + Self::new()
>> + }
>> +}
>> +
>> +impl<T> OnceLock<T> {
>> + /// Create a new [`OnceLock`].
>> + ///
>> + /// The returned instance will be empty.
>> + pub const fn new() -> Self {
>> + // INVARIANT: The container is empty and we set `init` to `0`.
>> + Self {
>> + value: Opaque::uninit(),
>> + init: Atomic::new(0),
>> + }
>> + }
>> +
>> + /// Get a reference to the contained object.
>> + ///
>> + /// Returns [`None`] if this [`OnceLock`] is empty.
>> + pub fn as_ref(&self) -> Option<&T> {
>> + if self.init.load(Acquire) == 2 {
>> + // SAFETY: As determined by the load above, the object is ready for shared access.
>
> // SAFETY: By the safety requirements of `Self`, `self.init == 2` means that `self.value` contains
> // a valid value.
By the *type invariants* I guess?
>
>> + Some(unsafe { &*self.value.get() })
>> + } else {
>> + None
>> + }
>> + }
>> +
>> + /// Populate the [`OnceLock`].
>> + ///
>> + /// Returns `true` if the [`OnceLock`] was successfully populated.
>> + pub fn populate(&self, value: T) -> bool {
>> + // INVARIANT: We obtain exclusive access to the contained allocation and write 1 to
>> + // `init`.
>> + if let Ok(0) = self.init.cmpxchg(0, 1, Acquire) {
>> + // SAFETY: We obtained exclusive access to the contained object.
>> + unsafe { core::ptr::write(self.value.get(), value) };
>> + // INVARIANT: We release our exclusive access and transition the object to shared
>> + // access.
>> + self.init.store(2, Release);
>> + true
>> + } else {
>> + false
>> + }
>> + }
>> +}
>> +
>> +impl<T: Copy> OnceLock<T> {
>> + /// Get a copy of the contained object.
>> + ///
>> + /// Returns [`None`] if the [`OnceLock`] is empty.
>> + pub fn copy(&self) -> Option<T> {
>> + if self.init.load(Acquire) == 2 {
>> + // SAFETY: As determined by the load above, the object is ready for shared access.
>> + Some(unsafe { *self.value.get() })
>> + } else {
>> + None
>> + }
>
> The impl can just be:
>
> self.as_ref().copied()
Nice. I was thinking of dropping this method and just have callers do
my_once_lock.as_ref().map(|v| v.copied())
What do you think?
Best regards,
Andreas Hindborg
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v14 1/7] rust: sync: add `OnceLock`
2025-07-03 9:03 ` Andreas Hindborg
@ 2025-07-03 9:42 ` Benno Lossin
2025-07-03 16:25 ` Andreas Hindborg
0 siblings, 1 reply; 34+ messages in thread
From: Benno Lossin @ 2025-07-03 9:42 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
On Thu Jul 3, 2025 at 11:03 AM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> On Wed Jul 2, 2025 at 3:18 PM CEST, Andreas Hindborg wrote:
>>> +///
>>> +/// # Example
>>> +///
>>> +/// ```
>>> +/// # use kernel::sync::once_lock::OnceLock;
>>> +/// let value = OnceLock::new();
>>> +/// assert_eq!(None, value.as_ref());
>>> +///
>>> +/// let status = value.populate(42u8);
>>> +/// assert_eq!(true, status);
>>> +/// assert_eq!(Some(&42u8), value.as_ref());
>>> +/// assert_eq!(Some(42u8), value.copy());
>>> +///
>>> +/// let status = value.populate(101u8);
>>> +/// assert_eq!(false, status);
>>> +/// assert_eq!(Some(&42u8), value.as_ref());
>>> +/// assert_eq!(Some(42u8), value.copy());
>>> +/// ```
>>> +pub struct OnceLock<T> {
>>> + init: Atomic<u32>,
>>> + value: Opaque<T>,
>>> +}
>>> +
>>> +impl<T> Default for OnceLock<T> {
>>> + fn default() -> Self {
>>> + Self::new()
>>> + }
>>> +}
>>> +
>>> +impl<T> OnceLock<T> {
>>> + /// Create a new [`OnceLock`].
>>> + ///
>>> + /// The returned instance will be empty.
>>> + pub const fn new() -> Self {
>>> + // INVARIANT: The container is empty and we set `init` to `0`.
>>> + Self {
>>> + value: Opaque::uninit(),
>>> + init: Atomic::new(0),
>>> + }
>>> + }
>>> +
>>> + /// Get a reference to the contained object.
>>> + ///
>>> + /// Returns [`None`] if this [`OnceLock`] is empty.
>>> + pub fn as_ref(&self) -> Option<&T> {
>>> + if self.init.load(Acquire) == 2 {
>>> + // SAFETY: As determined by the load above, the object is ready for shared access.
>>
>> // SAFETY: By the safety requirements of `Self`, `self.init == 2` means that `self.value` contains
>> // a valid value.
>
> By the *type invariants* I guess?
Oh yeah.
>>> + Some(unsafe { &*self.value.get() })
>>> + } else {
>>> + None
>>> + }
>>> + }
>>> +
>>> + /// Populate the [`OnceLock`].
>>> + ///
>>> + /// Returns `true` if the [`OnceLock`] was successfully populated.
>>> + pub fn populate(&self, value: T) -> bool {
>>> + // INVARIANT: We obtain exclusive access to the contained allocation and write 1 to
>>> + // `init`.
>>> + if let Ok(0) = self.init.cmpxchg(0, 1, Acquire) {
>>> + // SAFETY: We obtained exclusive access to the contained object.
>>> + unsafe { core::ptr::write(self.value.get(), value) };
>>> + // INVARIANT: We release our exclusive access and transition the object to shared
>>> + // access.
>>> + self.init.store(2, Release);
>>> + true
>>> + } else {
>>> + false
>>> + }
>>> + }
>>> +}
>>> +
>>> +impl<T: Copy> OnceLock<T> {
>>> + /// Get a copy of the contained object.
>>> + ///
>>> + /// Returns [`None`] if the [`OnceLock`] is empty.
>>> + pub fn copy(&self) -> Option<T> {
>>> + if self.init.load(Acquire) == 2 {
>>> + // SAFETY: As determined by the load above, the object is ready for shared access.
>>> + Some(unsafe { *self.value.get() })
>>> + } else {
>>> + None
>>> + }
>>
>> The impl can just be:
>>
>> self.as_ref().copied()
>
> Nice. I was thinking of dropping this method and just have callers do
>
> my_once_lock.as_ref().map(|v| v.copied())
>
> What do you think?
There is `Option::copied`, so no need for the `.map` call. I don't
really have a preference, if users always want to access it by-value,
then we should have `copy`.
---
Cheers,
Benno
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v14 1/7] rust: sync: add `OnceLock`
2025-07-03 9:42 ` Benno Lossin
@ 2025-07-03 16:25 ` Andreas Hindborg
2025-07-03 20:41 ` Benno Lossin
0 siblings, 1 reply; 34+ messages in thread
From: Andreas Hindborg @ 2025-07-03 16:25 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
"Benno Lossin" <lossin@kernel.org> writes:
> On Thu Jul 3, 2025 at 11:03 AM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> On Wed Jul 2, 2025 at 3:18 PM CEST, Andreas Hindborg wrote:
[...]
>>>> + Some(unsafe { &*self.value.get() })
>>>> + } else {
>>>> + None
>>>> + }
>>>> + }
>>>> +
>>>> + /// Populate the [`OnceLock`].
>>>> + ///
>>>> + /// Returns `true` if the [`OnceLock`] was successfully populated.
>>>> + pub fn populate(&self, value: T) -> bool {
>>>> + // INVARIANT: We obtain exclusive access to the contained allocation and write 1 to
>>>> + // `init`.
>>>> + if let Ok(0) = self.init.cmpxchg(0, 1, Acquire) {
>>>> + // SAFETY: We obtained exclusive access to the contained object.
>>>> + unsafe { core::ptr::write(self.value.get(), value) };
>>>> + // INVARIANT: We release our exclusive access and transition the object to shared
>>>> + // access.
>>>> + self.init.store(2, Release);
>>>> + true
>>>> + } else {
>>>> + false
>>>> + }
>>>> + }
>>>> +}
>>>> +
>>>> +impl<T: Copy> OnceLock<T> {
>>>> + /// Get a copy of the contained object.
>>>> + ///
>>>> + /// Returns [`None`] if the [`OnceLock`] is empty.
>>>> + pub fn copy(&self) -> Option<T> {
>>>> + if self.init.load(Acquire) == 2 {
>>>> + // SAFETY: As determined by the load above, the object is ready for shared access.
>>>> + Some(unsafe { *self.value.get() })
>>>> + } else {
>>>> + None
>>>> + }
>>>
>>> The impl can just be:
>>>
>>> self.as_ref().copied()
>>
>> Nice. I was thinking of dropping this method and just have callers do
>>
>> my_once_lock.as_ref().map(|v| v.copied())
>>
>> What do you think?
>
> There is `Option::copied`, so no need for the `.map` call.
Cool.
> I don't
> really have a preference, if users always want to access it by-value,
> then we should have `copy`.
But should it be `copy` or `copied` like `Option`?
Best regards,
Andreas Hindborg
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v14 1/7] rust: sync: add `OnceLock`
2025-07-03 16:25 ` Andreas Hindborg
@ 2025-07-03 20:41 ` Benno Lossin
0 siblings, 0 replies; 34+ messages in thread
From: Benno Lossin @ 2025-07-03 20:41 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
On Thu Jul 3, 2025 at 6:25 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> On Thu Jul 3, 2025 at 11:03 AM CEST, Andreas Hindborg wrote:
>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>> On Wed Jul 2, 2025 at 3:18 PM CEST, Andreas Hindborg wrote:
>>>>> +impl<T: Copy> OnceLock<T> {
>>>>> + /// Get a copy of the contained object.
>>>>> + ///
>>>>> + /// Returns [`None`] if the [`OnceLock`] is empty.
>>>>> + pub fn copy(&self) -> Option<T> {
>>>>> + if self.init.load(Acquire) == 2 {
>>>>> + // SAFETY: As determined by the load above, the object is ready for shared access.
>>>>> + Some(unsafe { *self.value.get() })
>>>>> + } else {
>>>>> + None
>>>>> + }
>>>>
>>>> The impl can just be:
>>>>
>>>> self.as_ref().copied()
>>>
>>> Nice. I was thinking of dropping this method and just have callers do
>>>
>>> my_once_lock.as_ref().map(|v| v.copied())
>>>
>>> What do you think?
>>
>> There is `Option::copied`, so no need for the `.map` call.
>
> Cool.
>
>> I don't
>> really have a preference, if users always want to access it by-value,
>> then we should have `copy`.
>
> But should it be `copy` or `copied` like `Option`?
I'd say `copy`. With `copied` I'm thinking of something that turns
`OnceLock<&T>` into `OnceLock<T>`, which is weird...
---
Cheers,
Benno
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v14 1/7] rust: sync: add `OnceLock`
2025-07-02 13:18 ` [PATCH v14 1/7] rust: sync: add `OnceLock` Andreas Hindborg
2025-07-02 13:32 ` Alice Ryhl
2025-07-02 15:07 ` Benno Lossin
@ 2025-07-03 9:36 ` Wren Turkal
2025-07-03 16:41 ` Andreas Hindborg
2 siblings, 1 reply; 34+ messages in thread
From: Wren Turkal @ 2025-07-03 9:36 UTC (permalink / raw)
To: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
On 7/2/25 6:18 AM, Andreas Hindborg wrote:
> Introduce the `OnceLock` type, a container that can only be written once.
> The container uses an internal atomic to synchronize writes to the internal
> value.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/sync.rs | 1 +
> rust/kernel/sync/once_lock.rs | 104 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 105 insertions(+)
>
> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> index c7c0e552bafe..f2ee07315091 100644
> --- a/rust/kernel/sync.rs
> +++ b/rust/kernel/sync.rs
> @@ -15,6 +15,7 @@
> mod condvar;
> pub mod lock;
> mod locked_by;
> +pub mod once_lock;
> pub mod poll;
> pub mod rcu;
>
> diff --git a/rust/kernel/sync/once_lock.rs b/rust/kernel/sync/once_lock.rs
> new file mode 100644
> index 000000000000..cd311bea3919
> --- /dev/null
> +++ b/rust/kernel/sync/once_lock.rs
> @@ -0,0 +1,104 @@
> +//! A container that can be initialized at most once.
> +
> +use super::atomic::ordering::Acquire;
> +use super::atomic::ordering::Release;
> +use super::atomic::Atomic;
> +use kernel::types::Opaque;
> +
> +/// A container that can be populated at most once. Thread safe.
> +///
> +/// Once the a [`OnceLock`] is populated, it remains populated by the same object for the
> +/// lifetime `Self`.
> +///
> +/// # Invariants
> +///
> +/// `init` tracks the state of the container:
> +///
> +/// - If the container is empty, `init` is `0`.
> +/// - If the container is mutably accessed, `init` is `1`.
> +/// - If the container is populated and ready for shared access, `init` is `2`.
> +///
> +/// # Example
> +///
> +/// ```
> +/// # use kernel::sync::once_lock::OnceLock;
> +/// let value = OnceLock::new();
> +/// assert_eq!(None, value.as_ref());
> +///
> +/// let status = value.populate(42u8);
> +/// assert_eq!(true, status);
> +/// assert_eq!(Some(&42u8), value.as_ref());
> +/// assert_eq!(Some(42u8), value.copy());
> +///
> +/// let status = value.populate(101u8);
> +/// assert_eq!(false, status);
> +/// assert_eq!(Some(&42u8), value.as_ref());
> +/// assert_eq!(Some(42u8), value.copy());
> +/// ```
> +pub struct OnceLock<T> {
> + init: Atomic<u32>,
> + value: Opaque<T>,
> +}
This type looks very much like the Once type in rust's stdlib. I am
wondering if the api could be changed to match that api. I know that
this type is trying to provide a version subset of std::sync::OnceLock
that doesn't allow resetting the type like these apis:
* https://doc.rust-lang.org/std/sync/struct.OnceLock.html#method.get_mut
* https://doc.rust-lang.org/std/sync/struct.OnceLock.html#method.take
However, these methods can only be used on mut. See here for failing
example:
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=a78e51203c5b9555e3c151e162f0acab
I think it might make more sense to match the api of the stdlib API and
maybe only implement the methods you need.
> +
> +impl<T> Default for OnceLock<T> {
> + fn default() -> Self {
> + Self::new()
> + }
> +}
Any reason not to use #[derive(Default)]?
> +
> +impl<T> OnceLock<T> {
> + /// Create a new [`OnceLock`].
> + ///
> + /// The returned instance will be empty.
> + pub const fn new() -> Self {
Like new in std OnceLock. Matches. Good.
> + // INVARIANT: The container is empty and we set `init` to `0`.
> + Self {
> + value: Opaque::uninit(),
> + init: Atomic::new(0),
> + }
> + }
> +
> + /// Get a reference to the contained object.
> + ///
> + /// Returns [`None`] if this [`OnceLock`] is empty.
> + pub fn as_ref(&self) -> Option<&T> {
Looks like the get method in the OnceLock.
> + if self.init.load(Acquire) == 2 {
> + // SAFETY: As determined by the load above, the object is ready for shared access.
> + Some(unsafe { &*self.value.get() })
> + } else {
> + None
> + }
> + }
> +
> + /// Populate the [`OnceLock`].
> + ///
> + /// Returns `true` if the [`OnceLock`] was successfully populated.
> + pub fn populate(&self, value: T) -> bool {
Looks like set in OnceLock.
Might also be worth implementing get_or_{try,}init, which get the value
while initializing.
> + // INVARIANT: We obtain exclusive access to the contained allocation and write 1 to
> + // `init`.
> + if let Ok(0) = self.init.cmpxchg(0, 1, Acquire) {
> + // SAFETY: We obtained exclusive access to the contained object.
> + unsafe { core::ptr::write(self.value.get(), value) };
> + // INVARIANT: We release our exclusive access and transition the object to shared
> + // access.
> + self.init.store(2, Release);
> + true
> + } else {
> + false
> + }
> + }
> +}
> +
> +impl<T: Copy> OnceLock<T> {
> + /// Get a copy of the contained object.
> + ///
> + /// Returns [`None`] if the [`OnceLock`] is empty.
> + pub fn copy(&self) -> Option<T> {
No equivalent in OnceLock. Similar to something like this:
x.get().copied().unwrap(); // x is a OnceLock
Example:
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=f21068e55f73722544fb5ad341bce1c5
Maybe not specifically needed?
> + if self.init.load(Acquire) == 2 {
> + // SAFETY: As determined by the load above, the object is ready for shared access.
> + Some(unsafe { *self.value.get() })
> + } else {
> + None
> + }
> + }
> +}
>
--
You're more amazing than you think!
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v14 1/7] rust: sync: add `OnceLock`
2025-07-03 9:36 ` Wren Turkal
@ 2025-07-03 16:41 ` Andreas Hindborg
0 siblings, 0 replies; 34+ messages in thread
From: Andreas Hindborg @ 2025-07-03 16:41 UTC (permalink / raw)
To: Wren Turkal
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye,
rust-for-linux, linux-kernel, linux-kbuild, Petr Pavlu,
Sami Tolvanen, Daniel Gomez, Simona Vetter, Greg KH,
Fiona Behrens, Daniel Almeida, linux-modules
"Wren Turkal" <wt@penguintechs.org> writes:
> On 7/2/25 6:18 AM, Andreas Hindborg wrote:
[...]
>> +pub struct OnceLock<T> {
>> + init: Atomic<u32>,
>> + value: Opaque<T>,
>> +}
>
> This type looks very much like the Once type in rust's stdlib. I am
> wondering if the api could be changed to match that api. I know that
> this type is trying to provide a version subset of std::sync::OnceLock
> that doesn't allow resetting the type like these apis:
>
> * https://doc.rust-lang.org/std/sync/struct.OnceLock.html#method.get_mut
> * https://doc.rust-lang.org/std/sync/struct.OnceLock.html#method.take
>
> However, these methods can only be used on mut. See here for failing
> example:
> https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=a78e51203c5b9555e3c151e162f0acab
>
> I think it might make more sense to match the api of the stdlib API and
> maybe only implement the methods you need.
I agree, it would be nice to match the names to std. But I do not like
that they have `OnceLock::set`, `OnceLock::try_init` and
`OnceLock::get_or{_try}_init`. Why is it not `OnceLock::init` or
`OnceLock::try_set`?
>
>> +
>> +impl<T> Default for OnceLock<T> {
>> + fn default() -> Self {
>> + Self::new()
>> + }
>> +}
>
> Any reason not to use #[derive(Default)]?
We don't have `Default` for neither `Atomic` or `Opaque`.
[...]
> Might also be worth implementing get_or_{try,}init, which get the value
> while initializing.
I did not have a user for those, so not adding for now. It would be dead
code. They should be fairly straight forward to add though.
>
>> + // INVARIANT: We obtain exclusive access to the contained allocation and write 1 to
>> + // `init`.
>> + if let Ok(0) = self.init.cmpxchg(0, 1, Acquire) {
>> + // SAFETY: We obtained exclusive access to the contained object.
>> + unsafe { core::ptr::write(self.value.get(), value) };
>> + // INVARIANT: We release our exclusive access and transition the object to shared
>> + // access.
>> + self.init.store(2, Release);
>> + true
>> + } else {
>> + false
>> + }
>> + }
>> +}
>> +
>> +impl<T: Copy> OnceLock<T> {
>> + /// Get a copy of the contained object.
>> + ///
>> + /// Returns [`None`] if the [`OnceLock`] is empty.
>> + pub fn copy(&self) -> Option<T> {
>
> No equivalent in OnceLock. Similar to something like this:
>
> x.get().copied().unwrap(); // x is a OnceLock
>
> Example:
> https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=f21068e55f73722544fb5ad341bce1c5
>
> Maybe not specifically needed?
I don't actually have a user for this, so I think I will drop it.
Best regards,
Andreas Hindborg
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v14 2/7] rust: str: add radix prefixed integer parsing functions
2025-07-02 13:18 [PATCH v14 0/7] rust: extend `module!` macro with integer parameter support Andreas Hindborg
2025-07-02 13:18 ` [PATCH v14 1/7] rust: sync: add `OnceLock` Andreas Hindborg
@ 2025-07-02 13:18 ` Andreas Hindborg
2025-07-02 13:18 ` [PATCH v14 3/7] rust: introduce module_param module Andreas Hindborg
` (4 subsequent siblings)
6 siblings, 0 replies; 34+ messages in thread
From: Andreas Hindborg @ 2025-07-02 13:18 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Benno Lossin, Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules, Andreas Hindborg
Add the trait `ParseInt` for parsing string representations of integers
where the string representations are optionally prefixed by a radix
specifier. Implement the trait for the primitive integer types.
Tested-by: Daniel Gomez <da.gomez@samsung.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Suggested-by: Benno Lossin <benno.lossin@proton.me>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/str.rs | 2 +
rust/kernel/str/parse_int.rs | 148 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 150 insertions(+)
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index a927db8e079c..2b6c8b4a0ae4 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -8,6 +8,8 @@
use crate::prelude::*;
+pub mod parse_int;
+
/// Byte string without UTF-8 validity guarantee.
#[repr(transparent)]
pub struct BStr([u8]);
diff --git a/rust/kernel/str/parse_int.rs b/rust/kernel/str/parse_int.rs
new file mode 100644
index 000000000000..48eb4c202984
--- /dev/null
+++ b/rust/kernel/str/parse_int.rs
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Integer parsing functions.
+//!
+//! Integer parsing functions for parsing signed and unsigned integers
+//! potentially prefixed with `0x`, `0o`, or `0b`.
+
+use crate::prelude::*;
+use crate::str::BStr;
+use core::ops::Deref;
+
+// Make `FromStrRadix` a public type with a private name. This seals
+// `ParseInt`, that is, prevents downstream users from implementing the
+// trait.
+mod private {
+ use crate::prelude::*;
+ use crate::str::BStr;
+
+ /// Trait that allows parsing a [`&BStr`] to an integer with a radix.
+ pub trait FromStrRadix: Sized {
+ /// Parse `src` to [`Self`] using radix `radix`.
+ fn from_str_radix(src: &BStr, radix: u32) -> Result<Self>;
+
+ /// Tries to convert `value` into [`Self`] and negates the resulting value.
+ fn from_u64_negated(value: u64) -> Result<Self>;
+ }
+}
+
+/// Extract the radix from an integer literal optionally prefixed with
+/// one of `0x`, `0X`, `0o`, `0O`, `0b`, `0B`, `0`.
+fn strip_radix(src: &BStr) -> (u32, &BStr) {
+ match src.deref() {
+ [b'0', b'x' | b'X', rest @ ..] => (16, rest.as_ref()),
+ [b'0', b'o' | b'O', rest @ ..] => (8, rest.as_ref()),
+ [b'0', b'b' | b'B', rest @ ..] => (2, rest.as_ref()),
+ // NOTE: We are including the leading zero to be able to parse
+ // literal `0` here. If we removed it as a radix prefix, we would
+ // not be able to parse `0`.
+ [b'0', ..] => (8, src),
+ _ => (10, src),
+ }
+}
+
+/// Trait for parsing string representations of integers.
+///
+/// Strings beginning with `0x`, `0o`, or `0b` are parsed as hex, octal, or
+/// binary respectively. Strings beginning with `0` otherwise are parsed as
+/// octal. Anything else is parsed as decimal. A leading `+` or `-` is also
+/// permitted. Any string parsed by [`kstrtol()`] or [`kstrtoul()`] will be
+/// successfully parsed.
+///
+/// [`kstrtol()`]: https://docs.kernel.org/core-api/kernel-api.html#c.kstrtol
+/// [`kstrtoul()`]: https://docs.kernel.org/core-api/kernel-api.html#c.kstrtoul
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::str::parse_int::ParseInt;
+/// # use kernel::b_str;
+///
+/// assert_eq!(Ok(0u8), u8::from_str(b_str!("0")));
+///
+/// assert_eq!(Ok(0xa2u8), u8::from_str(b_str!("0xa2")));
+/// assert_eq!(Ok(-0xa2i32), i32::from_str(b_str!("-0xa2")));
+///
+/// assert_eq!(Ok(-0o57i8), i8::from_str(b_str!("-0o57")));
+/// assert_eq!(Ok(0o57i8), i8::from_str(b_str!("057")));
+///
+/// assert_eq!(Ok(0b1001i16), i16::from_str(b_str!("0b1001")));
+/// assert_eq!(Ok(-0b1001i16), i16::from_str(b_str!("-0b1001")));
+///
+/// assert_eq!(Ok(127i8), i8::from_str(b_str!("127")));
+/// assert!(i8::from_str(b_str!("128")).is_err());
+/// assert_eq!(Ok(-128i8), i8::from_str(b_str!("-128")));
+/// assert!(i8::from_str(b_str!("-129")).is_err());
+/// assert_eq!(Ok(255u8), u8::from_str(b_str!("255")));
+/// assert!(u8::from_str(b_str!("256")).is_err());
+/// ```
+pub trait ParseInt: private::FromStrRadix + TryFrom<u64> {
+ /// Parse a string according to the description in [`Self`].
+ fn from_str(src: &BStr) -> Result<Self> {
+ match src.deref() {
+ [b'-', rest @ ..] => {
+ let (radix, digits) = strip_radix(rest.as_ref());
+ // 2's complement values range from -2^(b-1) to 2^(b-1)-1.
+ // So if we want to parse negative numbers as positive and
+ // later multiply by -1, we have to parse into a larger
+ // integer. We choose `u64` as sufficiently large.
+ //
+ // NOTE: 128 bit integers are not available on all
+ // platforms, hence the choice of 64 bits.
+ let val =
+ u64::from_str_radix(core::str::from_utf8(digits).map_err(|_| EINVAL)?, radix)
+ .map_err(|_| EINVAL)?;
+ Self::from_u64_negated(val)
+ }
+ _ => {
+ let (radix, digits) = strip_radix(src);
+ Self::from_str_radix(digits, radix).map_err(|_| EINVAL)
+ }
+ }
+ }
+}
+
+macro_rules! impl_parse_int {
+ ($($ty:ty),*) => {
+ $(
+ impl private::FromStrRadix for $ty {
+ fn from_str_radix(src: &BStr, radix: u32) -> Result<Self> {
+ <$ty>::from_str_radix(core::str::from_utf8(src).map_err(|_| EINVAL)?, radix)
+ .map_err(|_| EINVAL)
+ }
+
+ fn from_u64_negated(value: u64) -> Result<Self> {
+ const ABS_MIN: u64 = {
+ #[allow(unused_comparisons)]
+ if <$ty>::MIN < 0 {
+ 1u64 << (<$ty>::BITS - 1)
+ } else {
+ 0
+ }
+ };
+
+ if value > ABS_MIN {
+ return Err(EINVAL);
+ }
+
+ if value == ABS_MIN {
+ return Ok(<$ty>::MIN);
+ }
+
+ // SAFETY: The above checks guarantee that `value` fits into `Self`:
+ // - if `Self` is unsigned, then `ABS_MIN == 0` and thus we have returned above
+ // (either `EINVAL` or `MIN`).
+ // - if `Self` is signed, then we have that `0 <= value < ABS_MIN`. And since
+ // `ABS_MIN - 1` fits into `Self` by construction, `value` also does.
+ let value: Self = unsafe { value.try_into().unwrap_unchecked() };
+
+ Ok((!value).wrapping_add(1))
+ }
+ }
+
+ impl ParseInt for $ty {}
+ )*
+ };
+}
+
+impl_parse_int![i8, u8, i16, u16, i32, u32, i64, u64, isize, usize];
--
2.47.2
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v14 3/7] rust: introduce module_param module
2025-07-02 13:18 [PATCH v14 0/7] rust: extend `module!` macro with integer parameter support Andreas Hindborg
2025-07-02 13:18 ` [PATCH v14 1/7] rust: sync: add `OnceLock` Andreas Hindborg
2025-07-02 13:18 ` [PATCH v14 2/7] rust: str: add radix prefixed integer parsing functions Andreas Hindborg
@ 2025-07-02 13:18 ` Andreas Hindborg
2025-07-02 15:21 ` Benno Lossin
2025-07-03 21:49 ` Danilo Krummrich
2025-07-02 13:18 ` [PATCH v14 4/7] rust: module: use a reference in macros::module::module Andreas Hindborg
` (3 subsequent siblings)
6 siblings, 2 replies; 34+ messages in thread
From: Andreas Hindborg @ 2025-07-02 13:18 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Benno Lossin, Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules, Andreas Hindborg
Add types and traits for interfacing the C moduleparam API.
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/lib.rs | 1 +
rust/kernel/module_param.rs | 191 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 192 insertions(+)
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 6b4774b2b1c3..2b439ea06185 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -87,6 +87,7 @@
pub mod list;
pub mod miscdevice;
pub mod mm;
+pub mod module_param;
#[cfg(CONFIG_NET)]
pub mod net;
pub mod of;
diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs
new file mode 100644
index 000000000000..ca4be7e45ff7
--- /dev/null
+++ b/rust/kernel/module_param.rs
@@ -0,0 +1,191 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Support for module parameters.
+//!
+//! C header: [`include/linux/moduleparam.h`](srctree/include/linux/moduleparam.h)
+
+use crate::prelude::*;
+use crate::str::BStr;
+use bindings;
+use kernel::sync::once_lock::OnceLock;
+
+/// Newtype to make `bindings::kernel_param` [`Sync`].
+#[repr(transparent)]
+#[doc(hidden)]
+pub struct RacyKernelParam(bindings::kernel_param);
+
+impl RacyKernelParam {
+ #[doc(hidden)]
+ pub const fn new(val: bindings::kernel_param) -> Self {
+ Self(val)
+ }
+}
+
+// SAFETY: C kernel handles serializing access to this type. We never access it
+// from Rust module.
+unsafe impl Sync for RacyKernelParam {}
+
+/// Types that can be used for module parameters.
+// NOTE: This trait is `Copy` because drop could produce unsoundness during teardown.
+pub trait ModuleParam: Sized + Copy {
+ /// The [`ModuleParam`] will be used by the kernel module through this type.
+ ///
+ /// This may differ from `Self` if, for example, `Self` needs to track
+ /// ownership without exposing it or allocate extra space for other possible
+ /// parameter values.
+ // This is required to support string parameters in the future.
+ type Value: ?Sized;
+
+ /// Parse a parameter argument into the parameter value.
+ fn try_from_param_arg(arg: &BStr) -> Result<Self>;
+}
+
+/// Set the module parameter from a string.
+///
+/// Used to set the parameter value at kernel initialization, when loading
+/// the module or when set through `sysfs`.
+///
+/// See `struct kernel_param_ops.set`.
+///
+/// # Safety
+///
+/// - If `val` is non-null then it must point to a valid null-terminated string that must be valid
+/// for reads for the duration of the call.
+/// - `param` must be a pointer to a `bindings::kernel_param` initialized by the rust module macro.
+/// The pointee must be valid for reads for the duration of the call.
+///
+/// # Note
+///
+/// - The safety requirements are satisfied by C API contract when this function is invoked by the
+/// module subsystem C code.
+/// - Currently, we only support read-only parameters that are not readable from `sysfs`. Thus, this
+/// function is only called at kernel initialization time, or at module load time, and we have
+/// exclusive access to the parameter for the duration of the function.
+///
+/// [`module!`]: macros::module
+unsafe extern "C" fn set_param<T>(val: *const c_char, param: *const bindings::kernel_param) -> c_int
+where
+ T: ModuleParam,
+{
+ // NOTE: If we start supporting arguments without values, val _is_ allowed
+ // to be null here.
+ if val.is_null() {
+ // TODO: Use pr_warn_once available.
+ crate::pr_warn!("Null pointer passed to `module_param::set_param`");
+ return EINVAL.to_errno();
+ }
+
+ // SAFETY: By function safety requirement, val is non-null, null-terminated
+ // and valid for reads for the duration of this function.
+ let arg = unsafe { CStr::from_char_ptr(val) };
+
+ crate::error::from_result(|| {
+ let new_value = T::try_from_param_arg(arg)?;
+
+ // SAFETY: By function safety requirements, this access is safe.
+ let container = unsafe { &*((*param).__bindgen_anon_1.arg as *mut OnceLock<T>) };
+
+ container
+ .populate(new_value)
+ .then_some(0)
+ .ok_or(kernel::error::code::EEXIST)
+ })
+}
+
+macro_rules! impl_int_module_param {
+ ($ty:ident) => {
+ impl ModuleParam for $ty {
+ type Value = $ty;
+
+ fn try_from_param_arg(arg: &BStr) -> Result<Self> {
+ <$ty as crate::str::parse_int::ParseInt>::from_str(arg)
+ }
+ }
+ };
+}
+
+impl_int_module_param!(i8);
+impl_int_module_param!(u8);
+impl_int_module_param!(i16);
+impl_int_module_param!(u16);
+impl_int_module_param!(i32);
+impl_int_module_param!(u32);
+impl_int_module_param!(i64);
+impl_int_module_param!(u64);
+impl_int_module_param!(isize);
+impl_int_module_param!(usize);
+
+/// A wrapper for kernel parameters.
+///
+/// This type is instantiated by the [`module!`] macro when module parameters are
+/// defined. You should never need to instantiate this type directly.
+///
+/// Note: This type is `pub` because it is used by module crates to access
+/// parameter values.
+pub struct ModuleParamAccess<T> {
+ value: OnceLock<T>,
+ default: T,
+}
+
+// SAFETY: We only create shared references to the contents of this container,
+// so if `T` is `Sync`, so is `ModuleParamAccess`.
+unsafe impl<T: Sync> Sync for ModuleParamAccess<T> {}
+
+impl<T> ModuleParamAccess<T> {
+ #[doc(hidden)]
+ pub const fn new(default: T) -> Self {
+ Self {
+ value: OnceLock::new(),
+ default,
+ }
+ }
+
+ /// Get a shared reference to the parameter value.
+ // Note: When sysfs access to parameters are enabled, we have to pass in a
+ // held lock guard here.
+ pub fn get(&self) -> &T {
+ self.value.as_ref().unwrap_or(&self.default)
+ }
+
+ /// Get a mutable pointer to `self`.
+ ///
+ /// NOTE: In most cases it is not safe deref the returned pointer.
+ pub const fn as_void_ptr(&self) -> *mut c_void {
+ (self as *const Self).cast_mut().cast()
+ }
+}
+
+#[doc(hidden)]
+/// Generate a static [`kernel_param_ops`](srctree/include/linux/moduleparam.h) struct.
+///
+/// # Examples
+///
+/// ```ignore
+/// make_param_ops!(
+/// /// Documentation for new param ops.
+/// PARAM_OPS_MYTYPE, // Name for the static.
+/// MyType // A type which implements [`ModuleParam`].
+/// );
+/// ```
+macro_rules! make_param_ops {
+ ($ops:ident, $ty:ty) => {
+ #[doc(hidden)]
+ pub static $ops: $crate::bindings::kernel_param_ops = $crate::bindings::kernel_param_ops {
+ flags: 0,
+ set: Some(set_param::<$ty>),
+ get: None,
+ free: None,
+ };
+ };
+}
+
+make_param_ops!(PARAM_OPS_I8, i8);
+make_param_ops!(PARAM_OPS_U8, u8);
+make_param_ops!(PARAM_OPS_I16, i16);
+make_param_ops!(PARAM_OPS_U16, u16);
+make_param_ops!(PARAM_OPS_I32, i32);
+make_param_ops!(PARAM_OPS_U32, u32);
+make_param_ops!(PARAM_OPS_I64, i64);
+make_param_ops!(PARAM_OPS_U64, u64);
+make_param_ops!(PARAM_OPS_ISIZE, isize);
+make_param_ops!(PARAM_OPS_USIZE, usize);
--
2.47.2
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v14 3/7] rust: introduce module_param module
2025-07-02 13:18 ` [PATCH v14 3/7] rust: introduce module_param module Andreas Hindborg
@ 2025-07-02 15:21 ` Benno Lossin
2025-07-04 11:45 ` Andreas Hindborg
2025-07-03 21:49 ` Danilo Krummrich
1 sibling, 1 reply; 34+ messages in thread
From: Benno Lossin @ 2025-07-02 15:21 UTC (permalink / raw)
To: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
On Wed Jul 2, 2025 at 3:18 PM CEST, Andreas Hindborg wrote:
> Add types and traits for interfacing the C moduleparam API.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
I have some nits below, but overall
Reviewed-by: Benno Lossin <lossin@kernel.org>
> ---
> rust/kernel/lib.rs | 1 +
> rust/kernel/module_param.rs | 191 ++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 192 insertions(+)
I really like how the `OnceLock` usage turned out here! Thanks for the
quick impl!
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 6b4774b2b1c3..2b439ea06185 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -87,6 +87,7 @@
> pub mod list;
> pub mod miscdevice;
> pub mod mm;
> +pub mod module_param;
> #[cfg(CONFIG_NET)]
> pub mod net;
> pub mod of;
> diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs
> new file mode 100644
> index 000000000000..ca4be7e45ff7
> --- /dev/null
> +++ b/rust/kernel/module_param.rs
> @@ -0,0 +1,191 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Support for module parameters.
> +//!
> +//! C header: [`include/linux/moduleparam.h`](srctree/include/linux/moduleparam.h)
> +
> +use crate::prelude::*;
> +use crate::str::BStr;
> +use bindings;
> +use kernel::sync::once_lock::OnceLock;
> +
> +/// Newtype to make `bindings::kernel_param` [`Sync`].
> +#[repr(transparent)]
> +#[doc(hidden)]
> +pub struct RacyKernelParam(bindings::kernel_param);
Can you remind me why this is called `Racy`? Maybe add the explainer in
a comment? (and if it's named racy, why is it okay?)
If it doesn't have a real reason, maybe it should be called
`KernelParam`?
> +
> +impl RacyKernelParam {
> + #[doc(hidden)]
> + pub const fn new(val: bindings::kernel_param) -> Self {
> + Self(val)
> + }
> +}
> +
> +// SAFETY: C kernel handles serializing access to this type. We never access it
> +// from Rust module.
> +unsafe impl Sync for RacyKernelParam {}
> +
> +/// Types that can be used for module parameters.
> +// NOTE: This trait is `Copy` because drop could produce unsoundness during teardown.
> +pub trait ModuleParam: Sized + Copy {
> + /// The [`ModuleParam`] will be used by the kernel module through this type.
> + ///
> + /// This may differ from `Self` if, for example, `Self` needs to track
> + /// ownership without exposing it or allocate extra space for other possible
> + /// parameter values.
> + // This is required to support string parameters in the future.
> + type Value: ?Sized;
This isn't used anywhere in the patchset and AFAIK the kernel is moving
away from module params, so I'm not so sure if we're going to have
strings as params.
Or do you already have those patches ready/plan to use strings? If not,
then I think we should just remove this type and when we actually need
them add it.
> +
> + /// Parse a parameter argument into the parameter value.
> + fn try_from_param_arg(arg: &BStr) -> Result<Self>;
> +}
> +
> +impl<T> ModuleParamAccess<T> {
> + #[doc(hidden)]
> + pub const fn new(default: T) -> Self {
> + Self {
> + value: OnceLock::new(),
> + default,
> + }
> + }
> +
> + /// Get a shared reference to the parameter value.
> + // Note: When sysfs access to parameters are enabled, we have to pass in a
> + // held lock guard here.
> + pub fn get(&self) -> &T {
> + self.value.as_ref().unwrap_or(&self.default)
> + }
> +
> + /// Get a mutable pointer to `self`.
> + ///
> + /// NOTE: In most cases it is not safe deref the returned pointer.
> + pub const fn as_void_ptr(&self) -> *mut c_void {
> + (self as *const Self).cast_mut().cast()
There is `core::ptr::from_ref` that we should use instead of the `as`
cast.
---
Cheers,
Benno
> + }
> +}
> +
> +#[doc(hidden)]
> +/// Generate a static [`kernel_param_ops`](srctree/include/linux/moduleparam.h) struct.
> +///
> +/// # Examples
> +///
> +/// ```ignore
> +/// make_param_ops!(
> +/// /// Documentation for new param ops.
> +/// PARAM_OPS_MYTYPE, // Name for the static.
> +/// MyType // A type which implements [`ModuleParam`].
> +/// );
> +/// ```
> +macro_rules! make_param_ops {
> + ($ops:ident, $ty:ty) => {
> + #[doc(hidden)]
> + pub static $ops: $crate::bindings::kernel_param_ops = $crate::bindings::kernel_param_ops {
> + flags: 0,
> + set: Some(set_param::<$ty>),
> + get: None,
> + free: None,
> + };
> + };
> +}
> +
> +make_param_ops!(PARAM_OPS_I8, i8);
> +make_param_ops!(PARAM_OPS_U8, u8);
> +make_param_ops!(PARAM_OPS_I16, i16);
> +make_param_ops!(PARAM_OPS_U16, u16);
> +make_param_ops!(PARAM_OPS_I32, i32);
> +make_param_ops!(PARAM_OPS_U32, u32);
> +make_param_ops!(PARAM_OPS_I64, i64);
> +make_param_ops!(PARAM_OPS_U64, u64);
> +make_param_ops!(PARAM_OPS_ISIZE, isize);
> +make_param_ops!(PARAM_OPS_USIZE, usize);
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v14 3/7] rust: introduce module_param module
2025-07-02 15:21 ` Benno Lossin
@ 2025-07-04 11:45 ` Andreas Hindborg
2025-07-06 20:00 ` Miguel Ojeda
0 siblings, 1 reply; 34+ messages in thread
From: Andreas Hindborg @ 2025-07-04 11:45 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
"Benno Lossin" <lossin@kernel.org> writes:
> On Wed Jul 2, 2025 at 3:18 PM CEST, Andreas Hindborg wrote:
>> Add types and traits for interfacing the C moduleparam API.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>
> I have some nits below, but overall
>
> Reviewed-by: Benno Lossin <lossin@kernel.org>
>
>> ---
>> rust/kernel/lib.rs | 1 +
>> rust/kernel/module_param.rs | 191 ++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 192 insertions(+)
>
> I really like how the `OnceLock` usage turned out here! Thanks for the
> quick impl!
>
>>
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index 6b4774b2b1c3..2b439ea06185 100644
>> --- a/rust/kernel/lib.rs
>> +++ b/rust/kernel/lib.rs
>> @@ -87,6 +87,7 @@
>> pub mod list;
>> pub mod miscdevice;
>> pub mod mm;
>> +pub mod module_param;
>> #[cfg(CONFIG_NET)]
>> pub mod net;
>> pub mod of;
>> diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs
>> new file mode 100644
>> index 000000000000..ca4be7e45ff7
>> --- /dev/null
>> +++ b/rust/kernel/module_param.rs
>> @@ -0,0 +1,191 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Support for module parameters.
>> +//!
>> +//! C header: [`include/linux/moduleparam.h`](srctree/include/linux/moduleparam.h)
>> +
>> +use crate::prelude::*;
>> +use crate::str::BStr;
>> +use bindings;
>> +use kernel::sync::once_lock::OnceLock;
>> +
>> +/// Newtype to make `bindings::kernel_param` [`Sync`].
>> +#[repr(transparent)]
>> +#[doc(hidden)]
>> +pub struct RacyKernelParam(bindings::kernel_param);
>
> Can you remind me why this is called `Racy`? Maybe add the explainer in
> a comment? (and if it's named racy, why is it okay?)
>
> If it doesn't have a real reason, maybe it should be called
> `KernelParam`?
It is an inherited name from way back. The type exists to allow a static
`bindings::kernel_param`, as this C type is not `Sync`.
I agree, it should just be `KernelParam`.
>
>> +
>> +impl RacyKernelParam {
>> + #[doc(hidden)]
>> + pub const fn new(val: bindings::kernel_param) -> Self {
>> + Self(val)
>> + }
>> +}
>> +
>> +// SAFETY: C kernel handles serializing access to this type. We never access it
>> +// from Rust module.
>> +unsafe impl Sync for RacyKernelParam {}
>> +
>> +/// Types that can be used for module parameters.
>> +// NOTE: This trait is `Copy` because drop could produce unsoundness during teardown.
>> +pub trait ModuleParam: Sized + Copy {
>> + /// The [`ModuleParam`] will be used by the kernel module through this type.
>> + ///
>> + /// This may differ from `Self` if, for example, `Self` needs to track
>> + /// ownership without exposing it or allocate extra space for other possible
>> + /// parameter values.
>> + // This is required to support string parameters in the future.
>> + type Value: ?Sized;
>
> This isn't used anywhere in the patchset and AFAIK the kernel is moving
> away from module params, so I'm not so sure if we're going to have
> strings as params.
The kernel dropping module parameters depends on who you ask. But
regardless, we should probably remove it for now.
>
> Or do you already have those patches ready/plan to use strings? If not,
> then I think we should just remove this type and when we actually need
> them add it.
They are in the old rust branch and they need some work. I do not have a
user for them, which is why I am not including them in this series.
>
>> +
>> + /// Parse a parameter argument into the parameter value.
>> + fn try_from_param_arg(arg: &BStr) -> Result<Self>;
>> +}
>> +
>
>> +impl<T> ModuleParamAccess<T> {
>> + #[doc(hidden)]
>> + pub const fn new(default: T) -> Self {
>> + Self {
>> + value: OnceLock::new(),
>> + default,
>> + }
>> + }
>> +
>> + /// Get a shared reference to the parameter value.
>> + // Note: When sysfs access to parameters are enabled, we have to pass in a
>> + // held lock guard here.
>> + pub fn get(&self) -> &T {
>> + self.value.as_ref().unwrap_or(&self.default)
>> + }
>> +
>> + /// Get a mutable pointer to `self`.
>> + ///
>> + /// NOTE: In most cases it is not safe deref the returned pointer.
>> + pub const fn as_void_ptr(&self) -> *mut c_void {
>> + (self as *const Self).cast_mut().cast()
>
> There is `core::ptr::from_ref` that we should use instead of the `as`
> cast.
Cool 👍
Best regards,
Andreas Hindborg
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v14 3/7] rust: introduce module_param module
2025-07-04 11:45 ` Andreas Hindborg
@ 2025-07-06 20:00 ` Miguel Ojeda
0 siblings, 0 replies; 34+ messages in thread
From: Miguel Ojeda @ 2025-07-06 20:00 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Benno Lossin, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
On Fri, Jul 4, 2025 at 1:45 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> It is an inherited name from way back.
Yeah, that name comes from one of the very first PRs. I think we may
have discussed its name in one of the early calls or maybe I just came
up with the name.
Either way, it is almost 5 years old so I would suggest taking a look
at naming etc. with fresh eyes.
Cheers,
Miguel
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v14 3/7] rust: introduce module_param module
2025-07-02 13:18 ` [PATCH v14 3/7] rust: introduce module_param module Andreas Hindborg
2025-07-02 15:21 ` Benno Lossin
@ 2025-07-03 21:49 ` Danilo Krummrich
2025-07-04 7:29 ` Andreas Hindborg
1 sibling, 1 reply; 34+ messages in thread
From: Danilo Krummrich @ 2025-07-03 21:49 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Benno Lossin, Nicolas Schier,
Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
On 7/2/25 3:18 PM, Andreas Hindborg wrote:
> + /// Get a shared reference to the parameter value.
> + // Note: When sysfs access to parameters are enabled, we have to pass in a
> + // held lock guard here.
> + pub fn get(&self) -> &T {
> + self.value.as_ref().unwrap_or(&self.default)
> + }
I think you forgot to rename this.
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v14 3/7] rust: introduce module_param module
2025-07-03 21:49 ` Danilo Krummrich
@ 2025-07-04 7:29 ` Andreas Hindborg
2025-07-04 7:37 ` Andreas Hindborg
0 siblings, 1 reply; 34+ messages in thread
From: Andreas Hindborg @ 2025-07-04 7:29 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Benno Lossin, Nicolas Schier,
Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
"Danilo Krummrich" <dakr@kernel.org> writes:
> On 7/2/25 3:18 PM, Andreas Hindborg wrote:
>> + /// Get a shared reference to the parameter value.
>> + // Note: When sysfs access to parameters are enabled, we have to pass in a
>> + // held lock guard here.
>> + pub fn get(&self) -> &T {
>> + self.value.as_ref().unwrap_or(&self.default)
>> + }
>
> I think you forgot to rename this.
Yes, thanks for being persistent on this :)
Best regards,
Andreas Hindborg
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v14 3/7] rust: introduce module_param module
2025-07-04 7:29 ` Andreas Hindborg
@ 2025-07-04 7:37 ` Andreas Hindborg
2025-07-04 9:59 ` Benno Lossin
0 siblings, 1 reply; 34+ messages in thread
From: Andreas Hindborg @ 2025-07-04 7:37 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Benno Lossin, Nicolas Schier,
Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
Andreas Hindborg <a.hindborg@kernel.org> writes:
> "Danilo Krummrich" <dakr@kernel.org> writes:
>
>> On 7/2/25 3:18 PM, Andreas Hindborg wrote:
>>> + /// Get a shared reference to the parameter value.
>>> + // Note: When sysfs access to parameters are enabled, we have to pass in a
>>> + // held lock guard here.
>>> + pub fn get(&self) -> &T {
>>> + self.value.as_ref().unwrap_or(&self.default)
>>> + }
>>
>> I think you forgot to rename this.
>
> Yes, thanks for being persistent on this :)
Actually, there is a discussion on whether to keep the API similar to
`std::sync::OnceLock` [1] but also whether to rename this to something
other than `OnceLock` [2]. Depending on how that resolves, it might make
sense to stay with `get` or rename to something else.
Best regards,
Andreas Hindborg
[1] https://lore.kernel.org/all/35e1fef4-b715-4827-a498-bdde9b58b51c@penguintechs.org
[2] https://lore.kernel.org/all/CAH5fLggY2Ei14nVJzLBEoR1Rut1GKU4SZX=+14tuRH1aSuQVTA@mail.gmail.com
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v14 3/7] rust: introduce module_param module
2025-07-04 7:37 ` Andreas Hindborg
@ 2025-07-04 9:59 ` Benno Lossin
2025-07-04 11:46 ` Andreas Hindborg
0 siblings, 1 reply; 34+ messages in thread
From: Benno Lossin @ 2025-07-04 9:59 UTC (permalink / raw)
To: Andreas Hindborg, Danilo Krummrich
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Nicolas Schier, Trevor Gross,
Adam Bratschi-Kaye, rust-for-linux, linux-kernel, linux-kbuild,
Petr Pavlu, Sami Tolvanen, Daniel Gomez, Simona Vetter, Greg KH,
Fiona Behrens, Daniel Almeida, linux-modules
On Fri Jul 4, 2025 at 9:37 AM CEST, Andreas Hindborg wrote:
> Andreas Hindborg <a.hindborg@kernel.org> writes:
>
>> "Danilo Krummrich" <dakr@kernel.org> writes:
>>
>>> On 7/2/25 3:18 PM, Andreas Hindborg wrote:
>>>> + /// Get a shared reference to the parameter value.
>>>> + // Note: When sysfs access to parameters are enabled, we have to pass in a
>>>> + // held lock guard here.
>>>> + pub fn get(&self) -> &T {
>>>> + self.value.as_ref().unwrap_or(&self.default)
>>>> + }
>>>
>>> I think you forgot to rename this.
>>
>> Yes, thanks for being persistent on this :)
>
> Actually, there is a discussion on whether to keep the API similar to
> `std::sync::OnceLock` [1] but also whether to rename this to something
> other than `OnceLock` [2]. Depending on how that resolves, it might make
> sense to stay with `get` or rename to something else.
But this is for the `ModuleParamAccess`, right? There I think it makes
sense to choose `access` or `value`.
---
Cheers,
Benno
> Best regards,
> Andreas Hindborg
>
>
> [1] https://lore.kernel.org/all/35e1fef4-b715-4827-a498-bdde9b58b51c@penguintechs.org
> [2] https://lore.kernel.org/all/CAH5fLggY2Ei14nVJzLBEoR1Rut1GKU4SZX=+14tuRH1aSuQVTA@mail.gmail.com
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v14 3/7] rust: introduce module_param module
2025-07-04 9:59 ` Benno Lossin
@ 2025-07-04 11:46 ` Andreas Hindborg
0 siblings, 0 replies; 34+ messages in thread
From: Andreas Hindborg @ 2025-07-04 11:46 UTC (permalink / raw)
To: Benno Lossin
Cc: Danilo Krummrich, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Nicolas Schier, Trevor Gross,
Adam Bratschi-Kaye, rust-for-linux, linux-kernel, linux-kbuild,
Petr Pavlu, Sami Tolvanen, Daniel Gomez, Simona Vetter, Greg KH,
Fiona Behrens, Daniel Almeida, linux-modules
"Benno Lossin" <lossin@kernel.org> writes:
> On Fri Jul 4, 2025 at 9:37 AM CEST, Andreas Hindborg wrote:
>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>
>>> "Danilo Krummrich" <dakr@kernel.org> writes:
>>>
>>>> On 7/2/25 3:18 PM, Andreas Hindborg wrote:
>>>>> + /// Get a shared reference to the parameter value.
>>>>> + // Note: When sysfs access to parameters are enabled, we have to pass in a
>>>>> + // held lock guard here.
>>>>> + pub fn get(&self) -> &T {
>>>>> + self.value.as_ref().unwrap_or(&self.default)
>>>>> + }
>>>>
>>>> I think you forgot to rename this.
>>>
>>> Yes, thanks for being persistent on this :)
>>
>> Actually, there is a discussion on whether to keep the API similar to
>> `std::sync::OnceLock` [1] but also whether to rename this to something
>> other than `OnceLock` [2]. Depending on how that resolves, it might make
>> sense to stay with `get` or rename to something else.
>
> But this is for the `ModuleParamAccess`, right? There I think it makes
> sense to choose `access` or `value`.
Right, sorry. My context window was too small here.
Best regards,
Andreas Hindborg
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v14 4/7] rust: module: use a reference in macros::module::module
2025-07-02 13:18 [PATCH v14 0/7] rust: extend `module!` macro with integer parameter support Andreas Hindborg
` (2 preceding siblings ...)
2025-07-02 13:18 ` [PATCH v14 3/7] rust: introduce module_param module Andreas Hindborg
@ 2025-07-02 13:18 ` Andreas Hindborg
2025-07-02 13:18 ` [PATCH v14 5/7] rust: module: update the module macro with module parameter support Andreas Hindborg
` (2 subsequent siblings)
6 siblings, 0 replies; 34+ messages in thread
From: Andreas Hindborg @ 2025-07-02 13:18 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Benno Lossin, Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules, Andreas Hindborg
When we add parameter support to the module macro, we want to be able to
pass a reference to `ModuleInfo` to a helper function. That is not possible
when we move out of the local `modinfo`. So change the function to access
the local via reference rather than value.
Reviewed-by: Benno Lossin <lossin@kernel.org>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/macros/module.rs | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index 2ddd2eeb2852..1a867a1e787e 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -179,26 +179,26 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
// Rust does not allow hyphens in identifiers, use underscore instead.
let ident = info.name.replace('-', "_");
let mut modinfo = ModInfoBuilder::new(ident.as_ref());
- if let Some(author) = info.author {
- modinfo.emit("author", &author);
+ if let Some(author) = &info.author {
+ modinfo.emit("author", author);
}
- if let Some(authors) = info.authors {
+ if let Some(authors) = &info.authors {
for author in authors {
- modinfo.emit("author", &author);
+ modinfo.emit("author", author);
}
}
- if let Some(description) = info.description {
- modinfo.emit("description", &description);
+ if let Some(description) = &info.description {
+ modinfo.emit("description", description);
}
modinfo.emit("license", &info.license);
- if let Some(aliases) = info.alias {
+ if let Some(aliases) = &info.alias {
for alias in aliases {
- modinfo.emit("alias", &alias);
+ modinfo.emit("alias", alias);
}
}
- if let Some(firmware) = info.firmware {
+ if let Some(firmware) = &info.firmware {
for fw in firmware {
- modinfo.emit("firmware", &fw);
+ modinfo.emit("firmware", fw);
}
}
--
2.47.2
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v14 5/7] rust: module: update the module macro with module parameter support
2025-07-02 13:18 [PATCH v14 0/7] rust: extend `module!` macro with integer parameter support Andreas Hindborg
` (3 preceding siblings ...)
2025-07-02 13:18 ` [PATCH v14 4/7] rust: module: use a reference in macros::module::module Andreas Hindborg
@ 2025-07-02 13:18 ` Andreas Hindborg
2025-07-02 15:38 ` Benno Lossin
2025-07-02 13:18 ` [PATCH v14 6/7] rust: samples: add a module parameter to the rust_minimal sample Andreas Hindborg
2025-07-02 13:18 ` [PATCH v14 7/7] modules: add rust modules files to MAINTAINERS Andreas Hindborg
6 siblings, 1 reply; 34+ messages in thread
From: Andreas Hindborg @ 2025-07-02 13:18 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Benno Lossin, Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules, Andreas Hindborg
Allow module parameters to be declared in the rust `module!` macro.
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/macros/helpers.rs | 25 +++++++
rust/macros/lib.rs | 31 +++++++++
rust/macros/module.rs | 177 ++++++++++++++++++++++++++++++++++++++++++++++---
3 files changed, 223 insertions(+), 10 deletions(-)
diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs
index e2602be402c1..365d7eb499c0 100644
--- a/rust/macros/helpers.rs
+++ b/rust/macros/helpers.rs
@@ -10,6 +10,17 @@ pub(crate) fn try_ident(it: &mut token_stream::IntoIter) -> Option<String> {
}
}
+pub(crate) fn try_sign(it: &mut token_stream::IntoIter) -> Option<char> {
+ let peek = it.clone().next();
+ match peek {
+ Some(TokenTree::Punct(punct)) if punct.as_char() == '-' => {
+ let _ = it.next();
+ Some(punct.as_char())
+ }
+ _ => None,
+ }
+}
+
pub(crate) fn try_literal(it: &mut token_stream::IntoIter) -> Option<String> {
if let Some(TokenTree::Literal(literal)) = it.next() {
Some(literal.to_string())
@@ -103,3 +114,17 @@ pub(crate) fn file() -> String {
proc_macro::Span::call_site().file()
}
}
+
+/// Parse a token stream of the form `expected_name: "value",` and return the
+/// string in the position of "value".
+///
+/// # Panics
+///
+/// - On parse error.
+pub(crate) fn expect_string_field(it: &mut token_stream::IntoIter, expected_name: &str) -> String {
+ assert_eq!(expect_ident(it), expected_name);
+ assert_eq!(expect_punct(it), ':');
+ let string = expect_string(it);
+ assert_eq!(expect_punct(it), ',');
+ string
+}
diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
index fa847cf3a9b5..2fb520dc930a 100644
--- a/rust/macros/lib.rs
+++ b/rust/macros/lib.rs
@@ -28,6 +28,30 @@
/// The `type` argument should be a type which implements the [`Module`]
/// trait. Also accepts various forms of kernel metadata.
///
+/// The `params` field describe module parameters. Each entry has the form
+///
+/// ```ignore
+/// parameter_name: type {
+/// default: default_value,
+/// description: "Description",
+/// }
+/// ```
+///
+/// `type` may be one of
+///
+/// - [`i8`]
+/// - [`u8`]
+/// - [`i8`]
+/// - [`u8`]
+/// - [`i16`]
+/// - [`u16`]
+/// - [`i32`]
+/// - [`u32`]
+/// - [`i64`]
+/// - [`u64`]
+/// - [`isize`]
+/// - [`usize`]
+///
/// C header: [`include/linux/moduleparam.h`](srctree/include/linux/moduleparam.h)
///
/// [`Module`]: ../kernel/trait.Module.html
@@ -44,6 +68,12 @@
/// description: "My very own kernel module!",
/// license: "GPL",
/// alias: ["alternate_module_name"],
+/// params: {
+/// my_parameter: i64 {
+/// default: 1,
+/// description: "This parameter has a default of 1",
+/// },
+/// },
/// }
///
/// struct MyModule(i32);
@@ -52,6 +82,7 @@
/// fn init(_module: &'static ThisModule) -> Result<Self> {
/// let foo: i32 = 42;
/// pr_info!("I contain: {}\n", foo);
+/// pr_info!("i32 param is: {}\n", module_parameters::my_parameter.read());
/// Ok(Self(foo))
/// }
/// }
diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index 1a867a1e787e..2b8123fe9fb0 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -26,6 +26,7 @@ struct ModInfoBuilder<'a> {
module: &'a str,
counter: usize,
buffer: String,
+ param_buffer: String,
}
impl<'a> ModInfoBuilder<'a> {
@@ -34,10 +35,11 @@ fn new(module: &'a str) -> Self {
module,
counter: 0,
buffer: String::new(),
+ param_buffer: String::new(),
}
}
- fn emit_base(&mut self, field: &str, content: &str, builtin: bool) {
+ fn emit_base(&mut self, field: &str, content: &str, builtin: bool, param: bool) {
let string = if builtin {
// Built-in modules prefix their modinfo strings by `module.`.
format!(
@@ -51,8 +53,14 @@ fn emit_base(&mut self, field: &str, content: &str, builtin: bool) {
format!("{field}={content}\0")
};
+ let buffer = if param {
+ &mut self.param_buffer
+ } else {
+ &mut self.buffer
+ };
+
write!(
- &mut self.buffer,
+ buffer,
"
{cfg}
#[doc(hidden)]
@@ -75,20 +83,118 @@ fn emit_base(&mut self, field: &str, content: &str, builtin: bool) {
self.counter += 1;
}
- fn emit_only_builtin(&mut self, field: &str, content: &str) {
- self.emit_base(field, content, true)
+ fn emit_only_builtin(&mut self, field: &str, content: &str, param: bool) {
+ self.emit_base(field, content, true, param)
}
- fn emit_only_loadable(&mut self, field: &str, content: &str) {
- self.emit_base(field, content, false)
+ fn emit_only_loadable(&mut self, field: &str, content: &str, param: bool) {
+ self.emit_base(field, content, false, param)
}
fn emit(&mut self, field: &str, content: &str) {
- self.emit_only_builtin(field, content);
- self.emit_only_loadable(field, content);
+ self.emit_internal(field, content, false);
+ }
+
+ fn emit_internal(&mut self, field: &str, content: &str, param: bool) {
+ self.emit_only_builtin(field, content, param);
+ self.emit_only_loadable(field, content, param);
+ }
+
+ fn emit_param(&mut self, field: &str, param: &str, content: &str) {
+ let content = format!("{param}:{content}", param = param, content = content);
+ self.emit_internal(field, &content, true);
+ }
+
+ fn emit_params(&mut self, info: &ModuleInfo) {
+ let Some(params) = &info.params else {
+ return;
+ };
+
+ for param in params {
+ let ops = param_ops_path(¶m.ptype);
+
+ // Note: The spelling of these fields is dictated by the user space
+ // tool `modinfo`.
+ self.emit_param("parmtype", ¶m.name, ¶m.ptype);
+ self.emit_param("parm", ¶m.name, ¶m.description);
+
+ write!(
+ self.param_buffer,
+ "
+ pub(crate) static {param_name}:
+ ::kernel::module_param::ModuleParamAccess<{param_type}> =
+ ::kernel::module_param::ModuleParamAccess::new({param_default});
+
+ #[link_section = \"__param\"]
+ #[used]
+ static __{module_name}_{param_name}_struct:
+ ::kernel::module_param::RacyKernelParam =
+ ::kernel::module_param::RacyKernelParam::new(
+ ::kernel::bindings::kernel_param {{
+ name: if cfg!(MODULE) {{
+ ::kernel::c_str!(\"{param_name}\").as_bytes_with_nul()
+ }} else {{
+ ::kernel::c_str!(\"{module_name}.{param_name}\").as_bytes_with_nul()
+ }}.as_ptr(),
+ // SAFETY: `__this_module` is constructed by the kernel at load time
+ // and will not be freed until the module is unloaded.
+ #[cfg(MODULE)]
+ mod_: unsafe {{
+ (&::kernel::bindings::__this_module
+ as *const ::kernel::bindings::module)
+ .cast_mut()
+ }},
+ #[cfg(not(MODULE))]
+ mod_: ::core::ptr::null_mut(),
+ ops: &{ops} as *const ::kernel::bindings::kernel_param_ops,
+ perm: 0, // Will not appear in sysfs
+ level: -1,
+ flags: 0,
+ __bindgen_anon_1:
+ ::kernel::bindings::kernel_param__bindgen_ty_1 {{
+ arg: {param_name}.as_void_ptr()
+ }},
+ }}
+ );
+ ",
+ module_name = info.name,
+ param_type = param.ptype,
+ param_default = param.default,
+ param_name = param.name,
+ ops = ops,
+ )
+ .unwrap();
+ }
+ }
+}
+
+fn param_ops_path(param_type: &str) -> &'static str {
+ match param_type {
+ "i8" => "::kernel::module_param::PARAM_OPS_I8",
+ "u8" => "::kernel::module_param::PARAM_OPS_U8",
+ "i16" => "::kernel::module_param::PARAM_OPS_I16",
+ "u16" => "::kernel::module_param::PARAM_OPS_U16",
+ "i32" => "::kernel::module_param::PARAM_OPS_I32",
+ "u32" => "::kernel::module_param::PARAM_OPS_U32",
+ "i64" => "::kernel::module_param::PARAM_OPS_I64",
+ "u64" => "::kernel::module_param::PARAM_OPS_U64",
+ "isize" => "::kernel::module_param::PARAM_OPS_ISIZE",
+ "usize" => "::kernel::module_param::PARAM_OPS_USIZE",
+ t => panic!("Unsupported parameter type {}", t),
}
}
+fn expect_param_default(param_it: &mut token_stream::IntoIter) -> String {
+ assert_eq!(expect_ident(param_it), "default");
+ assert_eq!(expect_punct(param_it), ':');
+ let sign = try_sign(param_it);
+ let default = try_literal(param_it).expect("Expected default param value");
+ assert_eq!(expect_punct(param_it), ',');
+ let mut value = sign.map(String::from).unwrap_or_default();
+ value.push_str(&default);
+ value
+}
+
#[derive(Debug, Default)]
struct ModuleInfo {
type_: String,
@@ -99,6 +205,50 @@ struct ModuleInfo {
description: Option<String>,
alias: Option<Vec<String>>,
firmware: Option<Vec<String>>,
+ params: Option<Vec<Parameter>>,
+}
+
+#[derive(Debug)]
+struct Parameter {
+ name: String,
+ ptype: String,
+ default: String,
+ description: String,
+}
+
+fn expect_params(it: &mut token_stream::IntoIter) -> Vec<Parameter> {
+ let params = expect_group(it);
+ assert_eq!(params.delimiter(), Delimiter::Brace);
+ let mut it = params.stream().into_iter();
+ let mut parsed = Vec::new();
+
+ loop {
+ let param_name = match it.next() {
+ Some(TokenTree::Ident(ident)) => ident.to_string(),
+ Some(_) => panic!("Expected Ident or end"),
+ None => break,
+ };
+
+ assert_eq!(expect_punct(&mut it), ':');
+ let param_type = expect_ident(&mut it);
+ let group = expect_group(&mut it);
+ assert_eq!(group.delimiter(), Delimiter::Brace);
+ assert_eq!(expect_punct(&mut it), ',');
+
+ let mut param_it = group.stream().into_iter();
+ let param_default = expect_param_default(&mut param_it);
+ let param_description = expect_string_field(&mut param_it, "description");
+ expect_end(&mut param_it);
+
+ parsed.push(Parameter {
+ name: param_name,
+ ptype: param_type,
+ default: param_default,
+ description: param_description,
+ })
+ }
+
+ parsed
}
impl ModuleInfo {
@@ -114,6 +264,7 @@ fn parse(it: &mut token_stream::IntoIter) -> Self {
"license",
"alias",
"firmware",
+ "params",
];
const REQUIRED_KEYS: &[&str] = &["type", "name", "license"];
let mut seen_keys = Vec::new();
@@ -140,6 +291,7 @@ fn parse(it: &mut token_stream::IntoIter) -> Self {
"license" => info.license = expect_string_ascii(it),
"alias" => info.alias = Some(expect_string_array(it)),
"firmware" => info.firmware = Some(expect_string_array(it)),
+ "params" => info.params = Some(expect_params(it)),
_ => panic!("Unknown key \"{key}\". Valid keys are: {EXPECTED_KEYS:?}."),
}
@@ -205,7 +357,9 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
// Built-in modules also export the `file` modinfo string.
let file =
std::env::var("RUST_MODFILE").expect("Unable to fetch RUST_MODFILE environmental variable");
- modinfo.emit_only_builtin("file", &file);
+ modinfo.emit_only_builtin("file", &file, false);
+
+ modinfo.emit_params(&info);
format!(
"
@@ -369,15 +523,18 @@ unsafe fn __exit() {{
__MOD.assume_init_drop();
}}
}}
-
{modinfo}
}}
}}
+ mod module_parameters {{
+ {params}
+ }}
",
type_ = info.type_,
name = info.name,
ident = ident,
modinfo = modinfo.buffer,
+ params = modinfo.param_buffer,
initcall_section = ".initcall6.init"
)
.parse()
--
2.47.2
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v14 5/7] rust: module: update the module macro with module parameter support
2025-07-02 13:18 ` [PATCH v14 5/7] rust: module: update the module macro with module parameter support Andreas Hindborg
@ 2025-07-02 15:38 ` Benno Lossin
2025-07-04 12:29 ` Andreas Hindborg
0 siblings, 1 reply; 34+ messages in thread
From: Benno Lossin @ 2025-07-02 15:38 UTC (permalink / raw)
To: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
On Wed Jul 2, 2025 at 3:18 PM CEST, Andreas Hindborg wrote:
> Allow module parameters to be declared in the rust `module!` macro.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
A few nits below, with those fixed
Reviewed-by: Benno Lossin <lossin@kernel.org>
> ---
> rust/macros/helpers.rs | 25 +++++++
> rust/macros/lib.rs | 31 +++++++++
> rust/macros/module.rs | 177 ++++++++++++++++++++++++++++++++++++++++++++++---
> 3 files changed, 223 insertions(+), 10 deletions(-)
> + fn emit_params(&mut self, info: &ModuleInfo) {
> + let Some(params) = &info.params else {
> + return;
> + };
> +
> + for param in params {
> + let ops = param_ops_path(¶m.ptype);
> +
> + // Note: The spelling of these fields is dictated by the user space
> + // tool `modinfo`.
> + self.emit_param("parmtype", ¶m.name, ¶m.ptype);
> + self.emit_param("parm", ¶m.name, ¶m.description);
> +
> + write!(
> + self.param_buffer,
> + "
> + pub(crate) static {param_name}:
> + ::kernel::module_param::ModuleParamAccess<{param_type}> =
> + ::kernel::module_param::ModuleParamAccess::new({param_default});
> +
> + #[link_section = \"__param\"]
> + #[used]
> + static __{module_name}_{param_name}_struct:
Does it make sense to move this static to a `const _: () = {};` block?
> + ::kernel::module_param::RacyKernelParam =
> + ::kernel::module_param::RacyKernelParam::new(
> + ::kernel::bindings::kernel_param {{
> + name: if cfg!(MODULE) {{
s/cfg/::core::cfg/
:)
Also there seems to only be a 2-space indentation here.
> + ::kernel::c_str!(\"{param_name}\").as_bytes_with_nul()
> + }} else {{
> + ::kernel::c_str!(\"{module_name}.{param_name}\").as_bytes_with_nul()
> + }}.as_ptr(),
> + // SAFETY: `__this_module` is constructed by the kernel at load time
> + // and will not be freed until the module is unloaded.
> + #[cfg(MODULE)]
> + mod_: unsafe {{
> + (&::kernel::bindings::__this_module
> + as *const ::kernel::bindings::module)
> + .cast_mut()
> + }},
It doesn't stop with the improvements...
https://github.com/Rust-for-Linux/linux/issues/1176
Maybe we should also have one to use it here, but eh we can do that
later (and it's not as bad to forget about :)
> + #[cfg(not(MODULE))]
> + mod_: ::core::ptr::null_mut(),
> + ops: &{ops} as *const ::kernel::bindings::kernel_param_ops,
::core::ptr::from_ref(&{ops})
> + perm: 0, // Will not appear in sysfs
> + level: -1,
> + flags: 0,
> + __bindgen_anon_1:
> + ::kernel::bindings::kernel_param__bindgen_ty_1 {{
> + arg: {param_name}.as_void_ptr()
> + }},
Formatting?
+ __bindgen_anon_1: ::kernel::bindings::kernel_param__bindgen_ty_1 {{
+ arg: {param_name}.as_void_ptr()
+ }},
---
Cheers,
Benno
> + }}
> + );
> + ",
> + module_name = info.name,
> + param_type = param.ptype,
> + param_default = param.default,
> + param_name = param.name,
> + ops = ops,
> + )
> + .unwrap();
> + }
> + }
> +}
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v14 5/7] rust: module: update the module macro with module parameter support
2025-07-02 15:38 ` Benno Lossin
@ 2025-07-04 12:29 ` Andreas Hindborg
2025-07-04 12:48 ` Benno Lossin
0 siblings, 1 reply; 34+ messages in thread
From: Andreas Hindborg @ 2025-07-04 12:29 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
"Benno Lossin" <lossin@kernel.org> writes:
> On Wed Jul 2, 2025 at 3:18 PM CEST, Andreas Hindborg wrote:
>> Allow module parameters to be declared in the rust `module!` macro.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>
> A few nits below, with those fixed
>
> Reviewed-by: Benno Lossin <lossin@kernel.org>
>
>> ---
>> rust/macros/helpers.rs | 25 +++++++
>> rust/macros/lib.rs | 31 +++++++++
>> rust/macros/module.rs | 177 ++++++++++++++++++++++++++++++++++++++++++++++---
>> 3 files changed, 223 insertions(+), 10 deletions(-)
>
>> + fn emit_params(&mut self, info: &ModuleInfo) {
>> + let Some(params) = &info.params else {
>> + return;
>> + };
>> +
>> + for param in params {
>> + let ops = param_ops_path(¶m.ptype);
>> +
>> + // Note: The spelling of these fields is dictated by the user space
>> + // tool `modinfo`.
>> + self.emit_param("parmtype", ¶m.name, ¶m.ptype);
>> + self.emit_param("parm", ¶m.name, ¶m.description);
>> +
>> + write!(
>> + self.param_buffer,
>> + "
>> + pub(crate) static {param_name}:
>> + ::kernel::module_param::ModuleParamAccess<{param_type}> =
>> + ::kernel::module_param::ModuleParamAccess::new({param_default});
>> +
>> + #[link_section = \"__param\"]
>> + #[used]
>> + static __{module_name}_{param_name}_struct:
>
> Does it make sense to move this static to a `const _: () = {};` block?
Yes, that makes sense.
>
>> + ::kernel::module_param::RacyKernelParam =
>> + ::kernel::module_param::RacyKernelParam::new(
>> + ::kernel::bindings::kernel_param {{
>> + name: if cfg!(MODULE) {{
>
> s/cfg/::core::cfg/
OK.
>
> :)
>
> Also there seems to only be a 2-space indentation here.
Fixed.
>
>> + ::kernel::c_str!(\"{param_name}\").as_bytes_with_nul()
>> + }} else {{
>> + ::kernel::c_str!(\"{module_name}.{param_name}\").as_bytes_with_nul()
>> + }}.as_ptr(),
>> + // SAFETY: `__this_module` is constructed by the kernel at load time
>> + // and will not be freed until the module is unloaded.
>> + #[cfg(MODULE)]
>> + mod_: unsafe {{
>> + (&::kernel::bindings::__this_module
>> + as *const ::kernel::bindings::module)
>> + .cast_mut()
>> + }},
>
> It doesn't stop with the improvements...
>
> https://github.com/Rust-for-Linux/linux/issues/1176
>
> Maybe we should also have one to use it here, but eh we can do that
> later (and it's not as bad to forget about :)
Applying `from_ref`.
>
>> + #[cfg(not(MODULE))]
>> + mod_: ::core::ptr::null_mut(),
>> + ops: &{ops} as *const ::kernel::bindings::kernel_param_ops,
>
> ::core::ptr::from_ref(&{ops})
👍
>
>> + perm: 0, // Will not appear in sysfs
>> + level: -1,
>> + flags: 0,
>> + __bindgen_anon_1:
>> + ::kernel::bindings::kernel_param__bindgen_ty_1 {{
>> + arg: {param_name}.as_void_ptr()
>> + }},
>
> Formatting?
>
> + __bindgen_anon_1: ::kernel::bindings::kernel_param__bindgen_ty_1 {{
> + arg: {param_name}.as_void_ptr()
> + }},
That makes the line more than 100 characters after changing other
formatting things. Perhaps I should just left shift all this?
Best regards,
Andreas Hindborg
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v14 5/7] rust: module: update the module macro with module parameter support
2025-07-04 12:29 ` Andreas Hindborg
@ 2025-07-04 12:48 ` Benno Lossin
2025-07-04 13:51 ` Andreas Hindborg
0 siblings, 1 reply; 34+ messages in thread
From: Benno Lossin @ 2025-07-04 12:48 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
On Fri Jul 4, 2025 at 2:29 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> On Wed Jul 2, 2025 at 3:18 PM CEST, Andreas Hindborg wrote:
>>> + perm: 0, // Will not appear in sysfs
>>> + level: -1,
>>> + flags: 0,
>>> + __bindgen_anon_1:
>>> + ::kernel::bindings::kernel_param__bindgen_ty_1 {{
>>> + arg: {param_name}.as_void_ptr()
>>> + }},
>>
>> Formatting?
>>
>> + __bindgen_anon_1: ::kernel::bindings::kernel_param__bindgen_ty_1 {{
>> + arg: {param_name}.as_void_ptr()
>> + }},
>
>
> That makes the line more than 100 characters after changing other
> formatting things. Perhaps I should just left shift all this?
Not sure what you mean by left shift? When I tried it, it was fine, but
it could have changed with the other things... Do you have a branch with
your changes?
---
Cheers,
Benno
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v14 5/7] rust: module: update the module macro with module parameter support
2025-07-04 12:48 ` Benno Lossin
@ 2025-07-04 13:51 ` Andreas Hindborg
2025-07-04 14:00 ` Benno Lossin
0 siblings, 1 reply; 34+ messages in thread
From: Andreas Hindborg @ 2025-07-04 13:51 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
"Benno Lossin" <lossin@kernel.org> writes:
> On Fri Jul 4, 2025 at 2:29 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> On Wed Jul 2, 2025 at 3:18 PM CEST, Andreas Hindborg wrote:
>>>> + perm: 0, // Will not appear in sysfs
>>>> + level: -1,
>>>> + flags: 0,
>>>> + __bindgen_anon_1:
>>>> + ::kernel::bindings::kernel_param__bindgen_ty_1 {{
>>>> + arg: {param_name}.as_void_ptr()
>>>> + }},
>>>
>>> Formatting?
>>>
>>> + __bindgen_anon_1: ::kernel::bindings::kernel_param__bindgen_ty_1 {{
>>> + arg: {param_name}.as_void_ptr()
>>> + }},
>>
>>
>> That makes the line more than 100 characters after changing other
>> formatting things. Perhaps I should just left shift all this?
>
> Not sure what you mean by left shift? When I tried it, it was fine, but
> it could have changed with the other things... Do you have a branch with
> your changes?
Move all the code template so the least indented start at column 0.
My WIP branch is here [1].
Best regards,
Andreas Hindborg
[1] https://github.com/metaspace/linux/tree/module-params
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v14 5/7] rust: module: update the module macro with module parameter support
2025-07-04 13:51 ` Andreas Hindborg
@ 2025-07-04 14:00 ` Benno Lossin
0 siblings, 0 replies; 34+ messages in thread
From: Benno Lossin @ 2025-07-04 14:00 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
On Fri Jul 4, 2025 at 3:51 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> On Fri Jul 4, 2025 at 2:29 PM CEST, Andreas Hindborg wrote:
>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>> On Wed Jul 2, 2025 at 3:18 PM CEST, Andreas Hindborg wrote:
>>>>> + perm: 0, // Will not appear in sysfs
>>>>> + level: -1,
>>>>> + flags: 0,
>>>>> + __bindgen_anon_1:
>>>>> + ::kernel::bindings::kernel_param__bindgen_ty_1 {{
>>>>> + arg: {param_name}.as_void_ptr()
>>>>> + }},
>>>>
>>>> Formatting?
>>>>
>>>> + __bindgen_anon_1: ::kernel::bindings::kernel_param__bindgen_ty_1 {{
>>>> + arg: {param_name}.as_void_ptr()
>>>> + }},
>>>
>>>
>>> That makes the line more than 100 characters after changing other
>>> formatting things. Perhaps I should just left shift all this?
>>
>> Not sure what you mean by left shift? When I tried it, it was fine, but
>> it could have changed with the other things... Do you have a branch with
>> your changes?
>
> Move all the code template so the least indented start at column 0.
>
> My WIP branch is here [1].
If you dedent the contents of the string once, then everything fits in
100 columns.
---
Cheers,
Benno
> Best regards,
> Andreas Hindborg
>
>
> [1] https://github.com/metaspace/linux/tree/module-params
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v14 6/7] rust: samples: add a module parameter to the rust_minimal sample
2025-07-02 13:18 [PATCH v14 0/7] rust: extend `module!` macro with integer parameter support Andreas Hindborg
` (4 preceding siblings ...)
2025-07-02 13:18 ` [PATCH v14 5/7] rust: module: update the module macro with module parameter support Andreas Hindborg
@ 2025-07-02 13:18 ` Andreas Hindborg
2025-07-02 13:18 ` [PATCH v14 7/7] modules: add rust modules files to MAINTAINERS Andreas Hindborg
6 siblings, 0 replies; 34+ messages in thread
From: Andreas Hindborg @ 2025-07-02 13:18 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Benno Lossin, Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules, Andreas Hindborg
Showcase the rust module parameter support by adding a module parameter to
the `rust_minimal` sample.
Reviewed-by: Benno Lossin <lossin@kernel.org>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
samples/rust/rust_minimal.rs | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/samples/rust/rust_minimal.rs b/samples/rust/rust_minimal.rs
index 1fc7a1be6b6d..c04cc07b3249 100644
--- a/samples/rust/rust_minimal.rs
+++ b/samples/rust/rust_minimal.rs
@@ -10,6 +10,12 @@
authors: ["Rust for Linux Contributors"],
description: "Rust minimal sample",
license: "GPL",
+ params: {
+ test_parameter: i64 {
+ default: 1,
+ description: "This parameter has a default of 1",
+ },
+ },
}
struct RustMinimal {
@@ -20,6 +26,10 @@ impl kernel::Module for RustMinimal {
fn init(_module: &'static ThisModule) -> Result<Self> {
pr_info!("Rust minimal sample (init)\n");
pr_info!("Am I built-in? {}\n", !cfg!(MODULE));
+ pr_info!(
+ "test_parameter: {}\n",
+ *module_parameters::test_parameter.get()
+ );
let mut numbers = KVec::new();
numbers.push(72, GFP_KERNEL)?;
--
2.47.2
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v14 7/7] modules: add rust modules files to MAINTAINERS
2025-07-02 13:18 [PATCH v14 0/7] rust: extend `module!` macro with integer parameter support Andreas Hindborg
` (5 preceding siblings ...)
2025-07-02 13:18 ` [PATCH v14 6/7] rust: samples: add a module parameter to the rust_minimal sample Andreas Hindborg
@ 2025-07-02 13:18 ` Andreas Hindborg
6 siblings, 0 replies; 34+ messages in thread
From: Andreas Hindborg @ 2025-07-02 13:18 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Benno Lossin, Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules, Andreas Hindborg
The module subsystem people agreed to maintain rust support for modules
[1]. Thus, add entries for relevant files to modules entry in MAINTAINERS.
Link: https://lore.kernel.org/all/0d9e596a-5316-4e00-862b-fd77552ae4b5@suse.com/ [1]
Acked-by: Daniel Gomez <da.gomez@samsung.com>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
MAINTAINERS | 2 ++
1 file changed, 2 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index fe0cf0a2e6e5..aa5618350723 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16796,6 +16796,8 @@ F: include/linux/module*.h
F: kernel/module/
F: lib/test_kmod.c
F: lib/tests/module/
+F: rust/kernel/module_param.rs
+F: rust/macros/module.rs
F: scripts/module*
F: tools/testing/selftests/kmod/
F: tools/testing/selftests/module/
--
2.47.2
^ permalink raw reply related [flat|nested] 34+ messages in thread