Message-ID: <Z_k7HtIZaSWeJvM4@yury>
Date: Fri, 11 Apr 2025 11:54:06 -0400
From: Yury Norov <yury.norov@...il.com>
To: Viresh Kumar <viresh.kumar@...aro.org>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>,
	Miguel Ojeda <miguel.ojeda.sandonis@...il.com>,
	Danilo Krummrich <dakr@...hat.com>, Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Danilo Krummrich <dakr@...nel.org>, linux-pm@...r.kernel.org,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Stephen Boyd <sboyd@...nel.org>, Nishanth Menon <nm@...com>,
	rust-for-linux@...r.kernel.org,
	Manos Pitsidianakis <manos.pitsidianakis@...aro.org>,
	Erik Schilling <erik.schilling@...aro.org>,
	Alex Bennée <alex.bennee@...aro.org>,
	Joakim Bech <joakim.bech@...aro.org>, Rob Herring <robh@...nel.org>,
	Burak Emir <bqe@...gle.com>,
	Rasmus Villemoes <linux@...musvillemoes.dk>,
	Russell King <linux@...linux.org.uk>, linux-clk@...r.kernel.org,
	Michael Turquette <mturquette@...libre.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V9 03/17] rust: cpumask: Add initial abstractions

On Fri, Apr 11, 2025 at 04:25:02PM +0530, Viresh Kumar 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@...aro.org>
> ---
>  rust/kernel/cpumask.rs | 328 +++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs     |   1 +
>  2 files changed, 329 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..a9d22c1d7a5a
> --- /dev/null
> +++ b/rust/kernel/cpumask.rs
> @@ -0,0 +1,328 @@
> +// 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.
> +///
> +/// 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

This line is too long to me.

> +/// 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 _ as _
> +    }
> +
> +    /// 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_cpu`.
> +        unsafe { bindings::__cpumask_set_cpu(cpu, self.as_raw()) };
> +    }
> +
> +    /// Clear `cpu` in the cpumask.
> +    ///
> +    /// Equivalent to the kernel's `__cpumask_clear_cpu` API.

Similarly to bitmaps, can you explain here that this is a non-atomic
operation?

> +    #[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()) };
> +    }
> +
> +    /// Test `cpu` in the cpumask.
> +    ///
> +    /// Equivalent to the kernel's `cpumask_test_cpu` API.
> +    #[inline]
> +    pub fn test(&self, cpu: i32) -> bool {
> +        // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `cpumask_test_cpu`.
> +        unsafe { bindings::cpumask_test_cpu(cpu, self.as_raw()) }
> +    }
> +
> +    /// Set all CPUs in the cpumask.
> +    ///
> +    /// Equivalent to the kernel's `cpumask_setall` API.
> +    #[inline]
> +    pub fn setall(&mut self) {
> +        // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `cpumask_setall`.
> +        unsafe { bindings::cpumask_setall(self.as_raw()) };
> +    }
> +
> +    /// Checks if cpumask is empty.
> +    ///
> +    /// Equivalent to the kernel's `cpumask_empty` API.
> +    #[inline]
> +    pub fn empty(&self) -> bool {
> +        // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `cpumask_empty`.
> +        unsafe { bindings::cpumask_empty(self.as_raw()) }
> +    }
> +
> +    /// Checks if cpumask is full.
> +    ///
> +    /// Equivalent to the kernel's `cpumask_full` API.
> +    #[inline]
> +    pub fn full(&self) -> bool {
> +        // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `cpumask_full`.
> +        unsafe { bindings::cpumask_full(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.
> +///
> +/// Rust abstraction for the C `struct cpumask_var_t`.
> +///
> +/// # Invariants
> +///
> +/// A [`CpumaskVar`] 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 [`CpumaskVar`].
> +///
> +/// ## Examples
> +///
> +/// The following example demonstrates how to create and update a [`CpumaskVar`].
> +///
> +/// ```
> +/// use kernel::cpumask::CpumaskVar;
> +///
> +/// let mut mask = CpumaskVar::new(GFP_KERNEL).unwrap();
> +///
> +/// assert!(mask.empty());
> +/// mask.set(2);
> +/// assert!(mask.test(2));
> +/// mask.set(3);
> +/// assert!(mask.test(3));
> +/// assert_eq!(mask.weight(), 2);
> +///
> +/// let mask2 = CpumaskVar::try_clone(&mask).unwrap();
> +/// assert!(mask2.test(2));
> +/// assert!(mask2.test(3));
> +/// assert_eq!(mask2.weight(), 2);
> +/// ```
> +pub struct CpumaskVar {
> +    #[cfg(CONFIG_CPUMASK_OFFSTACK)]
> +    ptr: NonNull<Cpumask>,
> +    #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
> +    mask: Cpumask,
> +}
> +
> +impl CpumaskVar {
> +    /// Creates an initialized instance of the [`CpumaskVar`].
> +    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 `CpumaskVar` 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 `CpumaskVar` goes out of scope.
> +            mask: unsafe { core::mem::zeroed() },
> +        })
> +    }
> +
> +    /// Creates an uninitialized instance of the [`CpumaskVar`].

I would do this another way: introduce new() that calls
alloc_cpumask_var(), and new_zero() binded to zalloc() version. Your
statement here is simply wrong because I can pass GFP_ZERO and 'hack'
all your architecture.

> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that the returned [`CpumaskVar`] 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.

I'm not sure I understand this sentence. What's wrong with safety when
the alloc() function sleeps? Even if something is wrong. If you really
want to protect your users, you'd introduce new_sync() version that
returns error if user provides sleeping flags.

To that extend, once you write so many flavors of constructors, I bet
your users will be happy if you hide the 'flags' entirely:
        new_gfp(flags);
        new();
        new_zero(); // or znew()?
        new_sync();
        
> +                //
> +                // INVARIANT: The associated memory is freed when the `CpumaskVar` 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 `CpumaskVar` 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 {

The 'from' (wrt cpumasks) has a special meaning: search for a cpu
starting from a given one. This 'from_raw' may confuse readers. Have
you any other name for it in mind?

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

Just clone(), I think.

> +        // 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 [`CpumaskVar`] behave like a pointer to [`Cpumask`].
> +impl Deref for CpumaskVar {
> +    type Target = Cpumask;
> +
> +    #[cfg(CONFIG_CPUMASK_OFFSTACK)]
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: The caller owns CpumaskVar, so it is safe to deref the cpumask.
> +        unsafe { &*self.ptr.as_ptr() }
> +    }
> +
> +    #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
> +    fn deref(&self) -> &Self::Target {
> +        &self.mask
> +    }
> +}
> +
> +impl DerefMut for CpumaskVar {
> +    #[cfg(CONFIG_CPUMASK_OFFSTACK)]
> +    fn deref_mut(&mut self) -> &mut Cpumask {
> +        // SAFETY: The caller owns CpumaskVar, 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 CpumaskVar {
> +    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 de07aadd1ff5..75f78f6bfaa6 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -42,6 +42,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

Powered by blists - more mailing lists