linux-next: build warnings after merge of the access_once tree

linux-next: build warnings after merge of the access_once tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 955 bytes --]

Hi Christian,

After merging the access_once tree, today's linux-next build (arm
multi_v7_defconfig) produced lots of this warning:

In file included from include/linux/linkage.h:4:0,
                 from include/linux/preempt.h:9,
                 from include/linux/spinlock.h:50,
                 from include/linux/lockref.h:17,
                 from lib/lockref.c:2:
In function '__read_once_size',
    inlined from 'lockref_get' at lib/lockref.c:50:2:
include/linux/compiler.h:216:3: warning: call to 'data_access_exceeds_word_size' declared with attribute warning: data access exceeds word size and won't be atomic
   data_access_exceeds_word_size();
   ^

Introduced by commit 6becd6bd5e89 ("compiler.h: Fix word size check for
READ/WRITE_ONCE") presumably interacting with commit 4d3199e4ca8e
("locking: Remove ACCESS_ONCE() usage") from the tip tree.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: linux-next: build warnings after merge of the access_once tree

[Christian Borntraeger]

Am 26.03.2015 um 09:31 schrieb Stephen Rothwell:
> Hi Christian,
> 
> After merging the access_once tree, today's linux-next build (arm
> multi_v7_defconfig) produced lots of this warning:
> 
> In file included from include/linux/linkage.h:4:0,
>                  from include/linux/preempt.h:9,
>                  from include/linux/spinlock.h:50,
>                  from include/linux/lockref.h:17,
>                  from lib/lockref.c:2:
> In function '__read_once_size',
>     inlined from 'lockref_get' at lib/lockref.c:50:2:
> include/linux/compiler.h:216:3: warning: call to 'data_access_exceeds_word_size' declared with attribute warning: data access exceeds word size and won't be atomic
>    data_access_exceeds_word_size();
>    ^
> 
> Introduced by commit 6becd6bd5e89 ("compiler.h: Fix word size check for
> READ/WRITE_ONCE") presumably interacting with commit 4d3199e4ca8e
> ("locking: Remove ACCESS_ONCE() usage") from the tip tree.
> 

The point of my patch was to actually re-enable the formerly broken check.
And indeed on arm 32 bit 
we read a 64bit variable (lock_count). There is no possible way of doing that
in an atomic fashion with READ_ONCE, so the warning is probably correct

code in lib/lockref.c

#define CMPXCHG_LOOP(CODE, SUCCESS) do {                                        \
        struct lockref old;                                                     \
        BUILD_BUG_ON(sizeof(old) != 8);                                         \
        old.lock_count = READ_ONCE(lockref->lock_count);                        \
        while (likely(arch_spin_value_unlocked(old.lock.rlock.raw_lock))) {     \
                struct lockref new = old, prev = old;                           \
                CODE                                                            \
                old.lock_count = cmpxchg64_relaxed(&lockref->lock_count,        \
                                                   old.lock_count,              \
                                                   new.lock_count);             \
                if (likely(old.lock_count == prev.lock_count)) {                \
                        SUCCESS;                                                \
                }                                                               \
                cpu_relax_lowlatency();                                         \
        }                                                                       \
} while (0)

Re: linux-next: build warnings after merge of the access_once tree

[Peter Zijlstra]

On Thu, Mar 26, 2015 at 07:31:12PM +1100, Stephen Rothwell wrote:
> Hi Christian,
> 
> After merging the access_once tree, today's linux-next build (arm
> multi_v7_defconfig) produced lots of this warning:
> 
> In file included from include/linux/linkage.h:4:0,
>                  from include/linux/preempt.h:9,
>                  from include/linux/spinlock.h:50,
>                  from include/linux/lockref.h:17,
>                  from lib/lockref.c:2:
> In function '__read_once_size',
>     inlined from 'lockref_get' at lib/lockref.c:50:2:
> include/linux/compiler.h:216:3: warning: call to 'data_access_exceeds_word_size' declared with attribute warning: data access exceeds word size and won't be atomic
>    data_access_exceeds_word_size();
>    ^
> 
> Introduced by commit 6becd6bd5e89 ("compiler.h: Fix word size check for
> READ/WRITE_ONCE") presumably interacting with commit 4d3199e4ca8e
> ("locking: Remove ACCESS_ONCE() usage") from the tip tree.

Hmm, valid warning though, ARM is a 32bit arch and therefore it will
'have' to load a u64 in two goes, which violates the
ACCESS_ONCE/READ_ONCE 'promise'.

Now ARM can indeed to the cmpxchg64 thing, but I'm not sure what to do
here, I suspect the code is fine, seeing how the cmpxchg64 will fail if
the split loads got it wrong, but I've not overly thought about it.

Re: linux-next: build warnings after merge of the access_once tree

[Will Deacon]

On Thu, Mar 26, 2015 at 10:34:42AM +0000, Peter Zijlstra wrote:
> On Thu, Mar 26, 2015 at 07:31:12PM +1100, Stephen Rothwell wrote:
> > Hi Christian,
> > 
> > After merging the access_once tree, today's linux-next build (arm
> > multi_v7_defconfig) produced lots of this warning:
> > 
> > In file included from include/linux/linkage.h:4:0,
> >                  from include/linux/preempt.h:9,
> >                  from include/linux/spinlock.h:50,
> >                  from include/linux/lockref.h:17,
> >                  from lib/lockref.c:2:
> > In function '__read_once_size',
> >     inlined from 'lockref_get' at lib/lockref.c:50:2:
> > include/linux/compiler.h:216:3: warning: call to 'data_access_exceeds_word_size' declared with attribute warning: data access exceeds word size and won't be atomic
> >    data_access_exceeds_word_size();
> >    ^
> > 
> > Introduced by commit 6becd6bd5e89 ("compiler.h: Fix word size check for
> > READ/WRITE_ONCE") presumably interacting with commit 4d3199e4ca8e
> > ("locking: Remove ACCESS_ONCE() usage") from the tip tree.
> 
> Hmm, valid warning though, ARM is a 32bit arch and therefore it will
> 'have' to load a u64 in two goes, which violates the
> ACCESS_ONCE/READ_ONCE 'promise'.
> 
> Now ARM can indeed to the cmpxchg64 thing, but I'm not sure what to do
> here, I suspect the code is fine, seeing how the cmpxchg64 will fail if
> the split loads got it wrong, but I've not overly thought about it.

Yeah, I think it's fine because, as you point out, the cmpxchg can only
succeed if the 64-bit load appeared to be single-copy atomic (amongst other
things).

Have fun getting ACCESS_ONCE to figure that out, though...

Will

Re: linux-next: build warnings after merge of the access_once tree

[Peter Zijlstra]

On Thu, Mar 26, 2015 at 01:27:50PM +0000, Will Deacon wrote:
> On Thu, Mar 26, 2015 at 10:34:42AM +0000, Peter Zijlstra wrote:
> > On Thu, Mar 26, 2015 at 07:31:12PM +1100, Stephen Rothwell wrote:
> > > In function '__read_once_size',
> > >     inlined from 'lockref_get' at lib/lockref.c:50:2:


> Yeah, I think it's fine because, as you point out, the cmpxchg can only
> succeed if the 64-bit load appeared to be single-copy atomic (amongst other
> things).

So one option to get rid of this warning is to rely on the fact that all
CMPXCHG_LOOP users are at the beginning of !pure function calls, which
already imply a compiler barrier and therefore it must already emit that
load.

And as already argued, split loads aren't an issue because the cmpxchg
will catch those for us.

So we can either just remove the READ_ONCE(), or replace it with a
leading barrier() call just to be on the paranoid side of things.

Any preferences?

Something like so, but with a sensible comment I suppose.

---
 lib/lockref.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/lockref.c b/lib/lockref.c
index 494994bf17c8..b5ca1f65c8a3 100644
--- a/lib/lockref.c
+++ b/lib/lockref.c
@@ -18,7 +18,8 @@
 #define CMPXCHG_LOOP(CODE, SUCCESS) do {					\
 	struct lockref old;							\
 	BUILD_BUG_ON(sizeof(old) != 8);						\
-	old.lock_count = READ_ONCE(lockref->lock_count);			\
+	barrier();								\
+	old.lock_count = lockref->lock_count;					\
 	while (likely(arch_spin_value_unlocked(old.lock.rlock.raw_lock))) {  	\
 		struct lockref new = old, prev = old;				\
 		CODE								\

Re: linux-next: build warnings after merge of the access_once tree

[Will Deacon]

On Thu, Mar 26, 2015 at 02:22:20PM +0000, Peter Zijlstra wrote:
> On Thu, Mar 26, 2015 at 01:27:50PM +0000, Will Deacon wrote:
> > On Thu, Mar 26, 2015 at 10:34:42AM +0000, Peter Zijlstra wrote:
> > > On Thu, Mar 26, 2015 at 07:31:12PM +1100, Stephen Rothwell wrote:
> > > > In function '__read_once_size',
> > > >     inlined from 'lockref_get' at lib/lockref.c:50:2:
> 
> 
> > Yeah, I think it's fine because, as you point out, the cmpxchg can only
> > succeed if the 64-bit load appeared to be single-copy atomic (amongst other
> > things).
> 
> So one option to get rid of this warning is to rely on the fact that all
> CMPXCHG_LOOP users are at the beginning of !pure function calls, which
> already imply a compiler barrier and therefore it must already emit that
> load.
> 
> And as already argued, split loads aren't an issue because the cmpxchg
> will catch those for us.
> 
> So we can either just remove the READ_ONCE(), or replace it with a
> leading barrier() call just to be on the paranoid side of things.

If we remove the READ_ONCE then I think the barrier is a good idea, just in
case the LTO guys get their paws on this and we see subtle breakage.

> Any preferences?
> 
> Something like so, but with a sensible comment I suppose.
> 
> ---
>  lib/lockref.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/lockref.c b/lib/lockref.c
> index 494994bf17c8..b5ca1f65c8a3 100644
> --- a/lib/lockref.c
> +++ b/lib/lockref.c
> @@ -18,7 +18,8 @@
>  #define CMPXCHG_LOOP(CODE, SUCCESS) do {					\
>  	struct lockref old;							\
>  	BUILD_BUG_ON(sizeof(old) != 8);						\
> -	old.lock_count = READ_ONCE(lockref->lock_count);			\
> +	barrier();								\
> +	old.lock_count = lockref->lock_count;					\
>  	while (likely(arch_spin_value_unlocked(old.lock.rlock.raw_lock))) {  	\
>  		struct lockref new = old, prev = old;				\
>  		CODE								\

Is ACCESS_ONCE actually going away? It has its problems, but I think it's
what we want here and reads better than magic barrier() imo.

Will

Re: linux-next: build warnings after merge of the access_once tree

[Peter Zijlstra]

On Thu, Mar 26, 2015 at 02:41:54PM +0000, Will Deacon wrote:
> > +++ b/lib/lockref.c
> > @@ -18,7 +18,8 @@
> >  #define CMPXCHG_LOOP(CODE, SUCCESS) do {					\
> >  	struct lockref old;							\
> >  	BUILD_BUG_ON(sizeof(old) != 8);						\
> > -	old.lock_count = READ_ONCE(lockref->lock_count);			\
> > +	barrier();								\
> > +	old.lock_count = lockref->lock_count;					\
> >  	while (likely(arch_spin_value_unlocked(old.lock.rlock.raw_lock))) {  	\
> >  		struct lockref new = old, prev = old;				\
> >  		CODE								\
> 
> Is ACCESS_ONCE actually going away? 

I've been arguing for that yes, having two APIs for the 'same' thing is
confusing at best, and as the comment near the READ_ONCE() thing
explains, ACCESS_ONCE() has serious, silent, issues.

> It has its problems, but I think it's
> what we want here and reads better than magic barrier() imo.

Yeah, but its also misleading because we rely on silent fail. Part of
the ACCESS_ONCE() semantics is that it should avoid split loads, and
we're here actually relying on emitting just that.

Re: linux-next: build warnings after merge of the access_once tree

[Will Deacon]

On Thu, Mar 26, 2015 at 02:51:44PM +0000, Peter Zijlstra wrote:
> On Thu, Mar 26, 2015 at 02:41:54PM +0000, Will Deacon wrote:
> > > +++ b/lib/lockref.c
> > > @@ -18,7 +18,8 @@
> > >  #define CMPXCHG_LOOP(CODE, SUCCESS) do {					\
> > >  	struct lockref old;							\
> > >  	BUILD_BUG_ON(sizeof(old) != 8);						\
> > > -	old.lock_count = READ_ONCE(lockref->lock_count);			\
> > > +	barrier();								\
> > > +	old.lock_count = lockref->lock_count;					\
> > >  	while (likely(arch_spin_value_unlocked(old.lock.rlock.raw_lock))) {  	\
> > >  		struct lockref new = old, prev = old;				\
> > >  		CODE								\
> > 
> > Is ACCESS_ONCE actually going away? 
> 
> I've been arguing for that yes, having two APIs for the 'same' thing is
> confusing at best, and as the comment near the READ_ONCE() thing
> explains, ACCESS_ONCE() has serious, silent, issues.
> 
> > It has its problems, but I think it's
> > what we want here and reads better than magic barrier() imo.
> 
> Yeah, but its also misleading because we rely on silent fail. Part of
> the ACCESS_ONCE() semantics is that it should avoid split loads, and
> we're here actually relying on emitting just that.

In which case, on the premise that we comment the barrier():

  Acked-by: Will Deacon <will.deacon@arm.com>

As an aside, ARMv7 (32-bit) with LPAE *can* emit single-copy atomic 64-bit
memory accesses and we rely on that for things like atomic64_read and
writing ptes. If we see WRITE_ONCE(pte), then we'll have genuine issues
with the way it's currently implemented.

Will

Re: linux-next: build warnings after merge of the access_once tree

[Linus Torvalds]

On Thu, Mar 26, 2015 at 7:22 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> So we can either just remove the READ_ONCE(), or replace it with a
> leading barrier() call just to be on the paranoid side of things.

NOOO!

> Any preferences?

Not a preference: a _requirement_.

Get rid of the f*cking size checks etc on READ_ONCE() and friends.

They are about - wait for it - "reading a value once".

Note how it doesn't say ANYTHING about "atomic" or anything like that.
It's about reading *ONCE*.

> Something like so, but with a sensible comment I suppose.

Hell f*cking no. The "something like so" is huge and utter crap,
because the barrier is on the wrong side.

> -       old.lock_count = READ_ONCE(lockref->lock_count);                        \
> +       barrier();                                                              \
> +       old.lock_count = lockref->lock_count;                                   \
>         while (likely(arch_spin_value_unlocked(old.lock.rlock.raw_lock))) {     \
>                 struct lockref new = old, prev = old;                           \

The WHOLE point of the READ_ONCE (formerly ACCESS_ONCE) is that it
tells the compiler that it cannot reload the value.

Notice how it is *not* about atomicitiy. The compiler can read the
value in fifteen pieces, randomly mixing one bit or five. Nobody
cares.

But the important thing is that ONCE IT IS READ, it is never read
again. That's the "once" part.

Why is that important? It's important because we have to absolutely
guartantee that the value we *test* is the same value we use later.
That's a common concern with mutable variables, and is the only reason
for READ_ONCE() in the first place.

The whole atomicity etc crap was just that - crap. It was never about
atomicitiy. It was about the compiler not reloading values.

So no. No barriers. No "removal of READ_ONCE". Just get rid of the
broken "sanity" checks in the READ_ONCE implementation that are just
pure garbage.

The checks in ACCESS_ONCE() were because apparently gcc got things
wrong - dropping the volatile - for aggregate types. They were never
supposed to be about atomicity, even if there clearly was some
confusion about that.

Really. Just get rid of the checks - they were wrong. They were
clearly very close to *introducing* a bug, rather than fixing anything
at all.

                             Linus

Re: linux-next: build warnings after merge of the access_once tree

[Linus Torvalds]

On Thu, Mar 26, 2015 at 9:15 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Really. Just get rid of the checks - they were wrong. They were
> clearly very close to *introducing* a bug, rather than fixing anything
> at all.

Side note: we will continue to expect the compiler to do single-word
accesses as a single acccess, rather than splitting things up. And
that's fine. READ_ONCE() uses "volatile", and it means on a language
level that the actual access is "visible", so it's a reasonable
expectation to have.

So the proper patch looks something like this:

    diff --git a/include/linux/compiler.h b/include/linux/compiler.h
    index 1b45e4a0519b..f36e1abf56ea 100644
    --- a/include/linux/compiler.h
    +++ b/include/linux/compiler.h
    @@ -198,10 +198,6 @@ __compiletime_warning("data access exceeds
word size and won't be atomic")
     #endif
     ;

    -static __always_inline void data_access_exceeds_word_size(void)
    -{
    -}
    -
     static __always_inline void __read_once_size(const volatile void
*p, void *res, int size)
     {
             switch (size) {
    @@ -214,7 +210,6 @@ static __always_inline void
__read_once_size(const volatile void *p, void *res,
             default:
                     barrier();
                     __builtin_memcpy((void *)res, (const void *)p, size);
    -                data_access_exceeds_word_size();
                     barrier();
             }
     }
    @@ -231,7 +226,6 @@ static __always_inline void
__write_once_size(volatile void *p, void *res, int s
             default:
                     barrier();
                     __builtin_memcpy((void *)p, (const void *)res, size);
    -                data_access_exceeds_word_size();
                     barrier();
             }
     }

and let's just leave it at that.

Note again how the default case just guarantees that it is never
reloaded by the compiler. That's the primary issue this is all about.
The whole "we expect the compiler to not be shit" is secondary and
_may_ be an issue in some places, but is not what the main goal is.

                             Linus

Re: linux-next: build warnings after merge of the access_once tree

[Peter Zijlstra]

On Thu, Mar 26, 2015 at 09:15:21AM -0700, Linus Torvalds wrote:
> Notice how it is *not* about atomicitiy. The compiler can read the
> value in fifteen pieces, randomly mixing one bit or five. Nobody
> cares.

If you read Documentation/memory-barriers.txt you'll find that it very
much also is about reading it in one go.

"The ACCESS_ONCE() function can prevent any number of optimizations that,
while perfectly safe in single-threaded code, can be fatal in concurrent
code.  Here are some examples of these sorts of optimizations:

 ...

 (*) For aligned memory locations whose size allows them to be accessed
     with a single memory-reference instruction, prevents "load tearing"
     and "store tearing," ..."

There are many places in the kernel where we rely and use ACCESS_ONCE()
in order to 'guarantee' single loads. Paul is the expert here, but from
what I understand the compiler is not allowed to split loads for
volatile reads (assuming the load is both naturally aligned and of
machine word size).

And the size check in READ_ONCE() helps asserting this.

Re: linux-next: build warnings after merge of the access_once tree

[Peter Zijlstra]

On Thu, Mar 26, 2015 at 09:21:50AM -0700, Linus Torvalds wrote:
> So the proper patch looks something like this:
> 
>     diff --git a/include/linux/compiler.h b/include/linux/compiler.h
>     index 1b45e4a0519b..f36e1abf56ea 100644
>     --- a/include/linux/compiler.h
>     +++ b/include/linux/compiler.h
>     @@ -198,10 +198,6 @@ __compiletime_warning("data access exceeds
> word size and won't be atomic")
>      #endif
>      ;

You also want to get rid of that ^^^ declaration which includes the
compiletime warning.

>     -static __always_inline void data_access_exceeds_word_size(void)
>     -{
>     -}
>     -
>      static __always_inline void __read_once_size(const volatile void
> *p, void *res, int size)
>      {
>              switch (size) {
>     @@ -214,7 +210,6 @@ static __always_inline void
> __read_once_size(const volatile void *p, void *res,
>              default:
>                      barrier();
>                      __builtin_memcpy((void *)res, (const void *)p, size);
>     -                data_access_exceeds_word_size();
>                      barrier();
>              }
>      }
>     @@ -231,7 +226,6 @@ static __always_inline void
> __write_once_size(volatile void *p, void *res, int s
>              default:
>                      barrier();
>                      __builtin_memcpy((void *)p, (const void *)res, size);
>     -                data_access_exceeds_word_size();
>                      barrier();
>              }
>      }

So this still has the potential to generate significantly worse code
than the previous ACCESS_ONCE() because of the dual barrier() and god
only knows how memcpy gets implemented.

As it turns out, the ARM version of __builtin_memcpy() does result in a
single double word load because its all smart and such, but a 'trivial'
implementation might just end up doing 8 byte copies.

Can't we make an argument that these barrier calls are not required? The
memcpy() call already guarantees we emit the loads and its opaque so the
compiler cannot 'cache' the value. So I see not immediate reason for the
dual memory clobber.

Re: linux-next: build warnings after merge of the access_once tree

[Peter Zijlstra]

On Thu, Mar 26, 2015 at 05:36:47PM +0100, Peter Zijlstra wrote:
> Can't we make an argument that these barrier calls are not required? The
> memcpy() call already guarantees we emit the loads and its opaque so the
> compiler cannot 'cache' the value. So I see not immediate reason for the
> dual memory clobber.

Oh wait, it needs to reassess the content of the target variable after
the memcpy of course.

Could we then at least make the 64bit case unconditional as well?

Re: linux-next: build warnings after merge of the access_once tree

[Peter Zijlstra]

On Thu, Mar 26, 2015 at 05:44:42PM +0100, Peter Zijlstra wrote:
> On Thu, Mar 26, 2015 at 05:36:47PM +0100, Peter Zijlstra wrote:
> > Can't we make an argument that these barrier calls are not required? The
> > memcpy() call already guarantees we emit the loads and its opaque so the
> > compiler cannot 'cache' the value. So I see not immediate reason for the
> > dual memory clobber.
> 
> Oh wait, it needs to reassess the content of the target variable after
> the memcpy of course.
> 
> Could we then at least make the 64bit case unconditional as well?

Like so.

---
 include/linux/compiler.h | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 1b45e4a0519b..0e41ca0e5927 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -192,29 +192,16 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
 
 #include <uapi/linux/types.h>
 
-static __always_inline void data_access_exceeds_word_size(void)
-#ifdef __compiletime_warning
-__compiletime_warning("data access exceeds word size and won't be atomic")
-#endif
-;
-
-static __always_inline void data_access_exceeds_word_size(void)
-{
-}
-
 static __always_inline void __read_once_size(const volatile void *p, void *res, int size)
 {
 	switch (size) {
 	case 1: *(__u8 *)res = *(volatile __u8 *)p; break;
 	case 2: *(__u16 *)res = *(volatile __u16 *)p; break;
 	case 4: *(__u32 *)res = *(volatile __u32 *)p; break;
-#ifdef CONFIG_64BIT
 	case 8: *(__u64 *)res = *(volatile __u64 *)p; break;
-#endif
 	default:
 		barrier();
 		__builtin_memcpy((void *)res, (const void *)p, size);
-		data_access_exceeds_word_size();
 		barrier();
 	}
 }
@@ -225,13 +212,10 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
 	case 1: *(volatile __u8 *)p = *(__u8 *)res; break;
 	case 2: *(volatile __u16 *)p = *(__u16 *)res; break;
 	case 4: *(volatile __u32 *)p = *(__u32 *)res; break;
-#ifdef CONFIG_64BIT
 	case 8: *(volatile __u64 *)p = *(__u64 *)res; break;
-#endif
 	default:
 		barrier();
 		__builtin_memcpy((void *)p, (const void *)res, size);
-		data_access_exceeds_word_size();
 		barrier();
 	}
 }

Re: linux-next: build warnings after merge of the access_once tree

[Peter Zijlstra]

On Thu, Mar 26, 2015 at 09:45:07AM -0700, Linus Torvalds wrote:

> Stop this idiocy.

Yeah, clearly I can type faster than I can think straight :/


In any case, I've the below patch; do you want to take it now or do you
want me to route it through tip/locking/urgent or something like that?

---
Subject: kernel: Remove atomicy checks from {READ,WRITE}_ONCE
From: Peter Zijlstra <peterz@infradead.org>
Date: Thu, 26 Mar 2015 17:45:37 +0100

The fact that volatile allows for atomic load/stores is a special case
not a requirement for {READ,WRITE}_ONCE(). Their primary purpose is to
force the compiler to emit load/stores _once_.

So remove the warning as it is correct behaviour. This also implies that
the u64 case is not 64bit only, so remove the #ifdef so we can generate
better code in that case.

Cc: Stephen Rothwell <sfr@canb.auug.org.au>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
Cc: Will Deacon <will.deacon@arm.com>
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/compiler.h | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 1b45e4a0519b..0e41ca0e5927 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -192,29 +192,16 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
 
 #include <uapi/linux/types.h>
 
-static __always_inline void data_access_exceeds_word_size(void)
-#ifdef __compiletime_warning
-__compiletime_warning("data access exceeds word size and won't be atomic")
-#endif
-;
-
-static __always_inline void data_access_exceeds_word_size(void)
-{
-}
-
 static __always_inline void __read_once_size(const volatile void *p, void *res, int size)
 {
 	switch (size) {
 	case 1: *(__u8 *)res = *(volatile __u8 *)p; break;
 	case 2: *(__u16 *)res = *(volatile __u16 *)p; break;
 	case 4: *(__u32 *)res = *(volatile __u32 *)p; break;
-#ifdef CONFIG_64BIT
 	case 8: *(__u64 *)res = *(volatile __u64 *)p; break;
-#endif
 	default:
 		barrier();
 		__builtin_memcpy((void *)res, (const void *)p, size);
-		data_access_exceeds_word_size();
 		barrier();
 	}
 }
@@ -225,13 +212,10 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
 	case 1: *(volatile __u8 *)p = *(__u8 *)res; break;
 	case 2: *(volatile __u16 *)p = *(__u16 *)res; break;
 	case 4: *(volatile __u32 *)p = *(__u32 *)res; break;
-#ifdef CONFIG_64BIT
 	case 8: *(volatile __u64 *)p = *(__u64 *)res; break;
-#endif
 	default:
 		barrier();
 		__builtin_memcpy((void *)p, (const void *)res, size);
-		data_access_exceeds_word_size();
 		barrier();
 	}
 }

Re: linux-next: build warnings after merge of the access_once tree

[Paul E. McKenney]

On Thu, Mar 26, 2015 at 09:37:50AM -0700, Linus Torvalds wrote:
> On Mar 26, 2015 9:29 AM, "Peter Zijlstra" <peterz@infradead.org> wrote:
> >
> > And the size check in READ_ONCE() helps asserting this.
> 
> No it d does NOT.
> 
> Even the documentation you quoted agrees with me: the shearing issue is
> only for sizes where it is appropriate. The size check is wrong, because it
> warns explicitly about the sizes where the shearing will happen, but the
> code is supposed to work anyway.
> 
> All the documentation documents a *special case* (although a common one),
> not some generic requirement.
> 
> And the point is - the warning is wrong, and it is counterproductive. And
> it causes people who should know better to try to introduce bugs.
> 
> The warning must go. This whole thread proves it. The warning is dangerous
> and actively misleading.

The only cases I can think of where this warning would be useful is
in 64-bit architecture-specific code that used u64 (or long long or
whatever) and needed to avoid both reloads and load shearing.  In those
(admittedly rare, perhaps nonexistent) cases, presumably the surrounding
code should do __native_word() if needed.  (In case the code were to be
mindlessly copied to a 32-bit architecture or something.)

Not sure that adding this to memory-barriers.txt is worthwhile, just
trying to make sure that I understand the tradeoffs.

							Thanx, Paul

Re: linux-next: build warnings after merge of the access_once tree

[Will Deacon]

On Thu, Mar 26, 2015 at 05:07:48PM +0000, Peter Zijlstra wrote:
> On Thu, Mar 26, 2015 at 09:45:07AM -0700, Linus Torvalds wrote:
> 
> > Stop this idiocy.
> 
> Yeah, clearly I can type faster than I can think straight :/
> 
> 
> In any case, I've the below patch; do you want to take it now or do you
> want me to route it through tip/locking/urgent or something like that?

This patch also works fine for me. I managed to get the compiler to split a
64-bit load into 2x32-bit loads using memcpy, so I do like keeping the
8-byte case available for 32-bit architectures than can make use of it.

	Acked-by: Will Deacon <will.deacon@arm.com>

Will

> Subject: kernel: Remove atomicy checks from {READ,WRITE}_ONCE
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Thu, 26 Mar 2015 17:45:37 +0100
> 
> The fact that volatile allows for atomic load/stores is a special case
> not a requirement for {READ,WRITE}_ONCE(). Their primary purpose is to
> force the compiler to emit load/stores _once_.
> 
> So remove the warning as it is correct behaviour. This also implies that
> the u64 case is not 64bit only, so remove the #ifdef so we can generate
> better code in that case.
> 
> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  include/linux/compiler.h | 16 ----------------
>  1 file changed, 16 deletions(-)
> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 1b45e4a0519b..0e41ca0e5927 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -192,29 +192,16 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>  
>  #include <uapi/linux/types.h>
>  
> -static __always_inline void data_access_exceeds_word_size(void)
> -#ifdef __compiletime_warning
> -__compiletime_warning("data access exceeds word size and won't be atomic")
> -#endif
> -;
> -
> -static __always_inline void data_access_exceeds_word_size(void)
> -{
> -}
> -
>  static __always_inline void __read_once_size(const volatile void *p, void *res, int size)
>  {
>  	switch (size) {
>  	case 1: *(__u8 *)res = *(volatile __u8 *)p; break;
>  	case 2: *(__u16 *)res = *(volatile __u16 *)p; break;
>  	case 4: *(__u32 *)res = *(volatile __u32 *)p; break;
> -#ifdef CONFIG_64BIT
>  	case 8: *(__u64 *)res = *(volatile __u64 *)p; break;
> -#endif
>  	default:
>  		barrier();
>  		__builtin_memcpy((void *)res, (const void *)p, size);
> -		data_access_exceeds_word_size();
>  		barrier();
>  	}
>  }
> @@ -225,13 +212,10 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
>  	case 1: *(volatile __u8 *)p = *(__u8 *)res; break;
>  	case 2: *(volatile __u16 *)p = *(__u16 *)res; break;
>  	case 4: *(volatile __u32 *)p = *(__u32 *)res; break;
> -#ifdef CONFIG_64BIT
>  	case 8: *(volatile __u64 *)p = *(__u64 *)res; break;
> -#endif
>  	default:
>  		barrier();
>  		__builtin_memcpy((void *)p, (const void *)res, size);
> -		data_access_exceeds_word_size();
>  		barrier();
>  	}
>  }
> 

Re: linux-next: build warnings after merge of the access_once tree

[Christian Borntraeger]

Am 26.03.2015 um 18:07 schrieb Peter Zijlstra:
> On Thu, Mar 26, 2015 at 09:45:07AM -0700, Linus Torvalds wrote:
> 
>> Stop this idiocy.
> 
> Yeah, clearly I can type faster than I can think straight :/
> 
> 
> In any case, I've the below patch; do you want to take it now or do you
> want me to route it through tip/locking/urgent or something like that?

Its not urgent. Current upstream has a broken check (gcc will not emit the
warning if the function is static). I just fixed the check in my next tree
but I can certainly drop that tree.

> 
> ---
> Subject: kernel: Remove atomicy checks from {READ,WRITE}_ONCE
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Thu, 26 Mar 2015 17:45:37 +0100
> 
> The fact that volatile allows for atomic load/stores is a special case
> not a requirement for {READ,WRITE}_ONCE(). Their primary purpose is to
> force the compiler to emit load/stores _once_.
> 
> So remove the warning as it is correct behaviour. This also implies that
> the u64 case is not 64bit only, so remove the #ifdef so we can generate
> better code in that case.
> 
> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>


> ---
>  include/linux/compiler.h | 16 ----------------
>  1 file changed, 16 deletions(-)
> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 1b45e4a0519b..0e41ca0e5927 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -192,29 +192,16 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
> 
>  #include <uapi/linux/types.h>
> 
> -static __always_inline void data_access_exceeds_word_size(void)
> -#ifdef __compiletime_warning
> -__compiletime_warning("data access exceeds word size and won't be atomic")
> -#endif
> -;
> -
> -static __always_inline void data_access_exceeds_word_size(void)
> -{
> -}
> -
>  static __always_inline void __read_once_size(const volatile void *p, void *res, int size)
>  {
>  	switch (size) {
>  	case 1: *(__u8 *)res = *(volatile __u8 *)p; break;
>  	case 2: *(__u16 *)res = *(volatile __u16 *)p; break;
>  	case 4: *(__u32 *)res = *(volatile __u32 *)p; break;
> -#ifdef CONFIG_64BIT
>  	case 8: *(__u64 *)res = *(volatile __u64 *)p; break;
> -#endif
>  	default:
>  		barrier();
>  		__builtin_memcpy((void *)res, (const void *)p, size);
> -		data_access_exceeds_word_size();
>  		barrier();
>  	}
>  }
> @@ -225,13 +212,10 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
>  	case 1: *(volatile __u8 *)p = *(__u8 *)res; break;
>  	case 2: *(volatile __u16 *)p = *(__u16 *)res; break;
>  	case 4: *(volatile __u32 *)p = *(__u32 *)res; break;
> -#ifdef CONFIG_64BIT
>  	case 8: *(volatile __u64 *)p = *(__u64 *)res; break;
> -#endif
>  	default:
>  		barrier();
>  		__builtin_memcpy((void *)p, (const void *)res, size);
> -		data_access_exceeds_word_size();
>  		barrier();
>  	}
>  }
> 

Re: linux-next: build warnings after merge of the access_once tree

[Christian Borntraeger]

Am 26.03.2015 um 17:15 schrieb Linus Torvalds:
> On Thu, Mar 26, 2015 at 7:22 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>>
>> So we can either just remove the READ_ONCE(), or replace it with a
>> leading barrier() call just to be on the paranoid side of things.
> 
> NOOO!
> 
>> Any preferences?
> 
> Not a preference: a _requirement_.
> 
> Get rid of the f*cking size checks etc on READ_ONCE() and friends.

Oh I just added that check back then because some guy named
Linus suggested something like that ;-)

--- snip ---
(Btw, it's not just aggregate types, even non-aggregate types like
"long long" are not necessarily safe, to give the same 64-bit on
x86-32 example. So adding an assert that it's smaller or equal in size
to a "long" might also not be unreasonable)
--- snip ---

https://www.marc.info/?l=linux-kernel&m=141565366209769&w=1

I am fine with Peters patch :-)

Christian

> 
> They are about - wait for it - "reading a value once".
> 
> Note how it doesn't say ANYTHING about "atomic" or anything like that.
> It's about reading *ONCE*.


> 
>> Something like so, but with a sensible comment I suppose.
> 
> Hell f*cking no. The "something like so" is huge and utter crap,
> because the barrier is on the wrong side.
> 
>> -       old.lock_count = READ_ONCE(lockref->lock_count);                        \
>> +       barrier();                                                              \
>> +       old.lock_count = lockref->lock_count;                                   \
>>         while (likely(arch_spin_value_unlocked(old.lock.rlock.raw_lock))) {     \
>>                 struct lockref new = old, prev = old;                           \
> 
> The WHOLE point of the READ_ONCE (formerly ACCESS_ONCE) is that it
> tells the compiler that it cannot reload the value.
> 
> Notice how it is *not* about atomicitiy. The compiler can read the
> value in fifteen pieces, randomly mixing one bit or five. Nobody
> cares.
> 
> But the important thing is that ONCE IT IS READ, it is never read
> again. That's the "once" part.
> 
> Why is that important? It's important because we have to absolutely
> guartantee that the value we *test* is the same value we use later.
> That's a common concern with mutable variables, and is the only reason
> for READ_ONCE() in the first place.
> 
> The whole atomicity etc crap was just that - crap. It was never about
> atomicitiy. It was about the compiler not reloading values.
> 
> So no. No barriers. No "removal of READ_ONCE". Just get rid of the
> broken "sanity" checks in the READ_ONCE implementation that are just
> pure garbage.
> 
> The checks in ACCESS_ONCE() were because apparently gcc got things
> wrong - dropping the volatile - for aggregate types. They were never
> supposed to be about atomicity, even if there clearly was some
> confusion about that.
> 
> Really. Just get rid of the checks - they were wrong. They were
> clearly very close to *introducing* a bug, rather than fixing anything
> at all.
> 
>                              Linus
> 

Re: linux-next: build warnings after merge of the access_once tree

[Linus Torvalds]

On Thu, Mar 26, 2015 at 10:24 AM, Christian Borntraeger
<borntraeger@de.ibm.com> wrote:
>
> Oh I just added that check back then because some guy named
> Linus suggested something like that ;-)

Yes, my bad.

In my defense, that was when we were talking about ACCESS_ONCE()
causing bugs with gcc due to the blind use of "volatile" that it turns
out gcc doesn't necessarily like.

With the memcpy fallback (and the simpler "scalar pointer copy by
hand"), I think READ_ONCE() (and WRITE_ONCE()) are safe.

                      Linus

Re: linux-next: build warnings after merge of the access_once tree

[Christian Borntraeger]

Am 26.03.2015 um 18:52 schrieb Linus Torvalds:
> On Thu, Mar 26, 2015 at 10:24 AM, Christian Borntraeger
> <borntraeger@de.ibm.com> wrote:
>>
>> Oh I just added that check back then because some guy named
>> Linus suggested something like that ;-)
> 
> Yes, my bad.
> 
> In my defense, that was when we were talking about ACCESS_ONCE()
> causing bugs with gcc due to the blind use of "volatile" that it turns
> out gcc doesn't necessarily like.
> 
> With the memcpy fallback (and the simpler "scalar pointer copy by
> hand"), I think READ_ONCE() (and WRITE_ONCE()) are safe.
> 
>                       Linus
> 

FWIW, I dropped the warning fixp patch from my linux-next branch so
everything should be back to normal and we can merge Peters patch for 4.0 or
4.1.

Christian

Re: linux-next: build warnings after merge of the access_once tree

[Christian Borntraeger]

Am 26.03.2015 um 18:23 schrieb Christian Borntraeger:
> Am 26.03.2015 um 18:07 schrieb Peter Zijlstra:
>> On Thu, Mar 26, 2015 at 09:45:07AM -0700, Linus Torvalds wrote:
>>
>>> Stop this idiocy.
>>
>> Yeah, clearly I can type faster than I can think straight :/
>>
>>
>> In any case, I've the below patch; do you want to take it now or do you
>> want me to route it through tip/locking/urgent or something like that?
> 
> Its not urgent. Current upstream has a broken check (gcc will not emit the
> warning if the function is static). I just fixed the check in my next tree
> but I can certainly drop that tree.
> 

Thinking more about that, the removal of the ifdef for 64bit data might be
a reason to schedule that for 4.0.


>>
>> ---
>> Subject: kernel: Remove atomicy checks from {READ,WRITE}_ONCE
>> From: Peter Zijlstra <peterz@infradead.org>
>> Date: Thu, 26 Mar 2015 17:45:37 +0100
>>
>> The fact that volatile allows for atomic load/stores is a special case
>> not a requirement for {READ,WRITE}_ONCE(). Their primary purpose is to
>> force the compiler to emit load/stores _once_.
>>
>> So remove the warning as it is correct behaviour. This also implies that
>> the u64 case is not 64bit only, so remove the #ifdef so we can generate
>> better code in that case.
>>
>> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
>> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@elte.hu>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Cc: Davidlohr Bueso <dave@stgolabs.net>
>> Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
>> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> 
>> ---
>>  include/linux/compiler.h | 16 ----------------
>>  1 file changed, 16 deletions(-)
>>
>> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
>> index 1b45e4a0519b..0e41ca0e5927 100644
>> --- a/include/linux/compiler.h
>> +++ b/include/linux/compiler.h
>> @@ -192,29 +192,16 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>>
>>  #include <uapi/linux/types.h>
>>
>> -static __always_inline void data_access_exceeds_word_size(void)
>> -#ifdef __compiletime_warning
>> -__compiletime_warning("data access exceeds word size and won't be atomic")
>> -#endif
>> -;
>> -
>> -static __always_inline void data_access_exceeds_word_size(void)
>> -{
>> -}
>> -
>>  static __always_inline void __read_once_size(const volatile void *p, void *res, int size)
>>  {
>>  	switch (size) {
>>  	case 1: *(__u8 *)res = *(volatile __u8 *)p; break;
>>  	case 2: *(__u16 *)res = *(volatile __u16 *)p; break;
>>  	case 4: *(__u32 *)res = *(volatile __u32 *)p; break;
>> -#ifdef CONFIG_64BIT
>>  	case 8: *(__u64 *)res = *(volatile __u64 *)p; break;
>> -#endif
>>  	default:
>>  		barrier();
>>  		__builtin_memcpy((void *)res, (const void *)p, size);
>> -		data_access_exceeds_word_size();
>>  		barrier();
>>  	}
>>  }
>> @@ -225,13 +212,10 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
>>  	case 1: *(volatile __u8 *)p = *(__u8 *)res; break;
>>  	case 2: *(volatile __u16 *)p = *(__u16 *)res; break;
>>  	case 4: *(volatile __u32 *)p = *(__u32 *)res; break;
>> -#ifdef CONFIG_64BIT
>>  	case 8: *(volatile __u64 *)p = *(__u64 *)res; break;
>> -#endif
>>  	default:
>>  		barrier();
>>  		__builtin_memcpy((void *)p, (const void *)res, size);
>> -		data_access_exceeds_word_size();
>>  		barrier();
>>  	}
>>  }
>>
>