diff mbox

[2/2] ARM: remove unnecessary flush of anon pages in flush(_kernel)_dcache_page()

Message ID 1348695659-27603-3-git-send-email-gmbnomis@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Simon Baatz Sept. 26, 2012, 9:40 p.m. UTC 
On non-aliasing VIPT D-caches, there is no need to flush the kernel
mapping of anon pages in flush_kernel_dcache_page() and
flush_dcache_page() directly.  If the page is mapped as executable
later, the necessary D/I-cache flush will be done in
__sync_icache_dcache().

Signed-off-by: Simon Baatz <gmbnomis@gmail.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Russell King <linux@arm.linux.org.uk>
---
 arch/arm/mm/flush.c |   60 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 25 deletions(-)

Comments

Catalin Marinas Sept. 27, 2012, 12:23 p.m. UTC | #1 
On Wed, Sep 26, 2012 at 10:40:59PM +0100, Simon Baatz wrote:
> On non-aliasing VIPT D-caches, there is no need to flush the kernel
> mapping of anon pages in flush_kernel_dcache_page() and
> flush_dcache_page() directly.  If the page is mapped as executable
> later, the necessary D/I-cache flush will be done in
> __sync_icache_dcache().
> 
> Signed-off-by: Simon Baatz <gmbnomis@gmail.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Russell King <linux@arm.linux.org.uk>

I would suggest you make this patch the first one rather than
introducing __flush_kernel_dcache_page() and subsequently replacing its
code.
Simon Baatz Sept. 27, 2012, 5:35 p.m. UTC | #2 
Hi Catalin,

On Thu, Sep 27, 2012 at 01:23:28PM +0100, Catalin Marinas wrote:
> On Wed, Sep 26, 2012 at 10:40:59PM +0100, Simon Baatz wrote:
> > On non-aliasing VIPT D-caches, there is no need to flush the kernel
> > mapping of anon pages in flush_kernel_dcache_page() and
> > flush_dcache_page() directly.  If the page is mapped as executable
> > later, the necessary D/I-cache flush will be done in
> > __sync_icache_dcache().
> > 
> > Signed-off-by: Simon Baatz <gmbnomis@gmail.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Russell King <linux@arm.linux.org.uk>
> 
> I would suggest you make this patch the first one rather than
> introducing __flush_kernel_dcache_page() and subsequently replacing its
> code.

The reason for this structure is that I did not want to mix the bug
fix (which may qualify for stable) with the improvement.  If I do the
patches the other way around and only the bug fix patch will be
picked for stable, flush_dcache_page() and flush_kernel_dcache_page()
will have different logic in which cases to flush.  We know that the
current logic is solid, we don't have much evidence for the logic of
the proposed improvement.

From this point of view, I find the current structure cleaner, but I
have no strong opinion about this.  If we can't apply that particular
patch for stable anyway or you still think it is better the other way
around, I can change that of course.

- Simon
diff mbox

Patch

diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
index 982db2f..077c8fd 100644
--- a/arch/arm/mm/flush.c
+++ b/arch/arm/mm/flush.c
@@ -219,15 +219,18 @@  void __flush_kernel_dcache_page(struct page *page)
 
 	mapping = page_mapping(page);
 
-	if (!cache_ops_need_broadcast() &&
-	    mapping && !mapping_mapped(mapping))
-		clear_bit(PG_dcache_clean, &page->flags);
-	else {
-		__cpuc_flush_dcache_area(page_address(page), PAGE_SIZE);
-		if (mapping && !cache_is_vivt())
-			__flush_icache_all();
-		set_bit(PG_dcache_clean, &page->flags);
+	if (!cache_ops_need_broadcast()) {
+		if ((mapping && !mapping_mapped(mapping)) ||
+		    (!mapping && cache_is_vipt_nonaliasing())) {
+			clear_bit(PG_dcache_clean, &page->flags);
+			return;
+		}
 	}
+
+	__cpuc_flush_dcache_area(page_address(page), PAGE_SIZE);
+	if (mapping && !cache_is_vivt())
+		__flush_icache_all();
+	set_bit(PG_dcache_clean, &page->flags);
 }
 EXPORT_SYMBOL(__flush_kernel_dcache_page);
 
@@ -296,16 +299,20 @@  void __sync_icache_dcache(pte_t pteval)
  * of this page.
  *
  * We have three cases to consider:
- *  - VIPT non-aliasing cache: fully coherent so nothing required.
+ *  - VIPT non-aliasing cache:
+ *          D-cache: fully coherent so nothing required.
+ *          I-cache: Ensure I/D coherency in case of an already mapped page;
+ *                   __sync_icache_dcache() will handle the other cases.
+ *  - VIPT aliasing:
+ *          D-cache: need to handle one alias in our current VM view.
+ *          I-cache: same as VIPT non-aliasing cache
  *  - VIVT: fully aliasing, so we need to handle every alias in our
  *          current VM view.
- *  - VIPT aliasing: need to handle one alias in our current VM view.
  *
- * If we need to handle aliasing:
- *  If the page only exists in the page cache and there are no user
- *  space mappings, we can be lazy and remember that we may have dirty
- *  kernel cache lines for later.  Otherwise, we assume we have
- *  aliasing mappings.
+ * If the page only exists in the page cache and there are no user
+ * space mappings, we can be lazy and remember that we may have dirty
+ * kernel cache lines for later.  Otherwise, we assume we have
+ * aliasing mappings.
  *
  * Note that we disable the lazy flush for SMP configurations where
  * the cache maintenance operations are not automatically broadcasted.
@@ -323,17 +330,20 @@  void flush_dcache_page(struct page *page)
 
 	mapping = page_mapping(page);
 
-	if (!cache_ops_need_broadcast() &&
-	    mapping && !mapping_mapped(mapping))
-		clear_bit(PG_dcache_clean, &page->flags);
-	else {
-		__flush_dcache_page(mapping, page);
-		if (mapping && cache_is_vivt())
-			__flush_dcache_aliases(mapping, page);
-		else if (mapping)
-			__flush_icache_all();
-		set_bit(PG_dcache_clean, &page->flags);
+	if (!cache_ops_need_broadcast()) {
+		if ((mapping && !mapping_mapped(mapping)) ||
+		    (!mapping && cache_is_vipt_nonaliasing())) {
+			clear_bit(PG_dcache_clean, &page->flags);
+			return;
+		}
 	}
+
+	__flush_dcache_page(mapping, page);
+	if (mapping && cache_is_vivt())
+		__flush_dcache_aliases(mapping, page);
+	else if (mapping)
+		__flush_icache_all();
+	set_bit(PG_dcache_clean, &page->flags);
 }
 EXPORT_SYMBOL(flush_dcache_page);