Message-ID: <0653b5d5-7a62-4baa-a500-4c110d816ba0@proton.me>
Date: Sun, 16 Jun 2024 15:55:12 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Boqun Feng <boqun.feng@...il.com>
Cc: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>, Gary Guo <gary@...yguo.net>, rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org, llvm@...ts.linux.dev, Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Wedson Almeida Filho <wedsonaf@...il.com>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...sung.com>, Alice Ryhl <aliceryhl@...gle.com>, Alan Stern <stern@...land.harvard.edu>, Andrea Parri <parri.andrea@...il.com>, Will Deacon <will@...nel.org>, Peter Zijlstra <peterz@...radead.org>, Nicholas Piggin <npiggin@...il.com>, David Howells <dhowells@...hat.com>, Jade Alglave <j.alglave@....ac.uk>, Luc Maranget <luc.maranget@...ia.fr>, "Paul E. McKenney" <paulmck@...nel.org>, Akira Yokosawa <akiyks@...il.com>, Daniel Lustig <dlustig@...dia.com>, Joel Fernandes <joel@...lfernandes.org>, Nathan Chancellor <nathan@...nel.org>, Nick Desaulniers <ndesaulniers@...gle.com>,
	kent.overstreet@...il.com, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, elver@...gle.com, Mark Rutland <mark.rutland@....com>, Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>, Catalin Marinas <catalin.marinas@....com>, torvalds@...ux-foundation.org, linux-arm-kernel@...ts.infradead.org, linux-fsdevel@...r.kernel.org, Trevor Gross <tmgross@...ch.edu>, dakr@...hat.com
Subject: Re: [RFC 2/2] rust: sync: Add atomic support

On 16.06.24 17:34, Boqun Feng wrote:
> On Sun, Jun 16, 2024 at 03:06:36PM +0000, Benno Lossin wrote:
>> On 16.06.24 16:08, Boqun Feng wrote:
>>> 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@...il.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>;
>>
>> Eh, I would think that we could just do a text replacement in this case.
>> Or if that doesn't work, Coccinelle should be able to do this...
>>
>>> 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.
>>
>> I don't think that we're at the "let's just put it in" stage. This is an
>> RFC version, so it should be fine to completely change the approach.
> 
> I'm fine as well. I wasn't trying to rush anything, but as I mentioned
> below, I need some more design from people who want it to understand
> whether that's a good idea.
> 
>> I agree, that we can't get it 100% right the first time, but we should
>> at least strive to get a good version.
>>
>>>> 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.
>>
>> I don't think that the idea was to "do the design later". I don't even
>> know how you would do that, since you need the design to submit a patch.
>>
> 
> Then I might mis-understand Gary? He said:
> 
> "Can we avoid two types and use a generic `Atomic<T>` and then implement
> on `Atomic<i32>` and `Atomic<i64>` instead?"
> 
> , which means just replace `impl AtomicI32` with `impl Atomic<i32>` to
> me.

This is a fair interpretation, but what prevents you from merging the
impls of functions that can be? I assumed that you would do that
automatically.

>> I can't offer you a complete API description, since that would require
>> me writing it up myself. But I would recommend trying to get it to work
>> with generics. I got a few other comments:
> 
> We should work on things that are concrete, right? It's fine that the
> design is not complete, and it's fine if you just recommend. But without
> a somewhat concrete design (doesn't have to be complete), I cannot be
> sure about whether we have the same vision of the future of generic
> atomics (see my question to Gary), that's a bit hard for me to try to

Sorry, which question?
Also to be aligned on the vision, I think we should rather talk about
the vision and not the design, since the design that we want to have now
can be different from the vision. On that note, what do you envision the
future of the atomic API?

> work something out (plus I personally don't think it's a good idea, it's
> OK to me, but not good). Anyway, I wasn't trying to refuse to do this
> just based on personal reasons, but I do need to understand what you are
> all proposing, because I don't have one myself.

I went back through the thread and here is what I think your argument
against generics is: people should think about size and alignment when
using atomics, which is problematic when allowing users to leave the
atomic generic.
But as I argued before, this is not an issue. Have I overlooked another
argument? Because I don't see anything else.

>> - I don't think that we should resort to a script to generate the Rust
>>   code since it prevents adding good documentation & examples to the
>>   various methods. AFAIU you want to generate the functions from
>>   `scripts/atomic/atomics.tbl` to keep it in sync with the C side. I
>>   looked at the git log of that file and it hasn't been changed
>>   significantly since its inception. I don't think that there is any
>>   benefit to generating the functions from that file.
> 
> I'll leave this to other atomic maintainers.
> 
>> - most of the documented functions say "See `c_function`", I don't like
>>   this, can we either copy the C documentation (I imagine it not
>>   changing that often, or is that assumption wrong?) or write our own?
> 
> You're not wrong. AN in C side, we do have some documentation template
> to generate the comments (see scripts/atomic/kerneldoc). But first the
> format is for doxygen(I guess?), and second as you just bring up, the
> templates are tied with the bash script.

I see a bash script similarly to how Wedson sees proc macros ;)
We should try *hard* to avoid them and only use them when there is no
other way.

>> - we should try to use either const generic or normal parameters for the
>>   access ordering instead of putting it in the function name.
> 
> Why is it important? Keeping it in the current way brings the value that
> it's not too much different than their C counterparts. Could you explain
> a bit the pros and cons on suffix vs const generic approach?

Reduce code duplication, instead of 3 different variants, we can have
one. It allows people to build ergonomic APIs that allows the user to
decide which synchronization they use under the hood.

>> - why do we need both non-return and return variants?
>>
> 
> Historical reason I guess. Plus maybe some architectures have a better
> implementation on non-return atomics IIRC.

Could we get some more concrete arguments for why we would need these in
rust? If the reason is purely historical, then we shouldn't expose the
non-return variant IMO. If it is because of performance, then we can
only expose them in the respective arches.

---
Cheers,
Benno

Powered by blists - more mailing lists