[RFC,v3,01/26] asm-generic: atomic64: allow using generic atomic64 on 64bit platforms
diff mbox series

Message ID 39e1313ff3cf3eab6ceb5ae322fcd3e5d4432167.1580882335.git.thehajime@gmail.com
State New
Headers show
Series
  • [RFC,v3,01/26] asm-generic: atomic64: allow using generic atomic64 on 64bit platforms
Related show

Commit Message

Hajime Tazaki Feb. 5, 2020, 7:30 a.m. UTC
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.

Signed-off-by: Octavian Purdila <tavi.purdila@gmail.com>
Cc: Will Deacon <will@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
---
 include/asm-generic/atomic64.h | 2 ++
 1 file changed, 2 insertions(+)

Comments

Peter Zijlstra Feb. 5, 2020, 9:34 a.m. UTC | #1
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.
Octavian Purdila Feb. 5, 2020, 12:24 p.m. UTC | #2
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.
Anton Ivanov Feb. 5, 2020, 12:29 p.m. UTC | #3
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).
Peter Zijlstra Feb. 5, 2020, 12:49 p.m. UTC | #4
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.
Octavian Purdila Feb. 5, 2020, 2 p.m. UTC | #5
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
Peter Zijlstra Feb. 5, 2020, 5:13 p.m. UTC | #6
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.
Octavian Purdila Feb. 7, 2020, 12:32 p.m. UTC | #7
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)

Patch
diff mbox series

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) }