Unnecessary cache-line flush on page table updates ?

Unnecessary cache-line flush on page table updates ?

[heechul Yun]

Catalin,

Based on TRM of Cortex A9, the MMU reads page table entries from L1-D
cache not from memory. Then I think we do not need to flush the cache
line in the following code because MMU will always see up-to-date view
of page table in both UP and SMP systems.

linux/arch/arm/mm/proc-v7.S

ENTRY(cpu_v7_set_pte_ext)
	...
        mcr     p15, 0, r0, c7, c10, 1          @ flush_pte from
D-cache // why we need this in A9?
        ?

If this is a necessary one, could you please explain the reason? Thanks.

Unnecessary cache-line flush on page table updates ?

[Catalin Marinas]

On Fri, Jul 01, 2011 at 08:04:42AM +0100, heechul Yun wrote:
> Based on TRM of Cortex A9, the MMU reads page table entries from L1-D
> cache not from memory. Then I think we do not need to flush the cache
> line in the following code because MMU will always see up-to-date view
> of page table in both UP and SMP systems.
> 
> linux/arch/arm/mm/proc-v7.S
> 
> ENTRY(cpu_v7_set_pte_ext)
> 	...
>         mcr     p15, 0, r0, c7, c10, 1          @ flush_pte from
> D-cache // why we need this in A9?
>         ?
> 
> If this is a necessary one, could you please explain the reason? Thanks.

No, it's not necessary, only that this file is used by other processors
as well. The solution below checks the ID_MMFR3[23:20] bits (coherent
walk) and avoid flushing if the value is 1. The same could be done for
PMD entries, though that's less critical than the PTEs.

Please note that the patch is not fully tested.

8<--------------------

Unnecessary cache-line flush on page table updates ?

[heechul Yun]

Great.

Removing the PTE flush seems to have a noticeable performance
difference in my test. The followings are lmbench 3.0a performance
result measured on a Cortex A9 SMP platform. So far, I did not have
any problem while doing various test.

=========
mm-patch:
=========
Pagefaults on /tmp/XXX: 3.0759 microseconds
Process fork+exit: 464.5414 microseconds
Process fork+execve: 785.4944 microseconds
Process fork+/bin/sh -c: 488.6204 microseconds

=========
original:
=========
Pagefaults on /tmp/XXX: 3.6209 microseconds
Process fork+exit: 485.5236 microseconds
Process fork+execve: 820.0613 microseconds
Process fork+/bin/sh -c: 2966.3828 microseconds

Heechul

On Fri, Jul 1, 2011 at 3:10 AM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Fri, Jul 01, 2011 at 08:04:42AM +0100, heechul Yun wrote:
>> Based on TRM of Cortex A9, the MMU reads page table entries from L1-D
>> cache not from memory. Then I think we do not need to flush the cache
>> line in the following code because MMU will always see up-to-date view
>> of page table in both UP and SMP systems.
>>
>> linux/arch/arm/mm/proc-v7.S
>>
>> ENTRY(cpu_v7_set_pte_ext)
>> ? ? ? ...
>> ? ? ? ? mcr ? ? p15, 0, r0, c7, c10, 1 ? ? ? ? ?@ flush_pte from
>> D-cache // why we need this in A9?
>> ? ? ? ? ?
>>
>> If this is a necessary one, could you please explain the reason? Thanks.
>
> No, it's not necessary, only that this file is used by other processors
> as well. The solution below checks the ID_MMFR3[23:20] bits (coherent
> walk) and avoid flushing if the value is 1. The same could be done for
> PMD entries, though that's less critical than the PTEs.
>
> Please note that the patch is not fully tested.
>
> 8<--------------------
>
> From 67bd5ebdf622637f8293286146441e6292713c3d Mon Sep 17 00:00:00 2001
> From: Catalin Marinas <catalin.marinas@arm.com>
> Date: Fri, 1 Jul 2011 10:57:07 +0100
> Subject: [PATCH] ARMv7: Do not clean the PTE coherent page table walk is supported
>
> This patch adds a check for the ID_MMFR3[23:20] bits (coherent walk) and
> only cleans the D-cache corresponding to a PTE if coherent page table
> walks are not supported.
>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
> ?arch/arm/mm/proc-v7.S | ? ?4 +++-
> ?1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
> index 8013afc..fc5b36f 100644
> --- a/arch/arm/mm/proc-v7.S
> +++ b/arch/arm/mm/proc-v7.S
> @@ -166,7 +166,9 @@ ENTRY(cpu_v7_set_pte_ext)
> ?ARM( ?str ? ? r3, [r0, #2048]! )
> ?THUMB( ? ? ? ?add ? ? r0, r0, #2048 )
> ?THUMB( ? ? ? ?str ? ? r3, [r0] )
> - ? ? ? mcr ? ? p15, 0, r0, c7, c10, 1 ? ? ? ? ?@ flush_pte
> + ? ? ? mrc ? ? p15, 0, r3, c0, c1, 7 ? ? ? ? ? @ read ID_MMFR3
> + ? ? ? tst ? ? r3, #0xf << 20 ? ? ? ? ? ? ? ? ?@ check the coherent walk bits
> + ? ? ? mcreq ? p15, 0, r0, c7, c10, 1 ? ? ? ? ?@ flush_pte
> ?#endif
> ? ? ? ?mov ? ? pc, lr
> ?ENDPROC(cpu_v7_set_pte_ext)
>
> --
> Catalin
>

Unnecessary cache-line flush on page table updates ?

[Catalin Marinas]

On Fri, Jul 01, 2011 at 10:42:00PM +0100, heechul Yun wrote:
> Great.
> 
> Removing the PTE flush seems to have a noticeable performance
> difference in my test. The followings are lmbench 3.0a performance
> result measured on a Cortex A9 SMP platform. So far, I did not have
> any problem while doing various test.
> 
> =========
> mm-patch:
> =========
> Pagefaults on /tmp/XXX: 3.0759 microseconds
> Process fork+exit: 464.5414 microseconds
> Process fork+execve: 785.4944 microseconds
> Process fork+/bin/sh -c: 488.6204 microseconds
> 
> =========
> original:
> =========
> Pagefaults on /tmp/XXX: 3.6209 microseconds
> Process fork+exit: 485.5236 microseconds
> Process fork+execve: 820.0613 microseconds
> Process fork+/bin/sh -c: 2966.3828 microseconds

Given these results, I think it's worth merging the patch. Can I add
your Tested-by?

I think there can be a few other optimisations in the TLB area but it
needs some digging.

Thanks.

-- 
Catalin

Unnecessary cache-line flush on page table updates ?

[Russell King - ARM Linux]

On Mon, Jul 04, 2011 at 10:45:32AM +0100, Catalin Marinas wrote:
> Given these results, I think it's worth merging the patch. Can I add
> your Tested-by?

If we're going to make this change to use the coherent information,
let's address all the places in one go, which are:

	clean_pmd_entry
	flush_pmd_entry
	clean_pte_table

These require a little more thought as we aren't guaranteed to have
ID_MMFR3 in place - maybe they should be callbacks into the per-CPU
code.

It would also be a good idea to change the comment from "flush_pte"
to "Clean data cache to PoU".

> I think there can be a few other optimisations in the TLB area but it
> needs some digging.

The single-TLB model works fairly well, but as I thought the lack of
mcr%? processing by GCC makes the asm/tlbflush.h code fairly disgusting
even for a v6+v7 kernel.  Luckily, we can play some tricks and sort
some of that out.  The patch below is not complete (and can result in
some rules of the architecture being violated - namely the requirement
for an ISB after the BTB flush without a branch between) but it
illustrates the idea:

 arch/arm/include/asm/tlbflush.h |  216 +++++++++++++++++----------------------
 1 files changed, 92 insertions(+), 124 deletions(-)

diff --git a/arch/arm/include/asm/tlbflush.h b/arch/arm/include/asm/tlbflush.h
index d2005de..252874c 100644
--- a/arch/arm/include/asm/tlbflush.h
+++ b/arch/arm/include/asm/tlbflush.h
@@ -34,15 +34,15 @@
 #define TLB_V6_D_ASID	(1 << 17)
 #define TLB_V6_I_ASID	(1 << 18)
 
-#define TLB_BTB		(1 << 28)
-
 /* Unified Inner Shareable TLB operations (ARMv7 MP extensions) */
 #define TLB_V7_UIS_PAGE	(1 << 19)
 #define TLB_V7_UIS_FULL (1 << 20)
 #define TLB_V7_UIS_ASID (1 << 21)
 
 /* Inner Shareable BTB operation (ARMv7 MP extensions) */
-#define TLB_V7_IS_BTB	(1 << 22)
+#define TLB_V7_IS_BTB	(1 << 26)
+#define TLB_BTB		(1 << 27)
+#define TLB_BTB_BARRIER	(1 << 28)
 
 #define TLB_L2CLEAN_FR	(1 << 29)		/* Feroceon */
 #define TLB_DCLEAN	(1 << 30)
@@ -99,7 +99,7 @@
 # define v4_always_flags	(-1UL)
 #endif
 
-#define fa_tlb_flags	(TLB_WB | TLB_BTB | TLB_DCLEAN | \
+#define fa_tlb_flags	(TLB_WB | TLB_BTB_BARRIER | TLB_BTB | TLB_DCLEAN | \
 			 TLB_V4_U_FULL | TLB_V4_U_PAGE)
 
 #ifdef CONFIG_CPU_TLB_FA
@@ -166,7 +166,7 @@
 # define v4wb_always_flags	(-1UL)
 #endif
 
-#define v6wbi_tlb_flags (TLB_WB | TLB_DCLEAN | TLB_BTB | \
+#define v6wbi_tlb_flags (TLB_WB | TLB_DCLEAN | TLB_BTB_BARRIER | TLB_BTB | \
 			 TLB_V6_I_FULL | TLB_V6_D_FULL | \
 			 TLB_V6_I_PAGE | TLB_V6_D_PAGE | \
 			 TLB_V6_I_ASID | TLB_V6_D_ASID)
@@ -184,10 +184,12 @@
 # define v6wbi_always_flags	(-1UL)
 #endif
 
-#define v7wbi_tlb_flags_smp	(TLB_WB | TLB_DCLEAN | TLB_V7_IS_BTB | \
-			 TLB_V7_UIS_FULL | TLB_V7_UIS_PAGE | TLB_V7_UIS_ASID)
-#define v7wbi_tlb_flags_up	(TLB_WB | TLB_DCLEAN | TLB_BTB | \
-			 TLB_V6_U_FULL | TLB_V6_U_PAGE | TLB_V6_U_ASID)
+#define v7wbi_tlb_flags_smp	(TLB_WB | TLB_DCLEAN | \
+				 TLB_BTB_BARRIER | TLB_V7_IS_BTB | \
+			TLB_V7_UIS_FULL | TLB_V7_UIS_PAGE | TLB_V7_UIS_ASID)
+#define v7wbi_tlb_flags_up	(TLB_WB | TLB_DCLEAN | \
+				 TLB_BTB_BARRIER | TLB_BTB | \
+			TLB_V6_U_FULL | TLB_V6_U_PAGE | TLB_V6_U_ASID)
 
 #ifdef CONFIG_CPU_TLB_V7
 
@@ -322,6 +324,18 @@ extern struct cpu_tlb_fns cpu_tlb;
 
 #define tlb_flag(f)	((always_tlb_flags & (f)) || (__tlb_flag & possible_tlb_flags & (f)))
 
+#define tlb_op(f, regs, arg)						\
+	do {								\
+		if (always_tlb_flags & (f))				\
+			asm("mcr p15, 0, %0, " regs			\
+			    : : "r" (arg) : "cc");			\
+		else if (possible_tlb_flags & (f))			\
+			asm("tst %1, %2\n\t"				\
+			    "mcrne p15, 0, %0, " regs			\
+			    : : "r" (arg), "r" (__tlb_flag), "Ir" (f)	\
+			    : "cc");					\
+	} while (0)
+
 static inline void local_flush_tlb_all(void)
 {
 	const int zero = 0;
@@ -330,26 +344,19 @@ static inline void local_flush_tlb_all(void)
 	if (tlb_flag(TLB_WB))
 		dsb();
 
-	if (tlb_flag(TLB_V3_FULL))
-		asm("mcr p15, 0, %0, c6, c0, 0" : : "r" (zero) : "cc");
-	if (tlb_flag(TLB_V4_U_FULL | TLB_V6_U_FULL))
-		asm("mcr p15, 0, %0, c8, c7, 0" : : "r" (zero) : "cc");
-	if (tlb_flag(TLB_V4_D_FULL | TLB_V6_D_FULL))
-		asm("mcr p15, 0, %0, c8, c6, 0" : : "r" (zero) : "cc");
-	if (tlb_flag(TLB_V4_I_FULL | TLB_V6_I_FULL))
-		asm("mcr p15, 0, %0, c8, c5, 0" : : "r" (zero) : "cc");
-	if (tlb_flag(TLB_V7_UIS_FULL))
-		asm("mcr p15, 0, %0, c8, c3, 0" : : "r" (zero) : "cc");
+	tlb_op(TLB_V3_FULL, "c6, c0, 0", zero);
+	tlb_op(TLB_V4_U_FULL | TLB_V6_U_FULL, "c8, c7, 0", zero);
+	tlb_op(TLB_V4_D_FULL | TLB_V6_D_FULL, "c8, c6, 0", zero);
+	tlb_op(TLB_V4_I_FULL | TLB_V6_I_FULL, "c8, c5, 0", zero);
+	tlb_op(TLB_V7_UIS_FULL, "c8, c3, 0", zero);
 
-	if (tlb_flag(TLB_BTB)) {
-		/* flush the branch target cache */
-		asm("mcr p15, 0, %0, c7, c5, 6" : : "r" (zero) : "cc");
-		dsb();
-		isb();
-	}
-	if (tlb_flag(TLB_V7_IS_BTB)) {
-		/* flush the branch target cache */
-		asm("mcr p15, 0, %0, c7, c1, 6" : : "r" (zero) : "cc");
+	/* flush the branch target cache */
+	tlb_op(TLB_BTB, "c7, c5, 6", zero);
+
+	/* flush the branch target cache */
+	tlb_op(TLB_V7_IS_BTB, "c7, c1, 6", zero);
+
+	if (tlb_flag(TLB_BTB_BARRIER)) {
 		dsb();
 		isb();
 	}
@@ -364,42 +371,32 @@ static inline void local_flush_tlb_mm(struct mm_struct *mm)
 	if (tlb_flag(TLB_WB))
 		dsb();
 
-	if (cpumask_test_cpu(get_cpu(), mm_cpumask(mm))) {
-		if (tlb_flag(TLB_V3_FULL))
-			asm("mcr p15, 0, %0, c6, c0, 0" : : "r" (zero) : "cc");
-		if (tlb_flag(TLB_V4_U_FULL))
-			asm("mcr p15, 0, %0, c8, c7, 0" : : "r" (zero) : "cc");
-		if (tlb_flag(TLB_V4_D_FULL))
-			asm("mcr p15, 0, %0, c8, c6, 0" : : "r" (zero) : "cc");
-		if (tlb_flag(TLB_V4_I_FULL))
-			asm("mcr p15, 0, %0, c8, c5, 0" : : "r" (zero) : "cc");
+	if (possible_tlb_flags & (TLB_V3_FULL|TLB_V4_U_FULL|TLB_V4_D_FULL|TLB_V4_I_FULL) &&
+	    cpumask_test_cpu(get_cpu(), mm_cpumask(mm))) {
+		tlb_op(TLB_V3_FULL, "c6, c0, 0", zero);
+		tlb_op(TLB_V4_U_FULL, "c8, c7, 0", zero);
+		tlb_op(TLB_V4_D_FULL, "c8, c6, 0", zero);
+		tlb_op(TLB_V4_I_FULL, "c8, c5, 0", zero);
 	}
 	put_cpu();
 
-	if (tlb_flag(TLB_V6_U_ASID))
-		asm("mcr p15, 0, %0, c8, c7, 2" : : "r" (asid) : "cc");
-	if (tlb_flag(TLB_V6_D_ASID))
-		asm("mcr p15, 0, %0, c8, c6, 2" : : "r" (asid) : "cc");
-	if (tlb_flag(TLB_V6_I_ASID))
-		asm("mcr p15, 0, %0, c8, c5, 2" : : "r" (asid) : "cc");
-	if (tlb_flag(TLB_V7_UIS_ASID))
+	tlb_op(TLB_V6_U_ASID, "c8, c7, 2", asid);
+	tlb_op(TLB_V6_D_ASID, "c8, c6, 2", asid);
+	tlb_op(TLB_V6_I_ASID, "c8, c5, 2", asid);
 #ifdef CONFIG_ARM_ERRATA_720789
-		asm("mcr p15, 0, %0, c8, c3, 0" : : "r" (zero) : "cc");
+	tlb_op(TLB_V7_UIS_ASID, "c8, c3, 0", zero);
 #else
-		asm("mcr p15, 0, %0, c8, c3, 2" : : "r" (asid) : "cc");
+	tlb_op(TLB_V7_UIS_ASID, "c8, c3, 2", asid);
 #endif
 
-	if (tlb_flag(TLB_BTB)) {
-		/* flush the branch target cache */
-		asm("mcr p15, 0, %0, c7, c5, 6" : : "r" (zero) : "cc");
-		dsb();
-	}
-	if (tlb_flag(TLB_V7_IS_BTB)) {
-		/* flush the branch target cache */
-		asm("mcr p15, 0, %0, c7, c1, 6" : : "r" (zero) : "cc");
+	/* flush the branch target cache */
+	tlb_op(TLB_BTB, "c7, c5, 6", zero);
+
+	/* flush the branch target cache */
+	tlb_op(TLB_V7_IS_BTB, "c7, c1, 6", zero);
+
+	if (tlb_flag(TLB_BTB_BARRIER))
 		dsb();
-		isb();
-	}
 }
 
 static inline void
@@ -413,43 +410,33 @@ local_flush_tlb_page(struct vm_area_struct *vma, unsigned long uaddr)
 	if (tlb_flag(TLB_WB))
 		dsb();
 
-	if (cpumask_test_cpu(smp_processor_id(), mm_cpumask(vma->vm_mm))) {
-		if (tlb_flag(TLB_V3_PAGE))
-			asm("mcr p15, 0, %0, c6, c0, 0" : : "r" (uaddr) : "cc");
-		if (tlb_flag(TLB_V4_U_PAGE))
-			asm("mcr p15, 0, %0, c8, c7, 1" : : "r" (uaddr) : "cc");
-		if (tlb_flag(TLB_V4_D_PAGE))
-			asm("mcr p15, 0, %0, c8, c6, 1" : : "r" (uaddr) : "cc");
-		if (tlb_flag(TLB_V4_I_PAGE))
-			asm("mcr p15, 0, %0, c8, c5, 1" : : "r" (uaddr) : "cc");
+	if (possible_tlb_flags & (TLB_V3_PAGE|TLB_V4_U_PAGE|TLB_V4_D_PAGE|TLB_V4_I_PAGE|TLB_V4_I_FULL) &&
+	    cpumask_test_cpu(smp_processor_id(), mm_cpumask(vma->vm_mm))) {
+		tlb_op(TLB_V3_PAGE, "c6, c0, 0", uaddr);
+		tlb_op(TLB_V4_U_PAGE, "c8, c7, 1", uaddr);
+		tlb_op(TLB_V4_D_PAGE, "c8, c6, 1", uaddr);
+		tlb_op(TLB_V4_I_PAGE, "c8, c5, 1", uaddr);
 		if (!tlb_flag(TLB_V4_I_PAGE) && tlb_flag(TLB_V4_I_FULL))
 			asm("mcr p15, 0, %0, c8, c5, 0" : : "r" (zero) : "cc");
 	}
 
-	if (tlb_flag(TLB_V6_U_PAGE))
-		asm("mcr p15, 0, %0, c8, c7, 1" : : "r" (uaddr) : "cc");
-	if (tlb_flag(TLB_V6_D_PAGE))
-		asm("mcr p15, 0, %0, c8, c6, 1" : : "r" (uaddr) : "cc");
-	if (tlb_flag(TLB_V6_I_PAGE))
-		asm("mcr p15, 0, %0, c8, c5, 1" : : "r" (uaddr) : "cc");
-	if (tlb_flag(TLB_V7_UIS_PAGE))
+	tlb_op(TLB_V6_U_PAGE, "c8, c7, 1", uaddr);
+	tlb_op(TLB_V6_D_PAGE, "c8, c6, 1", uaddr);
+	tlb_op(TLB_V6_I_PAGE, "c8, c5, 1", uaddr);
 #ifdef CONFIG_ARM_ERRATA_720789
-		asm("mcr p15, 0, %0, c8, c3, 3" : : "r" (uaddr & PAGE_MASK) : "cc");
+	tlb_op(TLB_V7_UIS_PAGE, "c8, c3, 3", uaddr & PAGE_MASK);
 #else
-		asm("mcr p15, 0, %0, c8, c3, 1" : : "r" (uaddr) : "cc");
+	tlb_op(TLB_V7_UIS_PAGE, "c8, c3, 1", uaddr);
 #endif
 
-	if (tlb_flag(TLB_BTB)) {
-		/* flush the branch target cache */
-		asm("mcr p15, 0, %0, c7, c5, 6" : : "r" (zero) : "cc");
-		dsb();
-	}
-	if (tlb_flag(TLB_V7_IS_BTB)) {
-		/* flush the branch target cache */
-		asm("mcr p15, 0, %0, c7, c1, 6" : : "r" (zero) : "cc");
+	/* flush the branch target cache */
+	tlb_op(TLB_BTB, "c7, c5, 6", zero);
+
+	/* flush the branch target cache */
+	tlb_op(TLB_V7_IS_BTB, "c7, c1, 6", zero);
+
+	if (tlb_flag(TLB_BTB_BARRIER))
 		dsb();
-		isb();
-	}
 }
 
 static inline void local_flush_tlb_kernel_page(unsigned long kaddr)
@@ -462,35 +449,25 @@ static inline void local_flush_tlb_kernel_page(unsigned long kaddr)
 	if (tlb_flag(TLB_WB))
 		dsb();
 
-	if (tlb_flag(TLB_V3_PAGE))
-		asm("mcr p15, 0, %0, c6, c0, 0" : : "r" (kaddr) : "cc");
-	if (tlb_flag(TLB_V4_U_PAGE))
-		asm("mcr p15, 0, %0, c8, c7, 1" : : "r" (kaddr) : "cc");
-	if (tlb_flag(TLB_V4_D_PAGE))
-		asm("mcr p15, 0, %0, c8, c6, 1" : : "r" (kaddr) : "cc");
-	if (tlb_flag(TLB_V4_I_PAGE))
-		asm("mcr p15, 0, %0, c8, c5, 1" : : "r" (kaddr) : "cc");
+	tlb_op(TLB_V3_PAGE, "c6, c0, 0", kaddr);
+	tlb_op(TLB_V4_U_PAGE, "c8, c7, 1", kaddr);
+	tlb_op(TLB_V4_D_PAGE, "c8, c6, 1", kaddr);
+	tlb_op(TLB_V4_I_PAGE, "c8, c5, 1", kaddr);
 	if (!tlb_flag(TLB_V4_I_PAGE) && tlb_flag(TLB_V4_I_FULL))
 		asm("mcr p15, 0, %0, c8, c5, 0" : : "r" (zero) : "cc");
 
-	if (tlb_flag(TLB_V6_U_PAGE))
-		asm("mcr p15, 0, %0, c8, c7, 1" : : "r" (kaddr) : "cc");
-	if (tlb_flag(TLB_V6_D_PAGE))
-		asm("mcr p15, 0, %0, c8, c6, 1" : : "r" (kaddr) : "cc");
-	if (tlb_flag(TLB_V6_I_PAGE))
-		asm("mcr p15, 0, %0, c8, c5, 1" : : "r" (kaddr) : "cc");
-	if (tlb_flag(TLB_V7_UIS_PAGE))
-		asm("mcr p15, 0, %0, c8, c3, 1" : : "r" (kaddr) : "cc");
-
-	if (tlb_flag(TLB_BTB)) {
-		/* flush the branch target cache */
-		asm("mcr p15, 0, %0, c7, c5, 6" : : "r" (zero) : "cc");
-		dsb();
-		isb();
-	}
-	if (tlb_flag(TLB_V7_IS_BTB)) {
-		/* flush the branch target cache */
-		asm("mcr p15, 0, %0, c7, c1, 6" : : "r" (zero) : "cc");
+	tlb_op(TLB_V6_U_PAGE, "c8, c7, 1", kaddr);
+	tlb_op(TLB_V6_D_PAGE, "c8, c6, 1", kaddr);
+	tlb_op(TLB_V6_I_PAGE, "c8, c5, 1", kaddr);
+	tlb_op(TLB_V7_UIS_PAGE, "c8, c3, 1", kaddr);
+
+	/* flush the branch target cache */
+	tlb_op(TLB_BTB, "c7, c5, 6", zero);
+
+	/* flush the branch target cache */
+	tlb_op(TLB_V7_IS_BTB, "c7, c1, 6", zero);
+
+	if (tlb_flag(TLB_BTB_BARRIER)) {
 		dsb();
 		isb();
 	}
@@ -513,13 +490,8 @@ static inline void flush_pmd_entry(pmd_t *pmd)
 {
 	const unsigned int __tlb_flag = __cpu_tlb_flags;
 
-	if (tlb_flag(TLB_DCLEAN))
-		asm("mcr	p15, 0, %0, c7, c10, 1	@ flush_pmd"
-			: : "r" (pmd) : "cc");
-
-	if (tlb_flag(TLB_L2CLEAN_FR))
-		asm("mcr	p15, 1, %0, c15, c9, 1  @ L2 flush_pmd"
-			: : "r" (pmd) : "cc");
+	tlb_op(TLB_DCLEAN, "c7, c10, 1	@ flush_pmd", pmd);
+	tlb_op(TLB_L2CLEAN_FR, "c15, c9, 1  @ L2 flush_pmd", pmd);
 
 	if (tlb_flag(TLB_WB))
 		dsb();
@@ -529,15 +501,11 @@ static inline void clean_pmd_entry(pmd_t *pmd)
 {
 	const unsigned int __tlb_flag = __cpu_tlb_flags;
 
-	if (tlb_flag(TLB_DCLEAN))
-		asm("mcr	p15, 0, %0, c7, c10, 1	@ flush_pmd"
-			: : "r" (pmd) : "cc");
-
-	if (tlb_flag(TLB_L2CLEAN_FR))
-		asm("mcr	p15, 1, %0, c15, c9, 1  @ L2 flush_pmd"
-			: : "r" (pmd) : "cc");
+	tlb_op(TLB_DCLEAN, "c7, c10, 1	@ flush_pmd", pmd);
+	tlb_op(TLB_L2CLEAN_FR, "c15, c9, 1  @ L2 flush_pmd", pmd);
 }
 
+#undef tlb_op
 #undef tlb_flag
 #undef always_tlb_flags
 #undef possible_tlb_flags

Unnecessary cache-line flush on page table updates ?

[Catalin Marinas]

On Mon, Jul 04, 2011 at 11:02:21AM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 04, 2011 at 10:45:32AM +0100, Catalin Marinas wrote:
> > Given these results, I think it's worth merging the patch. Can I add
> > your Tested-by?
> 
> If we're going to make this change to use the coherent information,
> let's address all the places in one go, which are:
> 
>         clean_pmd_entry
>         flush_pmd_entry
>         clean_pte_table
> 
> These require a little more thought as we aren't guaranteed to have
> ID_MMFR3 in place - maybe they should be callbacks into the per-CPU
> code.

For the first two, can we not clear the TLB_DCLEAN bit in
__cpu_tlb_flags (only a single check at boot time?).

For the last one, we could add a tlb_flags() check.

> It would also be a good idea to change the comment from "flush_pte"
> to "Clean data cache to PoU".

OK.

> > I think there can be a few other optimisations in the TLB area but it
> > needs some digging.
> 
> The single-TLB model works fairly well, but as I thought the lack of
> mcr%? processing by GCC makes the asm/tlbflush.h code fairly disgusting
> even for a v6+v7 kernel.  Luckily, we can play some tricks and sort
> some of that out.  The patch below is not complete (and can result in
> some rules of the architecture being violated - namely the requirement
> for an ISB after the BTB flush without a branch between) but it
> illustrates the idea:

I'm not sure about this rule, I can ask for some clarification (we are
not changing the memory map of the branch we execute).

According to the ARM ARM, the TLB invalidation sequence should be:

STR rx, [Translation table entry]          ; write new entry to the translation table
Clean cache line [Translation table entry] ; This operation is not required with the
                                           ; Multiprocessing Extensions.
DSB            ; ensures visibility of the data cleaned from the D Cache
Invalidate TLB entry by MVA (and ASID if non-global) [page address]
Invalidate BTC
DSB            ; ensure completion of the Invalidate TLB operation
ISB            ; ensure table changes visible to instruction fetch

So we have a DSB and ISB (when we don't return to user) unconditionally.

Starting with Cortex-A8 (well, unless you enable some errata
workarounds), the BTB is a no-op anyway, so maybe we could have the BTB
unconditionally as well (for ARMv6/v7). The only problem is the inner
shareable if we are on SMP - maybe we can do some run-time code patching
for it.

> diff --git a/arch/arm/include/asm/tlbflush.h b/arch/arm/include/asm/tlbflush.h
> index d2005de..252874c 100644
> --- a/arch/arm/include/asm/tlbflush.h
> +++ b/arch/arm/include/asm/tlbflush.h
> @@ -34,15 +34,15 @@
>  #define TLB_V6_D_ASID  (1 << 17)
>  #define TLB_V6_I_ASID  (1 << 18)
> 
> -#define TLB_BTB                (1 << 28)
> -
>  /* Unified Inner Shareable TLB operations (ARMv7 MP extensions) */
>  #define TLB_V7_UIS_PAGE        (1 << 19)
>  #define TLB_V7_UIS_FULL (1 << 20)
>  #define TLB_V7_UIS_ASID (1 << 21)
> 
>  /* Inner Shareable BTB operation (ARMv7 MP extensions) */
> -#define TLB_V7_IS_BTB  (1 << 22)
> +#define TLB_V7_IS_BTB  (1 << 26)
> +#define TLB_BTB                (1 << 27)
> +#define TLB_BTB_BARRIER        (1 << 28)

I won't comment on the BTB changes until we clarify the conditionality
of barriers and BTB.

-- 
Catalin

Unnecessary cache-line flush on page table updates ?

[Russell King - ARM Linux]

On Mon, Jul 04, 2011 at 11:43:29AM +0100, Catalin Marinas wrote:
> According to the ARM ARM, the TLB invalidation sequence should be:
> 
> STR rx, [Translation table entry]          ; write new entry to the translation table
> Clean cache line [Translation table entry] ; This operation is not required with the
>                                            ; Multiprocessing Extensions.
> DSB            ; ensures visibility of the data cleaned from the D Cache
> Invalidate TLB entry by MVA (and ASID if non-global) [page address]
> Invalidate BTC
> DSB            ; ensure completion of the Invalidate TLB operation
> ISB            ; ensure table changes visible to instruction fetch
> 
> So we have a DSB and ISB (when we don't return to user) unconditionally.

That is the "cover all cases of following code" example sequence - it
is intended that at the end of the sequence, the CPU will be able to
execute whatever code irrespective of what it is.

The clarity of that comes from reading the actual rules given before
the example code:

| For TLB maintenance, the translation table walk is treated as a separate
| observer:
| ? A write to the translation tables, after it has been cleaned from the
|   cache if appropriate, is only guaranteed to be seen by a translation
|   table walk caused by an explicit load or store after the execution of
|   both a DSB and an ISB.
|
|   However, it is guaranteed that any writes to the translation tables are
|   not seen by any explicit memory access that occurs in program order
|   before the write to the translation tables.

In other words, if we write to the translation tables to setup a new
mapping where one did not previously exist, we only need an intervening
DSB and ISB when we want to access data through that that mapping.  So,
ioremap(), vmalloc(), vmap(), module allocation, etc would require this.

If we are tearing down a mapping, then we don't need any barrier for
individual PTE entries, but we do if we remove a higher-level (PMD/PUD)
table.

Note that it is irrelevant whether or not we had to clean the cache.

That "However" clause is an interesting one though - it seems to imply
that no barrier is required when we zero out a new page table, before
we link the new page table into the higher order table.  The memory we
allocated for the page table doesn't become a page table until it is
linked into the page table tree.  It also raises the question about how
the system knows that a particular store is to something that's a page
table and something that isn't...  Given that normal memory accesses are
unordered, I think this paragraph is misleading and wrong.

So, I think we need a DSB+ISB in clean_pte_table() to ensure that the
zeroing of that page will be visible to everyone coincidentally or before
the table is linked into the page table tree.  Maybe the arch people can
clarify that?

| ? For the base ARMv7 architecture and versions of the architecture before
|   ARMv7, if the translation tables are held in Write-Back Cacheable memory,
|   the caches must be cleaned to the point of unification after writing to
|   the translation tables and before the DSB instruction. This ensures that
|   the updated translation table are visible to a hardware translation
|   table walk.

IOW, if ID_MMFR3 is implemented on ARMv7+ and it indicates that coherent
walks are supported, we don't need to clean the cache entries for
PUD/PMD/PTE tables.

| ? A write to the translation tables, after it has been cleaned from the
|   cache if appropriate, is only guaranteed to be seen by a translation
|   table walk caused by the instruction fetch of an instruction that
|   follows the write to the translation tables after both a DSB and an ISB.

So we also need a DSB and ISB if we are going to execute code from the
new mapping.  This applies to module_alloc() only.

As far as the BTB goes, I wonder if we can postpone that for user TLB
ops by setting a TIF_ flag and checking that before returning to userspace.
That would avoid having to needlessly destroy the cached branch information
for kernel space while looping over the page tables.  The only other place
that needs to worry about that is module_alloc() and vmap/vmalloc with
PROT_KERNEL_EXEC, all of which can be done in flush_cache_vmap().
Thoughts?

Unnecessary cache-line flush on page table updates ?

[Catalin Marinas]

On Mon, Jul 04, 2011 at 12:13:38PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 04, 2011 at 11:43:29AM +0100, Catalin Marinas wrote:
> > According to the ARM ARM, the TLB invalidation sequence should be:
> > 
> > STR rx, [Translation table entry]          ; write new entry to the translation table
> > Clean cache line [Translation table entry] ; This operation is not required with the
> >                                            ; Multiprocessing Extensions.
> > DSB            ; ensures visibility of the data cleaned from the D Cache
> > Invalidate TLB entry by MVA (and ASID if non-global) [page address]
> > Invalidate BTC
> > DSB            ; ensure completion of the Invalidate TLB operation
> > ISB            ; ensure table changes visible to instruction fetch
> > 
> > So we have a DSB and ISB (when we don't return to user) unconditionally.
> 
> That is the "cover all cases of following code" example sequence - it
> is intended that at the end of the sequence, the CPU will be able to
> execute whatever code irrespective of what it is.
> 
> The clarity of that comes from reading the actual rules given before
> the example code:
> 
> | For TLB maintenance, the translation table walk is treated as a separate
> | observer:
> | ? A write to the translation tables, after it has been cleaned from the
> |   cache if appropriate, is only guaranteed to be seen by a translation
> |   table walk caused by an explicit load or store after the execution of
> |   both a DSB and an ISB.
> |
> |   However, it is guaranteed that any writes to the translation tables are
> |   not seen by any explicit memory access that occurs in program order
> |   before the write to the translation tables.
> 
> In other words, if we write to the translation tables to setup a new
> mapping where one did not previously exist, we only need an intervening
> DSB and ISB when we want to access data through that that mapping.  So,
> ioremap(), vmalloc(), vmap(), module allocation, etc would require this.

Yes.

> If we are tearing down a mapping, then we don't need any barrier for
> individual PTE entries, but we do if we remove a higher-level (PMD/PUD)
> table.

It depends on the use, I don't think we can generally assume that any
tearing down is fine since there are cases where we need a guaranteed
fault (zap_vma_ptes may only tear down PTE entries but the driver using
it expects a fault if something else tries to access that location). A
DSB would be enough.

> That "However" clause is an interesting one though - it seems to imply
> that no barrier is required when we zero out a new page table, before
> we link the new page table into the higher order table.  The memory we
> allocated for the page table doesn't become a page table until it is
> linked into the page table tree. 

That's my understanding of that clause as well.

> It also raises the question about how
> the system knows that a particular store is to something that's a page
> table and something that isn't...  Given that normal memory accesses are
> unordered, I think this paragraph is misleading and wrong.

Reordering of accesses can happen because of load speculation, store
reordering in the write buffer or delays on the bus outside the CPU. The
ARM processors do not issue stores speculatively. When a memory access
is issued, it checks the TLB and may perform a PTW (otherwise the
external bus wouldn't know the address. For explicit accesses, if the
PTW fails, it raises a fault (and we also need a precise abort).

So in case of load from memory vs store to page table, just because the
store is always issued after the load, the address translation for the
load is done before the changes to the page table.

In case of store vs store, AFAIK the stores in the write buffer already
have the physical address (hence the TLB look-up) and give that they are
issued in program order, there is no risk of one being visible before
the other (which should have gone through the write buffer).

> So, I think we need a DSB+ISB in clean_pte_table() to ensure that the
> zeroing of that page will be visible to everyone coincidentally or before
> the table is linked into the page table tree.  Maybe the arch people can
> clarify that?

I can ask if you need a better explanation than above.

> | ? For the base ARMv7 architecture and versions of the architecture before
> |   ARMv7, if the translation tables are held in Write-Back Cacheable memory,
> |   the caches must be cleaned to the point of unification after writing to
> |   the translation tables and before the DSB instruction. This ensures that
> |   the updated translation table are visible to a hardware translation
> |   table walk.
> 
> IOW, if ID_MMFR3 is implemented on ARMv7+ and it indicates that coherent
> walks are supported, we don't need to clean the cache entries for
> PUD/PMD/PTE tables.

Yes.

> | ? A write to the translation tables, after it has been cleaned from the
> |   cache if appropriate, is only guaranteed to be seen by a translation
> |   table walk caused by the instruction fetch of an instruction that
> |   follows the write to the translation tables after both a DSB and an ISB.
> 
> So we also need a DSB and ISB if we are going to execute code from the
> new mapping.  This applies to module_alloc() only.

Yes.

> As far as the BTB goes, I wonder if we can postpone that for user TLB
> ops by setting a TIF_ flag and checking that before returning to userspace.
> That would avoid having to needlessly destroy the cached branch information
> for kernel space while looping over the page tables.  The only other place
> that needs to worry about that is module_alloc() and vmap/vmalloc with
> PROT_KERNEL_EXEC, all of which can be done in flush_cache_vmap().

With setting and checking the TIF_ flag we penalise newer hardware
(Cortex-A8 onwards) where the BTB invalidation is a no-op. But I'll
check with the people here if there are any implications with deferring
the BTB invalidation.

-- 
Catalin

Unnecessary cache-line flush on page table updates ?

[Russell King - ARM Linux]

On Mon, Jul 04, 2011 at 04:58:35PM +0100, Catalin Marinas wrote:
> On Mon, Jul 04, 2011 at 12:13:38PM +0100, Russell King - ARM Linux wrote:
> > If we are tearing down a mapping, then we don't need any barrier for
> > individual PTE entries, but we do if we remove a higher-level (PMD/PUD)
> > table.
> 
> It depends on the use, I don't think we can generally assume that any
> tearing down is fine since there are cases where we need a guaranteed
> fault (zap_vma_ptes may only tear down PTE entries but the driver using
> it expects a fault if something else tries to access that location). A
> DSB would be enough.

It depends whether we're unmapping due to munmap or for kernel internal
stuff, or whether we're doing it as a result of disposing with the
page tables.  In the latter case, avoiding the DSBs would be a good
trick to pull.

> > That "However" clause is an interesting one though - it seems to imply
> > that no barrier is required when we zero out a new page table, before
> > we link the new page table into the higher order table.  The memory we
> > allocated for the page table doesn't become a page table until it is
> > linked into the page table tree. 
> 
> That's my understanding of that clause as well.
> 
> > It also raises the question about how
> > the system knows that a particular store is to something that's a page
> > table and something that isn't...  Given that normal memory accesses are
> > unordered, I think this paragraph is misleading and wrong.
> 
> Reordering of accesses can happen because of load speculation, store
> reordering in the write buffer or delays on the bus outside the CPU. The
> ARM processors do not issue stores speculatively. When a memory access
> is issued, it checks the TLB and may perform a PTW (otherwise the
> external bus wouldn't know the address. For explicit accesses, if the
> PTW fails, it raises a fault (and we also need a precise abort).

I think you missed my point.  The order in which stores to normal memory
appear is not determinable.  In the case of writeback caches, it depends
on the cache replacement algorithm, the state of the cache and its
allocation behaviour.

It is entirely possible that the store to the page tables _could_ appear
in memory (and therefore visible to the MMU) before the stores to the
new page table initializing it to zeros.

For instance, lets say that the L1 writeback cache is read allocate only,
and does not read entries from the L1 cache.

The new page table happens to contain some L1 cache lines, but the upper
table entry is not in the L1 cache.  This means that stores to the new
page table hit the L1 cache lines, which become dirty.  The store to the
upper table entry bypasses the L1 cache and is immediately placed into
the write buffer.

This means in all probability that the MMU will see the new page table
before it is visibly initialized.

> > As far as the BTB goes, I wonder if we can postpone that for user TLB
> > ops by setting a TIF_ flag and checking that before returning to userspace.
> > That would avoid having to needlessly destroy the cached branch information
> > for kernel space while looping over the page tables.  The only other place
> > that needs to worry about that is module_alloc() and vmap/vmalloc with
> > PROT_KERNEL_EXEC, all of which can be done in flush_cache_vmap().
> 
> With setting and checking the TIF_ flag we penalise newer hardware
> (Cortex-A8 onwards) where the BTB invalidation is a no-op. But I'll
> check with the people here if there are any implications with deferring
> the BTB invalidation.

Thanks.

Unnecessary cache-line flush on page table updates ?

[Russell King - ARM Linux]

On Mon, Jul 04, 2011 at 08:58:19PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 04, 2011 at 04:58:35PM +0100, Catalin Marinas wrote:
> > With setting and checking the TIF_ flag we penalise newer hardware
> > (Cortex-A8 onwards) where the BTB invalidation is a no-op. But I'll
> > check with the people here if there are any implications with deferring
> > the BTB invalidation.
> 
> Thanks.

Please can you also check whether BTC invalidation is required when a
page is removed from the page tables for the purpose of swapping it
out (so the PTE entry becomes zero).

Also, is BTC invalidation required if the very same page that was
there before is later restored without any modification to the
instruction content of that page.  (I'm thinking page is aged to old,
and so unmapped, then a prefetch abort which reinstates the same page
that was there previously.)

Finally, is BTC invalidation required if a different physical page
containing the same instruction content as before is placed at that
location?

I expect all but the last case requires BTC invalidation.

Lastly, please check whether population and removal of page table
entries for NX pages require BTC invalidation.  I expect not.

Unnecessary cache-line flush on page table updates ?

[Catalin Marinas]

On Tue, Jul 05, 2011 at 12:20:19AM +0100, Russell King - ARM Linux wrote:
> Please can you also check whether BTC invalidation is required when a
> page is removed from the page tables for the purpose of swapping it
> out (so the PTE entry becomes zero).

Not required.

> Also, is BTC invalidation required if the very same page that was
> there before is later restored without any modification to the
> instruction content of that page.  (I'm thinking page is aged to old,
> and so unmapped, then a prefetch abort which reinstates the same page
> that was there previously.)

Not required.

> Finally, is BTC invalidation required if a different physical page
> containing the same instruction content as before is placed at that
> location?

Required (see the comment below).

> Lastly, please check whether population and removal of page table
> entries for NX pages require BTC invalidation.  I expect not.
 
Not required.

Basically the rules for BTC are similar to those for the I-cache, but it
can be either ASID-tagged VIVT or VIPT, independent of the type of the
I-cache. I think current implementations of the BTC are ASID-tagged VIVT
but the architecture doesn't mandate this.

So the mapping of a data page doesn't require BTC invalidation, even if
the page is not marked as XN.

Having a branch between BTC and ISB is OK as long as we don't change the
mapping of the branch.

Mapping a page in a previously unmapped location doesn't generally
require BTC invalidation if we guarantee that there are no BTC entries
for that location (IOW, we got a new ASID or unmapping the page
previously had a BTC invalidation).

-- 
Catalin

Unnecessary cache-line flush on page table updates ?

[Russell King - ARM Linux]

On Wed, Jul 06, 2011 at 05:05:51PM +0100, Catalin Marinas wrote:
> On Tue, Jul 05, 2011 at 12:20:19AM +0100, Russell King - ARM Linux wrote:
> > Please can you also check whether BTC invalidation is required when a
> > page is removed from the page tables for the purpose of swapping it
> > out (so the PTE entry becomes zero).
> 
> Not required.
> 
> > Also, is BTC invalidation required if the very same page that was
> > there before is later restored without any modification to the
> > instruction content of that page.  (I'm thinking page is aged to old,
> > and so unmapped, then a prefetch abort which reinstates the same page
> > that was there previously.)
> 
> Not required.
> 
> > Finally, is BTC invalidation required if a different physical page
> > containing the same instruction content as before is placed at that
> > location?
> 
> Required (see the comment below).
> 
> > Lastly, please check whether population and removal of page table
> > entries for NX pages require BTC invalidation.  I expect not.
>  
> Not required.
> 
> Basically the rules for BTC are similar to those for the I-cache, but it
> can be either ASID-tagged VIVT or VIPT, independent of the type of the
> I-cache. I think current implementations of the BTC are ASID-tagged VIVT
> but the architecture doesn't mandate this.
> 
> So the mapping of a data page doesn't require BTC invalidation, even if
> the page is not marked as XN.
> 
> Having a branch between BTC and ISB is OK as long as we don't change the
> mapping of the branch.
> 
> Mapping a page in a previously unmapped location doesn't generally
> require BTC invalidation if we guarantee that there are no BTC entries
> for that location (IOW, we got a new ASID or unmapping the page
> previously had a BTC invalidation).

Okay, so I can say with confidence then that how things stand in my tree,
which limits BTC invalidation to:

1. For kernel mappings, flush_icache_range() which must be called prior
   to code placed in them being executed.

2. For user mappings, __sync_icache_dcache's icache flush, which is
   called before a non-zero user PTE is inserted.

together covers all the necessary points.

The area which needs more to focus some further work is
__sync_icache_dcache(), which is fairly over-zealous about all the
flushing.  In a boot of the Versatile Express platform, it inserts
193k PTEs.  Of that, 192k are for userspace.  For userspace:

87k are for new executable pages, 87 are for executable pages with
their attributes (eg, young-ness/read-only ness) changed.

54k are for new non-executable pages, 51k are for non-executable pages
with attributes changed.

__sync_icache_dcache() handles these userspace PTEs as follows:

0 satisfy the !pte_present_user() test.

105k are ignored due to the VIPT non-aliasing cache and the PTE being
non-executable.

0 have invalid PFNs.

964 result in a Dcache flush.  This number does not increment through
repeated monitoring of the stats via 'cat'.

87k result in an Icache invalidate with BTC invalidation.

Around 130 (of 193k) calls into this code can be saved by checking
whether the PFN has changed and the state of the young bit (no point
doing any flushing if the page is marked old as it won't be accessible
to the user in that state.)

130 may not sound like many, but that's just from a boot.  Over time,
pages will be aged, which will push that number up.  We really ought not
flush the I-cache and BTC just because we marked the page old, and then
again when it is marked young.

That's also something which won't really show up in benchmarks, as it
depends on pushing virtual memory out of the system which is not
something which benchmarks are particularly good at measuring.  It's
effect depends on overall system behaviour depends on the system setup
and memory pressure.

Unnecessary cache-line flush on page table updates ?

[Catalin Marinas]

On Wed, Jul 06, 2011 at 07:08:14PM +0100, Russell King - ARM Linux wrote:
> Okay, so I can say with confidence then that how things stand in my tree,
> which limits BTC invalidation to:
> 
> 1. For kernel mappings, flush_icache_range() which must be called prior
>    to code placed in them being executed.
> 
> 2. For user mappings, __sync_icache_dcache's icache flush, which is
>    called before a non-zero user PTE is inserted.

What about:

flush_cache_user_range()
flush_ptrace_access()

They are fine as long as you haven't removed the BTC invalidation from
__cpuc_coherent_(kern|user)_range.

> The area which needs more to focus some further work is
> __sync_icache_dcache(), which is fairly over-zealous about all the
> flushing.

Another thing that could be optimised is not to clean and invalidate the
D-cache but only clean to the PoU. The only problem is that
(flush|invalidate)_kernel_vmap_area, functions that seem to used only in
a single place. The semantics in cachetlb.txt claim to be used for I/O,
which means that they are already broken since we don't handle the L2
cache.

8<----------------------

Unnecessary cache-line flush on page table updates ?

[Russell King - ARM Linux]

On Mon, Jul 11, 2011 at 05:49:20PM +0100, Catalin Marinas wrote:
> On Wed, Jul 06, 2011 at 07:08:14PM +0100, Russell King - ARM Linux wrote:
> > Okay, so I can say with confidence then that how things stand in my tree,
> > which limits BTC invalidation to:
> > 
> > 1. For kernel mappings, flush_icache_range() which must be called prior
> >    to code placed in them being executed.
> > 
> > 2. For user mappings, __sync_icache_dcache's icache flush, which is
> >    called before a non-zero user PTE is inserted.
> 
> What about:
> 
> flush_cache_user_range()
> flush_ptrace_access()
> 
> They are fine as long as you haven't removed the BTC invalidation from
> __cpuc_coherent_(kern|user)_range.

That's the basis of flush_icache_range(), so that still has the BTC
invalidate.

> > The area which needs more to focus some further work is
> > __sync_icache_dcache(), which is fairly over-zealous about all the
> > flushing.
> 
> Another thing that could be optimised is not to clean and invalidate the
> D-cache but only clean to the PoU. The only problem is that
> (flush|invalidate)_kernel_vmap_area, functions that seem to used only in
> a single place. The semantics in cachetlb.txt claim to be used for I/O,
> which means that they are already broken since we don't handle the L2
> cache.

Those are newly introduced to cope with XFS wanting DMA to vmap'd areas
to work.  They're there to handle the vmalloc-space alias of the pages.
The DMA API sorts out the kernel direct-mapped plus L2 for non-virtually
tagged L2 caches.

So they're just an additional pre-flush and post-invalidate calls around
the DMA API to cope with vmalloc space aliases.  So I don't think they're
broken.

Unnecessary cache-line flush on page table updates ?

[Catalin Marinas]

On Mon, Jul 11, 2011 at 06:01:41PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 11, 2011 at 05:49:20PM +0100, Catalin Marinas wrote:
> > On Wed, Jul 06, 2011 at 07:08:14PM +0100, Russell King - ARM Linux wrote:
> > > The area which needs more to focus some further work is
> > > __sync_icache_dcache(), which is fairly over-zealous about all the
> > > flushing.
> > 
> > Another thing that could be optimised is not to clean and invalidate the
> > D-cache but only clean to the PoU. The only problem is that
> > (flush|invalidate)_kernel_vmap_area, functions that seem to used only in
> > a single place. The semantics in cachetlb.txt claim to be used for I/O,
> > which means that they are already broken since we don't handle the L2
> > cache.
> 
> Those are newly introduced to cope with XFS wanting DMA to vmap'd areas
> to work.  They're there to handle the vmalloc-space alias of the pages.
> The DMA API sorts out the kernel direct-mapped plus L2 for non-virtually
> tagged L2 caches.
> 
> So they're just an additional pre-flush and post-invalidate calls around
> the DMA API to cope with vmalloc space aliases.  So I don't think they're
> broken.

OK, so in this case these functions need to go to the point of
coherency. We could also optimise them to do pre-cleaning and
post-invalidation rather than always clean&invalidate.

Can we not use dmac_flush_range() (or dma_clean_range and dma_inv_range
via dmac_(map|unmap)_area) instead of __cpuc_flush_dcache_area?

-- 
Catalin

Unnecessary cache-line flush on page table updates ?

[Russell King - ARM Linux]

On Tue, Jul 12, 2011 at 02:09:07PM +0100, Catalin Marinas wrote:
> On Mon, Jul 11, 2011 at 06:01:41PM +0100, Russell King - ARM Linux wrote:
> > On Mon, Jul 11, 2011 at 05:49:20PM +0100, Catalin Marinas wrote:
> > > On Wed, Jul 06, 2011 at 07:08:14PM +0100, Russell King - ARM Linux wrote:
> > > > The area which needs more to focus some further work is
> > > > __sync_icache_dcache(), which is fairly over-zealous about all the
> > > > flushing.
> > > 
> > > Another thing that could be optimised is not to clean and invalidate the
> > > D-cache but only clean to the PoU. The only problem is that
> > > (flush|invalidate)_kernel_vmap_area, functions that seem to used only in
> > > a single place. The semantics in cachetlb.txt claim to be used for I/O,
> > > which means that they are already broken since we don't handle the L2
> > > cache.
> > 
> > Those are newly introduced to cope with XFS wanting DMA to vmap'd areas
> > to work.  They're there to handle the vmalloc-space alias of the pages.
> > The DMA API sorts out the kernel direct-mapped plus L2 for non-virtually
> > tagged L2 caches.
> > 
> > So they're just an additional pre-flush and post-invalidate calls around
> > the DMA API to cope with vmalloc space aliases.  So I don't think they're
> > broken.
> 
> OK, so in this case these functions need to go to the point of
> coherency. We could also optimise them to do pre-cleaning and
> post-invalidation rather than always clean&invalidate.
> 
> Can we not use dmac_flush_range() (or dma_clean_range and dma_inv_range
> via dmac_(map|unmap)_area) instead of __cpuc_flush_dcache_area?

We got rid of the clean and invalidate interfaces because they weren't
suitable for cross-CPU cache handling (they were defined by implementation
rather than purpose.)

We don't have enough information here to be able to call the map/unmap
functions (the DMA direction would be a nonsense) so I think these
should be new callbacks into the CPU cache code, and we do whatever
is necessary in the low level stuff, rather than trying to overload
existing functions.

Unnecessary cache-line flush on page table updates ?

[Catalin Marinas]

On Mon, Jul 04, 2011 at 08:58:19PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 04, 2011 at 04:58:35PM +0100, Catalin Marinas wrote:
> > On Mon, Jul 04, 2011 at 12:13:38PM +0100, Russell King - ARM Linux wrote:
> > > That "However" clause is an interesting one though - it seems to imply
> > > that no barrier is required when we zero out a new page table, before
> > > we link the new page table into the higher order table.  The memory we
> > > allocated for the page table doesn't become a page table until it is
> > > linked into the page table tree. 
> > > 
> > > It also raises the question about how
> > > the system knows that a particular store is to something that's a page
> > > table and something that isn't...  Given that normal memory accesses are
> > > unordered, I think this paragraph is misleading and wrong.
> > 
> > Reordering of accesses can happen because of load speculation, store
> > reordering in the write buffer or delays on the bus outside the CPU. The
> > ARM processors do not issue stores speculatively. When a memory access
> > is issued, it checks the TLB and may perform a PTW (otherwise the
> > external bus wouldn't know the address. For explicit accesses, if the
> > PTW fails, it raises a fault (and we also need a precise abort).
> 
> I think you missed my point.  The order in which stores to normal memory
> appear is not determinable.  In the case of writeback caches, it depends
> on the cache replacement algorithm, the state of the cache and its
> allocation behaviour.
> 
> It is entirely possible that the store to the page tables _could_ appear
> in memory (and therefore visible to the MMU) before the stores to the
> new page table initializing it to zeros.

I think I got your point now. Do you mean:

1. allocate a page for pte
2. zero the page
3. write the pmd entry pointing to the pte page

In this case we need a DSB between 2 and 3 (and maybe a cache flush as
well, which we already do) otherwise we could speculatively load garbage
into the TLB.

Going back to the "however" clause - "any writes to the translation
tables are not seen by any explicit memory access that occurs in program
order before the write to the translation tables" - the important issue
here is the understanding of "seen". An explicit memory access "sees" a
write to the translation table if it uses the new translation to
calculate the physical address. But in the case above, point 3 doesn't
need to see the page zeroing at point 3, they are independent, hence the
need for a DSB.

Maybe the clause could be clarified a bit - I'll ping RG.

> For instance, lets say that the L1 writeback cache is read allocate only,
> and does not read entries from the L1 cache.
> 
> The new page table happens to contain some L1 cache lines, but the upper
> table entry is not in the L1 cache.  This means that stores to the new
> page table hit the L1 cache lines, which become dirty.  The store to the
> upper table entry bypasses the L1 cache and is immediately placed into
> the write buffer.
> 
> This means in all probability that the MMU will see the new page table
> before it is visibly initialized.

If the MMU can read the L1 cache, than we need a DSB before the write to
the upper memory (I wonder whether a DMB would do, since the MMU is just
another observer). If the MMU can't read the L1 cache, than we need a
cache clean as well.

-- 
Catalin

Unnecessary cache-line flush on page table updates ?

[Russell King - ARM Linux]

On Mon, Jul 04, 2011 at 12:13:38PM +0100, Russell King - ARM Linux wrote:
> As far as the BTB goes, I wonder if we can postpone that for user TLB
> ops by setting a TIF_ flag and checking that before returning to userspace.
> That would avoid having to needlessly destroy the cached branch information
> for kernel space while looping over the page tables.  The only other place
> that needs to worry about that is module_alloc() and vmap/vmalloc with
> PROT_KERNEL_EXEC, all of which can be done in flush_cache_vmap().

Actually, we don't need to do BTC invalidate in flush_cache_vmap(),
but we do need to do a dsb+isb.

Firstly, the majority of mappings are created with NX set, so the BTC
can't be involved in anything created there (as we can't execute code
there.)

Secondly, if we're loading a code into kernel space to execute, then we
need to ensure I/D coherency via flush_icache_range(), which has to
happen after the mappings have been created, and this already does our
BTC invalidate+dsb+isb.

So, as far as kernel space TLB invalidation goes, my conclusion is that
we do not have to touch the BTC at all there, and we can leave that to
flush_icache_range() (and therefore cpu..coherent_kern_range) to deal
with.

Practically, testing it out on Versatile Express loading/unloading a
few modules shows no ill effects from dropping the BTC invalidates from
the kernel TLB invalidate ops.

8<----------
From: Russell King <rmk+kernel@arm.linux.org.uk>
ARM: btc: avoid invalidating the branch target cache on kernel TLB maintanence

Kernel space needs very little in the way of BTC maintanence as most
mappings which are created and destroyed are non-executable, and so
could never enter the instruction stream.

The case which does warrant BTC maintanence is when a module is loaded.
This creates a new executable mapping, but at that point the pages have
not been initialized with code and data, so at that point they contain
unpredictable information.  Invalidating the BTC at this stage serves
little useful purpose.

Before we execute module code, we call flush_icache_range(), which deals
with the BTC maintanence requirements.  This ensures that we have a BTC
maintanence operation before we execute code via the newly created
mapping.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/include/asm/tlbflush.h |   23 -----------------------
 arch/arm/mm/tlb-v6.S            |    1 -
 arch/arm/mm/tlb-v7.S            |    4 ----
 3 files changed, 0 insertions(+), 28 deletions(-)

diff --git a/arch/arm/include/asm/tlbflush.h b/arch/arm/include/asm/tlbflush.h
index 9aeddce..3704d03 100644
--- a/arch/arm/include/asm/tlbflush.h
+++ b/arch/arm/include/asm/tlbflush.h
@@ -481,19 +481,6 @@ static inline void local_flush_tlb_kernel_page(unsigned long kaddr)
 		asm("mcr p15, 0, %0, c8, c5, 1" : : "r" (kaddr) : "cc");
 	if (tlb_flag(TLB_V7_UIS_PAGE))
 		asm("mcr p15, 0, %0, c8, c3, 1" : : "r" (kaddr) : "cc");
-
-	if (tlb_flag(TLB_BTB)) {
-		/* flush the branch target cache */
-		asm("mcr p15, 0, %0, c7, c5, 6" : : "r" (zero) : "cc");
-		dsb();
-		isb();
-	}
-	if (tlb_flag(TLB_V7_IS_BTB)) {
-		/* flush the branch target cache */
-		asm("mcr p15, 0, %0, c7, c1, 6" : : "r" (zero) : "cc");
-		dsb();
-		isb();
-	}
 }
 
 /*
diff --git a/arch/arm/mm/tlb-v6.S b/arch/arm/mm/tlb-v6.S
index 73d7d89..cdbfda5 100644
--- a/arch/arm/mm/tlb-v6.S
+++ b/arch/arm/mm/tlb-v6.S
@@ -83,7 +83,6 @@ ENTRY(v6wbi_flush_kern_tlb_range)
 	add	r0, r0, #PAGE_SZ
 	cmp	r0, r1
 	blo	1b
-	mcr	p15, 0, r2, c7, c5, 6		@ flush BTAC/BTB
 	mcr	p15, 0, r2, c7, c10, 4		@ data synchronization barrier
 	mcr	p15, 0, r2, c7, c5, 4		@ prefetch flush
 	mov	pc, lr
diff --git a/arch/arm/mm/tlb-v7.S b/arch/arm/mm/tlb-v7.S
index 53cd5b4..dc84f72 100644
--- a/arch/arm/mm/tlb-v7.S
+++ b/arch/arm/mm/tlb-v7.S
@@ -75,11 +75,7 @@ ENTRY(v7wbi_flush_kern_tlb_range)
 	add	r0, r0, #PAGE_SZ
 	cmp	r0, r1
 	blo	1b
-	mov	r2, #0
-	ALT_SMP(mcr	p15, 0, r2, c7, c1, 6)	@ flush BTAC/BTB Inner Shareable
-	ALT_UP(mcr	p15, 0, r2, c7, c5, 6)	@ flush BTAC/BTB
 	dsb
-	isb
 	mov	pc, lr
 ENDPROC(v7wbi_flush_kern_tlb_range)
 

Unnecessary cache-line flush on page table updates ?

[Catalin Marinas]

On Mon, Jul 04, 2011 at 10:55:07PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 04, 2011 at 12:13:38PM +0100, Russell King - ARM Linux wrote:
> > As far as the BTB goes, I wonder if we can postpone that for user TLB
> > ops by setting a TIF_ flag and checking that before returning to userspace.
> > That would avoid having to needlessly destroy the cached branch information
> > for kernel space while looping over the page tables.  The only other place
> > that needs to worry about that is module_alloc() and vmap/vmalloc with
> > PROT_KERNEL_EXEC, all of which can be done in flush_cache_vmap().
> 
> Actually, we don't need to do BTC invalidate in flush_cache_vmap(),
> but we do need to do a dsb+isb.

Why would we need an ISB?

> Firstly, the majority of mappings are created with NX set, so the BTC
> can't be involved in anything created there (as we can't execute code
> there.)

OK.

> Secondly, if we're loading a code into kernel space to execute, then we
> need to ensure I/D coherency via flush_icache_range(), which has to
> happen after the mappings have been created, and this already does our
> BTC invalidate+dsb+isb.

OK.

> So, as far as kernel space TLB invalidation goes, my conclusion is that
> we do not have to touch the BTC at all there, and we can leave that to
> flush_icache_range() (and therefore cpu..coherent_kern_range) to deal
> with.
> 
> Practically, testing it out on Versatile Express loading/unloading a
> few modules shows no ill effects from dropping the BTC invalidates from
> the kernel TLB invalidate ops.

AFAIK the branch predictor is transparent on Cortex-A9 and the BTB
maintenance are no-ops. You wouldn't notice any issues if you remove
them (you can check ID_MMFR1[31:28].

> 8<----------
> From: Russell King <rmk+kernel@arm.linux.org.uk>
> ARM: btc: avoid invalidating the branch target cache on kernel TLB maintanence
> 
> Kernel space needs very little in the way of BTC maintanence as most
> mappings which are created and destroyed are non-executable, and so
> could never enter the instruction stream.
> 
> The case which does warrant BTC maintanence is when a module is loaded.
> This creates a new executable mapping, but at that point the pages have
> not been initialized with code and data, so at that point they contain
> unpredictable information.  Invalidating the BTC at this stage serves
> little useful purpose.
> 
> Before we execute module code, we call flush_icache_range(), which deals
> with the BTC maintanence requirements.  This ensures that we have a BTC
> maintanence operation before we execute code via the newly created
> mapping.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

The patch looks fine (it would be good if we get some testing on an
ARM11 platform).

-- 
Catalin

Unnecessary cache-line flush on page table updates ?

[Russell King - ARM Linux]

On Tue, Jul 05, 2011 at 10:26:00AM +0100, Catalin Marinas wrote:
> On Mon, Jul 04, 2011 at 10:55:07PM +0100, Russell King - ARM Linux wrote:
> > On Mon, Jul 04, 2011 at 12:13:38PM +0100, Russell King - ARM Linux wrote:
> > > As far as the BTB goes, I wonder if we can postpone that for user TLB
> > > ops by setting a TIF_ flag and checking that before returning to userspace.
> > > That would avoid having to needlessly destroy the cached branch information
> > > for kernel space while looping over the page tables.  The only other place
> > > that needs to worry about that is module_alloc() and vmap/vmalloc with
> > > PROT_KERNEL_EXEC, all of which can be done in flush_cache_vmap().
> > 
> > Actually, we don't need to do BTC invalidate in flush_cache_vmap(),
> > but we do need to do a dsb+isb.
> 
> Why would we need an ISB?

Because the ARM ARM tells us that we must do a dsb+isb to ensure that
instructions are visible to the instruction stream after a change to
the page tables.

I've since added the dsb+isbs back to the TLB ops because the ARM ARM
is qutie explicit that both are required to ensure that explicit
accesses see the effect of the TLB operation.  To me it is rather
perverse that an ISB is required to ensure that this sequence works:

	write page table entry
	clean
	dsb
	flush tlb entry
	dsb
	isb
	read/write new page

but the ARM ARM is quite explicit that this sequence is insufficient:

	write page table entry
	clean
	dsb
	flush tlb entry
	dsb
	read/write new page

to ensure visibility of the new page.

I suspect that the ARM ARM is wrong here - and the ISB is only required
if you're going to be executing from the new page.  Maybe that's already
been fixed?  Is there an update to DDI0406B?

Unnecessary cache-line flush on page table updates ?

[Catalin Marinas]

On Tue, Jul 05, 2011 at 10:46:52AM +0100, Russell King - ARM Linux wrote:
> On Tue, Jul 05, 2011 at 10:26:00AM +0100, Catalin Marinas wrote:
> > On Mon, Jul 04, 2011 at 10:55:07PM +0100, Russell King - ARM Linux wrote:
> > > On Mon, Jul 04, 2011 at 12:13:38PM +0100, Russell King - ARM Linux wrote:
> > > > As far as the BTB goes, I wonder if we can postpone that for user TLB
> > > > ops by setting a TIF_ flag and checking that before returning to userspace.
> > > > That would avoid having to needlessly destroy the cached branch information
> > > > for kernel space while looping over the page tables.  The only other place
> > > > that needs to worry about that is module_alloc() and vmap/vmalloc with
> > > > PROT_KERNEL_EXEC, all of which can be done in flush_cache_vmap().
> > > 
> > > Actually, we don't need to do BTC invalidate in flush_cache_vmap(),
> > > but we do need to do a dsb+isb.
> > 
> > Why would we need an ISB?
> 
> Because the ARM ARM tells us that we must do a dsb+isb to ensure that
> instructions are visible to the instruction stream after a change to
> the page tables.
> 
> I've since added the dsb+isbs back to the TLB ops because the ARM ARM
> is qutie explicit that both are required to ensure that explicit
> accesses see the effect of the TLB operation.  To me it is rather
> perverse that an ISB is required to ensure that this sequence works:
> 
> 	write page table entry
> 	clean
> 	dsb
> 	flush tlb entry
> 	dsb
> 	isb
> 	read/write new page

The same requirement can be found in latest (internal only) ARM ARM as
well. I think the reason is that some memory access already in the
pipeline (but after the TLB flush) may have sampled the state of the
page table using an old TLB entry, even though the actual memory access
will happen after the DSB.

-- 
Catalin

Unnecessary cache-line flush on page table updates ?

[Russell King - ARM Linux]

On Wed, Jul 06, 2011 at 04:52:14PM +0100, Catalin Marinas wrote:
> On Tue, Jul 05, 2011 at 10:46:52AM +0100, Russell King - ARM Linux wrote:
> > On Tue, Jul 05, 2011 at 10:26:00AM +0100, Catalin Marinas wrote:
> > > On Mon, Jul 04, 2011 at 10:55:07PM +0100, Russell King - ARM Linux wrote:
> > > > On Mon, Jul 04, 2011 at 12:13:38PM +0100, Russell King - ARM Linux wrote:
> > > > > As far as the BTB goes, I wonder if we can postpone that for user TLB
> > > > > ops by setting a TIF_ flag and checking that before returning to userspace.
> > > > > That would avoid having to needlessly destroy the cached branch information
> > > > > for kernel space while looping over the page tables.  The only other place
> > > > > that needs to worry about that is module_alloc() and vmap/vmalloc with
> > > > > PROT_KERNEL_EXEC, all of which can be done in flush_cache_vmap().
> > > > 
> > > > Actually, we don't need to do BTC invalidate in flush_cache_vmap(),
> > > > but we do need to do a dsb+isb.
> > > 
> > > Why would we need an ISB?
> > 
> > Because the ARM ARM tells us that we must do a dsb+isb to ensure that
> > instructions are visible to the instruction stream after a change to
> > the page tables.
> > 
> > I've since added the dsb+isbs back to the TLB ops because the ARM ARM
> > is qutie explicit that both are required to ensure that explicit
> > accesses see the effect of the TLB operation.  To me it is rather
> > perverse that an ISB is required to ensure that this sequence works:
> > 
> > 	write page table entry
> > 	clean
> > 	dsb
> > 	flush tlb entry
> > 	dsb
> > 	isb
> > 	read/write new page
> 
> The same requirement can be found in latest (internal only) ARM ARM as
> well. I think the reason is that some memory access already in the
> pipeline (but after the TLB flush) may have sampled the state of the
> page table using an old TLB entry, even though the actual memory access
> will happen after the DSB.

It would be useful to have a statement from RG on this.  Could you
pass this to him please?

Thanks.

Unnecessary cache-line flush on page table updates ?

[Catalin Marinas]

On Wed, Jul 06, 2011 at 04:55:25PM +0100, Russell King - ARM Linux wrote:
> On Wed, Jul 06, 2011 at 04:52:14PM +0100, Catalin Marinas wrote:
> > On Tue, Jul 05, 2011 at 10:46:52AM +0100, Russell King - ARM Linux wrote:
> > > I've since added the dsb+isbs back to the TLB ops because the ARM ARM
> > > is qutie explicit that both are required to ensure that explicit
> > > accesses see the effect of the TLB operation.  To me it is rather
> > > perverse that an ISB is required to ensure that this sequence works:
> > > 
> > > 	write page table entry
> > > 	clean
> > > 	dsb
> > > 	flush tlb entry
> > > 	dsb
> > > 	isb
> > > 	read/write new page
> > 
> > The same requirement can be found in latest (internal only) ARM ARM as
> > well. I think the reason is that some memory access already in the
> > pipeline (but after the TLB flush) may have sampled the state of the
> > page table using an old TLB entry, even though the actual memory access
> > will happen after the DSB.
> 
> It would be useful to have a statement from RG on this.  Could you
> pass this to him please?

Just checked - it is required as per the ARM ARM and there are cores
that rely on the ISB. As I said, it depends on the pipeline
implementation.

-- 
Catalin

Unnecessary cache-line flush on page table updates ?

[Russell King - ARM Linux]

On Tue, Jul 05, 2011 at 10:26:00AM +0100, Catalin Marinas wrote:
> AFAIK the branch predictor is transparent on Cortex-A9 and the BTB
> maintenance are no-ops. You wouldn't notice any issues if you remove
> them (you can check ID_MMFR1[31:28].

It's not transparent:

CPU: MMFR0=0x00100103 MMFR1=0x20000000 MMFR2=0x01230000 MMFR3=0x00102111

0b0010 Branch predictor requires flushing on:
	? enabling or disabling the MMU
	? writing new data to instruction locations
	? writing new mappings to the translation tables
	? any change to the TTBR0, TTBR1, or TTBCR registers without a
	  corresponding change to the FCSE ProcessID or ContextID.

Unnecessary cache-line flush on page table updates ?

[Catalin Marinas]

On Tue, Jul 05, 2011 at 11:48:58AM +0100, Russell King - ARM Linux wrote:
> On Tue, Jul 05, 2011 at 10:26:00AM +0100, Catalin Marinas wrote:
> > AFAIK the branch predictor is transparent on Cortex-A9 and the BTB
> > maintenance are no-ops. You wouldn't notice any issues if you remove
> > them (you can check ID_MMFR1[31:28].
> 
> It's not transparent:
> 
> CPU: MMFR0=0x00100103 MMFR1=0x20000000 MMFR2=0x01230000 MMFR3=0x00102111
> 
> 0b0010 Branch predictor requires flushing on:
> 	? enabling or disabling the MMU
> 	? writing new data to instruction locations
> 	? writing new mappings to the translation tables
> 	? any change to the TTBR0, TTBR1, or TTBCR registers without a
> 	  corresponding change to the FCSE ProcessID or ContextID.

OK, it makes sense. But what's really confusing - the A8 has the same
value for ID_MMFR1 (according to the TRM) but the BTB instructions are
no-ops (can can be enabled by the ACTLR.IBE bit). I suspect the ID_MMFR1
is wrong in this case.

Anyway, as long as we stick to the ARM ARM requirements we should be OK.

-- 
Catalin

Unnecessary cache-line flush on page table updates ?

[Russell King - ARM Linux]

On Tue, Jul 05, 2011 at 02:54:24PM +0100, Catalin Marinas wrote:
> On Tue, Jul 05, 2011 at 11:48:58AM +0100, Russell King - ARM Linux wrote:
> > On Tue, Jul 05, 2011 at 10:26:00AM +0100, Catalin Marinas wrote:
> > > AFAIK the branch predictor is transparent on Cortex-A9 and the BTB
> > > maintenance are no-ops. You wouldn't notice any issues if you remove
> > > them (you can check ID_MMFR1[31:28].
> > 
> > It's not transparent:
> > 
> > CPU: MMFR0=0x00100103 MMFR1=0x20000000 MMFR2=0x01230000 MMFR3=0x00102111
> > 
> > 0b0010 Branch predictor requires flushing on:
> > 	? enabling or disabling the MMU
> > 	? writing new data to instruction locations
> > 	? writing new mappings to the translation tables
> > 	? any change to the TTBR0, TTBR1, or TTBCR registers without a
> > 	  corresponding change to the FCSE ProcessID or ContextID.
> 
> OK, it makes sense. But what's really confusing - the A8 has the same
> value for ID_MMFR1 (according to the TRM) but the BTB instructions are
> no-ops (can can be enabled by the ACTLR.IBE bit). I suspect the ID_MMFR1
> is wrong in this case.

Grumble.  This kind of thing worries me.

If the ID registers are wrong, then they can't be relied upon by software
making decisions about what can be skipped.  So how can we be confident
that MMFR3 23:20 != 0 really does mean that we don't need to clean the
cache when writing the page tables, and is not a result of some bug?

Unnecessary cache-line flush on page table updates ?

[Catalin Marinas]

On Tue, Jul 05, 2011 at 03:15:10PM +0100, Russell King - ARM Linux wrote:
> On Tue, Jul 05, 2011 at 02:54:24PM +0100, Catalin Marinas wrote:
> > On Tue, Jul 05, 2011 at 11:48:58AM +0100, Russell King - ARM Linux wrote:
> > > On Tue, Jul 05, 2011 at 10:26:00AM +0100, Catalin Marinas wrote:
> > > > AFAIK the branch predictor is transparent on Cortex-A9 and the BTB
> > > > maintenance are no-ops. You wouldn't notice any issues if you remove
> > > > them (you can check ID_MMFR1[31:28].
> > > 
> > > It's not transparent:
> > > 
> > > CPU: MMFR0=0x00100103 MMFR1=0x20000000 MMFR2=0x01230000 MMFR3=0x00102111
> > > 
> > > 0b0010 Branch predictor requires flushing on:
> > > 	? enabling or disabling the MMU
> > > 	? writing new data to instruction locations
> > > 	? writing new mappings to the translation tables
> > > 	? any change to the TTBR0, TTBR1, or TTBCR registers without a
> > > 	  corresponding change to the FCSE ProcessID or ContextID.
> > 
> > OK, it makes sense. But what's really confusing - the A8 has the same
> > value for ID_MMFR1 (according to the TRM) but the BTB instructions are
> > no-ops (can can be enabled by the ACTLR.IBE bit). I suspect the ID_MMFR1
> > is wrong in this case.
> 
> Grumble.  This kind of thing worries me.
> 
> If the ID registers are wrong, then they can't be relied upon by software
> making decisions about what can be skipped.  

It could be because the BTB operations are configurable via an ACTLR
bit, but the ID registers are fixed values.

> So how can we be confident
> that MMFR3 23:20 != 0 really does mean that we don't need to clean the
> cache when writing the page tables, and is not a result of some bug?

Well, you can get other bugs, you can never guarantee anything. But when
they are found, there are errata workarounds. This would actually be a
very easy to detect issue.

-- 
Catalin

Unnecessary cache-line flush on page table updates ?

[Russell King - ARM Linux]

On Mon, Jul 04, 2011 at 11:43:29AM +0100, Catalin Marinas wrote:
> On Mon, Jul 04, 2011 at 11:02:21AM +0100, Russell King - ARM Linux wrote:
> > The single-TLB model works fairly well, but as I thought the lack of
> > mcr%? processing by GCC makes the asm/tlbflush.h code fairly disgusting
> > even for a v6+v7 kernel.  Luckily, we can play some tricks and sort
> > some of that out.  The patch below is not complete (and can result in
> > some rules of the architecture being violated - namely the requirement
> > for an ISB after the BTB flush without a branch between) but it
> > illustrates the idea:
> 
> I'm not sure about this rule, I can ask for some clarification (we are
> not changing the memory map of the branch we execute).

There's no need for clarification on the BTB and branch issue, the
ARM ARM is quite clear on this topic:

Branch predictor maintenance operations and the memory order model
The following rule describes the effect of the memory order model on
the branch predictor maintenance operations:
? Any invalidation of the branch predictor is guaranteed to take effect only
  after one of the following:
  ? execution of a ISB instruction
  ? taking an exception
  ? return from an exception.
Therefore, if a branch instruction appears between an invalidate branch
prediction instruction and an ISB operation, exception entry or exception
return, it is UNPREDICTABLE whether the branch instruction is affected by
the invalidate. Software must avoid this ordering of instructions, because
it might lead to UNPREDICTABLE behavior.

The branch predictor maintenance operations must be used to invalidate
entries in the branch predictor after any of the following events:
? enabling or disabling the MMU
? writing new data to instruction locations
? writing new mappings to the translation tables
? changes to the TTBR0, TTBR1, or TTBCR registers, unless accompanied by
  a change to the ContextID or the FCSE ProcessID.

Failure to invalidate entries might give UNPREDICTABLE results, caused by
the execution of old branches.

So:

	mcr	p15, 0, %0, c7, c5, 6 @ BTC invalidate
	tst	%1, #value
	beq	no_dsb_isb
	dsb
	isb
no_dsb_isb:

is strictly not predictable whether the branch will be taken.

The only remaining question is: the operations prior to the BTC invalidate
will not have changed the code path which 'beq' is a part of, so is it
_really_ the case that a BTC invalidate will make such a branch
unpredictable?

I guess the reason for this is that if the BTC is half-way through an
invalidate, its state may not be determinable, and so it is not
determinable whether the branch will be taken irrespective of the
previous BTC state before the invalidate and the new state after the
isb.

Or, to put it another way, the BTC can return random results between
the BTC invalidate and the following ISB.

Unnecessary cache-line flush on page table updates ?

[Catalin Marinas]

On Mon, Jul 04, 2011 at 02:05:21PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 04, 2011 at 11:43:29AM +0100, Catalin Marinas wrote:
> > On Mon, Jul 04, 2011 at 11:02:21AM +0100, Russell King - ARM Linux wrote:
> > > The single-TLB model works fairly well, but as I thought the lack of
> > > mcr%? processing by GCC makes the asm/tlbflush.h code fairly disgusting
> > > even for a v6+v7 kernel.  Luckily, we can play some tricks and sort
> > > some of that out.  The patch below is not complete (and can result in
> > > some rules of the architecture being violated - namely the requirement
> > > for an ISB after the BTB flush without a branch between) but it
> > > illustrates the idea:
> > 
> > I'm not sure about this rule, I can ask for some clarification (we are
> > not changing the memory map of the branch we execute).
> 
> There's no need for clarification on the BTB and branch issue, the
> ARM ARM is quite clear on this topic:
> 
> Branch predictor maintenance operations and the memory order model
> The following rule describes the effect of the memory order model on
> the branch predictor maintenance operations:
> ? Any invalidation of the branch predictor is guaranteed to take effect only
>   after one of the following:
>   ? execution of a ISB instruction
>   ? taking an exception
>   ? return from an exception.
> Therefore, if a branch instruction appears between an invalidate branch
> prediction instruction and an ISB operation, exception entry or exception
> return, it is UNPREDICTABLE whether the branch instruction is affected by
> the invalidate. Software must avoid this ordering of instructions, because
> it might lead to UNPREDICTABLE behavior.

So my this rule should we always use an ISB even if we only flushed user
TLB entries? Otherwise we get some branches before the exception return.

My reading is that it only affects the branch in the TLB op function for
which we don't change any mapping, so there isn't any risk of
mis-prediction. That's why I think I should ask for clarification if we
can't avoid the branch.

-- 
Catalin

Unnecessary cache-line flush on page table updates ?

[heechul Yun]

On Mon, Jul 4, 2011 at 2:45 AM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Fri, Jul 01, 2011 at 10:42:00PM +0100, heechul Yun wrote:
>> Great.
>>
>> Removing the PTE flush seems to have a noticeable performance
>> difference in my test. The followings are lmbench 3.0a performance
>> result measured on a Cortex A9 SMP platform. So far, I did not have
>> any problem while doing various test.
>>
>> =========
>> mm-patch:
>> =========
>> Pagefaults on /tmp/XXX: 3.0759 microseconds
>> Process fork+exit: 464.5414 microseconds
>> Process fork+execve: 785.4944 microseconds
>> Process fork+/bin/sh -c: 488.6204 microseconds
>>
>> =========
>> original:
>> =========
>> Pagefaults on /tmp/XXX: 3.6209 microseconds
>> Process fork+exit: 485.5236 microseconds
>> Process fork+execve: 820.0613 microseconds
>> Process fork+/bin/sh -c: 2966.3828 microseconds
>
> Given these results, I think it's worth merging the patch. Can I add
> your Tested-by?

Yes you can.
I think this patch will be valuable for many applications (e.g.,
server applications).

Heechul

>
> I think there can be a few other optimisations in the TLB area but it
> needs some digging.
>
> Thanks.
>
> --
> Catalin
>

Unnecessary cache-line flush on page table updates ?

[heechul Yun]

On Fri, Jul 1, 2011 at 2:42 PM, heechul Yun <heechul@illinois.edu> wrote:
> Great.
>
> Removing the PTE flush seems to have a noticeable performance
> difference in my test. The followings are lmbench 3.0a performance
> result measured on a Cortex A9 SMP platform. So far, I did not have
> any problem while doing various test.
>
> =========
> mm-patch:
> =========
> Pagefaults on /tmp/XXX: 3.0759 microseconds
> Process fork+exit: 464.5414 microseconds
> Process fork+execve: 785.4944 microseconds
> Process fork+/bin/sh -c: 488.6204 microseconds

I realized that I made a big mistake on the  fork+/bin/sh data (other
numbers are fine). What happened was that there was no /bin/sh
(android platform has /system/bin/sh instead). When I did the first
test with the original kernel I made /bin as a symlink to /system/bin
which I forgot to do after flashing the system with the new, patched,
kernel.

The following is a result of the patched kernel after the correct symlink

Pagefaults on /tmp/XXX: 3.0991 microseconds
Process fork+exit: 465.1620 microseconds
Process fork+execve: 781.5077 microseconds
Process fork+/bin/sh -c: 2804.2023 microseconds

It's still noticeably better (about 5~15%) but not substantial.
Sorry for the confusion.

Heechul

>
> =========
> original:
> =========
> Pagefaults on /tmp/XXX: 3.6209 microseconds
> Process fork+exit: 485.5236 microseconds
> Process fork+execve: 820.0613 microseconds
> Process fork+/bin/sh -c: 2966.3828 microseconds
>
> Heechul
>
> On Fri, Jul 1, 2011 at 3:10 AM, Catalin Marinas <catalin.marinas@arm.com> wrote:
>> On Fri, Jul 01, 2011 at 08:04:42AM +0100, heechul Yun wrote:
>>> Based on TRM of Cortex A9, the MMU reads page table entries from L1-D
>>> cache not from memory. Then I think we do not need to flush the cache
>>> line in the following code because MMU will always see up-to-date view
>>> of page table in both UP and SMP systems.
>>>
>>> linux/arch/arm/mm/proc-v7.S
>>>
>>> ENTRY(cpu_v7_set_pte_ext)
>>> ? ? ? ...
>>> ? ? ? ? mcr ? ? p15, 0, r0, c7, c10, 1 ? ? ? ? ?@ flush_pte from
>>> D-cache // why we need this in A9?
>>> ? ? ? ? ?
>>>
>>> If this is a necessary one, could you please explain the reason? Thanks.
>>
>> No, it's not necessary, only that this file is used by other processors
>> as well. The solution below checks the ID_MMFR3[23:20] bits (coherent
>> walk) and avoid flushing if the value is 1. The same could be done for
>> PMD entries, though that's less critical than the PTEs.
>>
>> Please note that the patch is not fully tested.
>>
>> 8<--------------------
>>
>> From 67bd5ebdf622637f8293286146441e6292713c3d Mon Sep 17 00:00:00 2001
>> From: Catalin Marinas <catalin.marinas@arm.com>
>> Date: Fri, 1 Jul 2011 10:57:07 +0100
>> Subject: [PATCH] ARMv7: Do not clean the PTE coherent page table walk is supported
>>
>> This patch adds a check for the ID_MMFR3[23:20] bits (coherent walk) and
>> only cleans the D-cache corresponding to a PTE if coherent page table
>> walks are not supported.
>>
>> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
>> ---
>> ?arch/arm/mm/proc-v7.S | ? ?4 +++-
>> ?1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
>> index 8013afc..fc5b36f 100644
>> --- a/arch/arm/mm/proc-v7.S
>> +++ b/arch/arm/mm/proc-v7.S
>> @@ -166,7 +166,9 @@ ENTRY(cpu_v7_set_pte_ext)
>> ?ARM( ?str ? ? r3, [r0, #2048]! )
>> ?THUMB( ? ? ? ?add ? ? r0, r0, #2048 )
>> ?THUMB( ? ? ? ?str ? ? r3, [r0] )
>> - ? ? ? mcr ? ? p15, 0, r0, c7, c10, 1 ? ? ? ? ?@ flush_pte
>> + ? ? ? mrc ? ? p15, 0, r3, c0, c1, 7 ? ? ? ? ? @ read ID_MMFR3
>> + ? ? ? tst ? ? r3, #0xf << 20 ? ? ? ? ? ? ? ? ?@ check the coherent walk bits
>> + ? ? ? mcreq ? p15, 0, r0, c7, c10, 1 ? ? ? ? ?@ flush_pte
>> ?#endif
>> ? ? ? ?mov ? ? pc, lr
>> ?ENDPROC(cpu_v7_set_pte_ext)
>>
>> --
>> Catalin
>>
>