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

[Yury Norov <yury.norov@gmail.com>]

On Fri, Nov 07, 2025 at 04:06:19PM -0800, Mitchell Levy wrote:
> 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.

Looks like that.