* Re: [PATCH v2] sparc64: Reduce TLB flushes during hugepage unmap
2016-02-03 23:00 [PATCH v2] sparc64: Reduce TLB flushes during hugepage unmap Nitin Gupta
@ 2016-02-09 18:33 ` Nitin Gupta
2016-02-09 22:12 ` David Miller
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Nitin Gupta @ 2016-02-09 18:33 UTC (permalink / raw)
To: sparclinux
Gentle reminder for review comments.
Also adding Naoya to CC whom I forgot last time, sorry.
Thanks,
Nitin
On 02/03/2016 03:00 PM, Nitin Gupta wrote:
> During hugepage unmap, TSB and TLB flushes are currently
> issued at every PAGE_SIZE'd boundary which is unnecessary.
> We now issue the flush at REAL_HPAGE_SIZE boundaries only.
>
> Without this patch workloads which unmap a large hugepage
> backed VMA region get CPU lockups due to excessive TLB
> flush calls.
>
> Signed-off-by: Nitin Gupta <nitin.m.gupta@oracle.com>
> ---
> arch/sparc/include/asm/tlbflush_64.h | 3 ++-
> arch/sparc/mm/hugetlbpage.c | 7 ++++++-
> arch/sparc/mm/tlb.c | 2 +-
> arch/sparc/mm/tsb.c | 21 ++++++++++++++-------
> 4 files changed, 23 insertions(+), 10 deletions(-)
>
> Changelog v1 vs v2:
> - Access PTEs in order (David Miller)
> - Issue TLB and TSB flush after clearing PTEs (David Miller)
>
> diff --git a/arch/sparc/include/asm/tlbflush_64.h b/arch/sparc/include/asm/tlbflush_64.h
> index dea1cfa..5c28f78 100644
> --- a/arch/sparc/include/asm/tlbflush_64.h
> +++ b/arch/sparc/include/asm/tlbflush_64.h
> @@ -16,7 +16,8 @@ struct tlb_batch {
>
> void flush_tsb_kernel_range(unsigned long start, unsigned long end);
> void flush_tsb_user(struct tlb_batch *tb);
> -void flush_tsb_user_page(struct mm_struct *mm, unsigned long vaddr);
> +void flush_tsb_user_page(struct mm_struct *mm, unsigned long vaddr,
> + bool is_huge);
>
> /* TLB flush operations. */
>
> diff --git a/arch/sparc/mm/hugetlbpage.c b/arch/sparc/mm/hugetlbpage.c
> index 131eaf4..d8f625c 100644
> --- a/arch/sparc/mm/hugetlbpage.c
> +++ b/arch/sparc/mm/hugetlbpage.c
> @@ -202,10 +202,15 @@ pte_t huge_ptep_get_and_clear(struct mm_struct *mm, unsigned long addr,
> addr &= HPAGE_MASK;
>
> for (i = 0; i < (1 << HUGETLB_PAGE_ORDER); i++) {
> - pte_clear(mm, addr, ptep);
> + *ptep = __pte(0UL);
> addr += PAGE_SIZE;
> ptep++;
> }
> + /* Issue TSB and TLB flush at REAL_HPAGE_SIZE boundaries */
> + flush_tsb_user_page(mm, addr, true);
> + flush_tsb_user_page(mm, addr + REAL_HPAGE_SIZE, true);
> + global_flush_tlb_page(mm, addr);
> + global_flush_tlb_page(mm, addr + REAL_HPAGE_SIZE);
>
> return entry;
> }
> diff --git a/arch/sparc/mm/tlb.c b/arch/sparc/mm/tlb.c
> index 9df2190..1a5de57 100644
> --- a/arch/sparc/mm/tlb.c
> +++ b/arch/sparc/mm/tlb.c
> @@ -84,7 +84,7 @@ static void tlb_batch_add_one(struct mm_struct *mm, unsigned long vaddr,
> }
>
> if (!tb->active) {
> - flush_tsb_user_page(mm, vaddr);
> + flush_tsb_user_page(mm, vaddr, false);
> global_flush_tlb_page(mm, vaddr);
> goto out;
> }
> diff --git a/arch/sparc/mm/tsb.c b/arch/sparc/mm/tsb.c
> index a065766..3af2848 100644
> --- a/arch/sparc/mm/tsb.c
> +++ b/arch/sparc/mm/tsb.c
> @@ -94,18 +94,25 @@ void flush_tsb_user(struct tlb_batch *tb)
> spin_unlock_irqrestore(&mm->context.lock, flags);
> }
>
> -void flush_tsb_user_page(struct mm_struct *mm, unsigned long vaddr)
> +/*
> + * @is_huge is true if the page containing @vaddr is guaranteed to
> + * be a huge page. If false, then TSBs for both, base and huge page
> + * size are flushed.
> + */
> +void flush_tsb_user_page(struct mm_struct *mm, unsigned long vaddr,
> + bool is_huge)
> {
> unsigned long nentries, base, flags;
>
> spin_lock_irqsave(&mm->context.lock, flags);
>
> - base = (unsigned long) mm->context.tsb_block[MM_TSB_BASE].tsb;
> - nentries = mm->context.tsb_block[MM_TSB_BASE].tsb_nentries;
> - if (tlb_type = cheetah_plus || tlb_type = hypervisor)
> - base = __pa(base);
> - __flush_tsb_one_entry(base, vaddr, PAGE_SHIFT, nentries);
> -
> + if (!is_huge) {
> + base = (unsigned long) mm->context.tsb_block[MM_TSB_BASE].tsb;
> + nentries = mm->context.tsb_block[MM_TSB_BASE].tsb_nentries;
> + if (tlb_type = cheetah_plus || tlb_type = hypervisor)
> + base = __pa(base);
> + __flush_tsb_one_entry(base, vaddr, PAGE_SHIFT, nentries);
> + }
> #if defined(CONFIG_HUGETLB_PAGE) || defined(CONFIG_TRANSPARENT_HUGEPAGE)
> if (mm->context.tsb_block[MM_TSB_HUGE].tsb) {
> base = (unsigned long) mm->context.tsb_block[MM_TSB_HUGE].tsb;
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] sparc64: Reduce TLB flushes during hugepage unmap
2016-02-03 23:00 [PATCH v2] sparc64: Reduce TLB flushes during hugepage unmap Nitin Gupta
2016-02-09 18:33 ` Nitin Gupta
@ 2016-02-09 22:12 ` David Miller
2016-03-21 4:28 ` David Miller
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2016-02-09 22:12 UTC (permalink / raw)
To: sparclinux
From: Nitin Gupta <nitin.m.gupta@oracle.com>
Date: Tue, 9 Feb 2016 10:33:42 -0800
> Gentle reminder for review comments.
> Also adding Naoya to CC whom I forgot last time, sorry.
I'm travelling, conferencing, and giving a keynote on networking.
I will therefore probably not be able to fully deal with this for at
least another week, sorry.
If a patch is in patchwork, I know about it, it's on my TODO list,
and you never need to send me a reminder.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] sparc64: Reduce TLB flushes during hugepage unmap
2016-02-03 23:00 [PATCH v2] sparc64: Reduce TLB flushes during hugepage unmap Nitin Gupta
2016-02-09 18:33 ` Nitin Gupta
2016-02-09 22:12 ` David Miller
@ 2016-03-21 4:28 ` David Miller
2016-03-22 1:47 ` Nitin Gupta
2016-03-25 23:55 ` Nitin Gupta
4 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2016-03-21 4:28 UTC (permalink / raw)
To: sparclinux
From: Nitin Gupta <nitin.m.gupta@oracle.com>
Date: Wed, 3 Feb 2016 15:00:23 -0800
> During hugepage unmap, TSB and TLB flushes are currently
> issued at every PAGE_SIZE'd boundary which is unnecessary.
> We now issue the flush at REAL_HPAGE_SIZE boundaries only.
>
> Without this patch workloads which unmap a large hugepage
> backed VMA region get CPU lockups due to excessive TLB
> flush calls.
>
> Signed-off-by: Nitin Gupta <nitin.m.gupta@oracle.com>
I thought a lot about this stuff tonight, and I think we need to be
more intelligent about this.
Doing a synchronous flush unconditionally is not good. In particular,
we aren't even checking if the original PTE was mapped or not which
is going to be the most common case when a new mapping is created.
Also, we can't skip the D-cache flushes that older cpus need, as done
by tlb_batch_add().
Therefore let's teach the TLB batcher what we're actually trying to do
and what the optimization is, instead of trying so hard to bypass it
altogether.
In asm/pgtable_64.h provide is_hugetlb_pte(), I'd implement it like
this:
static inline unsigned long __pte_huge_mask(void)
{
unsigned long mask;
__asm__ __volatile__(
"\n661: sethi %%uhi(%1), %0\n"
" sllx %0, 32, %0\n"
" .section .sun4v_2insn_patch, \"ax\"\n"
" .word 661b\n"
" mov %2, %0\n"
" nop\n"
" .previous\n"
: "=r" (mask)
: "i" (_PAGE_SZHUGE_4U), "i" (_PAGE_SZHUGE_4V));
return mask;
}
Then pte_mkhuge() becomes:
static inline pte_t pte_mkhuge(pte_t pte)
{
return __pte(pte_val(pte) | __pte_huge_mask());
}
and then:
static inline bool is_hugetlb_pte(pte_t pte)
{
return (pte_val(pte) & __pte_huge_mask());
}
And then in tlb_batch_add() can detect if the orignal PTE is huge:
bool huge = is_hugetlb_pte(orig);
and then the end of the function is:
if (huge & (vaddr & (REAL_HPAGE_SIZE - 1)))
return;
if (!fullmm)
tlb_batch_add_one(mm, vaddr, pte_exec(orig), huge);
and tlb_batch_add_one() takes 'huge' and uses it to drive the flushing.
For a synchronous flush, we pass it down to flush_tsb_user_page().
For a batched flush we store it in tlb_batch, and any time 'huge'
changes, we do a flush_tlb_pending(), just the same as if tb->tlb_nr
hit TLB_BATCH_NR.
Then flush_tsb_user() uses 'tb->huge' to decide whether to flush
MM_TSB_BASE or not.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] sparc64: Reduce TLB flushes during hugepage unmap
2016-02-03 23:00 [PATCH v2] sparc64: Reduce TLB flushes during hugepage unmap Nitin Gupta
` (2 preceding siblings ...)
2016-03-21 4:28 ` David Miller
@ 2016-03-22 1:47 ` Nitin Gupta
2016-03-25 23:55 ` Nitin Gupta
4 siblings, 0 replies; 6+ messages in thread
From: Nitin Gupta @ 2016-03-22 1:47 UTC (permalink / raw)
To: sparclinux
On 03/20/2016 09:28 PM, David Miller wrote:
> From: Nitin Gupta <nitin.m.gupta@oracle.com>
> Date: Wed, 3 Feb 2016 15:00:23 -0800
>
>> During hugepage unmap, TSB and TLB flushes are currently
>> issued at every PAGE_SIZE'd boundary which is unnecessary.
>> We now issue the flush at REAL_HPAGE_SIZE boundaries only.
>>
>> Without this patch workloads which unmap a large hugepage
>> backed VMA region get CPU lockups due to excessive TLB
>> flush calls.
>>
>> Signed-off-by: Nitin Gupta <nitin.m.gupta@oracle.com>
>
> I thought a lot about this stuff tonight, and I think we need to be
> more intelligent about this.
>
> Doing a synchronous flush unconditionally is not good. In particular,
> we aren't even checking if the original PTE was mapped or not which
> is going to be the most common case when a new mapping is created.
>
> Also, we can't skip the D-cache flushes that older cpus need, as done
> by tlb_batch_add().
>
> Therefore let's teach the TLB batcher what we're actually trying to do
> and what the optimization is, instead of trying so hard to bypass it
> altogether.
>
> In asm/pgtable_64.h provide is_hugetlb_pte(), I'd implement it like
> this:
>
> static inline unsigned long __pte_huge_mask(void)
> {
> unsigned long mask;
>
> __asm__ __volatile__(
> "\n661: sethi %%uhi(%1), %0\n"
> " sllx %0, 32, %0\n"
> " .section .sun4v_2insn_patch, \"ax\"\n"
> " .word 661b\n"
> " mov %2, %0\n"
> " nop\n"
> " .previous\n"
> : "=r" (mask)
> : "i" (_PAGE_SZHUGE_4U), "i" (_PAGE_SZHUGE_4V));
>
> return mask;
> }
>
> Then pte_mkhuge() becomes:
>
> static inline pte_t pte_mkhuge(pte_t pte)
> {
> return __pte(pte_val(pte) | __pte_huge_mask());
> }
>
> and then:
>
> static inline bool is_hugetlb_pte(pte_t pte)
> {
> return (pte_val(pte) & __pte_huge_mask());
> }
>
> And then in tlb_batch_add() can detect if the orignal PTE is huge:
>
> bool huge = is_hugetlb_pte(orig);
>
> and then the end of the function is:
>
> if (huge & (vaddr & (REAL_HPAGE_SIZE - 1)))
> return;
> if (!fullmm)
> tlb_batch_add_one(mm, vaddr, pte_exec(orig), huge);
>
> and tlb_batch_add_one() takes 'huge' and uses it to drive the flushing.
>
> For a synchronous flush, we pass it down to flush_tsb_user_page().
>
> For a batched flush we store it in tlb_batch, and any time 'huge'
> changes, we do a flush_tlb_pending(), just the same as if tb->tlb_nr
> hit TLB_BATCH_NR.
>
> Then flush_tsb_user() uses 'tb->huge' to decide whether to flush
> MM_TSB_BASE or not.
>
Thanks for detailed notes. I will now combine my last two patches
which fix TLB flushing on PTE change and zero'ing out resp., along
with changes you suggested and send a v2 soon.
Thanks,
Nitin
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] sparc64: Reduce TLB flushes during hugepage unmap
2016-02-03 23:00 [PATCH v2] sparc64: Reduce TLB flushes during hugepage unmap Nitin Gupta
` (3 preceding siblings ...)
2016-03-22 1:47 ` Nitin Gupta
@ 2016-03-25 23:55 ` Nitin Gupta
4 siblings, 0 replies; 6+ messages in thread
From: Nitin Gupta @ 2016-03-25 23:55 UTC (permalink / raw)
To: sparclinux
On 03/20/2016 09:28 PM, David Miller wrote:
> From: Nitin Gupta <nitin.m.gupta@oracle.com>
> Date: Wed, 3 Feb 2016 15:00:23 -0800
>
>> During hugepage unmap, TSB and TLB flushes are currently
>> issued at every PAGE_SIZE'd boundary which is unnecessary.
>> We now issue the flush at REAL_HPAGE_SIZE boundaries only.
>>
>> Without this patch workloads which unmap a large hugepage
>> backed VMA region get CPU lockups due to excessive TLB
>> flush calls.
>>
>> Signed-off-by: Nitin Gupta <nitin.m.gupta@oracle.com>
>
> I thought a lot about this stuff tonight, and I think we need to be
> more intelligent about this.
>
> Doing a synchronous flush unconditionally is not good. In particular,
> we aren't even checking if the original PTE was mapped or not which
> is going to be the most common case when a new mapping is created.
>
> Also, we can't skip the D-cache flushes that older cpus need, as done
> by tlb_batch_add().
>
> Therefore let's teach the TLB batcher what we're actually trying to do
> and what the optimization is, instead of trying so hard to bypass it
> altogether.
>
> In asm/pgtable_64.h provide is_hugetlb_pte(), I'd implement it like
> this:
>
> static inline unsigned long __pte_huge_mask(void)
> {
> unsigned long mask;
>
> __asm__ __volatile__(
> "\n661: sethi %%uhi(%1), %0\n"
> " sllx %0, 32, %0\n"
> " .section .sun4v_2insn_patch, \"ax\"\n"
> " .word 661b\n"
> " mov %2, %0\n"
> " nop\n"
> " .previous\n"
> : "=r" (mask)
> : "i" (_PAGE_SZHUGE_4U), "i" (_PAGE_SZHUGE_4V));
>
> return mask;
> }
>
> Then pte_mkhuge() becomes:
>
> static inline pte_t pte_mkhuge(pte_t pte)
> {
> return __pte(pte_val(pte) | __pte_huge_mask());
> }
>
> and then:
>
> static inline bool is_hugetlb_pte(pte_t pte)
> {
> return (pte_val(pte) & __pte_huge_mask());
> }
>
> And then in tlb_batch_add() can detect if the orignal PTE is huge:
>
> bool huge = is_hugetlb_pte(orig);
>
> and then the end of the function is:
>
> if (huge & (vaddr & (REAL_HPAGE_SIZE - 1)))
> return;
Wouldn't this cause a race:
- PTE for REAL_HPAGE_SIZE aligned vaddr is cleared
- huge-TSB, TLB flushes issued (or scheduled) for this vaddr
- another thread accesses PTE for vaddr + PAGE_SIZE
- TSB, TLB again created for aligned vaddr which is then never cleared.
So, I think the solution is just clear/set hugetlb pte in loop
by direct assignment and after the loop, call set_pte_at() for base
addr and addr + REAL_HPAGE_SIZE
This is ugly but this problem would disappear when I'm done with
pagetable trimming patch which allocates page table till PMD level
only for 8M hugepage backed area just like THP. With this patch
we would have just one PMD to change so above race cannot happen,
and would save a lot of memory.
Thanks,
Nitin
> if (!fullmm)
> tlb_batch_add_one(mm, vaddr, pte_exec(orig), huge);
>
> and tlb_batch_add_one() takes 'huge' and uses it to drive the flushing.
>
> For a synchronous flush, we pass it down to flush_tsb_user_page().
>
> For a batched flush we store it in tlb_batch, and any time 'huge'
> changes, we do a flush_tlb_pending(), just the same as if tb->tlb_nr
> hit TLB_BATCH_NR.
>
> Then flush_tsb_user() uses 'tb->huge' to decide whether to flush
> MM_TSB_BASE or not.
> --
> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 6+ messages in thread