| Message ID | 39e1313ff3cf3eab6ceb5ae322fcd3e5d4432167.1580882335.git.thehajime@gmail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [RFC,v3,01/26] asm-generic: atomic64: allow using generic atomic64 on 64bit platforms | expand |
On Wed, Feb 05, 2020 at 04:30:10PM +0900, Hajime Tazaki wrote: > From: Octavian Purdila <tavi.purdila@gmail.com> > > With CONFIG_64BIT enabled, atomic64 via CONFIG_GENERIC_ATOMIC64 options > are not compiled due to type conflict of atomic64_t defined in > linux/type.h. > > This commit fixes the issue and allow using generic atomic64 ops. > > Currently, LKL is only the user which defines GENERIC_ATOMIC64 > (lib/atomic64.c) under CONFIG_64BIT environment. Thus, there is no > issues before this commit. Uhhhhh, no. Not allowing GENERIC_ATOMIC64 on 64BIT is a *feature*. Any 64bit arch that needs GENERIC_ATOMIC64 is an utter piece of crap. Please explain more.
On Wed, Feb 5, 2020 at 11:34 AM Peter Zijlstra <peterz@infradead.org> wrote: > > On Wed, Feb 05, 2020 at 04:30:10PM +0900, Hajime Tazaki wrote: > > From: Octavian Purdila <tavi.purdila@gmail.com> > > > > With CONFIG_64BIT enabled, atomic64 via CONFIG_GENERIC_ATOMIC64 options > > are not compiled due to type conflict of atomic64_t defined in > > linux/type.h. > > > > This commit fixes the issue and allow using generic atomic64 ops. > > > > Currently, LKL is only the user which defines GENERIC_ATOMIC64 > > (lib/atomic64.c) under CONFIG_64BIT environment. Thus, there is no > > issues before this commit. > > Uhhhhh, no. > > Not allowing GENERIC_ATOMIC64 on 64BIT is a *feature*. > > Any 64bit arch that needs GENERIC_ATOMIC64 is an utter piece of crap. > > Please explain more. > Hi Peter, I was not aware that not allowing GENERIC_ATOMIC64 was intentional. I understand your point that a 64 bit architecture that can't handle 64 bit atomic operation is broken. One way to deal with this in LKL would be to use GCC atomic builtins or if that doesn't work expose them as host operations. This would keep LKL as a meta-arch that can run on multiple physical architectures. I'll give it a try.
On 05/02/2020 12:24, Octavian Purdila wrote: > On Wed, Feb 5, 2020 at 11:34 AM Peter Zijlstra <peterz@infradead.org> wrote: >> >> On Wed, Feb 05, 2020 at 04:30:10PM +0900, Hajime Tazaki wrote: >>> From: Octavian Purdila <tavi.purdila@gmail.com> >>> >>> With CONFIG_64BIT enabled, atomic64 via CONFIG_GENERIC_ATOMIC64 options >>> are not compiled due to type conflict of atomic64_t defined in >>> linux/type.h. >>> >>> This commit fixes the issue and allow using generic atomic64 ops. >>> >>> Currently, LKL is only the user which defines GENERIC_ATOMIC64 >>> (lib/atomic64.c) under CONFIG_64BIT environment. Thus, there is no >>> issues before this commit. >> >> Uhhhhh, no. >> >> Not allowing GENERIC_ATOMIC64 on 64BIT is a *feature*. >> >> Any 64bit arch that needs GENERIC_ATOMIC64 is an utter piece of crap. >> >> Please explain more. >> > > Hi Peter, > > I was not aware that not allowing GENERIC_ATOMIC64 was intentional. I > understand your point that a 64 bit architecture that can't handle 64 > bit atomic operation is broken. > > One way to deal with this in LKL would be to use GCC atomic builtins > or if that doesn't work expose them as host operations. This would > keep LKL as a meta-arch that can run on multiple physical > architectures. I'll give it a try. > > _______________________________________________ > linux-um mailing list > linux-um@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-um > You can lift a set of defines which do that for most compilers/arches out of OVS code. Have a look at lib/ovs-atomic*.h It should do exactly what you want (+/- cutting it down to things you need).
On Wed, Feb 05, 2020 at 02:24:38PM +0200, Octavian Purdila wrote: > I was not aware that not allowing GENERIC_ATOMIC64 was intentional. I It might not have been, but presented with this patch, I feel like it should've been :-) > understand your point that a 64 bit architecture that can't handle 64 > bit atomic operation is broken. (sadly they actually exist, I shall name no names) > One way to deal with this in LKL would be to use GCC atomic builtins > or if that doesn't work expose them as host operations. This would > keep LKL as a meta-arch that can run on multiple physical > architectures. I'll give it a try. What is this LKL you speak of and how does it do the 32bit atomics? One thing to keep in mind is that the C11 atomics (_Atomic) don't trivially map to the LKMM -- although I keep forgetting the exact details, there is a paper on it somewhere. Also, once you're limited to a specific arch the issue also becomes much easier than C11 vs LKMM in general.
On Wed, Feb 5, 2020 at 2:49 PM Peter Zijlstra <peterz@infradead.org> wrote: > > On Wed, Feb 05, 2020 at 02:24:38PM +0200, Octavian Purdila wrote: > > I was not aware that not allowing GENERIC_ATOMIC64 was intentional. I > > It might not have been, but presented with this patch, I feel like it > should've been :-) > > > understand your point that a 64 bit architecture that can't handle 64 > > bit atomic operation is broken. > > (sadly they actually exist, I shall name no names) > > > One way to deal with this in LKL would be to use GCC atomic builtins > > or if that doesn't work expose them as host operations. This would > > keep LKL as a meta-arch that can run on multiple physical > > architectures. I'll give it a try. > > What is this LKL you speak of and how does it do the 32bit atomics? > LKL is a build of the Linux kernel as a library that can run in many environments including multiple architectures and OSes [1] For 32bit atomics LKL also uses the asm-generic implementation. It is very similar with generic 64bit atomic implementation and it is used by multiple 32bit arches. I think this was my original reasoning for this patch and not going with C11 atomics. > One thing to keep in mind is that the C11 atomics (_Atomic) don't > trivially map to the LKMM -- although I keep forgetting the exact > details, there is a paper on it somewhere. Also, once you're limited to > a specific arch the issue also becomes much easier than C11 vs LKMM in > general. Thanks, I will look it up. [1] https://github.com/lkl/linux
On Wed, Feb 05, 2020 at 04:00:41PM +0200, Octavian Purdila wrote: > On Wed, Feb 5, 2020 at 2:49 PM Peter Zijlstra <peterz@infradead.org> wrote: > > > > On Wed, Feb 05, 2020 at 02:24:38PM +0200, Octavian Purdila wrote: > > > I was not aware that not allowing GENERIC_ATOMIC64 was intentional. I > > > > It might not have been, but presented with this patch, I feel like it > > should've been :-) > > > > > understand your point that a 64 bit architecture that can't handle 64 > > > bit atomic operation is broken. > > > > (sadly they actually exist, I shall name no names) > > > > > One way to deal with this in LKL would be to use GCC atomic builtins > > > or if that doesn't work expose them as host operations. This would > > > keep LKL as a meta-arch that can run on multiple physical > > > architectures. I'll give it a try. > > > > What is this LKL you speak of and how does it do the 32bit atomics? > > > > LKL is a build of the Linux kernel as a library that can run in many > environments including multiple architectures and OSes [1] Thanks, I'll put it on the to-read list. > For 32bit atomics LKL also uses the asm-generic implementation. It is > very similar with generic 64bit atomic implementation and it is used > by multiple 32bit arches. I think this was my original reasoning for > this patch and not going with C11 atomics. Uh no, asm-generic/atomic.h is radically different from lib/atomic64.c. asm-generic/atomic.h builds all required atomic operations from cmpxchg() (loops), while lib/atomic64.c builds 64bit atomics by using a hashed set of spinlocks. The asm-generic stuff gives you real atomic ops, albeit sub-optimal, lib/atomic64.c gives you a turd.
On Wed, Feb 5, 2020 at 7:13 PM Peter Zijlstra <peterz@infradead.org> wrote: > > On Wed, Feb 05, 2020 at 04:00:41PM +0200, Octavian Purdila wrote: > > On Wed, Feb 5, 2020 at 2:49 PM Peter Zijlstra <peterz@infradead.org> wrote: > > > > > > On Wed, Feb 05, 2020 at 02:24:38PM +0200, Octavian Purdila wrote: > > > > I was not aware that not allowing GENERIC_ATOMIC64 was intentional. I > > > > > > It might not have been, but presented with this patch, I feel like it > > > should've been :-) > > > > > > > understand your point that a 64 bit architecture that can't handle 64 > > > > bit atomic operation is broken. > > > > > > (sadly they actually exist, I shall name no names) > > > > > > > One way to deal with this in LKL would be to use GCC atomic builtins > > > > or if that doesn't work expose them as host operations. This would > > > > keep LKL as a meta-arch that can run on multiple physical > > > > architectures. I'll give it a try. > > > > > > What is this LKL you speak of and how does it do the 32bit atomics? > > > > > > > LKL is a build of the Linux kernel as a library that can run in many > > environments including multiple architectures and OSes [1] > > Thanks, I'll put it on the to-read list. > > > For 32bit atomics LKL also uses the asm-generic implementation. It is > > very similar with generic 64bit atomic implementation and it is used > > by multiple 32bit arches. I think this was my original reasoning for > > this patch and not going with C11 atomics. > > Uh no, asm-generic/atomic.h is radically different from lib/atomic64.c. > > asm-generic/atomic.h builds all required atomic operations from > cmpxchg() (loops), while lib/atomic64.c builds 64bit atomics by using a > hashed set of spinlocks. > > The asm-generic stuff gives you real atomic ops, albeit sub-optimal, > lib/atomic64.c gives you a turd. Ah, yes, I overlooked that. (I think that they are equivalent for LKL because it is a non-SMP arch only at this moment, but I agree that the better approach is to have a proper implementation in LKL)
diff --git a/include/asm-generic/atomic64.h b/include/asm-generic/atomic64.h index 370f01d4450f..9b15847baae5 100644 --- a/include/asm-generic/atomic64.h +++ b/include/asm-generic/atomic64.h @@ -9,9 +9,11 @@ #define _ASM_GENERIC_ATOMIC64_H #include <linux/types.h> +#ifndef CONFIG_64BIT typedef struct { s64 counter; } atomic64_t; +#endif #define ATOMIC64_INIT(i) { (i) }