Re: [PATCH V4 1/2] rust: Add initial cpumask abstractions

[Viresh Kumar]

On 02-04-25, 11:47, Yury Norov wrote:
> On Wed, Apr 02, 2025 at 11:08:42AM +0530, Viresh Kumar wrote:
> > +    /// 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.

I don't think I need it to be atomic:

> Can you link the project that actually uses this API?

https://lore.kernel.org/all/3054a0eb12330914cd6165ad460d9844ee8c19e2.1738832119.git.viresh.kumar@xxxxxxxxxx/
(search for "mask.set" here).

> Better if you just prepend that series with this 2 patches
> and move them together.

That's why I did earlier, but now separated them. I can provide links
in both the series to each other to make this easier to look though.

I can also prepend that series with these patches once they are fully
reviewed.

> > +    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.

Sure.

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

'Box' means heap allocated normally but CpumaskVar sounds better.

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

Okay, I will import and use cpumask_test_cpu(), use that for testing
the output after every operation and do weight check once at the last
to make sure all CPUs are set in the mask.

> 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. 

That is a lot of patches adding bindings for OPP/cpufreq frameworks
and a driver. Not sue if I should be mixing them again with this
series. I will provide links though.

> 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.

I was expecting to get this merged separately, so I don't need to push
a big chunk together (where reviews eventually take lot more time).

But I am okay to merge it all once and just collect Acks until the
time the client changes are fully reviewed.

-- 
viresh