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

Groups > linux.kernel > #1190129 > unrolled thread

[PATCH] mm: Flush the TLB for a single address in a huge page

Back to article view | Back to linux.kernel

Contents

  [PATCH] mm: Flush the TLB for a single address in a huge page Catalin Marinas <catalin.marinas@arm.com> - 2015-07-22 19:30 +0200
    Re: [PATCH] mm: Flush the TLB for a single address in a huge page Catalin Marinas <catalin.marinas@arm.com> - 2015-07-23 00:50 +0200
      Re: [PATCH] mm: Flush the TLB for a single address in a huge page Dave Hansen <dave.hansen@intel.com> - 2015-07-23 01:10 +0200
        Re: [PATCH] mm: Flush the TLB for a single address in a huge page Andrea Arcangeli <aarcange@redhat.com> - 2015-07-23 16:20 +0200
          Re: [PATCH] mm: Flush the TLB for a single address in a huge page Andrea Arcangeli <aarcange@redhat.com> - 2015-07-23 19:20 +0200

#1190129 — [PATCH] mm: Flush the TLB for a single address in a huge page

When the page table entry is a huge page (and not a table), there is no
need to flush the TLB by range. This patch changes flush_tlb_range() to
flush_tlb_page() in functions where we know the pmd entry is a huge
page.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andrea Arcangeli <aarcange@redhat.com>
---

Hi,

That's just a minor improvement but it saves iterating over each small
page in a huge page when a single TLB entry is used (we already have a
similar assumption in __tlb_adjust_range).

Thanks.

 mm/pgtable-generic.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c
index 6b674e00153c..ff17eca26211 100644
--- a/mm/pgtable-generic.c
+++ b/mm/pgtable-generic.c
@@ -67,7 +67,7 @@ int pmdp_set_access_flags(struct vm_area_struct *vma,
 	VM_BUG_ON(address & ~HPAGE_PMD_MASK);
 	if (changed) {
 		set_pmd_at(vma->vm_mm, address, pmdp, entry);
-		flush_tlb_range(vma, address, address + HPAGE_PMD_SIZE);
+		flush_tlb_page(vma, address);
 	}
 	return changed;
 #else /* CONFIG_TRANSPARENT_HUGEPAGE */
@@ -101,7 +101,7 @@ int pmdp_clear_flush_young(struct vm_area_struct *vma,
 #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
 	young = pmdp_test_and_clear_young(vma, address, pmdp);
 	if (young)
-		flush_tlb_range(vma, address, address + HPAGE_PMD_SIZE);
+		flush_tlb_page(vma, address);
 	return young;
 }
 #endif
@@ -128,7 +128,7 @@ pmd_t pmdp_huge_clear_flush(struct vm_area_struct *vma, unsigned long address,
 	VM_BUG_ON(address & ~HPAGE_PMD_MASK);
 	VM_BUG_ON(!pmd_trans_huge(*pmdp));
 	pmd = pmdp_huge_get_and_clear(vma->vm_mm, address, pmdp);
-	flush_tlb_range(vma, address, address + HPAGE_PMD_SIZE);
+	flush_tlb_page(vma, address);
 	return pmd;
 }
 #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
@@ -143,7 +143,7 @@ void pmdp_splitting_flush(struct vm_area_struct *vma, unsigned long address,
 	VM_BUG_ON(address & ~HPAGE_PMD_MASK);
 	set_pmd_at(vma->vm_mm, address, pmdp, pmd);
 	/* tlb flush only to serialize against gup-fast */
-	flush_tlb_range(vma, address, address + HPAGE_PMD_SIZE);
+	flush_tlb_page(vma, address);
 }
 #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
 #endif
@@ -195,7 +195,7 @@ void pmdp_invalidate(struct vm_area_struct *vma, unsigned long address,
 {
 	pmd_t entry = *pmdp;
 	set_pmd_at(vma->vm_mm, address, pmdp, pmd_mknotpresent(entry));
-	flush_tlb_range(vma, address, address + HPAGE_PMD_SIZE);
+	flush_tlb_page(vma, address);
 }
 #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
 #endif
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1190342

On 22 July 2015 at 22:39, David Rientjes <rientjes@google.com> wrote:
> On Wed, 22 Jul 2015, Catalin Marinas wrote:
>
>> When the page table entry is a huge page (and not a table), there is no
>> need to flush the TLB by range. This patch changes flush_tlb_range() to
>> flush_tlb_page() in functions where we know the pmd entry is a huge
>> page.
>>
>> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Andrea Arcangeli <aarcange@redhat.com>
>> ---
>>
>> Hi,
>>
>> That's just a minor improvement but it saves iterating over each small
>> page in a huge page when a single TLB entry is used (we already have a
>> similar assumption in __tlb_adjust_range).
>
> For x86 smp, this seems to mean the difference between unconditional
> flush_tlb_page() and local_flush_tlb() due to
> tlb_single_page_flush_ceiling, so I don't think this just removes the
> iteration.

You are right, on x86 the tlb_single_page_flush_ceiling seems to be
33, so for an HPAGE_SIZE range the code does a local_flush_tlb()
always. I would say a single page TLB flush is more efficient than a
whole TLB flush but I'm not familiar enough with x86.

Alternatively, I could introduce a flush_tlb_pmd_huge_page (suggested
by Andrea separately) and let the architectures deal with this as they
see fit. The default definition would do a flush_tlb_range(vma,
address, address + HPAGE_SIZE). For arm64, I'll define it as
flush_tlb_page(vma, address).

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1190348

On 07/22/2015 03:48 PM, Catalin Marinas wrote:
> You are right, on x86 the tlb_single_page_flush_ceiling seems to be
> 33, so for an HPAGE_SIZE range the code does a local_flush_tlb()
> always. I would say a single page TLB flush is more efficient than a
> whole TLB flush but I'm not familiar enough with x86.

The last time I looked, the instruction to invalidate a single page is
more expensive than the instruction to flush the entire TLB.  We also
don't bother doing ranged flushes _ever_ for hugetlbfs TLB
invalidations, but that was just because the work done around commit
e7b52ffd4 didn't see any benefit.

That said, I can't imagine this will hurt anything.  We also have TLBs
that can mix 2M and 4k pages and I don't think we did back when we put
that code in originally.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1190951

On Thu, Jul 23, 2015 at 11:49:38AM +0100, Catalin Marinas wrote:
> On Thu, Jul 23, 2015 at 12:05:21AM +0100, Dave Hansen wrote:
> > On 07/22/2015 03:48 PM, Catalin Marinas wrote:
> > > You are right, on x86 the tlb_single_page_flush_ceiling seems to be
> > > 33, so for an HPAGE_SIZE range the code does a local_flush_tlb()
> > > always. I would say a single page TLB flush is more efficient than a
> > > whole TLB flush but I'm not familiar enough with x86.
> > 
> > The last time I looked, the instruction to invalidate a single page is
> > more expensive than the instruction to flush the entire TLB. 
> 
> I was thinking of the overall cost of re-populating the TLB after being
> nuked rather than the instruction itself.

Unless I'm not aware about timing differences in flushing 2MB TLB
entries vs flushing 4kb TLB entries with invlpg, the benchmarks that
have been run to tune the optimal tlb_single_page_flush_ceiling value,
should already guarantee us that this is a valid optimization (as we
just got one entry, we're not even close to the 33 ceiling that makes
it more a grey area).

> > That said, I can't imagine this will hurt anything.  We also have TLBs
> > that can mix 2M and 4k pages and I don't think we did back when we put
> > that code in originally.

Dave, I'm confused about this. We should still stick to an invariant
that we can't ever mix 2M and 4k TLB entries if their mappings end up
overlapping on the same physical memory (if this isn't enforced in
common code, some x86 implementation errata triggers, and it really
oopses with machine checks so it's not just theoretical). Perhaps I
misunderstood what you meant with mix 2M and 4k pages though.

> Another question is whether flushing a single address is enough for a
> huge page. I assumed it is since tlb_remove_pmd_tlb_entry() only adjusts

That's the primary reason why the range flush was used currently (and
it must be still used in pmdp_collapse_flush as that deals with 4k TLB
entries, but your patch correctly isn't touching that one).

I recall having used flush_tlb_page initially for the 2MB invalidates,
but then I switched to the range version purely to be safer. If we can
optimize this now I'd certainly be happy about that. Back then there
was not yet tlb_remove_pmd_tlb_entry which already started to optimize
things for this.

> the mmu_gather range by PAGE_SIZE (rather than HPAGE_SIZE) and
> no-one complained so far. AFAICT, there are only 3 architectures
> that don't use asm-generic/tlb.h but they all seem to handle this
> case:

Agreed that archs using the generic tlb.h that sets the tlb->end to
address+PAGE_SIZE should be fine with the flush_tlb_page.

> arch/arm: it implements tlb_remove_pmd_tlb_entry() in a similar way to
> the generic one
> 
> arch/s390: tlb_remove_pmd_tlb_entry() is a no-op

I guess s390 is fine too but I'm not convinced that the fact it won't
adjust the tlb->start/end is a guarantees that flush_tlb_page is
enough when a single 2MB TLB has to be invalidated (not during range
zapping).

For the range zapping, could the arch decide to unconditionally flush
the whole TLB without doing the tlb->start/end tracking by overriding
tlb_gather_mmu in a way that won't call __tlb_reset_range? There seems
to be quite some flexibility in the per-arch tlb_gather_mmu setup in
order to unconditionally set tlb->start/end to the total range zapped,
without actually narrowing it down during the pagetable walk.

This is why I was thinking a flush_tlb_pmd_huge_page might have been
safer. However if hugetlbfs is basically assuming flush_tlb_page works
like Dave said, and if s390 is fine as well, I think we can just apply
this patch which follows the generic tlb_remove_pmd_tlb_entry
optimization.

Thanks,
Andrea
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1191156

On Thu, Jul 23, 2015 at 09:55:33AM -0700, Dave Hansen wrote:
> On 07/23/2015 09:16 AM, Catalin Marinas wrote:
> > Anyway, if you want to keep the option of a full TLB flush for x86 on
> > huge pages, I'm happy to repost a v2 with a separate
> > flush_tlb_pmd_huge_page that arch code can define as it sees fit.
> 
> I think your patch is fine on x86.  We need to keep an eye out for any
> regressions, but I think it's OK.

That's my view as well.

I've read more of the other thread and I quote Ingo:

" It barely makes sense for a 2 pages and gets exponentially
worse. It's probably done in microcode and its performance is
horrible. "

So in our case it's just 1 page (not 2, not 33), and considering it
prevents to invalidate all other TLB entries, it's most certainly a
win: it requires zero additional infrastructure and best of all it can
also avoid to flush the entire TLB for remote CPUs too again without
infrastructure or pfn arrays or multiple invlpg.

As further confirmation that for 1 entry invlpg is worth it, even
flush_tlb_page->flush_tlb_func invokes __flush_tlb_single in the IPI
handler instead of local_flush_tlb().

So the discussion there was about the additional infrastructure and a
flood of invlpg, perhaps more than 33, I agree a local_flush_tlb()
sounds better for that.

The question left for x86 is if invlpg is even slower for 2MB pages
than it is for 4k pages, but I'd be surprised if it is, especially on
newer CPUs where the TLB can use different page size for each TLB
entry. Why we didn't do flush_tlb_page before wasn't related to such a
concern at least.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web