Date:	Tue, 13 Jan 2015 09:47:55 -0800
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Christian Borntraeger <borntraeger@...ibm.com>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	Davidlohr Bueso <dave@...olabs.net>,
	linux-kernel@...r.kernel.org, mingo@...nel.org,
	laijs@...fujitsu.com, dipankar@...ibm.com,
	akpm@...ux-foundation.org, mathieu.desnoyers@...icios.com,
	josh@...htriplett.org, tglx@...utronix.de, rostedt@...dmis.org,
	dhowells@...hat.com, edumazet@...gle.com, dvhart@...ux.intel.com,
	fweisbec@...il.com, oleg@...hat.com,
	Pranith Kumar <bobby.prani@...il.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH tip/core/rcu 01/14] rcu: Protect rcu_boost() lockless
 accesses with ACCESS_ONCE()

On Tue, Jan 13, 2015 at 09:18:47AM +0100, Christian Borntraeger wrote:
> Am 12.01.2015 um 23:12 schrieb Paul E. McKenney:
> > On Mon, Jan 12, 2015 at 09:59:57AM +0100, Peter Zijlstra wrote:
> >> On Fri, Jan 09, 2015 at 10:58:50PM +0100, Christian Borntraeger wrote:
> >>> Am 09.01.2015 um 14:56 schrieb Peter Zijlstra:
> >>>> On Fri, Jan 09, 2015 at 05:49:54AM -0800, Paul E. McKenney wrote:
> >>>>>> That reminds me, I think the new conversion for stores will most likely
> >>>>>> introduce silly arg bugs:
> >>>>>>
> >>>>>> -       ACCESS_ONCE(a) = b;
> >>>>>> +       ASSIGN_ONCE(b, a);
> >>>>>
> >>>>> I was planning to do mine by hand for this sort of reason.
> >>>>>
> >>>>> Or are you thinking of something more subtle than the case where
> >>>>> "b" is an unparenthesized comma-separated expression?
> >>>>
> >>>> I think he's revering to the wrong way around-ness of the thing.
> >>>>
> >>>> Its a bit of a mixed bag on assignments, but for instance
> >>>> rcu_assign_pointer() takes them the right way around, as does
> >>>> atomic_set().
> >>>>
> >>>> So yes, I think the ASSIGN_ONCE() thing got the arguments the wrong way
> >>>> around.
> >>>>
> >>>> We could maybe still change it, before its in too long ?
> >>>
> >>> Linus initial proposal was inspired by put_user model which is (val,
> >>> ptr) and I took that. 
> >>
> >> Yeah, like I said, its a bit of a mixed bag. We've got plenty examples
> >> of the wrong way around.
> >>
> >>> As my focus was on avoiding the volatile bug,
> >>> all my current conversions are READ_ONCE as no potential ASSIGN_ONCE
> >>> user was done on a non-scalar type, so I have no first hand
> >>> experience. 
> >>
> >> So the implication there is that we'd preserve ACCESS_ONCE() for use on
> >> scalar types. I don't think we should do that, I think we should just
> >> en-mass convert to {READ,WRITE}/{LOAD,STORE}_ONCE() and kill off
> >> ACCESS_ONCE().
> > 
> > Yep.  For one thing, the proposed replacements work much better with
> > C11 than does ACCESS_ONCE().
> 
> As we agreed there is no perfect interface regarding val,x vs. x,val.
> But it seems that there is some consensus that I should push something like the following (still whitespace damaged) to Linus for 3.19?
> Peter, Davidlohr, Paul (maybe Linus) can you ACK/NACK?
> 
> 
> Subject: Change ASSIGN_ONCE(val, x) to WRITE_ONCE(x, val)
> 
> Feedback has shown that WRITE_ONCE(x, val) is easier to use than ASSIGN_ONCE(val,x).
> There are no in-tree users yet, so lets change it.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@...ibm.com>

Acked-by: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>

> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 84734a7..38865c7 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -215,7 +215,7 @@ static __always_inline void __read_once_size(volatile void *p, void *res, int si
>         }
>  }
> 
> -static __always_inline void __assign_once_size(volatile void *p, void *res, int size)
> +static __always_inline void __write_once_size(volatile void *p, void *res, int size)
>  {
>         switch (size) {
>         case 1: *(volatile __u8 *)p = *(__u8 *)res; break;
> @@ -235,15 +235,15 @@ static __always_inline void __assign_once_size(volatile void *p, void *res, int
>  /*
>   * 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
> + * READ_ONCE, WRITE_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.
> + * WRITE_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., 32 bits or 64 bits)
> - * READ_ONCE() and ASSIGN_ONCE()  will fall back to memcpy and print a
> + * READ_ONCE() and WRITE_ONCE()  will fall back to memcpy and print a
>   * compile-time warning.
>   *
>   * Their two major use cases are: (1) Mediating communication between
> @@ -257,8 +257,8 @@ static __always_inline void __assign_once_size(volatile void *p, void *res, int
>  #define READ_ONCE(x) \
>         ({ typeof(x) __val; __read_once_size(&x, &__val, sizeof(__val)); __val; })
> 
> -#define ASSIGN_ONCE(val, x) \
> -       ({ typeof(x) __val; __val = val; __assign_once_size(&x, &__val, sizeof(__val)); __val; })
> +#define WRITE_ONCE(x, val) \
> +       ({ typeof(x) __val; __val = val; __write_once_size(&x, &__val, sizeof(__val)); __val; })
> 
>  #endif /* __KERNEL__ */
> 
> @@ -458,7 +458,7 @@ static __always_inline void __assign_once_size(volatile void *p, void *res, int
>   * with an explicit memory barrier or atomic instruction that provides the
>   * required ordering.
>   *
> - * If possible use READ_ONCE/ASSIGN_ONCE instead.
> + * If possible use READ_ONCE/WRITE_ONCE instead.
>   */
>  #define __ACCESS_ONCE(x) ({ \
>          __maybe_unused typeof(x) __var = (typeof(x)) 0; 
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists