Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Paul E. McKenney]

On Mon, Jan 23, 2017 at 07:12:03PM +1100, Michael Ellerman wrote:
> "Paul E. McKenney"  writes:
> 
> > On Sat, Jan 14, 2017 at 11:54:17AM -0800, Paul E. McKenney wrote:
> >> On Sat, Jan 14, 2017 at 10:35:50AM +0100, Ingo Molnar wrote:
> >> > * Paul E. McKenney  wrote:
> >
> > [ . . . ]
> >
> >> > > + */
> >> > > +#ifdef CONFIG_PPC
> >> > > +#define smp_mb__after_unlock_lock()   smp_mb()  /* Full ordering for 
> >> > > lock. */
> >> > > +#else /* #ifdef CONFIG_PPC */
> >> > > +#define smp_mb__after_unlock_lock()   do { } while (0)
> >> > > +#endif /* #else #ifdef CONFIG_PPC */
> >> > 
> >> > Yeah, so I realize that this was pre-existing code, but putting 
> >> > CONFIG_$ARCH
> >> > #ifdefs into generic headers is generally frowned upon.
> >> > 
> >> > The canonical approach would be either to define a helper Kconfig 
> >> > variable that 
> >> > can be set by PPC (but other architectures don't need to set it), or to 
> >> > expose a 
> >> > suitable macro (function) for architectures to define in their barrier.h 
> >> > arch 
> >> > header file.
> >> 
> >> Very well, I will add a separate commit for this.  4.11 OK?
> >
> > Does the patch below seem reasonable?
> >
> > Thanx, Paul
> >
> > 
> >
> > commit 271c0601237c41a279f975563e13837bace0df03
> > Author: Paul E. McKenney 
> > Date:   Sat Jan 14 13:32:50 2017 -0800
> >
> > rcu: Make arch select smp_mb__after_unlock_lock() strength
> > 
> > The definition of smp_mb__after_unlock_lock() is currently smp_mb()
> > for CONFIG_PPC and a no-op otherwise.  It would be better to instead
> > provide an architecture-selectable Kconfig option, and select the
> > strength of smp_mb__after_unlock_lock() based on that option.  This
> > commit therefore creates CONFIG_ARCH_WEAK_RELACQ, has PPC select it,
> > and bases the definition of smp_mb__after_unlock_lock() on this new
> > CONFIG_ARCH_WEAK_RELACQ Kconfig option.
> > 
> > Reported-by: Ingo Molnar 
> > Signed-off-by: Paul E. McKenney 
> > Cc: Peter Zijlstra 
> > Cc: Will Deacon 
> > Cc: Boqun Feng 
> > Cc: Benjamin Herrenschmidt 
> > Cc: Paul Mackerras 
> > Cc: Michael Ellerman 
> > Cc: 
> 
> Personally I'd call it ARCH_WEAK_RELEASE_ACQUIRE, which is longer but
> clearer I think. But it's not a big deal, so which ever you prefer.

ARCH_WEAK_RELEASE_ACQUIRE it is!

> Acked-by: Michael Ellerman 

Applied, thank you!

Thanx, Paul

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Paul E. McKenney]

On Mon, Jan 23, 2017 at 07:12:03PM +1100, Michael Ellerman wrote:
> "Paul E. McKenney"  writes:
> 
> > On Sat, Jan 14, 2017 at 11:54:17AM -0800, Paul E. McKenney wrote:
> >> On Sat, Jan 14, 2017 at 10:35:50AM +0100, Ingo Molnar wrote:
> >> > * Paul E. McKenney  wrote:
> >
> > [ . . . ]
> >
> >> > > + */
> >> > > +#ifdef CONFIG_PPC
> >> > > +#define smp_mb__after_unlock_lock()   smp_mb()  /* Full ordering for 
> >> > > lock. */
> >> > > +#else /* #ifdef CONFIG_PPC */
> >> > > +#define smp_mb__after_unlock_lock()   do { } while (0)
> >> > > +#endif /* #else #ifdef CONFIG_PPC */
> >> > 
> >> > Yeah, so I realize that this was pre-existing code, but putting 
> >> > CONFIG_$ARCH
> >> > #ifdefs into generic headers is generally frowned upon.
> >> > 
> >> > The canonical approach would be either to define a helper Kconfig 
> >> > variable that 
> >> > can be set by PPC (but other architectures don't need to set it), or to 
> >> > expose a 
> >> > suitable macro (function) for architectures to define in their barrier.h 
> >> > arch 
> >> > header file.
> >> 
> >> Very well, I will add a separate commit for this.  4.11 OK?
> >
> > Does the patch below seem reasonable?
> >
> > Thanx, Paul
> >
> > 
> >
> > commit 271c0601237c41a279f975563e13837bace0df03
> > Author: Paul E. McKenney 
> > Date:   Sat Jan 14 13:32:50 2017 -0800
> >
> > rcu: Make arch select smp_mb__after_unlock_lock() strength
> > 
> > The definition of smp_mb__after_unlock_lock() is currently smp_mb()
> > for CONFIG_PPC and a no-op otherwise.  It would be better to instead
> > provide an architecture-selectable Kconfig option, and select the
> > strength of smp_mb__after_unlock_lock() based on that option.  This
> > commit therefore creates CONFIG_ARCH_WEAK_RELACQ, has PPC select it,
> > and bases the definition of smp_mb__after_unlock_lock() on this new
> > CONFIG_ARCH_WEAK_RELACQ Kconfig option.
> > 
> > Reported-by: Ingo Molnar 
> > Signed-off-by: Paul E. McKenney 
> > Cc: Peter Zijlstra 
> > Cc: Will Deacon 
> > Cc: Boqun Feng 
> > Cc: Benjamin Herrenschmidt 
> > Cc: Paul Mackerras 
> > Cc: Michael Ellerman 
> > Cc: 
> 
> Personally I'd call it ARCH_WEAK_RELEASE_ACQUIRE, which is longer but
> clearer I think. But it's not a big deal, so which ever you prefer.

ARCH_WEAK_RELEASE_ACQUIRE it is!

> Acked-by: Michael Ellerman 

Applied, thank you!

Thanx, Paul

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Michael Ellerman]

"Paul E. McKenney"  writes:

> On Sat, Jan 14, 2017 at 11:54:17AM -0800, Paul E. McKenney wrote:
>> On Sat, Jan 14, 2017 at 10:35:50AM +0100, Ingo Molnar wrote:
>> > * Paul E. McKenney  wrote:
>
> [ . . . ]
>
>> > > + */
>> > > +#ifdef CONFIG_PPC
>> > > +#define smp_mb__after_unlock_lock() smp_mb()  /* Full ordering for 
>> > > lock. */
>> > > +#else /* #ifdef CONFIG_PPC */
>> > > +#define smp_mb__after_unlock_lock() do { } while (0)
>> > > +#endif /* #else #ifdef CONFIG_PPC */
>> > 
>> > Yeah, so I realize that this was pre-existing code, but putting 
>> > CONFIG_$ARCH
>> > #ifdefs into generic headers is generally frowned upon.
>> > 
>> > The canonical approach would be either to define a helper Kconfig variable 
>> > that 
>> > can be set by PPC (but other architectures don't need to set it), or to 
>> > expose a 
>> > suitable macro (function) for architectures to define in their barrier.h 
>> > arch 
>> > header file.
>> 
>> Very well, I will add a separate commit for this.  4.11 OK?
>
> Does the patch below seem reasonable?
>
>   Thanx, Paul
>
> 
>
> commit 271c0601237c41a279f975563e13837bace0df03
> Author: Paul E. McKenney 
> Date:   Sat Jan 14 13:32:50 2017 -0800
>
> rcu: Make arch select smp_mb__after_unlock_lock() strength
> 
> The definition of smp_mb__after_unlock_lock() is currently smp_mb()
> for CONFIG_PPC and a no-op otherwise.  It would be better to instead
> provide an architecture-selectable Kconfig option, and select the
> strength of smp_mb__after_unlock_lock() based on that option.  This
> commit therefore creates CONFIG_ARCH_WEAK_RELACQ, has PPC select it,
> and bases the definition of smp_mb__after_unlock_lock() on this new
> CONFIG_ARCH_WEAK_RELACQ Kconfig option.
> 
> Reported-by: Ingo Molnar 
> Signed-off-by: Paul E. McKenney 
> Cc: Peter Zijlstra 
> Cc: Will Deacon 
> Cc: Boqun Feng 
> Cc: Benjamin Herrenschmidt 
> Cc: Paul Mackerras 
> Cc: Michael Ellerman 
> Cc: 

Personally I'd call it ARCH_WEAK_RELEASE_ACQUIRE, which is longer but
clearer I think. But it's not a big deal, so which ever you prefer.

Acked-by: Michael Ellerman 

cheers

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Michael Ellerman]

"Paul E. McKenney"  writes:

> On Sat, Jan 14, 2017 at 11:54:17AM -0800, Paul E. McKenney wrote:
>> On Sat, Jan 14, 2017 at 10:35:50AM +0100, Ingo Molnar wrote:
>> > * Paul E. McKenney  wrote:
>
> [ . . . ]
>
>> > > + */
>> > > +#ifdef CONFIG_PPC
>> > > +#define smp_mb__after_unlock_lock() smp_mb()  /* Full ordering for 
>> > > lock. */
>> > > +#else /* #ifdef CONFIG_PPC */
>> > > +#define smp_mb__after_unlock_lock() do { } while (0)
>> > > +#endif /* #else #ifdef CONFIG_PPC */
>> > 
>> > Yeah, so I realize that this was pre-existing code, but putting 
>> > CONFIG_$ARCH
>> > #ifdefs into generic headers is generally frowned upon.
>> > 
>> > The canonical approach would be either to define a helper Kconfig variable 
>> > that 
>> > can be set by PPC (but other architectures don't need to set it), or to 
>> > expose a 
>> > suitable macro (function) for architectures to define in their barrier.h 
>> > arch 
>> > header file.
>> 
>> Very well, I will add a separate commit for this.  4.11 OK?
>
> Does the patch below seem reasonable?
>
>   Thanx, Paul
>
> 
>
> commit 271c0601237c41a279f975563e13837bace0df03
> Author: Paul E. McKenney 
> Date:   Sat Jan 14 13:32:50 2017 -0800
>
> rcu: Make arch select smp_mb__after_unlock_lock() strength
> 
> The definition of smp_mb__after_unlock_lock() is currently smp_mb()
> for CONFIG_PPC and a no-op otherwise.  It would be better to instead
> provide an architecture-selectable Kconfig option, and select the
> strength of smp_mb__after_unlock_lock() based on that option.  This
> commit therefore creates CONFIG_ARCH_WEAK_RELACQ, has PPC select it,
> and bases the definition of smp_mb__after_unlock_lock() on this new
> CONFIG_ARCH_WEAK_RELACQ Kconfig option.
> 
> Reported-by: Ingo Molnar 
> Signed-off-by: Paul E. McKenney 
> Cc: Peter Zijlstra 
> Cc: Will Deacon 
> Cc: Boqun Feng 
> Cc: Benjamin Herrenschmidt 
> Cc: Paul Mackerras 
> Cc: Michael Ellerman 
> Cc: 

Personally I'd call it ARCH_WEAK_RELEASE_ACQUIRE, which is longer but
clearer I think. But it's not a big deal, so which ever you prefer.

Acked-by: Michael Ellerman 

cheers

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Ingo Molnar]

* Paul E. McKenney  wrote:

> On Sun, Jan 15, 2017 at 10:40:58AM +0100, Ingo Molnar wrote:
> > 
> > * Paul E. McKenney  wrote:
> > 
> > > > [sounds of rummaging around in the Git tree]
> > > > 
> > > > I found this commit of yours from ancient history (more than a year 
> > > > ago!):
> > > > 
> > > >   commit 12d560f4ea87030667438a169912380be00cea4b
> > > >   Author: Paul E. McKenney 
> > > >   Date:   Tue Jul 14 18:35:23 2015 -0700
> > > > 
> > > > rcu,locking: Privatize smp_mb__after_unlock_lock()
> > > > 
> > > > RCU is the only thing that uses smp_mb__after_unlock_lock(), and is
> > > > likely the only thing that ever will use it, so this commit makes 
> > > > this
> > > > macro private to RCU.
> > > > 
> > > > Signed-off-by: Paul E. McKenney 
> > > > Cc: Will Deacon 
> > > > Cc: Peter Zijlstra 
> > > > Cc: Benjamin Herrenschmidt 
> > > > Cc: "linux-a...@vger.kernel.org" 
> > > > 
> > > > So I concur and I'm fine with your patch - or with the status quo code 
> > > > as well.
> > > 
> > > I already have the patch queued, so how about I keep it if I get an ack
> > > from the powerpc guys and drop it otherwise?
> > 
> > Yeah, sounds good! Your patch made me look up 'RelAcq' so it has 
> > documentation 
> > value as well ;-)
> 
> ;-) ;-) ;-)
> 
> Looking forward, my guess would be that if some other code needs
> smp_mb__after_unlock_lock() or if some other architecture needs
> non-smb_mb() special handling, I should consider making it work the
> same as smp_mb__after_atomic() and friends.  Does that seem like a
> reasonable thought?

Yeah, absolutely - it's just that the pattern triggered the 'this looks a bit 
too 
specialized' response in me, but after seeing the details (again ...) I agree 
that 
this time is different!

Thanks,

Ingo

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Ingo Molnar]

* Paul E. McKenney  wrote:

> On Sun, Jan 15, 2017 at 10:40:58AM +0100, Ingo Molnar wrote:
> > 
> > * Paul E. McKenney  wrote:
> > 
> > > > [sounds of rummaging around in the Git tree]
> > > > 
> > > > I found this commit of yours from ancient history (more than a year 
> > > > ago!):
> > > > 
> > > >   commit 12d560f4ea87030667438a169912380be00cea4b
> > > >   Author: Paul E. McKenney 
> > > >   Date:   Tue Jul 14 18:35:23 2015 -0700
> > > > 
> > > > rcu,locking: Privatize smp_mb__after_unlock_lock()
> > > > 
> > > > RCU is the only thing that uses smp_mb__after_unlock_lock(), and is
> > > > likely the only thing that ever will use it, so this commit makes 
> > > > this
> > > > macro private to RCU.
> > > > 
> > > > Signed-off-by: Paul E. McKenney 
> > > > Cc: Will Deacon 
> > > > Cc: Peter Zijlstra 
> > > > Cc: Benjamin Herrenschmidt 
> > > > Cc: "linux-a...@vger.kernel.org" 
> > > > 
> > > > So I concur and I'm fine with your patch - or with the status quo code 
> > > > as well.
> > > 
> > > I already have the patch queued, so how about I keep it if I get an ack
> > > from the powerpc guys and drop it otherwise?
> > 
> > Yeah, sounds good! Your patch made me look up 'RelAcq' so it has 
> > documentation 
> > value as well ;-)
> 
> ;-) ;-) ;-)
> 
> Looking forward, my guess would be that if some other code needs
> smp_mb__after_unlock_lock() or if some other architecture needs
> non-smb_mb() special handling, I should consider making it work the
> same as smp_mb__after_atomic() and friends.  Does that seem like a
> reasonable thought?

Yeah, absolutely - it's just that the pattern triggered the 'this looks a bit 
too 
specialized' response in me, but after seeing the details (again ...) I agree 
that 
this time is different!

Thanks,

Ingo

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Paul E. McKenney]

On Sun, Jan 15, 2017 at 10:40:58AM +0100, Ingo Molnar wrote:
> 
> * Paul E. McKenney  wrote:
> 
> > > [sounds of rummaging around in the Git tree]
> > > 
> > > I found this commit of yours from ancient history (more than a year ago!):
> > > 
> > >   commit 12d560f4ea87030667438a169912380be00cea4b
> > >   Author: Paul E. McKenney 
> > >   Date:   Tue Jul 14 18:35:23 2015 -0700
> > > 
> > > rcu,locking: Privatize smp_mb__after_unlock_lock()
> > > 
> > > RCU is the only thing that uses smp_mb__after_unlock_lock(), and is
> > > likely the only thing that ever will use it, so this commit makes this
> > > macro private to RCU.
> > > 
> > > Signed-off-by: Paul E. McKenney 
> > > Cc: Will Deacon 
> > > Cc: Peter Zijlstra 
> > > Cc: Benjamin Herrenschmidt 
> > > Cc: "linux-a...@vger.kernel.org" 
> > > 
> > > So I concur and I'm fine with your patch - or with the status quo code as 
> > > well.
> > 
> > I already have the patch queued, so how about I keep it if I get an ack
> > from the powerpc guys and drop it otherwise?
> 
> Yeah, sounds good! Your patch made me look up 'RelAcq' so it has 
> documentation 
> value as well ;-)

;-) ;-) ;-)

Looking forward, my guess would be that if some other code needs
smp_mb__after_unlock_lock() or if some other architecture needs
non-smb_mb() special handling, I should consider making it work the
same as smp_mb__after_atomic() and friends.  Does that seem like a
reasonable thought?

Thanx, Paul

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Paul E. McKenney]

On Sun, Jan 15, 2017 at 10:40:58AM +0100, Ingo Molnar wrote:
> 
> * Paul E. McKenney  wrote:
> 
> > > [sounds of rummaging around in the Git tree]
> > > 
> > > I found this commit of yours from ancient history (more than a year ago!):
> > > 
> > >   commit 12d560f4ea87030667438a169912380be00cea4b
> > >   Author: Paul E. McKenney 
> > >   Date:   Tue Jul 14 18:35:23 2015 -0700
> > > 
> > > rcu,locking: Privatize smp_mb__after_unlock_lock()
> > > 
> > > RCU is the only thing that uses smp_mb__after_unlock_lock(), and is
> > > likely the only thing that ever will use it, so this commit makes this
> > > macro private to RCU.
> > > 
> > > Signed-off-by: Paul E. McKenney 
> > > Cc: Will Deacon 
> > > Cc: Peter Zijlstra 
> > > Cc: Benjamin Herrenschmidt 
> > > Cc: "linux-a...@vger.kernel.org" 
> > > 
> > > So I concur and I'm fine with your patch - or with the status quo code as 
> > > well.
> > 
> > I already have the patch queued, so how about I keep it if I get an ack
> > from the powerpc guys and drop it otherwise?
> 
> Yeah, sounds good! Your patch made me look up 'RelAcq' so it has 
> documentation 
> value as well ;-)

;-) ;-) ;-)

Looking forward, my guess would be that if some other code needs
smp_mb__after_unlock_lock() or if some other architecture needs
non-smb_mb() special handling, I should consider making it work the
same as smp_mb__after_atomic() and friends.  Does that seem like a
reasonable thought?

Thanx, Paul

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Ingo Molnar]

* Paul E. McKenney  wrote:

> > [sounds of rummaging around in the Git tree]
> > 
> > I found this commit of yours from ancient history (more than a year ago!):
> > 
> >   commit 12d560f4ea87030667438a169912380be00cea4b
> >   Author: Paul E. McKenney 
> >   Date:   Tue Jul 14 18:35:23 2015 -0700
> > 
> > rcu,locking: Privatize smp_mb__after_unlock_lock()
> > 
> > RCU is the only thing that uses smp_mb__after_unlock_lock(), and is
> > likely the only thing that ever will use it, so this commit makes this
> > macro private to RCU.
> > 
> > Signed-off-by: Paul E. McKenney 
> > Cc: Will Deacon 
> > Cc: Peter Zijlstra 
> > Cc: Benjamin Herrenschmidt 
> > Cc: "linux-a...@vger.kernel.org" 
> > 
> > So I concur and I'm fine with your patch - or with the status quo code as 
> > well.
> 
> I already have the patch queued, so how about I keep it if I get an ack
> from the powerpc guys and drop it otherwise?

Yeah, sounds good! Your patch made me look up 'RelAcq' so it has documentation 
value as well ;-)

Thanks,

Ingo

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Ingo Molnar]

* Paul E. McKenney  wrote:

> > [sounds of rummaging around in the Git tree]
> > 
> > I found this commit of yours from ancient history (more than a year ago!):
> > 
> >   commit 12d560f4ea87030667438a169912380be00cea4b
> >   Author: Paul E. McKenney 
> >   Date:   Tue Jul 14 18:35:23 2015 -0700
> > 
> > rcu,locking: Privatize smp_mb__after_unlock_lock()
> > 
> > RCU is the only thing that uses smp_mb__after_unlock_lock(), and is
> > likely the only thing that ever will use it, so this commit makes this
> > macro private to RCU.
> > 
> > Signed-off-by: Paul E. McKenney 
> > Cc: Will Deacon 
> > Cc: Peter Zijlstra 
> > Cc: Benjamin Herrenschmidt 
> > Cc: "linux-a...@vger.kernel.org" 
> > 
> > So I concur and I'm fine with your patch - or with the status quo code as 
> > well.
> 
> I already have the patch queued, so how about I keep it if I get an ack
> from the powerpc guys and drop it otherwise?

Yeah, sounds good! Your patch made me look up 'RelAcq' so it has documentation 
value as well ;-)

Thanks,

Ingo

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Paul E. McKenney]

On Sun, Jan 15, 2017 at 08:57:11AM +0100, Ingo Molnar wrote:
> 
> * Paul E. McKenney  wrote:
> 
> > On Sun, Jan 15, 2017 at 08:11:23AM +0100, Ingo Molnar wrote:
> > > 
> > > * Paul E. McKenney  wrote:
> > > 
> > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > > index 357b32aaea48..5fdfe874229e 100644
> > > > --- a/include/linux/rcupdate.h
> > > > +++ b/include/linux/rcupdate.h
> > > > @@ -1175,11 +1175,11 @@ do { \
> > > >   * if the UNLOCK and LOCK are executed by the same CPU or if the
> > > >   * UNLOCK and LOCK operate on the same lock variable.
> > > >   */
> > > > -#ifdef CONFIG_PPC
> > > > +#ifdef CONFIG_ARCH_WEAK_RELACQ
> > > >  #define smp_mb__after_unlock_lock()smp_mb()  /* Full ordering for 
> > > > lock. */
> > > > -#else /* #ifdef CONFIG_PPC */
> > > > +#else /* #ifdef CONFIG_ARCH_WEAK_RELACQ */
> > > >  #define smp_mb__after_unlock_lock()do { } while (0)
> > > > -#endif /* #else #ifdef CONFIG_PPC */
> > > > +#endif /* #else #ifdef CONFIG_ARCH_WEAK_RELACQ */
> > > >  
> > > >  
> > > 
> > > So at the risk of sounding totally pedantic, why not structure it like 
> > > the 
> > > existing smp_mb__before/after*() primitives in barrier.h?
> > > 
> > > That allows asm-generic/barrier.h to pick up the definition - for example 
> > > in the 
> > > case of smp_acquire__after_ctrl_dep() we do:
> > > 
> > >  #ifndef smp_acquire__after_ctrl_dep
> > >  #define smp_acquire__after_ctrl_dep()   smp_rmb()
> > >  #endif
> > > 
> > > Which allows Tile to relax it:
> > > 
> > >   arch/tile/include/asm/barrier.h:#define smp_acquire__after_ctrl_dep()   
> > > barrier()
> > > 
> > > I.e. I'd move the API definition out of rcupdate.h and into barrier.h - 
> > > even 
> > > though tree-RCU is the only user of this barrier type.
> > 
> > I wouldn't have any problem with that, however, some time back it was
> > moved into RCU because (you guessed it!) RCU is the only user.  ;-)
> 
> Indeed ...
> 
> [sounds of rummaging around in the Git tree]
> 
> I found this commit of yours from ancient history (more than a year ago!):
> 
>   commit 12d560f4ea87030667438a169912380be00cea4b
>   Author: Paul E. McKenney 
>   Date:   Tue Jul 14 18:35:23 2015 -0700
> 
> rcu,locking: Privatize smp_mb__after_unlock_lock()
> 
> RCU is the only thing that uses smp_mb__after_unlock_lock(), and is
> likely the only thing that ever will use it, so this commit makes this
> macro private to RCU.
> 
> Signed-off-by: Paul E. McKenney 
> Cc: Will Deacon 
> Cc: Peter Zijlstra 
> Cc: Benjamin Herrenschmidt 
> Cc: "linux-a...@vger.kernel.org" 
> 
> So I concur and I'm fine with your patch - or with the status quo code as 
> well.

I already have the patch queued, so how about I keep it if I get an ack
from the powerpc guys and drop it otherwise?

Thanx, Paul

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Paul E. McKenney]

On Sun, Jan 15, 2017 at 08:57:11AM +0100, Ingo Molnar wrote:
> 
> * Paul E. McKenney  wrote:
> 
> > On Sun, Jan 15, 2017 at 08:11:23AM +0100, Ingo Molnar wrote:
> > > 
> > > * Paul E. McKenney  wrote:
> > > 
> > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > > index 357b32aaea48..5fdfe874229e 100644
> > > > --- a/include/linux/rcupdate.h
> > > > +++ b/include/linux/rcupdate.h
> > > > @@ -1175,11 +1175,11 @@ do { \
> > > >   * if the UNLOCK and LOCK are executed by the same CPU or if the
> > > >   * UNLOCK and LOCK operate on the same lock variable.
> > > >   */
> > > > -#ifdef CONFIG_PPC
> > > > +#ifdef CONFIG_ARCH_WEAK_RELACQ
> > > >  #define smp_mb__after_unlock_lock()smp_mb()  /* Full ordering for 
> > > > lock. */
> > > > -#else /* #ifdef CONFIG_PPC */
> > > > +#else /* #ifdef CONFIG_ARCH_WEAK_RELACQ */
> > > >  #define smp_mb__after_unlock_lock()do { } while (0)
> > > > -#endif /* #else #ifdef CONFIG_PPC */
> > > > +#endif /* #else #ifdef CONFIG_ARCH_WEAK_RELACQ */
> > > >  
> > > >  
> > > 
> > > So at the risk of sounding totally pedantic, why not structure it like 
> > > the 
> > > existing smp_mb__before/after*() primitives in barrier.h?
> > > 
> > > That allows asm-generic/barrier.h to pick up the definition - for example 
> > > in the 
> > > case of smp_acquire__after_ctrl_dep() we do:
> > > 
> > >  #ifndef smp_acquire__after_ctrl_dep
> > >  #define smp_acquire__after_ctrl_dep()   smp_rmb()
> > >  #endif
> > > 
> > > Which allows Tile to relax it:
> > > 
> > >   arch/tile/include/asm/barrier.h:#define smp_acquire__after_ctrl_dep()   
> > > barrier()
> > > 
> > > I.e. I'd move the API definition out of rcupdate.h and into barrier.h - 
> > > even 
> > > though tree-RCU is the only user of this barrier type.
> > 
> > I wouldn't have any problem with that, however, some time back it was
> > moved into RCU because (you guessed it!) RCU is the only user.  ;-)
> 
> Indeed ...
> 
> [sounds of rummaging around in the Git tree]
> 
> I found this commit of yours from ancient history (more than a year ago!):
> 
>   commit 12d560f4ea87030667438a169912380be00cea4b
>   Author: Paul E. McKenney 
>   Date:   Tue Jul 14 18:35:23 2015 -0700
> 
> rcu,locking: Privatize smp_mb__after_unlock_lock()
> 
> RCU is the only thing that uses smp_mb__after_unlock_lock(), and is
> likely the only thing that ever will use it, so this commit makes this
> macro private to RCU.
> 
> Signed-off-by: Paul E. McKenney 
> Cc: Will Deacon 
> Cc: Peter Zijlstra 
> Cc: Benjamin Herrenschmidt 
> Cc: "linux-a...@vger.kernel.org" 
> 
> So I concur and I'm fine with your patch - or with the status quo code as 
> well.

I already have the patch queued, so how about I keep it if I get an ack
from the powerpc guys and drop it otherwise?

Thanx, Paul

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Ingo Molnar]

* Paul E. McKenney  wrote:

> On Sun, Jan 15, 2017 at 08:11:23AM +0100, Ingo Molnar wrote:
> > 
> > * Paul E. McKenney  wrote:
> > 
> > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > index 357b32aaea48..5fdfe874229e 100644
> > > --- a/include/linux/rcupdate.h
> > > +++ b/include/linux/rcupdate.h
> > > @@ -1175,11 +1175,11 @@ do { \
> > >   * if the UNLOCK and LOCK are executed by the same CPU or if the
> > >   * UNLOCK and LOCK operate on the same lock variable.
> > >   */
> > > -#ifdef CONFIG_PPC
> > > +#ifdef CONFIG_ARCH_WEAK_RELACQ
> > >  #define smp_mb__after_unlock_lock()  smp_mb()  /* Full ordering for 
> > > lock. */
> > > -#else /* #ifdef CONFIG_PPC */
> > > +#else /* #ifdef CONFIG_ARCH_WEAK_RELACQ */
> > >  #define smp_mb__after_unlock_lock()  do { } while (0)
> > > -#endif /* #else #ifdef CONFIG_PPC */
> > > +#endif /* #else #ifdef CONFIG_ARCH_WEAK_RELACQ */
> > >  
> > >  
> > 
> > So at the risk of sounding totally pedantic, why not structure it like the 
> > existing smp_mb__before/after*() primitives in barrier.h?
> > 
> > That allows asm-generic/barrier.h to pick up the definition - for example 
> > in the 
> > case of smp_acquire__after_ctrl_dep() we do:
> > 
> >  #ifndef smp_acquire__after_ctrl_dep
> >  #define smp_acquire__after_ctrl_dep()   smp_rmb()
> >  #endif
> > 
> > Which allows Tile to relax it:
> > 
> >   arch/tile/include/asm/barrier.h:#define smp_acquire__after_ctrl_dep()   
> > barrier()
> > 
> > I.e. I'd move the API definition out of rcupdate.h and into barrier.h - 
> > even 
> > though tree-RCU is the only user of this barrier type.
> 
> I wouldn't have any problem with that, however, some time back it was
> moved into RCU because (you guessed it!) RCU is the only user.  ;-)

Indeed ...

[sounds of rummaging around in the Git tree]

I found this commit of yours from ancient history (more than a year ago!):

  commit 12d560f4ea87030667438a169912380be00cea4b
  Author: Paul E. McKenney 
  Date:   Tue Jul 14 18:35:23 2015 -0700

rcu,locking: Privatize smp_mb__after_unlock_lock()

RCU is the only thing that uses smp_mb__after_unlock_lock(), and is
likely the only thing that ever will use it, so this commit makes this
macro private to RCU.

Signed-off-by: Paul E. McKenney 
Cc: Will Deacon 
Cc: Peter Zijlstra 
Cc: Benjamin Herrenschmidt 
Cc: "linux-a...@vger.kernel.org" 

So I concur and I'm fine with your patch - or with the status quo code as well.

Thanks,

Ingo

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Ingo Molnar]

* Paul E. McKenney  wrote:

> On Sun, Jan 15, 2017 at 08:11:23AM +0100, Ingo Molnar wrote:
> > 
> > * Paul E. McKenney  wrote:
> > 
> > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > index 357b32aaea48..5fdfe874229e 100644
> > > --- a/include/linux/rcupdate.h
> > > +++ b/include/linux/rcupdate.h
> > > @@ -1175,11 +1175,11 @@ do { \
> > >   * if the UNLOCK and LOCK are executed by the same CPU or if the
> > >   * UNLOCK and LOCK operate on the same lock variable.
> > >   */
> > > -#ifdef CONFIG_PPC
> > > +#ifdef CONFIG_ARCH_WEAK_RELACQ
> > >  #define smp_mb__after_unlock_lock()  smp_mb()  /* Full ordering for 
> > > lock. */
> > > -#else /* #ifdef CONFIG_PPC */
> > > +#else /* #ifdef CONFIG_ARCH_WEAK_RELACQ */
> > >  #define smp_mb__after_unlock_lock()  do { } while (0)
> > > -#endif /* #else #ifdef CONFIG_PPC */
> > > +#endif /* #else #ifdef CONFIG_ARCH_WEAK_RELACQ */
> > >  
> > >  
> > 
> > So at the risk of sounding totally pedantic, why not structure it like the 
> > existing smp_mb__before/after*() primitives in barrier.h?
> > 
> > That allows asm-generic/barrier.h to pick up the definition - for example 
> > in the 
> > case of smp_acquire__after_ctrl_dep() we do:
> > 
> >  #ifndef smp_acquire__after_ctrl_dep
> >  #define smp_acquire__after_ctrl_dep()   smp_rmb()
> >  #endif
> > 
> > Which allows Tile to relax it:
> > 
> >   arch/tile/include/asm/barrier.h:#define smp_acquire__after_ctrl_dep()   
> > barrier()
> > 
> > I.e. I'd move the API definition out of rcupdate.h and into barrier.h - 
> > even 
> > though tree-RCU is the only user of this barrier type.
> 
> I wouldn't have any problem with that, however, some time back it was
> moved into RCU because (you guessed it!) RCU is the only user.  ;-)

Indeed ...

[sounds of rummaging around in the Git tree]

I found this commit of yours from ancient history (more than a year ago!):

  commit 12d560f4ea87030667438a169912380be00cea4b
  Author: Paul E. McKenney 
  Date:   Tue Jul 14 18:35:23 2015 -0700

rcu,locking: Privatize smp_mb__after_unlock_lock()

RCU is the only thing that uses smp_mb__after_unlock_lock(), and is
likely the only thing that ever will use it, so this commit makes this
macro private to RCU.

Signed-off-by: Paul E. McKenney 
Cc: Will Deacon 
Cc: Peter Zijlstra 
Cc: Benjamin Herrenschmidt 
Cc: "linux-a...@vger.kernel.org" 

So I concur and I'm fine with your patch - or with the status quo code as well.

Thanks,

Ingo

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Paul E. McKenney]

On Sun, Jan 15, 2017 at 08:11:23AM +0100, Ingo Molnar wrote:
> 
> * Paul E. McKenney  wrote:
> 
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index 357b32aaea48..5fdfe874229e 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -1175,11 +1175,11 @@ do { \
> >   * if the UNLOCK and LOCK are executed by the same CPU or if the
> >   * UNLOCK and LOCK operate on the same lock variable.
> >   */
> > -#ifdef CONFIG_PPC
> > +#ifdef CONFIG_ARCH_WEAK_RELACQ
> >  #define smp_mb__after_unlock_lock()smp_mb()  /* Full ordering for 
> > lock. */
> > -#else /* #ifdef CONFIG_PPC */
> > +#else /* #ifdef CONFIG_ARCH_WEAK_RELACQ */
> >  #define smp_mb__after_unlock_lock()do { } while (0)
> > -#endif /* #else #ifdef CONFIG_PPC */
> > +#endif /* #else #ifdef CONFIG_ARCH_WEAK_RELACQ */
> >  
> >  
> 
> So at the risk of sounding totally pedantic, why not structure it like the 
> existing smp_mb__before/after*() primitives in barrier.h?
> 
> That allows asm-generic/barrier.h to pick up the definition - for example in 
> the 
> case of smp_acquire__after_ctrl_dep() we do:
> 
>  #ifndef smp_acquire__after_ctrl_dep
>  #define smp_acquire__after_ctrl_dep()   smp_rmb()
>  #endif
> 
> Which allows Tile to relax it:
> 
>   arch/tile/include/asm/barrier.h:#define smp_acquire__after_ctrl_dep()   
> barrier()
> 
> I.e. I'd move the API definition out of rcupdate.h and into barrier.h - even 
> though tree-RCU is the only user of this barrier type.

I wouldn't have any problem with that, however, some time back it was
moved into RCU because (you guessed it!) RCU is the only user.  ;-)

Thanx, Paul

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Paul E. McKenney]

On Sun, Jan 15, 2017 at 08:11:23AM +0100, Ingo Molnar wrote:
> 
> * Paul E. McKenney  wrote:
> 
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index 357b32aaea48..5fdfe874229e 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -1175,11 +1175,11 @@ do { \
> >   * if the UNLOCK and LOCK are executed by the same CPU or if the
> >   * UNLOCK and LOCK operate on the same lock variable.
> >   */
> > -#ifdef CONFIG_PPC
> > +#ifdef CONFIG_ARCH_WEAK_RELACQ
> >  #define smp_mb__after_unlock_lock()smp_mb()  /* Full ordering for 
> > lock. */
> > -#else /* #ifdef CONFIG_PPC */
> > +#else /* #ifdef CONFIG_ARCH_WEAK_RELACQ */
> >  #define smp_mb__after_unlock_lock()do { } while (0)
> > -#endif /* #else #ifdef CONFIG_PPC */
> > +#endif /* #else #ifdef CONFIG_ARCH_WEAK_RELACQ */
> >  
> >  
> 
> So at the risk of sounding totally pedantic, why not structure it like the 
> existing smp_mb__before/after*() primitives in barrier.h?
> 
> That allows asm-generic/barrier.h to pick up the definition - for example in 
> the 
> case of smp_acquire__after_ctrl_dep() we do:
> 
>  #ifndef smp_acquire__after_ctrl_dep
>  #define smp_acquire__after_ctrl_dep()   smp_rmb()
>  #endif
> 
> Which allows Tile to relax it:
> 
>   arch/tile/include/asm/barrier.h:#define smp_acquire__after_ctrl_dep()   
> barrier()
> 
> I.e. I'd move the API definition out of rcupdate.h and into barrier.h - even 
> though tree-RCU is the only user of this barrier type.

I wouldn't have any problem with that, however, some time back it was
moved into RCU because (you guessed it!) RCU is the only user.  ;-)

Thanx, Paul

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Ingo Molnar]

* Paul E. McKenney  wrote:

> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 357b32aaea48..5fdfe874229e 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -1175,11 +1175,11 @@ do { \
>   * if the UNLOCK and LOCK are executed by the same CPU or if the
>   * UNLOCK and LOCK operate on the same lock variable.
>   */
> -#ifdef CONFIG_PPC
> +#ifdef CONFIG_ARCH_WEAK_RELACQ
>  #define smp_mb__after_unlock_lock()  smp_mb()  /* Full ordering for lock. */
> -#else /* #ifdef CONFIG_PPC */
> +#else /* #ifdef CONFIG_ARCH_WEAK_RELACQ */
>  #define smp_mb__after_unlock_lock()  do { } while (0)
> -#endif /* #else #ifdef CONFIG_PPC */
> +#endif /* #else #ifdef CONFIG_ARCH_WEAK_RELACQ */
>  
>  

So at the risk of sounding totally pedantic, why not structure it like the 
existing smp_mb__before/after*() primitives in barrier.h?

That allows asm-generic/barrier.h to pick up the definition - for example in 
the 
case of smp_acquire__after_ctrl_dep() we do:

 #ifndef smp_acquire__after_ctrl_dep
 #define smp_acquire__after_ctrl_dep()   smp_rmb()
 #endif

Which allows Tile to relax it:

  arch/tile/include/asm/barrier.h:#define smp_acquire__after_ctrl_dep()   
barrier()

I.e. I'd move the API definition out of rcupdate.h and into barrier.h - even 
though tree-RCU is the only user of this barrier type.

Thanks,

Ingo

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Ingo Molnar]

* Paul E. McKenney  wrote:

> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 357b32aaea48..5fdfe874229e 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -1175,11 +1175,11 @@ do { \
>   * if the UNLOCK and LOCK are executed by the same CPU or if the
>   * UNLOCK and LOCK operate on the same lock variable.
>   */
> -#ifdef CONFIG_PPC
> +#ifdef CONFIG_ARCH_WEAK_RELACQ
>  #define smp_mb__after_unlock_lock()  smp_mb()  /* Full ordering for lock. */
> -#else /* #ifdef CONFIG_PPC */
> +#else /* #ifdef CONFIG_ARCH_WEAK_RELACQ */
>  #define smp_mb__after_unlock_lock()  do { } while (0)
> -#endif /* #else #ifdef CONFIG_PPC */
> +#endif /* #else #ifdef CONFIG_ARCH_WEAK_RELACQ */
>  
>  

So at the risk of sounding totally pedantic, why not structure it like the 
existing smp_mb__before/after*() primitives in barrier.h?

That allows asm-generic/barrier.h to pick up the definition - for example in 
the 
case of smp_acquire__after_ctrl_dep() we do:

 #ifndef smp_acquire__after_ctrl_dep
 #define smp_acquire__after_ctrl_dep()   smp_rmb()
 #endif

Which allows Tile to relax it:

  arch/tile/include/asm/barrier.h:#define smp_acquire__after_ctrl_dep()   
barrier()

I.e. I'd move the API definition out of rcupdate.h and into barrier.h - even 
though tree-RCU is the only user of this barrier type.

Thanks,

Ingo

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Ingo Molnar]

* Paul E. McKenney  wrote:

> > > + */
> > > +#ifdef CONFIG_PPC
> > > +#define smp_mb__after_unlock_lock()  smp_mb()  /* Full ordering for 
> > > lock. */
> > > +#else /* #ifdef CONFIG_PPC */
> > > +#define smp_mb__after_unlock_lock()  do { } while (0)
> > > +#endif /* #else #ifdef CONFIG_PPC */
> > 
> > Yeah, so I realize that this was pre-existing code, but putting CONFIG_$ARCH
> > #ifdefs into generic headers is generally frowned upon.
> > 
> > The canonical approach would be either to define a helper Kconfig variable 
> > that 
> > can be set by PPC (but other architectures don't need to set it), or to 
> > expose a 
> > suitable macro (function) for architectures to define in their barrier.h 
> > arch 
> > header file.
> 
> Very well, I will add a separate commit for this.  4.11 OK?

Sure!

Thanks,

Ingo

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Ingo Molnar]

* Paul E. McKenney  wrote:

> > > + */
> > > +#ifdef CONFIG_PPC
> > > +#define smp_mb__after_unlock_lock()  smp_mb()  /* Full ordering for 
> > > lock. */
> > > +#else /* #ifdef CONFIG_PPC */
> > > +#define smp_mb__after_unlock_lock()  do { } while (0)
> > > +#endif /* #else #ifdef CONFIG_PPC */
> > 
> > Yeah, so I realize that this was pre-existing code, but putting CONFIG_$ARCH
> > #ifdefs into generic headers is generally frowned upon.
> > 
> > The canonical approach would be either to define a helper Kconfig variable 
> > that 
> > can be set by PPC (but other architectures don't need to set it), or to 
> > expose a 
> > suitable macro (function) for architectures to define in their barrier.h 
> > arch 
> > header file.
> 
> Very well, I will add a separate commit for this.  4.11 OK?

Sure!

Thanks,

Ingo

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Paul E. McKenney]

On Sat, Jan 14, 2017 at 10:35:50AM +0100, Ingo Molnar wrote:
> 
> * Paul E. McKenney  wrote:
> 
> > If a process invokes synchronize_srcu(), is delayed just the right amount
> > of time, and thus does not sleep when waiting for the grace period to
> > complete, there is no ordering between the end of the grace period and
> > the code following the synchronize_srcu().  Similarly, there can be a
> > lack of ordering between the end of the SRCU grace period and callback
> > invocation.
> > 
> > This commit adds the necessary ordering.
> > 
> > Reported-by: Lance Roy 
> > Signed-off-by: Paul E. McKenney 
> > ---
> >  include/linux/rcupdate.h | 12 
> >  kernel/rcu/srcu.c|  5 +
> >  kernel/rcu/tree.h| 12 
> >  3 files changed, 17 insertions(+), 12 deletions(-)
> > 
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index 01f71e1d2e94..608d56f908f2 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -1161,5 +1161,17 @@ do { \
> > ftrace_dump(oops_dump_mode); \
> >  } while (0)
> >  
> > +/*
> > + * Place this after a lock-acquisition primitive to guarantee that
> > + * an UNLOCK+LOCK pair act as a full barrier.  This guarantee applies
> > + * if the UNLOCK and LOCK are executed by the same CPU or if the
> > + * UNLOCK and LOCK operate on the same lock variable.
> 
> minor typo:
> 
>   s/an UNLOCK+LOCK pair act as
> an UNLOCK+LOCK pair acts as

Fixed.

> > + */
> > +#ifdef CONFIG_PPC
> > +#define smp_mb__after_unlock_lock()smp_mb()  /* Full ordering for 
> > lock. */
> > +#else /* #ifdef CONFIG_PPC */
> > +#define smp_mb__after_unlock_lock()do { } while (0)
> > +#endif /* #else #ifdef CONFIG_PPC */
> 
> Yeah, so I realize that this was pre-existing code, but putting CONFIG_$ARCH
> #ifdefs into generic headers is generally frowned upon.
> 
> The canonical approach would be either to define a helper Kconfig variable 
> that 
> can be set by PPC (but other architectures don't need to set it), or to 
> expose a 
> suitable macro (function) for architectures to define in their barrier.h arch 
> header file.

Very well, I will add a separate commit for this.  4.11 OK?

Thanx, Paul

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Paul E. McKenney]

On Sat, Jan 14, 2017 at 10:35:50AM +0100, Ingo Molnar wrote:
> 
> * Paul E. McKenney  wrote:
> 
> > If a process invokes synchronize_srcu(), is delayed just the right amount
> > of time, and thus does not sleep when waiting for the grace period to
> > complete, there is no ordering between the end of the grace period and
> > the code following the synchronize_srcu().  Similarly, there can be a
> > lack of ordering between the end of the SRCU grace period and callback
> > invocation.
> > 
> > This commit adds the necessary ordering.
> > 
> > Reported-by: Lance Roy 
> > Signed-off-by: Paul E. McKenney 
> > ---
> >  include/linux/rcupdate.h | 12 
> >  kernel/rcu/srcu.c|  5 +
> >  kernel/rcu/tree.h| 12 
> >  3 files changed, 17 insertions(+), 12 deletions(-)
> > 
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index 01f71e1d2e94..608d56f908f2 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -1161,5 +1161,17 @@ do { \
> > ftrace_dump(oops_dump_mode); \
> >  } while (0)
> >  
> > +/*
> > + * Place this after a lock-acquisition primitive to guarantee that
> > + * an UNLOCK+LOCK pair act as a full barrier.  This guarantee applies
> > + * if the UNLOCK and LOCK are executed by the same CPU or if the
> > + * UNLOCK and LOCK operate on the same lock variable.
> 
> minor typo:
> 
>   s/an UNLOCK+LOCK pair act as
> an UNLOCK+LOCK pair acts as

Fixed.

> > + */
> > +#ifdef CONFIG_PPC
> > +#define smp_mb__after_unlock_lock()smp_mb()  /* Full ordering for 
> > lock. */
> > +#else /* #ifdef CONFIG_PPC */
> > +#define smp_mb__after_unlock_lock()do { } while (0)
> > +#endif /* #else #ifdef CONFIG_PPC */
> 
> Yeah, so I realize that this was pre-existing code, but putting CONFIG_$ARCH
> #ifdefs into generic headers is generally frowned upon.
> 
> The canonical approach would be either to define a helper Kconfig variable 
> that 
> can be set by PPC (but other architectures don't need to set it), or to 
> expose a 
> suitable macro (function) for architectures to define in their barrier.h arch 
> header file.

Very well, I will add a separate commit for this.  4.11 OK?

Thanx, Paul

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Paul E. McKenney]

On Sat, Jan 14, 2017 at 11:54:17AM -0800, Paul E. McKenney wrote:
> On Sat, Jan 14, 2017 at 10:35:50AM +0100, Ingo Molnar wrote:
> > * Paul E. McKenney  wrote:

[ . . . ]

> > > + */
> > > +#ifdef CONFIG_PPC
> > > +#define smp_mb__after_unlock_lock()  smp_mb()  /* Full ordering for 
> > > lock. */
> > > +#else /* #ifdef CONFIG_PPC */
> > > +#define smp_mb__after_unlock_lock()  do { } while (0)
> > > +#endif /* #else #ifdef CONFIG_PPC */
> > 
> > Yeah, so I realize that this was pre-existing code, but putting CONFIG_$ARCH
> > #ifdefs into generic headers is generally frowned upon.
> > 
> > The canonical approach would be either to define a helper Kconfig variable 
> > that 
> > can be set by PPC (but other architectures don't need to set it), or to 
> > expose a 
> > suitable macro (function) for architectures to define in their barrier.h 
> > arch 
> > header file.
> 
> Very well, I will add a separate commit for this.  4.11 OK?

Does the patch below seem reasonable?

Thanx, Paul



commit 271c0601237c41a279f975563e13837bace0df03
Author: Paul E. McKenney 
Date:   Sat Jan 14 13:32:50 2017 -0800

rcu: Make arch select smp_mb__after_unlock_lock() strength

The definition of smp_mb__after_unlock_lock() is currently smp_mb()
for CONFIG_PPC and a no-op otherwise.  It would be better to instead
provide an architecture-selectable Kconfig option, and select the
strength of smp_mb__after_unlock_lock() based on that option.  This
commit therefore creates CONFIG_ARCH_WEAK_RELACQ, has PPC select it,
and bases the definition of smp_mb__after_unlock_lock() on this new
CONFIG_ARCH_WEAK_RELACQ Kconfig option.

Reported-by: Ingo Molnar 
Signed-off-by: Paul E. McKenney 
Cc: Peter Zijlstra 
Cc: Will Deacon 
Cc: Boqun Feng 
Cc: Benjamin Herrenschmidt 
Cc: Paul Mackerras 
Cc: Michael Ellerman 
Cc: 

diff --git a/arch/Kconfig b/arch/Kconfig
index 99839c23d453..94dd90d33f95 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -316,6 +316,9 @@ config HAVE_CMPXCHG_LOCAL
 config HAVE_CMPXCHG_DOUBLE
bool
 
+config ARCH_WEAK_RELACQ
+   bool
+
 config ARCH_WANT_IPC_PARSE_VERSION
bool
 
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index a8ee573fe610..e7083d27271e 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -165,6 +165,7 @@ config PPC
select HAVE_ARCH_HARDENED_USERCOPY
select HAVE_KERNEL_GZIP
select HAVE_CC_STACKPROTECTOR
+   select ARCH_WEAK_RELACQ
 
 config GENERIC_CSUM
def_bool CPU_LITTLE_ENDIAN
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 357b32aaea48..5fdfe874229e 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -1175,11 +1175,11 @@ do { \
  * if the UNLOCK and LOCK are executed by the same CPU or if the
  * UNLOCK and LOCK operate on the same lock variable.
  */
-#ifdef CONFIG_PPC
+#ifdef CONFIG_ARCH_WEAK_RELACQ
 #define smp_mb__after_unlock_lock()smp_mb()  /* Full ordering for lock. */
-#else /* #ifdef CONFIG_PPC */
+#else /* #ifdef CONFIG_ARCH_WEAK_RELACQ */
 #define smp_mb__after_unlock_lock()do { } while (0)
-#endif /* #else #ifdef CONFIG_PPC */
+#endif /* #else #ifdef CONFIG_ARCH_WEAK_RELACQ */
 
 
 #endif /* __LINUX_RCUPDATE_H */

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Paul E. McKenney]

On Sat, Jan 14, 2017 at 11:54:17AM -0800, Paul E. McKenney wrote:
> On Sat, Jan 14, 2017 at 10:35:50AM +0100, Ingo Molnar wrote:
> > * Paul E. McKenney  wrote:

[ . . . ]

> > > + */
> > > +#ifdef CONFIG_PPC
> > > +#define smp_mb__after_unlock_lock()  smp_mb()  /* Full ordering for 
> > > lock. */
> > > +#else /* #ifdef CONFIG_PPC */
> > > +#define smp_mb__after_unlock_lock()  do { } while (0)
> > > +#endif /* #else #ifdef CONFIG_PPC */
> > 
> > Yeah, so I realize that this was pre-existing code, but putting CONFIG_$ARCH
> > #ifdefs into generic headers is generally frowned upon.
> > 
> > The canonical approach would be either to define a helper Kconfig variable 
> > that 
> > can be set by PPC (but other architectures don't need to set it), or to 
> > expose a 
> > suitable macro (function) for architectures to define in their barrier.h 
> > arch 
> > header file.
> 
> Very well, I will add a separate commit for this.  4.11 OK?

Does the patch below seem reasonable?

Thanx, Paul



commit 271c0601237c41a279f975563e13837bace0df03
Author: Paul E. McKenney 
Date:   Sat Jan 14 13:32:50 2017 -0800

rcu: Make arch select smp_mb__after_unlock_lock() strength

The definition of smp_mb__after_unlock_lock() is currently smp_mb()
for CONFIG_PPC and a no-op otherwise.  It would be better to instead
provide an architecture-selectable Kconfig option, and select the
strength of smp_mb__after_unlock_lock() based on that option.  This
commit therefore creates CONFIG_ARCH_WEAK_RELACQ, has PPC select it,
and bases the definition of smp_mb__after_unlock_lock() on this new
CONFIG_ARCH_WEAK_RELACQ Kconfig option.

Reported-by: Ingo Molnar 
Signed-off-by: Paul E. McKenney 
Cc: Peter Zijlstra 
Cc: Will Deacon 
Cc: Boqun Feng 
Cc: Benjamin Herrenschmidt 
Cc: Paul Mackerras 
Cc: Michael Ellerman 
Cc: 

diff --git a/arch/Kconfig b/arch/Kconfig
index 99839c23d453..94dd90d33f95 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -316,6 +316,9 @@ config HAVE_CMPXCHG_LOCAL
 config HAVE_CMPXCHG_DOUBLE
bool
 
+config ARCH_WEAK_RELACQ
+   bool
+
 config ARCH_WANT_IPC_PARSE_VERSION
bool
 
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index a8ee573fe610..e7083d27271e 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -165,6 +165,7 @@ config PPC
select HAVE_ARCH_HARDENED_USERCOPY
select HAVE_KERNEL_GZIP
select HAVE_CC_STACKPROTECTOR
+   select ARCH_WEAK_RELACQ
 
 config GENERIC_CSUM
def_bool CPU_LITTLE_ENDIAN
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 357b32aaea48..5fdfe874229e 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -1175,11 +1175,11 @@ do { \
  * if the UNLOCK and LOCK are executed by the same CPU or if the
  * UNLOCK and LOCK operate on the same lock variable.
  */
-#ifdef CONFIG_PPC
+#ifdef CONFIG_ARCH_WEAK_RELACQ
 #define smp_mb__after_unlock_lock()smp_mb()  /* Full ordering for lock. */
-#else /* #ifdef CONFIG_PPC */
+#else /* #ifdef CONFIG_ARCH_WEAK_RELACQ */
 #define smp_mb__after_unlock_lock()do { } while (0)
-#endif /* #else #ifdef CONFIG_PPC */
+#endif /* #else #ifdef CONFIG_ARCH_WEAK_RELACQ */
 
 
 #endif /* __LINUX_RCUPDATE_H */

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Ingo Molnar]

* Paul E. McKenney  wrote:

> If a process invokes synchronize_srcu(), is delayed just the right amount
> of time, and thus does not sleep when waiting for the grace period to
> complete, there is no ordering between the end of the grace period and
> the code following the synchronize_srcu().  Similarly, there can be a
> lack of ordering between the end of the SRCU grace period and callback
> invocation.
> 
> This commit adds the necessary ordering.
> 
> Reported-by: Lance Roy 
> Signed-off-by: Paul E. McKenney 
> ---
>  include/linux/rcupdate.h | 12 
>  kernel/rcu/srcu.c|  5 +
>  kernel/rcu/tree.h| 12 
>  3 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 01f71e1d2e94..608d56f908f2 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -1161,5 +1161,17 @@ do { \
>   ftrace_dump(oops_dump_mode); \
>  } while (0)
>  
> +/*
> + * Place this after a lock-acquisition primitive to guarantee that
> + * an UNLOCK+LOCK pair act as a full barrier.  This guarantee applies
> + * if the UNLOCK and LOCK are executed by the same CPU or if the
> + * UNLOCK and LOCK operate on the same lock variable.

minor typo:

  s/an UNLOCK+LOCK pair act as
an UNLOCK+LOCK pair acts as

> + */
> +#ifdef CONFIG_PPC
> +#define smp_mb__after_unlock_lock()  smp_mb()  /* Full ordering for lock. */
> +#else /* #ifdef CONFIG_PPC */
> +#define smp_mb__after_unlock_lock()  do { } while (0)
> +#endif /* #else #ifdef CONFIG_PPC */

Yeah, so I realize that this was pre-existing code, but putting CONFIG_$ARCH
#ifdefs into generic headers is generally frowned upon.

The canonical approach would be either to define a helper Kconfig variable that 
can be set by PPC (but other architectures don't need to set it), or to expose 
a 
suitable macro (function) for architectures to define in their barrier.h arch 
header file.

Thanks,

Ingo

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Ingo Molnar]

* Paul E. McKenney  wrote:

> If a process invokes synchronize_srcu(), is delayed just the right amount
> of time, and thus does not sleep when waiting for the grace period to
> complete, there is no ordering between the end of the grace period and
> the code following the synchronize_srcu().  Similarly, there can be a
> lack of ordering between the end of the SRCU grace period and callback
> invocation.
> 
> This commit adds the necessary ordering.
> 
> Reported-by: Lance Roy 
> Signed-off-by: Paul E. McKenney 
> ---
>  include/linux/rcupdate.h | 12 
>  kernel/rcu/srcu.c|  5 +
>  kernel/rcu/tree.h| 12 
>  3 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 01f71e1d2e94..608d56f908f2 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -1161,5 +1161,17 @@ do { \
>   ftrace_dump(oops_dump_mode); \
>  } while (0)
>  
> +/*
> + * Place this after a lock-acquisition primitive to guarantee that
> + * an UNLOCK+LOCK pair act as a full barrier.  This guarantee applies
> + * if the UNLOCK and LOCK are executed by the same CPU or if the
> + * UNLOCK and LOCK operate on the same lock variable.

minor typo:

  s/an UNLOCK+LOCK pair act as
an UNLOCK+LOCK pair acts as

> + */
> +#ifdef CONFIG_PPC
> +#define smp_mb__after_unlock_lock()  smp_mb()  /* Full ordering for lock. */
> +#else /* #ifdef CONFIG_PPC */
> +#define smp_mb__after_unlock_lock()  do { } while (0)
> +#endif /* #else #ifdef CONFIG_PPC */

Yeah, so I realize that this was pre-existing code, but putting CONFIG_$ARCH
#ifdefs into generic headers is generally frowned upon.

The canonical approach would be either to define a helper Kconfig variable that 
can be set by PPC (but other architectures don't need to set it), or to expose 
a 
suitable macro (function) for architectures to define in their barrier.h arch 
header file.

Thanks,

Ingo

[PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Paul E. McKenney]

If a process invokes synchronize_srcu(), is delayed just the right amount
of time, and thus does not sleep when waiting for the grace period to
complete, there is no ordering between the end of the grace period and
the code following the synchronize_srcu().  Similarly, there can be a
lack of ordering between the end of the SRCU grace period and callback
invocation.

This commit adds the necessary ordering.

Reported-by: Lance Roy 
Signed-off-by: Paul E. McKenney 
---
 include/linux/rcupdate.h | 12 
 kernel/rcu/srcu.c|  5 +
 kernel/rcu/tree.h| 12 
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 01f71e1d2e94..608d56f908f2 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -1161,5 +1161,17 @@ do { \
ftrace_dump(oops_dump_mode); \
 } while (0)
 
+/*
+ * Place this after a lock-acquisition primitive to guarantee that
+ * an UNLOCK+LOCK pair act as a full barrier.  This guarantee applies
+ * if the UNLOCK and LOCK are executed by the same CPU or if the
+ * UNLOCK and LOCK operate on the same lock variable.
+ */
+#ifdef CONFIG_PPC
+#define smp_mb__after_unlock_lock()smp_mb()  /* Full ordering for lock. */
+#else /* #ifdef CONFIG_PPC */
+#define smp_mb__after_unlock_lock()do { } while (0)
+#endif /* #else #ifdef CONFIG_PPC */
+
 
 #endif /* __LINUX_RCUPDATE_H */
diff --git a/kernel/rcu/srcu.c b/kernel/rcu/srcu.c
index edfdfadec821..2c9265035154 100644
--- a/kernel/rcu/srcu.c
+++ b/kernel/rcu/srcu.c
@@ -363,6 +363,7 @@ void call_srcu(struct srcu_struct *sp, struct rcu_head 
*head,
head->next = NULL;
head->func = func;
spin_lock_irqsave(>queue_lock, flags);
+   smp_mb__after_unlock_lock(); /* Caller's prior accesses before GP. */
rcu_batch_queue(>batch_queue, head);
if (!sp->running) {
sp->running = true;
@@ -396,6 +397,7 @@ static void __synchronize_srcu(struct srcu_struct *sp, int 
trycount)
head->next = NULL;
head->func = wakeme_after_rcu;
spin_lock_irq(>queue_lock);
+   smp_mb__after_unlock_lock(); /* Caller's prior accesses before GP. */
if (!sp->running) {
/* steal the processing owner */
sp->running = true;
@@ -417,6 +419,8 @@ static void __synchronize_srcu(struct srcu_struct *sp, int 
trycount)
 
if (!done)
wait_for_completion();
+
+   smp_mb(); /* Caller's later accesses after GP. */
 }
 
 /**
@@ -591,6 +595,7 @@ static void srcu_invoke_callbacks(struct srcu_struct *sp)
int i;
struct rcu_head *head;
 
+   smp_mb(); /* Callback accesses after GP. */
for (i = 0; i < SRCU_CALLBACK_BATCH; i++) {
head = rcu_batch_dequeue(>batch_done);
if (!head)
diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index fe98dd24adf8..abcc25bdcb29 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -688,18 +688,6 @@ static inline void rcu_nocb_q_lengths(struct rcu_data 
*rdp, long *ql, long *qll)
 #endif /* #ifdef CONFIG_RCU_TRACE */
 
 /*
- * Place this after a lock-acquisition primitive to guarantee that
- * an UNLOCK+LOCK pair act as a full barrier.  This guarantee applies
- * if the UNLOCK and LOCK are executed by the same CPU or if the
- * UNLOCK and LOCK operate on the same lock variable.
- */
-#ifdef CONFIG_PPC
-#define smp_mb__after_unlock_lock()smp_mb()  /* Full ordering for lock. */
-#else /* #ifdef CONFIG_PPC */
-#define smp_mb__after_unlock_lock()do { } while (0)
-#endif /* #else #ifdef CONFIG_PPC */
-
-/*
  * Wrappers for the rcu_node::lock acquire and release.
  *
  * Because the rcu_nodes form a tree, the tree traversal locking will observe
-- 
2.5.2

[PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Paul E. McKenney]

If a process invokes synchronize_srcu(), is delayed just the right amount
of time, and thus does not sleep when waiting for the grace period to
complete, there is no ordering between the end of the grace period and
the code following the synchronize_srcu().  Similarly, there can be a
lack of ordering between the end of the SRCU grace period and callback
invocation.

This commit adds the necessary ordering.

Reported-by: Lance Roy 
Signed-off-by: Paul E. McKenney 
---
 include/linux/rcupdate.h | 12 
 kernel/rcu/srcu.c|  5 +
 kernel/rcu/tree.h| 12 
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 01f71e1d2e94..608d56f908f2 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -1161,5 +1161,17 @@ do { \
ftrace_dump(oops_dump_mode); \
 } while (0)
 
+/*
+ * Place this after a lock-acquisition primitive to guarantee that
+ * an UNLOCK+LOCK pair act as a full barrier.  This guarantee applies
+ * if the UNLOCK and LOCK are executed by the same CPU or if the
+ * UNLOCK and LOCK operate on the same lock variable.
+ */
+#ifdef CONFIG_PPC
+#define smp_mb__after_unlock_lock()smp_mb()  /* Full ordering for lock. */
+#else /* #ifdef CONFIG_PPC */
+#define smp_mb__after_unlock_lock()do { } while (0)
+#endif /* #else #ifdef CONFIG_PPC */
+
 
 #endif /* __LINUX_RCUPDATE_H */
diff --git a/kernel/rcu/srcu.c b/kernel/rcu/srcu.c
index edfdfadec821..2c9265035154 100644
--- a/kernel/rcu/srcu.c
+++ b/kernel/rcu/srcu.c
@@ -363,6 +363,7 @@ void call_srcu(struct srcu_struct *sp, struct rcu_head 
*head,
head->next = NULL;
head->func = func;
spin_lock_irqsave(>queue_lock, flags);
+   smp_mb__after_unlock_lock(); /* Caller's prior accesses before GP. */
rcu_batch_queue(>batch_queue, head);
if (!sp->running) {
sp->running = true;
@@ -396,6 +397,7 @@ static void __synchronize_srcu(struct srcu_struct *sp, int 
trycount)
head->next = NULL;
head->func = wakeme_after_rcu;
spin_lock_irq(>queue_lock);
+   smp_mb__after_unlock_lock(); /* Caller's prior accesses before GP. */
if (!sp->running) {
/* steal the processing owner */
sp->running = true;
@@ -417,6 +419,8 @@ static void __synchronize_srcu(struct srcu_struct *sp, int 
trycount)
 
if (!done)
wait_for_completion();
+
+   smp_mb(); /* Caller's later accesses after GP. */
 }
 
 /**
@@ -591,6 +595,7 @@ static void srcu_invoke_callbacks(struct srcu_struct *sp)
int i;
struct rcu_head *head;
 
+   smp_mb(); /* Callback accesses after GP. */
for (i = 0; i < SRCU_CALLBACK_BATCH; i++) {
head = rcu_batch_dequeue(>batch_done);
if (!head)
diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index fe98dd24adf8..abcc25bdcb29 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -688,18 +688,6 @@ static inline void rcu_nocb_q_lengths(struct rcu_data 
*rdp, long *ql, long *qll)
 #endif /* #ifdef CONFIG_RCU_TRACE */
 
 /*
- * Place this after a lock-acquisition primitive to guarantee that
- * an UNLOCK+LOCK pair act as a full barrier.  This guarantee applies
- * if the UNLOCK and LOCK are executed by the same CPU or if the
- * UNLOCK and LOCK operate on the same lock variable.
- */
-#ifdef CONFIG_PPC
-#define smp_mb__after_unlock_lock()smp_mb()  /* Full ordering for lock. */
-#else /* #ifdef CONFIG_PPC */
-#define smp_mb__after_unlock_lock()do { } while (0)
-#endif /* #else #ifdef CONFIG_PPC */
-
-/*
  * Wrappers for the rcu_node::lock acquire and release.
  *
  * Because the rcu_nodes form a tree, the tree traversal locking will observe
-- 
2.5.2