sys_riscv_flush_icache

sys_riscv_flush_icache

[Christoph Hellwig]

Hi Palmer and Paul,

as part of the nommu work and various mailing list discussion I've been
taking a look at the sys_riscv_flush_icache system call, and found
various lose ends, including the local flag which seems completely
bogus.  Btw, are there any test cases covering this system call?

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 1/8] riscv: fix the flags argument type for riscv_riscv_flush_icache

[Christoph Hellwig]

While uinptr_t is identical to unsigned long for all Linux platforms,
defining a flags argument as an uinptr_t doesn't make any sense, so
change it to an unsigned long instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/include/asm/vdso.h | 2 +-
 arch/riscv/kernel/sys_riscv.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/include/asm/vdso.h b/arch/riscv/include/asm/vdso.h
index 7a7fce63c474..a87a9d106220 100644
--- a/arch/riscv/include/asm/vdso.h
+++ b/arch/riscv/include/asm/vdso.h
@@ -27,6 +27,6 @@ struct vdso_data {
 	(void __user *)((unsigned long)(base) + __vdso_##name);			\
 })
 
-asmlinkage long sys_riscv_flush_icache(uintptr_t, uintptr_t, uintptr_t);
+asmlinkage long sys_riscv_flush_icache(uintptr_t, uintptr_t, unsigned long);
 
 #endif /* _ASM_RISCV_VDSO_H */
diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
index f3619f59d85c..61f100acbd4c 100644
--- a/arch/riscv/kernel/sys_riscv.c
+++ b/arch/riscv/kernel/sys_riscv.c
@@ -55,7 +55,7 @@ SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
  * in there for forwards compatibility.
  */
 SYSCALL_DEFINE3(riscv_flush_icache, uintptr_t, start, uintptr_t, end,
-	uintptr_t, flags)
+		unsigned long, flags)
 {
 	/* Check the reserved flags. */
 	if (unlikely(flags & ~SYS_RISCV_FLUSH_ICACHE_ALL))
-- 
2.20.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 2/8] riscv: remove SYS_RISCV_FLUSH_ICACHE_LOCAL #define

[Christoph Hellwig]

It is much better to just check for the supporte flags directly rather than
hiding them behind a flag.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/include/asm/cacheflush.h | 1 -
 arch/riscv/kernel/sys_riscv.c       | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/riscv/include/asm/cacheflush.h b/arch/riscv/include/asm/cacheflush.h
index 555b20b11dc3..c8f159740c38 100644
--- a/arch/riscv/include/asm/cacheflush.h
+++ b/arch/riscv/include/asm/cacheflush.h
@@ -103,6 +103,5 @@ void flush_icache_mm(struct mm_struct *mm, bool local);
  * Bits in sys_riscv_flush_icache()'s flags argument.
  */
 #define SYS_RISCV_FLUSH_ICACHE_LOCAL 1UL
-#define SYS_RISCV_FLUSH_ICACHE_ALL   (SYS_RISCV_FLUSH_ICACHE_LOCAL)
 
 #endif /* _ASM_RISCV_CACHEFLUSH_H */
diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
index 61f100acbd4c..5bad71078b61 100644
--- a/arch/riscv/kernel/sys_riscv.c
+++ b/arch/riscv/kernel/sys_riscv.c
@@ -58,7 +58,7 @@ SYSCALL_DEFINE3(riscv_flush_icache, uintptr_t, start, uintptr_t, end,
 		unsigned long, flags)
 {
 	/* Check the reserved flags. */
-	if (unlikely(flags & ~SYS_RISCV_FLUSH_ICACHE_ALL))
+	if (unlikely(flags & ~SYS_RISCV_FLUSH_ICACHE_LOCAL))
 		return -EINVAL;
 
 	flush_icache_mm(current->mm, flags & SYS_RISCV_FLUSH_ICACHE_LOCAL);
-- 
2.20.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 3/8] riscv: move sys_riscv_flush_icache to cacheflush.c

[Christoph Hellwig]

No real need to split this over to files.  This allows marking
flush_icache_mm static and dropping a superflous argument to it, as
well as consolidating the documentation.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/include/asm/cacheflush.h |  2 --
 arch/riscv/kernel/sys_riscv.c       | 27 ---------------
 arch/riscv/mm/cacheflush.c          | 53 +++++++++++++++++++++--------
 3 files changed, 38 insertions(+), 44 deletions(-)

diff --git a/arch/riscv/include/asm/cacheflush.h b/arch/riscv/include/asm/cacheflush.h
index c8f159740c38..b86ac3a4653a 100644
--- a/arch/riscv/include/asm/cacheflush.h
+++ b/arch/riscv/include/asm/cacheflush.h
@@ -90,12 +90,10 @@ static inline void flush_dcache_page(struct page *page)
 #ifndef CONFIG_SMP
 
 #define flush_icache_all() local_flush_icache_all()
-#define flush_icache_mm(mm, local) flush_icache_all()
 
 #else /* CONFIG_SMP */
 
 void flush_icache_all(void);
-void flush_icache_mm(struct mm_struct *mm, bool local);
 
 #endif /* CONFIG_SMP */
 
diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
index 5bad71078b61..d13e03de3e1a 100644
--- a/arch/riscv/kernel/sys_riscv.c
+++ b/arch/riscv/kernel/sys_riscv.c
@@ -7,7 +7,6 @@
 
 #include <linux/syscalls.h>
 #include <asm/unistd.h>
-#include <asm/cacheflush.h>
 
 static long riscv_sys_mmap(unsigned long addr, unsigned long len,
 			   unsigned long prot, unsigned long flags,
@@ -39,29 +38,3 @@ SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
 	return riscv_sys_mmap(addr, len, prot, flags, fd, offset, 12);
 }
 #endif /* !CONFIG_64BIT */
-
-/*
- * Allows the instruction cache to be flushed from userspace.  Despite RISC-V
- * having a direct 'fence.i' instruction available to userspace (which we
- * can't trap!), that's not actually viable when running on Linux because the
- * kernel might schedule a process on another hart.  There is no way for
- * userspace to handle this without invoking the kernel (as it doesn't know the
- * thread->hart mappings), so we've defined a RISC-V specific system call to
- * flush the instruction cache.
- *
- * sys_riscv_flush_icache() is defined to flush the instruction cache over an
- * address range, with the flush applying to either all threads or just the
- * caller.  We don't currently do anything with the address range, that's just
- * in there for forwards compatibility.
- */
-SYSCALL_DEFINE3(riscv_flush_icache, uintptr_t, start, uintptr_t, end,
-		unsigned long, flags)
-{
-	/* Check the reserved flags. */
-	if (unlikely(flags & ~SYS_RISCV_FLUSH_ICACHE_LOCAL))
-		return -EINVAL;
-
-	flush_icache_mm(current->mm, flags & SYS_RISCV_FLUSH_ICACHE_LOCAL);
-
-	return 0;
-}
diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
index 3f15938dec89..4f78d6552476 100644
--- a/arch/riscv/mm/cacheflush.c
+++ b/arch/riscv/mm/cacheflush.c
@@ -3,8 +3,10 @@
  * Copyright (C) 2017 SiFive
  */
 
+#include <linux/syscalls.h>
 #include <asm/pgtable.h>
 #include <asm/cacheflush.h>
+#include <asm/unistd.h>
 
 #ifdef CONFIG_SMP
 
@@ -15,17 +17,7 @@ void flush_icache_all(void)
 	sbi_remote_fence_i(NULL);
 }
 
-/*
- * Performs an icache flush for the given MM context.  RISC-V has no direct
- * mechanism for instruction cache shoot downs, so instead we send an IPI that
- * informs the remote harts they need to flush their local instruction caches.
- * To avoid pathologically slow behavior in a common case (a bunch of
- * single-hart processes on a many-hart machine, ie 'make -j') we avoid the
- * IPIs for harts that are not currently executing a MM context and instead
- * schedule a deferred local instruction cache flush to be performed before
- * execution resumes on each hart.
- */
-void flush_icache_mm(struct mm_struct *mm, bool local)
+static void flush_icache_mm(bool local)
 {
 	unsigned int cpu;
 	cpumask_t others, hmask, *mask;
@@ -33,7 +25,7 @@ void flush_icache_mm(struct mm_struct *mm, bool local)
 	preempt_disable();
 
 	/* Mark every hart's icache as needing a flush for this MM. */
-	mask = &mm->context.icache_stale_mask;
+	mask = &current->mm->context.icache_stale_mask;
 	cpumask_setall(mask);
 	/* Flush this hart's I$ now, and mark it as flushed. */
 	cpu = smp_processor_id();
@@ -44,9 +36,9 @@ void flush_icache_mm(struct mm_struct *mm, bool local)
 	 * Flush the I$ of other harts concurrently executing, and mark them as
 	 * flushed.
 	 */
-	cpumask_andnot(&others, mm_cpumask(mm), cpumask_of(cpu));
+	cpumask_andnot(&others, mm_cpumask(current->mm), cpumask_of(cpu));
 	local |= cpumask_empty(&others);
-	if (mm != current->active_mm || !local) {
+	if (current->mm != current->active_mm || !local) {
 		riscv_cpuid_to_hartid_mask(&others, &hmask);
 		sbi_remote_fence_i(hmask.bits);
 	} else {
@@ -63,9 +55,40 @@ void flush_icache_mm(struct mm_struct *mm, bool local)
 
 	preempt_enable();
 }
-
+#else
+#define flush_icache_mm(local)	flush_icache_all()
 #endif /* CONFIG_SMP */
 
+/*
+ * Allows the instruction cache to be flushed from userspace.  Despite RISC-V
+ * having a direct 'fence.i' instruction available to userspace (which we
+ * can't trap!), that's not actually viable when running on Linux because the
+ * kernel might schedule a process on another hart.  There is no way for
+ * userspace to handle this without invoking the kernel (as it doesn't know the
+ * thread->hart mappings), so we've defined a RISC-V specific system call to
+ * flush the instruction cache.
+ *
+ * sys_riscv_flush_icache() is defined to flush the instruction cache over an
+ * address range, with the flush applying to either all threads or just the
+ * caller.  We don't currently do anything with the address range, that's just
+ * in there for forwards compatibility.
+ *
+ * To avoid pathologically slow behavior in a common case (a bunch of
+ * single-hart processes on a many-hart machine, ie 'make -j') we avoid the
+ * remove flush for harts that are not currently executing a MM context and
+ * instead schedule a deferred local instruction cache flush to be performed
+ * before execution resumes on each hart.
+ */
+SYSCALL_DEFINE3(riscv_flush_icache, uintptr_t, start, uintptr_t, end,
+		unsigned long, flags)
+{
+	/* Check the reserved flags. */
+	if (unlikely(flags & ~SYS_RISCV_FLUSH_ICACHE_LOCAL))
+		return -EINVAL;
+	flush_icache_mm(flags & SYS_RISCV_FLUSH_ICACHE_LOCAL);
+	return 0;
+}
+
 void flush_icache_pte(pte_t pte)
 {
 	struct page *page = pte_page(pte);
-- 
2.20.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 4/8] riscv: remove the active_mm check in sys_riscv_flush_icache

[Christoph Hellwig]

current->active_mm is always the same as current->mm for user processes
(see Documentation/vm/active_mm.rst for details), and given that we are
directly in a syscall handler this is obviously the case here.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/mm/cacheflush.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
index 4f78d6552476..eed715de4795 100644
--- a/arch/riscv/mm/cacheflush.c
+++ b/arch/riscv/mm/cacheflush.c
@@ -38,7 +38,7 @@ static void flush_icache_mm(bool local)
 	 */
 	cpumask_andnot(&others, mm_cpumask(current->mm), cpumask_of(cpu));
 	local |= cpumask_empty(&others);
-	if (current->mm != current->active_mm || !local) {
+	if (!local) {
 		riscv_cpuid_to_hartid_mask(&others, &hmask);
 		sbi_remote_fence_i(hmask.bits);
 	} else {
-- 
2.20.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 5/8] riscv: actually clear icache_stale_mask for all harts in mm_cpumask

[Christoph Hellwig]

The comment in flush_icache_mm claim that we mark all harts that we
are sending the remote sfence.i to are marked as flushed, but we only
actually do this for the current one.  Fix the code to actually mark
all.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/mm/cacheflush.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
index eed715de4795..dacf72f94d12 100644
--- a/arch/riscv/mm/cacheflush.c
+++ b/arch/riscv/mm/cacheflush.c
@@ -20,21 +20,23 @@ void flush_icache_all(void)
 static void flush_icache_mm(bool local)
 {
 	unsigned int cpu;
-	cpumask_t others, hmask, *mask;
+	cpumask_t others, hmask;
 
 	preempt_disable();
 
-	/* Mark every hart's icache as needing a flush for this MM. */
-	mask = &current->mm->context.icache_stale_mask;
-	cpumask_setall(mask);
+	/*
+	 * Mark the I$ for all harts not concurrently executing as needing a
+	 * flush for this MM.
+	 */
+	cpumask_andnot(&current->mm->context.icache_stale_mask,
+		       cpu_possible_mask, mm_cpumask(current->mm));
+
 	/* Flush this hart's I$ now, and mark it as flushed. */
 	cpu = smp_processor_id();
-	cpumask_clear_cpu(cpu, mask);
 	local_flush_icache_all();
 
 	/*
-	 * Flush the I$ of other harts concurrently executing, and mark them as
-	 * flushed.
+	 * Flush the I$ of other harts concurrently executing.
 	 */
 	cpumask_andnot(&others, mm_cpumask(current->mm), cpumask_of(cpu));
 	local |= cpumask_empty(&others);
-- 
2.20.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 5/8] riscv: actually clear icache_stale_mask for all harts in mm_cpumask

[Atish Patra]

On Thu, 2019-08-22 at 15:56 +0900, Christoph Hellwig wrote:
> The comment in flush_icache_mm claim that we mark all harts that we
> are sending the remote sfence.i to are marked as flushed, but we only
> actually do this for the current one.  Fix the code to actually mark
> all.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  arch/riscv/mm/cacheflush.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> index eed715de4795..dacf72f94d12 100644
> --- a/arch/riscv/mm/cacheflush.c
> +++ b/arch/riscv/mm/cacheflush.c
> @@ -20,21 +20,23 @@ void flush_icache_all(void)
>  static void flush_icache_mm(bool local)
>  {
>  	unsigned int cpu;
> -	cpumask_t others, hmask, *mask;
> +	cpumask_t others, hmask;
>  
>  	preempt_disable();
>  
> -	/* Mark every hart's icache as needing a flush for this MM. */
> -	mask = &current->mm->context.icache_stale_mask;
> -	cpumask_setall(mask);
> +	/*
> +	 * Mark the I$ for all harts not concurrently executing as
> needing a
> +	 * flush for this MM.
> +	 */
> +	cpumask_andnot(&current->mm->context.icache_stale_mask,
> +		       cpu_possible_mask, mm_cpumask(current->mm));
> +

I guess you have added cpu_possible_mask keeping cpu hotplug in mind
for future.

I think it should be cpu_present_mask instead of cpu_possible_mask as
they can be different in case where some cpus are just waiting in the
boot loader.

>  	/* Flush this hart's I$ now, and mark it as flushed. */
>  	cpu = smp_processor_id();
> -	cpumask_clear_cpu(cpu, mask);
>  	local_flush_icache_all();
>  
>  	/*
> -	 * Flush the I$ of other harts concurrently executing, and mark
> them as
> -	 * flushed.
> +	 * Flush the I$ of other harts concurrently executing.
>  	 */
>  	cpumask_andnot(&others, mm_cpumask(current->mm),
> cpumask_of(cpu));
>  	local |= cpumask_empty(&others);

Otherwise, looks good.

Reviewed-by: Atish Patra <atish.patra@wdc.com>

-- 
Regards,
Atish
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 5/8] riscv: actually clear icache_stale_mask for all harts in mm_cpumask

[hch]

On Thu, Aug 22, 2019 at 06:29:35PM +0000, Atish Patra wrote:
> I guess you have added cpu_possible_mask keeping cpu hotplug in mind
> for future.
> 
> I think it should be cpu_present_mask instead of cpu_possible_mask as
> they can be different in case where some cpus are just waiting in the
> boot loader.

I don't think it matters.  The only place where we actually check
icache_stale_mask is in flush_icache_deferred, which only does
a cpumask_test_cpu for the currently running CPU.  So I'd rather
opt for setting a few more bits and prepare for cpu hotplug.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 6/8] riscv: use get_cpu and put_cpu in sys_riscv_flush_icache

[Christoph Hellwig]

Use get_cpu/put_cpu instead of opencoding them.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/mm/cacheflush.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
index dacf72f94d12..9180b2e93058 100644
--- a/arch/riscv/mm/cacheflush.c
+++ b/arch/riscv/mm/cacheflush.c
@@ -19,11 +19,9 @@ void flush_icache_all(void)
 
 static void flush_icache_mm(bool local)
 {
-	unsigned int cpu;
+	unsigned int cpu = get_cpu();
 	cpumask_t others, hmask;
 
-	preempt_disable();
-
 	/*
 	 * Mark the I$ for all harts not concurrently executing as needing a
 	 * flush for this MM.
@@ -32,7 +30,6 @@ static void flush_icache_mm(bool local)
 		       cpu_possible_mask, mm_cpumask(current->mm));
 
 	/* Flush this hart's I$ now, and mark it as flushed. */
-	cpu = smp_processor_id();
 	local_flush_icache_all();
 
 	/*
@@ -55,7 +52,7 @@ static void flush_icache_mm(bool local)
 		smp_mb();
 	}
 
-	preempt_enable();
+	put_cpu();
 }
 #else
 #define flush_icache_mm(local)	flush_icache_all()
-- 
2.20.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 6/8] riscv: use get_cpu and put_cpu in sys_riscv_flush_icache

[Atish Patra]

On Thu, 2019-08-22 at 15:56 +0900, Christoph Hellwig wrote:
> Use get_cpu/put_cpu instead of opencoding them.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  arch/riscv/mm/cacheflush.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> index dacf72f94d12..9180b2e93058 100644
> --- a/arch/riscv/mm/cacheflush.c
> +++ b/arch/riscv/mm/cacheflush.c
> @@ -19,11 +19,9 @@ void flush_icache_all(void)
>  
>  static void flush_icache_mm(bool local)
>  {
> -	unsigned int cpu;
> +	unsigned int cpu = get_cpu();

I thought it is recommended to put variables with assignment at the
end. But I can't find anything about it in coding guidelines. So it
might be all in my head ;-)

>  	cpumask_t others, hmask;
>  
> -	preempt_disable();
> -
>  	/*
>  	 * Mark the I$ for all harts not concurrently executing as
> needing a
>  	 * flush for this MM.
> @@ -32,7 +30,6 @@ static void flush_icache_mm(bool local)
>  		       cpu_possible_mask, mm_cpumask(current->mm));
>  
>  	/* Flush this hart's I$ now, and mark it as flushed. */
> -	cpu = smp_processor_id();
>  	local_flush_icache_all();
>  
>  	/*
> @@ -55,7 +52,7 @@ static void flush_icache_mm(bool local)
>  		smp_mb();
>  	}
>  
> -	preempt_enable();
> +	put_cpu();
>  }
>  #else
>  #define flush_icache_mm(local)	flush_icache_all()

Otherwise, looks good.
Reviewed-by: Atish Patra <atish.patra@wdc.com>

-- 
Regards,
Atish
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 6/8] riscv: use get_cpu and put_cpu in sys_riscv_flush_icache

[hch]

On Thu, Aug 22, 2019 at 05:49:20PM +0000, Atish Patra wrote:
> On Thu, 2019-08-22 at 15:56 +0900, Christoph Hellwig wrote:
> > Use get_cpu/put_cpu instead of opencoding them.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  arch/riscv/mm/cacheflush.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> > index dacf72f94d12..9180b2e93058 100644
> > --- a/arch/riscv/mm/cacheflush.c
> > +++ b/arch/riscv/mm/cacheflush.c
> > @@ -19,11 +19,9 @@ void flush_icache_all(void)
> >  
> >  static void flush_icache_mm(bool local)
> >  {
> > -	unsigned int cpu;
> > +	unsigned int cpu = get_cpu();
> 
> I thought it is recommended to put variables with assignment at the
> end. But I can't find anything about it in coding guidelines. So it
> might be all in my head ;-)

I've never heard of that before.  In fact I usually keep them at
the beginning.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 6/8] riscv: use get_cpu and put_cpu in sys_riscv_flush_icache

[Atish Patra]

On Mon, 2019-08-26 at 13:38 +0200, hch@lst.de wrote:
> On Thu, Aug 22, 2019 at 05:49:20PM +0000, Atish Patra wrote:
> > On Thu, 2019-08-22 at 15:56 +0900, Christoph Hellwig wrote:
> > > Use get_cpu/put_cpu instead of opencoding them.
> > > 
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > >  arch/riscv/mm/cacheflush.c | 7 ++-----
> > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/arch/riscv/mm/cacheflush.c
> > > b/arch/riscv/mm/cacheflush.c
> > > index dacf72f94d12..9180b2e93058 100644
> > > --- a/arch/riscv/mm/cacheflush.c
> > > +++ b/arch/riscv/mm/cacheflush.c
> > > @@ -19,11 +19,9 @@ void flush_icache_all(void)
> > >  
> > >  static void flush_icache_mm(bool local)
> > >  {
> > > -	unsigned int cpu;
> > > +	unsigned int cpu = get_cpu();
> > 
> > I thought it is recommended to put variables with assignment at the
> > end. But I can't find anything about it in coding guidelines. So it
> > might be all in my head ;-)
> 
> I've never heard of that before.  In fact I usually keep them at
> the beginning.

Ok. I will keep that in mind :)

-- 
Regards,
Atish
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 7/8] riscv: improve the local flushing logic in sys_riscv_flush_icache

[Christoph Hellwig]

If we have to offload any remote sfence the SBI we might as well let it
handle the local one as well.  This significantly simplifies the cpumask
operations and streamlines the code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/mm/cacheflush.c | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
index 9180b2e93058..8f1134715fec 100644
--- a/arch/riscv/mm/cacheflush.c
+++ b/arch/riscv/mm/cacheflush.c
@@ -20,7 +20,6 @@ void flush_icache_all(void)
 static void flush_icache_mm(bool local)
 {
 	unsigned int cpu = get_cpu();
-	cpumask_t others, hmask;
 
 	/*
 	 * Mark the I$ for all harts not concurrently executing as needing a
@@ -29,27 +28,23 @@ static void flush_icache_mm(bool local)
 	cpumask_andnot(&current->mm->context.icache_stale_mask,
 		       cpu_possible_mask, mm_cpumask(current->mm));
 
-	/* Flush this hart's I$ now, and mark it as flushed. */
-	local_flush_icache_all();
-
 	/*
-	 * Flush the I$ of other harts concurrently executing.
+	 * It's assumed that at least one strongly ordered operation is
+	 * performed on this hart between setting a hart's cpumask bit and
+	 * scheduling this MM context on that hart.  Sending an SBI remote
+	 * message will do this, but in the case where no messages are sent we
+	 * still need to order this hart's writes with flush_icache_deferred().
 	 */
-	cpumask_andnot(&others, mm_cpumask(current->mm), cpumask_of(cpu));
-	local |= cpumask_empty(&others);
-	if (!local) {
-		riscv_cpuid_to_hartid_mask(&others, &hmask);
-		sbi_remote_fence_i(hmask.bits);
-	} else {
-		/*
-		 * It's assumed that at least one strongly ordered operation is
-		 * performed on this hart between setting a hart's cpumask bit
-		 * and scheduling this MM context on that hart.  Sending an SBI
-		 * remote message will do this, but in the case where no
-		 * messages are sent we still need to order this hart's writes
-		 * with flush_icache_deferred().
-		 */
+	cpu = get_cpu();
+	if (local ||
+	    cpumask_any_but(mm_cpumask(current->mm), cpu) >= nr_cpu_ids) {
+		local_flush_icache_all();
 		smp_mb();
+	} else {
+		cpumask_t hmask;
+
+		riscv_cpuid_to_hartid_mask(mm_cpumask(current->mm), &hmask);
+		sbi_remote_fence_i(cpumask_bits(&hmask));
 	}
 
 	put_cpu();
-- 
2.20.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 7/8] riscv: improve the local flushing logic in sys_riscv_flush_icache

[Atish Patra]

On Thu, 2019-08-22 at 15:56 +0900, Christoph Hellwig wrote:
> If we have to offload any remote sfence the SBI we might as well let
> it
> handle the local one as well.  This significantly simplifies the
> cpumask
> operations and streamlines the code.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  arch/riscv/mm/cacheflush.c | 33 ++++++++++++++-------------------
>  1 file changed, 14 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> index 9180b2e93058..8f1134715fec 100644
> --- a/arch/riscv/mm/cacheflush.c
> +++ b/arch/riscv/mm/cacheflush.c
> @@ -20,7 +20,6 @@ void flush_icache_all(void)
>  static void flush_icache_mm(bool local)
>  {
>  	unsigned int cpu = get_cpu();

get_cpu should be removed from here as it is called again in below.
> -	cpumask_t others, hmask;
>  
>  	/*
>  	 * Mark the I$ for all harts not concurrently executing as
> needing a
> @@ -29,27 +28,23 @@ static void flush_icache_mm(bool local)
>  	cpumask_andnot(&current->mm->context.icache_stale_mask,
>  		       cpu_possible_mask, mm_cpumask(current->mm));
>  
> -	/* Flush this hart's I$ now, and mark it as flushed. */
> -	local_flush_icache_all();
> -
>  	/*
> -	 * Flush the I$ of other harts concurrently executing.
> +	 * It's assumed that at least one strongly ordered operation is
> +	 * performed on this hart between setting a hart's cpumask bit
> and
> +	 * scheduling this MM context on that hart.  Sending an SBI
> remote
> +	 * message will do this, but in the case where no messages are
> sent we
> +	 * still need to order this hart's writes with
> flush_icache_deferred().
>  	 */
> -	cpumask_andnot(&others, mm_cpumask(current->mm),
> cpumask_of(cpu));
> -	local |= cpumask_empty(&others);
> -	if (!local) {
> -		riscv_cpuid_to_hartid_mask(&others, &hmask);
> -		sbi_remote_fence_i(hmask.bits);
> -	} else {
> -		/*
> -		 * It's assumed that at least one strongly ordered
> operation is
> -		 * performed on this hart between setting a hart's
> cpumask bit
> -		 * and scheduling this MM context on that
> hart.  Sending an SBI
> -		 * remote message will do this, but in the case where
> no
> -		 * messages are sent we still need to order this hart's
> writes
> -		 * with flush_icache_deferred().
> -		 */
> +	cpu = get_cpu();
> +	if (local ||
> +	    cpumask_any_but(mm_cpumask(current->mm), cpu) >=
> nr_cpu_ids) {
> +		local_flush_icache_all();
>  		smp_mb();
> +	} else {
> +		cpumask_t hmask;
> +
> +		riscv_cpuid_to_hartid_mask(mm_cpumask(current->mm),
> &hmask);
> +		sbi_remote_fence_i(cpumask_bits(&hmask));
>  	}
>  
>  	put_cpu();

Otherwise, looks good to me.

Reviewed-by: Atish Patra <atish.patra@wdc.com>

-- 
Regards,
Atish
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 7/8] riscv: improve the local flushing logic in sys_riscv_flush_icache

[hch]

On Thu, Aug 22, 2019 at 05:34:46PM +0000, Atish Patra wrote:
> > index 9180b2e93058..8f1134715fec 100644
> > --- a/arch/riscv/mm/cacheflush.c
> > +++ b/arch/riscv/mm/cacheflush.c
> > @@ -20,7 +20,6 @@ void flush_icache_all(void)
> >  static void flush_icache_mm(bool local)
> >  {
> >  	unsigned int cpu = get_cpu();
> 
> get_cpu should be removed from here as it is called again in below.

Indeed.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 8/8] riscv: ignore the SYS_RISCV_FLUSH_ICACHE_LOCAL flag

[Christoph Hellwig]

The SYS_RISCV_FLUSH_ICACHE_LOCAL is built on the flawed assumption that
there is such a thing as a local cpu outside of in-kernel preemption
disabled sections.  Just ignore the flag as it can't be used in a safe
way.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/include/asm/cacheflush.h |  2 +-
 arch/riscv/mm/cacheflush.c          | 13 ++++++++-----
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/include/asm/cacheflush.h b/arch/riscv/include/asm/cacheflush.h
index b86ac3a4653a..3c18d956c970 100644
--- a/arch/riscv/include/asm/cacheflush.h
+++ b/arch/riscv/include/asm/cacheflush.h
@@ -100,6 +100,6 @@ void flush_icache_all(void);
 /*
  * Bits in sys_riscv_flush_icache()'s flags argument.
  */
-#define SYS_RISCV_FLUSH_ICACHE_LOCAL 1UL
+#define SYS_RISCV_FLUSH_ICACHE_LOCAL 1UL /* ignored */
 
 #endif /* _ASM_RISCV_CACHEFLUSH_H */
diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
index 8f1134715fec..4b6ecc3431e2 100644
--- a/arch/riscv/mm/cacheflush.c
+++ b/arch/riscv/mm/cacheflush.c
@@ -17,7 +17,7 @@ void flush_icache_all(void)
 	sbi_remote_fence_i(NULL);
 }
 
-static void flush_icache_mm(bool local)
+static void flush_icache_mm(void)
 {
 	unsigned int cpu = get_cpu();
 
@@ -36,8 +36,7 @@ static void flush_icache_mm(bool local)
 	 * still need to order this hart's writes with flush_icache_deferred().
 	 */
 	cpu = get_cpu();
-	if (local ||
-	    cpumask_any_but(mm_cpumask(current->mm), cpu) >= nr_cpu_ids) {
+	if (cpumask_any_but(mm_cpumask(current->mm), cpu) >= nr_cpu_ids) {
 		local_flush_icache_all();
 		smp_mb();
 	} else {
@@ -50,7 +49,7 @@ static void flush_icache_mm(bool local)
 	put_cpu();
 }
 #else
-#define flush_icache_mm(local)	flush_icache_all()
+#define flush_icache_mm()	flush_icache_all()
 #endif /* CONFIG_SMP */
 
 /*
@@ -72,6 +71,10 @@ static void flush_icache_mm(bool local)
  * remove flush for harts that are not currently executing a MM context and
  * instead schedule a deferred local instruction cache flush to be performed
  * before execution resumes on each hart.
+ *
+ * Note that we ignore the SYS_RISCV_FLUSH_ICACHE_LOCAL flag, as there is
+ * absolutely not way to ensure the local CPU is still the same by the time we
+ * return from the syscall.
  */
 SYSCALL_DEFINE3(riscv_flush_icache, uintptr_t, start, uintptr_t, end,
 		unsigned long, flags)
@@ -79,7 +82,7 @@ SYSCALL_DEFINE3(riscv_flush_icache, uintptr_t, start, uintptr_t, end,
 	/* Check the reserved flags. */
 	if (unlikely(flags & ~SYS_RISCV_FLUSH_ICACHE_LOCAL))
 		return -EINVAL;
-	flush_icache_mm(flags & SYS_RISCV_FLUSH_ICACHE_LOCAL);
+	flush_icache_mm();
 	return 0;
 }
 
-- 
2.20.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 8/8] riscv: ignore the SYS_RISCV_FLUSH_ICACHE_LOCAL flag

[Palmer Dabbelt]

On Wed, 21 Aug 2019 23:56:12 PDT (-0700), Christoph Hellwig wrote:
> The SYS_RISCV_FLUSH_ICACHE_LOCAL is built on the flawed assumption that
> there is such a thing as a local cpu outside of in-kernel preemption
> disabled sections.  Just ignore the flag as it can't be used in a safe
> way.

This is meant to perform a context-local flush, not a cpu-local flush.  The 
whole point here is that userspace doesn't know anything about CPUs, just 
contexts -- that's why we have this deferred flush mechanism.  I think the 
logic is complicated but sound, and removing this will almost certainly lead to 
huge performance degradation.

Maybe I'm missing something, what is the specific issue?

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  arch/riscv/include/asm/cacheflush.h |  2 +-
>  arch/riscv/mm/cacheflush.c          | 13 ++++++++-----
>  2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/arch/riscv/include/asm/cacheflush.h b/arch/riscv/include/asm/cacheflush.h
> index b86ac3a4653a..3c18d956c970 100644
> --- a/arch/riscv/include/asm/cacheflush.h
> +++ b/arch/riscv/include/asm/cacheflush.h
> @@ -100,6 +100,6 @@ void flush_icache_all(void);
>  /*
>   * Bits in sys_riscv_flush_icache()'s flags argument.
>   */
> -#define SYS_RISCV_FLUSH_ICACHE_LOCAL 1UL
> +#define SYS_RISCV_FLUSH_ICACHE_LOCAL 1UL /* ignored */
>
>  #endif /* _ASM_RISCV_CACHEFLUSH_H */
> diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> index 8f1134715fec..4b6ecc3431e2 100644
> --- a/arch/riscv/mm/cacheflush.c
> +++ b/arch/riscv/mm/cacheflush.c
> @@ -17,7 +17,7 @@ void flush_icache_all(void)
>  	sbi_remote_fence_i(NULL);
>  }
>
> -static void flush_icache_mm(bool local)
> +static void flush_icache_mm(void)
>  {
>  	unsigned int cpu = get_cpu();
>
> @@ -36,8 +36,7 @@ static void flush_icache_mm(bool local)
>  	 * still need to order this hart's writes with flush_icache_deferred().
>  	 */
>  	cpu = get_cpu();
> -	if (local ||
> -	    cpumask_any_but(mm_cpumask(current->mm), cpu) >= nr_cpu_ids) {
> +	if (cpumask_any_but(mm_cpumask(current->mm), cpu) >= nr_cpu_ids) {
>  		local_flush_icache_all();
>  		smp_mb();
>  	} else {
> @@ -50,7 +49,7 @@ static void flush_icache_mm(bool local)
>  	put_cpu();
>  }
>  #else
> -#define flush_icache_mm(local)	flush_icache_all()
> +#define flush_icache_mm()	flush_icache_all()
>  #endif /* CONFIG_SMP */
>
>  /*
> @@ -72,6 +71,10 @@ static void flush_icache_mm(bool local)
>   * remove flush for harts that are not currently executing a MM context and
>   * instead schedule a deferred local instruction cache flush to be performed
>   * before execution resumes on each hart.
> + *
> + * Note that we ignore the SYS_RISCV_FLUSH_ICACHE_LOCAL flag, as there is
> + * absolutely not way to ensure the local CPU is still the same by the time we
> + * return from the syscall.
>   */
>  SYSCALL_DEFINE3(riscv_flush_icache, uintptr_t, start, uintptr_t, end,
>  		unsigned long, flags)
> @@ -79,7 +82,7 @@ SYSCALL_DEFINE3(riscv_flush_icache, uintptr_t, start, uintptr_t, end,
>  	/* Check the reserved flags. */
>  	if (unlikely(flags & ~SYS_RISCV_FLUSH_ICACHE_LOCAL))
>  		return -EINVAL;
> -	flush_icache_mm(flags & SYS_RISCV_FLUSH_ICACHE_LOCAL);
> +	flush_icache_mm();
>  	return 0;
>  }

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 8/8] riscv: ignore the SYS_RISCV_FLUSH_ICACHE_LOCAL flag

[Christoph Hellwig]

On Tue, Aug 27, 2019 at 06:10:33PM -0700, Palmer Dabbelt wrote:
> This is meant to perform a context-local flush, not a cpu-local flush.  The 
> whole point here is that userspace doesn't know anything about CPUs, just 
> contexts -- that's why we have this deferred flush mechanism.  I think the 
> logic is complicated but sound, and removing this will almost certainly 
> lead to huge performance degradation.

All calls to flush_icache_mm are local to the context.  Take a look at
what the current code does:

 - set all bits in context.icache_stale_mask
 - clear the current cpu from context.icache_stale_mask
 - flush the cpu local icache
 - create a local others mask containing every cpu running the context
   except for the current one
 - now if others is empty OR the local flag is set don't do anything
   but a memory barrier, else flush the other cpus

>
> Maybe I'm missing something, what is the specific issue?

The issue is that the current implementation of
SYS_RISCV_FLUSH_ICACHE_LOCAL only flushes the icache of the currently
running core, which is an interface that can't be used correctly.

riscv_flush_icache without that flag on the other handle already just
flushes the caches for the cpus that run the current context, and then
causes a deferred flush if the context gets run on another cpu
eventually.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 8/8] riscv: ignore the SYS_RISCV_FLUSH_ICACHE_LOCAL flag

[Palmer Dabbelt]

On Tue, 27 Aug 2019 23:09:42 PDT (-0700), Christoph Hellwig wrote:
> On Tue, Aug 27, 2019 at 06:10:33PM -0700, Palmer Dabbelt wrote:
>> This is meant to perform a context-local flush, not a cpu-local flush.  The
>> whole point here is that userspace doesn't know anything about CPUs, just
>> contexts -- that's why we have this deferred flush mechanism.  I think the
>> logic is complicated but sound, and removing this will almost certainly
>> lead to huge performance degradation.
>
> All calls to flush_icache_mm are local to the context.  Take a look at
> what the current code does:
>
>  - set all bits in context.icache_stale_mask
>  - clear the current cpu from context.icache_stale_mask
>  - flush the cpu local icache
>  - create a local others mask containing every cpu running the context
>    except for the current one
>  - now if others is empty OR the local flag is set don't do anything
>    but a memory barrier, else flush the other cpus
>
>>
>> Maybe I'm missing something, what is the specific issue?
>
> The issue is that the current implementation of
> SYS_RISCV_FLUSH_ICACHE_LOCAL only flushes the icache of the currently
> running core, which is an interface that can't be used correctly.

This is used by userspace as a thread-local icache barrier: there's an 
immediate fence on the current hart, and one will be executed before that 
thread makes it to userspace on another hart.  As far as I can tell this is 
implemented correctly but not optimally: there's always a fence, but we emit an 
unnecessary fence when a different thread in the same context is scheduled on a 
different hart.

I suppose maybe we should attach the local fence mask to a task_struct instead of an 
mm_struct, which would trade off an extra load in the scheduler (to check both 
places) or more fences in the global case (on every thread getting scheduled) 
for fewer fences in the local case.  I feel like it's not really worth worrying 
about this for now.

The construct seems reasonable to me.

> riscv_flush_icache without that flag on the other handle already just
> flushes the caches for the cpus that run the current context, and then
> causes a deferred flush if the context gets run on another cpu
> eventually.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 8/8] riscv: ignore the SYS_RISCV_FLUSH_ICACHE_LOCAL flag

[Christoph Hellwig]

On Tue, Sep 03, 2019 at 11:46:33AM -0700, Palmer Dabbelt wrote:
> This is used by userspace as a thread-local icache barrier: there's an 
> immediate fence on the current hart, and one will be executed before that 
> thread makes it to userspace on another hart.  As far as I can tell this is 
> implemented correctly but not optimally: there's always a fence, but we 
> emit an unnecessary fence when a different thread in the same context is 
> scheduled on a different hart.
>
> I suppose maybe we should attach the local fence mask to a task_struct 
> instead of an mm_struct, which would trade off an extra load in the 
> scheduler (to check both places) or more fences in the global case (on 
> every thread getting scheduled) for fewer fences in the local case.  I feel 
> like it's not really worth worrying about this for now.
>
> The construct seems reasonable to me.

I haven't been able to poke holes into that idea yet, but I'll try
a bit more once I find a little time.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 8/8] riscv: ignore the SYS_RISCV_FLUSH_ICACHE_LOCAL flag

[Palmer Dabbelt]

On Fri, 06 Sep 2019 10:07:25 PDT (-0700), Christoph Hellwig wrote:
> On Tue, Sep 03, 2019 at 11:46:33AM -0700, Palmer Dabbelt wrote:
>> This is used by userspace as a thread-local icache barrier: there's an
>> immediate fence on the current hart, and one will be executed before that
>> thread makes it to userspace on another hart.  As far as I can tell this is
>> implemented correctly but not optimally: there's always a fence, but we
>> emit an unnecessary fence when a different thread in the same context is
>> scheduled on a different hart.
>>
>> I suppose maybe we should attach the local fence mask to a task_struct
>> instead of an mm_struct, which would trade off an extra load in the
>> scheduler (to check both places) or more fences in the global case (on
>> every thread getting scheduled) for fewer fences in the local case.  I feel
>> like it's not really worth worrying about this for now.
>>
>> The construct seems reasonable to me.
>
> I haven't been able to poke holes into that idea yet, but I'll try
> a bit more once I find a little time.

OK, well, LMK if you find anything -- it's in the user ABI, which is a bad 
place to have something fundamentally broken :)

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv