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

Groups > linux.kernel > #1702043 > unrolled thread

[PATCH -v2 0/4] Getting rid of smp_mb__before_spinlock

Back to article view | Back to linux.kernel

Contents

  [PATCH -v2 0/4] Getting rid of smp_mb__before_spinlock Peter Zijlstra <peterz@infradead.org> - 2017-08-02 13:50 +0200
    [PATCH -v2 1/4] mm: Rework {set,clear,mm}_tlb_flush_pending() Peter Zijlstra <peterz@infradead.org> - 2017-08-02 13:50 +0200
      Re: [PATCH -v2 1/4] mm: Rework {set,clear,mm}_tlb_flush_pending() Peter Zijlstra <peterz@infradead.org> - 2017-08-02 15:10 +0200
        Re: [PATCH -v2 1/4] mm: Rework {set,clear,mm}_tlb_flush_pending() Mel Gorman <mgorman@suse.de> - 2017-08-02 16:00 +0200
          Re: [PATCH -v2 1/4] mm: Rework {set,clear,mm}_tlb_flush_pending() Peter Zijlstra <peterz@infradead.org> - 2017-08-02 16:20 +0200
      Re: [PATCH -v2 1/4] mm: Rework {set,clear,mm}_tlb_flush_pending() Mel Gorman <mgorman@suse.de> - 2017-08-02 15:10 +0200
      ARC stuff (was Re: [PATCH -v2 1/4] mm: Rework  {set,clear,mm}_tlb_flush_pending()) Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2017-08-02 15:10 +0200
        Re: ARC stuff (was Re: [PATCH -v2 1/4] mm: Rework  {set,clear,mm}_tlb_flush_pending()) Peter Zijlstra <peterz@infradead.org> - 2017-08-02 15:20 +0200
          Re: ARC stuff (was Re: [PATCH -v2 1/4] mm: Rework  {set,clear,mm}_tlb_flush_pending()) Peter Zijlstra <peterz@infradead.org> - 2017-08-11 16:20 +0200
      Re: [PATCH -v2 1/4] mm: Rework {set,clear,mm}_tlb_flush_pending() Will Deacon <will.deacon@arm.com> - 2017-08-03 17:30 +0200
      Re: [PATCH -v2 1/4] mm: Rework {set,clear,mm}_tlb_flush_pending() Peter Zijlstra <peterz@infradead.org> - 2017-08-11 11:50 +0200

#1702043 — [PATCH -v2 0/4] Getting rid of smp_mb__before_spinlock

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]

#1702044 — [PATCH -v2 1/4] 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().

NOTE: this relies on flush_tlb_range() to guarantee:

   (1) it ensures that prior page table updates are visible to the
       page table walker and
   (2) it ensures that subsequent memory accesses are only made
       visible after the invalidation has completed

This is required for architectures that implement TRANSPARENT_HUGEPAGE
(arc, arm, arm64, mips, powerpc, s390, sparc, x86) or otherwise use
mm_tlb_flush_pending() in their page-table operations (arm, arm64,
x86).

This appears true for:

 - arm (DSB ISB before and after),
 - arm64 (DSB ISHST before, and DSB ISH after),
 - powerpc (PTESYNC before and after),
 - s390 and x86 TLB invalidate are serializing instructions

But I failed to understand the situation for:

 - arc, mips, sparc

Now SPARC64 is a wee bit special in that flush_tlb_range() is a no-op
and it flushes the TLBs using arch_{enter,leave}_lazy_mmu_mode()
inside the PTL. It still needs to guarantee the PTL unlock happens
_after_ the invalidate completes.

Vineet, Ralf and Dave could you guys please have a look?

Cc: Russell King <linux@armlinux.org.uk>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Vineet Gupta <vgupta@synopsys.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Rik van Riel <riel@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/mm_types.h |   33 +++++++++++++++++++++++++++------
 mm/huge_memory.c         |   20 ++++++++++++++++++++
 mm/migrate.c             |    6 ------
 3 files changed, 47 insertions(+), 12 deletions(-)

--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -527,23 +527,44 @@ 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
+	 * The only time this value is relevant is when there are indeed pages
+	 * to flush. And we'll only flush pages after changing them, which
+	 * requires the PTL.
+	 *
+	 * So the ordering here is:
+	 *
+	 *	mm->tlb_flush_pending = true;
+	 *	spin_lock(&ptl);
+	 *	...
+	 *	set_pte_at();
+	 *	spin_unlock(&ptl);
+	 *
+	 *				spin_lock(&ptl)
+	 *				mm_tlb_flush_pending();
+	 *				....
+	 *				spin_unlock(&ptl);
+	 *
+	 *	flush_tlb_range();
+	 *	mm->tlb_flush_pending = false;
+	 *
+	 * So the =true store is constrained by the PTL unlock, and the =false
+	 * store is constrained by the TLB invalidate.
 	 */
-	smp_mb__before_spinlock();
 }
 /* Clearing is done after a TLB flush, which also provides a barrier. */
 static inline void clear_tlb_flush_pending(struct mm_struct *mm)
 {
-	barrier();
+	/* see set_tlb_flush_pending */
 	mm->tlb_flush_pending = false;
 }
 #else
--- 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;
@@ -1496,10 +1497,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(vma->vm_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
@@ -1937,12 +1937,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]

#1702091 — Re: [PATCH -v2 1/4] mm: Rework {set,clear,mm}_tlb_flush_pending()

On Wed, Aug 02, 2017 at 02:00:40PM +0100, Mel Gorman wrote:
> On Wed, Aug 02, 2017 at 01:38:38PM +0200, Peter Zijlstra wrote:
> > 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().
> > 
> 
> Note that Nadav has a patch in parallel on it's way towards Andrew's
> tree that I suggested to always always check if a TLB flush is pending
> under the PTL. A conflict will happen but will be trivial to resolve.

Got a link?

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

#1702136 — Re: [PATCH -v2 1/4] mm: Rework {set,clear,mm}_tlb_flush_pending()

On Wed, Aug 02, 2017 at 03:05:50PM +0200, Peter Zijlstra wrote:
> On Wed, Aug 02, 2017 at 02:00:40PM +0100, Mel Gorman wrote:
> > On Wed, Aug 02, 2017 at 01:38:38PM +0200, Peter Zijlstra wrote:
> > > 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().
> > > 
> > 
> > Note that Nadav has a patch in parallel on it's way towards Andrew's
> > tree that I suggested to always always check if a TLB flush is pending
> > under the PTL. A conflict will happen but will be trivial to resolve.
> 
> Got a link?

Subject: Revert "mm: numa: defer TLB flush for THP migration as long as possible"

http://lkml.kernel.org/r/20170731164325.235019-4-namit@vmware.com


-- 
Mel Gorman
SUSE Labs

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

#1702150 — Re: [PATCH -v2 1/4] mm: Rework {set,clear,mm}_tlb_flush_pending()

On Wed, Aug 02, 2017 at 02:52:03PM +0100, Mel Gorman wrote:
> On Wed, Aug 02, 2017 at 03:05:50PM +0200, Peter Zijlstra wrote:
> > On Wed, Aug 02, 2017 at 02:00:40PM +0100, Mel Gorman wrote:
> > > On Wed, Aug 02, 2017 at 01:38:38PM +0200, Peter Zijlstra wrote:
> > > > 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().
> > > > 
> > > 
> > > Note that Nadav has a patch in parallel on it's way towards Andrew's
> > > tree that I suggested to always always check if a TLB flush is pending
> > > under the PTL. A conflict will happen but will be trivial to resolve.
> > 
> > Got a link?
> 
> Subject: Revert "mm: numa: defer TLB flush for THP migration as long as possible"
> 
> http://lkml.kernel.org/r/20170731164325.235019-4-namit@vmware.com
> 

Thanks, yes that should be easy to resolve.

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

#1702092 — Re: [PATCH -v2 1/4] mm: Rework {set,clear,mm}_tlb_flush_pending()

On Wed, Aug 02, 2017 at 01:38:38PM +0200, Peter Zijlstra wrote:
> 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().
> 

Note that Nadav has a patch in parallel on it's way towards Andrew's
tree that I suggested to always always check if a TLB flush is pending
under the PTL. A conflict will happen but will be trivial to resolve.

-- 
Mel Gorman
SUSE Labs

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

#1702097 — ARC stuff (was Re: [PATCH -v2 1/4] mm: Rework {set,clear,mm}_tlb_flush_pending())

On 08/02/2017 05:19 PM, 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().
>
> NOTE: this relies on flush_tlb_range() to guarantee:
>
>     (1) it ensures that prior page table updates are visible to the
>         page table walker and

ARC doesn't have hw page walker so this is not relevant.

>     (2) it ensures that subsequent memory accesses are only made
>         visible after the invalidation has completed

flush_tlb_range() does a bunch of aux register accesses, I need to check with hw 
folks if those can be assumed to serializing w.r.t. memory ordering.
But if not then we need to add an explicit smb barrier (which will not be paired ? 
) and would be penalizing the other callers of flush_tlb_range(). Will a new API 
for this be an overkill ? Is a memory barrier needed here anyways - like ARM !


>
> This is required for architectures that implement TRANSPARENT_HUGEPAGE
> (arc, arm, arm64, mips, powerpc, s390, sparc, x86) or otherwise use
> mm_tlb_flush_pending() in their page-table operations (arm, arm64,
> x86).
>
> This appears true for:
>
>   - arm (DSB ISB before and after),
>   - arm64 (DSB ISHST before, and DSB ISH after),
>   - powerpc (PTESYNC before and after),
>   - s390 and x86 TLB invalidate are serializing instructions
>
> But I failed to understand the situation for:
>
>   - arc, mips, sparc
>
> Now SPARC64 is a wee bit special in that flush_tlb_range() is a no-op
> and it flushes the TLBs using arch_{enter,leave}_lazy_mmu_mode()
> inside the PTL. It still needs to guarantee the PTL unlock happens
> _after_ the invalidate completes.
>
> Vineet, Ralf and Dave could you guys please have a look?
>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Vineet Gupta <vgupta@synopsys.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Rik van Riel <riel@redhat.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>   include/linux/mm_types.h |   33 +++++++++++++++++++++++++++------
>   mm/huge_memory.c         |   20 ++++++++++++++++++++
>   mm/migrate.c             |    6 ------
>   3 files changed, 47 insertions(+), 12 deletions(-)
>
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -527,23 +527,44 @@ 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
> +	 * The only time this value is relevant is when there are indeed pages
> +	 * to flush. And we'll only flush pages after changing them, which
> +	 * requires the PTL.
> +	 *
> +	 * So the ordering here is:
> +	 *
> +	 *	mm->tlb_flush_pending = true;
> +	 *	spin_lock(&ptl);
> +	 *	...
> +	 *	set_pte_at();
> +	 *	spin_unlock(&ptl);
> +	 *
> +	 *				spin_lock(&ptl)
> +	 *				mm_tlb_flush_pending();
> +	 *				....
> +	 *				spin_unlock(&ptl);
> +	 *
> +	 *	flush_tlb_range();
> +	 *	mm->tlb_flush_pending = false;
> +	 *
> +	 * So the =true store is constrained by the PTL unlock, and the =false
> +	 * store is constrained by the TLB invalidate.
>   	 */
> -	smp_mb__before_spinlock();
>   }
>   /* Clearing is done after a TLB flush, which also provides a barrier. */
>   static inline void clear_tlb_flush_pending(struct mm_struct *mm)
>   {
> -	barrier();
> +	/* see set_tlb_flush_pending */
>   	mm->tlb_flush_pending = false;
>   }
>   #else
> --- 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;
> @@ -1496,10 +1497,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(vma->vm_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
> @@ -1937,12 +1937,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]

#1702109 — Re: ARC stuff (was Re: [PATCH -v2 1/4] mm: Rework {set,clear,mm}_tlb_flush_pending())

On Wed, Aug 02, 2017 at 06:30:43PM +0530, Vineet Gupta wrote:
> On 08/02/2017 05:19 PM, 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().
> > 
> > NOTE: this relies on flush_tlb_range() to guarantee:
> > 
> >     (1) it ensures that prior page table updates are visible to the
> >         page table walker and
> 
> ARC doesn't have hw page walker so this is not relevant.

Well, you still want your software walker to observe the new PTE entries
before you start shooting down the old ones. So I would expect at least
an smp_wmb() before the TLB invalidate to order against another CPU
doing a software TLB fill, such that the other CPU will indeed observe
the new PTE after it has observed the TLB missing.

> >     (2) it ensures that subsequent memory accesses are only made
> >         visible after the invalidation has completed
> 
> flush_tlb_range() does a bunch of aux register accesses, I need to check
> with hw folks if those can be assumed to serializing w.r.t. memory ordering.
> But if not then we need to add an explicit smb barrier (which will not be
> paired ? )

It would pair with the ACQUIRE from the PTL in the below example.

> and would be penalizing the other callers of flush_tlb_range().
> Will a new API for this be an overkill ? Is a memory barrier needed here
> anyways - like ARM !

It is needed at the very least if you do transparant huge pages as per
the existing logic (this requirement isn't new per this patch, I was
just the silly person wondering if flush_tlb_range() does indeed provide
the ordering assumed).

But yes, lots of architectures do provide this ordering already and
some, like ARM and PPC do so with quite expensive barriers.

To me it's also a natural / expected ordering, but that could just be
me :-)

> >   	/*
> > +	 * The only time this value is relevant is when there are indeed pages
> > +	 * to flush. And we'll only flush pages after changing them, which
> > +	 * requires the PTL.
> > +	 *
> > +	 * So the ordering here is:
> > +	 *
> > +	 *	mm->tlb_flush_pending = true;
> > +	 *	spin_lock(&ptl);
> > +	 *	...
> > +	 *	set_pte_at();
> > +	 *	spin_unlock(&ptl);
> > +	 *
> > +	 *				spin_lock(&ptl)
> > +	 *				mm_tlb_flush_pending();
> > +	 *				....
> > +	 *				spin_unlock(&ptl);
> > +	 *
> > +	 *	flush_tlb_range();
> > +	 *	mm->tlb_flush_pending = false;
> > +	 *
> > +	 * So the =true store is constrained by the PTL unlock, and the =false
> > +	 * store is constrained by the TLB invalidate.
> >   	 */
> >   }
> >   /* Clearing is done after a TLB flush, which also provides a barrier. */

See, not a new requirement.. I only mucked with the ordering for
setting it, clearing already relied on the flush_tlb_range().

> >   static inline void clear_tlb_flush_pending(struct mm_struct *mm)
> >   {
> > +	/* see set_tlb_flush_pending */
> >   	mm->tlb_flush_pending = false;
> >   }

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

#1709666 — Re: ARC stuff (was Re: [PATCH -v2 1/4] mm: Rework {set,clear,mm}_tlb_flush_pending())

On Wed, Aug 02, 2017 at 03:17:10PM +0200, Peter Zijlstra wrote:
> On Wed, Aug 02, 2017 at 06:30:43PM +0530, Vineet Gupta wrote:
> > flush_tlb_range() does a bunch of aux register accesses, I need to check
> > with hw folks if those can be assumed to serializing w.r.t. memory ordering.
> > But if not then we need to add an explicit smb barrier (which will not be
> > paired ? )
> 
> It would pair with the ACQUIRE from the PTL in the below example.
> 
> > and would be penalizing the other callers of flush_tlb_range().
> > Will a new API for this be an overkill ? Is a memory barrier needed here
> > anyways - like ARM !
> 
> It is needed at the very least if you do transparant huge pages as per
> the existing logic (this requirement isn't new per this patch, I was
> just the silly person wondering if flush_tlb_range() does indeed provide
> the ordering assumed).

Any word on this? It just got way worse and anything SMP needs to
provide this.

See commit:

  0a2dd266dd6b ("mm: make tlb_flush_pending global")

And these semantics are now required for the correct operation of KSM
and MADV_{FREE,DONT_NEED}.

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

#1703199 — Re: [PATCH -v2 1/4] mm: Rework {set,clear,mm}_tlb_flush_pending()

On Wed, Aug 02, 2017 at 01:38:38PM +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().
> 
> NOTE: this relies on flush_tlb_range() to guarantee:
> 
>    (1) it ensures that prior page table updates are visible to the
>        page table walker and
>    (2) it ensures that subsequent memory accesses are only made
>        visible after the invalidation has completed

Works for me:

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

Will

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

#1709421 — Re: [PATCH -v2 1/4] mm: Rework {set,clear,mm}_tlb_flush_pending()

On Wed, Aug 02, 2017 at 01:38:38PM +0200, Peter Zijlstra wrote:
>  	/*
> +	 * The only time this value is relevant is when there are indeed pages
> +	 * to flush. And we'll only flush pages after changing them, which
> +	 * requires the PTL.
> +	 *
> +	 * So the ordering here is:
> +	 *
> +	 *	mm->tlb_flush_pending = true;
> +	 *	spin_lock(&ptl);
> +	 *	...
> +	 *	set_pte_at();
> +	 *	spin_unlock(&ptl);
> +	 *
> +	 *				spin_lock(&ptl)
> +	 *				mm_tlb_flush_pending();
> +	 *				....

Crud, so while I was rebasing Nadav's patches I realized that this does
not in fact work for PPC and split PTL. Because the PPC lwsync relies on
the address dependency to actual produce the ordering.

Also, since Nadav switched to atomic_inc/atomic_dec, I'll send a patch
to add smp_mb__after_atomic(), and

> +	 *				spin_unlock(&ptl);
> +	 *
> +	 *	flush_tlb_range();
> +	 *	mm->tlb_flush_pending = false;
> +	 *
> +	 * So the =true store is constrained by the PTL unlock, and the =false
> +	 * store is constrained by the TLB invalidate.
>  	 */
>  }
>  /* Clearing is done after a TLB flush, which also provides a barrier. */
>  static inline void clear_tlb_flush_pending(struct mm_struct *mm)
>  {
> +	/* see set_tlb_flush_pending */

smp_mb__before_atomic() here. That also avoids the whole reliance on the
tlb_flush nonsense.

It will overstuff on barriers on some platforms though :/

>  	mm->tlb_flush_pending = false;
>  }

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web