[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <68ba01db.170a0220.31417f.72dc@mx.google.com>
Date: Thu, 4 Sep 2025 14:17:07 -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 Thu, Sep 04, 2025 at 04:27:43PM -0400, Yury Norov wrote:
> On Thu, Sep 04, 2025 at 12:53:59PM -0700, Mitchell Levy wrote:
> > 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:
>
> ...
>
> > > > + /// 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.
>
> Then don't use that tool - it's broken. In kernel we used to have 80-chars
> limit for the lines, recently relaxed to 100.
Sure, will fix.
> > > > + /// 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 :)
>
> If you think that in future there will be a user who will not need to
> disable preemption before dereferencing a percpu pointer, then you can
> add another less restricted flavor of the helper.
Yeah, that's fair.
> > 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.
>
> I don't want you to panic just because of invlid user call, but
> whatever you call in comment must be enforced in code, right?
>
>
> You can use the guard, if it guarantees the preemption disabled; or
> you can return None; you can create CONFIG_RUST_PERCPU_HARDENED for
> panics.
Yes, the existence of a `CpuGuard` guarantees that preemption is
disabled.
> Please refer the recent bitmap API wrapper, and how erroneous request
> is handled there.
>
> https://lore.kernel.org/all/20250904165015.3791895-4-bqe@google.com/
Thanks,
Mitchell
Powered by blists - more mailing lists