Message-ID: <CACQBu=WMu_CmzERJAHmjiuQ-NZe6DK2kAkvP0cnFN-Y+QhRwvg@mail.gmail.com>
Date: Wed, 2 Apr 2025 18:00:26 +0200
From: Burak Emir <bqe@...gle.com>
To: Yury Norov <yury.norov@...il.com>
Cc: Viresh Kumar <viresh.kumar@...aro.org>, 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>
Subject: Re: [PATCH V4 1/2] rust: Add initial cpumask abstractions

On Wed, Apr 2, 2025 at 5:47 PM Yury Norov <yury.norov@...il.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@...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.

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

Powered by blists - more mailing lists