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

Groups > linux.kernel > #1659988 > unrolled thread

[RFC][PATCH 0/5] Getting rid of smp_mb__before_spinlock

Back to article view | Back to linux.kernel

Contents

  [RFC][PATCH 0/5] Getting rid of smp_mb__before_spinlock Peter Zijlstra <peterz@infradead.org> - 2017-06-07 18:30 +0200
    [RFC][PATCH 2/5] locking: Introduce smp_mb__after_spinlock(). Peter Zijlstra <peterz@infradead.org> - 2017-06-07 18:30 +0200
    [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Peter Zijlstra <peterz@infradead.org> - 2017-06-07 18:30 +0200
      Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Will Deacon <will.deacon@arm.com> - 2017-06-09 16:50 +0200
        Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Peter Zijlstra <peterz@infradead.org> - 2017-06-09 20:50 +0200
    [RFC][PATCH 3/5] overlayfs: Remove smp_mb__before_spinlock() usage Peter Zijlstra <peterz@infradead.org> - 2017-06-07 18:30 +0200
    [RFC][PATCH 4/5] locking: Remove smp_mb__before_spinlock() Peter Zijlstra <peterz@infradead.org> - 2017-06-07 18:30 +0200
    [RFC][PATCH 5/5] powerpc: Remove SYNC from _switch Peter Zijlstra <peterz@infradead.org> - 2017-06-07 18:30 +0200
      Re: [RFC][PATCH 5/5] powerpc: Remove SYNC from _switch Nicholas Piggin <npiggin@gmail.com> - 2017-06-08 02:40 +0200
        Re: [RFC][PATCH 5/5] powerpc: Remove SYNC from _switch Peter Zijlstra <peterz@infradead.org> - 2017-06-08 09:00 +0200
          Re: [RFC][PATCH 5/5] powerpc: Remove SYNC from _switch Nicholas Piggin <npiggin@gmail.com> - 2017-06-08 09:40 +0200
            Re: [RFC][PATCH 5/5] powerpc: Remove SYNC from _switch Peter Zijlstra <peterz@infradead.org> - 2017-06-08 10:00 +0200
              Re: [RFC][PATCH 5/5] powerpc: Remove SYNC from _switch Nicholas Piggin <npiggin@gmail.com> - 2017-06-08 10:30 +0200
              Re: [RFC][PATCH 5/5] powerpc: Remove SYNC from _switch Michael Ellerman <mpe@ellerman.id.au> - 2017-06-08 12:00 +0200
                Re: [RFC][PATCH 5/5] powerpc: Remove SYNC from _switch Nicholas Piggin <npiggin@gmail.com> - 2017-06-08 12:10 +0200
                  Re: [RFC][PATCH 5/5] powerpc: Remove SYNC from _switch Peter Zijlstra <peterz@infradead.org> - 2017-06-08 14:50 +0200
                    Re: [RFC][PATCH 5/5] powerpc: Remove SYNC from _switch Nicholas Piggin <npiggin@gmail.com> - 2017-06-08 15:20 +0200
                      Re: [RFC][PATCH 5/5] powerpc: Remove SYNC from _switch Peter Zijlstra <peterz@infradead.org> - 2017-06-08 15:50 +0200
    Re: [RFC][PATCH 0/5] Getting rid of smp_mb__before_spinlock Will Deacon <will.deacon@arm.com> - 2017-06-09 17:00 +0200

#1659988 — [RFC][PATCH 0/5] Getting rid of smp_mb__before_spinlock

There was a thread on this somewhere about a year ago, I finally remembered to
finish this :-)

This series removes two smp_mb__before_spinlock() (ab)users and converts the
scheduler to use smp_mb__after_spinlock(), which provides more guarantees with
the same amount of barriers.

[toc] | [next] | [standalone]

#1659989 — [RFC][PATCH 2/5] locking: Introduce smp_mb__after_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, both of
which already defined smp_mb__before_spinlock() as a smp_mb().

At the same time, we can get a much stronger construct if we place
that same barrier _inside_ the spin_lock(). In that case we upgrade
the RCpc spinlock to an RCsc.  That would make all schedule() calls
fully transitive against one another.

Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/arm64/include/asm/spinlock.h   |    2 ++
 arch/powerpc/include/asm/spinlock.h |    3 +++
 include/linux/spinlock.h            |   36 ++++++++++++++++++++++++++++++++++++
 kernel/sched/core.c                 |    4 ++--
 4 files changed, 43 insertions(+), 2 deletions(-)

--- a/arch/arm64/include/asm/spinlock.h
+++ b/arch/arm64/include/asm/spinlock.h
@@ -367,5 +367,7 @@ static inline int arch_read_trylock(arch
  * smp_mb__before_spinlock() can restore the required ordering.
  */
 #define smp_mb__before_spinlock()	smp_mb()
+/* See include/linux/spinlock.h */
+#define smp_mb__after_spinlock()	smp_mb()
 
 #endif /* __ASM_SPINLOCK_H */
--- a/arch/powerpc/include/asm/spinlock.h
+++ b/arch/powerpc/include/asm/spinlock.h
@@ -342,5 +342,8 @@ static inline void arch_write_unlock(arc
 #define arch_read_relax(lock)	__rw_yield(lock)
 #define arch_write_relax(lock)	__rw_yield(lock)
 
+/* See include/linux/spinlock.h */
+#define smp_mb__after_spinlock()   smp_mb()
+
 #endif /* __KERNEL__ */
 #endif /* __ASM_SPINLOCK_H */
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -130,6 +130,42 @@ do {								\
 #define smp_mb__before_spinlock()	smp_wmb()
 #endif
 
+/*
+ * 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 imply 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
+
 /**
  * raw_spin_unlock_wait - wait until the spinlock gets unlocked
  * @lock: the spinlock in question.
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1969,8 +1969,8 @@ try_to_wake_up(struct task_struct *p, un
 	 * 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;
 
@@ -3383,8 +3383,8 @@ static void __sched notrace __schedule(b
 	 * 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();
 	rq_lock(rq, &rf);
+	smp_mb__after_spinlock();
 
 	/* Promote REQ to ACT */
 	rq->clock_update_flags <<= 1;

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

#1659998 — [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending()

Commit:

  af2c1401e6f9 ("mm: numa: guarantee that tlb_flush_pending updates are visible before page table updates")

added smp_mb__before_spinlock() to set_tlb_flush_pending(). I think we
can solve the same problem without this barrier.

If instead we mandate that mm_tlb_flush_pending() is used while
holding the PTL we're guaranteed to observe prior
set_tlb_flush_pending() instances.

For this to work we need to rework migrate_misplaced_transhuge_page()
a little and move the test up into do_huge_pmd_numa_page().

Cc: Mel Gorman <mgorman@suse.de>
Cc: Rik van Riel <riel@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -527,18 +527,16 @@ static inline cpumask_t *mm_cpumask(stru
  */
 static inline bool mm_tlb_flush_pending(struct mm_struct *mm)
 {
-	barrier();
+	/*
+	 * Must be called with PTL held; such that our PTL acquire will have
+	 * observed the store from set_tlb_flush_pending().
+	 */
 	return mm->tlb_flush_pending;
 }
 static inline void set_tlb_flush_pending(struct mm_struct *mm)
 {
 	mm->tlb_flush_pending = true;
-
-	/*
-	 * Guarantee that the tlb_flush_pending store does not leak into the
-	 * critical section updating the page tables
-	 */
-	smp_mb__before_spinlock();
+	barrier();
 }
 /* Clearing is done after a TLB flush, which also provides a barrier. */
 static inline void clear_tlb_flush_pending(struct mm_struct *mm)
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1410,6 +1410,7 @@ int do_huge_pmd_numa_page(struct vm_faul
 	unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
 	int page_nid = -1, this_nid = numa_node_id();
 	int target_nid, last_cpupid = -1;
+	bool need_flush = false;
 	bool page_locked;
 	bool migrated = false;
 	bool was_writable;
@@ -1490,10 +1491,29 @@ int do_huge_pmd_numa_page(struct vm_faul
 	}
 
 	/*
+	 * Since we took the NUMA fault, we must have observed the !accessible
+	 * bit. Make sure all other CPUs agree with that, to avoid them
+	 * modifying the page we're about to migrate.
+	 *
+	 * Must be done under PTL such that we'll observe the relevant
+	 * set_tlb_flush_pending().
+	 */
+	if (mm_tlb_flush_pending(mm))
+		need_flush = true;
+
+	/*
 	 * Migrate the THP to the requested node, returns with page unlocked
 	 * and access rights restored.
 	 */
 	spin_unlock(vmf->ptl);
+
+	/*
+	 * We are not sure a pending tlb flush here is for a huge page
+	 * mapping or not. Hence use the tlb range variant
+	 */
+	if (need_flush)
+		flush_tlb_range(vma, haddr, haddr + HPAGE_PMD_SIZE);
+
 	migrated = migrate_misplaced_transhuge_page(vma->vm_mm, vma,
 				vmf->pmd, pmd, vmf->address, page, target_nid);
 	if (migrated) {
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1935,12 +1935,6 @@ int migrate_misplaced_transhuge_page(str
 		put_page(new_page);
 		goto out_fail;
 	}
-	/*
-	 * We are not sure a pending tlb flush here is for a huge page
-	 * mapping or not. Hence use the tlb range variant
-	 */
-	if (mm_tlb_flush_pending(mm))
-		flush_tlb_range(vma, mmun_start, mmun_end);
 
 	/* Prepare a page as a migration target */
 	__SetPageLocked(new_page);

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

#1662537 — Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending()

On Wed, Jun 07, 2017 at 06:15:02PM +0200, Peter Zijlstra wrote:
> Commit:
> 
>   af2c1401e6f9 ("mm: numa: guarantee that tlb_flush_pending updates are visible before page table updates")
> 
> added smp_mb__before_spinlock() to set_tlb_flush_pending(). I think we
> can solve the same problem without this barrier.
> 
> If instead we mandate that mm_tlb_flush_pending() is used while
> holding the PTL we're guaranteed to observe prior
> set_tlb_flush_pending() instances.
> 
> For this to work we need to rework migrate_misplaced_transhuge_page()
> a little and move the test up into do_huge_pmd_numa_page().
> 
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Rik van Riel <riel@redhat.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -527,18 +527,16 @@ static inline cpumask_t *mm_cpumask(stru
>   */
>  static inline bool mm_tlb_flush_pending(struct mm_struct *mm)
>  {
> -	barrier();
> +	/*
> +	 * Must be called with PTL held; such that our PTL acquire will have
> +	 * observed the store from set_tlb_flush_pending().
> +	 */
>  	return mm->tlb_flush_pending;
>  }
>  static inline void set_tlb_flush_pending(struct mm_struct *mm)
>  {
>  	mm->tlb_flush_pending = true;
> -
> -	/*
> -	 * Guarantee that the tlb_flush_pending store does not leak into the
> -	 * critical section updating the page tables
> -	 */
> -	smp_mb__before_spinlock();
> +	barrier();

Why do you need the barrier() here? Isn't the ptl unlock sufficient?

Will

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

#1662685 — Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending()

On Fri, Jun 09, 2017 at 03:45:54PM +0100, Will Deacon wrote:
> On Wed, Jun 07, 2017 at 06:15:02PM +0200, Peter Zijlstra wrote:
> > Commit:
> > 
> >   af2c1401e6f9 ("mm: numa: guarantee that tlb_flush_pending updates are visible before page table updates")
> > 
> > added smp_mb__before_spinlock() to set_tlb_flush_pending(). I think we
> > can solve the same problem without this barrier.
> > 
> > If instead we mandate that mm_tlb_flush_pending() is used while
> > holding the PTL we're guaranteed to observe prior
> > set_tlb_flush_pending() instances.
> > 
> > For this to work we need to rework migrate_misplaced_transhuge_page()
> > a little and move the test up into do_huge_pmd_numa_page().
> > 
> > Cc: Mel Gorman <mgorman@suse.de>
> > Cc: Rik van Riel <riel@redhat.com>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> > --- a/include/linux/mm_types.h
> > +++ b/include/linux/mm_types.h
> > @@ -527,18 +527,16 @@ static inline cpumask_t *mm_cpumask(stru
> >   */
> >  static inline bool mm_tlb_flush_pending(struct mm_struct *mm)
> >  {
> > -	barrier();
> > +	/*
> > +	 * Must be called with PTL held; such that our PTL acquire will have
> > +	 * observed the store from set_tlb_flush_pending().
> > +	 */
> >  	return mm->tlb_flush_pending;
> >  }
> >  static inline void set_tlb_flush_pending(struct mm_struct *mm)
> >  {
> >  	mm->tlb_flush_pending = true;
> > -
> > -	/*
> > -	 * Guarantee that the tlb_flush_pending store does not leak into the
> > -	 * critical section updating the page tables
> > -	 */
> > -	smp_mb__before_spinlock();
> > +	barrier();
> 
> Why do you need the barrier() here? Isn't the ptl unlock sufficient?

General paranioa I think. I'll have another look.

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

#1660000 — [RFC][PATCH 3/5] overlayfs: Remove smp_mb__before_spinlock() usage

While we could replace the smp_mb__before_spinlock() with the new
smp_mb__after_spinlock(), the normal pattern is to use
smp_store_release() to publish an object that is used for
lockless_dereference() -- and mirrors the regular rcu_assign_pointer()
/ rcu_dereference() patterns.

Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 fs/overlayfs/readdir.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -446,14 +446,14 @@ static int ovl_dir_fsync(struct file *fi
 
 			ovl_path_upper(dentry, &upperpath);
 			realfile = ovl_path_open(&upperpath, O_RDONLY);
-			smp_mb__before_spinlock();
+
 			inode_lock(inode);
 			if (!od->upperfile) {
 				if (IS_ERR(realfile)) {
 					inode_unlock(inode);
 					return PTR_ERR(realfile);
 				}
-				od->upperfile = realfile;
+				smp_store_release(&od->upperfile, realfile);
 			} else {
 				/* somebody has beaten us to it */
 				if (!IS_ERR(realfile))

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

#1660003 — [RFC][PATCH 4/5] locking: Remove smp_mb__before_spinlock()

Now that there are no users of smp_mb__before_spinlock() left, remove
it entirely.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 Documentation/memory-barriers.txt                    |    5 ---
 Documentation/translations/ko_KR/memory-barriers.txt |    5 ---
 arch/arm64/include/asm/spinlock.h                    |    9 ------
 arch/powerpc/include/asm/barrier.h                   |    2 -
 fs/userfaultfd.c                                     |   25 ++++++++-----------
 include/linux/spinlock.h                             |   13 ---------
 6 files changed, 13 insertions(+), 46 deletions(-)

--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -1982,10 +1982,7 @@ In all cases there are variants on "ACQU
      ACQUIRE operation has completed.
 
      Memory operations issued before the ACQUIRE may be completed after
-     the ACQUIRE operation has completed.  An smp_mb__before_spinlock(),
-     combined with a following ACQUIRE, orders prior stores against
-     subsequent loads and stores.  Note that this is weaker than smp_mb()!
-     The smp_mb__before_spinlock() primitive is free on many architectures.
+     the ACQUIRE operation has completed.
 
  (2) RELEASE operation implication:
 
--- a/Documentation/translations/ko_KR/memory-barriers.txt
+++ b/Documentation/translations/ko_KR/memory-barriers.txt
@@ -1956,10 +1956,7 @@ MMIO 쓰기 배리어
      뒤에 완료됩니다.
 
      ACQUIRE 앞에서 요청된 메모리 오퍼레이션은 ACQUIRE 오퍼레이션이 완료된 후에
-     완료될 수 있습니다.  smp_mb__before_spinlock() 뒤에 ACQUIRE 가 실행되는
-     코드 블록은 블록 앞의 스토어를 블록 뒤의 로드와 스토어에 대해 순서
-     맞춥니다.  이건 smp_mb() 보다 완화된 것임을 기억하세요!  많은 아키텍쳐에서
-     smp_mb__before_spinlock() 은 사실 아무일도 하지 않습니다.
+     완료될 수 있습니다.
 
  (2) RELEASE 오퍼레이션의 영향:
 
--- a/arch/arm64/include/asm/spinlock.h
+++ b/arch/arm64/include/asm/spinlock.h
@@ -358,15 +358,6 @@ static inline int arch_read_trylock(arch
 #define arch_read_relax(lock)	cpu_relax()
 #define arch_write_relax(lock)	cpu_relax()
 
-/*
- * Accesses appearing in program order before a spin_lock() operation
- * can be reordered with accesses inside the critical section, by virtue
- * of arch_spin_lock being constructed using acquire semantics.
- *
- * In cases where this is problematic (e.g. try_to_wake_up), an
- * smp_mb__before_spinlock() can restore the required ordering.
- */
-#define smp_mb__before_spinlock()	smp_mb()
 /* See include/linux/spinlock.h */
 #define smp_mb__after_spinlock()	smp_mb()
 
--- a/arch/powerpc/include/asm/barrier.h
+++ b/arch/powerpc/include/asm/barrier.h
@@ -74,8 +74,6 @@ do {									\
 	___p1;								\
 })
 
-#define smp_mb__before_spinlock()   smp_mb()
-
 #include <asm-generic/barrier.h>
 
 #endif /* _ASM_POWERPC_BARRIER_H */
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -109,27 +109,24 @@ static int userfaultfd_wake_function(wai
 		goto out;
 	WRITE_ONCE(uwq->waken, true);
 	/*
-	 * The implicit smp_mb__before_spinlock in try_to_wake_up()
-	 * renders uwq->waken visible to other CPUs before the task is
-	 * waken.
+	 * The Program-Order guarantees provided by the scheduler
+	 * ensure uwq->waken is visible before the task is woken.
 	 */
 	ret = wake_up_state(wq->private, mode);
-	if (ret)
+	if (ret) {
 		/*
 		 * Wake only once, autoremove behavior.
 		 *
-		 * After the effect of list_del_init is visible to the
-		 * other CPUs, the waitqueue may disappear from under
-		 * us, see the !list_empty_careful() in
-		 * handle_userfault(). try_to_wake_up() has an
-		 * implicit smp_mb__before_spinlock, and the
-		 * wq->private is read before calling the extern
-		 * function "wake_up_state" (which in turns calls
-		 * try_to_wake_up). While the spin_lock;spin_unlock;
-		 * wouldn't be enough, the smp_mb__before_spinlock is
-		 * enough to avoid an explicit smp_mb() here.
+		 * After the effect of list_del_init is visible to the other
+		 * CPUs, the waitqueue may disappear from under us, see the
+		 * !list_empty_careful() in handle_userfault().
+		 *
+		 * try_to_wake_up() has an implicit smp_mb(), and the
+		 * wq->private is read before calling the extern function
+		 * "wake_up_state" (which in turns calls try_to_wake_up).
 		 */
 		list_del_init(&wq->entry);
+	}
 out:
 	return ret;
 }
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -118,19 +118,6 @@ 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().
- */
-#ifndef smp_mb__before_spinlock
-#define smp_mb__before_spinlock()	smp_wmb()
-#endif
-
-/*
  * This barrier must provide two things:
  *
  *   - it must guarantee a STORE before the spin_lock() is ordered against a

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

#1660004 — [RFC][PATCH 5/5] powerpc: Remove SYNC from _switch

Now that the scheduler's rq->lock is RCsc and thus provides full
transitivity between scheduling actions. And since we cannot migrate
current, a task needs a switch-out and a switch-in in order to
migrate, in which case the RCsc provides all the ordering we need.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/powerpc/kernel/entry_64.S |    8 --------
 1 file changed, 8 deletions(-)

--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -488,14 +488,6 @@ _GLOBAL(_switch)
 	std	r23,_CCR(r1)
 	std	r1,KSP(r3)	/* Set old stack pointer */
 
-#ifdef CONFIG_SMP
-	/* We need a sync somewhere here to make sure that if the
-	 * previous task gets rescheduled on another CPU, it sees all
-	 * stores it has performed on this one.
-	 */
-	sync
-#endif /* CONFIG_SMP */
-
 	/*
 	 * If we optimise away the clear of the reservation in system
 	 * calls because we know the CPU tracks the address of the

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

#1660631 — Re: [RFC][PATCH 5/5] powerpc: Remove SYNC from _switch

On Wed, 07 Jun 2017 18:15:06 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> Now that the scheduler's rq->lock is RCsc and thus provides full
> transitivity between scheduling actions. And since we cannot migrate
> current, a task needs a switch-out and a switch-in in order to
> migrate, in which case the RCsc provides all the ordering we need.

Hi Peter,

I'm actually just working on removing this right now too, so
good timing.

I think we can't "just" remove it, because it is required to order
MMIO on powerpc as well.

But what I have done is to comment that some other primitives are
already providing the hwsync for other, so we don't have to add
another one in _switch.

Thanks,
Nick

> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/powerpc/kernel/entry_64.S |    8 --------
>  1 file changed, 8 deletions(-)
> 
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -488,14 +488,6 @@ _GLOBAL(_switch)
>  	std	r23,_CCR(r1)
>  	std	r1,KSP(r3)	/* Set old stack pointer */
>  
> -#ifdef CONFIG_SMP
> -	/* We need a sync somewhere here to make sure that if the
> -	 * previous task gets rescheduled on another CPU, it sees all
> -	 * stores it has performed on this one.
> -	 */
> -	sync
> -#endif /* CONFIG_SMP */
> -
>  	/*
>  	 * If we optimise away the clear of the reservation in system
>  	 * calls because we know the CPU tracks the address of the
> 
> 

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

#1660803 — Re: [RFC][PATCH 5/5] powerpc: Remove SYNC from _switch

On Thu, Jun 08, 2017 at 10:32:44AM +1000, Nicholas Piggin wrote:
> On Wed, 07 Jun 2017 18:15:06 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > Now that the scheduler's rq->lock is RCsc and thus provides full
> > transitivity between scheduling actions. And since we cannot migrate
> > current, a task needs a switch-out and a switch-in in order to
> > migrate, in which case the RCsc provides all the ordering we need.
> 
> Hi Peter,
> 
> I'm actually just working on removing this right now too, so
> good timing.
> 
> I think we can't "just" remove it, because it is required to order
> MMIO on powerpc as well.

How is MMIO special? That is, there is only MMIO before we call into
schedule() right? So the rq->lock should be sufficient to order that
too.

> 
> But what I have done is to comment that some other primitives are
> already providing the hwsync for other, so we don't have to add
> another one in _switch.

Right, so this patch relies on the smp_mb__before_spinlock ->
smp_mb__after_spinlock conversion that makes the rq->lock RCsc and
should thus provide the required SYNC for migrations.

That said, I think you can already use the smp_mb__before_spinlock() as
that is done with IRQs disabled, but its a more difficult argument. The
rq->lock RCsc property should be more obvious.

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

#1660841 — Re: [RFC][PATCH 5/5] powerpc: Remove SYNC from _switch

On Thu, 8 Jun 2017 08:54:00 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Jun 08, 2017 at 10:32:44AM +1000, Nicholas Piggin wrote:
> > On Wed, 07 Jun 2017 18:15:06 +0200
> > Peter Zijlstra <peterz@infradead.org> wrote:
> >   
> > > Now that the scheduler's rq->lock is RCsc and thus provides full
> > > transitivity between scheduling actions. And since we cannot migrate
> > > current, a task needs a switch-out and a switch-in in order to
> > > migrate, in which case the RCsc provides all the ordering we need.  
> > 
> > Hi Peter,
> > 
> > I'm actually just working on removing this right now too, so
> > good timing.
> > 
> > I think we can't "just" remove it, because it is required to order
> > MMIO on powerpc as well.  
> 
> How is MMIO special? That is, there is only MMIO before we call into
> schedule() right? So the rq->lock should be sufficient to order that
> too.

MMIO uses different barriers. spinlock and smp_ type barriers do
not order it.

> > 
> > But what I have done is to comment that some other primitives are
> > already providing the hwsync for other, so we don't have to add
> > another one in _switch.  
> 
> Right, so this patch relies on the smp_mb__before_spinlock ->
> smp_mb__after_spinlock conversion that makes the rq->lock RCsc and
> should thus provide the required SYNC for migrations.

AFAIKS either one will do, so long as there is a hwsync there. The
point is just that I have added some commentary in the generic and
powerpc parts to make it clear we're relying on that behavior of
the primitive. smp_mb* is not guaranteed to order MMIO, it's just
that it does on powerpc.

> That said, I think you can already use the smp_mb__before_spinlock() as
> that is done with IRQs disabled, but its a more difficult argument. The
> rq->lock RCsc property should be more obvious.

This is what I got.

https://patchwork.ozlabs.org/patch/770154/

But I'm not sure if I followed I'm not sure why it's a more
difficult argument: any time a process moves it must first execute
a hwsync on the current CPU after it has performed all its access
there, and then it must execute hwsync on the new CPU before it
performs any new access.

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

#1660885 — Re: [RFC][PATCH 5/5] powerpc: Remove SYNC from _switch

On Thu, Jun 08, 2017 at 05:29:38PM +1000, Nicholas Piggin wrote:
> On Thu, 8 Jun 2017 08:54:00 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Thu, Jun 08, 2017 at 10:32:44AM +1000, Nicholas Piggin wrote:
> > > On Wed, 07 Jun 2017 18:15:06 +0200
> > > Peter Zijlstra <peterz@infradead.org> wrote:
> > >   
> > > > Now that the scheduler's rq->lock is RCsc and thus provides full
> > > > transitivity between scheduling actions. And since we cannot migrate
> > > > current, a task needs a switch-out and a switch-in in order to
> > > > migrate, in which case the RCsc provides all the ordering we need.  
> > > 
> > > Hi Peter,
> > > 
> > > I'm actually just working on removing this right now too, so
> > > good timing.
> > > 
> > > I think we can't "just" remove it, because it is required to order
> > > MMIO on powerpc as well.  
> > 
> > How is MMIO special? That is, there is only MMIO before we call into
> > schedule() right? So the rq->lock should be sufficient to order that
> > too.
> 
> MMIO uses different barriers. spinlock and smp_ type barriers do
> not order it.

Right, but you only have SYNC, which is what makes it possible at all.

Some of the other architectures are not so lucky and need a different
barrier, ARM for instance needs DSB(ISH) vs the DMB(ISH) provided by
smp_mb(). IA64, MIPS and a few others are in the same boat as ARM.

> > > But what I have done is to comment that some other primitives are
> > > already providing the hwsync for other, so we don't have to add
> > > another one in _switch.  
> > 
> > Right, so this patch relies on the smp_mb__before_spinlock ->
> > smp_mb__after_spinlock conversion that makes the rq->lock RCsc and
> > should thus provide the required SYNC for migrations.
> 
> AFAIKS either one will do, so long as there is a hwsync there. The
> point is just that I have added some commentary in the generic and
> powerpc parts to make it clear we're relying on that behavior of
> the primitive. smp_mb* is not guaranteed to order MMIO, it's just
> that it does on powerpc.

I'm not particularly happy with the generic comment; I don't feel we
should care that PPC is special here.

> > That said, I think you can already use the smp_mb__before_spinlock() as
> > that is done with IRQs disabled, but its a more difficult argument. The
> > rq->lock RCsc property should be more obvious.
> 
> This is what I got.
> 
> https://patchwork.ozlabs.org/patch/770154/

Your comment isn't fully correct, smp_cond_load_acquire() isn't
necessarily done by CPUy. It might be easiest to simply refer to the
"Notes on Program-Order guarantees on SMP systems." comment.

> But I'm not sure if I followed I'm not sure why it's a more
> difficult argument: any time a process moves it must first execute
> a hwsync on the current CPU after it has performed all its access
> there, and then it must execute hwsync on the new CPU before it
> performs any new access.

Yeah, its not a terribly difficult argument either way, but I feel the
RSsc rq->lock on is slightly easier.

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

#1660929 — Re: [RFC][PATCH 5/5] powerpc: Remove SYNC from _switch

On Thu, 8 Jun 2017 09:57:20 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Jun 08, 2017 at 05:29:38PM +1000, Nicholas Piggin wrote:
> > On Thu, 8 Jun 2017 08:54:00 +0200
> > Peter Zijlstra <peterz@infradead.org> wrote:
> >   
> > > On Thu, Jun 08, 2017 at 10:32:44AM +1000, Nicholas Piggin wrote:  
> > > > On Wed, 07 Jun 2017 18:15:06 +0200
> > > > Peter Zijlstra <peterz@infradead.org> wrote:
> > > >     
> > > > > Now that the scheduler's rq->lock is RCsc and thus provides full
> > > > > transitivity between scheduling actions. And since we cannot migrate
> > > > > current, a task needs a switch-out and a switch-in in order to
> > > > > migrate, in which case the RCsc provides all the ordering we need.    
> > > > 
> > > > Hi Peter,
> > > > 
> > > > I'm actually just working on removing this right now too, so
> > > > good timing.
> > > > 
> > > > I think we can't "just" remove it, because it is required to order
> > > > MMIO on powerpc as well.    
> > > 
> > > How is MMIO special? That is, there is only MMIO before we call into
> > > schedule() right? So the rq->lock should be sufficient to order that
> > > too.  
> > 
> > MMIO uses different barriers. spinlock and smp_ type barriers do
> > not order it.  
> 
> Right, but you only have SYNC, which is what makes it possible at all.

Yeah, but a future CPU in theory could implement some other barrier
which provides hwsync ordering for cacheable memory but not uncacheable.
smp_mb* barriers would be able to use that new type of barrier, except
here.

> Some of the other architectures are not so lucky and need a different
> barrier, ARM for instance needs DSB(ISH) vs the DMB(ISH) provided by
> smp_mb(). IA64, MIPS and a few others are in the same boat as ARM.
> 
> > > > But what I have done is to comment that some other primitives are
> > > > already providing the hwsync for other, so we don't have to add
> > > > another one in _switch.    
> > > 
> > > Right, so this patch relies on the smp_mb__before_spinlock ->
> > > smp_mb__after_spinlock conversion that makes the rq->lock RCsc and
> > > should thus provide the required SYNC for migrations.  
> > 
> > AFAIKS either one will do, so long as there is a hwsync there. The
> > point is just that I have added some commentary in the generic and
> > powerpc parts to make it clear we're relying on that behavior of
> > the primitive. smp_mb* is not guaranteed to order MMIO, it's just
> > that it does on powerpc.  
> 
> I'm not particularly happy with the generic comment; I don't feel we
> should care that PPC is special here.

I think we do though, because its smp_mb happens to also order mmio.

Your patch I think failed to capture that unless I miss something. It's
not that the rq lock is RCsc that we can remove the hwsync, it's that
the smp_mb__before/after_spinlock has a hwsync in it.

As a counter-example: I think you can implement RCsc spinlocks in
powerpc using ll/sc+isync for the acquire, but that would be insufficient
because no hwsync for MMIO.

> > > That said, I think you can already use the smp_mb__before_spinlock() as
> > > that is done with IRQs disabled, but its a more difficult argument. The
> > > rq->lock RCsc property should be more obvious.  
> > 
> > This is what I got.
> > 
> > https://patchwork.ozlabs.org/patch/770154/  
> 
> Your comment isn't fully correct, smp_cond_load_acquire() isn't
> necessarily done by CPUy. It might be easiest to simply refer to the
> "Notes on Program-Order guarantees on SMP systems." comment.

True, thanks.

> > But I'm not sure if I followed I'm not sure why it's a more
> > difficult argument: any time a process moves it must first execute
> > a hwsync on the current CPU after it has performed all its access
> > there, and then it must execute hwsync on the new CPU before it
> > performs any new access.  
> 
> Yeah, its not a terribly difficult argument either way, but I feel the
> RSsc rq->lock on is slightly easier.

It is neater to have the barrier inside the lock, I think.

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

#1661010 — Re: [RFC][PATCH 5/5] powerpc: Remove SYNC from _switch

Peter Zijlstra <peterz@infradead.org> writes:
> On Thu, Jun 08, 2017 at 05:29:38PM +1000, Nicholas Piggin wrote:
>> On Thu, 8 Jun 2017 08:54:00 +0200
>> Peter Zijlstra <peterz@infradead.org> wrote:
>> > 
>> > Right, so this patch relies on the smp_mb__before_spinlock ->
>> > smp_mb__after_spinlock conversion that makes the rq->lock RCsc and
>> > should thus provide the required SYNC for migrations.
>> 
>> AFAIKS either one will do, so long as there is a hwsync there. The
>> point is just that I have added some commentary in the generic and
>> powerpc parts to make it clear we're relying on that behavior of
>> the primitive. smp_mb* is not guaranteed to order MMIO, it's just
>> that it does on powerpc.
>
> I'm not particularly happy with the generic comment; I don't feel we
> should care that PPC is special here.

I think it'd be nice if there was *some* comment on the two uses of
smp_mb__after_spinlock(), it's fairly subtle, but I don't think it needs
to mention PPC specifically.


If we have:

arch/powerpc/include/asm/barrier.h:
+/*
+ * This must resolve to hwsync on SMP for the context switch path. See
+ * _switch.
+ */
 #define smp_mb__after_spinlock()   smp_mb()


And then something in _switch() that says "we rely on the
smp_mb__after_spinlock() in the scheduler core being a hwsync", that
should probably be sufficient.

cheers

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

#1661032 — Re: [RFC][PATCH 5/5] powerpc: Remove SYNC from _switch

On Thu, 08 Jun 2017 19:54:30 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Peter Zijlstra <peterz@infradead.org> writes:
> > On Thu, Jun 08, 2017 at 05:29:38PM +1000, Nicholas Piggin wrote:  
> >> On Thu, 8 Jun 2017 08:54:00 +0200
> >> Peter Zijlstra <peterz@infradead.org> wrote:  
> >> > 
> >> > Right, so this patch relies on the smp_mb__before_spinlock ->
> >> > smp_mb__after_spinlock conversion that makes the rq->lock RCsc and
> >> > should thus provide the required SYNC for migrations.  
> >> 
> >> AFAIKS either one will do, so long as there is a hwsync there. The
> >> point is just that I have added some commentary in the generic and
> >> powerpc parts to make it clear we're relying on that behavior of
> >> the primitive. smp_mb* is not guaranteed to order MMIO, it's just
> >> that it does on powerpc.  
> >
> > I'm not particularly happy with the generic comment; I don't feel we
> > should care that PPC is special here.  
> 
> I think it'd be nice if there was *some* comment on the two uses of
> smp_mb__after_spinlock(), it's fairly subtle, but I don't think it needs
> to mention PPC specifically.
> 
> 
> If we have:
> 
> arch/powerpc/include/asm/barrier.h:
> +/*
> + * This must resolve to hwsync on SMP for the context switch path. See
> + * _switch.
> + */
>  #define smp_mb__after_spinlock()   smp_mb()
> 
> 
> And then something in _switch() that says "we rely on the
> smp_mb__after_spinlock() in the scheduler core being a hwsync", that
> should probably be sufficient.

I have those, I just also would like one in the core scheduler's use
of smp_mb__after_spinlock(), because it would be easy for core scheduler
change to miss that quirk. Sure we can say that Peter and scheduler
maintainers know about powerpc oddities, but then why shouldn't it also
go into a comment there?

Thanks,
Nick

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

#1661135 — Re: [RFC][PATCH 5/5] powerpc: Remove SYNC from _switch

On Thu, Jun 08, 2017 at 08:00:15PM +1000, Nicholas Piggin wrote:

> I have those, I just also would like one in the core scheduler's use
> of smp_mb__after_spinlock(), because it would be easy for core scheduler
> change to miss that quirk. Sure we can say that Peter and scheduler
> maintainers know about powerpc oddities, but then why shouldn't it also
> go into a comment there?

So the core scheduler guarantees smp_mb() or equivalent full transitive
ordering happens at schedule() time.

It has for a fairly long time and I don't think we'll ever get rid of
that, its fairly fundamental.

PPC is special in that smp_mb() ends up being the strongest ordering
primitive there is. But note that PPC is not unique, afaict Alpha is in
the same boat. They rely on the MB from the scheduler core.

IA64 OTOH, while they have smp_mb() == mb() still needs SYNC.I in
__switch_to() to serialize against (instruction) cache flushes.

So while I'm all for adding comments explaining what the core provides,
I don't see immediate reasons to call out PPC.

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

#1661166 — Re: [RFC][PATCH 5/5] powerpc: Remove SYNC from _switch

On Thu, 8 Jun 2017 14:45:40 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Jun 08, 2017 at 08:00:15PM +1000, Nicholas Piggin wrote:
> 
> > I have those, I just also would like one in the core scheduler's use
> > of smp_mb__after_spinlock(), because it would be easy for core scheduler
> > change to miss that quirk. Sure we can say that Peter and scheduler
> > maintainers know about powerpc oddities, but then why shouldn't it also
> > go into a comment there?  
> 
> So the core scheduler guarantees smp_mb() or equivalent full transitive
> ordering happens at schedule() time.
> 
> It has for a fairly long time and I don't think we'll ever get rid of
> that, its fairly fundamental.
> 
> PPC is special in that smp_mb() ends up being the strongest ordering
> primitive there is. But note that PPC is not unique, afaict Alpha is in
> the same boat. They rely on the MB from the scheduler core.
> 
> IA64 OTOH, while they have smp_mb() == mb() still needs SYNC.I in
> __switch_to() to serialize against (instruction) cache flushes.
> 
> So while I'm all for adding comments explaining what the core provides,
> I don't see immediate reasons to call out PPC.

I guess I see your point... okay, will constrain the comment to powerpc
context switch and primitives code. Any fundamental change to such
scheduler barriers I guess would require at least a glance over arch
switch code :)

My plan is to send the powerpc sync removal patch for hopefully 4.13
merge. I'm pretty sure it will be equally happy with your patches.
Unless you can see any problems with it? More eyes would be welcome.

Thanks,
Nick

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

#1661223 — Re: [RFC][PATCH 5/5] powerpc: Remove SYNC from _switch

On Thu, Jun 08, 2017 at 11:18:13PM +1000, Nicholas Piggin wrote:
> I guess I see your point... okay, will constrain the comment to powerpc
> context switch and primitives code. Any fundamental change to such
> scheduler barriers I guess would require at least a glance over arch
> switch code :)

Just so.

> My plan is to send the powerpc sync removal patch for hopefully 4.13
> merge. I'm pretty sure it will be equally happy with your patches.
> Unless you can see any problems with it? More eyes would be welcome.

Feel free to Cc me. I don't see a problem with removing that SYNC now,
I'll rebase my patches on top.

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

#1662543

On Wed, Jun 07, 2017 at 06:15:01PM +0200, Peter Zijlstra wrote:
> There was a thread on this somewhere about a year ago, I finally remembered to
> finish this :-)
> 
> This series removes two smp_mb__before_spinlock() (ab)users and converts the
> scheduler to use smp_mb__after_spinlock(), which provides more guarantees with
> the same amount of barriers.

For the arm64 bits:

Acked-by: Will Deacon <will.deacon@arm.com>

Will

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web