Re: [PATCH v3 1/7] rust: percpu: introduce a rust API for per-CPU variables

[Mitchell Levy <levymitchell0@gmail.com>]

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