Message-Id: <DBCTCZ5HUZOF.2DJX63Q0VWWFN@kernel.org>
Date: Tue, 15 Jul 2025 19:44:01 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Boqun Feng" <boqun.feng@...il.com>
Cc: "Mitchell Levy" <levymitchell0@...il.com>, "Miguel Ojeda"
 <ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...il.com>, "Gary Guo"
 <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Andreas Hindborg" <a.hindborg@...nel.org>,
 "Alice Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>,
 "Andrew Morton" <akpm@...ux-foundation.org>, "Dennis Zhou"
 <dennis@...nel.org>, "Tejun Heo" <tj@...nel.org>, "Christoph Lameter"
 <cl@...ux.com>, "Danilo Krummrich" <dakr@...nel.org>,
 <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>,
 <linux-mm@...ck.org>
Subject: Re: [PATCH v2 3/5] rust: percpu: add a rust per-CPU variable test

On Tue Jul 15, 2025 at 6:31 PM CEST, Boqun Feng wrote:
> On Tue, Jul 15, 2025 at 05:55:13PM +0200, Benno Lossin wrote:
>> On Tue Jul 15, 2025 at 4:10 PM CEST, Boqun Feng wrote:
>> > On Tue, Jul 15, 2025 at 01:31:06PM +0200, Benno Lossin wrote:
>> > [...]
>> >> >> > +impl kernel::Module for PerCpuTestModule {
>> >> >> > +    fn init(_module: &'static ThisModule) -> Result<Self, Error> {
>> >> >> > +        pr_info!("rust percpu test start\n");
>> >> >> > +
>> >> >> > +        let mut native: i64 = 0;
>> >> >> > +        // SAFETY: PERCPU is properly defined
>> >> >> > +        let mut pcpu: StaticPerCpu<i64> = unsafe { unsafe_get_per_cpu!(PERCPU) };
>> >> >> 
>> >> >> I don't understand why we need unsafe here, can't we just create
>> >> >> something specially in the `define_per_cpu` macro that is then confirmed
>> >> >> by the `get_per_cpu!` macro and thus it can be safe?
>> >> >
>> >> > As is, something like
>> >> >     define_per_cpu!(PERCPU: i32 = 0);
>> >> >
>> >> >     fn func() {
>> >> >         let mut pcpu: StaticPerCpu<i64> = unsafe { unsafe_get_per_cpu!(PERCPU) };
>> >> >     }
>> >> > will compile, but any usage of `pcpu` will be UB. This is because
>> >> > `unsafe_get_per_cpu!` is just blindly casting pointers and, as far as I
>> >> > know, the compiler does not do any checking of pointer casts. If you
>> >> > have thoughts/ideas on how to get around this problem, I'd certainly
>> >> > *like* to provide a safe API here :)
>> >> 
>> >> I haven't taken a look at your implementation, but you do have the type
>> >> declared in `define_per_cpu!`, so it's a bit of a mystery to me why you
>> >> can't get that out in `unsafe_get_per_cpu!`...
>> >> 
>> >> Maybe in a few weeks I'll be able to take a closer look.
>> >> 
>> >> >> > +        // SAFETY: We only have one PerCpu that points at PERCPU
>> >> >> > +        unsafe { pcpu.get(CpuGuard::new()) }.with(|val: &mut i64| {
>> >> >> 
>> >> >> Hmm I also don't like the unsafe part here...
>> >> >> 
>> >> >> Can't we use the same API that `thread_local!` in the standard library
>> >
>> > 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)`.

>> 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 :)

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

> If we would like to model it conceptually, it's more like an array
> that's index by CpuId to me.

Gotcha, but this model is missing the access control/synchronization. So
I'm not so sure how useful it is.

(I think I asked this somewhere else, but the number of CPUs doesn't
change, right?)

>> to detect context switches.
>> 
>> >> >> has:
>> >> >> 
>> >> >>     https://doc.rust-lang.org/std/macro.thread_local.html
>> >> >> 
>> >> >> So in this example you would store a `Cell<i64>` instead.
>> >> >> 
>> >> >> I'm not familiar with per CPU variables, but if you're usually storing
>> >> >> `Copy` types, then this is much better wrt not having unsafe code
>> >> >> everywhere.
>> >> >> 
>> >> >> If one also often stores `!Copy` types, then we might be able to get
>> >> >> away with `RefCell`, but that's a small runtime overhead -- which is
>> >> >> probably bad given that per cpu variables are most likely used for
>> >> >> performance reasons? In that case the user might just need to store
>> >> >> `UnsafeCell` and use unsafe regardless. (or we invent something
>> >
>> > This sounds reasonable to me.
>> >
>> >> >> specifically for that case, eg tokens that are statically known to be
>> >> >> unique etc)
>> >> >
>> >> > I'm open to including a specialization for `T: Copy` in a similar vein
>> >> > to what I have here for numeric types. Off the top of my head, that
>> >> > shouldn't require any user-facing `unsafe`. But yes, I believe there is
>> >> > a significant amount of interest in having `!Copy` per-CPU variables.
>> >> > (At least, I'm interested in having them around for experimenting with
>> >> > using Rust for HV drivers.)
>> >> 
>> >> What kinds of types would you like to store? Allocations? Just integers
>> >> in bigger structs? Mutexes?
>> >> 
>> >
>> > In the VMBus driver, there is a percpu work_struct.
>> 
>> Do you have a link? Or better yet a Rust struct description of what you
>> think it will look like :)
>> 
>
> Not Rust code yet, but here is the corresponding C code:
>
> 	https://github.com/Rust-for-Linux/linux/blob/rust-next/drivers/hv/vmbus_drv.c#L1396

Thanks!

> But please note that we are not solely developing the abstraction for
> this usage, but more for generally understand how to wrap percpu
> functionality similar to the usage in C.

Well, I have to start somewhere for looking at the use-cases :)

If you have more, just let me see. (probably won't have enough time to
look at them now, but maybe in a couple weeks)

>> >> > I would definitely like to avoid *requiring* the use of `RefCell` since,
>> >> > as you mention, it does have a runtime overhead. Per-CPU variables can
>> >> > be used for "logical" reasons rather than just as a performance
>> >> > optimization, so there might be some cases where paying the runtime
>> >> > overhead is ok. But that's certainly not true in all cases. That said,
>> >> > perhaps there could be a safely obtainable token type that only passes a
>> >> > `&T` (rather than a `&mut T`) to its closure, and then if a user doesn't
>> >> > mind the runtime overhead, they can choose `T` to be a `RefCell`.
>> >> > Thoughts?
>> >> 
>> >> So I think using an API similar to `thread_local!` will allow us to have
>> >> multiple other APIs that slot into that. `Cell<T>` for `T: Copy`,
>> >> `RefCell<T>` for cases where you don't care about the runtime overhead,
>> >> plain `T` for cases where you only need `&T`. For the case where you
>> >> need `&mut T`, we could have something like a `TokenCell<T>` that gives
>> >> out a token that you need to mutably borrow in order to get `&mut T`.
>> >> Finally for anything else that is too restricted by this, users can also
>> >> use `UnsafeCell<T>` although that requires `unsafe`.
>> >> 
>> >> I think the advantage of this is that the common cases are all safe and
>> >> very idiomatic. In the current design, you *always* have to use unsafe.
>> >> 
>> >
>> > I agree, but like I said, we need to figure out the unsafe interface
>> > that C already uses and build API upon it. I think focusing on the
>> > unsafe mechanism may be the way to start: you cannot implement something
>> > that cannot be implemented, and we don't have the magic pthread_key here
>> > ;-)
>> 
>> Sure we can do some experimentation, but I don't think we should put
>> unsafe abstractions upstream that we intend to replace with a safe
>> abstraction later. Otherwise people are going to depend on it and it's
>
> I doubt we can replace the unsafe abstraction with a safe one, if users
> really care the performance then they would really need to use some
> unsafe API to build their safe abstraction.

That sounds pretty pessimistic, why do you think that?

>> going to be a mess. Do the experimenting out of tree and learn there.
>
> I disagree, Rust as a language its own should be able to do what C does
> including being able to implement the percpu functionality same as C,
> there is nothing wrong with a set of Rust primitives in the kernel that
> provides fundamental percpu functionality the other core facilities can
> rely on. The better part is that it will have all the safety requirement
> documented well.

Sure, but we haven't even tried to make it safe, so I don't think we
should add them now in this state.

---
Cheers,
Benno

Powered by blists - more mailing lists