Re: [PATCH v2 3/5] rust: percpu: add a rust per-CPU variable test

["Benno Lossin" <lossin@kernel.org>]

On Wed Jul 16, 2025 at 7:52 PM CEST, Boqun Feng wrote:
> On Wed, Jul 16, 2025 at 07:21:32PM +0200, Benno Lossin wrote:
>> On Wed Jul 16, 2025 at 5:33 PM CEST, Boqun Feng wrote:
>> > On Wed, Jul 16, 2025 at 12:32:04PM +0200, Benno Lossin wrote:
>> >> On Tue Jul 15, 2025 at 11:34 PM CEST, Boqun Feng wrote:
>> >> > On Tue, Jul 15, 2025 at 07:44:01PM +0200, Benno Lossin wrote:
>> >> > [...]
>> >> >> >> >
>> >> >> >> > First of all, `thread_local!` has to be implemented by some sys-specific
>> >> >> >> > unsafe mechanism, right? For example on unix, I think it's using
>> >> >> >> > pthread_key_t:
>> >> >> >> >
>> >> >> >> > 	https://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_key_create.html
>> >> >> >> >
>> >> >> >> > what we are implementing (or wrapping) is the very basic unsafe
>> >> >> >> > mechanism for percpu here. Surely we can explore the design for a safe
>> >> >> >> > API, but the unsafe mechanism is probably necessary to look into at
>> >> >> >> > first.
>> >> >> >> 
>> >> >> >> But this is intended to be used by drivers, right? If so, then we should
>> >> >> >
>> >> >> > Not necessarily only for drivers, we can also use it for implementing
>> >> >> > other safe abstraction (e.g. hazard pointers, percpu counters etc)
>> >> >> 
>> >> >> That's fair, but then it should be `pub(crate)`.
>> >> >> 
>> >> >
>> >> > Fine by me, but please see below.
>> >> >
>> >> >> >> do our usual due diligence and work out a safe abstraction. Only fall
>> >> >> >> back to unsafe if it isn't possible.
>> >> >> >> 
>> >> >> >
>> >> >> > All I'm saying is instead of figuring out a safe abstraction at first,
>> >> >> > we should probably focus on identifying how to implement it and which
>> >> >> > part is really unsafe and the safety requirement for that.
>> >> >> 
>> >> >> Yeah. But then we should do that before merging :)
>> >> >> 
>> >> >
>> >> > Well, who's talknig about merging? ;-) I thought we just began reviewing
>> >> > here ;-)
>> >> 
>> >> I understand [PATCH] emails as "I want to merge this" and [RFC PATCH] as
>> >
>> > But it doesn't mean "merge as it is", right? I don't think either I or
>> > Mitchell implied that, I'm surprised that you had to mention that,
>> 
>> Yeah that is true, but it at least shows the intention :)
>> 
>> > also based on "I often mute those" below, making it "[PATCH]" seems to
>> > be a practical way to get more attention if one wants to get some
>> > reviews.
>> 
>> That is true, I do usually read the titles of RFC patches though and
>> sometimes take a look eg your atomics series.
>> 
>> >> "I want to talk about merging this". It might be that I haven't seen the
>> >> RFC patch series, because I often mute those.
>> >> 
>> >
>> > Well, then you cannot blame people to move from "RFC PATCH" to "PATCH"
>> > stage for more reviews, right? And you cannot make rules about what the
>> > difference between [PATCH] and [RFC PATCH] if you ignore one of them ;-)
>> 
>> I'm not trying to blame anyone. I saw a lot of unsafe in the example and
>> thought "we can do better" and since I haven't heard any sufficient
>> arguments showing that it's impossible to improve, we should do some
>> design work.
>> 
>
> I agree with you, and I like what you're proposing, but I think design
> work can be done at "PATCH" stage, right? And sometimes, it's also OK to
> do some design work even at some version like "v12" ;-)

Yeah of course. The thing is just that nobody asked why there was unsafe
and thus I got the impression that people thought this would be a good
abstraction for percpu. (don't take from this that it's bad :)

> Also I want to see more forward-progress actions about the design work
> improvement. For example, we can examine every case that makes
> unsafe_get_per_cpu!() unsafe, and see if we can improve that by typing
> or something else. We always can "do better", but the important part is
> how to get there ;-)

Yeah that would be a starting point :)

>> >> >> >> I'm not familiar with percpu, but from the name I assumed that it's
>> >> >> >> "just a variable for each cpu" so similar to `thread_local!`, but it's
>> >> >> >> bound to the specific cpu instead of the thread.
>> >> >> >> 
>> >> >> >> That in my mind should be rather easy to support in Rust at least with
>> >> >> >> the thread_local-style API. You just need to ensure that no reference
>> >> >> >> can escape the cpu, so we can make it `!Send` & `!Sync` + rely on klint
>> >> >> >
>> >> >> > Not really, in kernel, we have plenty of use cases that we read the
>> >> >> > other CPU's percpu variables. For example, each CPU keeps it's own
>> >> >> > counter and we sum them other in another CPU.
>> >> >> 
>> >> >> But then you need some sort of synchronization?
>> >> >> 
>> >> >
>> >> > Right, but the synchronization can exist either in the percpu operations
>> >> > themselves or outside the percpu operations. Some cases, the data types
>> >> > are small enough to fit in atomic data types, and operations are just
>> >> > load/store/cmpxchg etc, then operations on the current cpu and remote
>> >> > read will be naturally synchronized. Sometimes extra synchronization is
>> >> > needed.
>> >> 
>> >> Sure, so we probably want direct atomics support. What about "extra
>> >> synchronization"? Is that using locks or RCU or what else?
>> >> 
>> >
>> > It's up to the users obviously, It could be some sort of locking or RCU,
>> > it's case by case.
>> 
>> Makes sense, what do you need in the VMS driver?
>> 
>
> In VMBus driver, it's actually isolate, i.e. each CPU only access it's
> own work_struct, so synchronization between CPUs is not needed.

I see, so we could either just start out with no sync support or --
which I would prefer -- get a list of the most common use-cases and
implement those too (or at least design the first part compatibly with
further extensions).

---
Cheers,
Benno