Message-Id: <DBCR1OCNYAUW.1VLAY1HWCHLGI@kernel.org>
Date: Tue, 15 Jul 2025 17:55:13 +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 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
do our usual due diligence and work out a safe abstraction. Only fall
back to unsafe if it isn't possible.

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

>> > 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
going to be a mess. Do the experimenting out of tree and learn there.

---
Cheers,
Benno

Powered by blists - more mailing lists