Message-ID: <aHZhcNCayTOQhvYh@Mac.home>
Date: Tue, 15 Jul 2025 07:10:56 -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 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.

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

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

Regards,
Boqun

> > For `UnsafeCell`, if a user of the API were to have something like a
> > `PerCpu<UnsafeCell<T>>` that safely spits out a `&UnsafeCell<T>`, my
> > understanding is that mutating the underlying `T` would require the
> > exact same safety guarantees as what's here, except now it'd need a much
> > bigger unsafe block and would have to do all of its manipulations via
> > pointers. That seems like a pretty big ergonomics burden without a clear
> > (to me) benefit.
> 
> It would require the same amount of unsafe & safety comments, but it
> wouldn't be bigger comments, since you can just as well create `&mut T`
> to the value.
> 
> ---
> Cheers,
> Benno

Powered by blists - more mailing lists