linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] RISC-V: Issue a local tlb flush if possible.
@ 2019-08-10  1:43 Atish Patra
  2019-08-10  3:30 ` Anup Patel
                   ` (5 more replies)
  0 siblings, 6 replies; 18+ messages in thread
From: Atish Patra @ 2019-08-10  1:43 UTC (permalink / raw)
  To: linux-kernel
  Cc: Atish Patra, Albert Ou, Alexios Zavras, Allison Randal,
	Greg Kroah-Hartman, linux-riscv, Palmer Dabbelt, Paul Walmsley,
	Anup Patel

In RISC-V, tlb flush happens via SBI which is expensive.
If the target cpumask contains a local hartid, some cost
can be saved by issuing a local tlb flush as we do that
in OpenSBI anyways.

Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
 arch/riscv/include/asm/tlbflush.h | 33 +++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/include/asm/tlbflush.h b/arch/riscv/include/asm/tlbflush.h
index 687dd19735a7..b32ba4fa5888 100644
--- a/arch/riscv/include/asm/tlbflush.h
+++ b/arch/riscv/include/asm/tlbflush.h
@@ -8,6 +8,7 @@
 #define _ASM_RISCV_TLBFLUSH_H
 
 #include <linux/mm_types.h>
+#include <linux/sched.h>
 #include <asm/smp.h>
 
 /*
@@ -46,14 +47,38 @@ static inline void remote_sfence_vma(struct cpumask *cmask, unsigned long start,
 				     unsigned long size)
 {
 	struct cpumask hmask;
+	struct cpumask tmask;
+	int cpuid = smp_processor_id();
 
 	cpumask_clear(&hmask);
-	riscv_cpuid_to_hartid_mask(cmask, &hmask);
-	sbi_remote_sfence_vma(hmask.bits, start, size);
+	cpumask_clear(&tmask);
+
+	if (cmask)
+		cpumask_copy(&tmask, cmask);
+	else
+		cpumask_copy(&tmask, cpu_online_mask);
+
+	if (cpumask_test_cpu(cpuid, &tmask)) {
+		/* Save trap cost by issuing a local tlb flush here */
+		if ((start == 0 && size == -1) || (size > PAGE_SIZE))
+			local_flush_tlb_all();
+		else if (size == PAGE_SIZE)
+			local_flush_tlb_page(start);
+		cpumask_clear_cpu(cpuid, &tmask);
+	} else if (cpumask_empty(&tmask)) {
+		/* cpumask is empty. So just do a local flush */
+		local_flush_tlb_all();
+		return;
+	}
+
+	if (!cpumask_empty(&tmask)) {
+		riscv_cpuid_to_hartid_mask(&tmask, &hmask);
+		sbi_remote_sfence_vma(hmask.bits, start, size);
+	}
 }
 
-#define flush_tlb_all() sbi_remote_sfence_vma(NULL, 0, -1)
-#define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr, 0)
+#define flush_tlb_all() remote_sfence_vma(NULL, 0, -1)
+#define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr, (addr) + PAGE_SIZE)
 #define flush_tlb_range(vma, start, end) \
 	remote_sfence_vma(mm_cpumask((vma)->vm_mm), start, (end) - (start))
 #define flush_tlb_mm(mm) \
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH] RISC-V: Issue a local tlb flush if possible.
  2019-08-10  1:43 [PATCH] RISC-V: Issue a local tlb flush if possible Atish Patra
@ 2019-08-10  3:30 ` Anup Patel
  2019-08-10  5:28   ` Atish Patra
  2019-08-10  6:37 ` Andreas Schwab
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 18+ messages in thread
From: Anup Patel @ 2019-08-10  3:30 UTC (permalink / raw)
  To: Atish Patra
  Cc: linux-kernel@vger.kernel.org List, Albert Ou, Alexios Zavras,
	Greg Kroah-Hartman, Palmer Dabbelt, Anup Patel, Paul Walmsley,
	linux-riscv, Allison Randal

On Sat, Aug 10, 2019 at 7:13 AM Atish Patra <atish.patra@wdc.com> wrote:
>
> In RISC-V, tlb flush happens via SBI which is expensive.
> If the target cpumask contains a local hartid, some cost
> can be saved by issuing a local tlb flush as we do that
> in OpenSBI anyways.
>
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> ---
>  arch/riscv/include/asm/tlbflush.h | 33 +++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/arch/riscv/include/asm/tlbflush.h b/arch/riscv/include/asm/tlbflush.h
> index 687dd19735a7..b32ba4fa5888 100644
> --- a/arch/riscv/include/asm/tlbflush.h
> +++ b/arch/riscv/include/asm/tlbflush.h
> @@ -8,6 +8,7 @@
>  #define _ASM_RISCV_TLBFLUSH_H
>
>  #include <linux/mm_types.h>
> +#include <linux/sched.h>
>  #include <asm/smp.h>
>
>  /*
> @@ -46,14 +47,38 @@ static inline void remote_sfence_vma(struct cpumask *cmask, unsigned long start,
>                                      unsigned long size)
>  {
>         struct cpumask hmask;
> +       struct cpumask tmask;
> +       int cpuid = smp_processor_id();
>
>         cpumask_clear(&hmask);
> -       riscv_cpuid_to_hartid_mask(cmask, &hmask);
> -       sbi_remote_sfence_vma(hmask.bits, start, size);
> +       cpumask_clear(&tmask);
> +
> +       if (cmask)
> +               cpumask_copy(&tmask, cmask);
> +       else
> +               cpumask_copy(&tmask, cpu_online_mask);

This can be further simplified.

We can totally avoid tmask, cpumask_copy(), and cpumask_clear()
by directly updating hmask.

In addition to this patch, we should also handle the case of
empty hart mask in OpenSBI.

> +
> +       if (cpumask_test_cpu(cpuid, &tmask)) {
> +               /* Save trap cost by issuing a local tlb flush here */
> +               if ((start == 0 && size == -1) || (size > PAGE_SIZE))
> +                       local_flush_tlb_all();
> +               else if (size == PAGE_SIZE)
> +                       local_flush_tlb_page(start);
> +               cpumask_clear_cpu(cpuid, &tmask);
> +       } else if (cpumask_empty(&tmask)) {
> +               /* cpumask is empty. So just do a local flush */
> +               local_flush_tlb_all();
> +               return;
> +       }
> +
> +       if (!cpumask_empty(&tmask)) {
> +               riscv_cpuid_to_hartid_mask(&tmask, &hmask);
> +               sbi_remote_sfence_vma(hmask.bits, start, size);
> +       }
>  }
>
> -#define flush_tlb_all() sbi_remote_sfence_vma(NULL, 0, -1)
> -#define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr, 0)
> +#define flush_tlb_all() remote_sfence_vma(NULL, 0, -1)
> +#define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr, (addr) + PAGE_SIZE)
>  #define flush_tlb_range(vma, start, end) \
>         remote_sfence_vma(mm_cpumask((vma)->vm_mm), start, (end) - (start))
>  #define flush_tlb_mm(mm) \
> --
> 2.21.0
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Regards,
Anup

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] RISC-V: Issue a local tlb flush if possible.
  2019-08-10  3:30 ` Anup Patel
@ 2019-08-10  5:28   ` Atish Patra
  0 siblings, 0 replies; 18+ messages in thread
From: Atish Patra @ 2019-08-10  5:28 UTC (permalink / raw)
  To: Anup Patel
  Cc: linux-kernel@vger.kernel.org List, Albert Ou, Alexios Zavras,
	Greg Kroah-Hartman, Palmer Dabbelt, Anup Patel, Paul Walmsley,
	linux-riscv, Allison Randal



On 8/9/19, 8:30 PM, "Anup Patel" <anup@brainfault.org> wrote:

    On Sat, Aug 10, 2019 at 7:13 AM Atish Patra <atish.patra@wdc.com> wrote:
    >
    > In RISC-V, tlb flush happens via SBI which is expensive.
    > If the target cpumask contains a local hartid, some cost
    > can be saved by issuing a local tlb flush as we do that
    > in OpenSBI anyways.
    >
    > Signed-off-by: Atish Patra <atish.patra@wdc.com>
    > ---
    >  arch/riscv/include/asm/tlbflush.h | 33 +++++++++++++++++++++++++++----
    >  1 file changed, 29 insertions(+), 4 deletions(-)
    >
    > diff --git a/arch/riscv/include/asm/tlbflush.h b/arch/riscv/include/asm/tlbflush.h
    > index 687dd19735a7..b32ba4fa5888 100644
    > --- a/arch/riscv/include/asm/tlbflush.h
    > +++ b/arch/riscv/include/asm/tlbflush.h
    > @@ -8,6 +8,7 @@
    >  #define _ASM_RISCV_TLBFLUSH_H
    >
    >  #include <linux/mm_types.h>
    > +#include <linux/sched.h>
    >  #include <asm/smp.h>
    >
    >  /*
    > @@ -46,14 +47,38 @@ static inline void remote_sfence_vma(struct cpumask *cmask, unsigned long start,
    >                                      unsigned long size)
    >  {
    >         struct cpumask hmask;
    > +       struct cpumask tmask;
    > +       int cpuid = smp_processor_id();
    >
    >         cpumask_clear(&hmask);
    > -       riscv_cpuid_to_hartid_mask(cmask, &hmask);
    > -       sbi_remote_sfence_vma(hmask.bits, start, size);
    > +       cpumask_clear(&tmask);
    > +
    > +       if (cmask)
    > +               cpumask_copy(&tmask, cmask);
    > +       else
    > +               cpumask_copy(&tmask, cpu_online_mask);
    
    This can be further simplified.
    
    We can totally avoid tmask, cpumask_copy(), and cpumask_clear()
    by directly updating hmask.
    
Ahh yes. Thanks for pointing out.

    In addition to this patch, we should also handle the case of
    empty hart mask in OpenSBI.
    
Yes. I have few other fixes as well (around fifo race condition and local flushing in OpenSBI).
I will send them soon.

Regards,
Atish
    > +
    > +       if (cpumask_test_cpu(cpuid, &tmask)) {
    > +               /* Save trap cost by issuing a local tlb flush here */
    > +               if ((start == 0 && size == -1) || (size > PAGE_SIZE))
    > +                       local_flush_tlb_all();
    > +               else if (size == PAGE_SIZE)
    > +                       local_flush_tlb_page(start);
    > +               cpumask_clear_cpu(cpuid, &tmask);
    > +       } else if (cpumask_empty(&tmask)) {
    > +               /* cpumask is empty. So just do a local flush */
    > +               local_flush_tlb_all();
    > +               return;
    > +       }
    > +
    > +       if (!cpumask_empty(&tmask)) {
    > +               riscv_cpuid_to_hartid_mask(&tmask, &hmask);
    > +               sbi_remote_sfence_vma(hmask.bits, start, size);
    > +       }
    >  }
    >
    > -#define flush_tlb_all() sbi_remote_sfence_vma(NULL, 0, -1)
    > -#define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr, 0)
    > +#define flush_tlb_all() remote_sfence_vma(NULL, 0, -1)
    > +#define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr, (addr) + PAGE_SIZE)
    >  #define flush_tlb_range(vma, start, end) \
    >         remote_sfence_vma(mm_cpumask((vma)->vm_mm), start, (end) - (start))
    >  #define flush_tlb_mm(mm) \
    > --
    > 2.21.0
    >
    >
    > _______________________________________________
    > linux-riscv mailing list
    > linux-riscv@lists.infradead.org
    > http://lists.infradead.org/mailman/listinfo/linux-riscv
    
    Regards,
    Anup
    


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] RISC-V: Issue a local tlb flush if possible.
  2019-08-10  1:43 [PATCH] RISC-V: Issue a local tlb flush if possible Atish Patra
  2019-08-10  3:30 ` Anup Patel
@ 2019-08-10  6:37 ` Andreas Schwab
  2019-08-10  9:21 ` Atish Patra
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 18+ messages in thread
From: Andreas Schwab @ 2019-08-10  6:37 UTC (permalink / raw)
  To: Atish Patra
  Cc: linux-kernel, Albert Ou, Alexios Zavras, Greg Kroah-Hartman,
	Palmer Dabbelt, Anup Patel, Paul Walmsley, linux-riscv,
	Allison Randal

On Aug 09 2019, Atish Patra <atish.patra@wdc.com> wrote:

> @@ -46,14 +47,38 @@ static inline void remote_sfence_vma(struct cpumask *cmask, unsigned long start,
>  				     unsigned long size)
>  {
>  	struct cpumask hmask;
> +	struct cpumask tmask;
> +	int cpuid = smp_processor_id();
>  
>  	cpumask_clear(&hmask);
> -	riscv_cpuid_to_hartid_mask(cmask, &hmask);
> -	sbi_remote_sfence_vma(hmask.bits, start, size);
> +	cpumask_clear(&tmask);
> +
> +	if (cmask)
> +		cpumask_copy(&tmask, cmask);
> +	else
> +		cpumask_copy(&tmask, cpu_online_mask);
> +
> +	if (cpumask_test_cpu(cpuid, &tmask)) {
> +		/* Save trap cost by issuing a local tlb flush here */
> +		if ((start == 0 && size == -1) || (size > PAGE_SIZE))
> +			local_flush_tlb_all();
> +		else if (size == PAGE_SIZE)
> +			local_flush_tlb_page(start);
> +		cpumask_clear_cpu(cpuid, &tmask);
> +	} else if (cpumask_empty(&tmask)) {
> +		/* cpumask is empty. So just do a local flush */
> +		local_flush_tlb_all();
> +		return;
> +	}
> +
> +	if (!cpumask_empty(&tmask)) {
> +		riscv_cpuid_to_hartid_mask(&tmask, &hmask);
> +		sbi_remote_sfence_vma(hmask.bits, start, size);
> +	}
>  }

I think it's becoming too big to be inline.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] RISC-V: Issue a local tlb flush if possible.
  2019-08-10  1:43 [PATCH] RISC-V: Issue a local tlb flush if possible Atish Patra
  2019-08-10  3:30 ` Anup Patel
  2019-08-10  6:37 ` Andreas Schwab
@ 2019-08-10  9:21 ` Atish Patra
  2019-08-12 14:56 ` Christoph Hellwig
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 18+ messages in thread
From: Atish Patra @ 2019-08-10  9:21 UTC (permalink / raw)
  To: linux-kernel
  Cc: Albert Ou, Alexios Zavras, Allison Randal, Greg Kroah-Hartman,
	linux-riscv, Palmer Dabbelt, Paul Walmsley, Anup Patel



On 8/9/19, 6:43 PM, "Atish Patra" <Atish.Patra@wdc.com> wrote:

    In RISC-V, tlb flush happens via SBI which is expensive.
    If the target cpumask contains a local hartid, some cost
    can be saved by issuing a local tlb flush as we do that
    in OpenSBI anyways.
    
    Signed-off-by: Atish Patra <atish.patra@wdc.com>
    ---
     arch/riscv/include/asm/tlbflush.h | 33 +++++++++++++++++++++++++++----
     1 file changed, 29 insertions(+), 4 deletions(-)
    
    diff --git a/arch/riscv/include/asm/tlbflush.h b/arch/riscv/include/asm/tlbflush.h
    index 687dd19735a7..b32ba4fa5888 100644
    --- a/arch/riscv/include/asm/tlbflush.h
    +++ b/arch/riscv/include/asm/tlbflush.h
    @@ -8,6 +8,7 @@
     #define _ASM_RISCV_TLBFLUSH_H
     
     #include <linux/mm_types.h>
    +#include <linux/sched.h>
     #include <asm/smp.h>
     
     /*
    @@ -46,14 +47,38 @@ static inline void remote_sfence_vma(struct cpumask *cmask, unsigned long start,
     				     unsigned long size)
     {
     	struct cpumask hmask;
    +	struct cpumask tmask;
    +	int cpuid = smp_processor_id();
     
     	cpumask_clear(&hmask);
    -	riscv_cpuid_to_hartid_mask(cmask, &hmask);
    -	sbi_remote_sfence_vma(hmask.bits, start, size);
    +	cpumask_clear(&tmask);
    +
    +	if (cmask)
    +		cpumask_copy(&tmask, cmask);
    +	else
    +		cpumask_copy(&tmask, cpu_online_mask);
    +
    +	if (cpumask_test_cpu(cpuid, &tmask)) {
    +		/* Save trap cost by issuing a local tlb flush here */
    +		if ((start == 0 && size == -1) || (size > PAGE_SIZE))
    +			local_flush_tlb_all();
    +		else if (size == PAGE_SIZE)
    +			local_flush_tlb_page(start);
    +		cpumask_clear_cpu(cpuid, &tmask);
    +	} else if (cpumask_empty(&tmask)) {
    +		/* cpumask is empty. So just do a local flush */
    +		local_flush_tlb_all();
    +		return;
    +	}
    +
    +	if (!cpumask_empty(&tmask)) {
    +		riscv_cpuid_to_hartid_mask(&tmask, &hmask);
    +		sbi_remote_sfence_vma(hmask.bits, start, size);
    +	}
     }
     
    -#define flush_tlb_all() sbi_remote_sfence_vma(NULL, 0, -1)
    -#define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr, 0)
    +#define flush_tlb_all() remote_sfence_vma(NULL, 0, -1)
    +#define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr, (addr) + PAGE_SIZE)

I did not see Paul's patch to fix this. I will remove this in v2 and rebase on top of Paul's patch.

Regards,
Atish
     #define flush_tlb_range(vma, start, end) \
     	remote_sfence_vma(mm_cpumask((vma)->vm_mm), start, (end) - (start))
     #define flush_tlb_mm(mm) \
    -- 
    2.21.0
    
    


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] RISC-V: Issue a local tlb flush if possible.
  2019-08-10  1:43 [PATCH] RISC-V: Issue a local tlb flush if possible Atish Patra
                   ` (2 preceding siblings ...)
  2019-08-10  9:21 ` Atish Patra
@ 2019-08-12 14:56 ` Christoph Hellwig
  2019-08-13  0:15   ` Atish Patra
  2019-08-12 15:36 ` Troy Benjegerdes
  2019-08-13 18:25 ` Paul Walmsley
  5 siblings, 1 reply; 18+ messages in thread
From: Christoph Hellwig @ 2019-08-12 14:56 UTC (permalink / raw)
  To: Atish Patra
  Cc: linux-kernel, Albert Ou, Alexios Zavras, Greg Kroah-Hartman,
	Palmer Dabbelt, Anup Patel, Paul Walmsley, linux-riscv,
	Allison Randal

I agree with the comment that we really should move this out of line
now, and also that we can simplify it further, which also includes
not bothering with the SBI call if we were the only online CPU.
I also thing we need to use get_cpu/put_cpu to be preemption safe.

Also why would we need to do a local flush if we have a mask that
doesn't include the local CPU?

How about something like:

void __riscv_flush_tlb(struct cpumask *cpumask, unsigned long start,
		unsigned long size)
{
	unsigned int cpu;

	if (!cpumask)
		cpumask = cpu_online_mask;

	cpu = get_cpu();
	if (!cpumask || cpumask_test_cpu(cpu, cpumask) {
		if ((start == 0 && size == -1) || size > PAGE_SIZE)
			local_flush_tlb_all();
		else if (size == PAGE_SIZE)
			local_flush_tlb_page(start);
		cpumask_clear_cpu(cpuid, cpumask);
	}

	if (!cpumask_empty(cpumask)) {
	  	struct cpumask hmask;

		riscv_cpuid_to_hartid_mask(cpumask, &hmask);
		sbi_remote_sfence_vma(hmask.bits, start, size);
	}
	put_cpu();
}

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] RISC-V: Issue a local tlb flush if possible.
  2019-08-10  1:43 [PATCH] RISC-V: Issue a local tlb flush if possible Atish Patra
                   ` (3 preceding siblings ...)
  2019-08-12 14:56 ` Christoph Hellwig
@ 2019-08-12 15:36 ` Troy Benjegerdes
  2019-08-12 17:13   ` Atish Patra
  2019-08-12 17:55   ` Christoph Hellwig
  2019-08-13 18:25 ` Paul Walmsley
  5 siblings, 2 replies; 18+ messages in thread
From: Troy Benjegerdes @ 2019-08-12 15:36 UTC (permalink / raw)
  To: Atish Patra
  Cc: Linux Kernel Mailing List, Albert Ou, Alexios Zavras,
	Greg Kroah-Hartman, Palmer Dabbelt, Anup Patel, Paul Walmsley,
	linux-riscv, Allison Randal, ron minnich



> On Aug 9, 2019, at 8:43 PM, Atish Patra <atish.patra@wdc.com> wrote:
> 
> In RISC-V, tlb flush happens via SBI which is expensive.
> If the target cpumask contains a local hartid, some cost
> can be saved by issuing a local tlb flush as we do that
> in OpenSBI anyways.

Is there anything other than convention and current usage that prevents
the kernel from natively handling TLB flushes without ever making the SBI
call?

Someone is eventually going to want to run the linux kernel in machine mode,
likely for performance and/or security reasons, and this will require flushing TLBs
natively anyway.


> 
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> ---
> arch/riscv/include/asm/tlbflush.h | 33 +++++++++++++++++++++++++++----
> 1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/tlbflush.h b/arch/riscv/include/asm/tlbflush.h
> index 687dd19735a7..b32ba4fa5888 100644
> --- a/arch/riscv/include/asm/tlbflush.h
> +++ b/arch/riscv/include/asm/tlbflush.h
> @@ -8,6 +8,7 @@
> #define _ASM_RISCV_TLBFLUSH_H
> 
> #include <linux/mm_types.h>
> +#include <linux/sched.h>
> #include <asm/smp.h>
> 
> /*
> @@ -46,14 +47,38 @@ static inline void remote_sfence_vma(struct cpumask *cmask, unsigned long start,
> 				     unsigned long size)
> {
> 	struct cpumask hmask;
> +	struct cpumask tmask;
> +	int cpuid = smp_processor_id();
> 
> 	cpumask_clear(&hmask);
> -	riscv_cpuid_to_hartid_mask(cmask, &hmask);
> -	sbi_remote_sfence_vma(hmask.bits, start, size);
> +	cpumask_clear(&tmask);
> +
> +	if (cmask)
> +		cpumask_copy(&tmask, cmask);
> +	else
> +		cpumask_copy(&tmask, cpu_online_mask);
> +
> +	if (cpumask_test_cpu(cpuid, &tmask)) {
> +		/* Save trap cost by issuing a local tlb flush here */
> +		if ((start == 0 && size == -1) || (size > PAGE_SIZE))
> +			local_flush_tlb_all();
> +		else if (size == PAGE_SIZE)
> +			local_flush_tlb_page(start);
> +		cpumask_clear_cpu(cpuid, &tmask);
> +	} else if (cpumask_empty(&tmask)) {
> +		/* cpumask is empty. So just do a local flush */
> +		local_flush_tlb_all();
> +		return;
> +	}
> +
> +	if (!cpumask_empty(&tmask)) {
> +		riscv_cpuid_to_hartid_mask(&tmask, &hmask);
> +		sbi_remote_sfence_vma(hmask.bits, start, size);
> +	}
> }
> 
> -#define flush_tlb_all() sbi_remote_sfence_vma(NULL, 0, -1)
> -#define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr, 0)
> +#define flush_tlb_all() remote_sfence_vma(NULL, 0, -1)
> +#define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr, (addr) + PAGE_SIZE)
> #define flush_tlb_range(vma, start, end) \
> 	remote_sfence_vma(mm_cpumask((vma)->vm_mm), start, (end) - (start))
> #define flush_tlb_mm(mm) \
> -- 
> 2.21.0
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] RISC-V: Issue a local tlb flush if possible.
  2019-08-12 15:36 ` Troy Benjegerdes
@ 2019-08-12 17:13   ` Atish Patra
  2019-08-12 17:55   ` Christoph Hellwig
  1 sibling, 0 replies; 18+ messages in thread
From: Atish Patra @ 2019-08-12 17:13 UTC (permalink / raw)
  To: troy.benjegerdes
  Cc: linux-riscv, Anup Patel, gregkh, rminnich, paul.walmsley, aou,
	linux-kernel, allison, alexios.zavras, palmer

On Mon, 2019-08-12 at 10:36 -0500, Troy Benjegerdes wrote:
> > On Aug 9, 2019, at 8:43 PM, Atish Patra <atish.patra@wdc.com>
> > wrote:
> > 
> > In RISC-V, tlb flush happens via SBI which is expensive.
> > If the target cpumask contains a local hartid, some cost
> > can be saved by issuing a local tlb flush as we do that
> > in OpenSBI anyways.
> 
> Is there anything other than convention and current usage that
> prevents
> the kernel from natively handling TLB flushes without ever making the
> SBI
> call?
> 
> Someone is eventually going to want to run the linux kernel in
> machine mode,
> likely for performance and/or security reasons, and this will require
> flushing TLBs
> natively anyway.
> 

The support is already added by Christoph in nommu series.

https://lkml.org/lkml/2019/6/10/935

The idea is to just send IPIs directly in Linux. The same approach is
not good in Supervisor mode until we can get rid of IPIs via SBI all
together. Otherwise, every tlbflush will be even more expensive as it
has to comeback to S mode and then execute sfence.vma.

> 
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > ---
> > arch/riscv/include/asm/tlbflush.h | 33 +++++++++++++++++++++++++++-
> > ---
> > 1 file changed, 29 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/riscv/include/asm/tlbflush.h
> > b/arch/riscv/include/asm/tlbflush.h
> > index 687dd19735a7..b32ba4fa5888 100644
> > --- a/arch/riscv/include/asm/tlbflush.h
> > +++ b/arch/riscv/include/asm/tlbflush.h
> > @@ -8,6 +8,7 @@
> > #define _ASM_RISCV_TLBFLUSH_H
> > 
> > #include <linux/mm_types.h>
> > +#include <linux/sched.h>
> > #include <asm/smp.h>
> > 
> > /*
> > @@ -46,14 +47,38 @@ static inline void remote_sfence_vma(struct
> > cpumask *cmask, unsigned long start,
> > 				     unsigned long size)
> > {
> > 	struct cpumask hmask;
> > +	struct cpumask tmask;
> > +	int cpuid = smp_processor_id();
> > 
> > 	cpumask_clear(&hmask);
> > -	riscv_cpuid_to_hartid_mask(cmask, &hmask);
> > -	sbi_remote_sfence_vma(hmask.bits, start, size);
> > +	cpumask_clear(&tmask);
> > +
> > +	if (cmask)
> > +		cpumask_copy(&tmask, cmask);
> > +	else
> > +		cpumask_copy(&tmask, cpu_online_mask);
> > +
> > +	if (cpumask_test_cpu(cpuid, &tmask)) {
> > +		/* Save trap cost by issuing a local tlb flush here */
> > +		if ((start == 0 && size == -1) || (size > PAGE_SIZE))
> > +			local_flush_tlb_all();
> > +		else if (size == PAGE_SIZE)
> > +			local_flush_tlb_page(start);
> > +		cpumask_clear_cpu(cpuid, &tmask);
> > +	} else if (cpumask_empty(&tmask)) {
> > +		/* cpumask is empty. So just do a local flush */
> > +		local_flush_tlb_all();
> > +		return;
> > +	}
> > +
> > +	if (!cpumask_empty(&tmask)) {
> > +		riscv_cpuid_to_hartid_mask(&tmask, &hmask);
> > +		sbi_remote_sfence_vma(hmask.bits, start, size);
> > +	}
> > }
> > 
> > -#define flush_tlb_all() sbi_remote_sfence_vma(NULL, 0, -1)
> > -#define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr, 0)
> > +#define flush_tlb_all() remote_sfence_vma(NULL, 0, -1)
> > +#define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr,
> > (addr) + PAGE_SIZE)
> > #define flush_tlb_range(vma, start, end) \
> > 	remote_sfence_vma(mm_cpumask((vma)->vm_mm), start, (end) -
> > (start))
> > #define flush_tlb_mm(mm) \
> > -- 
> > 2.21.0
> > 
> > 
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv

-- 
Regards,
Atish

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] RISC-V: Issue a local tlb flush if possible.
  2019-08-12 15:36 ` Troy Benjegerdes
  2019-08-12 17:13   ` Atish Patra
@ 2019-08-12 17:55   ` Christoph Hellwig
  1 sibling, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2019-08-12 17:55 UTC (permalink / raw)
  To: Troy Benjegerdes
  Cc: Atish Patra, Linux Kernel Mailing List, Albert Ou,
	Alexios Zavras, Greg Kroah-Hartman, Palmer Dabbelt, Anup Patel,
	Paul Walmsley, linux-riscv, Allison Randal, ron minnich

On Mon, Aug 12, 2019 at 10:36:25AM -0500, Troy Benjegerdes wrote:
> Is there anything other than convention and current usage that prevents
> the kernel from natively handling TLB flushes without ever making the SBI
> call?

Yes and no.

In all existing RISC-V implementation remote TLB flushes are simply
implementing using IPIs.  So you could trivially implement remote TLB
flush using IPIs, and in fact Gary Guo posted a series to do that a
while ago.

But: the RISC privileged spec requires that IPIs are only issued from
M-mode and only delivered to M-mode.  So what would be a trivial MMIO
write plus interupt to wakeup the remote hart actually turns into
a dance requiring multiple context switches between privile levels,
and without additional optimizations that will be even slower than the
current SBI based implementation.

I've started a prototype implementation and spec edits to relax this
and allow direct IPIs from S-mode to S-mode, which will speed up IPIs
by about an order of magnitude, and I hope this will be how future
RISC-V implementations work.

> Someone is eventually going to want to run the linux kernel in machine mode,
> likely for performance and/or security reasons, and this will require flushing TLBs
> natively anyway.

The nommu ports run in M-mode.  But running a MMU-enabled port in M-mode
is rather painful if not impossible (trust me, I've tried) due to how
the privileged spec says that M-mode generally runs without address
translation.  There is a workaround using the MPRV bit in mstatus, but
even that just uses the address translation for loads and stores, and
not for the program text.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] RISC-V: Issue a local tlb flush if possible.
  2019-08-12 14:56 ` Christoph Hellwig
@ 2019-08-13  0:15   ` Atish Patra
  2019-08-13 14:30     ` hch
  0 siblings, 1 reply; 18+ messages in thread
From: Atish Patra @ 2019-08-13  0:15 UTC (permalink / raw)
  To: hch
  Cc: linux-riscv, Anup Patel, gregkh, paul.walmsley, aou,
	linux-kernel, allison, alexios.zavras, palmer

On Mon, 2019-08-12 at 07:56 -0700, Christoph Hellwig wrote:
> I agree with the comment that we really should move this out of line
> now, and 

sure.

> also that we can simplify it further, which also includes
> not bothering with the SBI call if we were the only online CPU.

I already had that optimization in my patch.

> I also thing we need to use get_cpu/put_cpu to be preemption safe.
> 
ok. I will update that.

> Also why would we need to do a local flush if we have a mask that
> doesn't include the local CPU?
> 

I thought if it recieves an empty cpumask, then it should at least do a
local flush is the expected behavior. Are you saying that we should
just skip all and return ? 


> How about something like:
> 
> void __riscv_flush_tlb(struct cpumask *cpumask, unsigned long start,
> 		unsigned long size)
> {
> 	unsigned int cpu;
> 
> 	if (!cpumask)
> 		cpumask = cpu_online_mask;
> 
> 	cpu = get_cpu();
> 	if (!cpumask || cpumask_test_cpu(cpu, cpumask) {
> 		if ((start == 0 && size == -1) || size > PAGE_SIZE)
> 			local_flush_tlb_all();
> 		else if (size == PAGE_SIZE)
> 			local_flush_tlb_page(start);
> 		cpumask_clear_cpu(cpuid, cpumask);

This will modify the original cpumask which is not correct. clear part
has to be done on hmask to avoid a copy.

Regards,
Atish
> 	}
> 
> 	if (!cpumask_empty(cpumask)) {
> 	  	struct cpumask hmask;
> 
> 		riscv_cpuid_to_hartid_mask(cpumask, &hmask);
> 		sbi_remote_sfence_vma(hmask.bits, start, size);
> 	}
> 	put_cpu();
> }


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] RISC-V: Issue a local tlb flush if possible.
  2019-08-13  0:15   ` Atish Patra
@ 2019-08-13 14:30     ` hch
  2019-08-15 20:37       ` Atish Patra
  0 siblings, 1 reply; 18+ messages in thread
From: hch @ 2019-08-13 14:30 UTC (permalink / raw)
  To: Atish Patra
  Cc: hch, aou, gregkh, Anup Patel, linux-kernel, alexios.zavras,
	palmer, paul.walmsley, linux-riscv, allison

On Tue, Aug 13, 2019 at 12:15:15AM +0000, Atish Patra wrote:
> I thought if it recieves an empty cpumask, then it should at least do a
> local flush is the expected behavior. Are you saying that we should
> just skip all and return ? 

How could we ever receive an empty cpu mask?  I think it could only
be empty without the current cpu.  At least that is my reading of
the callers and a few other implementations.

> > 	if (!cpumask || cpumask_test_cpu(cpu, cpumask) {
> > 		if ((start == 0 && size == -1) || size > PAGE_SIZE)
> > 			local_flush_tlb_all();
> > 		else if (size == PAGE_SIZE)
> > 			local_flush_tlb_page(start);
> > 		cpumask_clear_cpu(cpuid, cpumask);
> 
> This will modify the original cpumask which is not correct. clear part
> has to be done on hmask to avoid a copy.

Indeed.  But looking at the x86 tlbflush implementation it seems like we
could use cpumask_any_but() to avoid having to modify the passed in
cpumask.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] RISC-V: Issue a local tlb flush if possible.
  2019-08-10  1:43 [PATCH] RISC-V: Issue a local tlb flush if possible Atish Patra
                   ` (4 preceding siblings ...)
  2019-08-12 15:36 ` Troy Benjegerdes
@ 2019-08-13 18:25 ` Paul Walmsley
  2019-08-14  1:49   ` Atish Patra
  5 siblings, 1 reply; 18+ messages in thread
From: Paul Walmsley @ 2019-08-13 18:25 UTC (permalink / raw)
  To: Atish Patra
  Cc: linux-kernel, Albert Ou, Alexios Zavras, Allison Randal,
	Greg Kroah-Hartman, linux-riscv, Palmer Dabbelt, Anup Patel

Hi Atish,

On Fri, 9 Aug 2019, Atish Patra wrote:

> In RISC-V, tlb flush happens via SBI which is expensive.
> If the target cpumask contains a local hartid, some cost
> can be saved by issuing a local tlb flush as we do that
> in OpenSBI anyways.
> 
> Signed-off-by: Atish Patra <atish.patra@wdc.com>

A few brief comments/questions beyond the ones that others have mentioned:

1. At some point, some RISC-V systems may implement this SBI call in 
hardware, rather than in software.  Then this might wind up becoming a 
de-optimization.  I don't think there's anything to do about that in code 
right now, but it might be worth adding a comment, and thinking about how 
that case might be handled in the platform specification group.

2. If this patch masks or reduces the likelihood of hitting the 
TLB-related crashes that we're seeing, we probably will want to hold off 
on merging this one until we're relatively certain that those other 
problems have been fixed. 



> ---
>  arch/riscv/include/asm/tlbflush.h | 33 +++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/tlbflush.h b/arch/riscv/include/asm/tlbflush.h
> index 687dd19735a7..b32ba4fa5888 100644
> --- a/arch/riscv/include/asm/tlbflush.h
> +++ b/arch/riscv/include/asm/tlbflush.h
> @@ -8,6 +8,7 @@
>  #define _ASM_RISCV_TLBFLUSH_H
>  
>  #include <linux/mm_types.h>
> +#include <linux/sched.h>
>  #include <asm/smp.h>
>  
>  /*
> @@ -46,14 +47,38 @@ static inline void remote_sfence_vma(struct cpumask *cmask, unsigned long start,
>  				     unsigned long size)
>  {
>  	struct cpumask hmask;
> +	struct cpumask tmask;
> +	int cpuid = smp_processor_id();
>  
>  	cpumask_clear(&hmask);
> -	riscv_cpuid_to_hartid_mask(cmask, &hmask);
> -	sbi_remote_sfence_vma(hmask.bits, start, size);
> +	cpumask_clear(&tmask);
> +
> +	if (cmask)
> +		cpumask_copy(&tmask, cmask);
> +	else
> +		cpumask_copy(&tmask, cpu_online_mask);
> +
> +	if (cpumask_test_cpu(cpuid, &tmask)) {
> +		/* Save trap cost by issuing a local tlb flush here */
> +		if ((start == 0 && size == -1) || (size > PAGE_SIZE))
> +			local_flush_tlb_all();
> +		else if (size == PAGE_SIZE)
> +			local_flush_tlb_page(start);
> +		cpumask_clear_cpu(cpuid, &tmask);
> +	} else if (cpumask_empty(&tmask)) {
> +		/* cpumask is empty. So just do a local flush */
> +		local_flush_tlb_all();
> +		return;
> +	}
> +
> +	if (!cpumask_empty(&tmask)) {
> +		riscv_cpuid_to_hartid_mask(&tmask, &hmask);
> +		sbi_remote_sfence_vma(hmask.bits, start, size);
> +	}
>  }
>  
> -#define flush_tlb_all() sbi_remote_sfence_vma(NULL, 0, -1)
> -#define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr, 0)
> +#define flush_tlb_all() remote_sfence_vma(NULL, 0, -1)
> +#define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr, (addr) + PAGE_SIZE)
>  #define flush_tlb_range(vma, start, end) \
>  	remote_sfence_vma(mm_cpumask((vma)->vm_mm), start, (end) - (start))
>  #define flush_tlb_mm(mm) \
> -- 
> 2.21.0
> 
> 


- Paul

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] RISC-V: Issue a local tlb flush if possible.
  2019-08-13 18:25 ` Paul Walmsley
@ 2019-08-14  1:49   ` Atish Patra
  0 siblings, 0 replies; 18+ messages in thread
From: Atish Patra @ 2019-08-14  1:49 UTC (permalink / raw)
  To: paul.walmsley
  Cc: linux-riscv, Anup Patel, gregkh, linux-kernel, allison,
	alexios.zavras, palmer, aou

On Tue, 2019-08-13 at 11:25 -0700, Paul Walmsley wrote:
> Hi Atish,
> 
> On Fri, 9 Aug 2019, Atish Patra wrote:
> 
> > In RISC-V, tlb flush happens via SBI which is expensive.
> > If the target cpumask contains a local hartid, some cost
> > can be saved by issuing a local tlb flush as we do that
> > in OpenSBI anyways.
> > 
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> 
> A few brief comments/questions beyond the ones that others have
> mentioned:
> 
> 1. At some point, some RISC-V systems may implement this SBI call in 
> hardware, rather than in software.  Then this might wind up becoming
> a 
> de-optimization.  I don't think there's anything to do about that in
> code 
> right now, but it might be worth adding a comment, and thinking about
> how 
> that case might be handled in the platform specification group.

I am fine with adding a comment. But I did not understand why this
would be deoptimization. I not exactly sure about the syntax of future
TLB flush instructions. But if we have a hardware instruction for
remote tlb flushing, it should be executed only for the harts other
than current hart. Right ? 

> 2. If this patch masks or reduces the likelihood of hitting the 
> TLB-related crashes that we're seeing, we probably will want to hold
> off 
> on merging this one until we're relatively certain that those other 
> problems have been fixed. 
> 

Yeah sure. I don't see any stress-ng error but still chasing down the
glibc error.

Regards,
Atish
> 
> 
> > ---
> >  arch/riscv/include/asm/tlbflush.h | 33
> > +++++++++++++++++++++++++++----
> >  1 file changed, 29 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/riscv/include/asm/tlbflush.h
> > b/arch/riscv/include/asm/tlbflush.h
> > index 687dd19735a7..b32ba4fa5888 100644
> > --- a/arch/riscv/include/asm/tlbflush.h
> > +++ b/arch/riscv/include/asm/tlbflush.h
> > @@ -8,6 +8,7 @@
> >  #define _ASM_RISCV_TLBFLUSH_H
> >  
> >  #include <linux/mm_types.h>
> > +#include <linux/sched.h>
> >  #include <asm/smp.h>
> >  
> >  /*
> > @@ -46,14 +47,38 @@ static inline void remote_sfence_vma(struct
> > cpumask *cmask, unsigned long start,
> >  				     unsigned long size)
> >  {
> >  	struct cpumask hmask;
> > +	struct cpumask tmask;
> > +	int cpuid = smp_processor_id();
> >  
> >  	cpumask_clear(&hmask);
> > -	riscv_cpuid_to_hartid_mask(cmask, &hmask);
> > -	sbi_remote_sfence_vma(hmask.bits, start, size);
> > +	cpumask_clear(&tmask);
> > +
> > +	if (cmask)
> > +		cpumask_copy(&tmask, cmask);
> > +	else
> > +		cpumask_copy(&tmask, cpu_online_mask);
> > +
> > +	if (cpumask_test_cpu(cpuid, &tmask)) {
> > +		/* Save trap cost by issuing a local tlb flush here */
> > +		if ((start == 0 && size == -1) || (size > PAGE_SIZE))
> > +			local_flush_tlb_all();
> > +		else if (size == PAGE_SIZE)
> > +			local_flush_tlb_page(start);
> > +		cpumask_clear_cpu(cpuid, &tmask);
> > +	} else if (cpumask_empty(&tmask)) {
> > +		/* cpumask is empty. So just do a local flush */
> > +		local_flush_tlb_all();
> > +		return;
> > +	}
> > +
> > +	if (!cpumask_empty(&tmask)) {
> > +		riscv_cpuid_to_hartid_mask(&tmask, &hmask);
> > +		sbi_remote_sfence_vma(hmask.bits, start, size);
> > +	}
> >  }
> >  
> > -#define flush_tlb_all() sbi_remote_sfence_vma(NULL, 0, -1)
> > -#define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr, 0)
> > +#define flush_tlb_all() remote_sfence_vma(NULL, 0, -1)
> > +#define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr,
> > (addr) + PAGE_SIZE)
> >  #define flush_tlb_range(vma, start, end) \
> >  	remote_sfence_vma(mm_cpumask((vma)->vm_mm), start, (end) -
> > (start))
> >  #define flush_tlb_mm(mm) \
> > -- 
> > 2.21.0
> > 
> > 
> 
> - Paul


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] RISC-V: Issue a local tlb flush if possible.
  2019-08-13 14:30     ` hch
@ 2019-08-15 20:37       ` Atish Patra
  2019-08-19 14:46         ` hch
  0 siblings, 1 reply; 18+ messages in thread
From: Atish Patra @ 2019-08-15 20:37 UTC (permalink / raw)
  To: hch
  Cc: linux-riscv, Anup Patel, gregkh, paul.walmsley, aou,
	linux-kernel, allison, alexios.zavras, palmer

On Tue, 2019-08-13 at 07:30 -0700, hch@infradead.org wrote:
> On Tue, Aug 13, 2019 at 12:15:15AM +0000, Atish Patra wrote:
> > I thought if it recieves an empty cpumask, then it should at least
> > do a
> > local flush is the expected behavior. Are you saying that we should
> > just skip all and return ? 
> 
> How could we ever receive an empty cpu mask?  I think it could only
> be empty without the current cpu.  At least that is my reading of
> the callers and a few other implementations.
> 

We get ton of them. Here is the stack dump.

[   16.735814] [<ffffffe000035498>] walk_stackframe+0x0/0xa0^M
298436 [   16.819037] [<ffffffe0000355f8>] show_stack+0x2a/0x34^M
298437 [   16.899648] [<ffffffe00067b54c>] dump_stack+0x62/0x7c^M
298438 [   16.977402] [<ffffffe0000ef6f6>] tlb_flush_mmu+0x14a/0x150^M
298439 [   17.054197] [<ffffffe0000ef7a4>] tlb_finish_mmu+0x42/0x72^M
298440 [   17.129986] [<ffffffe0000ede7c>] exit_mmap+0x8e/0xfa^M
298441 [   17.203669] [<ffffffe000037d54>] mmput.part.3+0x1a/0xc4^M
298442 [   17.275985] [<ffffffe000037e1e>] mmput+0x20/0x28^M
298443 [   17.345248] [<ffffffe0001143c2>] flush_old_exec+0x418/0x5f8^M
298444 [   17.415370] [<ffffffe000158408>]
load_elf_binary+0x262/0xc84^M
298445 [   17.483641] [<ffffffe000114614>]
search_binary_handler.part.7+0x72/0x172^M
298446 [   17.552078] [<ffffffe000114bb2>]
__do_execve_file+0x40c/0x56a^M
298447 [   17.617932] [<ffffffe00011503e>] sys_execve+0x26/0x32^M
298448 [   17.682164] [<ffffffe00003437e>] ret_from_syscall+0x0/0xe^M

It looks like it is in the path of clearing the old traces of already
run script or program.  I am not sure if the cpumask supposed to be
empty in this path.

Probably we should just issue tlb flush all for all CPUs instead of
just the local CPU.

> > > 	if (!cpumask || cpumask_test_cpu(cpu, cpumask) {
> > > 		if ((start == 0 && size == -1) || size > PAGE_SIZE)
> > > 			local_flush_tlb_all();
> > > 		else if (size == PAGE_SIZE)
> > > 			local_flush_tlb_page(start);
> > > 		cpumask_clear_cpu(cpuid, cpumask);
> > 
> > This will modify the original cpumask which is not correct. clear
> > part
> > has to be done on hmask to avoid a copy.
> 
> Indeed.  But looking at the x86 tlbflush implementation it seems like
> we
> could use cpumask_any_but() to avoid having to modify the passed in
> cpumask.

Looking at the x86 code, it uses cpumask_any_but to just test if there
any other cpu present apart from the current one.

If yes, it calls smp_call_function_many which ignores the current cpu
and execute tlb flush code on all other cpus.

For RISC-V, it has to still send the cpumask containing local cpu and
M-mode software may do a local tlb flush the tlbs again for no reason.


Regards,
Atish

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] RISC-V: Issue a local tlb flush if possible.
  2019-08-15 20:37       ` Atish Patra
@ 2019-08-19 14:46         ` hch
  2019-08-19 15:09           ` Anup Patel
  0 siblings, 1 reply; 18+ messages in thread
From: hch @ 2019-08-19 14:46 UTC (permalink / raw)
  To: Atish Patra
  Cc: hch, aou, gregkh, Anup Patel, linux-kernel, alexios.zavras,
	palmer, paul.walmsley, linux-riscv, allison

On Thu, Aug 15, 2019 at 08:37:04PM +0000, Atish Patra wrote:
> We get ton of them. Here is the stack dump.

Looks like we might not need to flush anything at all here as the
mm_struct was never scheduled to run on any cpu?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] RISC-V: Issue a local tlb flush if possible.
  2019-08-19 14:46         ` hch
@ 2019-08-19 15:09           ` Anup Patel
  2019-08-19 15:10             ` hch
  0 siblings, 1 reply; 18+ messages in thread
From: Anup Patel @ 2019-08-19 15:09 UTC (permalink / raw)
  To: hch
  Cc: Atish Patra, aou, gregkh, Anup Patel, linux-kernel,
	alexios.zavras, palmer, paul.walmsley, linux-riscv, allison

On Mon, Aug 19, 2019 at 8:16 PM hch@infradead.org <hch@infradead.org> wrote:
>
> On Thu, Aug 15, 2019 at 08:37:04PM +0000, Atish Patra wrote:
> > We get ton of them. Here is the stack dump.
>
> Looks like we might not need to flush anything at all here as the
> mm_struct was never scheduled to run on any cpu?

If we were using ASID then yes we don't need to flush anything
but currently we don't use ASID due to lack of HW support and
HW can certainly do speculatively page table walks so flushing
local TLB when MM mask is empty might help.

This just my theory and we need to stress test more.

Regards,
Anup

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] RISC-V: Issue a local tlb flush if possible.
  2019-08-19 15:09           ` Anup Patel
@ 2019-08-19 15:10             ` hch
  2019-08-20  0:02               ` Atish Patra
  0 siblings, 1 reply; 18+ messages in thread
From: hch @ 2019-08-19 15:10 UTC (permalink / raw)
  To: Anup Patel
  Cc: hch, Atish Patra, aou, gregkh, Anup Patel, linux-kernel,
	alexios.zavras, palmer, paul.walmsley, linux-riscv, allison

On Mon, Aug 19, 2019 at 08:39:02PM +0530, Anup Patel wrote:
> If we were using ASID then yes we don't need to flush anything
> but currently we don't use ASID due to lack of HW support and
> HW can certainly do speculatively page table walks so flushing
> local TLB when MM mask is empty might help.
> 
> This just my theory and we need to stress test more.

Well, when we context switch away from a mm we always flush the
local tlb.  So either the mm_struct has never been scheduled in,
or we alrady did a local_tlb_flush and we context switched it up.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] RISC-V: Issue a local tlb flush if possible.
  2019-08-19 15:10             ` hch
@ 2019-08-20  0:02               ` Atish Patra
  0 siblings, 0 replies; 18+ messages in thread
From: Atish Patra @ 2019-08-20  0:02 UTC (permalink / raw)
  To: anup, hch
  Cc: linux-riscv, Anup Patel, gregkh, paul.walmsley, aou,
	linux-kernel, allison, alexios.zavras, palmer

On Mon, 2019-08-19 at 08:10 -0700, hch@infradead.org wrote:
> On Mon, Aug 19, 2019 at 08:39:02PM +0530, Anup Patel wrote:
> > If we were using ASID then yes we don't need to flush anything
> > but currently we don't use ASID due to lack of HW support and
> > HW can certainly do speculatively page table walks so flushing
> > local TLB when MM mask is empty might help.
> > 
> > This just my theory and we need to stress test more.
> 
> Well, when we context switch away from a mm we always flush the
> local tlb.  So either the mm_struct has never been scheduled in,

Looking at the stack dump, it looks like this is the case. cpumask is
empty possibly after a fork/exec situation where forked child is being
replaced with actual program that is about to run.

I also looked at x86 & powerpc implementation which doesn't seem to do
anything special if cpumask is empty.

I will send a v2 with no tlb flushing if cpumask is empty.

> or we alrady did a local_tlb_flush and we context switched it up.


Regards,
Atish

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2019-08-20  0:02 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-10  1:43 [PATCH] RISC-V: Issue a local tlb flush if possible Atish Patra
2019-08-10  3:30 ` Anup Patel
2019-08-10  5:28   ` Atish Patra
2019-08-10  6:37 ` Andreas Schwab
2019-08-10  9:21 ` Atish Patra
2019-08-12 14:56 ` Christoph Hellwig
2019-08-13  0:15   ` Atish Patra
2019-08-13 14:30     ` hch
2019-08-15 20:37       ` Atish Patra
2019-08-19 14:46         ` hch
2019-08-19 15:09           ` Anup Patel
2019-08-19 15:10             ` hch
2019-08-20  0:02               ` Atish Patra
2019-08-12 15:36 ` Troy Benjegerdes
2019-08-12 17:13   ` Atish Patra
2019-08-12 17:55   ` Christoph Hellwig
2019-08-13 18:25 ` Paul Walmsley
2019-08-14  1:49   ` Atish Patra

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).