Re: [PATCH] advsync: Fix store-buffering sequence table

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Thu, Jul 06, 2017 at 09:09:05PM +0900, Akira Yokosawa wrote:
> On 2017/07/06 7:40 +0900, Akira Yokosawa wrote:
> > On 2017/07/06 7:32, Paul E. McKenney wrote:
> >> On Thu, Jul 06, 2017 at 07:15:24AM +0900, Akira Yokosawa wrote:
> >>> On 2017/07/05 10:32:49 -0700, Paul E. McKenney wrote:
> >>
> >> [ . . . ]
> >>
> >>>>>> So I cannot go with "volatile", but let's see if I can do something
> >>>>>> better than the gcc intrinsics.
> >>>>>
> >>>>> And if the litmus7 guys don't have anything for me, I can always write a
> >>>>> script to do the conversions.  So either way, we will have kernel-style
> >>>>> notation at some point.  ;-)
> >>>>
> >>>> And it turns out that the contents of a second "{}" in a litmus7 test
> >>>> is pulled directly into the output code, so an easy change.  ;-)
> >>>
> >>> Good news!
> >>>
> >>> So __atomic_thread_fence() will also be replaced with smp_mb() in
> >>> the litmus tests? That will reduce the width of the tables and eliminate
> >>> the need for the hspece adjustments in one-column layout.
> >>
> >> Oops, I forgot to push!  Done now.
> >>
> >> And yes, there is now smp_mb() in the litmus tests and the tables.
> > 
> > I'll take a look later.
> > As I mentioned earlier, __atomic_load_n() and __atomic_store_n() seem
> > to have fairly large overheads (on x86). You might want to see the changes
> > in the resulting statistics and reflect them in the text.
> 
> Hi Paul,
> 
> The definition of READ_ONCE() and WRITE_ONCE() given in the updated litmus
> tests is inconsistent with kernel API. They are accepting pointers such
> as x0 and x1.
> 
> In the article at LWN (https://lwn.net/Articles/720550/), litmus tests use the
> same definition as kernel API. They take the value expression such as *x0 and *x1.
> 
> My preference is to accept pointers as arguments as is the way in C functions,
> but the kernel community have chosen otherwise. We should be consist here.
> 
> So the definition should be:
> 
> ---
> #define READ_ONCE(x) __atomic_load_n(&(x), __ATOMIC_RELAXED)
> #define WRITE_ONCE(x, v) __atomic_store_n(&(x), (v), __ATOMIC_RELAXED)
> #define smp_mb() __atomic_thread_fence(__ATOMIC_SEQ_CST)
> ---
> 
> Or am I misreading?

No, you are correct!  I added a commit to fix this, and also added
a disclaimer as you suggested in your earlier email.

							Thanx, Paul