linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH 1/9] kernel: Provide READ_ONCE and ASSIGN_ONCE
@ 2014-12-05  2:12 George Spelvin
  2014-12-05  9:19 ` Christian Borntraeger
  0 siblings, 1 reply; 8+ messages in thread
From: George Spelvin @ 2014-12-05  2:12 UTC (permalink / raw)
  To: borntraeger; +Cc: linux, linux-arch, linux-kernel, paulmck, torvalds

> +#define READ_ONCE(p) \
> +	 typeof(p) __val; __read_once_size(&p, &__val, sizeof(__val)); __val; })
> +
> +#define ASSIGN_ONCE(val, p) \
> +	({ typeof(p) __val; __val = val; __assign_once_size(&p, &__val, sizeof(__val)); __val; })

Minor style nit: is it necessary to name a non-pointer variable "p"?
I expect typeof(p) to be a pointer type.

(The other fun style question, which is a lot less minor, is whether
ASSIGN_ONCE should be (src,dst) as above, or (dst,src) like = and
<string.h>.)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/9] kernel: Provide READ_ONCE and ASSIGN_ONCE
  2014-12-05  2:12 [PATCH 1/9] kernel: Provide READ_ONCE and ASSIGN_ONCE George Spelvin
@ 2014-12-05  9:19 ` Christian Borntraeger
  2014-12-05 16:00   ` George Spelvin
  0 siblings, 1 reply; 8+ messages in thread
From: Christian Borntraeger @ 2014-12-05  9:19 UTC (permalink / raw)
  To: George Spelvin; +Cc: linux-arch, linux-kernel, paulmck, torvalds

Am 05.12.2014 um 03:12 schrieb George Spelvin:
>> +#define READ_ONCE(p) \
>> +	 typeof(p) __val; __read_once_size(&p, &__val, sizeof(__val)); __val; })
>> +
>> +#define ASSIGN_ONCE(val, p) \
>> +	({ typeof(p) __val; __val = val; __assign_once_size(&p, &__val, sizeof(__val)); __val; })
> 
> Minor style nit: is it necessary to name a non-pointer variable "p"?
> I expect typeof(p) to be a pointer type.

v might be better.
> 
> (The other fun style question, which is a lot less minor, is whether
> ASSIGN_ONCE should be (src,dst) as above, or (dst,src) like = and
> <string.h>.)

I tend to prefer dst, src, but Linus used src, dst in his proposal - so I used that.




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/9] kernel: Provide READ_ONCE and ASSIGN_ONCE
  2014-12-05  9:19 ` Christian Borntraeger
@ 2014-12-05 16:00   ` George Spelvin
  0 siblings, 0 replies; 8+ messages in thread
From: George Spelvin @ 2014-12-05 16:00 UTC (permalink / raw)
  To: borntraeger, linux; +Cc: linux-arch, linux-kernel, paulmck, torvalds

>> Minor style nit: is it necessary to name a non-pointer variable "p"?
>> I expect typeof(p) to be a pointer type.

> v might be better.

The current ACCESS_ONCE uses x.  I also considered "var" and "mem".

>> (The other fun style question, which is a lot less minor, is whether
>> ASSIGN_ONCE should be (src,dst) as above, or (dst,src) like = and
>> <string.h>.)

> I tend to prefer dst, src, but Linus used src, dst in his proposal -
> so I used that.

The question is, does Linus actually care (hey, Linus, do you?), or was
that just a thoughtless part of a discussion about semantics?

Because if you prefer it too, there are arguments...
There are plenty of "store" operations with (src, dst) arguments,
because the focus is on the value being stored, so it comes first.

But the name "assign" almost always refers to the ":=" operation,
with the focus more on the destination.

(Now you have me thinking about German grammar and how the destination
can be identified by the dative "dem".  But even though German depends
on word order less than English, normally the dative comes first.)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/9] kernel: Provide READ_ONCE and ASSIGN_ONCE
       [not found] <CA+55aFzzEhbkoXnVGXAbq-HxejmWSyjMBN_aQM61J_zZLPXwAw@mail.gmail.com>
@ 2014-12-05 21:38 ` George Spelvin
  0 siblings, 0 replies; 8+ messages in thread
From: George Spelvin @ 2014-12-05 21:38 UTC (permalink / raw)
  To: borntraeger; +Cc: linux, linux-arch, linux-kernel, paulmck, torvalds

> Of prefer it to match the put_user model, which is (val, ptr). But as long
> as there is your safety (and the whole point of the macro is that it
> figures out the type from the pointer), I guess it doesn't matter too much
> in practice.
> 
> I think my original suggestion also wrote it in lower case, since it
> actually works like a function (well, template, whatever). Again kind of
> like the user copy "functions".
> 
> But I don't care *that* strongly.

That's an excellent point.  But it means that Christian, you're free to
do whatever seems good to you.

I'm not rabid about it, either; I just figure that now is a good time
to think carefully about it.

Personally, after running through some different names that work
with (src,dst) order I'd try the pairs:

	READ_ONCE(var) / WRITE_ONCE(value,var)
or
	LOAD_ONCE(var) / STORE_ONCE(value,var)

Of the two, I think I prefer the latter, because that's what the
operations are called at the hardware level, so it's most evocative of
the actual semantics.  "Assignment" is a higher-level concept.

But as I said, whatever you think best.  I'd just like to advocate for
some actual thinking, because it's going to be all over the kernel and
a pain to change.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/9] kernel: Provide READ_ONCE and ASSIGN_ONCE
  2014-12-04  9:24     ` Christian Borntraeger
@ 2014-12-04 14:41       ` Paul E. McKenney
  0 siblings, 0 replies; 8+ messages in thread
From: Paul E. McKenney @ 2014-12-04 14:41 UTC (permalink / raw)
  To: Christian Borntraeger; +Cc: linux-kernel, linux-arch, torvalds

On Thu, Dec 04, 2014 at 10:24:47AM +0100, Christian Borntraeger wrote:
> Am 04.12.2014 um 01:07 schrieb Paul E. McKenney:
> > On Wed, Dec 03, 2014 at 11:30:13PM +0100, Christian Borntraeger wrote:
> >> 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) step
> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145)
> >>
> >> Let's provide READ_ONCE/ASSIGN_ONCE that will do all accesses via
> >> scalar types as suggested by Linus Torvalds. Accesses larger than
> >> the machines word size cannot be guaranteed to be atomic. These
> >> macros will use memcpy and emit a build warning.
> >>
> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > 
> > With or without some nits below addressed:
> > 
> > Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > 
> >> ---
> >>  include/linux/compiler.h | 64 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 64 insertions(+)
> >>
> >> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> >> index d5ad7b1..947710e 100644
> >> --- a/include/linux/compiler.h
> >> +++ b/include/linux/compiler.h
> >> @@ -186,6 +186,70 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
> >>  # define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __LINE__)
> >>  #endif
> >>
> >> +#include <linux/types.h>
> >> +
> >> +void data_access_exceeds_word_size(void)
> >> +__compiletime_warning("data access exceeds word size and won't be atomic");
> >> +
> >> +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
> > 
> > You could get rid of the #ifdef above by doing "case sizeof(long)"
> > and switching the casts from u64 to unsigned long.
> 
> Wouldnt that cause a compile warning because we have two case statements
> with the same size?

Indeed it would!  Color me blind and oblivious.

> >> +	default:
> >> +		barrier();
> >> +		__builtin_memcpy((void *)res, (const void *)p, size);
> >> +		data_access_exceeds_word_size();
> >> +		barrier();
> >> +	}
> >> +}
> >> +
> >> +static __always_inline void __assign_once_size(volatile void *p, void *res, int size)
> >> +{
> >> +	switch (size) {
> >> +	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
> > 
> > Ditto.
> > 
> >> +	default:
> >> +		barrier();
> >> +		__builtin_memcpy((void *)p, (const void *)res, size);
> >> +		data_access_exceeds_word_size();
> >> +		barrier();
> >> +	}
> >> +}
> >> +
> >> +/*
> >> + * Prevent the compiler from merging or refetching reads or writes. The compiler
> >> + * is also forbidden from reordering successive instances of READ_ONCE,
> >> + * ASSIGN_ONCE and ACCESS_ONCE (see below), but only when the compiler is aware
> >> + * of some particular ordering.  One way to make the compiler aware of ordering
> >> + * is to put the two invocations of READ_ONCE, ASSIGN_ONCE or ACCESS_ONCE() in
> >> + * different C statements.
> >> + *
> >> + * In contrast to ACCESS_ONCE these two macros will also work on aggregate data
> >> + * types like structs or unions. If the size of the accessed data type exceeds
> >> + * the word size of the machine (e.g. 32bit or 64bit), the access might happen
> >> + * in multiple chunks, though.
> > 
> > This last sentence might be more clear if it was something like the
> > following:
> > 
> > 	If the size of the accessed data type exceeeds the word size of
> > 	the machine (e.g., 32 bits or 64 bits), ACCESS_ONCE() might
> > 	carry out the access in multiple chunks, but READ_ONCE() and
> > 	ASSIGN_ONCE() will give a link-time error.
> 
> The code in v4 now combines Linus (memcpy) and your (error) suggestion. We do a memcpy and a compile time _warning_
> 
> So I will do 
> 
>  * In contrast to ACCESS_ONCE these two macros will also work on aggregate
>  * data types like structs or unions. If the size of the accessed data
>  * type exceeds the word size of the machine (e.g., 32 bits or 64 bits)
>  * READ_ONCE() and ASSIGN_ONCE()  will fall back to memcpy and print a
>  * compile-time warning.

Very good!

							Thanx, Paul

> >> + *
> >> + * These macros do absolutely -nothing- to prevent the CPU from reordering,
> >> + * merging, or refetching absolutely anything at any time.  Their main intended
> >> + * use is to mediate communication between process-level code and irq/NMI
> >> + * handlers, all running on the same CPU.
> > 
> > This last sentence is now obsolete.  How about something like this?
> > 
> > 	Their two major use cases are: (1) Mediating communication
> > 	between process-level code and irq/NMI handlers, all running
> > 	on the same CPU, and (2) Ensuring that the compiler does not
> > 	fold, spindle, or otherwise mutilate accesses that either do
> > 	not require ordering or that interact with an explicit memory
> > 	barrier or atomic instruction that provides the required ordering.
> 
> sounds good. Will change.
> 
> Thanks
> 
> > 
> >> + */
> >> +
> >> +#define READ_ONCE(p) \
> >> +      ({ typeof(p) __val; __read_once_size(&p, &__val, sizeof(__val)); __val; })
> >> +
> >> +#define ASSIGN_ONCE(val, p) \
> >> +      ({ typeof(p) __val; __val = val; __assign_once_size(&p, &__val, sizeof(__val)); __val; })
> >> +
> >>  #endif /* __KERNEL__ */
> >>
> >>  #endif /* __ASSEMBLY__ */
> >> -- 
> >> 1.9.3
> >>
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/9] kernel: Provide READ_ONCE and ASSIGN_ONCE
  2014-12-04  0:07   ` Paul E. McKenney
@ 2014-12-04  9:24     ` Christian Borntraeger
  2014-12-04 14:41       ` Paul E. McKenney
  0 siblings, 1 reply; 8+ messages in thread
From: Christian Borntraeger @ 2014-12-04  9:24 UTC (permalink / raw)
  To: paulmck; +Cc: linux-kernel, linux-arch, torvalds

Am 04.12.2014 um 01:07 schrieb Paul E. McKenney:
> On Wed, Dec 03, 2014 at 11:30:13PM +0100, Christian Borntraeger wrote:
>> 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) step
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145)
>>
>> Let's provide READ_ONCE/ASSIGN_ONCE that will do all accesses via
>> scalar types as suggested by Linus Torvalds. Accesses larger than
>> the machines word size cannot be guaranteed to be atomic. These
>> macros will use memcpy and emit a build warning.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> With or without some nits below addressed:
> 
> Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
>> ---
>>  include/linux/compiler.h | 64 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 64 insertions(+)
>>
>> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
>> index d5ad7b1..947710e 100644
>> --- a/include/linux/compiler.h
>> +++ b/include/linux/compiler.h
>> @@ -186,6 +186,70 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>>  # define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __LINE__)
>>  #endif
>>
>> +#include <linux/types.h>
>> +
>> +void data_access_exceeds_word_size(void)
>> +__compiletime_warning("data access exceeds word size and won't be atomic");
>> +
>> +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
> 
> You could get rid of the #ifdef above by doing "case sizeof(long)"
> and switching the casts from u64 to unsigned long.

Wouldnt that cause a compile warning because we have two case statements
with the same size?

> 
>> +	default:
>> +		barrier();
>> +		__builtin_memcpy((void *)res, (const void *)p, size);
>> +		data_access_exceeds_word_size();
>> +		barrier();
>> +	}
>> +}
>> +
>> +static __always_inline void __assign_once_size(volatile void *p, void *res, int size)
>> +{
>> +	switch (size) {
>> +	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
> 
> Ditto.
> 
>> +	default:
>> +		barrier();
>> +		__builtin_memcpy((void *)p, (const void *)res, size);
>> +		data_access_exceeds_word_size();
>> +		barrier();
>> +	}
>> +}
>> +
>> +/*
>> + * Prevent the compiler from merging or refetching reads or writes. The compiler
>> + * is also forbidden from reordering successive instances of READ_ONCE,
>> + * ASSIGN_ONCE and ACCESS_ONCE (see below), but only when the compiler is aware
>> + * of some particular ordering.  One way to make the compiler aware of ordering
>> + * is to put the two invocations of READ_ONCE, ASSIGN_ONCE or ACCESS_ONCE() in
>> + * different C statements.
>> + *
>> + * In contrast to ACCESS_ONCE these two macros will also work on aggregate data
>> + * types like structs or unions. If the size of the accessed data type exceeds
>> + * the word size of the machine (e.g. 32bit or 64bit), the access might happen
>> + * in multiple chunks, though.
> 
> This last sentence might be more clear if it was something like the
> following:
> 
> 	If the size of the accessed data type exceeeds the word size of
> 	the machine (e.g., 32 bits or 64 bits), ACCESS_ONCE() might
> 	carry out the access in multiple chunks, but READ_ONCE() and
> 	ASSIGN_ONCE() will give a link-time error.

The code in v4 now combines Linus (memcpy) and your (error) suggestion. We do a memcpy and a compile time _warning_

So I will do 

 * In contrast to ACCESS_ONCE these two macros will also work on aggregate
 * data types like structs or unions. If the size of the accessed data
 * type exceeds the word size of the machine (e.g., 32 bits or 64 bits)
 * READ_ONCE() and ASSIGN_ONCE()  will fall back to memcpy and print a
 * compile-time warning.



> 
>> + *
>> + * These macros do absolutely -nothing- to prevent the CPU from reordering,
>> + * merging, or refetching absolutely anything at any time.  Their main intended
>> + * use is to mediate communication between process-level code and irq/NMI
>> + * handlers, all running on the same CPU.
> 
> This last sentence is now obsolete.  How about something like this?
> 
> 	Their two major use cases are: (1) Mediating communication
> 	between process-level code and irq/NMI handlers, all running
> 	on the same CPU, and (2) Ensuring that the compiler does not
> 	fold, spindle, or otherwise mutilate accesses that either do
> 	not require ordering or that interact with an explicit memory
> 	barrier or atomic instruction that provides the required ordering.

sounds good. Will change.

Thanks

> 
>> + */
>> +
>> +#define READ_ONCE(p) \
>> +      ({ typeof(p) __val; __read_once_size(&p, &__val, sizeof(__val)); __val; })
>> +
>> +#define ASSIGN_ONCE(val, p) \
>> +      ({ typeof(p) __val; __val = val; __assign_once_size(&p, &__val, sizeof(__val)); __val; })
>> +
>>  #endif /* __KERNEL__ */
>>
>>  #endif /* __ASSEMBLY__ */
>> -- 
>> 1.9.3
>>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/9] kernel: Provide READ_ONCE and ASSIGN_ONCE
  2014-12-03 22:30 ` [PATCH 1/9] kernel: Provide READ_ONCE and ASSIGN_ONCE Christian Borntraeger
@ 2014-12-04  0:07   ` Paul E. McKenney
  2014-12-04  9:24     ` Christian Borntraeger
  0 siblings, 1 reply; 8+ messages in thread
From: Paul E. McKenney @ 2014-12-04  0:07 UTC (permalink / raw)
  To: Christian Borntraeger; +Cc: linux-kernel, linux-arch, torvalds

On Wed, Dec 03, 2014 at 11:30:13PM +0100, Christian Borntraeger wrote:
> 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) step
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145)
> 
> Let's provide READ_ONCE/ASSIGN_ONCE that will do all accesses via
> scalar types as suggested by Linus Torvalds. Accesses larger than
> the machines word size cannot be guaranteed to be atomic. These
> macros will use memcpy and emit a build warning.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

With or without some nits below addressed:

Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  include/linux/compiler.h | 64 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index d5ad7b1..947710e 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -186,6 +186,70 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>  # define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __LINE__)
>  #endif
> 
> +#include <linux/types.h>
> +
> +void data_access_exceeds_word_size(void)
> +__compiletime_warning("data access exceeds word size and won't be atomic");
> +
> +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

You could get rid of the #ifdef above by doing "case sizeof(long)"
and switching the casts from u64 to unsigned long.

> +	default:
> +		barrier();
> +		__builtin_memcpy((void *)res, (const void *)p, size);
> +		data_access_exceeds_word_size();
> +		barrier();
> +	}
> +}
> +
> +static __always_inline void __assign_once_size(volatile void *p, void *res, int size)
> +{
> +	switch (size) {
> +	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

Ditto.

> +	default:
> +		barrier();
> +		__builtin_memcpy((void *)p, (const void *)res, size);
> +		data_access_exceeds_word_size();
> +		barrier();
> +	}
> +}
> +
> +/*
> + * Prevent the compiler from merging or refetching reads or writes. The compiler
> + * is also forbidden from reordering successive instances of READ_ONCE,
> + * ASSIGN_ONCE and ACCESS_ONCE (see below), but only when the compiler is aware
> + * of some particular ordering.  One way to make the compiler aware of ordering
> + * is to put the two invocations of READ_ONCE, ASSIGN_ONCE or ACCESS_ONCE() in
> + * different C statements.
> + *
> + * In contrast to ACCESS_ONCE these two macros will also work on aggregate data
> + * types like structs or unions. If the size of the accessed data type exceeds
> + * the word size of the machine (e.g. 32bit or 64bit), the access might happen
> + * in multiple chunks, though.

This last sentence might be more clear if it was something like the
following:

	If the size of the accessed data type exceeeds the word size of
	the machine (e.g., 32 bits or 64 bits), ACCESS_ONCE() might
	carry out the access in multiple chunks, but READ_ONCE() and
	ASSIGN_ONCE() will give a link-time error.

> + *
> + * These macros do absolutely -nothing- to prevent the CPU from reordering,
> + * merging, or refetching absolutely anything at any time.  Their main intended
> + * use is to mediate communication between process-level code and irq/NMI
> + * handlers, all running on the same CPU.

This last sentence is now obsolete.  How about something like this?

	Their two major use cases are: (1) Mediating communication
	between process-level code and irq/NMI handlers, all running
	on the same CPU, and (2) Ensuring that the compiler does not
	fold, spindle, or otherwise mutilate accesses that either do
	not require ordering or that interact with an explicit memory
	barrier or atomic instruction that provides the required ordering.

> + */
> +
> +#define READ_ONCE(p) \
> +      ({ typeof(p) __val; __read_once_size(&p, &__val, sizeof(__val)); __val; })
> +
> +#define ASSIGN_ONCE(val, p) \
> +      ({ typeof(p) __val; __val = val; __assign_once_size(&p, &__val, sizeof(__val)); __val; })
> +
>  #endif /* __KERNEL__ */
> 
>  #endif /* __ASSEMBLY__ */
> -- 
> 1.9.3
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/9] kernel: Provide READ_ONCE and ASSIGN_ONCE
  2014-12-03 22:30 [PATCHv4 0/9] ACCESS_ONCE and non-scalar accesses Christian Borntraeger
@ 2014-12-03 22:30 ` Christian Borntraeger
  2014-12-04  0:07   ` Paul E. McKenney
  0 siblings, 1 reply; 8+ messages in thread
From: Christian Borntraeger @ 2014-12-03 22:30 UTC (permalink / raw)
  To: linux-kernel; +Cc: linux-arch, paulmck, torvalds, 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) step
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145)

Let's provide READ_ONCE/ASSIGN_ONCE that will do all accesses via
scalar types as suggested by Linus Torvalds. Accesses larger than
the machines word size cannot be guaranteed to be atomic. These
macros will use memcpy and emit a build warning.

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

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index d5ad7b1..947710e 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -186,6 +186,70 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
 # define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __LINE__)
 #endif
 
+#include <linux/types.h>
+
+void data_access_exceeds_word_size(void)
+__compiletime_warning("data access exceeds word size and won't be atomic");
+
+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
+	default:
+		barrier();
+		__builtin_memcpy((void *)res, (const void *)p, size);
+		data_access_exceeds_word_size();
+		barrier();
+	}
+}
+
+static __always_inline void __assign_once_size(volatile void *p, void *res, int size)
+{
+	switch (size) {
+	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();
+	}
+}
+
+/*
+ * Prevent the compiler from merging or refetching reads or writes. The compiler
+ * is also forbidden from reordering successive instances of READ_ONCE,
+ * ASSIGN_ONCE and ACCESS_ONCE (see below), but only when the compiler is aware
+ * of some particular ordering.  One way to make the compiler aware of ordering
+ * is to put the two invocations of READ_ONCE, ASSIGN_ONCE or ACCESS_ONCE() in
+ * different C statements.
+ *
+ * In contrast to ACCESS_ONCE these two macros will also work on aggregate data
+ * types like structs or unions. If the size of the accessed data type exceeds
+ * the word size of the machine (e.g. 32bit or 64bit), the access might happen
+ * in multiple chunks, though.
+ *
+ * These macros do absolutely -nothing- to prevent the CPU from reordering,
+ * merging, or refetching absolutely anything at any time.  Their main intended
+ * use is to mediate communication between process-level code and irq/NMI
+ * handlers, all running on the same CPU.
+ */
+
+#define READ_ONCE(p) \
+      ({ typeof(p) __val; __read_once_size(&p, &__val, sizeof(__val)); __val; })
+
+#define ASSIGN_ONCE(val, p) \
+      ({ typeof(p) __val; __val = val; __assign_once_size(&p, &__val, sizeof(__val)); __val; })
+
 #endif /* __KERNEL__ */
 
 #endif /* __ASSEMBLY__ */
-- 
1.9.3


^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2014-12-05 21:38 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-05  2:12 [PATCH 1/9] kernel: Provide READ_ONCE and ASSIGN_ONCE George Spelvin
2014-12-05  9:19 ` Christian Borntraeger
2014-12-05 16:00   ` George Spelvin
     [not found] <CA+55aFzzEhbkoXnVGXAbq-HxejmWSyjMBN_aQM61J_zZLPXwAw@mail.gmail.com>
2014-12-05 21:38 ` George Spelvin
  -- strict thread matches above, loose matches on Subject: below --
2014-12-03 22:30 [PATCHv4 0/9] ACCESS_ONCE and non-scalar accesses Christian Borntraeger
2014-12-03 22:30 ` [PATCH 1/9] kernel: Provide READ_ONCE and ASSIGN_ONCE Christian Borntraeger
2014-12-04  0:07   ` Paul E. McKenney
2014-12-04  9:24     ` Christian Borntraeger
2014-12-04 14:41       ` Paul E. McKenney

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).