[PATCH 0/2] Rust: Add cpumask abstractions

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

This was earlier sent as part of a larger series [1] and it was suggested to
send these separately.

Depends on: [2].

--
Viresh


[1] https://lore.kernel.org/all/cover.1738832118.git.viresh.kumar@linaro.org/
[2] https://lore.kernel.org/all/20250224233938.3158-1-yury.norov@gmail.com/

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

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

-- 
2.31.1.272.g89b43f80a514

[PATCH 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 | 168 +++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs     |   1 +
 2 files changed, 169 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..13864424420b
--- /dev/null
+++ b/rust/kernel/cpumask.rs
@@ -0,0 +1,168 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! CPU mask abstractions.
+//!
+//! C header: [`include/linux/cpumask.h`](srctree/include/linux/cpumask.h)
+
+use crate::{bindings, error::Result, prelude::ENOMEM};
+
+#[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+use crate::prelude::{KBox, GFP_KERNEL};
+
+#[cfg(CONFIG_CPUMASK_OFFSTACK)]
+use core::ptr;
+
+/// A simple implementation of `struct cpumask` from the C code.
+pub struct Cpumask {
+    ptr: *mut bindings::cpumask,
+    owned: bool,
+}
+
+impl Cpumask {
+    /// Creates cpumask.
+    #[cfg(CONFIG_CPUMASK_OFFSTACK)]
+    fn new_inner(empty: bool) -> Result<Self> {
+        let mut ptr: *mut bindings::cpumask = ptr::null_mut();
+
+        // SAFETY: Depending on the value of `gfp_flags`, this call may sleep. Other than that, it
+        // is always safe to call this method.
+        if !unsafe {
+            if empty {
+                bindings::zalloc_cpumask_var(&mut ptr, bindings::GFP_KERNEL)
+            } else {
+                bindings::alloc_cpumask_var(&mut ptr, bindings::GFP_KERNEL)
+            }
+        } {
+            return Err(ENOMEM);
+        }
+
+        Ok(Self { ptr, owned: true })
+    }
+
+    /// Creates cpumask.
+    #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+    fn new_inner(empty: bool) -> Result<Self> {
+        let ptr = KBox::into_raw(KBox::new([bindings::cpumask::default(); 1], GFP_KERNEL)?);
+
+        // SAFETY: Depending on the value of `gfp_flags`, this call may sleep. Other than that, it
+        // is always safe to call this method.
+        if !unsafe {
+            if empty {
+                bindings::zalloc_cpumask_var(ptr, bindings::GFP_KERNEL)
+            } else {
+                bindings::alloc_cpumask_var(ptr, bindings::GFP_KERNEL)
+            }
+        } {
+            return Err(ENOMEM);
+        }
+
+        Ok(Self {
+            ptr: ptr as *mut _,
+            owned: true,
+        })
+    }
+
+    /// Creates empty cpumask.
+    pub fn new() -> Result<Self> {
+        Self::new_inner(true)
+    }
+
+    /// Creates uninitialized cpumask.
+    fn new_uninit() -> Result<Self> {
+        Self::new_inner(false)
+    }
+
+    /// Clones cpumask.
+    pub fn try_clone(&self) -> Result<Self> {
+        let mut cpumask = Self::new_uninit()?;
+
+        self.copy(&mut cpumask);
+        Ok(cpumask)
+    }
+
+    /// Creates a new abstraction instance of an existing `struct cpumask` pointer.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that `ptr` is valid, and non-null.
+    #[cfg(CONFIG_CPUMASK_OFFSTACK)]
+    pub unsafe fn get_cpumask(ptr: &mut *mut bindings::cpumask) -> Self {
+        Self {
+            ptr: *ptr,
+            owned: false,
+        }
+    }
+
+    /// Creates a new abstraction instance of an existing `struct cpumask` pointer.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that `ptr` is valid, and non-null.
+    #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+    pub unsafe fn get_cpumask(ptr: &mut bindings::cpumask_var_t) -> Self {
+        Self {
+            ptr: ptr as *mut _,
+            owned: false,
+        }
+    }
+
+    /// Obtain the raw `struct cpumask *`.
+    pub fn as_raw(&mut self) -> *mut bindings::cpumask {
+        self.ptr
+    }
+
+    /// Sets CPU in the cpumask.
+    ///
+    /// Update the cpumask with a single CPU.
+    pub fn set(&mut self, cpu: u32) {
+        // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
+        // call `cpumask_set_cpus()` for any CPU.
+        unsafe { bindings::cpumask_set_cpu(cpu, self.ptr) };
+    }
+
+    /// Clears CPU in the cpumask.
+    ///
+    /// Update the cpumask with a single CPU.
+    pub fn clear(&mut self, cpu: i32) {
+        // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
+        // call `cpumask_clear_cpu()` for any CPU.
+        unsafe { bindings::cpumask_clear_cpu(cpu, self.ptr) };
+    }
+
+    /// Sets all CPUs in the cpumask.
+    pub fn set_all(&mut self) {
+        // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
+        // call `cpumask_setall()`.
+        unsafe { bindings::cpumask_setall(self.ptr) };
+    }
+
+    /// Gets weight of a cpumask.
+    pub fn weight(&self) -> u32 {
+        // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
+        // call `cpumask_weight()`.
+        unsafe { bindings::cpumask_weight(self.ptr) }
+    }
+
+    /// Copies cpumask.
+    pub fn copy(&self, dstp: &mut Self) {
+        // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
+        // call `cpumask_copy()`.
+        unsafe { bindings::cpumask_copy(dstp.as_raw(), self.ptr) };
+    }
+}
+
+impl Drop for Cpumask {
+    fn drop(&mut self) {
+        if self.owned {
+            // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe
+            // to call `free_cpumask_var()`.
+            unsafe { bindings::free_cpumask_var(self.ptr) }
+
+            #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+            // SAFETY: The pointer was earlier initialized from the result of `KBox::into_raw()`.
+            unsafe {
+                drop(KBox::from_raw(self.ptr))
+            };
+        }
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 496ed32b0911..efbd7be98dab 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

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

[Alice Ryhl]

On Tue, Feb 25, 2025 at 10:47 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> 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 | 168 +++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs     |   1 +
>  2 files changed, 169 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..13864424420b
> --- /dev/null
> +++ b/rust/kernel/cpumask.rs
> @@ -0,0 +1,168 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! CPU mask abstractions.
> +//!
> +//! C header: [`include/linux/cpumask.h`](srctree/include/linux/cpumask.h)
> +
> +use crate::{bindings, error::Result, prelude::ENOMEM};
> +
> +#[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
> +use crate::prelude::{KBox, GFP_KERNEL};
> +
> +#[cfg(CONFIG_CPUMASK_OFFSTACK)]
> +use core::ptr;
> +
> +/// A simple implementation of `struct cpumask` from the C code.
> +pub struct Cpumask {
> +    ptr: *mut bindings::cpumask,
> +    owned: bool,
> +}
> +
> +impl Cpumask {
> +    /// Creates cpumask.
> +    #[cfg(CONFIG_CPUMASK_OFFSTACK)]
> +    fn new_inner(empty: bool) -> Result<Self> {
> +        let mut ptr: *mut bindings::cpumask = ptr::null_mut();
> +
> +        // SAFETY: Depending on the value of `gfp_flags`, this call may sleep. Other than that, it
> +        // is always safe to call this method.
> +        if !unsafe {
> +            if empty {
> +                bindings::zalloc_cpumask_var(&mut ptr, bindings::GFP_KERNEL)
> +            } else {
> +                bindings::alloc_cpumask_var(&mut ptr, bindings::GFP_KERNEL)
> +            }
> +        } {
> +            return Err(ENOMEM);
> +        }
> +
> +        Ok(Self { ptr, owned: true })
> +    }
> +
> +    /// Creates cpumask.
> +    #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
> +    fn new_inner(empty: bool) -> Result<Self> {
> +        let ptr = KBox::into_raw(KBox::new([bindings::cpumask::default(); 1], GFP_KERNEL)?);

I don't really understand this CPUMASK_OFFSTACK logic. You seem to
always allocate memory, but if OFFSTACK=n, then shouldn't it be on the
stack ...?

> +        // SAFETY: Depending on the value of `gfp_flags`, this call may sleep. Other than that, it
> +        // is always safe to call this method.
> +        if !unsafe {
> +            if empty {
> +                bindings::zalloc_cpumask_var(ptr, bindings::GFP_KERNEL)
> +            } else {
> +                bindings::alloc_cpumask_var(ptr, bindings::GFP_KERNEL)
> +            }
> +        } {
> +            return Err(ENOMEM);
> +        }
> +
> +        Ok(Self {
> +            ptr: ptr as *mut _,
> +            owned: true,
> +        })
> +    }
> +
> +    /// Creates empty cpumask.
> +    pub fn new() -> Result<Self> {
> +        Self::new_inner(true)
> +    }
> +
> +    /// Creates uninitialized cpumask.
> +    fn new_uninit() -> Result<Self> {
> +        Self::new_inner(false)
> +    }
> +
> +    /// Clones cpumask.
> +    pub fn try_clone(&self) -> Result<Self> {
> +        let mut cpumask = Self::new_uninit()?;
> +
> +        self.copy(&mut cpumask);
> +        Ok(cpumask)
> +    }
> +
> +    /// Creates a new abstraction instance of an existing `struct cpumask` pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that `ptr` is valid, and non-null.
> +    #[cfg(CONFIG_CPUMASK_OFFSTACK)]
> +    pub unsafe fn get_cpumask(ptr: &mut *mut bindings::cpumask) -> Self {
> +        Self {
> +            ptr: *ptr,
> +            owned: false,

Using an owned variable in this way isn't great, IMO. We should use
two different types such as Owned<CpuMask> and &CpuMask to represent
owned vs borrowed instead.

> +        }
> +    }
> +
> +    /// Creates a new abstraction instance of an existing `struct cpumask` pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that `ptr` is valid, and non-null.
> +    #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
> +    pub unsafe fn get_cpumask(ptr: &mut bindings::cpumask_var_t) -> Self {
> +        Self {
> +            ptr: ptr as *mut _,
> +            owned: false,
> +        }
> +    }
> +
> +    /// Obtain the raw `struct cpumask *`.
> +    pub fn as_raw(&mut self) -> *mut bindings::cpumask {
> +        self.ptr
> +    }
> +
> +    /// Sets CPU in the cpumask.
> +    ///
> +    /// Update the cpumask with a single CPU.
> +    pub fn set(&mut self, cpu: u32) {
> +        // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
> +        // call `cpumask_set_cpus()` for any CPU.
> +        unsafe { bindings::cpumask_set_cpu(cpu, self.ptr) };
> +    }
> +
> +    /// Clears CPU in the cpumask.
> +    ///
> +    /// Update the cpumask with a single CPU.
> +    pub fn clear(&mut self, cpu: i32) {
> +        // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
> +        // call `cpumask_clear_cpu()` for any CPU.
> +        unsafe { bindings::cpumask_clear_cpu(cpu, self.ptr) };
> +    }
> +
> +    /// Sets all CPUs in the cpumask.
> +    pub fn set_all(&mut self) {
> +        // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
> +        // call `cpumask_setall()`.
> +        unsafe { bindings::cpumask_setall(self.ptr) };
> +    }
> +
> +    /// Gets weight of a cpumask.
> +    pub fn weight(&self) -> u32 {
> +        // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
> +        // call `cpumask_weight()`.
> +        unsafe { bindings::cpumask_weight(self.ptr) }
> +    }
> +
> +    /// Copies cpumask.
> +    pub fn copy(&self, dstp: &mut Self) {
> +        // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
> +        // call `cpumask_copy()`.
> +        unsafe { bindings::cpumask_copy(dstp.as_raw(), self.ptr) };
> +    }
> +}
> +
> +impl Drop for Cpumask {
> +    fn drop(&mut self) {
> +        if self.owned {
> +            // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe
> +            // to call `free_cpumask_var()`.
> +            unsafe { bindings::free_cpumask_var(self.ptr) }

This is missing a semicolon, but it's not the last statement in the
block. Did you compile this with CPUMASK_OFFSTACK=n? I don't think it
compiles with that setting.

> +            #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
> +            // SAFETY: The pointer was earlier initialized from the result of `KBox::into_raw()`.
> +            unsafe {
> +                drop(KBox::from_raw(self.ptr))
> +            };

This looks like you did not run rustfmt.

Alice

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

[Viresh Kumar]

On 25-02-25, 10:55, Alice Ryhl wrote:
> On Tue, Feb 25, 2025 at 10:47 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > +impl Drop for Cpumask {
> > +    fn drop(&mut self) {
> > +        if self.owned {
> > +            // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe
> > +            // to call `free_cpumask_var()`.
> > +            unsafe { bindings::free_cpumask_var(self.ptr) }
> 
> This is missing a semicolon, but it's not the last statement in the
> block. Did you compile this with CPUMASK_OFFSTACK=n? I don't think it
> compiles with that setting.

I would always add a semicolon here, yeah I missed adding that but ..

I have missed minor things before sending a series a few times in the
past and this one really scared me thinking here I did it again :)

Though I was sure that I have built the code with both
CPUMASK_OFFSTACK=y and =n, I performed the builds again and it worked
(again).  That confused me even more :)

And here is what I think is happening here (which makes it build fine
accidentally):
- free_cpumask_var() has a return type of void.
- The block {} allows it to build fine without a semicolon here.
- I performed a simple test for this [1] and it works too.

> > +            #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
> > +            // SAFETY: The pointer was earlier initialized from the result of `KBox::into_raw()`.
> > +            unsafe {
> > +                drop(KBox::from_raw(self.ptr))
> > +            };
> 
> This looks like you did not run rustfmt.

I did this:

make CLIPPY=1 rustfmtcheck ARCH=arm64 O=../barm64t/ -j8 CROSS_COMPILE=aarch64-linux-gnu- CONFIG_DEBUG_SECTION_MISMATCH=y

I hope that is all I need ? I checked again with both CONFIG options,
doesn't complain with rustc 1.84.1.

-- 
viresh

[1] https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=70cd7d31633d98774a088fed68ebb00d

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

[Alice Ryhl]

On Tue, Feb 25, 2025 at 11:54 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 25-02-25, 10:55, Alice Ryhl wrote:
> > On Tue, Feb 25, 2025 at 10:47 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > +impl Drop for Cpumask {
> > > +    fn drop(&mut self) {
> > > +        if self.owned {
> > > +            // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe
> > > +            // to call `free_cpumask_var()`.
> > > +            unsafe { bindings::free_cpumask_var(self.ptr) }
> >
> > This is missing a semicolon, but it's not the last statement in the
> > block. Did you compile this with CPUMASK_OFFSTACK=n? I don't think it
> > compiles with that setting.
>
> I would always add a semicolon here, yeah I missed adding that but ..
>
> I have missed minor things before sending a series a few times in the
> past and this one really scared me thinking here I did it again :)
>
> Though I was sure that I have built the code with both
> CPUMASK_OFFSTACK=y and =n, I performed the builds again and it worked
> (again).  That confused me even more :)
>
> And here is what I think is happening here (which makes it build fine
> accidentally):
> - free_cpumask_var() has a return type of void.
> - The block {} allows it to build fine without a semicolon here.
> - I performed a simple test for this [1] and it works too.

Ah, you are right, the block makes it work in this particular case.

> > > +            #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
> > > +            // SAFETY: The pointer was earlier initialized from the result of `KBox::into_raw()`.
> > > +            unsafe {
> > > +                drop(KBox::from_raw(self.ptr))
> > > +            };
> >
> > This looks like you did not run rustfmt.
>
> I did this:
>
> make CLIPPY=1 rustfmtcheck ARCH=arm64 O=../barm64t/ -j8 CROSS_COMPILE=aarch64-linux-gnu- CONFIG_DEBUG_SECTION_MISMATCH=y
>
> I hope that is all I need ? I checked again with both CONFIG options,
> doesn't complain with rustc 1.84.1.

Hmm, ok. I would have expected it to format on one line:
unsafe { drop(KBox::from_raw(self.ptr)) };

That is what normally happens when the semi-colon is outside the
block. Not sure why it did not happen in this case.

Alice

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

[Viresh Kumar]

On 25-02-25, 12:34, Alice Ryhl wrote:
> Hmm, ok. I would have expected it to format on one line:
> unsafe { drop(KBox::from_raw(self.ptr)) };

I did try this earlier and rustfmt suggested to format it the way I
have done it currently :)

> That is what normally happens when the semi-colon is outside the
> block. Not sure why it did not happen in this case.

-- 
viresh

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

[Alice Ryhl]

On Tue, Feb 25, 2025 at 12:36 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 25-02-25, 12:34, Alice Ryhl wrote:
> > Hmm, ok. I would have expected it to format on one line:
> > unsafe { drop(KBox::from_raw(self.ptr)) };
>
> I did try this earlier and rustfmt suggested to format it the way I
> have done it currently :)
>
> > That is what normally happens when the semi-colon is outside the
> > block. Not sure why it did not happen in this case.

Very weird. I'd guess that the #[cfg] is at fault.

Alice

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

[Viresh Kumar]

On 25-02-25, 12:39, Alice Ryhl wrote:
> On Tue, Feb 25, 2025 at 12:36 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 25-02-25, 12:34, Alice Ryhl wrote:
> > > Hmm, ok. I would have expected it to format on one line:
> > > unsafe { drop(KBox::from_raw(self.ptr)) };
> >
> > I did try this earlier and rustfmt suggested to format it the way I
> > have done it currently :)
> >
> > > That is what normally happens when the semi-colon is outside the
> > > block. Not sure why it did not happen in this case.
> 
> Very weird. I'd guess that the #[cfg] is at fault.

Dropping #[cfg] does make it complain with current code and suggests
to format on one line.

-- 
viresh

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

[Viresh Kumar]

On 25-02-25, 10:55, Alice Ryhl wrote:
> On Tue, Feb 25, 2025 at 10:47 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > +    /// Creates cpumask.
> > +    #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
> > +    fn new_inner(empty: bool) -> Result<Self> {
> > +        let ptr = KBox::into_raw(KBox::new([bindings::cpumask::default(); 1], GFP_KERNEL)?);
> 
> I don't really understand this CPUMASK_OFFSTACK logic. You seem to
> always allocate memory, but if OFFSTACK=n, then shouldn't it be on the
> stack ...?

IIUC, the idea of the config option is to prevent stack overflow on
systems with high number of CPUs (> 256), in which case the memory for
the masks is allocated dynamically. Otherwise a local variable, in a
function or a struct (which may itself get allocated dynamically) is
fine.

In the CONFIG_CPUMASK_OFFSTACK=y case, the cpumask C core does the
allocation and the Rust code doesn't need to take care of the same.

In the CONFIG_CPUMASK_OFFSTACK=n case, the allocation must be done by
the caller (on stack or heap) and the cpumask C core will only clear
the mask.

I tried with an on-stack variable earlier but ran into problems as the
memory is shared with the C FFI and Rust moves it unless it is pinned.

One easy way to make it work was using Box, which keeps the code
simple.

Should I do it differently ?

-- 
viresh

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

[Alice Ryhl]

On Tue, Feb 25, 2025 at 12:48 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 25-02-25, 10:55, Alice Ryhl wrote:
> > On Tue, Feb 25, 2025 at 10:47 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > +    /// Creates cpumask.
> > > +    #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
> > > +    fn new_inner(empty: bool) -> Result<Self> {
> > > +        let ptr = KBox::into_raw(KBox::new([bindings::cpumask::default(); 1], GFP_KERNEL)?);
> >
> > I don't really understand this CPUMASK_OFFSTACK logic. You seem to
> > always allocate memory, but if OFFSTACK=n, then shouldn't it be on the
> > stack ...?
>
> IIUC, the idea of the config option is to prevent stack overflow on
> systems with high number of CPUs (> 256), in which case the memory for
> the masks is allocated dynamically. Otherwise a local variable, in a
> function or a struct (which may itself get allocated dynamically) is
> fine.
>
> In the CONFIG_CPUMASK_OFFSTACK=y case, the cpumask C core does the
> allocation and the Rust code doesn't need to take care of the same.
>
> In the CONFIG_CPUMASK_OFFSTACK=n case, the allocation must be done by
> the caller (on stack or heap) and the cpumask C core will only clear
> the mask.
>
> I tried with an on-stack variable earlier but ran into problems as the
> memory is shared with the C FFI and Rust moves it unless it is pinned.

Is it a problem if a value of type struct cpumask is moved? It looks
like it is just an array of longs?

Alice

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

[Viresh Kumar]

On Tue, 25 Feb 2025 at 17:23, Alice Ryhl <aliceryhl@google.com> wrote:

> Is it a problem if a value of type struct cpumask is moved? It looks
> like it is just an array of longs?

With the current code, if I replace the Box with an on-stack variable,
the kernel crashes.

In my usecase, the pointer to the cpumask array is sent to the OPP
core, which may update the content, though it doesn't save the pointer.

But for another usecase, the C code may end up saving the pointer.

--
Viresh

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

[Alice Ryhl]

On Tue, Feb 25, 2025 at 6:02 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On Tue, 25 Feb 2025 at 17:23, Alice Ryhl <aliceryhl@google.com> wrote:
>
> > Is it a problem if a value of type struct cpumask is moved? It looks
> > like it is just an array of longs?
>
> With the current code, if I replace the Box with an on-stack variable,
> the kernel crashes.
>
> In my usecase, the pointer to the cpumask array is sent to the OPP
> core, which may update the content, though it doesn't save the pointer.
>
> But for another usecase, the C code may end up saving the pointer.

I don't think that case applies here. Variables that could be stashed
like that need a destructor that removes the pointer from the relevant
C structures. This is because pinning only prevents the owner from
moving the variable - it doesn't prevent the owner from destroying it.

Alice

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

[Alice Ryhl]

On Tue, Feb 25, 2025 at 6:12 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Tue, Feb 25, 2025 at 6:02 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On Tue, 25 Feb 2025 at 17:23, Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > > Is it a problem if a value of type struct cpumask is moved? It looks
> > > like it is just an array of longs?
> >
> > With the current code, if I replace the Box with an on-stack variable,
> > the kernel crashes.
> >
> > In my usecase, the pointer to the cpumask array is sent to the OPP
> > core, which may update the content, though it doesn't save the pointer.
> >
> > But for another usecase, the C code may end up saving the pointer.
>
> I don't think that case applies here. Variables that could be stashed
> like that need a destructor that removes the pointer from the relevant
> C structures. This is because pinning only prevents the owner from
> moving the variable - it doesn't prevent the owner from destroying it.

I believe you can implement the offstack stuff like this. The idea is to have
one type that corresponds to `struct cpumask` and another type that corresponds
to `cpumask_var_t` on the C side. Only the latter type would need to do
anything wrt. the CPUMASK_OFFSTACK setting.


diff --git a/rust/kernel/cpumask.rs b/rust/kernel/cpumask.rs
index 13864424420b..f82dafbc4e61 100644
--- a/rust/kernel/cpumask.rs
+++ b/rust/kernel/cpumask.rs
@@ -4,111 +4,45 @@
 //!
 //! C header: [`include/linux/cpumask.h`](srctree/include/linux/cpumask.h)
 
-use crate::{bindings, error::Result, prelude::ENOMEM};
-
-#[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
-use crate::prelude::{KBox, GFP_KERNEL};
-
+use crate::{
+    alloc::{AllocError, Flags},
+    bindings,
+    prelude::*,
+    types::Opaque,
+};
 #[cfg(CONFIG_CPUMASK_OFFSTACK)]
-use core::ptr;
+use core::ptr::{self, NonNull};
 
-/// A simple implementation of `struct cpumask` from the C code.
+/// This corresponds to the C type `struct cpumask`.
+#[repr(transparent)]
 pub struct Cpumask {
-    ptr: *mut bindings::cpumask,
-    owned: bool,
+    mask: Opaque<bindings::cpumask>,
 }
 
 impl Cpumask {
-    /// Creates cpumask.
-    #[cfg(CONFIG_CPUMASK_OFFSTACK)]
-    fn new_inner(empty: bool) -> Result<Self> {
-        let mut ptr: *mut bindings::cpumask = ptr::null_mut();
-
-        // SAFETY: Depending on the value of `gfp_flags`, this call may sleep. Other than that, it
-        // is always safe to call this method.
-        if !unsafe {
-            if empty {
-                bindings::zalloc_cpumask_var(&mut ptr, bindings::GFP_KERNEL)
-            } else {
-                bindings::alloc_cpumask_var(&mut ptr, bindings::GFP_KERNEL)
-            }
-        } {
-            return Err(ENOMEM);
-        }
-
-        Ok(Self { ptr, owned: true })
-    }
-
-    /// Creates cpumask.
-    #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
-    fn new_inner(empty: bool) -> Result<Self> {
-        let ptr = KBox::into_raw(KBox::new([bindings::cpumask::default(); 1], GFP_KERNEL)?);
-
-        // SAFETY: Depending on the value of `gfp_flags`, this call may sleep. Other than that, it
-        // is always safe to call this method.
-        if !unsafe {
-            if empty {
-                bindings::zalloc_cpumask_var(ptr, bindings::GFP_KERNEL)
-            } else {
-                bindings::alloc_cpumask_var(ptr, bindings::GFP_KERNEL)
-            }
-        } {
-            return Err(ENOMEM);
-        }
-
-        Ok(Self {
-            ptr: ptr as *mut _,
-            owned: true,
-        })
-    }
-
-    /// Creates empty cpumask.
-    pub fn new() -> Result<Self> {
-        Self::new_inner(true)
-    }
-
-    /// Creates uninitialized cpumask.
-    fn new_uninit() -> Result<Self> {
-        Self::new_inner(false)
-    }
-
-    /// Clones cpumask.
-    pub fn try_clone(&self) -> Result<Self> {
-        let mut cpumask = Self::new_uninit()?;
-
-        self.copy(&mut cpumask);
-        Ok(cpumask)
-    }
-
-    /// Creates a new abstraction instance of an existing `struct cpumask` pointer.
+    /// Creates a reference to an existing `struct cpumask` pointer.
     ///
     /// # Safety
     ///
-    /// Callers must ensure that `ptr` is valid, and non-null.
-    #[cfg(CONFIG_CPUMASK_OFFSTACK)]
-    pub unsafe fn get_cpumask(ptr: &mut *mut bindings::cpumask) -> Self {
-        Self {
-            ptr: *ptr,
-            owned: false,
-        }
+    /// 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 {
+        unsafe { &mut *ptr.cast() }
     }
 
-    /// Creates a new abstraction instance of an existing `struct cpumask` pointer.
+    /// Creates a reference to an existing `struct cpumask` pointer.
     ///
     /// # Safety
     ///
-    /// Callers must ensure that `ptr` is valid, and non-null.
-    #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
-    pub unsafe fn get_cpumask(ptr: &mut bindings::cpumask_var_t) -> Self {
-        Self {
-            ptr: ptr as *mut _,
-            owned: false,
-        }
+    /// 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 {
+        unsafe { &*ptr.cast() }
     }
 
     /// Obtain the raw `struct cpumask *`.
-    pub fn as_raw(&mut self) -> *mut bindings::cpumask {
-        self.ptr
+    pub fn as_raw(&self) -> *mut bindings::cpumask {
+        self as *const Cpumask as *mut bindings::cpumask
     }
 
     /// Sets CPU in the cpumask.
@@ -117,7 +51,7 @@ pub fn as_raw(&mut self) -> *mut bindings::cpumask {
     pub fn set(&mut self, cpu: u32) {
         // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
         // call `cpumask_set_cpus()` for any CPU.
-        unsafe { bindings::cpumask_set_cpu(cpu, self.ptr) };
+        unsafe { bindings::cpumask_set_cpu(cpu, self.as_raw()) };
     }
 
     /// Clears CPU in the cpumask.
@@ -126,43 +60,74 @@ pub fn set(&mut self, cpu: u32) {
     pub fn clear(&mut self, cpu: i32) {
         // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
         // call `cpumask_clear_cpu()` for any CPU.
-        unsafe { bindings::cpumask_clear_cpu(cpu, self.ptr) };
+        unsafe { bindings::cpumask_clear_cpu(cpu, self.as_raw()) };
     }
 
     /// Sets all CPUs in the cpumask.
     pub fn set_all(&mut self) {
         // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
         // call `cpumask_setall()`.
-        unsafe { bindings::cpumask_setall(self.ptr) };
+        unsafe { bindings::cpumask_setall(self.as_raw()) };
     }
 
     /// Gets weight of a cpumask.
     pub fn weight(&self) -> u32 {
         // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
         // call `cpumask_weight()`.
-        unsafe { bindings::cpumask_weight(self.ptr) }
+        unsafe { bindings::cpumask_weight(self.as_raw()) }
     }
 
     /// Copies cpumask.
     pub fn copy(&self, dstp: &mut Self) {
         // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
         // call `cpumask_copy()`.
-        unsafe { bindings::cpumask_copy(dstp.as_raw(), self.ptr) };
+        unsafe { bindings::cpumask_copy(dstp.as_raw(), self.as_raw()) };
     }
 }
 
-impl Drop for Cpumask {
-    fn drop(&mut self) {
-        if self.owned {
-            // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe
-            // to call `free_cpumask_var()`.
-            unsafe { bindings::free_cpumask_var(self.ptr) }
+/// This corresponds to the C type alias `cpumask_var_t`.
+pub struct CpumaskBox {
+    #[cfg(CONFIG_CPUMASK_OFFSTACK)]
+    ptr: NonNull<Cpumask>,
+    #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+    mask: Cpumask,
+}
 
+impl 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();
+                unsafe { bindings::zalloc_cpumask_var(&mut ptr, _flags.as_raw()) };
+                NonNull::new(ptr.cast()).ok_or(AllocError)?
+            },
             #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
-            // SAFETY: The pointer was earlier initialized from the result of `KBox::into_raw()`.
-            unsafe {
-                drop(KBox::from_raw(self.ptr))
-            };
-        }
+            mask: unsafe { core::mem::zeroed() },
+        })
+    }
+}
+
+// Make CpumaskBox behave like a pointer to Cpumask.
+impl core::ops::Deref for CpumaskBox {
+    type Target = Cpumask;
+
+    #[cfg(CONFIG_CPUMASK_OFFSTACK)]
+    fn deref(&self) -> &Cpumask {
+        unsafe { &*self.ptr.as_ptr() }
+    }
+
+    #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+    fn deref(&self) -> &Cpumask {
+        &self.mask
+    }
+}
+
+impl Drop for CpumaskBox {
+    fn drop(&mut self) {
+        #[cfg(CONFIG_CPUMASK_OFFSTACK)]
+        unsafe {
+            bindings::free_cpumask_var(self.as_raw())
+        };
     }
 }

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

[Viresh Kumar]

On 25-02-25, 18:02, Alice Ryhl wrote:
> diff --git a/rust/kernel/cpumask.rs b/rust/kernel/cpumask.rs
> +/// This corresponds to the C type alias `cpumask_var_t`.
> +pub struct CpumaskBox {
> +    #[cfg(CONFIG_CPUMASK_OFFSTACK)]
> +    ptr: NonNull<Cpumask>,
> +    #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
> +    mask: Cpumask,
> +}
>  
> +impl 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();
> +                unsafe { bindings::zalloc_cpumask_var(&mut ptr, _flags.as_raw()) };
> +                NonNull::new(ptr.cast()).ok_or(AllocError)?
> +            },
>              #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
> -            // SAFETY: The pointer was earlier initialized from the result of `KBox::into_raw()`.
> -            unsafe {
> -                drop(KBox::from_raw(self.ptr))
> -            };
> -        }
> +            mask: unsafe { core::mem::zeroed() },

This will work correctly, yes, but I wasn't sure if the Rust code
should do this or depend on the C API and call zalloc_cpumask_var().
The implementation of struct cpumask allows this to work right now,
but any change where some internal state management is done, for
example, within the structure may break the Rust side.

I am okay with this way of doing it if that looks okay to you.

I have made following changes over your diff.

Thanks for your help in simplifying the code.

-- 
viresh

-------------------------8<-------------------------

diff --git a/rust/kernel/cpumask.rs b/rust/kernel/cpumask.rs
index f82dafbc4e61..d56f7dc9d762 100644
--- a/rust/kernel/cpumask.rs
+++ b/rust/kernel/cpumask.rs
@@ -13,20 +13,28 @@
 #[cfg(CONFIG_CPUMASK_OFFSTACK)]
 use core::ptr::{self, NonNull};
 
+#[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+use core::mem::MaybeUninit;
+
+use core::ops::{Deref, DerefMut};
+
 /// This corresponds to the C type `struct cpumask`.
 #[repr(transparent)]
-pub struct Cpumask {
-    mask: Opaque<bindings::cpumask>,
-}
+pub struct Cpumask(Opaque<bindings::cpumask>);
 
 impl Cpumask {
-    /// Creates a reference to an existing `struct cpumask` pointer.
+    /// 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 {
+    pub unsafe fn from_raw_mut<'a>(ptr: *mut bindings::cpumask_var_t) -> &'a mut Self {
+        // For this configuration, `cpumask_var_t` is defined as `[cpumask; 1]`.
+        #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+        let ptr: *mut bindings::cpumask = ptr as *mut _;
+
+        // SAFETY: Guaranteed by the safety requirements of the function.
         unsafe { &mut *ptr.cast() }
     }
 
@@ -36,7 +44,12 @@ pub unsafe fn from_raw_mut<'a>(ptr: *mut bindings::cpumask) -> &'a mut Self {
     ///
     /// 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 {
+    pub unsafe fn from_raw<'a>(ptr: *const bindings::cpumask_var_t) -> &'a Self {
+        // For this configuration, `cpumask_var_t` is defined as `[cpumask; 1]`.
+        #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+        let ptr: *const bindings::cpumask = ptr as *const _;
+
+        // SAFETY: Guaranteed by the safety requirements of the function.
         unsafe { &*ptr.cast() }
     }
 
@@ -45,41 +58,37 @@ pub fn as_raw(&self) -> *mut bindings::cpumask {
         self as *const Cpumask as *mut bindings::cpumask
     }
 
-    /// Sets CPU in the cpumask.
-    ///
-    /// Update the cpumask with a single CPU.
+    /// Sets `cpu` in the cpumask.
     pub fn set(&mut self, cpu: u32) {
-        // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
+        // SAFETY: `mask` is guaranteed to be valid for the lifetime of `self`. And it is safe to
         // call `cpumask_set_cpus()` for any CPU.
         unsafe { bindings::cpumask_set_cpu(cpu, self.as_raw()) };
     }
 
-    /// Clears CPU in the cpumask.
-    ///
-    /// Update the cpumask with a single CPU.
+    /// Clears `cpu` in the cpumask.
     pub fn clear(&mut self, cpu: i32) {
-        // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
+        // SAFETY: `mask` is guaranteed to be valid for the lifetime of `self`. And it is safe to
         // call `cpumask_clear_cpu()` for any CPU.
         unsafe { bindings::cpumask_clear_cpu(cpu, self.as_raw()) };
     }
 
     /// Sets all CPUs in the cpumask.
     pub fn set_all(&mut self) {
-        // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
+        // SAFETY: `mask` is guaranteed to be valid for the lifetime of `self`. And it is safe to
         // call `cpumask_setall()`.
         unsafe { bindings::cpumask_setall(self.as_raw()) };
     }
 
-    /// Gets weight of a cpumask.
+    /// Gets weight of the cpumask.
     pub fn weight(&self) -> u32 {
-        // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
+        // SAFETY: `mask` is guaranteed to be valid for the lifetime of `self`. And it is safe to
         // call `cpumask_weight()`.
         unsafe { bindings::cpumask_weight(self.as_raw()) }
     }
 
     /// Copies cpumask.
     pub fn copy(&self, dstp: &mut Self) {
-        // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
+        // SAFETY: `mask` is guaranteed to be valid for the lifetime of `self`. And it is safe to
         // call `cpumask_copy()`.
         unsafe { bindings::cpumask_copy(dstp.as_raw(), self.as_raw()) };
     }
@@ -94,26 +103,65 @@ pub struct CpumaskBox {
 }
 
 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.
                 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.
             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.
+                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.
+            mask: unsafe { MaybeUninit::uninit().assume_init() },
+        })
+    }
+
+    /// 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 core::ops::Deref for CpumaskBox {
+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() }
     }
 
@@ -123,9 +171,23 @@ fn deref(&self) -> &Cpumask {
     }
 }
 
+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: It is safe to free the cpumask.
         unsafe {
             bindings::free_cpumask_var(self.as_raw())
         };

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

[Alice Ryhl]

On Thu, Feb 27, 2025 at 11:46 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 25-02-25, 18:02, Alice Ryhl wrote:
> > diff --git a/rust/kernel/cpumask.rs b/rust/kernel/cpumask.rs
> > +/// This corresponds to the C type alias `cpumask_var_t`.
> > +pub struct CpumaskBox {
> > +    #[cfg(CONFIG_CPUMASK_OFFSTACK)]
> > +    ptr: NonNull<Cpumask>,
> > +    #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
> > +    mask: Cpumask,
> > +}
> >
> > +impl 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();
> > +                unsafe { bindings::zalloc_cpumask_var(&mut ptr, _flags.as_raw()) };
> > +                NonNull::new(ptr.cast()).ok_or(AllocError)?
> > +            },
> >              #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
> > -            // SAFETY: The pointer was earlier initialized from the result of `KBox::into_raw()`.
> > -            unsafe {
> > -                drop(KBox::from_raw(self.ptr))
> > -            };
> > -        }
> > +            mask: unsafe { core::mem::zeroed() },
>
> This will work correctly, yes, but I wasn't sure if the Rust code
> should do this or depend on the C API and call zalloc_cpumask_var().
> The implementation of struct cpumask allows this to work right now,
> but any change where some internal state management is done, for
> example, within the structure may break the Rust side.
>
> I am okay with this way of doing it if that looks okay to you.
>
> I have made following changes over your diff.
>
> Thanks for your help in simplifying the code.
>
> --
> viresh
>
> -------------------------8<-------------------------
>
> diff --git a/rust/kernel/cpumask.rs b/rust/kernel/cpumask.rs
> index f82dafbc4e61..d56f7dc9d762 100644
> --- a/rust/kernel/cpumask.rs
> +++ b/rust/kernel/cpumask.rs
> @@ -13,20 +13,28 @@
>  #[cfg(CONFIG_CPUMASK_OFFSTACK)]
>  use core::ptr::{self, NonNull};
>
> +#[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
> +use core::mem::MaybeUninit;
> +
> +use core::ops::{Deref, DerefMut};
> +
>  /// This corresponds to the C type `struct cpumask`.
>  #[repr(transparent)]
> -pub struct Cpumask {
> -    mask: Opaque<bindings::cpumask>,
> -}
> +pub struct Cpumask(Opaque<bindings::cpumask>);
>
>  impl Cpumask {
> -    /// Creates a reference to an existing `struct cpumask` pointer.
> +    /// 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 {
> +    pub unsafe fn from_raw_mut<'a>(ptr: *mut bindings::cpumask_var_t) -> &'a mut Self {

Why?

Perhaps put this on the CpumaskBox and keep the `struct cpumask` one here?

Alice

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

[Viresh Kumar]

On 27-02-25, 13:19, Alice Ryhl wrote:
> On Thu, Feb 27, 2025 at 11:46 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >  impl Cpumask {
> > -    /// Creates a reference to an existing `struct cpumask` pointer.
> > +    /// 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 {
> > +    pub unsafe fn from_raw_mut<'a>(ptr: *mut bindings::cpumask_var_t) -> &'a mut Self {
> 
> Why?

cpufreq core has a pointer to cpumask_var_t.

> Perhaps put this on the CpumaskBox and keep the `struct cpumask` one here?

Yeah, that worked. I think we will anyway have both type of users and
its better to have both implemented.

-- 
viresh

[PATCH 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>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index ec2428b82103..17e98d757b9b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4034,6 +4034,12 @@ M:	Yury Norov <yury.norov@gmail.com>
 S:	Maintained
 F:	rust/helpers/cpumask.c
 
+BITMAP API [RUST]
+M:	Viresh Kumar <viresh.kumar@linaro.org> (cpumask)
+R:	Yury Norov <yury.norov@gmail.com>
+S:	Maintained
+F:	rust/kernel/cpumask.rs
+
 BITOPS API
 M:	Yury Norov <yury.norov@gmail.com>
 R:	Rasmus Villemoes <linux@rasmusvillemoes.dk>
-- 
2.31.1.272.g89b43f80a514

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

[Yury Norov]

On Tue, Feb 25, 2025 at 03:17:15PM +0530, Viresh Kumar 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.
> 
> 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 ec2428b82103..17e98d757b9b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4034,6 +4034,12 @@ M:	Yury Norov <yury.norov@gmail.com>
>  S:	Maintained
>  F:	rust/helpers/cpumask.c
>  
> +BITMAP API [RUST]
> +M:	Viresh Kumar <viresh.kumar@linaro.org> (cpumask)
> +R:	Yury Norov <yury.norov@gmail.com>
> +S:	Maintained
> +F:	rust/kernel/cpumask.rs
> +
>  BITOPS API
>  M:	Yury Norov <yury.norov@gmail.com>
>  R:	Rasmus Villemoes <linux@rasmusvillemoes.dk>
> -- 
> 2.31.1.272.g89b43f80a514