Message-ID: <aHaCUFNUd_mErL7S@Mac.home>
Date: Tue, 15 Jul 2025 09:31:12 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Benno Lossin <lossin@...nel.org>
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 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)

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

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

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

> 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

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.

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

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

Regards,
Boqun

> 
> ---
> Cheers,
> Benno

Powered by blists - more mailing lists