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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250403104110.yz2d776fvxja3rj3@vireshk-i7>
Date: Thu, 3 Apr 2025 16:11:10 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: Yury Norov <yury.norov@...il.com>
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 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@linaro.org/
(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