Date: Sun, 16 Jun 2024 09:30:53 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Benno Lossin <benno.lossin@...ton.me>
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 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.

> >> 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.

> > 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.

> 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.

> >> - 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?

> 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?

Regards,
Boqun

> ---
> Cheers,
> Benno
> 

Powered by blists - more mailing lists