Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Alexei Starovoitov]

On Mon, Nov 24, 2014 at 12:34 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Mon, Nov 24, 2014 at 12:04 PM, David Howells <dhowells@redhat.com> wrote:
>>
>> Reserve ACCESS_ONCE() for reading and add an ASSIGN_ONCE() or something like
>> that for writing?
>
> I wouldn't mind that. We've had situations where reading and writing
> isn't really similar - like alpha where reading a byte is atomic, but
> writing one isn't.
>
> Then we could also make it have the "get_user()/put_user()" kind of
> semantics - .and then use the same "sizeopf()" tricks that we use for
> get_user/put_user.
>
> That would actually work around the gcc bug a completely different way:
>
>   #define ACCESS_ONCE(p) \
>       ({ typeof(*p) __val; __read_once_size(p, &__val, sizeof(__val)); __val; })
>
> and then we can do things like this:
>
>   static __always_inline void __read_once_size(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
>        }
>   }
>
> and same for ASSIGN_ONCE(val, p).
>
> That also hopefully avoids the whole "oops, gcc has a bug", because
> the actual volatile access is always done using a scalar type, even if
> the type of "__val" may in fact be a structure.

I've changed gcc pr58145-1.c reproducer to use
__read_once_size() approach above, but it
didn't help to workaround the bug.
gcc 'sra' pass still lost 'volatile' modifier.

modified reproducer:
struct S { unsigned int data; };
void bar(int val)
{
  struct S _s = { .data = val };
  *(volatile struct S *) 0x880000UL = ACCESS_ONCE(&_s);
}
assembler code looks as expected, but
internal gcc state after 'sra' pass is
missing volatile...

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Linus Torvalds]

On Mon, Nov 24, 2014 at 2:58 PM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> I've changed gcc pr58145-1.c reproducer to use
> __read_once_size() approach above

I don't think you did.

> modified reproducer:
> struct S { unsigned int data; };
> void bar(int val)
> {
>   struct S _s = { .data = val };
>   *(volatile struct S *) 0x880000UL = ACCESS_ONCE(&_s);
> }

My approach never had "volatile struct S *". The only volatile
pointers were the actual byte/word/etc pointers, and those generated
temporary values that were then assigned to the final type through a
cast.

Also, note that the kernel is compiled without strict aliasing, so the
casting to 'void *' and various smaller types is "safe" - even if the
C standard doesn't like it.

With strict aliasing, you'd need to make the read_once() macro not
just pass in the size, there would have to be some kind of union of
the type, and that would effectively mean that you can't use an inline
function, you'd have to do it in a big macro (because the type would
be per-site).

So with strict aliasing, you'd have to make it something like

        #define ACCESS_ONCE(p) \
      ({ union { typeof(*p) __val; char __array[sizeof(*p)]} __u;
__read_once_size(p, __u.__array, sizeof(__u)); __u.__val; })

Pretty? No. But then, the standard C aliasing rules are so broken that
"pretty" doesn't really come into play..

                  Linus

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Alexei Starovoitov]

On Mon, Nov 24, 2014 at 4:00 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Mon, Nov 24, 2014 at 2:58 PM, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>>
>> I've changed gcc pr58145-1.c reproducer to use
>> __read_once_size() approach above
>
> I don't think you did.
>
>> modified reproducer:
>> struct S { unsigned int data; };
>> void bar(int val)
>> {
>>   struct S _s = { .data = val };
>>   *(volatile struct S *) 0x880000UL = ACCESS_ONCE(&_s);
>> }
>
> My approach never had "volatile struct S *". The only volatile
> pointers were the actual byte/word/etc pointers, and those generated

you're right. In my invalid snippet above the ACCESS_ONCE
to struct on stack gets optimized away and only 'volatile struct *'
in left hand side is triggering the bug.

Have tried the following which blends your proposal
with original code from Christian:
/* bad
#define ACCESS_ONCE(x) *((volatile typeof(x) *)&(x))
*/

/* good */
#define ACCESS_ONCE(p) \
      ({ typeof(*p) __val; __read_once_size(p, &__val, sizeof(__val)); __val; })

static __always_inline void __read_once_size(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;
     case 8: *(u64 *)res = *(volatile u64 *)p; break;
     }
}

union ipte_control {
        unsigned long val;
        struct {
                unsigned long k  : 1;
                unsigned long kh : 31;
                unsigned long kg : 32;
        };
};

struct kvm_vcpu {
        union ipte_control ic;
};

void ipte_unlock_siif(struct kvm_vcpu *vcpu)
{
        union ipte_control old, new, *ic;

        ic = &vcpu->ic;
        do {
                new = old = ACCESS_ONCE(ic);
                new.kh--;
                if (!new.kh)
                        new.k = 0;
        } while (cmpxchg(&ic->val, old.val, new.val) != old.val);
}

generated code looks correct with and without strict-aliasing
and volatile marking is preserved properly.
(to check for volatile marks add -fdump-tree-optimized
 and look for {v} in *.optimized)

> Pretty? No. But then, the standard C aliasing rules are so broken that
> "pretty" doesn't really come into play..

Agree. I don't see any warnings or code generation issues with and
without strict-aliasing with your original __read_once_size(), so no need
to play union tricks. Initially I was worried that extra always_inline
function will make generated code worse in critical paths where
ACCESS_ONCE is used, but after looking close enough, it seems
all should be fine.

Note, with unmodified ACCESS_ONCE all architectures (even x64)
are missing volatile markings with gcc 4.6.3, 4.7.2 for Christian's
use case.

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Christian Borntraeger]

Am 24.11.2014 um 22:02 schrieb Linus Torvalds:
> On Mon, Nov 24, 2014 at 12:53 PM, Christian Borntraeger
> <borntraeger@de.ibm.com> wrote:
>>
>> That looks like a lot of changes all over ACCESS_ONCE -> ASSIGN_ONCE:
>> git grep "ACCESS_ONCE.*=.*"
>> gives me 200 placea not in Documentation.
> 
> Yeah, that's a bit annoying.
> 
> How about a combination of the two:
> 
>  - accept the fact that right now ACCESS_ONCE() is fairly widespread
> (even for writing)
> 
>  - but also admit that we'd be better off with a nicer interface
> 
> and make the solution be:
> 
>  - make ACCESS_ONCE() only work on scalars, and deprecate it
> 
>  - add new "read_once()" and "write_once()" interfaces that *do* work
> on (appropriately sized) structures and unions, and start migrating
> things over. In particular, start with the ones that can no longer use
> ACCESS_ONCE() because they aren't scalar..
> 
> That second point would make the conversion patches actually easier to
> read. Instead of this:
> 
>  static inline int arch_spin_is_locked(arch_spinlock_t *lock)
>  {
> -       struct __raw_tickets tmp = ACCESS_ONCE(lock->tickets);
> +       arch_spinlock_t tmp = {};
> 
> -       return tmp.tail != tmp.head;
> +       tmp.head_tail =ACCESS_ONCE(lock->head_tail);
> +       return tmp.tickets.tail != tmp.tickets.head;
>  }
> 
> which isn't *complex*, but is also not an obvious conversion, we'd have just
> 
>  static inline int arch_spin_is_locked(arch_spinlock_t *lock)
>  {
> -       struct __raw_tickets tmp = ACCESS_ONCE(lock->tickets);
> -       struct __raw_tickets tmp = read_once(lock->tickets);
> 
>         return tmp.tail != tmp.head;
>  }
> 
> which is a much simpler and more obvious change.
> 
> And then we could slowly try to migrate existing ACCESS_ONCE() users
> over (particularly writers).
> 
> Hmm? Too much?

I will give it a try. I will start with Alexei's version for ACCESS_ONCE and your snippets to build read_once and write_once. The only open question is, what to do with the "too large" accesses. Pauls initial patch showed several 
places, e.g. kernel/sched/fair.c accessing an u64 even on 32bit:
[...]
   age_stamp = ACCESS_ONCE(rq->age_stamp);
        avg = ACCESS_ONCE(rq->rt_avg);
[...]

I think I will simply not touch those...



Christian

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Linus Torvalds]

On Mon, Nov 24, 2014 at 12:53 PM, Christian Borntraeger
<borntraeger@de.ibm.com> wrote:
>
> That looks like a lot of changes all over ACCESS_ONCE -> ASSIGN_ONCE:
> git grep "ACCESS_ONCE.*=.*"
> gives me 200 placea not in Documentation.

Yeah, that's a bit annoying.

How about a combination of the two:

 - accept the fact that right now ACCESS_ONCE() is fairly widespread
(even for writing)

 - but also admit that we'd be better off with a nicer interface

and make the solution be:

 - make ACCESS_ONCE() only work on scalars, and deprecate it

 - add new "read_once()" and "write_once()" interfaces that *do* work
on (appropriately sized) structures and unions, and start migrating
things over. In particular, start with the ones that can no longer use
ACCESS_ONCE() because they aren't scalar..

That second point would make the conversion patches actually easier to
read. Instead of this:

 static inline int arch_spin_is_locked(arch_spinlock_t *lock)
 {
-       struct __raw_tickets tmp = ACCESS_ONCE(lock->tickets);
+       arch_spinlock_t tmp = {};

-       return tmp.tail != tmp.head;
+       tmp.head_tail =ACCESS_ONCE(lock->head_tail);
+       return tmp.tickets.tail != tmp.tickets.head;
 }

which isn't *complex*, but is also not an obvious conversion, we'd have just

 static inline int arch_spin_is_locked(arch_spinlock_t *lock)
 {
-       struct __raw_tickets tmp = ACCESS_ONCE(lock->tickets);
-       struct __raw_tickets tmp = read_once(lock->tickets);

        return tmp.tail != tmp.head;
 }

which is a much simpler and more obvious change.

And then we could slowly try to migrate existing ACCESS_ONCE() users
over (particularly writers).

Hmm? Too much?

                     Linus

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Christian Borntraeger]

Am 24.11.2014 um 21:34 schrieb Linus Torvalds:
> On Mon, Nov 24, 2014 at 12:04 PM, David Howells <dhowells@redhat.com> wrote:
>>
>> Reserve ACCESS_ONCE() for reading and add an ASSIGN_ONCE() or something like
>> that for writing?
> 
> I wouldn't mind that. We've had situations where reading and writing
> isn't really similar - like alpha where reading a byte is atomic, but
> writing one isn't.
> 
> Then we could also make it have the "get_user()/put_user()" kind of
> semantics - .and then use the same "sizeopf()" tricks that we use for
> get_user/put_user.
> 
> That would actually work around the gcc bug a completely different way:
> 
>   #define ACCESS_ONCE(p) \
>       ({ typeof(*p) __val; __read_once_size(p, &__val, sizeof(__val)); __val; })
> 
> and then we can do things like this:
> 
>   static __always_inline void __read_once_size(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
>        }
>   }
> 
> and same for ASSIGN_ONCE(val, p).
> 
> That also hopefully avoids the whole "oops, gcc has a bug", because
> the actual volatile access is always done using a scalar type, even if
> the type of "__val" may in fact be a structure.
> 
> Christian, how painful would that be? Sorry to try to make you do a
> totally different approach..

That looks like a lot of changes all over ACCESS_ONCE -> ASSIGN_ONCE:
git grep "ACCESS_ONCE.*=.*" 
gives me 200 placea not in Documentation.

Then there is still the 64bit accesses on 32bit via ACCESS_ONCE problem, which we could detect with a default cause in your code. We would need to audit and fix all places :-/


So the last proposal from Alexei, seems easier (for me at least :-) )

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Paul E. McKenney]

On Mon, Nov 24, 2014 at 12:29:07PM -0800, Alexei Starovoitov wrote:
> On Mon, Nov 24, 2014 at 11:07 AM, Christian Borntraeger
> <borntraeger@de.ibm.com> wrote:
> >
> > Anyone with a new propopal? ;-)                                        ^
> 
> one more proposal :)
> #define __ACCESS_ONCE(x) ({ typeof(x) __var = 0; (volatile typeof(x) *)&(x); })
> #define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))
> 
> works as lvalue...
> the basic idea is the same:
> constant zero can be used to initialize any scalar
> (including pointers), but unions and structs will fail to compile as:
> "error: invalid initializer"
> 
> If I'm reading pr58145 gcc bug report correctly, it
> miscompiles only structs, so we can let ACCESS_ONCE
> to work on unions. Then the following will rejects structs only:
> #define __ACCESS_ONCE(x) ({ (typeof(x))0; (volatile typeof(x) *)&(x); })
> #define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))

You beat me to it.  ;-)

However, your approach would allow an ACCESS_ONCE() of a long long
to compile on 32-bit systems where it has to be broken up into a
pair of 32-bit accesses.

							Thanx, Paul

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Paul E. McKenney]

On Mon, Nov 24, 2014 at 12:19:11PM -0800, Linus Torvalds wrote:
> On Mon, Nov 24, 2014 at 11:42 AM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> >
> > OK, how about the following?
> 
> Ugh. Disgusting.
> 
> Why the heck isn't it just "sizeof(*__vp) <= sizeof(long)"?
> 
> If the architecture has a 3-byte scalar type, then it probably has a
> 3-byte load.

Because I was allowing for the possibility of a 3-byte struct, which
as you point out below...

> > It complains if the variable is too large, for example, long long on
> > 32-bit systems or large structures.  It is OK loading from and storing
> > to small structures as well, which I am having a hard time thinking of
> > as a disadvantage.
> 
> .. but that's *exactly* the gcc bug in question. It's a word-sized
> struct that gcc loads twice.

...was a stupid thought anyway.

OK, how about the attempt below?  The initialization of __p complains
for structures and unions, but gets optimized out.

							Thanx, Paul

-------------------------------------------------------------------------

#define get_scalar_volatile_pointer(x) ({ \
	typeof(x) __maybe_unused __p = 0; \
	volatile typeof(x) *__vp = &(x); \
	BUILD_BUG_ON(sizeof(*__vp) > sizeof(long)); \
	__vp; })
#define ACCESS_ONCE(x) (*get_scalar_volatile_pointer(x))

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Christian Borntraeger]

Am 24.11.2014 um 21:29 schrieb Alexei Starovoitov:
> On Mon, Nov 24, 2014 at 11:07 AM, Christian Borntraeger
> <borntraeger@de.ibm.com> wrote:
>>
>> Anyone with a new propopal? ;-)                                        ^
> 
> one more proposal :)
> #define __ACCESS_ONCE(x) ({ typeof(x) __var = 0; (volatile typeof(x) *)&(x); })
> #define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))

This seems to work. I only had to add an __always_unused to __var.


> 
> works as lvalue...
> the basic idea is the same:
> constant zero can be used to initialize any scalar
> (including pointers), but unions and structs will fail to compile as:
> "error: invalid initializer"
> 
> If I'm reading pr58145 gcc bug report correctly, it
> miscompiles only structs, so we can let ACCESS_ONCE
> to work on unions. Then the following will rejects structs only:
> #define __ACCESS_ONCE(x) ({ (typeof(x))0; (volatile typeof(x) *)&(x); })
> #define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))
> --
> To unsubscribe from this list: send the line "unsubscribe linux-s390" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Linus Torvalds]

On Mon, Nov 24, 2014 at 12:04 PM, David Howells <dhowells@redhat.com> wrote:
>
> Reserve ACCESS_ONCE() for reading and add an ASSIGN_ONCE() or something like
> that for writing?

I wouldn't mind that. We've had situations where reading and writing
isn't really similar - like alpha where reading a byte is atomic, but
writing one isn't.

Then we could also make it have the "get_user()/put_user()" kind of
semantics - .and then use the same "sizeopf()" tricks that we use for
get_user/put_user.

That would actually work around the gcc bug a completely different way:

  #define ACCESS_ONCE(p) \
      ({ typeof(*p) __val; __read_once_size(p, &__val, sizeof(__val)); __val; })

and then we can do things like this:

  static __always_inline void __read_once_size(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
       }
  }

and same for ASSIGN_ONCE(val, p).

That also hopefully avoids the whole "oops, gcc has a bug", because
the actual volatile access is always done using a scalar type, even if
the type of "__val" may in fact be a structure.

Christian, how painful would that be? Sorry to try to make you do a
totally different approach..

                  Linus

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Alexei Starovoitov]

On Mon, Nov 24, 2014 at 11:07 AM, Christian Borntraeger
<borntraeger@de.ibm.com> wrote:
>
> Anyone with a new propopal? ;-)                                        ^

one more proposal :)
#define __ACCESS_ONCE(x) ({ typeof(x) __var = 0; (volatile typeof(x) *)&(x); })
#define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))

works as lvalue...
the basic idea is the same:
constant zero can be used to initialize any scalar
(including pointers), but unions and structs will fail to compile as:
"error: invalid initializer"

If I'm reading pr58145 gcc bug report correctly, it
miscompiles only structs, so we can let ACCESS_ONCE
to work on unions. Then the following will rejects structs only:
#define __ACCESS_ONCE(x) ({ (typeof(x))0; (volatile typeof(x) *)&(x); })
#define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Christian Borntraeger]

Am 24.11.2014 um 20:42 schrieb Paul E. McKenney:
> On Mon, Nov 24, 2014 at 11:14:42AM -0800, Linus Torvalds wrote:
>> On Mon, Nov 24, 2014 at 11:07 AM, Christian Borntraeger
>> <borntraeger@de.ibm.com> wrote:
>>>
>>> Looks really nice, but does not work with ACCESS_ONCE is on the left-hand side:
>>
>> Oh, I forgot about that. And that was indeed why I had done that whole
>> helper macro originally, with ACCESS_ONCE() itself just being the
>> dereference of the pointer.
> 
> OK, how about the following?
> 
> It complains if the variable is too large, for example, long long on
> 32-bit systems or large structures.  It is OK loading from and storing
> to small structures as well, which I am having a hard time thinking of
> as a disadvantage.

Well, the motivation for this series was that gcc  4.6 and 4.7 might ignore volatile for
such a case, see the original thread and this data structure

union ipte_control {
        unsigned long val;
        struct {
                unsigned long k  : 1;
                unsigned long kh : 31;
                unsigned long kg : 32;
        };
};

> ------------------------------------------------------------------------
> 
> #define get_scalar_volatile_pointer(x) ({ \
> 	volatile typeof(x) *__vp = &(x); \
> 	BUILD_BUG_ON(sizeof(*__vp) != sizeof(char) && \
> 		     sizeof(*__vp) != sizeof(short) && \
> 		     sizeof(*__vp) != sizeof(int) && \
> 		     sizeof(*__vp) != sizeof(long)); \
> 	__vp; })
> #define ACCESS_ONCE(x) (*get_scalar_volatile_pointer(x))
> 

This gives also several compiler errors when accessing u64 on a 32bit system. This is expected, but more widespread than expected - ouch.

Christian

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Linus Torvalds]

On Mon, Nov 24, 2014 at 11:42 AM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
>
> OK, how about the following?

Ugh. Disgusting.

Why the heck isn't it just "sizeof(*__vp) <= sizeof(long)"?

If the architecture has a 3-byte scalar type, then it probably has a
3-byte load.

> It complains if the variable is too large, for example, long long on
> 32-bit systems or large structures.  It is OK loading from and storing
> to small structures as well, which I am having a hard time thinking of
> as a disadvantage.

.. but that's *exactly* the gcc bug in question. It's a word-sized
struct that gcc loads twice.

                          Linus

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[David Howells]

Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> Looks really nice, but does not work with ACCESS_ONCE is on the left-hand side:
> 
> 
> include/linux/rculist.h: In function 'hlist_add_before_rcu':
> ./arch/x86/include/asm/barrier.h:127:18: error: lvalue required as left operand of assignment
>   ACCESS_ONCE(*p) = (v);      \
> 
> Alexei's variant is also broken:
> 
> include/linux/cgroup.h: In function 'task_css':
> include/linux/compiler.h:381:40: error: invalid operands to binary + (have 'struct css_set *' and 'struct css_set * volatile')
>  #define ACCESS_ONCE(x) (((typeof(x))0) + *(volatile typeof(x) *)&(x))
> 
> Anyone with a new propopal? ;-)                                        ^

Reserve ACCESS_ONCE() for reading and add an ASSIGN_ONCE() or something like
that for writing?

David

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Paul E. McKenney]

On Mon, Nov 24, 2014 at 11:14:42AM -0800, Linus Torvalds wrote:
> On Mon, Nov 24, 2014 at 11:07 AM, Christian Borntraeger
> <borntraeger@de.ibm.com> wrote:
> >
> > Looks really nice, but does not work with ACCESS_ONCE is on the left-hand side:
> 
> Oh, I forgot about that. And that was indeed why I had done that whole
> helper macro originally, with ACCESS_ONCE() itself just being the
> dereference of the pointer.

OK, how about the following?

It complains if the variable is too large, for example, long long on
32-bit systems or large structures.  It is OK loading from and storing
to small structures as well, which I am having a hard time thinking of
as a disadvantage.

							Thanx, Paul

------------------------------------------------------------------------

#define get_scalar_volatile_pointer(x) ({ \
	volatile typeof(x) *__vp = &(x); \
	BUILD_BUG_ON(sizeof(*__vp) != sizeof(char) && \
		     sizeof(*__vp) != sizeof(short) && \
		     sizeof(*__vp) != sizeof(int) && \
		     sizeof(*__vp) != sizeof(long)); \
	__vp; })
#define ACCESS_ONCE(x) (*get_scalar_volatile_pointer(x))

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Linus Torvalds]

On Mon, Nov 24, 2014 at 11:07 AM, Christian Borntraeger
<borntraeger@de.ibm.com> wrote:
>
> Looks really nice, but does not work with ACCESS_ONCE is on the left-hand side:

Oh, I forgot about that. And that was indeed why I had done that whole
helper macro originally, with ACCESS_ONCE() itself just being the
dereference of the pointer.

Duh.

                  Linus

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Christian Borntraeger]

Am 24.11.2014 um 19:35 schrieb Linus Torvalds:
> On Mon, Nov 24, 2014 at 10:02 AM, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>>
>> If the goal is to catch non-scalar users, the following is shorter:
>> #define ACCESS_ONCE(x) (((typeof(x))0) + *(volatile typeof(x) *)&(x))
> 
> Me likey. It probably works well in practice, although I think
> 
>  - the "(typeof(x))0)" seems unnecessary and wrong. Why not just "0"?
> The typeof is not just longer, but it is incorrect for pointer types
> (you can add 0 to a pointer, but you cannot add two pointers together)
> 
>  - it does mean that the resulting type ends up being upgraded to
> "int", for the usual C type reasons.
> 
> Note that the "upgraded to 'int'" is true with or without the
> "(typeof(x))0". If you add two 'char' values, the addition is still
> done in 'int'.
> 
> Maybe you *meant* that typeof to fix the second problem, like so:
> 
>   (typeof(x)) (0 + *(volatile typeof(x) *)&(x))
> 
> Hmm? That casts the result of the addition, not the zero.

Looks really nice, but does not work with ACCESS_ONCE is on the left-hand side:


include/linux/rculist.h: In function 'hlist_add_before_rcu':
./arch/x86/include/asm/barrier.h:127:18: error: lvalue required as left operand of assignment
  ACCESS_ONCE(*p) = (v);      \

Alexei's variant is also broken:

include/linux/cgroup.h: In function 'task_css':
include/linux/compiler.h:381:40: error: invalid operands to binary + (have 'struct css_set *' and 'struct css_set * volatile')
 #define ACCESS_ONCE(x) (((typeof(x))0) + *(volatile typeof(x) *)&(x))

Anyone with a new propopal? ;-)                                        ^

Christian

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Linus Torvalds]

On Mon, Nov 24, 2014 at 10:02 AM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> If the goal is to catch non-scalar users, the following is shorter:
> #define ACCESS_ONCE(x) (((typeof(x))0) + *(volatile typeof(x) *)&(x))

Me likey. It probably works well in practice, although I think

 - the "(typeof(x))0)" seems unnecessary and wrong. Why not just "0"?
The typeof is not just longer, but it is incorrect for pointer types
(you can add 0 to a pointer, but you cannot add two pointers together)

 - it does mean that the resulting type ends up being upgraded to
"int", for the usual C type reasons.

Note that the "upgraded to 'int'" is true with or without the
"(typeof(x))0". If you add two 'char' values, the addition is still
done in 'int'.

Maybe you *meant* that typeof to fix the second problem, like so:

  (typeof(x)) (0 + *(volatile typeof(x) *)&(x))

Hmm? That casts the result of the addition, not the zero.

             Linus

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Alexei Starovoitov]

On Mon, Nov 24, 2014 at 5:30 AM, David Howells <dhowells@redhat.com> wrote:
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>
>> +#define get_scalar_volatile_pointer(x) ({ \
>> +     typeof(x) *__p = &(x); \
>> +     volatile typeof(x) *__vp = __p; \
>> +     (void)(long)*__p; __vp; })
>> +#define ACCESS_ONCE(x) (*get_scalar_volatile_pointer(x))

If the goal is to catch non-scalar users, the following is shorter:
#define ACCESS_ONCE(x) (((typeof(x))0) + *(volatile typeof(x) *)&(x))

it will block union and struct accesses.
If you want to allow union and disallow struct, then replace + with ,

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Linus Torvalds]

On Mon, Nov 24, 2014 at 5:30 AM, David Howells <dhowells@redhat.com> wrote:
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>
>> +#define get_scalar_volatile_pointer(x) ({ \
>> +     typeof(x) *__p = &(x); \
>> +     volatile typeof(x) *__vp = __p; \
>> +     (void)(long)*__p; __vp; })
>> +#define ACCESS_ONCE(x) (*get_scalar_volatile_pointer(x))
>
> Might this cause two loads from memory under some conditions?  Once for the
> fourth line and once for the fifth?

Hmm. Since the fourth line isn't volatile, I'd expect gcc to optimize
it away. But you're right, it might be safer to make sure the "test
type" part is something like

   (void)sizeof((long)*__p)

instead - the "sizeof()" means that it will never actually get
evaluated, but the type-checking of casting *__p to (long) will still
catch the case of *__p being some non-scalar type that cannot be cast.

                   Linus

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[David Howells]

Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> +#define get_scalar_volatile_pointer(x) ({ \
> +	typeof(x) *__p = &(x); \
> +	volatile typeof(x) *__vp = __p; \
> +	(void)(long)*__p; __vp; })
> +#define ACCESS_ONCE(x) (*get_scalar_volatile_pointer(x))

Might this cause two loads from memory under some conditions?  Once for the
fourth line and once for the fifth?

(Apologies if this has already been discussed)

David

[PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Christian Borntraeger]

ACCESS_ONCE does not work reliably on non-scalar types. For
example gcc 4.6 and 4.7 might remove the volatile tag for such
accesses during the SRA (scalar replacement of aggregates) steps.
see  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145

This patch is based on an initial proof-of-concept from Linus
Torvalds that causes compile errors for such accesses.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 include/linux/compiler.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index d5ad7b1..8a92c93 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -377,8 +377,18 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
  * merging, or refetching absolutely anything at any time.  Its main intended
  * use is to mediate communication between process-level code and irq/NMI
  * handlers, all running on the same CPU.
+ *
+ * ACCESS_ONCE will only work on scalar types. For union types, ACCESS_ONCE
+ * on a union member will work as long as the size of the member matches the
+ * size of the union and the size is smaller than word size. See the x86
+ * spinlock implementation as an example. For other cases like page table
+ * structures, a barrier might be a good alternative.
  */
-#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
+#define get_scalar_volatile_pointer(x) ({ \
+	typeof(x) *__p = &(x); \
+	volatile typeof(x) *__vp = __p; \
+	(void)(long)*__p; __vp; })
+#define ACCESS_ONCE(x) (*get_scalar_volatile_pointer(x))
 
 /* Ignore/forbid kprobes attach on very low level functions marked by this attribute: */
 #ifdef CONFIG_KPROBES
-- 
1.9.3