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@xxxxxxxxxxxxxxxxxxxx/ > >> 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
