Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1610383 > unrolled thread

Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations Ingo Molnar <mingo@kernel.org> - 2017-03-28 10:00 +0200
    Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations Peter Zijlstra <peterz@infradead.org> - 2017-03-28 11:30 +0200
      Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations Dmitry Vyukov <dvyukov@google.com> - 2017-03-28 11:50 +0200
      Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations Dmitry Vyukov <dvyukov@google.com> - 2017-03-28 12:00 +0200
        Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations Ingo Molnar <mingo@kernel.org> - 2017-03-28 12:20 +0200
          Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations Dmitry Vyukov <dvyukov@google.com> - 2017-03-28 18:40 +0200
      Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations Ingo Molnar <mingo@kernel.org> - 2017-03-28 12:00 +0200

#1610383 — Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations

* Dmitry Vyukov <dvyukov@google.com> wrote:

> On Fri, Mar 24, 2017 at 11:57 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Dmitry Vyukov <dvyukov@google.com> wrote:
> >
> >> > Are just utterly disgusting that turn perfectly readable code into an
> >> > unreadable, unmaintainable mess.
> >> >
> >> > You need to find some better, cleaner solution please, or convince me that no
> >> > such solution is possible. NAK for the time being.
> >>
> >> Well, I can just write all functions as is. Does it better confirm to kernel
> >> style?
> >
> > I think writing the prototypes out as-is, properly organized, beats any of these
> > macro based solutions.
> 
> You mean write out the prototypes, but use what for definitions? Macros again?

No, regular C code.

I don't see the point of generating all this code via CPP - it's certainly not 
making it more readable to me. I.e. this patch I commented on is a step backwards 
for readability.

I'd prefer repetition and a higher overall line count over complex CPP constructs.

Thanks,

	Ingo

[toc] | [next] | [standalone]

#1610470

On Tue, Mar 28, 2017 at 09:52:32AM +0200, Ingo Molnar wrote:

> No, regular C code.
> 
> I don't see the point of generating all this code via CPP - it's certainly not 
> making it more readable to me. I.e. this patch I commented on is a step backwards 
> for readability.

Note that much of the atomic stuff we have today is all CPP already.

x86 is the exception because its 'weird', but most other archs are
almost pure CPP -- check Alpha for example, or asm-generic/atomic.h.

Also, look at linux/atomic.h, its a giant maze of CPP.

The CPP help us generate functions, reduces endless copy/paste (which
induces random differences -- read bugs) and construct variants
depending on the architecture input.

Yes, the CPP is a pain, but writing all that out explicitly is more of a
pain.



I've not yet looked too hard at these patches under consideration; and I
really wish we could get the compiler to do the right thing here, but
reducing the endless copy/paste that's otherwise the result of this, is
something I've found to be very valuable.

Not to mention that adding additional atomic ops got trivial (the set is
now near complete, so that's not much of an argument anymore -- but it
was, its what kept me sane sanitizing the atomic ops across all our 25+
architectures).

[toc] | [prev] | [next] | [standalone]

#1610493

On Tue, Mar 28, 2017 at 11:27 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Mar 28, 2017 at 09:52:32AM +0200, Ingo Molnar wrote:
>
>> No, regular C code.
>>
>> I don't see the point of generating all this code via CPP - it's certainly not
>> making it more readable to me. I.e. this patch I commented on is a step backwards
>> for readability.
>
> Note that much of the atomic stuff we have today is all CPP already.
>
> x86 is the exception because its 'weird', but most other archs are
> almost pure CPP -- check Alpha for example, or asm-generic/atomic.h.
>
> Also, look at linux/atomic.h, its a giant maze of CPP.
>
> The CPP help us generate functions, reduces endless copy/paste (which
> induces random differences -- read bugs) and construct variants
> depending on the architecture input.
>
> Yes, the CPP is a pain, but writing all that out explicitly is more of a
> pain.
>
>
>
> I've not yet looked too hard at these patches under consideration; and I
> really wish we could get the compiler to do the right thing here, but
> reducing the endless copy/paste that's otherwise the result of this, is
> something I've found to be very valuable.
>
> Not to mention that adding additional atomic ops got trivial (the set is
> now near complete, so that's not much of an argument anymore -- but it
> was, its what kept me sane sanitizing the atomic ops across all our 25+
> architectures).


I am almost done with Ingo's proposal, including de-macro-ifying x86
atomic ops code.
I am ready to do either of them, I think both have pros and cons and
there is no perfect solution. But please agree on something.


While we are here, one thing that I noticed is that 32-bit atomic code
uses 'long long' for 64-bit operands, while 64-bit code uses 'long'
for 64-bit operands. This sorta worked more of less before, but
ultimately it makes it impossible to write any portable code (e.g. you
don't know what format specifier to use to print return value of
atomic64_read, nor what local variable type to use to avoid compiler
warnings). With the try_cmpxchg it become worse, because 'long*' is
not convertible to 'long long*' so it is not possible to write any
portable code that uses it. If you declare 'old' variable as 'long'
32-bit code won't compile, if you declare it as 'long long' 64-bit
code won't compiler. I think we need to switch to a single type for
64-bit operands/return values, e.g. 'long long'.

[toc] | [prev] | [next] | [standalone]

#1610507

On Tue, Mar 28, 2017 at 11:51 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Peter Zijlstra <peterz@infradead.org> wrote:
>
>> On Tue, Mar 28, 2017 at 09:52:32AM +0200, Ingo Molnar wrote:
>>
>> > No, regular C code.
>> >
>> > I don't see the point of generating all this code via CPP - it's certainly not
>> > making it more readable to me. I.e. this patch I commented on is a step backwards
>> > for readability.
>>
>> Note that much of the atomic stuff we have today is all CPP already.
>
> Yeah, but there it's implementational: we pick up arch primitives depending on
> whether they are defined, such as:
>
> #ifndef atomic_read_acquire
> # define atomic_read_acquire(v)         smp_load_acquire(&(v)->counter)
> #endif
>
>> x86 is the exception because its 'weird', but most other archs are
>> almost pure CPP -- check Alpha for example, or asm-generic/atomic.h.
>
> include/asm-generic/atomic.h looks pretty clean and readable overall.
>
>> Also, look at linux/atomic.h, its a giant maze of CPP.
>
> Nah, that's OK, much of is is essentially __weak inlines implemented via CPP -
> i.e. CPP is filling in a missing compiler feature.
>
> But this patch I replied to appears to add instrumentation wrappery via CPP which
> looks like excessive and avoidable obfuscation to me.
>
> If it's much more readable and much more compact than the C version then maybe,
> but I'd like to see the C version first and see ...
>
>> The CPP help us generate functions, reduces endless copy/paste (which induces
>> random differences -- read bugs) and construct variants depending on the
>> architecture input.
>>
>> Yes, the CPP is a pain, but writing all that out explicitly is more of a
>> pain.
>
> So I'm not convinced that it's true in this case.
>
> Could we see the C version and compare? I could be wrong about it all.

Here it is (without instrumentation):
https://gist.github.com/dvyukov/e33d580f701019e0cd99429054ff1f9a

Instrumentation will add for each function:

 static __always_inline void atomic64_set(atomic64_t *v, long long i)
 {
+       kasan_check_write(v, sizeof(*v));
        arch_atomic64_set(v, i);
 }

[toc] | [prev] | [next] | [standalone]

#1610528

* Dmitry Vyukov <dvyukov@google.com> wrote:

> > So I'm not convinced that it's true in this case.
> >
> > Could we see the C version and compare? I could be wrong about it all.
> 
> Here it is (without instrumentation):
> https://gist.github.com/dvyukov/e33d580f701019e0cd99429054ff1f9a

Could you please include the full patch so that it can be discussed via email and 
such?

> Instrumentation will add for each function:
> 
>  static __always_inline void atomic64_set(atomic64_t *v, long long i)
>  {
> +       kasan_check_write(v, sizeof(*v));
>         arch_atomic64_set(v, i);
>  }

That in itself looks sensible and readable.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1611203

On Tue, Mar 28, 2017 at 12:15 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Dmitry Vyukov <dvyukov@google.com> wrote:
>
>> > So I'm not convinced that it's true in this case.
>> >
>> > Could we see the C version and compare? I could be wrong about it all.
>>
>> Here it is (without instrumentation):
>> https://gist.github.com/dvyukov/e33d580f701019e0cd99429054ff1f9a
>
> Could you please include the full patch so that it can be discussed via email and
> such?


Mailed the whole series.


>> Instrumentation will add for each function:
>>
>>  static __always_inline void atomic64_set(atomic64_t *v, long long i)
>>  {
>> +       kasan_check_write(v, sizeof(*v));
>>         arch_atomic64_set(v, i);
>>  }
>
> That in itself looks sensible and readable.
>
> Thanks,
>
>         Ingo

[toc] | [prev] | [next] | [standalone]

#1610511

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, Mar 28, 2017 at 09:52:32AM +0200, Ingo Molnar wrote:
> 
> > No, regular C code.
> > 
> > I don't see the point of generating all this code via CPP - it's certainly not 
> > making it more readable to me. I.e. this patch I commented on is a step backwards 
> > for readability.
> 
> Note that much of the atomic stuff we have today is all CPP already.

Yeah, but there it's implementational: we pick up arch primitives depending on 
whether they are defined, such as:

#ifndef atomic_read_acquire
# define atomic_read_acquire(v)		smp_load_acquire(&(v)->counter)
#endif

> x86 is the exception because its 'weird', but most other archs are
> almost pure CPP -- check Alpha for example, or asm-generic/atomic.h.

include/asm-generic/atomic.h looks pretty clean and readable overall.

> Also, look at linux/atomic.h, its a giant maze of CPP.

Nah, that's OK, much of is is essentially __weak inlines implemented via CPP - 
i.e. CPP is filling in a missing compiler feature.

But this patch I replied to appears to add instrumentation wrappery via CPP which 
looks like excessive and avoidable obfuscation to me.

If it's much more readable and much more compact than the C version then maybe, 
but I'd like to see the C version first and see ...

> The CPP help us generate functions, reduces endless copy/paste (which induces 
> random differences -- read bugs) and construct variants depending on the 
> architecture input.
> 
> Yes, the CPP is a pain, but writing all that out explicitly is more of a
> pain.

So I'm not convinced that it's true in this case.

Could we see the C version and compare? I could be wrong about it all.

Thanks,

	Ingo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web