arm64: change PoC D-cache flush to PoU

RFC PATCH New Ashok Kumar 3 participants 9 messages Dec 14, 2015 arm64

Share & tools

Ashok Kumar

author

2015-12-14 13:27 UTC #0 lore raw

cover arm64: change PoC D-cache flush to PoU

For keeping I and D coherent, dcache flush till PoU(Point of Unification) should be sufficient instead of doing till PoC(Point of coherence). In SoC with more levels of cache, there could be a performance hit in doing flush till PoC as __flush_dcache_area does both flush and invalidate. Introduced new API __flush_dcache_area_pou which does only clean till PoU.

Also deferred dcache flush in __cpu_copy_user_page to __sync_icache_dcache. May I know why I/D sync is needed in __cpu_copy_user_page? My understanding is that any self modifying code in userspace is supposed to take care of the coherency using the respective cache flush system call.

Ashok Kumar (2):
arm64: Defer dcache flush in __cpu_copy_user_page
arm64: Use PoU cache instr for I/D coherency

 arch/arm64/include/asm/cacheflush.h |  1 +
 arch/arm64/mm/cache.S               | 22 ++++++++++++++++++++++
 arch/arm64/mm/copypage.c            |  3 ++-
 arch/arm64/mm/flush.c               | 13 +++++++++----
 4 files changed, 34 insertions(+), 5 deletions(-)

Signature · 1 line

2.1.0

Ashok Kumar

author

2015-12-14 13:27 UTC #1 lore raw

RFC PATCH 1/2 arm64: Defer dcache flush in __cpu_copy_user_page

Defer dcache flushing to __sync_icache_dcache by calling flush_dcache_page which clears PG_dcache_clean flag.

Signed-off-by Ashok Kumar <[email protected]>

---
 arch/arm64/mm/copypage.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/arm64/mm/copypage.c +2 -1

index 13bbc3be..22e4cb4 100644
@@ -24,8 +24,9 @@
 
 void __cpu_copy_user_page(void *kto, const void *kfrom, unsigned long vaddr)
 {
+	struct page *page = virt_to_page(kto);
 	copy_page(kto, kfrom);
-	__flush_dcache_area(kto, PAGE_SIZE);
+	flush_dcache_page(page);
 }
 EXPORT_SYMBOL_GPL(__cpu_copy_user_page);

Signature · 1 line

2.1.0

Catalin Marinas

maintainer

2015-12-14 15:18 UTC #2 lore raw

RFC PATCH 1/2 arm64: Defer dcache flush in __cpu_copy_user_page

Quoted 13 lines · Ashok Kumar

Defer dcache flushing to __sync_icache_dcache by calling flush_dcache_page which clears PG_dcache_clean flag.
Signed-off-by Ashok Kumar <[email protected]>
---
 arch/arm64/mm/copypage.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/arm64/mm/copypage.c +0 -0
index 13bbc3be..22e4cb4 100644

arch/arm64/mm/copypage.c line 31

Quoted 9 lines

arch/arm64/mm/copypage.c +2 -1

@@ -24,8 +24,9 @@
 
 void __cpu_copy_user_page(void *kto, const void *kfrom, unsigned long vaddr)
 {
+	struct page *page = virt_to_page(kto);
 	copy_page(kto, kfrom);
-	__flush_dcache_area(kto, PAGE_SIZE);
+	flush_dcache_page(page);
 }
 EXPORT_SYMBOL_GPL(__cpu_copy_user_page);

Acked-by Catalin Marinas <[email protected]>

Signature · 1 line

Catalin

Ashok Kumar

author

2015-12-14 13:27 UTC #3 lore raw

RFC PATCH 2/2 arm64: Use PoU cache instr for I/D coherency

In reply to Ashok Kumar

In systems with three levels of cache(PoU at L1 and PoC at L3), PoC cache flush instructions flushes L2 and L3 caches which could affect performance.
For cache flushes for I and D coherency, PoU should suffice. So changing all I and D coherency related cache flushes to PoU.

Introduced a new __flush_dcache_area_pou API for dcache flush till PoU

Signed-off-by Ashok Kumar <[email protected]>

---
 arch/arm64/include/asm/cacheflush.h |  1 +
 arch/arm64/mm/cache.S               | 22 ++++++++++++++++++++++
 arch/arm64/mm/flush.c               | 13 +++++++++----
 3 files changed, 32 insertions(+), 4 deletions(-)

arch/arm64/include/asm/cacheflush.h +1 -0

index c75b8d0..e4b13f7 100644
@@ -68,6 +68,7 @@
 extern void flush_cache_range(struct vm_area_struct *vma, unsigned long start, unsigned long end);
 extern void flush_icache_range(unsigned long start, unsigned long end);
 extern void __flush_dcache_area(void *addr, size_t len);
+extern void __flush_dcache_area_pou(void *addr, size_t len);
 extern long __flush_cache_user_range(unsigned long start, unsigned long end);
 
 static inline void flush_cache_mm(struct mm_struct *mm)

arch/arm64/mm/cache.S +22 -0

index eb48d5d..037293c 100644
@@ -101,6 +101,28 @@ ENTRY(__flush_dcache_area)
 ENDPROC(__flush_dcache_area)
 
 /*
+ *	__flush_dcache_area_pou(kaddr, size)
+ *
+ *	Ensure that the data held in the page kaddr is written back to the
+ *	page in question till Point of Unification.
+ *
+ *	- kaddr   - kernel address
+ *	- size    - size in question
+ */
+ENTRY(__flush_dcache_area_pou)
+	dcache_line_size x2, x3
+	add	x1, x0, x1
+	sub	x3, x2, #1
+	bic	x0, x0, x3
+1:	dc	cvau, x0			// clean D line till PoU
+	add	x0, x0, x2
+	cmp	x0, x1
+	b.lo	1b
+	dsb	sy
+	ret
+ENDPROC(__flush_dcache_area_pou)
+
+/*
  *	__inval_cache_range(start, end)
  *	- start   - start address of region
  *	- end     - end address of region

arch/arm64/mm/flush.c +9 -4

index c26b804..6235af6 100644
@@ -41,7 +41,7 @@ static void flush_ptrace_access(struct vm_area_struct *vma, struct page *page,
 	if (vma->vm_flags & VM_EXEC) {
 		unsigned long addr = (unsigned long)kaddr;
 		if (icache_is_aliasing()) {
-			__flush_dcache_area(kaddr, len);
+			__flush_dcache_area_pou(kaddr, len);
 			__flush_icache_all();
 		} else {
 			flush_icache_range(addr, addr + len);

@@ -75,9 +75,14 @@ void __sync_icache_dcache(pte_t pte, unsigned long addr)
 		return;
 
 	if (!test_and_set_bit(PG_dcache_clean, &page->flags)) {
-		__flush_dcache_area(page_address(page),
-				PAGE_SIZE << compound_order(page));
-		__flush_icache_all();
+		if (icache_is_aliasing()) {
+			__flush_dcache_area_pou(page_address(page),
+					    PAGE_SIZE << compound_order(page));
+			__flush_icache_all();
+		} else
+			flush_icache_range(page_address(page),
+					   page_address(page) +
+					   (PAGE_SIZE << compound_order(page)));
 	} else if (icache_is_aivivt()) {
 		__flush_icache_all();
 	}

Signature · 1 line

2.1.0

Mark Rutland

reviewer

2015-12-14 14:04 UTC #4 lore raw

RFC PATCH 2/2 arm64: Use PoU cache instr for I/D coherency

Hi,

Quoted 33 lines · Ashok Kumar

In systems with three levels of cache(PoU at L1 and PoC at L3), PoC cache flush instructions flushes L2 and L3 caches which could affect performance.
For cache flushes for I and D coherency, PoU should suffice. So changing all I and D coherency related cache flushes to PoU.
Introduced a new __flush_dcache_area_pou API for dcache flush till PoU
Signed-off-by Ashok Kumar <[email protected]>
---
 arch/arm64/include/asm/cacheflush.h |  1 +
 arch/arm64/mm/cache.S               | 22 ++++++++++++++++++++++
 arch/arm64/mm/flush.c               | 13 +++++++++----
 3 files changed, 32 insertions(+), 4 deletions(-)
arch/arm64/include/asm/cacheflush.h +1 -0
index c75b8d0..e4b13f7 100644
@@ -68,6 +68,7 @@
 extern void flush_cache_range(struct vm_area_struct *vma, unsigned long start, unsigned long end);
 extern void flush_icache_range(unsigned long start, unsigned long end);
 extern void __flush_dcache_area(void *addr, size_t len);
+extern void __flush_dcache_area_pou(void *addr, size_t len);
 extern long __flush_cache_user_range(unsigned long start, unsigned long end);
 
 static inline void flush_cache_mm(struct mm_struct *mm)
arch/arm64/mm/cache.S +0 -0
index eb48d5d..037293c 100644
@@ -101,6 +101,28 @@ ENTRY(__flush_dcache_area)
 ENDPROC(__flush_dcache_area)

arch/arm64/mm/cache.S ENTRY(__flush_dcache_area) · line 111

Quoted 10 lines

arch/arm64/mm/cache.S +8 -0

 
 /*
+ *	__flush_dcache_area_pou(kaddr, size)
+ *
+ *	Ensure that the data held in the page kaddr is written back to the
+ *	page in question till Point of Unification.
+ *
+ *	- kaddr   - kernel address
+ *	- size    - size in question
+ */

I think it would be better to call this __clean_dcache_area_pou, to make it clean that there's no invalidate (i.e. it can only be used to push data out to the PoU).

arch/arm64/mm/cache.S ENTRY(__flush_dcache_area) · line 123

Quoted 12 lines

arch/arm64/mm/cache.S +12 -0

+ENTRY(__flush_dcache_area_pou)
+	dcache_line_size x2, x3
+	add	x1, x0, x1
+	sub	x3, x2, #1
+	bic	x0, x0, x3
+1:	dc	cvau, x0			// clean D line till PoU
+	add	x0, x0, x2
+	cmp	x0, x1
+	b.lo	1b
+	dsb	sy
+	ret
+ENDPROC(__flush_dcache_area_pou)

At the same time we can reduce the domin of that dsb to ish, given all CPUs will be in the same Inner-Shareable domain.

We could also factor the common logic into a macro, e.g.

	/*
	 * x0 - kaddr
	 * x1 - size
	 */
	.macro dcache_by_line_op op, domain
	dcache_line_size x2, x3
	add	x1, x0, x1
	sub	x3, x2, #1
	bic	x0, x0, x3
1:	dc	\op, x0
	add	x0, x0, x2
	cmp	x0, x1
	b.lo	1b
	dsb	\domain
	.endm

ENTRY(__flush_dcache_area)
dcache_by_line_op civac, sy
ret
ENDPIPROC(__flush_dcache_area)

ENTRY(__clean_dcache_area_pou)
dcache_by_line_op cvau, ish
ret
ENDPIPROC(__clean_dcache_area)

Quoted 23 lines

arch/arm64/mm/cache.S +2 -0

+
+/*
  *	__inval_cache_range(start, end)
  *	- start   - start address of region
  *	- end     - end address of region

arch/arm64/mm/flush.c +1 -2

index c26b804..6235af6 100644
@@ -41,7 +41,7 @@ static void flush_ptrace_access(struct vm_area_struct *vma, struct page *page,
 	if (vma->vm_flags & VM_EXEC) {
 		unsigned long addr = (unsigned long)kaddr;
 		if (icache_is_aliasing()) {
-			__flush_dcache_area(kaddr, len);
+			__flush_dcache_area_pou(kaddr, len);
 			__flush_icache_all();
 		} else {
 			flush_icache_range(addr, addr + len);

@@ -75,9 +75,14 @@ void __sync_icache_dcache(pte_t pte, unsigned long addr)
 		return;
 
 	if (!test_and_set_bit(PG_dcache_clean, &page->flags)) {
-		__flush_dcache_area(page_address(page),

arch/arm64/mm/flush.c void __sync_icache_dcache(pte_t pte, unsigned long addr) · line 85

Quoted 10 lines

arch/arm64/mm/flush.c +8 -2

-				PAGE_SIZE << compound_order(page));
-		__flush_icache_all();
+		if (icache_is_aliasing()) {
+			__flush_dcache_area_pou(page_address(page),
+					    PAGE_SIZE << compound_order(page));
+			__flush_icache_all();
+		} else
+			flush_icache_range(page_address(page),
+					   page_address(page) +
+					   (PAGE_SIZE << compound_order(page)));

Nit: If one side of an if/else has braces, the other side should too.

Other than those points, this looks good to me.

Thanks,
Mark.

Ashok Kumar

author

2015-12-14 16:48 UTC #5 lore raw

RFC PATCH 2/2 arm64: Use PoU cache instr for I/D coherency

Hi,

Thanks for the review. I will incorporate all the comments and post v2 soon.

Quoted 136 lines · Mark Rutland

Hi,
Quoted 33 lines · Ashok Kumar
In systems with three levels of cache(PoU at L1 and PoC at L3), PoC cache flush instructions flushes L2 and L3 caches which could affect performance.
For cache flushes for I and D coherency, PoU should suffice. So changing all I and D coherency related cache flushes to PoU.
Introduced a new __flush_dcache_area_pou API for dcache flush till PoU
Signed-off-by Ashok Kumar <[email protected]>
---
 arch/arm64/include/asm/cacheflush.h |  1 +
 arch/arm64/mm/cache.S               | 22 ++++++++++++++++++++++
 arch/arm64/mm/flush.c               | 13 +++++++++----
 3 files changed, 32 insertions(+), 4 deletions(-)
arch/arm64/include/asm/cacheflush.h +1 -0
index c75b8d0..e4b13f7 100644
@@ -68,6 +68,7 @@
 extern void flush_cache_range(struct vm_area_struct *vma, unsigned long start, unsigned long end);
 extern void flush_icache_range(unsigned long start, unsigned long end);
 extern void __flush_dcache_area(void *addr, size_t len);
+extern void __flush_dcache_area_pou(void *addr, size_t len);
 extern long __flush_cache_user_range(unsigned long start, unsigned long end);
 
 static inline void flush_cache_mm(struct mm_struct *mm)
arch/arm64/mm/cache.S +0 -0
index eb48d5d..037293c 100644
@@ -101,6 +101,28 @@ ENTRY(__flush_dcache_area)
 ENDPROC(__flush_dcache_area)
arch/arm64/mm/cache.S ENTRY(__flush_dcache_area) · line 111
Quoted 10 lines
arch/arm64/mm/cache.S +8 -0
 
 /*
+ *	__flush_dcache_area_pou(kaddr, size)
+ *
+ *	Ensure that the data held in the page kaddr is written back to the
+ *	page in question till Point of Unification.
+ *
+ *	- kaddr   - kernel address
+ *	- size    - size in question
+ */
I think it would be better to call this __clean_dcache_area_pou, to make it clean that there's no invalidate (i.e. it can only be used to push data out to the PoU).
arch/arm64/mm/cache.S ENTRY(__flush_dcache_area) · line 123
Quoted 12 lines
arch/arm64/mm/cache.S +12 -0
+ENTRY(__flush_dcache_area_pou)
+	dcache_line_size x2, x3
+	add	x1, x0, x1
+	sub	x3, x2, #1
+	bic	x0, x0, x3
+1:	dc	cvau, x0			// clean D line till PoU
+	add	x0, x0, x2
+	cmp	x0, x1
+	b.lo	1b
+	dsb	sy
+	ret
+ENDPROC(__flush_dcache_area_pou)
At the same time we can reduce the domin of that dsb to ish, given all CPUs will be in the same Inner-Shareable domain.
We could also factor the common logic into a macro, e.g.
	/*
	 * x0 - kaddr
	 * x1 - size
	 */
	.macro dcache_by_line_op op, domain
	dcache_line_size x2, x3
	add	x1, x0, x1
	sub	x3, x2, #1
	bic	x0, x0, x3
1:	dc	\op, x0
	add	x0, x0, x2
	cmp	x0, x1
	b.lo	1b
	dsb	\domain
	.endm
ENTRY(__flush_dcache_area)
dcache_by_line_op civac, sy
ret
ENDPIPROC(__flush_dcache_area)
ENTRY(__clean_dcache_area_pou)
dcache_by_line_op cvau, ish
ret
ENDPIPROC(__clean_dcache_area)
Quoted 23 lines
arch/arm64/mm/cache.S +2 -0
+
+/*
  *	__inval_cache_range(start, end)
  *	- start   - start address of region
  *	- end     - end address of region
arch/arm64/mm/flush.c +1 -2
index c26b804..6235af6 100644
@@ -41,7 +41,7 @@ static void flush_ptrace_access(struct vm_area_struct *vma, struct page *page,
 	if (vma->vm_flags & VM_EXEC) {
 		unsigned long addr = (unsigned long)kaddr;
 		if (icache_is_aliasing()) {
-			__flush_dcache_area(kaddr, len);
+			__flush_dcache_area_pou(kaddr, len);
 			__flush_icache_all();
 		} else {
 			flush_icache_range(addr, addr + len);
@@ -75,9 +75,14 @@ void __sync_icache_dcache(pte_t pte, unsigned long addr)
 		return;
 
 	if (!test_and_set_bit(PG_dcache_clean, &page->flags)) {
-		__flush_dcache_area(page_address(page),
arch/arm64/mm/flush.c void __sync_icache_dcache(pte_t pte, unsigned long addr) · line 85
Quoted 10 lines
arch/arm64/mm/flush.c +8 -2
-				PAGE_SIZE << compound_order(page));
-		__flush_icache_all();
+		if (icache_is_aliasing()) {
+			__flush_dcache_area_pou(page_address(page),
+					    PAGE_SIZE << compound_order(page));
+			__flush_icache_all();
+		} else
+			flush_icache_range(page_address(page),
+					   page_address(page) +
+					   (PAGE_SIZE << compound_order(page)));
Nit: If one side of an if/else has braces, the other side should too.
Other than those points, this looks good to me.
Thanks,
Mark.

Catalin Marinas

maintainer

2015-12-14 15:11 UTC #6 lore raw

cover arm64: change PoC D-cache flush to PoU

In reply to Ashok Kumar

Quoted 4 lines · Ashok Kumar

Also deferred dcache flush in __cpu_copy_user_page to __sync_icache_dcache. May I know why I/D sync is needed in __cpu_copy_user_page? My understanding is that any self modifying code in userspace is supposed to take care of the coherency using the respective cache flush system call.

I think it may have been there for historical reasons on arch/arm (115b22474eb1 - "ARM: 5794/1: Flush the D-cache during copy_user_highpage()") and imported in arch/arm64. But looking back at this, I don't think we need it for two reasons: dynamic linker no longer relocating symbols in a text page (and cause CoW) and set_pte_at() already calling __sync_icache_dcache().

Signature · 1 line

Catalin

Ashok Kumar

author

2015-12-14 16:46 UTC #7 lore raw

cover arm64: change PoC D-cache flush to PoU

Hi,

Thanks for the review.

Quoted 6 lines · Catalin Marinas

Quoted 4 lines · Ashok Kumar
Also deferred dcache flush in __cpu_copy_user_page to __sync_icache_dcache. May I know why I/D sync is needed in __cpu_copy_user_page? My understanding is that any self modifying code in userspace is supposed to take care of the coherency using the respective cache flush system call.

Quoted 6 lines

I think it may have been there for historical reasons on arch/arm (115b22474eb1 - "ARM: 5794/1: Flush the D-cache during copy_user_highpage()") and imported in arch/arm64. But looking back at this, I don't think we need it for two reasons: dynamic linker no longer relocating symbols in a text page (and cause CoW) and set_pte_at() already calling __sync_icache_dcache().

In that case, shall we get rid of the flush in __sync_icache_dcache also by not calling flush_dcache_page in __cpu_copy_user_page?

Quoted 4 lines

Signature · 2 lines
-- 
Catalin

Catalin Marinas

maintainer

2015-12-14 17:52 UTC #8 lore raw

cover arm64: change PoC D-cache flush to PoU

Quoted 14 lines · Ashok Kumar

Quoted 6 lines · Catalin Marinas
Quoted 4 lines · Ashok Kumar
Also deferred dcache flush in __cpu_copy_user_page to __sync_icache_dcache. May I know why I/D sync is needed in __cpu_copy_user_page? My understanding is that any self modifying code in userspace is supposed to take care of the coherency using the respective cache flush system call.
Quoted 6 lines
I think it may have been there for historical reasons on arch/arm (115b22474eb1 - "ARM: 5794/1: Flush the D-cache during copy_user_highpage()") and imported in arch/arm64. But looking back at this, I don't think we need it for two reasons: dynamic linker no longer relocating symbols in a text page (and cause CoW) and set_pte_at() already calling __sync_icache_dcache().

Quoted 2 lines

In that case, shall we get rid of the flush in __sync_icache_dcache also by not calling flush_dcache_page in __cpu_copy_user_page?

We should still keep it. Assuming that we get CoW mostly on no-exec pages, the penalty should be minimal since set_pte_at() only cares about executable mappings. But I can't guarantee that user space doesn't do any CoW on executable mappings. We could for example have some JIT'ed code that forked and the child changes some literal pool data (not instructions) causing a CoW. It would not expect the I/D caches to become incoherent.

Signature · 1 line

Catalin