Message-ID: <Z-1b_FkYUJEIj-YW@thinkpad>
Date: Wed, 2 Apr 2025 11:47:08 -0400
From: Yury Norov <yury.norov@...il.com>
To: Viresh Kumar <viresh.kumar@...aro.org>
Cc: Rasmus Villemoes <linux@...musvillemoes.dk>,
	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>,
	linux-kernel@...r.kernel.org, Danilo Krummrich <dakr@...hat.com>,
	rust-for-linux@...r.kernel.org,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Burak Emir <bqe@...gle.com>
Subject: Re: [PATCH V4 1/2] rust: Add initial cpumask abstractions

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@...aro.org>
> X-Mailer: git-send-email 2.31.1.272.g89b43f80a514
> In-Reply-To: <cover.1743572195.git.viresh.kumar@...aro.org>
> References: <cover.1743572195.git.viresh.kumar@...aro.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@...aro.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

Powered by blists - more mailing lists