Date: Wed, 19 Jun 2024 09:09:46 +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 18:30, Boqun Feng wrote:
> On Sun, Jun 16, 2024 at 03:55:12PM +0000, Benno Lossin wrote:
> [...]
>>>>
>>>> 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 think you missed the point, Gary's suggestion at that time sounds
> like: let's impl Atomic<i32> and Atomic<i64> first, and leave rest of
> the work for later, that is a "do the design later" to me.

Hmm, but wouldn't that just be less work for you?

>>>> 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?
> 
> 	https://lore.kernel.org/rust-for-linux/Zm7_XWe6ciy1yN-h@Boquns-Mac-mini.home/
> 
>> 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?
>>
> 
> Mine is simple, just providing AtomicI32 and AtomicI64 first, since
> there are immediate users right now, and should we learn more needs from
> the users, we get more idea about what a good API is, and we evolve from
> there.

That is fine, but since we want to get generics in the future anyways, I
think it would be good to already implement them now to also gather
feedback on them.

>>> 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
> 
> You mean you said it's a non-issue but gave me two counteract? If it's
> really a non-issue, you won't need a couneraction, right? In other words
> non-generic atomics do provide some value.

The second counteractions would provide exactly the same API surface as
your non-generic version, so I don't see how going non-generic provides
any value over going generic.
The first approach was intended for a future in which we are not scared
of people using generic atomics in their structs. I don't know if we are
going to be in that future, so I think we should go with the second
approach for the time being.

>> 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.
>>
> 
> I will just start with the existing mechanism and try to evolve, whether
> it's a script or proc macro, I don't mind, I want to get the work done
> and most relevant people can understand in the way the they prefer and
> step-by-step, move it to the place I think is the best for the project.

I don't think that we need a script or a proc macro. A few declarative
macros probably suffice if we go the way of generics.

>>>> - 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.
>>
> 
> I already mentioned why I think it's good in the current way, I will
> defer it to others on their inputs.
> 
>>>> - 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
> 
> Sure. Look like my memory is correct, at least on ARM64 they are
> different instructions (see arch/arm64/include/asm/atomic_lse.h)
> 
> non-return atomics on ARM64:
> 
> 	#define ATOMIC_OP(op, asm_op)						\
> 	static __always_inline void						\
> 	__lse_atomic_##op(int i, atomic_t *v)					\
> 	{									\
> 		asm volatile(							\
> 		__LSE_PREAMBLE							\
> 		"	" #asm_op "	%w[i], %[v]\n"				\
> 		: [v] "+Q" (v->counter)						\
> 		: [i] "r" (i));							\
> 	}
> 
> value-return atomics on ARM64:
> 
> 	#define ATOMIC_FETCH_OP(name, mb, op, asm_op, cl...)			\
> 	static __always_inline int						\
> 	__lse_atomic_fetch_##op##name(int i, atomic_t *v)			\
> 	{									\
> 		int old;							\
> 										\
> 		asm volatile(							\
> 		__LSE_PREAMBLE							\
> 		"	" #asm_op #mb "	%w[i], %w[old], %[v]"			\
> 		: [v] "+Q" (v->counter),					\
> 		  [old] "=r" (old)						\
> 		: [i] "r" (i)							\
> 		: cl);								\
> 										\
> 		return old;							\
> 	}
> 
> It may not be easy to see the different instructions from the pasted
> code, but you can find them in the header file, also you could notice
> that the number of operands is different?

This is not my expertise, so I believe you :)

>> non-return variant IMO. If it is because of performance, then we can
>> only expose them in the respective arches.
>>
> 
> Hmm.. so we ask user to write arch-specific code like:
> 
> 	pub fn increase_counter(&self) {
> 	    #[cfg(CONFIG_ARM64)]
> 	    self.counter.inc();
> 
> 	    #[cfg(CONFIG_X86_64)]
> 	    let _ = self.counter.inc_return();
> 	}
> 
> are you sure it's a good idea?

No that looks horrible.

Maybe there is something that we can do with generics, but I don't know
if it is worth it. I guess we can leave that open for the time being.

---
Cheers,
Benno

Powered by blists - more mailing lists