[PATCH V4 0/2] Rust: Add cpumask abstractions

[PATCH V4 0/2] Rust: Add cpumask abstractions

[Viresh Kumar]

Hello,

This series adds initial cpumask Rust abstractions and adds a new maintenance
entry for the same.

V3->V4:
- Create separate entry for cpumask in MAINTAINERS.

V2->V3:
- Improved comments, SAFETY, Invariants, and INVARIANT blocks.
- Add examples.
- Inline few methods.

V1->V2:
- Add Yury's Reviewed-by tag in 2/2.

- Implemented two different structures, Cpumask (corresponds to struct
  cpumask) and CpumaskBox (corresponds to cpumask_var_t). Thanks Alice for
  helping out.

--
Viresh

Viresh Kumar (2):
  rust: Add initial cpumask abstractions
  MAINTAINERS: Add entry for Rust bitmap API

 MAINTAINERS            |   6 +
 rust/kernel/cpumask.rs | 301 +++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs     |   1 +
 3 files changed, 308 insertions(+)
 create mode 100644 rust/kernel/cpumask.rs

-- 
2.31.1.272.g89b43f80a514

[PATCH V4 1/2] rust: Add initial cpumask abstractions

[Viresh Kumar]

Add initial Rust abstractions for struct cpumask, covering a subset of
its APIs. Additional APIs can be added as needed.

These abstractions will be used in upcoming Rust support for cpufreq and
OPP frameworks.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 rust/kernel/cpumask.rs | 301 +++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs     |   1 +
 2 files changed, 302 insertions(+)
 create mode 100644 rust/kernel/cpumask.rs

diff --git a/rust/kernel/cpumask.rs b/rust/kernel/cpumask.rs
new file mode 100644
index 000000000000..792210a77770
--- /dev/null
+++ b/rust/kernel/cpumask.rs
@@ -0,0 +1,301 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! CPU Mask abstractions.
+//!
+//! C header: [`include/linux/cpumask.h`](srctree/include/linux/cpumask.h)
+
+use crate::{
+    alloc::{AllocError, Flags},
+    bindings,
+    prelude::*,
+    types::Opaque,
+};
+
+#[cfg(CONFIG_CPUMASK_OFFSTACK)]
+use core::ptr::{self, NonNull};
+
+#[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+use core::mem::MaybeUninit;
+
+use core::ops::{Deref, DerefMut};
+
+/// A CPU Mask.
+///
+/// This represents the Rust abstraction for the C `struct cpumask`.
+///
+/// # Invariants
+///
+/// A [`Cpumask`] instance always corresponds to a valid C `struct cpumask`.
+///
+/// The callers must ensure that the `struct cpumask` is valid for access and remains valid for the
+/// lifetime of the returned reference.
+///
+/// ## Examples
+///
+/// The following example demonstrates how to update a [`Cpumask`].
+///
+/// ```
+/// use kernel::bindings;
+/// use kernel::cpumask::Cpumask;
+///
+/// fn set_clear_cpu(ptr: *mut bindings::cpumask, set_cpu: u32, clear_cpu: i32) {
+///     // SAFETY: The `ptr` is valid for writing and remains valid for the lifetime of the
+///     // returned reference.
+///     let mask = unsafe { Cpumask::from_raw_mut(ptr) };
+///     mask.set(set_cpu);
+///     mask.clear(clear_cpu);
+/// }
+/// ```
+#[repr(transparent)]
+pub struct Cpumask(Opaque<bindings::cpumask>);
+
+impl Cpumask {
+    /// Creates a mutable reference to an existing `struct cpumask` pointer.
+    ///
+    /// # Safety
+    ///
+    /// The caller must ensure that `ptr` is valid for writing and remains valid for the lifetime
+    /// of the returned reference.
+    pub unsafe fn from_raw_mut<'a>(ptr: *mut bindings::cpumask) -> &'a mut Self {
+        // SAFETY: Guaranteed by the safety requirements of the function.
+        //
+        // INVARIANT: The caller ensures that `ptr` is valid for writing and remains valid for the
+        // lifetime of the returned reference.
+        unsafe { &mut *ptr.cast() }
+    }
+
+    /// Creates a reference to an existing `struct cpumask` pointer.
+    ///
+    /// # Safety
+    ///
+    /// The caller must ensure that `ptr` is valid for reading and remains valid for the lifetime
+    /// of the returned reference.
+    pub unsafe fn from_raw<'a>(ptr: *const bindings::cpumask) -> &'a Self {
+        // SAFETY: Guaranteed by the safety requirements of the function.
+        //
+        // INVARIANT: The caller ensures that `ptr` is valid for reading and remains valid for the
+        // lifetime of the returned reference.
+        unsafe { &*ptr.cast() }
+    }
+
+    /// Obtain the raw `struct cpumask` pointer.
+    pub fn as_raw(&self) -> *mut bindings::cpumask {
+        self as *const Cpumask as *mut bindings::cpumask
+    }
+
+    /// Set `cpu` in the cpumask.
+    ///
+    /// Equivalent to the kernel's `cpumask_set_cpu` API.
+    #[inline]
+    pub fn set(&mut self, cpu: u32) {
+        // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `cpumask_set_cpus`.
+        unsafe { bindings::cpumask_set_cpu(cpu, self.as_raw()) };
+    }
+
+    /// Clear `cpu` in the cpumask.
+    ///
+    /// Equivalent to the kernel's `cpumask_clear_cpu` API.
+    #[inline]
+    pub fn clear(&mut self, cpu: i32) {
+        // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `cpumask_clear_cpu`.
+        unsafe { bindings::cpumask_clear_cpu(cpu, self.as_raw()) };
+    }
+
+    /// Set all CPUs in the cpumask.
+    ///
+    /// Equivalent to the kernel's `cpumask_setall` API.
+    #[inline]
+    pub fn set_all(&mut self) {
+        // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `cpumask_setall`.
+        unsafe { bindings::cpumask_setall(self.as_raw()) };
+    }
+
+    /// Get weight of the cpumask.
+    ///
+    /// Equivalent to the kernel's `cpumask_weight` API.
+    #[inline]
+    pub fn weight(&self) -> u32 {
+        // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `cpumask_weight`.
+        unsafe { bindings::cpumask_weight(self.as_raw()) }
+    }
+
+    /// Copy cpumask.
+    ///
+    /// Equivalent to the kernel's `cpumask_copy` API.
+    #[inline]
+    pub fn copy(&self, dstp: &mut Self) {
+        // SAFETY: By the type invariant, `Self::as_raw` is a valid argument to `cpumask_copy`.
+        unsafe { bindings::cpumask_copy(dstp.as_raw(), self.as_raw()) };
+    }
+}
+
+/// A CPU Mask pointer.
+///
+/// This represents the Rust abstraction for the C `struct cpumask_var_t`.
+///
+/// # Invariants
+///
+/// A [`CpumaskBox`] instance always corresponds to a valid C `struct cpumask_var_t`.
+///
+/// The callers must ensure that the `struct cpumask_var_t` is valid for access and remains valid
+/// for the lifetime of [`CpumaskBox`].
+///
+/// ## Examples
+///
+/// The following example demonstrates how to create and update a [`CpumaskBox`].
+///
+/// ```
+/// use kernel::cpumask::CpumaskBox;
+/// use kernel::error::Result;
+///
+/// fn cpumask_foo() -> Result {
+///     let mut mask = CpumaskBox::new(GFP_KERNEL)?;
+///
+///     assert_eq!(mask.weight(), 0);
+///     mask.set(2);
+///     assert_eq!(mask.weight(), 1);
+///     mask.set(3);
+///     assert_eq!(mask.weight(), 2);
+///
+///     let mask2 = CpumaskBox::try_clone(&mask)?;
+///     assert_eq!(mask2.weight(), 2);
+///
+///     Ok(())
+/// }
+/// ```
+pub struct CpumaskBox {
+    #[cfg(CONFIG_CPUMASK_OFFSTACK)]
+    ptr: NonNull<Cpumask>,
+    #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+    mask: Cpumask,
+}
+
+impl CpumaskBox {
+    /// Creates an initialized instance of the [`CpumaskBox`].
+    pub fn new(_flags: Flags) -> Result<Self, AllocError> {
+        Ok(Self {
+            #[cfg(CONFIG_CPUMASK_OFFSTACK)]
+            ptr: {
+                let mut ptr: *mut bindings::cpumask = ptr::null_mut();
+
+                // SAFETY: Depending on the value of `_flags`, this call may sleep. Other than
+                // that, it is always safe to call this method.
+                //
+                // INVARIANT: The associated memory is freed when the `CpumaskBox` goes out of
+                // scope.
+                unsafe { bindings::zalloc_cpumask_var(&mut ptr, _flags.as_raw()) };
+                NonNull::new(ptr.cast()).ok_or(AllocError)?
+            },
+
+            #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+            // SAFETY: FFI type is valid to be zero-initialized.
+            //
+            // INVARIANT: The associated memory is freed when the `CpumaskBox` goes out of scope.
+            mask: unsafe { core::mem::zeroed() },
+        })
+    }
+
+    /// Creates an uninitialized instance of the [`CpumaskBox`].
+    ///
+    /// # Safety
+    ///
+    /// The caller must ensure that the returned [`CpumaskBox`] is properly initialized before
+    /// getting used.
+    unsafe fn new_uninit(_flags: Flags) -> Result<Self, AllocError> {
+        Ok(Self {
+            #[cfg(CONFIG_CPUMASK_OFFSTACK)]
+            ptr: {
+                let mut ptr: *mut bindings::cpumask = ptr::null_mut();
+
+                // SAFETY: Depending on the value of `_flags`, this call may sleep. Other than
+                // that, it is always safe to call this method.
+                //
+                // INVARIANT: The associated memory is freed when the `CpumaskBox` goes out of
+                // scope.
+                unsafe { bindings::alloc_cpumask_var(&mut ptr, _flags.as_raw()) };
+                NonNull::new(ptr.cast()).ok_or(AllocError)?
+            },
+            #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+            // SAFETY: Guaranteed by the safety requirements of the function.
+            //
+            // INVARIANT: The associated memory is freed when the `CpumaskBox` goes out of scope.
+            mask: unsafe { MaybeUninit::uninit().assume_init() },
+        })
+    }
+
+    /// Creates a mutable reference to an existing `struct cpumask_var_t` pointer.
+    ///
+    /// # Safety
+    ///
+    /// The caller must ensure that `ptr` is valid for writing and remains valid for the lifetime
+    /// of the returned reference.
+    pub unsafe fn from_raw_mut<'a>(ptr: *mut bindings::cpumask_var_t) -> &'a mut Self {
+        // SAFETY: Guaranteed by the safety requirements of the function.
+        //
+        // INVARIANT: The caller ensures that `ptr` is valid for writing and remains valid for the
+        // lifetime of the returned reference.
+        unsafe { &mut *ptr.cast() }
+    }
+
+    /// Creates a reference to an existing `struct cpumask_var_t` pointer.
+    ///
+    /// # Safety
+    ///
+    /// The caller must ensure that `ptr` is valid for reading and remains valid for the lifetime
+    /// of the returned reference.
+    pub unsafe fn from_raw<'a>(ptr: *const bindings::cpumask_var_t) -> &'a Self {
+        // SAFETY: Guaranteed by the safety requirements of the function.
+        //
+        // INVARIANT: The caller ensures that `ptr` is valid for reading and remains valid for the
+        // lifetime of the returned reference.
+        unsafe { &*ptr.cast() }
+    }
+
+    /// Clones cpumask.
+    pub fn try_clone(cpumask: &Cpumask) -> Result<Self> {
+        // SAFETY: The returned cpumask_box is initialized right after this call.
+        let mut cpumask_box = unsafe { Self::new_uninit(GFP_KERNEL) }?;
+
+        cpumask.copy(&mut cpumask_box);
+        Ok(cpumask_box)
+    }
+}
+
+// Make [`CpumaskBox`] behave like a pointer to [`Cpumask`].
+impl Deref for CpumaskBox {
+    type Target = Cpumask;
+
+    #[cfg(CONFIG_CPUMASK_OFFSTACK)]
+    fn deref(&self) -> &Cpumask {
+        // SAFETY: The caller owns CpumaskBox, so it is safe to deref the cpumask.
+        unsafe { &*self.ptr.as_ptr() }
+    }
+
+    #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+    fn deref(&self) -> &Cpumask {
+        &self.mask
+    }
+}
+
+impl DerefMut for CpumaskBox {
+    #[cfg(CONFIG_CPUMASK_OFFSTACK)]
+    fn deref_mut(&mut self) -> &mut Cpumask {
+        // SAFETY: The caller owns CpumaskBox, so it is safe to deref the cpumask.
+        unsafe { self.ptr.as_mut() }
+    }
+
+    #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+    fn deref_mut(&mut self) -> &mut Cpumask {
+        &mut self.mask
+    }
+}
+
+impl Drop for CpumaskBox {
+    fn drop(&mut self) {
+        #[cfg(CONFIG_CPUMASK_OFFSTACK)]
+        // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `free_cpumask_var`.
+        unsafe {
+            bindings::free_cpumask_var(self.as_raw())
+        };
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 398242f92a96..dbed774ea9f7 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -40,6 +40,7 @@
 pub mod block;
 #[doc(hidden)]
 pub mod build_assert;
+pub mod cpumask;
 pub mod cred;
 pub mod device;
 pub mod device_id;
-- 
2.31.1.272.g89b43f80a514

[PATCH V4 2/2] MAINTAINERS: Add entry for Rust bitmap API

[Viresh Kumar]

Update the MAINTAINERS file to include the Rust abstractions for bitmap
API.

Yury has indicated that he does not wish to maintain the Rust code but
would like to be listed as a reviewer.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Yury Norov <yury.norov@gmail.com>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 315cff76df29..f67060f1b3e9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6113,6 +6113,12 @@ L:	linux-riscv@lists.infradead.org
 S:	Maintained
 F:	drivers/cpuidle/cpuidle-riscv-sbi.c
 
+CPUMASK API [RUST]
+M:	Viresh Kumar <viresh.kumar@linaro.org>
+R:	Yury Norov <yury.norov@gmail.com>
+S:	Maintained
+F:	rust/kernel/cpumask.rs
+
 CRAMFS FILESYSTEM
 M:	Nicolas Pitre <nico@fluxnic.net>
 S:	Maintained
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH V4 2/2] MAINTAINERS: Add entry for Rust bitmap API

[Miguel Ojeda]

On Wed, Apr 2, 2025 at 7:38 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Update the MAINTAINERS file to include the Rust abstractions for bitmap
> API.
>
> Yury has indicated that he does not wish to maintain the Rust code but
> would like to be listed as a reviewer.

Will patches go through the BITMAP API tree, then? i.e. you will
maintain it by sending patches to Yury's tree, right? That is great,
just wanted to confirm after all the discussions if I missed anything.

(By the way, the BITMAP API entry does not seem to have a `T:` field
-- from a quick look at the latest pull, is it
https://github.com/norov/linux?)

Thanks!

Cheers,
Miguel

Re: [PATCH V4 2/2] MAINTAINERS: Add entry for Rust bitmap API

[Yury Norov]

On Wed, Apr 02, 2025 at 11:08:43AM +0530, Viresh Kumar wrote:
> Update the MAINTAINERS file to include the Rust abstractions for bitmap
> API.

cpumask API you mean?

> 
> Yury has indicated that he does not wish to maintain the Rust code but
> would like to be listed as a reviewer.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> Reviewed-by: Yury Norov <yury.norov@gmail.com>
> ---
>  MAINTAINERS | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 315cff76df29..f67060f1b3e9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6113,6 +6113,12 @@ L:	linux-riscv@lists.infradead.org
>  S:	Maintained
>  F:	drivers/cpuidle/cpuidle-riscv-sbi.c
>  
> +CPUMASK API [RUST]
> +M:	Viresh Kumar <viresh.kumar@linaro.org>
> +R:	Yury Norov <yury.norov@gmail.com>
> +S:	Maintained
> +F:	rust/kernel/cpumask.rs
> +
>  CRAMFS FILESYSTEM
>  M:	Nicolas Pitre <nico@fluxnic.net>
>  S:	Maintained
> -- 
> 2.31.1.272.g89b43f80a514

Re: [PATCH V4 2/2] MAINTAINERS: Add entry for Rust bitmap API

[Yury Norov]

On Wed, Apr 02, 2025 at 01:39:22PM +0200, Miguel Ojeda wrote:
> On Wed, Apr 2, 2025 at 7:38 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > Update the MAINTAINERS file to include the Rust abstractions for bitmap
> > API.
> >
> > Yury has indicated that he does not wish to maintain the Rust code but
> > would like to be listed as a reviewer.
> 
> Will patches go through the BITMAP API tree, then? i.e. you will
> maintain it by sending patches to Yury's tree, right? That is great,
> just wanted to confirm after all the discussions if I missed anything.

I didn't plan to, but I can move the bindings if you find it
reasonable.

> (By the way, the BITMAP API entry does not seem to have a `T:` field
> -- from a quick look at the latest pull, is it
> https://github.com/norov/linux?)

Yes, it is. I'll add the 'T' section. 

Re: [PATCH V4 2/2] MAINTAINERS: Add entry for Rust bitmap API

[Miguel Ojeda]

On Wed, Apr 2, 2025 at 5:01 PM Yury Norov <yury.norov@gmail.com> wrote:
>
> I didn't plan to, but I can move the bindings if you find it
> reasonable.

That would be great if you can do it, i.e. we always try first to get
the maintainers of the C side involved.

> Yes, it is. I'll add the 'T' section.

Ah, nice, thanks!

Cheers,
Miguel

Re: [PATCH V4 1/2] rust: Add initial cpumask abstractions

[Yury Norov]

On Wed, Apr 02, 2025 at 11:08:42AM +0530, Viresh Kumar wrote:
> Wed,  2 Apr 2025 11:08:42 +0530
> Message-Id: <35f4223be4a51139348fed82420481b370d7b1db.1743572195.git.viresh.kumar@linaro.org>
> X-Mailer: git-send-email 2.31.1.272.g89b43f80a514
> In-Reply-To: <cover.1743572195.git.viresh.kumar@linaro.org>
> References: <cover.1743572195.git.viresh.kumar@linaro.org>
> MIME-Version: 1.0
> Content-Transfer-Encoding: 8bit
> Status: O
> Content-Length: 11430
> Lines: 334
> 
> Add initial Rust abstractions for struct cpumask, covering a subset of
> its APIs. Additional APIs can be added as needed.
> 
> These abstractions will be used in upcoming Rust support for cpufreq and
> OPP frameworks.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  rust/kernel/cpumask.rs | 301 +++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs     |   1 +
>  2 files changed, 302 insertions(+)
>  create mode 100644 rust/kernel/cpumask.rs
> 
> diff --git a/rust/kernel/cpumask.rs b/rust/kernel/cpumask.rs
> new file mode 100644
> index 000000000000..792210a77770
> --- /dev/null
> +++ b/rust/kernel/cpumask.rs
> @@ -0,0 +1,301 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! CPU Mask abstractions.
> +//!
> +//! C header: [`include/linux/cpumask.h`](srctree/include/linux/cpumask.h)
> +
> +use crate::{
> +    alloc::{AllocError, Flags},
> +    bindings,
> +    prelude::*,
> +    types::Opaque,
> +};
> +
> +#[cfg(CONFIG_CPUMASK_OFFSTACK)]
> +use core::ptr::{self, NonNull};
> +
> +#[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
> +use core::mem::MaybeUninit;
> +
> +use core::ops::{Deref, DerefMut};
> +
> +/// A CPU Mask.
> +///
> +/// This represents the Rust abstraction for the C `struct cpumask`.
> +///
> +/// # Invariants
> +///
> +/// A [`Cpumask`] instance always corresponds to a valid C `struct cpumask`.
> +///
> +/// The callers must ensure that the `struct cpumask` is valid for access and remains valid for the
> +/// lifetime of the returned reference.
> +///
> +/// ## Examples
> +///
> +/// The following example demonstrates how to update a [`Cpumask`].
> +///
> +/// ```
> +/// use kernel::bindings;
> +/// use kernel::cpumask::Cpumask;
> +///
> +/// fn set_clear_cpu(ptr: *mut bindings::cpumask, set_cpu: u32, clear_cpu: i32) {
> +///     // SAFETY: The `ptr` is valid for writing and remains valid for the lifetime of the
> +///     // returned reference.
> +///     let mask = unsafe { Cpumask::from_raw_mut(ptr) };
> +///     mask.set(set_cpu);
> +///     mask.clear(clear_cpu);
> +/// }
> +/// ```
> +#[repr(transparent)]
> +pub struct Cpumask(Opaque<bindings::cpumask>);
> +
> +impl Cpumask {
> +    /// Creates a mutable reference to an existing `struct cpumask` pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that `ptr` is valid for writing and remains valid for the lifetime
> +    /// of the returned reference.
> +    pub unsafe fn from_raw_mut<'a>(ptr: *mut bindings::cpumask) -> &'a mut Self {
> +        // SAFETY: Guaranteed by the safety requirements of the function.
> +        //
> +        // INVARIANT: The caller ensures that `ptr` is valid for writing and remains valid for the
> +        // lifetime of the returned reference.
> +        unsafe { &mut *ptr.cast() }
> +    }
> +
> +    /// Creates a reference to an existing `struct cpumask` pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that `ptr` is valid for reading and remains valid for the lifetime
> +    /// of the returned reference.
> +    pub unsafe fn from_raw<'a>(ptr: *const bindings::cpumask) -> &'a Self {
> +        // SAFETY: Guaranteed by the safety requirements of the function.
> +        //
> +        // INVARIANT: The caller ensures that `ptr` is valid for reading and remains valid for the
> +        // lifetime of the returned reference.
> +        unsafe { &*ptr.cast() }
> +    }
> +
> +    /// Obtain the raw `struct cpumask` pointer.
> +    pub fn as_raw(&self) -> *mut bindings::cpumask {
> +        self as *const Cpumask as *mut bindings::cpumask
> +    }
> +
> +    /// Set `cpu` in the cpumask.
> +    ///
> +    /// Equivalent to the kernel's `cpumask_set_cpu` API.
> +    #[inline]
> +    pub fn set(&mut self, cpu: u32) {
> +        // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `cpumask_set_cpus`.
> +        unsafe { bindings::cpumask_set_cpu(cpu, self.as_raw()) };
> +    }

Alright, this is an atomic operation. For bitmaps in rust, Burak and
Alice decided to switch naming, so 'set()' in C becomes 'set_atomic()'
in rust, and correspondingly, '__set()' becomes 'set()'.

I think it's maybe OK to switch naming for a different language. But
guys, can you please be consistent once you made a decision?

Burak, Alice, please comment.

Regardless, without looking at the end code I can't judge if you need
atomic or non-atomic ops. Can you link the project that actually uses
this API? Better if you just prepend that series with this 2 patches
and move them together.

> +    /// Clear `cpu` in the cpumask.
> +    ///
> +    /// Equivalent to the kernel's `cpumask_clear_cpu` API.
> +    #[inline]
> +    pub fn clear(&mut self, cpu: i32) {
> +        // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `cpumask_clear_cpu`.
> +        unsafe { bindings::cpumask_clear_cpu(cpu, self.as_raw()) };
> +    }
> +
> +    /// Set all CPUs in the cpumask.
> +    ///
> +    /// Equivalent to the kernel's `cpumask_setall` API.
> +    #[inline]
> +    pub fn set_all(&mut self) {
> +        // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `cpumask_setall`.
> +        unsafe { bindings::cpumask_setall(self.as_raw()) };
> +    }

Can you keep the name as 'setall'? This would help those grepping
methods roots in C sources.

> +    /// Get weight of the cpumask.
> +    ///
> +    /// Equivalent to the kernel's `cpumask_weight` API.
> +    #[inline]
> +    pub fn weight(&self) -> u32 {
> +        // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `cpumask_weight`.
> +        unsafe { bindings::cpumask_weight(self.as_raw()) }
> +    }
> +
> +    /// Copy cpumask.
> +    ///
> +    /// Equivalent to the kernel's `cpumask_copy` API.
> +    #[inline]
> +    pub fn copy(&self, dstp: &mut Self) {
> +        // SAFETY: By the type invariant, `Self::as_raw` is a valid argument to `cpumask_copy`.
> +        unsafe { bindings::cpumask_copy(dstp.as_raw(), self.as_raw()) };
> +    }
> +}
> +
> +/// A CPU Mask pointer.
> +///
> +/// This represents the Rust abstraction for the C `struct cpumask_var_t`.
> +///
> +/// # Invariants
> +///
> +/// A [`CpumaskBox`] instance always corresponds to a valid C `struct cpumask_var_t`.

Can you keep the C name? Maybe CpumaskVar? Or this 'Box' has special
meaning in rust?

> +///
> +/// The callers must ensure that the `struct cpumask_var_t` is valid for access and remains valid
> +/// for the lifetime of [`CpumaskBox`].
> +///
> +/// ## Examples
> +///
> +/// The following example demonstrates how to create and update a [`CpumaskBox`].
> +///
> +/// ```
> +/// use kernel::cpumask::CpumaskBox;
> +/// use kernel::error::Result;
> +///
> +/// fn cpumask_foo() -> Result {

cpumask_foo() what? This is not a good name for test, neither
for an example.

> +///     let mut mask = CpumaskBox::new(GFP_KERNEL)?;
> +///
> +///     assert_eq!(mask.weight(), 0);
> +///     mask.set(2);
> +///     assert_eq!(mask.weight(), 1);
> +///     mask.set(3);
> +///     assert_eq!(mask.weight(), 2);

Yeah, you don't import cpumask_test_cpu() for some reason, and has
to use .weight() here to illustrate how it works. For an example, I
think it's a rather bad example.

Also, because you have atomic setters (except setall) and non-atomic 
getter, I think you most likely abuse the atomic API in your code.
Please share your driver for the next round. 

I think it would be better to move this implementation together with
the client code. Now that we merged cpumask helpers and stabilized the
API, there's no need to merge dead lib code without clients.

Thanks,
Yury

Re: [PATCH V4 1/2] rust: Add initial cpumask abstractions

[Burak Emir]

On Wed, Apr 2, 2025 at 5:47 PM Yury Norov <yury.norov@gmail.com> wrote:
>
> On Wed, Apr 02, 2025 at 11:08:42AM +0530, Viresh Kumar wrote:
> > Wed,  2 Apr 2025 11:08:42 +0530
> > Message-Id: <35f4223be4a51139348fed82420481b370d7b1db.1743572195.git.viresh.kumar@linaro.org>
> > X-Mailer: git-send-email 2.31.1.272.g89b43f80a514
> > In-Reply-To: <cover.1743572195.git.viresh.kumar@linaro.org>
> > References: <cover.1743572195.git.viresh.kumar@linaro.org>
> > MIME-Version: 1.0
> > Content-Transfer-Encoding: 8bit
> > Status: O
> > Content-Length: 11430
> > Lines: 334
> >
> > Add initial Rust abstractions for struct cpumask, covering a subset of
> > its APIs. Additional APIs can be added as needed.
> >
> > These abstractions will be used in upcoming Rust support for cpufreq and
> > OPP frameworks.
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  rust/kernel/cpumask.rs | 301 +++++++++++++++++++++++++++++++++++++++++
> >  rust/kernel/lib.rs     |   1 +
> >  2 files changed, 302 insertions(+)
> >  create mode 100644 rust/kernel/cpumask.rs
> >
> > diff --git a/rust/kernel/cpumask.rs b/rust/kernel/cpumask.rs
> > new file mode 100644
> > index 000000000000..792210a77770
> > --- /dev/null
> > +++ b/rust/kernel/cpumask.rs
> > @@ -0,0 +1,301 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! CPU Mask abstractions.
> > +//!
> > +//! C header: [`include/linux/cpumask.h`](srctree/include/linux/cpumask.h)
> > +
> > +use crate::{
> > +    alloc::{AllocError, Flags},
> > +    bindings,
> > +    prelude::*,
> > +    types::Opaque,
> > +};
> > +
> > +#[cfg(CONFIG_CPUMASK_OFFSTACK)]
> > +use core::ptr::{self, NonNull};
> > +
> > +#[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
> > +use core::mem::MaybeUninit;
> > +
> > +use core::ops::{Deref, DerefMut};
> > +
> > +/// A CPU Mask.
> > +///
> > +/// This represents the Rust abstraction for the C `struct cpumask`.
> > +///
> > +/// # Invariants
> > +///
> > +/// A [`Cpumask`] instance always corresponds to a valid C `struct cpumask`.
> > +///
> > +/// The callers must ensure that the `struct cpumask` is valid for access and remains valid for the
> > +/// lifetime of the returned reference.
> > +///
> > +/// ## Examples
> > +///
> > +/// The following example demonstrates how to update a [`Cpumask`].
> > +///
> > +/// ```
> > +/// use kernel::bindings;
> > +/// use kernel::cpumask::Cpumask;
> > +///
> > +/// fn set_clear_cpu(ptr: *mut bindings::cpumask, set_cpu: u32, clear_cpu: i32) {
> > +///     // SAFETY: The `ptr` is valid for writing and remains valid for the lifetime of the
> > +///     // returned reference.
> > +///     let mask = unsafe { Cpumask::from_raw_mut(ptr) };
> > +///     mask.set(set_cpu);
> > +///     mask.clear(clear_cpu);
> > +/// }
> > +/// ```
> > +#[repr(transparent)]
> > +pub struct Cpumask(Opaque<bindings::cpumask>);
> > +
> > +impl Cpumask {
> > +    /// Creates a mutable reference to an existing `struct cpumask` pointer.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// The caller must ensure that `ptr` is valid for writing and remains valid for the lifetime
> > +    /// of the returned reference.
> > +    pub unsafe fn from_raw_mut<'a>(ptr: *mut bindings::cpumask) -> &'a mut Self {
> > +        // SAFETY: Guaranteed by the safety requirements of the function.
> > +        //
> > +        // INVARIANT: The caller ensures that `ptr` is valid for writing and remains valid for the
> > +        // lifetime of the returned reference.
> > +        unsafe { &mut *ptr.cast() }
> > +    }
> > +
> > +    /// Creates a reference to an existing `struct cpumask` pointer.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// The caller must ensure that `ptr` is valid for reading and remains valid for the lifetime
> > +    /// of the returned reference.
> > +    pub unsafe fn from_raw<'a>(ptr: *const bindings::cpumask) -> &'a Self {
> > +        // SAFETY: Guaranteed by the safety requirements of the function.
> > +        //
> > +        // INVARIANT: The caller ensures that `ptr` is valid for reading and remains valid for the
> > +        // lifetime of the returned reference.
> > +        unsafe { &*ptr.cast() }
> > +    }
> > +
> > +    /// Obtain the raw `struct cpumask` pointer.
> > +    pub fn as_raw(&self) -> *mut bindings::cpumask {
> > +        self as *const Cpumask as *mut bindings::cpumask
> > +    }
> > +
> > +    /// Set `cpu` in the cpumask.
> > +    ///
> > +    /// Equivalent to the kernel's `cpumask_set_cpu` API.
> > +    #[inline]
> > +    pub fn set(&mut self, cpu: u32) {
> > +        // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `cpumask_set_cpus`.
> > +        unsafe { bindings::cpumask_set_cpu(cpu, self.as_raw()) };
> > +    }
>
> Alright, this is an atomic operation. For bitmaps in rust, Burak and
> Alice decided to switch naming, so 'set()' in C becomes 'set_atomic()'
> in rust, and correspondingly, '__set()' becomes 'set()'.
>
> I think it's maybe OK to switch naming for a different language. But
> guys, can you please be consistent once you made a decision?
>
> Burak, Alice, please comment.

I really like the explicit naming convention that includes "atomic" if
an operation is atomic.
It seems also consistent with std library.

> Regardless, without looking at the end code I can't judge if you need
> atomic or non-atomic ops. Can you link the project that actually uses
> this API? Better if you just prepend that series with this 2 patches
> and move them together.

The type &mut self gives it away: the Rust type system enforces
exclusive access here due to aliasing rules.
So a non-atomic operation is sufficient here.

> > +    /// Clear `cpu` in the cpumask.
> > +    ///
> > +    /// Equivalent to the kernel's `cpumask_clear_cpu` API.
> > +    #[inline]
> > +    pub fn clear(&mut self, cpu: i32) {
> > +        // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `cpumask_clear_cpu`.
> > +        unsafe { bindings::cpumask_clear_cpu(cpu, self.as_raw()) };
> > +    }
> > +
> > +    /// Set all CPUs in the cpumask.
> > +    ///
> > +    /// Equivalent to the kernel's `cpumask_setall` API.
> > +    #[inline]
> > +    pub fn set_all(&mut self) {
> > +        // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `cpumask_setall`.
> > +        unsafe { bindings::cpumask_setall(self.as_raw()) };
> > +    }
>
> Can you keep the name as 'setall'? This would help those grepping
> methods roots in C sources.
>
> > +    /// Get weight of the cpumask.
> > +    ///
> > +    /// Equivalent to the kernel's `cpumask_weight` API.
> > +    #[inline]
> > +    pub fn weight(&self) -> u32 {
> > +        // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `cpumask_weight`.
> > +        unsafe { bindings::cpumask_weight(self.as_raw()) }
> > +    }
> > +
> > +    /// Copy cpumask.
> > +    ///
> > +    /// Equivalent to the kernel's `cpumask_copy` API.
> > +    #[inline]
> > +    pub fn copy(&self, dstp: &mut Self) {
> > +        // SAFETY: By the type invariant, `Self::as_raw` is a valid argument to `cpumask_copy`.
> > +        unsafe { bindings::cpumask_copy(dstp.as_raw(), self.as_raw()) };
> > +    }
> > +}
> > +
> > +/// A CPU Mask pointer.
> > +///
> > +/// This represents the Rust abstraction for the C `struct cpumask_var_t`.
> > +///
> > +/// # Invariants
> > +///
> > +/// A [`CpumaskBox`] instance always corresponds to a valid C `struct cpumask_var_t`.
>
> Can you keep the C name? Maybe CpumaskVar? Or this 'Box' has special
> meaning in rust?
>
> > +///
> > +/// The callers must ensure that the `struct cpumask_var_t` is valid for access and remains valid
> > +/// for the lifetime of [`CpumaskBox`].
> > +///
> > +/// ## Examples
> > +///
> > +/// The following example demonstrates how to create and update a [`CpumaskBox`].
> > +///
> > +/// ```
> > +/// use kernel::cpumask::CpumaskBox;
> > +/// use kernel::error::Result;
> > +///
> > +/// fn cpumask_foo() -> Result {
>
> cpumask_foo() what? This is not a good name for test, neither
> for an example.
>
> > +///     let mut mask = CpumaskBox::new(GFP_KERNEL)?;
> > +///
> > +///     assert_eq!(mask.weight(), 0);
> > +///     mask.set(2);
> > +///     assert_eq!(mask.weight(), 1);
> > +///     mask.set(3);
> > +///     assert_eq!(mask.weight(), 2);
>
> Yeah, you don't import cpumask_test_cpu() for some reason, and has
> to use .weight() here to illustrate how it works. For an example, I
> think it's a rather bad example.
>
> Also, because you have atomic setters (except setall) and non-atomic
> getter, I think you most likely abuse the atomic API in your code.
> Please share your driver for the next round.
>
> I think it would be better to move this implementation together with
> the client code. Now that we merged cpumask helpers and stabilized the
> API, there's no need to merge dead lib code without clients.
>
> Thanks,
> Yury

Re: [PATCH V4 2/2] MAINTAINERS: Add entry for Rust bitmap API

[Viresh Kumar]

On 02-04-25, 11:01, Yury Norov wrote:
> On Wed, Apr 02, 2025 at 01:39:22PM +0200, Miguel Ojeda wrote:
> > On Wed, Apr 2, 2025 at 7:38 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > Update the MAINTAINERS file to include the Rust abstractions for bitmap
> > > API.
> > >
> > > Yury has indicated that he does not wish to maintain the Rust code but
> > > would like to be listed as a reviewer.
> > 
> > Will patches go through the BITMAP API tree, then? i.e. you will
> > maintain it by sending patches to Yury's tree, right? That is great,
> > just wanted to confirm after all the discussions if I missed anything.

Right.

> I didn't plan to, but I can move the bindings if you find it
> reasonable.

Sounds good. Thanks.

-- 
viresh

Re: [PATCH V4 1/2] rust: Add initial cpumask abstractions

[Viresh Kumar]

On 02-04-25, 11:47, Yury Norov wrote:
> On Wed, Apr 02, 2025 at 11:08:42AM +0530, Viresh Kumar wrote:
> > +    /// Set `cpu` in the cpumask.
> > +    ///
> > +    /// Equivalent to the kernel's `cpumask_set_cpu` API.
> > +    #[inline]
> > +    pub fn set(&mut self, cpu: u32) {
> > +        // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `cpumask_set_cpus`.
> > +        unsafe { bindings::cpumask_set_cpu(cpu, self.as_raw()) };
> > +    }
> 
> Alright, this is an atomic operation. For bitmaps in rust, Burak and
> Alice decided to switch naming, so 'set()' in C becomes 'set_atomic()'
> in rust, and correspondingly, '__set()' becomes 'set()'.
> 
> I think it's maybe OK to switch naming for a different language. But
> guys, can you please be consistent once you made a decision?
> 
> Burak, Alice, please comment.
> 
> Regardless, without looking at the end code I can't judge if you need
> atomic or non-atomic ops.

I don't think I need it to be atomic:

> Can you link the project that actually uses this API?

https://lore.kernel.org/all/3054a0eb12330914cd6165ad460d9844ee8c19e2.1738832119.git.viresh.kumar@linaro.org/
(search for "mask.set" here).

> Better if you just prepend that series with this 2 patches
> and move them together.

That's why I did earlier, but now separated them. I can provide links
in both the series to each other to make this easier to look though.

I can also prepend that series with these patches once they are fully
reviewed.

> > +    pub fn set_all(&mut self) {
> > +        // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `cpumask_setall`.
> > +        unsafe { bindings::cpumask_setall(self.as_raw()) };
> > +    }
> 
> Can you keep the name as 'setall'? This would help those grepping
> methods roots in C sources.

Sure.

> > +/// A CPU Mask pointer.
> > +///
> > +/// This represents the Rust abstraction for the C `struct cpumask_var_t`.
> > +///
> > +/// # Invariants
> > +///
> > +/// A [`CpumaskBox`] instance always corresponds to a valid C `struct cpumask_var_t`.
> 
> Can you keep the C name? Maybe CpumaskVar? Or this 'Box' has special
> meaning in rust?

'Box' means heap allocated normally but CpumaskVar sounds better.

> > +/// fn cpumask_foo() -> Result {
> 
> cpumask_foo() what? This is not a good name for test, neither
> for an example.
> 
> > +///     let mut mask = CpumaskBox::new(GFP_KERNEL)?;
> > +///
> > +///     assert_eq!(mask.weight(), 0);
> > +///     mask.set(2);
> > +///     assert_eq!(mask.weight(), 1);
> > +///     mask.set(3);
> > +///     assert_eq!(mask.weight(), 2);
> 
> Yeah, you don't import cpumask_test_cpu() for some reason, and has
> to use .weight() here to illustrate how it works. For an example, I
> think it's a rather bad example.

Okay, I will import and use cpumask_test_cpu(), use that for testing
the output after every operation and do weight check once at the last
to make sure all CPUs are set in the mask.

> Also, because you have atomic setters (except setall) and non-atomic 
> getter, I think you most likely abuse the atomic API in your code.
> Please share your driver for the next round. 

That is a lot of patches adding bindings for OPP/cpufreq frameworks
and a driver. Not sue if I should be mixing them again with this
series. I will provide links though.

> I think it would be better to move this implementation together with
> the client code. Now that we merged cpumask helpers and stabilized the
> API, there's no need to merge dead lib code without clients.

I was expecting to get this merged separately, so I don't need to push
a big chunk together (where reviews eventually take lot more time).

But I am okay to merge it all once and just collect Acks until the
time the client changes are fully reviewed.

-- 
viresh

Re: [PATCH V4 1/2] rust: Add initial cpumask abstractions

[Viresh Kumar]

On 02-04-25, 18:00, Burak Emir wrote:
> On Wed, Apr 2, 2025 at 5:47 PM Yury Norov <yury.norov@gmail.com> wrote:
> > On Wed, Apr 02, 2025 at 11:08:42AM +0530, Viresh Kumar wrote:
> > > +    pub fn set(&mut self, cpu: u32) {
> > > +        // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `cpumask_set_cpus`.
> > > +        unsafe { bindings::cpumask_set_cpu(cpu, self.as_raw()) };
> > > +    }
> >
> > Alright, this is an atomic operation. For bitmaps in rust, Burak and
> > Alice decided to switch naming, so 'set()' in C becomes 'set_atomic()'
> > in rust, and correspondingly, '__set()' becomes 'set()'.
> >
> > I think it's maybe OK to switch naming for a different language. But
> > guys, can you please be consistent once you made a decision?
> >
> > Burak, Alice, please comment.
> 
> I really like the explicit naming convention that includes "atomic" if
> an operation is atomic.
> It seems also consistent with std library.
> 
> > Regardless, without looking at the end code I can't judge if you need
> > atomic or non-atomic ops. Can you link the project that actually uses
> > this API? Better if you just prepend that series with this 2 patches
> > and move them together.
> 
> The type &mut self gives it away: the Rust type system enforces
> exclusive access here due to aliasing rules.
> So a non-atomic operation is sufficient here.

I should leave it as is then, right ? Don't rename to set_atomic() ?

-- 
viresh

Re: [PATCH V4 1/2] rust: Add initial cpumask abstractions

[Burak Emir]

On Fri, Apr 11, 2025 at 9:09 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 02-04-25, 18:00, Burak Emir wrote:
> > On Wed, Apr 2, 2025 at 5:47 PM Yury Norov <yury.norov@gmail.com> wrote:
> > > On Wed, Apr 02, 2025 at 11:08:42AM +0530, Viresh Kumar wrote:
> > > > +    pub fn set(&mut self, cpu: u32) {
> > > > +        // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `cpumask_set_cpus`.
> > > > +        unsafe { bindings::cpumask_set_cpu(cpu, self.as_raw()) };
> > > > +    }
> > >
> > > Alright, this is an atomic operation. For bitmaps in rust, Burak and
> > > Alice decided to switch naming, so 'set()' in C becomes 'set_atomic()'
> > > in rust, and correspondingly, '__set()' becomes 'set()'.
> > >
> > > I think it's maybe OK to switch naming for a different language. But
> > > guys, can you please be consistent once you made a decision?
> > >
> > > Burak, Alice, please comment.
> >
> > I really like the explicit naming convention that includes "atomic" if
> > an operation is atomic.
> > It seems also consistent with std library.
> >
> > > Regardless, without looking at the end code I can't judge if you need
> > > atomic or non-atomic ops. Can you link the project that actually uses
> > > this API? Better if you just prepend that series with this 2 patches
> > > and move them together.
> >
> > The type &mut self gives it away: the Rust type system enforces
> > exclusive access here due to aliasing rules.
> > So a non-atomic operation is sufficient here.
>
> I should leave it as is then, right ? Don't rename to set_atomic() ?
>

The issue is that your code calls `bindings::cpumask_set_cpu` calls C
`cpumask_set_cpu` which calls `set_bit` which is atomic.
So if it was only about naming, you should call it `set_atomic`.

More important than the name is the meaning: you don't actually *need*
or want to call to call (atomic) `set_bit` underneath, since you have
`&mut self`.
So you should rather introduce and call `bindings::__cpumask_set_cpu`
which would call the C `__cpumask_set_cpu` which calls the
(non-atomic) `__set_bit`.

Example where we need bindings helpers for both atomic and non-atomic
versions: https://lore.kernel.org/rust-for-linux/20250327161617.117748-3-bqe@google.com/

cheers,
Burak