Re: [PATCH v3 5/7] rust: percpu: Support non-zeroable types for DynamicPerCpu

[Mitchell Levy <levymitchell0@gmail.com>]

On Thu, Sep 04, 2025 at 01:05:36AM +0200, Miguel Ojeda wrote:
> On Thu, Aug 28, 2025 at 9:01 PM Mitchell Levy <levymitchell0@gmail.com> wrote:
> >
> > +    /// Get a `*mut MaybeUninit<T>` to the per-CPU variable on the CPU represented by `cpu`. Note
> > +    /// that without some kind of synchronization, use of the returned pointer may cause a data
> > +    /// race. It is the caller's responsibility to use the returned pointer in a reasonable way.
> 
> Please try to make the first paragraph ("short description" / title) smaller.

Will do.

> Does "reasonable" mean anything different than any other raw pointer?

This will depend very heavily on `T`, there's a detailed discussion
here: https://docs.kernel.org/core-api/this_cpu_ops.html#remote-access-to-per-cpu-data

In general, remote accesses are strongly discouraged, and my intention
was mostly just wanting to reflect that in this documentation.

> > +    /// # Safety
> 
> Newline after section headers (also elsewhere).

Will do.

> > +    /// - The returned pointer is valid only if `self` is (that is, it points to a live allocation
> > +    ///   correctly sized and aligned to hold a `T`)
> > +    /// - The returned pointer is valid only if the bit corresponding to `cpu` is set in
> > +    ///   `Cpumask::possible()`.
> 
> It sounds like the returned pointer can be invalid without triggering
> UB -- could you please clarify why the method is `unsafe`?

Yes, this is true; strictly speaking, calling this function without
dereferencing the returned pointer is always safe. I suppose I find it
clearer that, when a function has preconditions that are necessary for
it to return a valid pointer, the "safe-ness" has more to do with the
functions preconditions than the act of dereferencing the pointer.

In this case, the pointer shouldn't be going very far, but I think this
logic applies especially well in cases where pointers might be getting
stored away for later (and so the validity of the dereference later on
might rely on non-local conditions).

All that said, I'm alright with having this be a safe function, with the
documentation noting these requirements for the returned pointer to be
valid.

> In addition, please use intra-doc links wherever possible (e.g. there
> a was also an `Arc` elsewhere).

Will do.

> > +        // SAFETY: The requirements of this function ensure this call is safe.
> > +        unsafe { bindings::per_cpu_ptr(self.0.cast(), cpu.as_u32()) }.cast()
> 
> Please try to explain why, not just that it is. It isn't clear how the
> safety preconditions, which only seem to talk about the returned
> pointer, make this OK.

This flows from the first requirement (that `self` is a live allocation,
which is necessary for `per_cpu_ptr` to return a valid pointer). Though,
as above, simply possessing this pointer isn't UB, so it's arguable that
any call to `per_cpu_ptr` (on x86 at least, I'm not sure how it's
implemented on other arches) is always safe. Regardless, I agree this
comment should be more clear (even if the function is safe, it's
probably worth noting why the pointer returned is valid when the
function preconditions are met); will fix.

Thanks,
Mitchell

> Thanks!
> 
> Cheers,
> Miguel