Message-ID: <68b9ee59.170a0220.a7a31.675c@mx.google.com>
Date: Thu, 4 Sep 2025 12:53:59 -0700
From: Mitchell Levy <levymitchell0@...il.com>
To: Yury Norov <yury.norov@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...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>,
	Benno Lossin <lossin@...nel.org>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	Tyler Hicks <code@...icks.com>, linux-kernel@...r.kernel.org,
	rust-for-linux@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v3 1/7] rust: percpu: introduce a rust API for per-CPU
 variables

On Wed, Sep 03, 2025 at 05:42:08PM -0400, Yury Norov wrote:
> On Thu, Aug 28, 2025 at 12:00:08PM -0700, Mitchell Levy wrote:
> > Per-CPU variables are an important tool for reducing lock contention,
> > especially in systems with many processors. They also provide a
> > convenient way to handle data that are logically associated with a
> > particular CPU (e.g., the currently running task). Therefore, add a Rust
> > API to make use of per-CPU variables.
> > 
> > Add a `CpuGuard` type that disables preemption for its lifetime. Add a
> > `PerCpuAllocation` type used to track dynamic allocations. Add a
> > `define_per_cpu!` macro to create static per-CPU allocations. Add
> > `DynamicPerCpu` and `StaticPerCpu` to provide a high-level API. Add a
> > `PerCpu` trait to unify the dynamic and static cases.
> > 
> > Co-developed-by: Boqun Feng <boqun.feng@...il.com>
> > Signed-off-by: Boqun Feng <boqun.feng@...il.com>
> > Signed-off-by: Mitchell Levy <levymitchell0@...il.com>
> > ---
> >  rust/helpers/helpers.c          |   2 +
> >  rust/helpers/percpu.c           |   9 ++
> >  rust/helpers/preempt.c          |  14 +++
> >  rust/kernel/lib.rs              |   3 +
> >  rust/kernel/percpu.rs           | 223 ++++++++++++++++++++++++++++++++++++++++
> >  rust/kernel/percpu/cpu_guard.rs |  35 +++++++
> >  rust/kernel/percpu/dynamic.rs   |  83 +++++++++++++++
> >  rust/kernel/percpu/static_.rs   | 132 ++++++++++++++++++++++++
> >  8 files changed, 501 insertions(+)
> 
> That's quite a massive patch. Can you please split-out C-binders, and
> maybe make separate patches for each .rs component?

Sure, will do so for v4.

> > diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> > index 7cf7fe95e41d..2fc8d26cfe66 100644

[...]

> > --- /dev/null
> > +++ b/rust/kernel/percpu.rs
> > @@ -0,0 +1,223 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//! This module contains abstractions for creating and using per-CPU variables from Rust.
> > +//! See the define_per_cpu! macro and the DynamicPerCpu<T> type, as well as the PerCpu<T> trait.
> > +
> > +pub mod cpu_guard;
> > +mod dynamic;
> > +mod static_;
> > +
> > +#[doc(inline)]
> > +pub use dynamic::*;
> > +#[doc(inline)]
> > +pub use static_::*;
> > +
> > +use bindings::{alloc_percpu, free_percpu};
> > +
> > +use crate::alloc::Flags;
> > +use crate::percpu::cpu_guard::CpuGuard;
> > +use crate::prelude::*;
> > +use crate::sync::Arc;
> > +use crate::types::Opaque;
> > +use crate::{declare_extern_per_cpu, get_static_per_cpu};
> > +
> > +use core::arch::asm;
> > +use core::cell::{Cell, RefCell, UnsafeCell};
> > +use core::mem::{align_of, size_of, MaybeUninit};
> > +
> > +use ffi::c_void;
> > +
> > +/// A per-CPU pointer; that is, an offset into the per-CPU area. Note that this type is NOT a smart
> > +/// pointer, it does not manage the allocation.
> > +pub struct PerCpuPtr<T>(*mut MaybeUninit<T>);
> > +
> > +/// Represents exclusive access to the memory location pointed at by a particular PerCpu<T>.
> > +pub struct PerCpuToken<'a, T> {
> > +    // INVARIANT: the current CPU's memory location associated with the per-CPU variable pointed at
> > +    // by `ptr` (i.e., the entry in the per-CPU area on the current CPU) has been initialized.
> > +    _guard: CpuGuard,
> > +    ptr: &'a PerCpuPtr<T>,
> > +}
> > +
> > +/// Represents access to the memory location pointed at by a particular PerCpu<T> where the type
> > +/// `T` manages access to the underlying memory to avoid aliaising troubles. (For example, `T`
> > +/// might be a `Cell` or `RefCell`.)
> > +pub struct CheckedPerCpuToken<'a, T> {
> > +    // INVARIANT: the current CPU's memory location associated with the per-CPU variable pointed at
> > +    // by `ptr` (i.e., the entry in the per-CPU area on the current CPU) has been initialized.
> > +    _guard: CpuGuard,
> > +    ptr: &'a PerCpuPtr<T>,
> > +}
> > +
> > +impl<T> PerCpuPtr<T> {
> > +    /// Makes a new PerCpuPtr from a raw per-CPU pointer.
> > +    ///
> > +    /// # Safety
> > +    /// `ptr` must be a valid per-CPU pointer.
> > +    pub unsafe fn new(ptr: *mut MaybeUninit<T>) -> Self {
> > +        Self(ptr)
> > +    }
> > +
> > +    /// Get a `&mut MaybeUninit<T>` to the per-CPU variable on the current CPU represented by `&self`
> > +    ///
> > +    /// # Safety
> > +    /// The returned `&mut T` must follow Rust's aliasing rules. That is, no other `&(mut) T` may
> > +    /// exist that points to the same location in memory. In practice, this means that `get_(mut_)ref`
> 
> How long is this line?

102 chars, or 103 if you include the newline. `rustfmt` doesn't break
the line, so I left it as-is for this patch. Happy to change it if it
poses a problem, though.

> > +    /// must not be called on another `PerCpuPtr<T>` that is a copy/clone of `&self` for as long as
> > +    /// the returned reference lives.
> > +    ///
> > +    /// CPU preemption must be disabled before calling this function and for the lifetime of the
> > +    /// returned reference. Otherwise, the returned reference might end up being a reference to a
> > +    /// different CPU's per-CPU area, causing the potential for a data race.
> > +    #[allow(clippy::mut_from_ref)] // Safety requirements prevent aliasing issues
> > +    pub unsafe fn get_mut_ref(&self) -> &mut MaybeUninit<T> {
> > +        // SAFETY: `self.get_ptr()` returns a valid pointer to a `MaybeUninit<T>` by its contract,
> > +        // and the safety requirements of this function ensure that the returned reference is
> > +        // exclusive.
> > +        unsafe { &mut *(self.get_ptr()) }
> > +    }
> 
> Here and everywhere: would it make sense to enforce it by testing
> the CPU with preemptible() before returning a reference? 

The only thing we could do would be to panic, which I don't 100% love.
Another alternative would be to take a &'a CpuGuard and bound the
lifetime of the returned reference to 'a, and then we don't need to do
any run-time checking at all.

Originally, I had left this to the caller because it might make sense
down the line for some complex behavior based on per-CPU (e.g., per-CPU
refcount) to do all its own management of per-CPU variables using
`PerCpuPtr` as a core primitive. In these cases, I believe there are
some times where being non-preemptible wouldn't actually be required
(that said, my thoughts on this aren't well reflected in the safety
comment, since I said it must be disabled... gah). But, the more I think
about it, the more I think these use cases would be better served by
just using `get_ptr` --- conjuring `&mut` references seems like it would
be a big footgun. And the safety comment already actually reflects these
thoughts somewhat :)

For v4 I will probably have this function take a &'a CpuGuard and use
that to bound the liftetime of the returned reference, unless there are
other thoughts on this point.

Thanks,
Mitchell

> Thanks,
> Yury

Powered by blists - more mailing lists