Re: [PATCH v4 1/9] rust: cpumask: Add a `Cpumask` iterator

[Mitchell Levy <levymitchell0@gmail.com>]

On Thu, Nov 06, 2025 at 07:25:27PM -0500, Yury Norov wrote:
> On Wed, Nov 05, 2025 at 03:01:13PM -0800, Mitchell Levy wrote:
> > Add an iterator for `Cpumask` making use of C's `cpumask_next`.
> > 
> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > Signed-off-by: Mitchell Levy <levymitchell0@gmail.com>
> > ---
> >  rust/helpers/cpumask.c |  5 +++++
> >  rust/kernel/cpumask.rs | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 52 insertions(+), 1 deletion(-)
> > 
> > diff --git a/rust/helpers/cpumask.c b/rust/helpers/cpumask.c
> > index eb10598a0242..d95bfa111191 100644
> > --- a/rust/helpers/cpumask.c
> > +++ b/rust/helpers/cpumask.c
> > @@ -42,6 +42,11 @@ bool rust_helper_cpumask_full(struct cpumask *srcp)
> >  	return cpumask_full(srcp);
> >  }
> >  
> > +unsigned int rust_helper_cpumask_next(int n, struct cpumask *srcp)
> > +{
> > +	return cpumask_next(n, srcp);
> > +}
> > +
> >  unsigned int rust_helper_cpumask_weight(struct cpumask *srcp)
> >  {
> >  	return cpumask_weight(srcp);
> > diff --git a/rust/kernel/cpumask.rs b/rust/kernel/cpumask.rs
> > index 3fcbff438670..b7401848f59e 100644
> > --- a/rust/kernel/cpumask.rs
> > +++ b/rust/kernel/cpumask.rs
> > @@ -6,7 +6,7 @@
> >  
> >  use crate::{
> >      alloc::{AllocError, Flags},
> > -    cpu::CpuId,
> > +    cpu::{self, CpuId},
> >      prelude::*,
> >      types::Opaque,
> >  };
> > @@ -161,6 +161,52 @@ pub fn copy(&self, dstp: &mut Self) {
> >      }
> >  }
> >  
> > +/// Iterator for a `Cpumask`.
> > +pub struct CpumaskIter<'a> {
> > +    mask: &'a Cpumask,
> > +    last: Option<u32>,
> 
> This is not the last, it's a current CPU.

Ah, I meant it in the sense of "the last cpuid we've seen", though now
that you point it out I agree the naming here is poor. Will correct to
`current`.

> > +}
> > +
> > +impl<'a> CpumaskIter<'a> {
> > +    /// Creates a new `CpumaskIter` for the given `Cpumask`.
> > +    fn new(mask: &'a Cpumask) -> CpumaskIter<'a> {
> > +        Self { mask, last: None }
> > +    }
> > +}
> > +
> > +impl<'a> Iterator for CpumaskIter<'a> {
> > +    type Item = CpuId;
> > +
> > +    fn next(&mut self) -> Option<Self::Item> {
> > +        // SAFETY: By the type invariant, `self.mask.as_raw` is a `struct cpumask *`.
> > +        let next = unsafe {
> > +            bindings::cpumask_next(
> > +                if let Some(last) = self.last {
> > +                    last.try_into().unwrap()
> > +                } else {
> > +                    -1
> > +                },
> > +                self.mask.as_raw(),
> > +            )
> > +        };
> > +
> > +        if next == cpu::nr_cpu_ids() {
> > +            None
> 
> Please:    if next >= cpu::nr_cpu_ids() {
>
> > +        } else {
> > +            self.last = Some(next);
> > +            // SAFETY: `cpumask_next` returns either `nr_cpu_ids` or a valid CPU ID.
> 
> Now that you've handled the no-found case in the previous block, the
> comment doesn't look correct. Can you either move it on top of the
> if-else, or just drop entirely?

Actually, now that I'm looking at this again, I think this whole if-else
thing should just be:
```
CpuId::from_u32(next)
```
which does exactly what we want here. I think this should address both
of your concerns, though please let me know if it doesn't.

Thanks,
Mitchell

> > +            unsafe { Some(CpuId::from_u32_unchecked(next)) }
> > +        }
> > +    }
> > +}
> > +
> > +impl Cpumask {
> > +    /// Returns an iterator over the set bits in the cpumask.
> > +    pub fn iter(&self) -> CpumaskIter<'_> {
> > +        CpumaskIter::new(self)
> > +    }
> > +}
> > +
> >  /// A CPU Mask pointer.
> >  ///
> >  /// Rust abstraction for the C `struct cpumask_var_t`.
> > 
> > -- 
> > 2.34.1