[PATCH v2 0/2] riscv: Align shared mappings to avoid cache aliasing

[PATCH v2 0/2] riscv: Align shared mappings to avoid cache aliasing

[Marc Gauthier]

Ensure optimal performance on larger VIPT caches by aligning shared
mappings to the maximum cache "span" (line size * number of sets) of
all CPU L1 instruction and data caches (L2 and up are rarely VIPT).
If the device tree does not provide cache parameters, use a conservative
16 KB alignment:  only large enough to avoid aliasing in most VIPT caches.

This patchset is based on Linux 5.4-rc7 and tested in simulation.

Changes in v2:
- Fix formatting per scripts/checkpatch.pl
- Edit include/asm/Kbuild to reflect shmparam.h addition

Signed-off-by: Marc Gauthier <consult-mg@gstardust.com>

Marc Gauthier (2):
  riscv: Align shared mappings to SHMLBA
  riscv: Set SHMLBA according to cache geometry

 arch/riscv/include/asm/Kbuild     |   1 -
 arch/riscv/include/asm/pgtable.h  |   4 ++
 arch/riscv/include/asm/shmparam.h |  12 ++++
 arch/riscv/kernel/cacheinfo.c     |  52 ++++++++++++++
 arch/riscv/kernel/sys_riscv.c     | 113 ++++++++++++++++++++++++++++++
 5 files changed, 181 insertions(+), 1 deletion(-)
 create mode 100644 arch/riscv/include/asm/shmparam.h

-- 
2.17.1


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

[PATCH v2 1/2] riscv: Align shared mappings to SHMLBA

[Marc Gauthier]

Align shared mappings according to SHMLBA for VIPT cache performance.

arch_get_unmapped_area() and arch_get_unmapped_area_topdown() are
essentially copies of their default implementations in mm/mmap.c,
modified to align the address to SHMLBA for shared mappings, i.e.
where MAP_SHARED is specified or a file pointer is provided.

Allow MAP_FIXED to request unaligned shared mappings.  Although this
may potentially reduce performance, very little software does this, as
it is not portable across architectures that enforce alignment.

Signed-off-by: Marc Gauthier <consult-mg@gstardust.com>
---
 arch/riscv/include/asm/pgtable.h |   4 ++
 arch/riscv/kernel/sys_riscv.c    | 113 +++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+)

diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
index d3221017194d..7d1cc47ac5f9 100644
--- a/arch/riscv/include/asm/pgtable.h
+++ b/arch/riscv/include/asm/pgtable.h
@@ -436,6 +436,10 @@ extern void *dtb_early_va;
 extern void setup_bootmem(void);
 extern void paging_init(void);
 
+/* We provide arch_get_unmapped_area to handle VIPT caches efficiently. */
+#define HAVE_ARCH_UNMAPPED_AREA
+#define HAVE_ARCH_UNMAPPED_AREA_TOPDOWN
+
 /*
  * Task size is 0x4000000000 for RV64 or 0x9fc00000 for RV32.
  * Note that PGDIR_SIZE must evenly divide TASK_SIZE.
diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
index f3619f59d85c..3e739b30b1f7 100644
--- a/arch/riscv/kernel/sys_riscv.c
+++ b/arch/riscv/kernel/sys_riscv.c
@@ -3,11 +3,15 @@
  * Copyright (C) 2012 Regents of the University of California
  * Copyright (C) 2014 Darius Rad <darius@bluespec.com>
  * Copyright (C) 2017 SiFive
+ * Copyright (C) 2019 Aril Inc
  */
 
 #include <linux/syscalls.h>
 #include <asm/unistd.h>
 #include <asm/cacheflush.h>
+#include <linux/shm.h>
+#include <linux/mman.h>
+#include <linux/security.h>
 
 static long riscv_sys_mmap(unsigned long addr, unsigned long len,
 			   unsigned long prot, unsigned long flags,
@@ -65,3 +69,112 @@ SYSCALL_DEFINE3(riscv_flush_icache, uintptr_t, start, uintptr_t, end,
 
 	return 0;
 }
+
+/*
+ * RISC-V requires implementations to function correctly in the presence
+ * of cache aliasing, regardless of page alignment.  It says nothing about
+ * performance.  To ensure healthy performance with commonly implemented
+ * VIPT caches, the following code avoids most cases of cache aliasing by
+ * aligning shared mappings such that all mappings of a given physical
+ * page of an object are at a multiple of SHMLBA bytes from each other.
+ *
+ * It does not enforce alignment.  Using MAP_FIXED to request unaligned
+ * shared mappings is not common, and may perform poorly with VIPT caches.
+ */
+unsigned long
+arch_get_unmapped_area(struct file *filp, unsigned long addr,
+			unsigned long len, unsigned long pgoff,
+			unsigned long flags)
+{
+	struct mm_struct *mm = current->mm;
+	struct vm_area_struct *vma, *prev;
+	struct vm_unmapped_area_info info;
+	const unsigned long pgoffset = pgoff << PAGE_SHIFT;
+	int do_align = (filp || (flags & MAP_SHARED));
+
+	if (len > TASK_SIZE - mmap_min_addr)
+		return -ENOMEM;
+
+	if (flags & MAP_FIXED)
+		return addr;
+
+	if (addr) {
+		if (do_align)
+			addr = ALIGN(addr, SHMLBA) + (pgoffset & (SHMLBA - 1));
+		else
+			addr = PAGE_ALIGN(addr);
+		vma = find_vma_prev(mm, addr, &prev);
+		if (TASK_SIZE - len >= addr && addr >= mmap_min_addr &&
+		    (!vma || addr + len <= vm_start_gap(vma)) &&
+		    (!prev || addr >= vm_end_gap(prev)))
+			return addr;
+	}
+
+	info.flags = 0;
+	info.length = len;
+	info.low_limit = mm->mmap_base;
+	info.high_limit = TASK_SIZE;
+	info.align_mask = do_align ? SHMLBA - 1 : 0;
+	info.align_offset = pgoffset;
+	return vm_unmapped_area(&info);
+}
+
+/*
+ * Similar to arch_get_unmapped_area(), but allocating top-down from below the
+ * stack's low limit (the base).
+ */
+unsigned long
+arch_get_unmapped_area_topdown(struct file *filp, unsigned long addr,
+				unsigned long len, unsigned long pgoff,
+				unsigned long flags)
+{
+	struct vm_area_struct *vma, *prev;
+	struct mm_struct *mm = current->mm;
+	struct vm_unmapped_area_info info;
+	const unsigned long pgoffset = pgoff << PAGE_SHIFT;
+	int do_align = (filp || (flags & MAP_SHARED));
+
+	/* requested length too big for entire address space */
+	if (len > TASK_SIZE - mmap_min_addr)
+		return -ENOMEM;
+
+	if (flags & MAP_FIXED)
+		return addr;
+
+	/* requesting a specific address */
+	if (addr) {
+		if (do_align)
+			addr = ALIGN(addr, SHMLBA) + (pgoffset & (SHMLBA - 1));
+		else
+			addr = PAGE_ALIGN(addr);
+		vma = find_vma_prev(mm, addr, &prev);
+		if (TASK_SIZE - len >= addr && addr >= mmap_min_addr &&
+				(!vma || addr + len <= vm_start_gap(vma)) &&
+				(!prev || addr >= vm_end_gap(prev)))
+			return addr;
+	}
+
+	info.flags = VM_UNMAPPED_AREA_TOPDOWN;
+	info.length = len;
+	info.low_limit = max(PAGE_SIZE, mmap_min_addr);
+	info.high_limit = mm->mmap_base;
+	info.align_mask = do_align ? SHMLBA - 1 : 0;
+	info.align_offset = pgoffset;
+	addr = vm_unmapped_area(&info);
+
+	/*
+	 * A failed mmap() very likely causes application failure,
+	 * so fall back to the bottom-up function here. This scenario
+	 * can happen with large stack limits and large mmap()
+	 * allocations.
+	 */
+	if (offset_in_page(addr)) {
+		VM_BUG_ON(addr != -ENOMEM);
+		info.flags = 0;
+		info.low_limit = TASK_UNMAPPED_BASE;
+		info.high_limit = TASK_SIZE;
+		addr = vm_unmapped_area(&info);
+	}
+
+	return addr;
+}
-- 
2.17.1


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

[PATCH v2 2/2] riscv: Set SHMLBA according to cache geometry

[Marc Gauthier]

Set SHMLBA to the maximum cache "span" (line size * number of sets) of
all CPU L1 instruction and data caches (L2 and up are rarely VIPT).
This avoids VIPT cache aliasing with minimal alignment constraints.

If the device tree does not provide cache parameters, use a conservative
default of 16 KB:  only large enough to avoid aliasing in most VIPT caches.

Signed-off-by: Marc Gauthier <consult-mg@gstardust.com>
---
 arch/riscv/include/asm/Kbuild     |  1 -
 arch/riscv/include/asm/shmparam.h | 12 +++++++
 arch/riscv/kernel/cacheinfo.c     | 52 +++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 1 deletion(-)
 create mode 100644 arch/riscv/include/asm/shmparam.h

diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild
index 16970f246860..3905765807af 100644
--- a/arch/riscv/include/asm/Kbuild
+++ b/arch/riscv/include/asm/Kbuild
@@ -27,7 +27,6 @@ generic-y += percpu.h
 generic-y += preempt.h
 generic-y += sections.h
 generic-y += serial.h
-generic-y += shmparam.h
 generic-y += topology.h
 generic-y += trace_clock.h
 generic-y += unaligned.h
diff --git a/arch/riscv/include/asm/shmparam.h b/arch/riscv/include/asm/shmparam.h
new file mode 100644
index 000000000000..9b6a98153648
--- /dev/null
+++ b/arch/riscv/include/asm/shmparam.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_RISCV_SHMPARAM_H
+#define _ASM_RISCV_SHMPARAM_H
+
+/*
+ * Minimum alignment of shared memory segments as a function of cache geometry.
+ */
+#define	SHMLBA	arch_shmlba()
+
+long arch_shmlba(void);
+
+#endif /* _ASM_RISCV_SHMPARAM_H */
diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
index 4c90c07d8c39..1bc7df8577d6 100644
--- a/arch/riscv/kernel/cacheinfo.c
+++ b/arch/riscv/kernel/cacheinfo.c
@@ -1,12 +1,61 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
  * Copyright (C) 2017 SiFive
+ * Copyright (C) 2019 Aril Inc
  */
 
 #include <linux/cacheinfo.h>
 #include <linux/cpu.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/mm.h>
+
+static long shmlba;
+
+
+/*
+ * Assuming  cache size = line size * #sets * N  for N-way associative caches,
+ * return the max cache "span" == (line size * #sets) == (cache size / N)
+ * across all L1 caches, or 0 if cache parameters are not available.
+ * VIPT caches with span > min page size are susceptible to aliasing.
+ */
+static long get_max_cache_span(void)
+{
+	struct cpu_cacheinfo *this_cpu_ci;
+	struct cacheinfo *this_leaf;
+	long span, max_span = 0;
+	int cpu, leaf;
+
+	for_each_possible_cpu(cpu) {
+		this_cpu_ci = get_cpu_cacheinfo(cpu);
+		this_leaf = this_cpu_ci->info_list;
+		for (leaf = 0; leaf < this_cpu_ci->num_leaves; leaf++) {
+			if (this_leaf->level > 1)
+				break;
+			span = this_leaf->coherency_line_size *
+			       this_leaf->number_of_sets;
+			if (span > max_span)
+				max_span = span;
+			this_leaf++;
+		}
+	}
+	return max_span;
+}
+
+/*
+ * Align shared mappings to the maximum cache "span" to avoid aliasing
+ * in VIPT caches, for performance.
+ * The returned SHMLBA value is always a power-of-two multiple of PAGE_SIZE.
+ */
+long arch_shmlba(void)
+{
+	if (shmlba == 0) {
+		long max_span = get_max_cache_span();
+
+		shmlba = max_span ? PAGE_ALIGN(max_span) : 4 * PAGE_SIZE;
+	}
+	return shmlba;
+}
 
 static void ci_leaf_init(struct cacheinfo *this_leaf,
 			 struct device_node *node,
@@ -93,6 +142,9 @@ static int __populate_cache_leaves(unsigned int cpu)
 	}
 	of_node_put(np);
 
+	/* Force recalculating SHMLBA if cache parameters are updated. */
+	shmlba = 0;
+
 	return 0;
 }
 
-- 
2.17.1


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

Re: [PATCH v2 1/2] riscv: Align shared mappings to SHMLBA

[Palmer Dabbelt]

On Tue, 26 Nov 2019 14:44:45 PST (-0800), consult-mg@gstardust.com wrote:
> Align shared mappings according to SHMLBA for VIPT cache performance.
>
> arch_get_unmapped_area() and arch_get_unmapped_area_topdown() are
> essentially copies of their default implementations in mm/mmap.c,
> modified to align the address to SHMLBA for shared mappings, i.e.
> where MAP_SHARED is specified or a file pointer is provided.
>
> Allow MAP_FIXED to request unaligned shared mappings.  Although this
> may potentially reduce performance, very little software does this, as
> it is not portable across architectures that enforce alignment.
>
> Signed-off-by: Marc Gauthier <consult-mg@gstardust.com>
> ---
>  arch/riscv/include/asm/pgtable.h |   4 ++
>  arch/riscv/kernel/sys_riscv.c    | 113 +++++++++++++++++++++++++++++++
>  2 files changed, 117 insertions(+)
>
> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> index d3221017194d..7d1cc47ac5f9 100644
> --- a/arch/riscv/include/asm/pgtable.h
> +++ b/arch/riscv/include/asm/pgtable.h
> @@ -436,6 +436,10 @@ extern void *dtb_early_va;
>  extern void setup_bootmem(void);
>  extern void paging_init(void);
>
> +/* We provide arch_get_unmapped_area to handle VIPT caches efficiently. */
> +#define HAVE_ARCH_UNMAPPED_AREA
> +#define HAVE_ARCH_UNMAPPED_AREA_TOPDOWN
> +
>  /*
>   * Task size is 0x4000000000 for RV64 or 0x9fc00000 for RV32.
>   * Note that PGDIR_SIZE must evenly divide TASK_SIZE.
> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
> index f3619f59d85c..3e739b30b1f7 100644
> --- a/arch/riscv/kernel/sys_riscv.c
> +++ b/arch/riscv/kernel/sys_riscv.c
> @@ -3,11 +3,15 @@
>   * Copyright (C) 2012 Regents of the University of California
>   * Copyright (C) 2014 Darius Rad <darius@bluespec.com>
>   * Copyright (C) 2017 SiFive
> + * Copyright (C) 2019 Aril Inc
>   */
>
>  #include <linux/syscalls.h>
>  #include <asm/unistd.h>
>  #include <asm/cacheflush.h>
> +#include <linux/shm.h>
> +#include <linux/mman.h>
> +#include <linux/security.h>
>
>  static long riscv_sys_mmap(unsigned long addr, unsigned long len,
>  			   unsigned long prot, unsigned long flags,
> @@ -65,3 +69,112 @@ SYSCALL_DEFINE3(riscv_flush_icache, uintptr_t, start, uintptr_t, end,
>
>  	return 0;
>  }
> +
> +/*
> + * RISC-V requires implementations to function correctly in the presence
> + * of cache aliasing, regardless of page alignment.  It says nothing about
> + * performance.  To ensure healthy performance with commonly implemented
> + * VIPT caches, the following code avoids most cases of cache aliasing by
> + * aligning shared mappings such that all mappings of a given physical
> + * page of an object are at a multiple of SHMLBA bytes from each other.
> + *
> + * It does not enforce alignment.  Using MAP_FIXED to request unaligned
> + * shared mappings is not common, and may perform poorly with VIPT caches.
> + */
> +unsigned long
> +arch_get_unmapped_area(struct file *filp, unsigned long addr,
> +			unsigned long len, unsigned long pgoff,
> +			unsigned long flags)
> +{
> +	struct mm_struct *mm = current->mm;
> +	struct vm_area_struct *vma, *prev;
> +	struct vm_unmapped_area_info info;
> +	const unsigned long pgoffset = pgoff << PAGE_SHIFT;
> +	int do_align = (filp || (flags & MAP_SHARED));
> +
> +	if (len > TASK_SIZE - mmap_min_addr)
> +		return -ENOMEM;
> +
> +	if (flags & MAP_FIXED)
> +		return addr;
> +
> +	if (addr) {
> +		if (do_align)
> +			addr = ALIGN(addr, SHMLBA) + (pgoffset & (SHMLBA - 1));
> +		else
> +			addr = PAGE_ALIGN(addr);
> +		vma = find_vma_prev(mm, addr, &prev);
> +		if (TASK_SIZE - len >= addr && addr >= mmap_min_addr &&
> +		    (!vma || addr + len <= vm_start_gap(vma)) &&
> +		    (!prev || addr >= vm_end_gap(prev)))
> +			return addr;
> +	}
> +
> +	info.flags = 0;
> +	info.length = len;
> +	info.low_limit = mm->mmap_base;
> +	info.high_limit = TASK_SIZE;
> +	info.align_mask = do_align ? SHMLBA - 1 : 0;
> +	info.align_offset = pgoffset;
> +	return vm_unmapped_area(&info);
> +}
> +
> +/*
> + * Similar to arch_get_unmapped_area(), but allocating top-down from below the
> + * stack's low limit (the base).
> + */
> +unsigned long
> +arch_get_unmapped_area_topdown(struct file *filp, unsigned long addr,
> +				unsigned long len, unsigned long pgoff,
> +				unsigned long flags)
> +{
> +	struct vm_area_struct *vma, *prev;
> +	struct mm_struct *mm = current->mm;
> +	struct vm_unmapped_area_info info;
> +	const unsigned long pgoffset = pgoff << PAGE_SHIFT;
> +	int do_align = (filp || (flags & MAP_SHARED));
> +
> +	/* requested length too big for entire address space */
> +	if (len > TASK_SIZE - mmap_min_addr)
> +		return -ENOMEM;
> +
> +	if (flags & MAP_FIXED)
> +		return addr;
> +
> +	/* requesting a specific address */
> +	if (addr) {
> +		if (do_align)
> +			addr = ALIGN(addr, SHMLBA) + (pgoffset & (SHMLBA - 1));
> +		else
> +			addr = PAGE_ALIGN(addr);
> +		vma = find_vma_prev(mm, addr, &prev);
> +		if (TASK_SIZE - len >= addr && addr >= mmap_min_addr &&
> +				(!vma || addr + len <= vm_start_gap(vma)) &&
> +				(!prev || addr >= vm_end_gap(prev)))
> +			return addr;
> +	}
> +
> +	info.flags = VM_UNMAPPED_AREA_TOPDOWN;
> +	info.length = len;
> +	info.low_limit = max(PAGE_SIZE, mmap_min_addr);
> +	info.high_limit = mm->mmap_base;
> +	info.align_mask = do_align ? SHMLBA - 1 : 0;
> +	info.align_offset = pgoffset;
> +	addr = vm_unmapped_area(&info);
> +
> +	/*
> +	 * A failed mmap() very likely causes application failure,
> +	 * so fall back to the bottom-up function here. This scenario
> +	 * can happen with large stack limits and large mmap()
> +	 * allocations.
> +	 */
> +	if (offset_in_page(addr)) {
> +		VM_BUG_ON(addr != -ENOMEM);
> +		info.flags = 0;
> +		info.low_limit = TASK_UNMAPPED_BASE;
> +		info.high_limit = TASK_SIZE;
> +		addr = vm_unmapped_area(&info);
> +	}
> +
> +	return addr;
> +}

It really feels like this should be generic code to me.  It looks like the only
major difference between this and the routines in arch/arm/mm/mmap.c is whether
or not MAP_FIXED alignment is enforced, so we could probably just make the
arch-specific code be arch_cache_is_vipt_aliasing() which on RISC-V would
always be false and on ARM would be the current cache_is_vipt_aliasing().

ARM is also using a different alignment expression, but I think they may be
equivilant.  They have

    #define COLOUR_ALIGN(addr,pgoff)                \
            ((((addr)+SHMLBA-1)&~(SHMLBA-1)) +      \
             (((pgoff)<<PAGE_SHIFT) & (SHMLBA-1)))

LMK if you're OK doing this, or if you want me to take over the patch set.

Re: [PATCH v2 2/2] riscv: Set SHMLBA according to cache geometry

[Palmer Dabbelt]

On Tue, 26 Nov 2019 14:44:46 PST (-0800), consult-mg@gstardust.com wrote:
> Set SHMLBA to the maximum cache "span" (line size * number of sets) of
> all CPU L1 instruction and data caches (L2 and up are rarely VIPT).
> This avoids VIPT cache aliasing with minimal alignment constraints.
>
> If the device tree does not provide cache parameters, use a conservative
> default of 16 KB:  only large enough to avoid aliasing in most VIPT caches.
>
> Signed-off-by: Marc Gauthier <consult-mg@gstardust.com>
> ---
>  arch/riscv/include/asm/Kbuild     |  1 -
>  arch/riscv/include/asm/shmparam.h | 12 +++++++
>  arch/riscv/kernel/cacheinfo.c     | 52 +++++++++++++++++++++++++++++++
>  3 files changed, 64 insertions(+), 1 deletion(-)
>  create mode 100644 arch/riscv/include/asm/shmparam.h
>
> diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild
> index 16970f246860..3905765807af 100644
> --- a/arch/riscv/include/asm/Kbuild
> +++ b/arch/riscv/include/asm/Kbuild
> @@ -27,7 +27,6 @@ generic-y += percpu.h
>  generic-y += preempt.h
>  generic-y += sections.h
>  generic-y += serial.h
> -generic-y += shmparam.h
>  generic-y += topology.h
>  generic-y += trace_clock.h
>  generic-y += unaligned.h
> diff --git a/arch/riscv/include/asm/shmparam.h b/arch/riscv/include/asm/shmparam.h
> new file mode 100644
> index 000000000000..9b6a98153648
> --- /dev/null
> +++ b/arch/riscv/include/asm/shmparam.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_RISCV_SHMPARAM_H
> +#define _ASM_RISCV_SHMPARAM_H
> +
> +/*
> + * Minimum alignment of shared memory segments as a function of cache geometry.
> + */
> +#define	SHMLBA	arch_shmlba()

I'd prefer if we inline the memoization, which would avoid the cost of a
function call in the general case.  You can also avoid that 0 test by
initializing the variable to PAGE_SIZE and the filling it out in our early init
code -- maybe setup_vm()?  That's what SPARC is doing.

> +
> +long arch_shmlba(void);
> +
> +#endif /* _ASM_RISCV_SHMPARAM_H */
> diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
> index 4c90c07d8c39..1bc7df8577d6 100644
> --- a/arch/riscv/kernel/cacheinfo.c
> +++ b/arch/riscv/kernel/cacheinfo.c
> @@ -1,12 +1,61 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /*
>   * Copyright (C) 2017 SiFive
> + * Copyright (C) 2019 Aril Inc
>   */
>
>  #include <linux/cacheinfo.h>
>  #include <linux/cpu.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/mm.h>
> +
> +static long shmlba;
> +
> +
> +/*
> + * Assuming  cache size = line size * #sets * N  for N-way associative caches,
> + * return the max cache "span" == (line size * #sets) == (cache size / N)
> + * across all L1 caches, or 0 if cache parameters are not available.
> + * VIPT caches with span > min page size are susceptible to aliasing.
> + */
> +static long get_max_cache_span(void)
> +{
> +	struct cpu_cacheinfo *this_cpu_ci;
> +	struct cacheinfo *this_leaf;
> +	long span, max_span = 0;
> +	int cpu, leaf;
> +
> +	for_each_possible_cpu(cpu) {
> +		this_cpu_ci = get_cpu_cacheinfo(cpu);
> +		this_leaf = this_cpu_ci->info_list;
> +		for (leaf = 0; leaf < this_cpu_ci->num_leaves; leaf++) {
> +			if (this_leaf->level > 1)
> +				break;
> +			span = this_leaf->coherency_line_size *
> +			       this_leaf->number_of_sets;
> +			if (span > max_span)
> +				max_span = span;
> +			this_leaf++;
> +		}
> +	}
> +	return max_span;
> +}
> +
> +/*
> + * Align shared mappings to the maximum cache "span" to avoid aliasing
> + * in VIPT caches, for performance.
> + * The returned SHMLBA value is always a power-of-two multiple of PAGE_SIZE.
> + */
> +long arch_shmlba(void)
> +{
> +	if (shmlba == 0) {
> +		long max_span = get_max_cache_span();
> +
> +		shmlba = max_span ? PAGE_ALIGN(max_span) : 4 * PAGE_SIZE;

I'd prefer to avoid sneaking in a default 4*PAGE_SIZE here, just default to
PAGE_SIZE and rely on systems with this behavior specifying the correct tuning
value in the device tree.  This avoids changing the behavior for existing
systems, which is a slight regression as the alignment uses more memory.  It's
not a big deal, but on systems that don't require alignment for high
performance there's no reason to just throw away memory -- particularly as we
have some RISC-V systems with pretty limited memory (I'm thinking of the
Kendryte boards, though I don't know how SHMLBA interacts with NOMMU so it
might not matter).

> +	}
> +	return shmlba;
> +}
>
>  static void ci_leaf_init(struct cacheinfo *this_leaf,
>  			 struct device_node *node,
> @@ -93,6 +142,9 @@ static int __populate_cache_leaves(unsigned int cpu)
>  	}
>  	of_node_put(np);
>
> +	/* Force recalculating SHMLBA if cache parameters are updated. */
> +	shmlba = 0;
> +
>  	return 0;
>  }

Re: [PATCH v2 2/2] riscv: Set SHMLBA according to cache geometry

[Marc Gauthier]

Palmer Dabbelt wrote on 2019-12-05 18:03:
> On Tue, 26 Nov 2019 14:44:46 PST (-0800), consult-mg@gstardust.com wrote:
>> Set SHMLBA to the maximum cache "span" (line size * number of sets) of
>> all CPU L1 instruction and data caches (L2 and up are rarely VIPT).
>> This avoids VIPT cache aliasing with minimal alignment constraints.
>>
>> If the device tree does not provide cache parameters, use a conservative
>> default of 16 KB:  only large enough to avoid aliasing in most VIPT 
>> caches.
>>
>> Signed-off-by: Marc Gauthier <consult-mg@gstardust.com>
>> ---
>>  arch/riscv/include/asm/Kbuild     |  1 -
>>  arch/riscv/include/asm/shmparam.h | 12 +++++++
>>  arch/riscv/kernel/cacheinfo.c     | 52 +++++++++++++++++++++++++++++++
>>  3 files changed, 64 insertions(+), 1 deletion(-)
>>  create mode 100644 arch/riscv/include/asm/shmparam.h
>>
>> diff --git a/arch/riscv/include/asm/Kbuild 
>> b/arch/riscv/include/asm/Kbuild
>> index 16970f246860..3905765807af 100644
>> --- a/arch/riscv/include/asm/Kbuild
>> +++ b/arch/riscv/include/asm/Kbuild
>> @@ -27,7 +27,6 @@ generic-y += percpu.h
>>  generic-y += preempt.h
>>  generic-y += sections.h
>>  generic-y += serial.h
>> -generic-y += shmparam.h
>>  generic-y += topology.h
>>  generic-y += trace_clock.h
>>  generic-y += unaligned.h
>> diff --git a/arch/riscv/include/asm/shmparam.h 
>> b/arch/riscv/include/asm/shmparam.h
>> new file mode 100644
>> index 000000000000..9b6a98153648
>> --- /dev/null
>> +++ b/arch/riscv/include/asm/shmparam.h
>> @@ -0,0 +1,12 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _ASM_RISCV_SHMPARAM_H
>> +#define _ASM_RISCV_SHMPARAM_H
>> +
>> +/*
>> + * Minimum alignment of shared memory segments as a function of 
>> cache geometry.
>> + */
>> +#define    SHMLBA    arch_shmlba()
>
> I'd prefer if we inline the memoization, which would avoid the cost of a
> function call in the general case.  You can also avoid that 0 test by
> initializing the variable to PAGE_SIZE and the filling it out in our 
> early init
> code -- maybe setup_vm()?  That's what SPARC is doing.

Good point.
Unlike SPARC, this patch re-uses existing code in 
drivers/base/cacheinfo.c to compute cache parameters.  To preserve that, 
it'll be more robust to initialize shmlba at a point certain to have 
those parameters -- at the comment far below, "Force recalculating 
SHMLBA if cache parameters are updated."  That way it keeps working if 
that point in time changes.


>> +
>> +long arch_shmlba(void);
>> +
>> +#endif /* _ASM_RISCV_SHMPARAM_H */
>> diff --git a/arch/riscv/kernel/cacheinfo.c 
>> b/arch/riscv/kernel/cacheinfo.c
>> index 4c90c07d8c39..1bc7df8577d6 100644
>> --- a/arch/riscv/kernel/cacheinfo.c
>> +++ b/arch/riscv/kernel/cacheinfo.c
>> @@ -1,12 +1,61 @@
>>  // SPDX-License-Identifier: GPL-2.0-only
>>  /*
>>   * Copyright (C) 2017 SiFive
>> + * Copyright (C) 2019 Aril Inc
>>   */
>>
>>  #include <linux/cacheinfo.h>
>>  #include <linux/cpu.h>
>>  #include <linux/of.h>
>>  #include <linux/of_device.h>
>> +#include <linux/mm.h>
>> +
>> +static long shmlba;
>> +
>> +
>> +/*
>> + * Assuming  cache size = line size * #sets * N  for N-way 
>> associative caches,
>> + * return the max cache "span" == (line size * #sets) == (cache size 
>> / N)
>> + * across all L1 caches, or 0 if cache parameters are not available.
>> + * VIPT caches with span > min page size are susceptible to aliasing.
>> + */
>> +static long get_max_cache_span(void)
>> +{
>> +    struct cpu_cacheinfo *this_cpu_ci;
>> +    struct cacheinfo *this_leaf;
>> +    long span, max_span = 0;
>> +    int cpu, leaf;
>> +
>> +    for_each_possible_cpu(cpu) {
>> +        this_cpu_ci = get_cpu_cacheinfo(cpu);
>> +        this_leaf = this_cpu_ci->info_list;
>> +        for (leaf = 0; leaf < this_cpu_ci->num_leaves; leaf++) {
>> +            if (this_leaf->level > 1)
>> +                break;
>> +            span = this_leaf->coherency_line_size *
>> +                   this_leaf->number_of_sets;
>> +            if (span > max_span)
>> +                max_span = span;
>> +            this_leaf++;
>> +        }
>> +    }
>> +    return max_span;
>> +}
>> +
>> +/*
>> + * Align shared mappings to the maximum cache "span" to avoid aliasing
>> + * in VIPT caches, for performance.
>> + * The returned SHMLBA value is always a power-of-two multiple of 
>> PAGE_SIZE.
>> + */
>> +long arch_shmlba(void)
>> +{
>> +    if (shmlba == 0) {
>> +        long max_span = get_max_cache_span();
>> +
>> +        shmlba = max_span ? PAGE_ALIGN(max_span) : 4 * PAGE_SIZE;
>
> I'd prefer to avoid sneaking in a default 4*PAGE_SIZE here, just 
> default to
> PAGE_SIZE and rely on systems with this behavior specifying the 
> correct tuning
> value in the device tree.

Fair enough.


> This avoids changing the behavior for existing
> systems, which is a slight regression as the alignment uses more 
> memory.  It's
> not a big deal, but on systems that don't require alignment for high
> performance there's no reason to just throw away memory -- 
> particularly as we
> have some RISC-V systems with pretty limited memory

Greater alignment takes up more virtual memory, not more physical memory.


> (I'm thinking of the
> Kendryte boards, though I don't know how SHMLBA interacts with NOMMU 
> so it
> might not matter).

There's no virtual memory in NOMMU, so indeed it doesn't matter.

M


>> +    }
>> +    return shmlba;
>> +}
>>
>>  static void ci_leaf_init(struct cacheinfo *this_leaf,
>>               struct device_node *node,
>> @@ -93,6 +142,9 @@ static int __populate_cache_leaves(unsigned int cpu)
>>      }
>>      of_node_put(np);
>>
>> +    /* Force recalculating SHMLBA if cache parameters are updated. */
>> +    shmlba = 0;
>> +
>>      return 0;
>>  }

Re: [PATCH v2 2/2] riscv: Set SHMLBA according to cache geometry

[Palmer Dabbelt]

On Thu, 05 Dec 2019 15:58:25 PST (-0800), consult-mg@gstardust.com wrote:
> Palmer Dabbelt wrote on 2019-12-05 18:03:
>> On Tue, 26 Nov 2019 14:44:46 PST (-0800), consult-mg@gstardust.com wrote:
>>> Set SHMLBA to the maximum cache "span" (line size * number of sets) of
>>> all CPU L1 instruction and data caches (L2 and up are rarely VIPT).
>>> This avoids VIPT cache aliasing with minimal alignment constraints.
>>>
>>> If the device tree does not provide cache parameters, use a conservative
>>> default of 16 KB:  only large enough to avoid aliasing in most VIPT
>>> caches.
>>>
>>> Signed-off-by: Marc Gauthier <consult-mg@gstardust.com>
>>> ---
>>>  arch/riscv/include/asm/Kbuild     |  1 -
>>>  arch/riscv/include/asm/shmparam.h | 12 +++++++
>>>  arch/riscv/kernel/cacheinfo.c     | 52 +++++++++++++++++++++++++++++++
>>>  3 files changed, 64 insertions(+), 1 deletion(-)
>>>  create mode 100644 arch/riscv/include/asm/shmparam.h
>>>
>>> diff --git a/arch/riscv/include/asm/Kbuild
>>> b/arch/riscv/include/asm/Kbuild
>>> index 16970f246860..3905765807af 100644
>>> --- a/arch/riscv/include/asm/Kbuild
>>> +++ b/arch/riscv/include/asm/Kbuild
>>> @@ -27,7 +27,6 @@ generic-y += percpu.h
>>>  generic-y += preempt.h
>>>  generic-y += sections.h
>>>  generic-y += serial.h
>>> -generic-y += shmparam.h
>>>  generic-y += topology.h
>>>  generic-y += trace_clock.h
>>>  generic-y += unaligned.h
>>> diff --git a/arch/riscv/include/asm/shmparam.h
>>> b/arch/riscv/include/asm/shmparam.h
>>> new file mode 100644
>>> index 000000000000..9b6a98153648
>>> --- /dev/null
>>> +++ b/arch/riscv/include/asm/shmparam.h
>>> @@ -0,0 +1,12 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +#ifndef _ASM_RISCV_SHMPARAM_H
>>> +#define _ASM_RISCV_SHMPARAM_H
>>> +
>>> +/*
>>> + * Minimum alignment of shared memory segments as a function of
>>> cache geometry.
>>> + */
>>> +#define    SHMLBA    arch_shmlba()
>>
>> I'd prefer if we inline the memoization, which would avoid the cost of a
>> function call in the general case.  You can also avoid that 0 test by
>> initializing the variable to PAGE_SIZE and the filling it out in our
>> early init
>> code -- maybe setup_vm()?  That's what SPARC is doing.
>
> Good point.
> Unlike SPARC, this patch re-uses existing code in
> drivers/base/cacheinfo.c to compute cache parameters.  To preserve that,
> it'll be more robust to initialize shmlba at a point certain to have
> those parameters -- at the comment far below, "Force recalculating
> SHMLBA if cache parameters are updated."  That way it keeps working if
> that point in time changes.

Works for me.

>>> +
>>> +long arch_shmlba(void);
>>> +
>>> +#endif /* _ASM_RISCV_SHMPARAM_H */
>>> diff --git a/arch/riscv/kernel/cacheinfo.c
>>> b/arch/riscv/kernel/cacheinfo.c
>>> index 4c90c07d8c39..1bc7df8577d6 100644
>>> --- a/arch/riscv/kernel/cacheinfo.c
>>> +++ b/arch/riscv/kernel/cacheinfo.c
>>> @@ -1,12 +1,61 @@
>>>  // SPDX-License-Identifier: GPL-2.0-only
>>>  /*
>>>   * Copyright (C) 2017 SiFive
>>> + * Copyright (C) 2019 Aril Inc
>>>   */
>>>
>>>  #include <linux/cacheinfo.h>
>>>  #include <linux/cpu.h>
>>>  #include <linux/of.h>
>>>  #include <linux/of_device.h>
>>> +#include <linux/mm.h>
>>> +
>>> +static long shmlba;
>>> +
>>> +
>>> +/*
>>> + * Assuming  cache size = line size * #sets * N  for N-way
>>> associative caches,
>>> + * return the max cache "span" == (line size * #sets) == (cache size
>>> / N)
>>> + * across all L1 caches, or 0 if cache parameters are not available.
>>> + * VIPT caches with span > min page size are susceptible to aliasing.
>>> + */
>>> +static long get_max_cache_span(void)
>>> +{
>>> +    struct cpu_cacheinfo *this_cpu_ci;
>>> +    struct cacheinfo *this_leaf;
>>> +    long span, max_span = 0;
>>> +    int cpu, leaf;
>>> +
>>> +    for_each_possible_cpu(cpu) {
>>> +        this_cpu_ci = get_cpu_cacheinfo(cpu);
>>> +        this_leaf = this_cpu_ci->info_list;
>>> +        for (leaf = 0; leaf < this_cpu_ci->num_leaves; leaf++) {
>>> +            if (this_leaf->level > 1)
>>> +                break;
>>> +            span = this_leaf->coherency_line_size *
>>> +                   this_leaf->number_of_sets;
>>> +            if (span > max_span)
>>> +                max_span = span;
>>> +            this_leaf++;
>>> +        }
>>> +    }
>>> +    return max_span;
>>> +}
>>> +
>>> +/*
>>> + * Align shared mappings to the maximum cache "span" to avoid aliasing
>>> + * in VIPT caches, for performance.
>>> + * The returned SHMLBA value is always a power-of-two multiple of
>>> PAGE_SIZE.
>>> + */
>>> +long arch_shmlba(void)
>>> +{
>>> +    if (shmlba == 0) {
>>> +        long max_span = get_max_cache_span();
>>> +
>>> +        shmlba = max_span ? PAGE_ALIGN(max_span) : 4 * PAGE_SIZE;
>>
>> I'd prefer to avoid sneaking in a default 4*PAGE_SIZE here, just
>> default to
>> PAGE_SIZE and rely on systems with this behavior specifying the
>> correct tuning
>> value in the device tree.
>
> Fair enough.
>
>
>> This avoids changing the behavior for existing
>> systems, which is a slight regression as the alignment uses more
>> memory.  It's
>> not a big deal, but on systems that don't require alignment for high
>> performance there's no reason to just throw away memory --
>> particularly as we
>> have some RISC-V systems with pretty limited memory
>
> Greater alignment takes up more virtual memory, not more physical memory.
>
>
>> (I'm thinking of the
>> Kendryte boards, though I don't know how SHMLBA interacts with NOMMU
>> so it
>> might not matter).
>
> There's no virtual memory in NOMMU, so indeed it doesn't matter.

Of course :).  I'd still like to leave the default alone, if only to prevent
people from relying on an arbitrary default decision.

>
> M
>
>
>>> +    }
>>> +    return shmlba;
>>> +}
>>>
>>>  static void ci_leaf_init(struct cacheinfo *this_leaf,
>>>               struct device_node *node,
>>> @@ -93,6 +142,9 @@ static int __populate_cache_leaves(unsigned int cpu)
>>>      }
>>>      of_node_put(np);
>>>
>>> +    /* Force recalculating SHMLBA if cache parameters are updated. */
>>> +    shmlba = 0;
>>> +
>>>      return 0;
>>>  }

Re: [PATCH v2 1/2] riscv: Align shared mappings to SHMLBA

[Marc Gauthier]

Palmer Dabbelt wrote on 2019-12-05 18:03:
> It really feels like this should be generic code to me.

Indeed.  The generic mm/mmap.c versions might be generalized a bit.


> It looks like the only
> major difference between this and the routines in arch/arm/mm/mmap.c 
> is whether
> or not MAP_FIXED alignment is enforced, so we could probably just make 
> the
> arch-specific code be arch_cache_is_vipt_aliasing() which on RISC-V would
> always be false and on ARM would be the current cache_is_vipt_aliasing().

Probably right.
ARM uses find_vma() instead of find_map_prev() as in the generic code, I 
haven't examined why exactly.
And their use of PAGE_MASK here is redundant (SHMLBA needs to be 
page-aligned):
         info.align_mask = do_align ? (PAGE_MASK & (SHMLBA - 1)) : 0;

"cache_is_vipt_aliasing" might be a bit misleading.  RISC-V caches can 
be VIPT-aliasing.  They simply can't let that show up functionally to 
software.  Maybe strict_vipt_aliasing, or ?


> ARM is also using a different alignment expression, but I think they 
> may be
> equivilant.  They have
>
>    #define COLOUR_ALIGN(addr,pgoff)                \
>            ((((addr)+SHMLBA-1)&~(SHMLBA-1)) +      \
>             (((pgoff)<<PAGE_SHIFT) & (SHMLBA-1)))

Yes.  After looking at this and others carefully, it was clearer to use 
the existing ALIGN()
macro, and now pgoff is shifted once instead of twice (optimization 
habit :).


> LMK if you're OK doing this, or if you want me to take over the patch 
> set.

You're certainly welcome to take this on, if so willing.

M