Re: [RFC v2 04/13] rust: sync: atomic: Add generic atomics

[Boqun Feng <boqun.feng@gmail.com>]

On Fri, Dec 13, 2024 at 03:32:47PM +0100, Alice Ryhl wrote:
> On Thu, Dec 12, 2024 at 6:34 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > On Thu, Dec 12, 2024 at 11:57:07AM +0100, Alice Ryhl wrote:
> > [...]
> > > > diff --git a/rust/kernel/sync/atomic/generic.rs b/rust/kernel/sync/atomic/generic.rs
> > > > new file mode 100644
> > > > index 000000000000..204da38e2691
> > > > --- /dev/null
> > > > +++ b/rust/kernel/sync/atomic/generic.rs
> > > > @@ -0,0 +1,253 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +
> > > > +//! Generic atomic primitives.
> > > > +
> > > > +use super::ops::*;
> > > > +use super::ordering::*;
> > > > +use crate::types::Opaque;
> > > > +
> > > > +/// A generic atomic variable.
> > > > +///
> > > > +/// `T` must impl [`AllowAtomic`], that is, an [`AtomicImpl`] has to be chosen.
> > > > +///
> > > > +/// # Invariants
> > > > +///
> > > > +/// Doing an atomic operation while holding a reference of [`Self`] won't cause a data race, this
> > > > +/// is guaranteed by the safety requirement of [`Self::from_ptr`] and the extra safety requirement
> > > > +/// of the usage on pointers returned by [`Self::as_ptr`].
> > > > +#[repr(transparent)]
> > > > +pub struct Atomic<T: AllowAtomic>(Opaque<T>);
> > > > +
> > > > +// SAFETY: `Atomic<T>` is safe to share among execution contexts because all accesses are atomic.
> > > > +unsafe impl<T: AllowAtomic> Sync for Atomic<T> {}
> > >
> > > Surely it should also be Send?
> > >
> >
> > It's `Send` here because `Opaque<T>` is `Send` when `T` is `Send`. And
> > in patch #9, I changed the definition of `AllowAtomic`, which is not a
> > subtrait of `Send` anymore, and an `impl Send` block was added there.
> >
> > > > +/// Atomics that support basic atomic operations.
> > > > +///
> > > > +/// TODO: Unless the `impl` is a `#[repr(transparet)]` new type of an existing [`AllowAtomic`], the
> > > > +/// impl block should be only done in atomic mod. And currently only basic integer types can
> > > > +/// implement this trait in atomic mod.
> > >
> > > What's up with this TODO? Can't you just write an appropriate safety
> > > requirement?
> > >
> >
> > Because the limited scope of types that allows atomic is an artificial
> > choice, i.e. we want to start with a limited number of types and make
> > forward progress, and the types that we don't want to support atomics
> > for now are not because of safety reasons, but more of a lack of
> > users/motivations. So I don't think this is something we should use
> > safety requirement to describe.
> 
> I found the wording very confusing. Could you reword it to say
> something about future possibilities?
> 

Sure, how about:

/// TODO: Currently the [`AllowAtomic`] types are restricted within
/// basic integer types (and their transparent new types). In the
/// future, we could extend the scope to more data types when there is a
/// clear and meaningful usage, but for now, [`AllowAtomic`] should only
/// be implemented inside atomic mod for the restricted types mentioned
/// above.

?

> > > > +/// # Safety
> > > > +///
> > > > +/// [`Self`] must have the same size and alignment as [`Self::Repr`].
> > > > +pub unsafe trait AllowAtomic: Sized + Send + Copy {
> > > > +    /// The backing atomic implementation type.
> > > > +    type Repr: AtomicImpl;
> > > > +
> > > > +    /// Converts into a [`Self::Repr`].
> > > > +    fn into_repr(self) -> Self::Repr;
> > > > +
> > > > +    /// Converts from a [`Self::Repr`].
> > > > +    fn from_repr(repr: Self::Repr) -> Self;
> > >
> > > What do you need these methods for?
> > >
> >
> > Converting a `AtomicImpl` value (currently only `i32` and `i64`) to a
> > `AllowAtomic` value without using transmute in `impl` block of
> > `Atomic<T>`. Any better idea?
> 
> You could use transmute?
> 

In a draft version, I did use transmute, but Benno commented that he
wanted to avoid arbitrary transmute as hard as possible (if I didn't
misunderstand him). Hence these two functions are provided. Now think
about it, I don't think doing either way (transmute or *_repr()
function) would affect most of users, since most of users won't need to 
impl `AllowAtomic` themselves, therefore I think keeping it as it is is
fine. Do you have any user observable concern of defining these
functions?

Regards,
Boqun

> Alice