[RFC 2/2] rust: sync: Add atomic support

[Boqun Feng]

On Sun, Jun 16, 2024 at 09:46:45AM +0000, Benno Lossin wrote:
> On 16.06.24 00:12, Boqun Feng wrote:
> > On Sat, Jun 15, 2024 at 07:09:30AM +0000, Benno Lossin wrote:
> >> On 15.06.24 03:33, Boqun Feng wrote:
> >>> On Fri, Jun 14, 2024 at 09:22:24PM +0000, Benno Lossin wrote:
> >>>> On 14.06.24 16:33, Boqun Feng wrote:
> >>>>> On Fri, Jun 14, 2024 at 11:59:58AM +0200, Miguel Ojeda wrote:
> >>>>>> On Thu, Jun 13, 2024 at 9:05 PM Boqun Feng <boqun.feng at gmail.com> wrote:
> >>>>>>>
> >>>>>>> Does this make sense?
> >>>>>>
> >>>>>> Implementation-wise, if you think it is simpler or more clear/elegant
> >>>>>> to have the extra lower level layer, then that sounds fine.
> >>>>>>
> >>>>>> However, I was mainly talking about what we would eventually expose to
> >>>>>> users, i.e. do we want to provide `Atomic<T>` to begin with? If yes,
> >>>>>
> >>>>> The truth is I don't know ;-) I don't have much data on which one is
> >>>>> better. Personally, I think AtomicI32 and AtomicI64 make the users have
> >>>>> to think about size, alignment, etc, and I think that's important for
> >>>>> atomic users and people who review their code, because before one uses
> >>>>> atomics, one should ask themselves: why don't I use a lock? Atomics
> >>>>> provide the ablities to do low level stuffs and when doing low level
> >>>>> stuffs, you want to be more explicit than ergonomic.
> >>>>
> >>>> How would this be different with `Atomic<i32>` and `Atomic<i64>`? Just
> >>>
> >>> The difference is that with Atomic{I32,I64} APIs, one has to choose (and
> >>> think about) the size when using atomics, and cannot leave that option
> >>> open. It's somewhere unconvenient, but as I said, atomics variables are
> >>> different. For example, if someone is going to implement a reference
> >>> counter struct, they can define as follow:
> >>>
> >>> 	struct Refcount<T> {
> >>> 	    refcount: AtomicI32,
> >>> 	    data: UnsafeCell<T>
> >>> 	}
> >>>
> >>> but with atomic generic, people can leave that option open and do:
> >>>
> >>> 	struct Refcount<R, T> {
> >>> 	    refcount: Atomic<R>,
> >>> 	    data: UnsafeCell<T>
> >>> 	}
> >>>
> >>> while it provides configurable options for experienced users, but it
> >>> also provides opportunities for sub-optimal types, e.g. Refcount<u8, T>:
> >>> on ll/sc architectures, because `data` and `refcount` can be in the same
> >>> machine-word, the accesses of `refcount` are affected by the accesses of
> >>> `data`.
> >>
> >> I think this is a non-issue. We have two options of counteracting this:
> >> 1. We can just point this out in reviews and force people to use
> >>    `Atomic<T>` with a concrete type. In cases where there really is the
> >>    need to be generic, we can have it.
> >> 2. We can add a private trait in the bounds for the generic, nobody
> >>    outside of the module can access it and thus they need to use a
> >>    concrete type:
> >>
> >>         // needs a better name
> >>         trait Integer {}
> >>         impl Integer for i32 {}
> >>         impl Integer for i64 {}
> >>
> >>         pub struct Atomic<T: Integer> {
> >>             /* ... */
> >>         }
> >>
> >> And then in the other module, you can't do this (with compiler error):
> >>
> >>         pub struct Refcount<R: Integer, T> {
> >>                             // ^^^^^^^ not found in this scope
> >>                             // note: trait `crate::atomic::Integer` exists but is inaccessible
> >>             refcount: Atomic<R>,
> >>             data: UnsafeCell<T>,
> >>         }
> >>
> >> I think that we can start with approach 2 and if we find a use-case
> >> where generics are really unavoidable, we can either put it in the same
> >> module as `Atomic<T>`, or change the access of `Integer`.
> >>
> > 
> > What's the issue of having AtomicI32 and AtomicI64 first then? We don't
> > need to do 1 or 2 until the real users show up.
> 
> Generics allow you to avoid code duplication (I don't think that you
> want to create the `Atomic{I32,I64}` types via macros...). We would have
> to do a lot of refactoring, when we want to introduce it. I don't see

You can simply do

	type AtomicI32=Atomic<i32>;

Plus, we always do refactoring in kernel, because it's impossible to get
everything right at the first time. TBH, it's too confident to think one
can.

> the harm of introducing generics from the get-go.
> 
> > And I'd like also to point out that there are a few more trait bound
> > designs needed for Atomic<T>, for example, Atomic<u32> and Atomic<i32>
> > have different sets of API (no inc_unless_negative() for u32).
> 
> Sure, just like Gary said, you can just do:
> 
>     impl Atomic<i32> {
>         pub fn inc_unless_negative(&self, ordering: Ordering) -> bool;
>     }
> 
> Or add a `HasNegative` trait.
> 
> > Don't make me wrong, I have no doubt we can handle this in the type
> > system, but given the design work need, won't it make sense that we take
> > baby steps on this? We can first introduce AtomicI32 and AtomicI64 which
> > already have real users, and then if there are some values of generic
> > atomics, we introduce them and have proper discussion on design.
> 
> I don't understand this point, why can't we put in the effort for a good
> design? AFAIK we normally spend considerable time to get the API right
> and I think in this case it would include making it generic.
> 

What's the design you propose here? Well, the conversation between us is
only the design bit I saw, elsewhere it's all handwaving that "generics
are overall really good". I'm happy to get the API right, and it's easy
and simple to do on concrete types. But IIUC, Gary's suggestion is to
only have Atomic<i32> and Atomic<i64> first, and do the design later,
which I really don't like. It may not be a complete design, but I need
to see the design now to understand whether we need to go to that
direction. I cannot just introduce a TBD generic.

Regards,
Boqun

> > To me, it's perfectly fine that Atomic{I32,I64} co-exist with Atomic<T>.
> > What's the downside? A bit specific example would help me understand
> > the real concern here.
> 
> I don't like that, why have two ways of doing the same thing? People
> will be confused whether they should use `AtomicI32` vs `Atomic<i32>`...
> 
> ---
> Cheers,
> Benno
>