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

Groups > linux.kernel > #1476232 > unrolled thread

Question on smp_mb__before_spinlock

Back to article view | Back to linux.kernel

Contents

  Question on smp_mb__before_spinlock Peter Zijlstra <peterz@infradead.org> - 2016-09-05 11:40 +0200
    Re: Question on smp_mb__before_spinlock kbuild test robot <lkp@intel.com> - 2016-09-05 12:00 +0200
      Re: Question on smp_mb__before_spinlock Peter Zijlstra <peterz@infradead.org> - 2016-09-05 12:20 +0200
        Re: Question on smp_mb__before_spinlock Fengguang Wu <lkp@intel.com> - 2016-09-05 13:30 +0200
    Re: Question on smp_mb__before_spinlock Will Deacon <will.deacon@arm.com> - 2016-09-05 12:20 +0200
      Re: Question on smp_mb__before_spinlock Peter Zijlstra <peterz@infradead.org> - 2016-09-06 13:20 +0200
        Re: Question on smp_mb__before_spinlock Will Deacon <will.deacon@arm.com> - 2016-09-06 19:50 +0200
    Re: Question on smp_mb__before_spinlock "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-09-05 12:40 +0200
      Re: Question on smp_mb__before_spinlock Peter Zijlstra <peterz@infradead.org> - 2016-09-05 13:40 +0200
        Re: Question on smp_mb__before_spinlock "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-09-05 16:00 +0200
    Re: Question on smp_mb__before_spinlock Nicholas Piggin <npiggin@gmail.com> - 2016-09-07 14:20 +0200
      Re: Question on smp_mb__before_spinlock Peter Zijlstra <peterz@infradead.org> - 2016-09-07 15:30 +0200
        Re: Question on smp_mb__before_spinlock Will Deacon <will.deacon@arm.com> - 2016-09-07 16:00 +0200

#1476232 — Question on smp_mb__before_spinlock

Hi all,

So recently I've had two separate issues that touched upon
smp_mb__before_spinlock().


Since its inception, our understanding of ACQUIRE, esp. as applied to
spinlocks, has changed somewhat. Also, I wonder if, with a simple
change, we cannot make it provide more.

The problem with the comment is that the STORE done by spin_lock isn't
itself ordered by the ACQUIRE, and therefore a later LOAD can pass over
it and cross with any prior STORE, rendering the default WMB
insufficient (pointed out by Alan).

Now, this is only really a problem on PowerPC and ARM64, the former of
which already defined smp_mb__before_spinlock() as a smp_mb(), the
latter does not, Will?

The second issue I wondered about is spinlock transitivity. All except
powerpc have RCsc locks, and since Power already does a full mb, would
it not make sense to put it _after_ the spin_lock(), which would provide
the same guarantee, but also upgrades the section to RCsc.

That would make all schedule() calls fully transitive against one
another.


That is, would something like the below make sense?

(does not deal with mm_types.h and overlayfs using
smp_mb__before_spnlock).

---
 arch/arm64/include/asm/barrier.h   |  2 ++
 arch/powerpc/include/asm/barrier.h |  2 +-
 include/linux/spinlock.h           | 41 +++++++++++++++++++++++++++++---------
 kernel/sched/core.c                |  5 +++--
 4 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h
index 4eea7f618dce..d5cc8b58f942 100644
--- a/arch/arm64/include/asm/barrier.h
+++ b/arch/arm64/include/asm/barrier.h
@@ -104,6 +104,8 @@ do {									\
 	VAL;								\
 })
 
+#define smp_mb__after_spinlock()	smp_mb()
+
 #include <asm-generic/barrier.h>
 
 #endif	/* __ASSEMBLY__ */
diff --git a/arch/powerpc/include/asm/barrier.h b/arch/powerpc/include/asm/barrier.h
index c0deafc212b8..23d64d7196b7 100644
--- a/arch/powerpc/include/asm/barrier.h
+++ b/arch/powerpc/include/asm/barrier.h
@@ -74,7 +74,7 @@ do {									\
 	___p1;								\
 })
 
-#define smp_mb__before_spinlock()   smp_mb()
+#define smp_mb__after_spinlock()   smp_mb()
 
 #include <asm-generic/barrier.h>
 
diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index 47dd0cebd204..284616dad607 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -118,16 +118,39 @@ do {								\
 #endif
 
 /*
- * Despite its name it doesn't necessarily has to be a full barrier.
- * It should only guarantee that a STORE before the critical section
- * can not be reordered with LOADs and STOREs inside this section.
- * spin_lock() is the one-way barrier, this LOAD can not escape out
- * of the region. So the default implementation simply ensures that
- * a STORE can not move into the critical section, smp_wmb() should
- * serialize it with another STORE done by spin_lock().
+ * This barrier must provide two things:
+ *
+ *   - it must guarantee a STORE before the spin_lock() is ordered against a
+ *     LOAD after it, see the comments at its two usage sites.
+ *
+ *   - it must ensure the critical section is RCsc.
+ *
+ * The latter is important for cases where we observe values written by other
+ * CPUs in spin-loops, without barriers, while being subject to scheduling.
+ *
+ * CPU0			CPU1			CPU2
+ * 
+ * 			for (;;) {
+ * 			  if (READ_ONCE(X))
+ * 			  	break;
+ * 			}
+ * X=1
+ * 			<sched-out>
+ * 						<sched-in>
+ * 						r = X;
+ *
+ * without transitivity it could be that CPU1 observes X!=0 breaks the loop,
+ * we get migrated and CPU2 sees X==0.
+ *
+ * Since most load-store architectures implement ACQUIRE with an smp_mb() after
+ * the LL/SC loop, they need no further barriers. Similarly all our TSO
+ * architectures imlpy an smp_mb() for each atomic instruction and equally don't
+ * need more.
+ *
+ * Architectures that can implement ACQUIRE better need to take care.
  */
-#ifndef smp_mb__before_spinlock
-#define smp_mb__before_spinlock()	smp_wmb()
+#ifndef smp_mb__after_spinlock
+#define smp_mb__after_spinlock()	do { } while (0)
 #endif
 
 /**
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 556cb07ab1cf..b151a33d393b 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2006,8 +2006,8 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
 	 * reordered with p->state check below. This pairs with mb() in
 	 * set_current_state() the waiting thread does.
 	 */
-	smp_mb__before_spinlock();
 	raw_spin_lock_irqsave(&p->pi_lock, flags);
+	smp_mb__after_spinlock();
 	if (!(p->state & state))
 		goto out;
 
@@ -3332,8 +3332,9 @@ static void __sched notrace __schedule(bool preempt)
 	 * can't be reordered with __set_current_state(TASK_INTERRUPTIBLE)
 	 * done by the caller to avoid the race with signal_wake_up().
 	 */
-	smp_mb__before_spinlock();
 	raw_spin_lock(&rq->lock);
+	smp_mb__after_spinlock();
+
 	cookie = lockdep_pin_lock(&rq->lock);
 
 	rq->clock_skip_update <<= 1; /* promote REQ to ACT */

[toc] | [next] | [standalone]

#1476260

[Multipart message — attachments visible in raw view] — view raw

Hi Peter,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.8-rc5 next-20160825]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Peter-Zijlstra/Question-on-smp_mb__before_spinlock/20160905-174026
config: x86_64-randconfig-x013-201636 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   In file included from include/linux/sched.h:27:0,
                    from include/linux/kasan.h:4,
                    from include/linux/slab.h:118,
                    from include/linux/crypto.h:24,
                    from arch/x86/kernel/asm-offsets.c:8:
   include/linux/mm_types.h: In function 'set_tlb_flush_pending':
>> include/linux/mm_types.h:557:2: error: implicit declaration of function 'smp_mb__before_spinlock' [-Werror=implicit-function-declaration]
     smp_mb__before_spinlock();
     ^~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors
   make[2]: *** [arch/x86/kernel/asm-offsets.s] Error 1
   make[2]: Target '__build' not remade because of errors.
   make[1]: *** [prepare0] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [sub-make] Error 2

vim +/smp_mb__before_spinlock +557 include/linux/mm_types.h

20841405 Rik van Riel 2013-12-18  551  	mm->tlb_flush_pending = true;
af2c1401 Mel Gorman   2013-12-18  552  
af2c1401 Mel Gorman   2013-12-18  553  	/*
af2c1401 Mel Gorman   2013-12-18  554  	 * Guarantee that the tlb_flush_pending store does not leak into the
af2c1401 Mel Gorman   2013-12-18  555  	 * critical section updating the page tables
af2c1401 Mel Gorman   2013-12-18  556  	 */
af2c1401 Mel Gorman   2013-12-18 @557  	smp_mb__before_spinlock();
20841405 Rik van Riel 2013-12-18  558  }
20841405 Rik van Riel 2013-12-18  559  /* Clearing is done after a TLB flush, which also provides a barrier. */
20841405 Rik van Riel 2013-12-18  560  static inline void clear_tlb_flush_pending(struct mm_struct *mm)

:::::: The code at line 557 was first introduced by commit
:::::: af2c1401e6f9177483be4fad876d0073669df9df mm: numa: guarantee that tlb_flush_pending updates are visible before page table updates

:::::: TO: Mel Gorman <mgorman@suse.de>
:::::: CC: Linus Torvalds <torvalds@linux-foundation.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

#1476275

On Mon, Sep 05, 2016 at 05:56:46PM +0800, kbuild test robot wrote:
> Hi Peter,
> 
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.8-rc5 next-20160825]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> [Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
> [Check https://git-scm.com/docs/git-format-patch for more information]

What happend to not applying patches that lack a SoB and the subject
doesn't even include [PATCH] either.

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

#1476317

Hi Peter,

On Mon, Sep 05, 2016 at 12:19:37PM +0200, Peter Zijlstra wrote:
>On Mon, Sep 05, 2016 at 05:56:46PM +0800, kbuild test robot wrote:
>> Hi Peter,
>>
>> [auto build test ERROR on linus/master]
>> [also build test ERROR on v4.8-rc5 next-20160825]
>> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>> [Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
>> [Check https://git-scm.com/docs/git-format-patch for more information]
>
>What happend to not applying patches that lack a SoB and the subject
>doesn't even include [PATCH] either.

Sorry the current logic assumes "Re: " in subject. Just fixed it to
ignore all patches w/o SOB.

Regards,
Fengguang

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

#1476279

On Mon, Sep 05, 2016 at 11:37:53AM +0200, Peter Zijlstra wrote:
> So recently I've had two separate issues that touched upon
> smp_mb__before_spinlock().
> 
> 
> Since its inception, our understanding of ACQUIRE, esp. as applied to
> spinlocks, has changed somewhat. Also, I wonder if, with a simple
> change, we cannot make it provide more.
> 
> The problem with the comment is that the STORE done by spin_lock isn't
> itself ordered by the ACQUIRE, and therefore a later LOAD can pass over
> it and cross with any prior STORE, rendering the default WMB
> insufficient (pointed out by Alan).
> 
> Now, this is only really a problem on PowerPC and ARM64, the former of
> which already defined smp_mb__before_spinlock() as a smp_mb(), the
> latter does not, Will?

I just replied to that thread and, assuming I've groked the sched/core.c
usage correctly, then it does look like we need to make that an smp_mb()
with the current code.

> The second issue I wondered about is spinlock transitivity. All except
> powerpc have RCsc locks, and since Power already does a full mb, would
> it not make sense to put it _after_ the spin_lock(), which would provide
> the same guarantee, but also upgrades the section to RCsc.
> 
> That would make all schedule() calls fully transitive against one
> another.

It would also match the way in which the arm64 atomic_*_return ops
are implemented, since full barrier semantics are required there.

> That is, would something like the below make sense?

Works for me, but I'll do a fix to smp_mb__before_spinlock anyway for
the stable tree.

The only slight annoyance is that, on arm64 anyway, a store-release
appearing in program order before the LOCK operation will be observed
in order, so if the write of CONDITION=1 in the try_to_wake_up case
used smp_store_release, we wouldn't need this barrier at all.

Will

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

#1477280

On Mon, Sep 05, 2016 at 11:10:22AM +0100, Will Deacon wrote:

> > The second issue I wondered about is spinlock transitivity. All except
> > powerpc have RCsc locks, and since Power already does a full mb, would
> > it not make sense to put it _after_ the spin_lock(), which would provide
> > the same guarantee, but also upgrades the section to RCsc.
> > 
> > That would make all schedule() calls fully transitive against one
> > another.
> 
> It would also match the way in which the arm64 atomic_*_return ops
> are implemented, since full barrier semantics are required there.

Hmm, are you sure; the way I read arch/arm64/include/asm/atomic_ll_sc.h
is that you do ll/sc-rel + mb.

> > That is, would something like the below make sense?
> 
> Works for me, but I'll do a fix to smp_mb__before_spinlock anyway for
> the stable tree.

Indeed, thanks!

> 
> The only slight annoyance is that, on arm64 anyway, a store-release
> appearing in program order before the LOCK operation will be observed
> in order, so if the write of CONDITION=1 in the try_to_wake_up case
> used smp_store_release, we wouldn't need this barrier at all.

Right, but this is because your load-acquire and store-release are much
stronger than Linux's. Not only are they RCsc, they are also globally
ordered irrespective of the variable (iirc).

This wouldn't work for PPC (even if we could find all such prior
stores).

OK, I suppose I'll go stare what we can do about the mm_types.h use and
spin a patch with Changelog.

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

#1477705

On Tue, Sep 06, 2016 at 01:17:53PM +0200, Peter Zijlstra wrote:
> On Mon, Sep 05, 2016 at 11:10:22AM +0100, Will Deacon wrote:
> 
> > > The second issue I wondered about is spinlock transitivity. All except
> > > powerpc have RCsc locks, and since Power already does a full mb, would
> > > it not make sense to put it _after_ the spin_lock(), which would provide
> > > the same guarantee, but also upgrades the section to RCsc.
> > > 
> > > That would make all schedule() calls fully transitive against one
> > > another.
> > 
> > It would also match the way in which the arm64 atomic_*_return ops
> > are implemented, since full barrier semantics are required there.
> 
> Hmm, are you sure; the way I read arch/arm64/include/asm/atomic_ll_sc.h
> is that you do ll/sc-rel + mb.

Yes, all I meant was that we put the full barrier at the end, but the
two things are indeed different sequences.

Will

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

#1476296

On Mon, Sep 05, 2016 at 11:37:53AM +0200, Peter Zijlstra wrote:
> Hi all,
> 
> So recently I've had two separate issues that touched upon
> smp_mb__before_spinlock().
> 
> 
> Since its inception, our understanding of ACQUIRE, esp. as applied to
> spinlocks, has changed somewhat. Also, I wonder if, with a simple
> change, we cannot make it provide more.
> 
> The problem with the comment is that the STORE done by spin_lock isn't
> itself ordered by the ACQUIRE, and therefore a later LOAD can pass over
> it and cross with any prior STORE, rendering the default WMB
> insufficient (pointed out by Alan).
> 
> Now, this is only really a problem on PowerPC and ARM64, the former of
> which already defined smp_mb__before_spinlock() as a smp_mb(), the
> latter does not, Will?
> 
> The second issue I wondered about is spinlock transitivity. All except
> powerpc have RCsc locks, and since Power already does a full mb, would
> it not make sense to put it _after_ the spin_lock(), which would provide
> the same guarantee, but also upgrades the section to RCsc.
> 
> That would make all schedule() calls fully transitive against one
> another.
> 
> 
> That is, would something like the below make sense?

Looks to me like you have reinvented smp_mb__after_unlock_lock()...

							Thanx, Paul

> (does not deal with mm_types.h and overlayfs using
> smp_mb__before_spnlock).
> 
> ---
>  arch/arm64/include/asm/barrier.h   |  2 ++
>  arch/powerpc/include/asm/barrier.h |  2 +-
>  include/linux/spinlock.h           | 41 +++++++++++++++++++++++++++++---------
>  kernel/sched/core.c                |  5 +++--
>  4 files changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h
> index 4eea7f618dce..d5cc8b58f942 100644
> --- a/arch/arm64/include/asm/barrier.h
> +++ b/arch/arm64/include/asm/barrier.h
> @@ -104,6 +104,8 @@ do {									\
>  	VAL;								\
>  })
> 
> +#define smp_mb__after_spinlock()	smp_mb()
> +
>  #include <asm-generic/barrier.h>
> 
>  #endif	/* __ASSEMBLY__ */
> diff --git a/arch/powerpc/include/asm/barrier.h b/arch/powerpc/include/asm/barrier.h
> index c0deafc212b8..23d64d7196b7 100644
> --- a/arch/powerpc/include/asm/barrier.h
> +++ b/arch/powerpc/include/asm/barrier.h
> @@ -74,7 +74,7 @@ do {									\
>  	___p1;								\
>  })
> 
> -#define smp_mb__before_spinlock()   smp_mb()
> +#define smp_mb__after_spinlock()   smp_mb()
> 
>  #include <asm-generic/barrier.h>
> 
> diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
> index 47dd0cebd204..284616dad607 100644
> --- a/include/linux/spinlock.h
> +++ b/include/linux/spinlock.h
> @@ -118,16 +118,39 @@ do {								\
>  #endif
> 
>  /*
> - * Despite its name it doesn't necessarily has to be a full barrier.
> - * It should only guarantee that a STORE before the critical section
> - * can not be reordered with LOADs and STOREs inside this section.
> - * spin_lock() is the one-way barrier, this LOAD can not escape out
> - * of the region. So the default implementation simply ensures that
> - * a STORE can not move into the critical section, smp_wmb() should
> - * serialize it with another STORE done by spin_lock().
> + * This barrier must provide two things:
> + *
> + *   - it must guarantee a STORE before the spin_lock() is ordered against a
> + *     LOAD after it, see the comments at its two usage sites.
> + *
> + *   - it must ensure the critical section is RCsc.
> + *
> + * The latter is important for cases where we observe values written by other
> + * CPUs in spin-loops, without barriers, while being subject to scheduling.
> + *
> + * CPU0			CPU1			CPU2
> + * 
> + * 			for (;;) {
> + * 			  if (READ_ONCE(X))
> + * 			  	break;
> + * 			}
> + * X=1
> + * 			<sched-out>
> + * 						<sched-in>
> + * 						r = X;
> + *
> + * without transitivity it could be that CPU1 observes X!=0 breaks the loop,
> + * we get migrated and CPU2 sees X==0.
> + *
> + * Since most load-store architectures implement ACQUIRE with an smp_mb() after
> + * the LL/SC loop, they need no further barriers. Similarly all our TSO
> + * architectures imlpy an smp_mb() for each atomic instruction and equally don't
> + * need more.
> + *
> + * Architectures that can implement ACQUIRE better need to take care.
>   */
> -#ifndef smp_mb__before_spinlock
> -#define smp_mb__before_spinlock()	smp_wmb()
> +#ifndef smp_mb__after_spinlock
> +#define smp_mb__after_spinlock()	do { } while (0)
>  #endif
> 
>  /**
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 556cb07ab1cf..b151a33d393b 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2006,8 +2006,8 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
>  	 * reordered with p->state check below. This pairs with mb() in
>  	 * set_current_state() the waiting thread does.
>  	 */
> -	smp_mb__before_spinlock();
>  	raw_spin_lock_irqsave(&p->pi_lock, flags);
> +	smp_mb__after_spinlock();
>  	if (!(p->state & state))
>  		goto out;
> 
> @@ -3332,8 +3332,9 @@ static void __sched notrace __schedule(bool preempt)
>  	 * can't be reordered with __set_current_state(TASK_INTERRUPTIBLE)
>  	 * done by the caller to avoid the race with signal_wake_up().
>  	 */
> -	smp_mb__before_spinlock();
>  	raw_spin_lock(&rq->lock);
> +	smp_mb__after_spinlock();
> +
>  	cookie = lockdep_pin_lock(&rq->lock);
> 
>  	rq->clock_skip_update <<= 1; /* promote REQ to ACT */
> 

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

#1476324

On Mon, Sep 05, 2016 at 03:37:14AM -0700, Paul E. McKenney wrote:
> On Mon, Sep 05, 2016 at 11:37:53AM +0200, Peter Zijlstra wrote:
> > Hi all,
> > 
> > So recently I've had two separate issues that touched upon
> > smp_mb__before_spinlock().
> > 
> > 
> > Since its inception, our understanding of ACQUIRE, esp. as applied to
> > spinlocks, has changed somewhat. Also, I wonder if, with a simple
> > change, we cannot make it provide more.
> > 
> > The problem with the comment is that the STORE done by spin_lock isn't
> > itself ordered by the ACQUIRE, and therefore a later LOAD can pass over
> > it and cross with any prior STORE, rendering the default WMB
> > insufficient (pointed out by Alan).
> > 
> > Now, this is only really a problem on PowerPC and ARM64, the former of
> > which already defined smp_mb__before_spinlock() as a smp_mb(), the
> > latter does not, Will?
> > 
> > The second issue I wondered about is spinlock transitivity. All except
> > powerpc have RCsc locks, and since Power already does a full mb, would
> > it not make sense to put it _after_ the spin_lock(), which would provide
> > the same guarantee, but also upgrades the section to RCsc.
> > 
> > That would make all schedule() calls fully transitive against one
> > another.
> > 
> > 
> > That is, would something like the below make sense?
> 
> Looks to me like you have reinvented smp_mb__after_unlock_lock()...

Will said the same, but that one doesn't in fact do the first bit, as
ARM64 also needs a full barrier for that, while it doesn't need that to
upgrade to RCsc.

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

#1476473

On Mon, Sep 05, 2016 at 01:34:35PM +0200, Peter Zijlstra wrote:
> On Mon, Sep 05, 2016 at 03:37:14AM -0700, Paul E. McKenney wrote:
> > On Mon, Sep 05, 2016 at 11:37:53AM +0200, Peter Zijlstra wrote:
> > > Hi all,
> > > 
> > > So recently I've had two separate issues that touched upon
> > > smp_mb__before_spinlock().
> > > 
> > > 
> > > Since its inception, our understanding of ACQUIRE, esp. as applied to
> > > spinlocks, has changed somewhat. Also, I wonder if, with a simple
> > > change, we cannot make it provide more.
> > > 
> > > The problem with the comment is that the STORE done by spin_lock isn't
> > > itself ordered by the ACQUIRE, and therefore a later LOAD can pass over
> > > it and cross with any prior STORE, rendering the default WMB
> > > insufficient (pointed out by Alan).
> > > 
> > > Now, this is only really a problem on PowerPC and ARM64, the former of
> > > which already defined smp_mb__before_spinlock() as a smp_mb(), the
> > > latter does not, Will?
> > > 
> > > The second issue I wondered about is spinlock transitivity. All except
> > > powerpc have RCsc locks, and since Power already does a full mb, would
> > > it not make sense to put it _after_ the spin_lock(), which would provide
> > > the same guarantee, but also upgrades the section to RCsc.
> > > 
> > > That would make all schedule() calls fully transitive against one
> > > another.
> > > 
> > > 
> > > That is, would something like the below make sense?
> > 
> > Looks to me like you have reinvented smp_mb__after_unlock_lock()...
> 
> Will said the same, but that one doesn't in fact do the first bit, as
> ARM64 also needs a full barrier for that, while it doesn't need that to
> upgrade to RCsc.

Fair enough!

							Thanx, Paul

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

#1478248

On Mon, 5 Sep 2016 11:37:53 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> Hi all,
> 
> So recently I've had two separate issues that touched upon
> smp_mb__before_spinlock().
> 
> 
> Since its inception, our understanding of ACQUIRE, esp. as applied to
> spinlocks, has changed somewhat. Also, I wonder if, with a simple
> change, we cannot make it provide more.
> 
> The problem with the comment is that the STORE done by spin_lock isn't
> itself ordered by the ACQUIRE, and therefore a later LOAD can pass over
> it and cross with any prior STORE, rendering the default WMB
> insufficient (pointed out by Alan).
> 
> Now, this is only really a problem on PowerPC and ARM64, the former of
> which already defined smp_mb__before_spinlock() as a smp_mb(), the
> latter does not, Will?
> 
> The second issue I wondered about is spinlock transitivity. All except
> powerpc have RCsc locks, and since Power already does a full mb, would
> it not make sense to put it _after_ the spin_lock(), which would provide
> the same guarantee, but also upgrades the section to RCsc.
> 
> That would make all schedule() calls fully transitive against one
> another.
> 
> 
> That is, would something like the below make sense?
> 
> (does not deal with mm_types.h and overlayfs using
> smp_mb__before_spnlock).
> 
> ---
>  arch/arm64/include/asm/barrier.h   |  2 ++
>  arch/powerpc/include/asm/barrier.h |  2 +-
>  include/linux/spinlock.h           | 41 +++++++++++++++++++++++++++++---------
>  kernel/sched/core.c                |  5 +++--
>  4 files changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h
> index 4eea7f618dce..d5cc8b58f942 100644
> --- a/arch/arm64/include/asm/barrier.h
> +++ b/arch/arm64/include/asm/barrier.h
> @@ -104,6 +104,8 @@ do {									\
>  	VAL;								\
>  })
>  
> +#define smp_mb__after_spinlock()	smp_mb()
> +
>  #include <asm-generic/barrier.h>
>  
>  #endif	/* __ASSEMBLY__ */
> diff --git a/arch/powerpc/include/asm/barrier.h b/arch/powerpc/include/asm/barrier.h
> index c0deafc212b8..23d64d7196b7 100644
> --- a/arch/powerpc/include/asm/barrier.h
> +++ b/arch/powerpc/include/asm/barrier.h
> @@ -74,7 +74,7 @@ do {									\
>  	___p1;								\
>  })
>  
> -#define smp_mb__before_spinlock()   smp_mb()
> +#define smp_mb__after_spinlock()   smp_mb()
>  
>  #include <asm-generic/barrier.h>
>  
> diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
> index 47dd0cebd204..284616dad607 100644
> --- a/include/linux/spinlock.h
> +++ b/include/linux/spinlock.h
> @@ -118,16 +118,39 @@ do {								\
>  #endif
>  
>  /*
> - * Despite its name it doesn't necessarily has to be a full barrier.
> - * It should only guarantee that a STORE before the critical section
> - * can not be reordered with LOADs and STOREs inside this section.
> - * spin_lock() is the one-way barrier, this LOAD can not escape out
> - * of the region. So the default implementation simply ensures that
> - * a STORE can not move into the critical section, smp_wmb() should
> - * serialize it with another STORE done by spin_lock().
> + * This barrier must provide two things:
> + *
> + *   - it must guarantee a STORE before the spin_lock() is ordered against a
> + *     LOAD after it, see the comments at its two usage sites.
> + *
> + *   - it must ensure the critical section is RCsc.
> + *
> + * The latter is important for cases where we observe values written by other
> + * CPUs in spin-loops, without barriers, while being subject to scheduling.
> + *
> + * CPU0			CPU1			CPU2
> + * 
> + * 			for (;;) {
> + * 			  if (READ_ONCE(X))
> + * 			  	break;
> + * 			}
> + * X=1
> + * 			<sched-out>
> + * 						<sched-in>
> + * 						r = X;
> + *
> + * without transitivity it could be that CPU1 observes X!=0 breaks the loop,
> + * we get migrated and CPU2 sees X==0.
> + *
> + * Since most load-store architectures implement ACQUIRE with an smp_mb() after
> + * the LL/SC loop, they need no further barriers. Similarly all our TSO
> + * architectures imlpy an smp_mb() for each atomic instruction and equally don't
> + * need more.
> + *
> + * Architectures that can implement ACQUIRE better need to take care.
>   */
> -#ifndef smp_mb__before_spinlock
> -#define smp_mb__before_spinlock()	smp_wmb()
> +#ifndef smp_mb__after_spinlock
> +#define smp_mb__after_spinlock()	do { } while (0)
>  #endif

It seems okay, but why not make it a special sched-only function name
to prevent it being used in generic code?

I would not mind seeing responsibility for the switch barrier moved to
generic context switch code like this (alternative for powerpc reducing
number of hwsync instructions was to add documentation and warnings about
the barriers in arch dependent and independent code). And pairing it with
a spinlock is reasonable.

It may not strictly be an "smp_" style of barrier if MMIO accesses are to
be ordered here too, despite critical section may only be providing
acquire/release for cacheable memory, so maybe it's slightly more
complicated than just cacheable RCsc?

This would end up flushing the store queue while holding the spinlock on
POWER, as opposed to before acquiring the lock. I doubt that's ever going
to be noticable, but if you already add a special new primitive here, then
an arch wrapper for raw_spin_lock() can let archs do their own thing here.

Thanks,
Nick

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

#1478299

On Wed, Sep 07, 2016 at 10:17:26PM +1000, Nicholas Piggin wrote:
> >  /*
> > + * This barrier must provide two things:
> > + *
> > + *   - it must guarantee a STORE before the spin_lock() is ordered against a
> > + *     LOAD after it, see the comments at its two usage sites.
> > + *
> > + *   - it must ensure the critical section is RCsc.
> > + *
> > + * The latter is important for cases where we observe values written by other
> > + * CPUs in spin-loops, without barriers, while being subject to scheduling.
> > + *
> > + * CPU0			CPU1			CPU2
> > + * 
> > + * 			for (;;) {
> > + * 			  if (READ_ONCE(X))
> > + * 			  	break;
> > + * 			}
> > + * X=1
> > + * 			<sched-out>
> > + * 						<sched-in>
> > + * 						r = X;
> > + *
> > + * without transitivity it could be that CPU1 observes X!=0 breaks the loop,
> > + * we get migrated and CPU2 sees X==0.
> > + *
> > + * Since most load-store architectures implement ACQUIRE with an smp_mb() after
> > + * the LL/SC loop, they need no further barriers. Similarly all our TSO
> > + * architectures imlpy an smp_mb() for each atomic instruction and equally don't
> > + * need more.
> > + *
> > + * Architectures that can implement ACQUIRE better need to take care.
> >   */
> > +#ifndef smp_mb__after_spinlock
> > +#define smp_mb__after_spinlock()	do { } while (0)
> >  #endif
> 
> It seems okay, but why not make it a special sched-only function name
> to prevent it being used in generic code?
> 
> I would not mind seeing responsibility for the switch barrier moved to
> generic context switch code like this (alternative for powerpc reducing
> number of hwsync instructions was to add documentation and warnings about
> the barriers in arch dependent and independent code). And pairing it with
> a spinlock is reasonable.
> 
> It may not strictly be an "smp_" style of barrier if MMIO accesses are to
> be ordered here too, despite critical section may only be providing
> acquire/release for cacheable memory, so maybe it's slightly more
> complicated than just cacheable RCsc?

Interesting idea..

So I'm not a fan of that raw_spin_lock wrapper, since that would end up
with a lot more boiler-plate code than just the one extra barrier.

But moving MMIO/DMA/TLB etc.. barriers into this spinlock might not be a
good idea, since those are typically fairly heavy barriers, and its
quite common to call schedule() without ending up in switch_to().

For PowerPC it works out, since there's only SYNC, no other option
afaik.

But ARM/ARM64 will have to do DSB(ISH) instead of DMB(ISH). IA64 would
need to issue "sync.i" and mips-octeon "synciobdma".

Will, any idea of the extra cost involved in DSB vs DMB?

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

#1478322

On Wed, Sep 07, 2016 at 03:23:54PM +0200, Peter Zijlstra wrote:
> On Wed, Sep 07, 2016 at 10:17:26PM +1000, Nicholas Piggin wrote:
> > It seems okay, but why not make it a special sched-only function name
> > to prevent it being used in generic code?
> > 
> > I would not mind seeing responsibility for the switch barrier moved to
> > generic context switch code like this (alternative for powerpc reducing
> > number of hwsync instructions was to add documentation and warnings about
> > the barriers in arch dependent and independent code). And pairing it with
> > a spinlock is reasonable.
> > 
> > It may not strictly be an "smp_" style of barrier if MMIO accesses are to
> > be ordered here too, despite critical section may only be providing
> > acquire/release for cacheable memory, so maybe it's slightly more
> > complicated than just cacheable RCsc?
> 
> Interesting idea..
> 
> So I'm not a fan of that raw_spin_lock wrapper, since that would end up
> with a lot more boiler-plate code than just the one extra barrier.
> 
> But moving MMIO/DMA/TLB etc.. barriers into this spinlock might not be a
> good idea, since those are typically fairly heavy barriers, and its
> quite common to call schedule() without ending up in switch_to().
> 
> For PowerPC it works out, since there's only SYNC, no other option
> afaik.
> 
> But ARM/ARM64 will have to do DSB(ISH) instead of DMB(ISH). IA64 would
> need to issue "sync.i" and mips-octeon "synciobdma".
> 
> Will, any idea of the extra cost involved in DSB vs DMB?

DSB is *much* more expensive, since it completes out-of-band communication
such as MMIO accesses and TLB invalidation, as well as plain old memory
accesses.

The only reason we have DSB in our __switch_to code is to complete cache
maintenance in case the task is going to migrate to another CPU; there's
just no way to know that at the point we need to do the barrier :(

Will

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web