Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1558915 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-01-14 10:30 +0100
    Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering Ingo Molnar <mingo@kernel.org> - 2017-01-14 10:40 +0100
      Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-01-15 04:50 +0100
        Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-01-15 04:50 +0100
          Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering Ingo Molnar <mingo@kernel.org> - 2017-01-15 08:20 +0100
            Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-01-15 08:50 +0100
              Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering Ingo Molnar <mingo@kernel.org> - 2017-01-15 09:00 +0100
                Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-01-15 10:30 +0100
                  Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering Ingo Molnar <mingo@kernel.org> - 2017-01-15 10:50 +0100
                    Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-01-15 20:50 +0100
                      Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering Ingo Molnar <mingo@kernel.org> - 2017-01-16 08:00 +0100
        Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering Ingo Molnar <mingo@kernel.org> - 2017-01-15 08:00 +0100

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

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 <ldr709@gmail.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 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(&sp->queue_lock, flags);
+	smp_mb__after_unlock_lock(); /* Caller's prior accesses before GP. */
 	rcu_batch_queue(&sp->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(&sp->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(&rcu.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(&sp->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

[toc] | [next] | [standalone]

#1558922

* Paul E. McKenney <paulmck@linux.vnet.ibm.com> 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 <ldr709@gmail.com>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>  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

[toc] | [prev] | [next] | [standalone]

#1559142

On Sat, Jan 14, 2017 at 10:35:50AM +0100, Ingo Molnar wrote:
> 
> * Paul E. McKenney <paulmck@linux.vnet.ibm.com> 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 <ldr709@gmail.com>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> >  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

[toc] | [prev] | [next] | [standalone]

#1559143

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 <paulmck@linux.vnet.ibm.com> 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 <paulmck@linux.vnet.ibm.com>
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 <mingo@kernel.org>
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
    Cc: Peter Zijlstra <peterz@infradead.org>
    Cc: Will Deacon <will.deacon@arm.com>
    Cc: Boqun Feng <boqun.feng@linux.vnet.ibm.com>
    Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
    Cc: Paul Mackerras <paulus@samba.org>
    Cc: Michael Ellerman <mpe@ellerman.id.au>
    Cc: <linuxppc-dev@lists.ozlabs.org>

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 */

[toc] | [prev] | [next] | [standalone]

#1559172

* Paul E. McKenney <paulmck@linux.vnet.ibm.com> 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

[toc] | [prev] | [next] | [standalone]

#1559177

On Sun, Jan 15, 2017 at 08:11:23AM +0100, Ingo Molnar wrote:
> 
> * Paul E. McKenney <paulmck@linux.vnet.ibm.com> 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

[toc] | [prev] | [next] | [standalone]

#1559178

* Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:

> On Sun, Jan 15, 2017 at 08:11:23AM +0100, Ingo Molnar wrote:
> > 
> > * Paul E. McKenney <paulmck@linux.vnet.ibm.com> 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 <paulmck@linux.vnet.ibm.com>
  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 <paulmck@linux.vnet.ibm.com>
    Cc: Will Deacon <will.deacon@arm.com>
    Cc: Peter Zijlstra <peterz@infradead.org>
    Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
    Cc: "linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>

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

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1559183

On Sun, Jan 15, 2017 at 08:57:11AM +0100, Ingo Molnar wrote:
> 
> * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> 
> > On Sun, Jan 15, 2017 at 08:11:23AM +0100, Ingo Molnar wrote:
> > > 
> > > * Paul E. McKenney <paulmck@linux.vnet.ibm.com> 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 <paulmck@linux.vnet.ibm.com>
>   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 <paulmck@linux.vnet.ibm.com>
>     Cc: Will Deacon <will.deacon@arm.com>
>     Cc: Peter Zijlstra <peterz@infradead.org>
>     Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>     Cc: "linux-arch@vger.kernel.org" <linux-arch@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

[toc] | [prev] | [next] | [standalone]

#1559185

* Paul E. McKenney <paulmck@linux.vnet.ibm.com> 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 <paulmck@linux.vnet.ibm.com>
> >   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 <paulmck@linux.vnet.ibm.com>
> >     Cc: Will Deacon <will.deacon@arm.com>
> >     Cc: Peter Zijlstra <peterz@infradead.org>
> >     Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >     Cc: "linux-arch@vger.kernel.org" <linux-arch@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

[toc] | [prev] | [next] | [standalone]

#1559334

On Sun, Jan 15, 2017 at 10:40:58AM +0100, Ingo Molnar wrote:
> 
> * Paul E. McKenney <paulmck@linux.vnet.ibm.com> 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 <paulmck@linux.vnet.ibm.com>
> > >   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 <paulmck@linux.vnet.ibm.com>
> > >     Cc: Will Deacon <will.deacon@arm.com>
> > >     Cc: Peter Zijlstra <peterz@infradead.org>
> > >     Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > >     Cc: "linux-arch@vger.kernel.org" <linux-arch@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

[toc] | [prev] | [next] | [standalone]

#1559481

* Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:

> On Sun, Jan 15, 2017 at 10:40:58AM +0100, Ingo Molnar wrote:
> > 
> > * Paul E. McKenney <paulmck@linux.vnet.ibm.com> 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 <paulmck@linux.vnet.ibm.com>
> > > >   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 <paulmck@linux.vnet.ibm.com>
> > > >     Cc: Will Deacon <will.deacon@arm.com>
> > > >     Cc: Peter Zijlstra <peterz@infradead.org>
> > > >     Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > > >     Cc: "linux-arch@vger.kernel.org" <linux-arch@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

[toc] | [prev] | [next] | [standalone]

#1559171

* Paul E. McKenney <paulmck@linux.vnet.ibm.com> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web