fix and improvement of flush(_kernel)_dcache_page()
cover fix and improvement of flush(_kernel)_dcache_page()
Hi,
another attempt to fix the regression caused by patch f8b63c1 "ARM:
6382/1" in 2.6.37 (see [2]). The problem occurs on VIVT (and probably VIPT aliasing) cache architectures for drivers that use flush_kernel_dcache_page(). For example, 3.6-rc6 (as any kernel beginning with 2.6.37) on kirkwood using mv_cesa produces data corruption when using direct I/O:
~# cryptsetup luksOpen /dev/sda2 c_sda2
Enter passphrase for /dev/sda2:
~# dd if=/dev/mapper/c_sda2 iflag=direct bs=4k count=16 2>/dev/null | sha1sum 9ae4997ec85ad9b7ab4b10341e42ace80f0ea5d6 -
~# dd if=/dev/mapper/c_sda2 bs=4k count=16 2>/dev/null | sha1sum ca39c5d4950b3704eff952c48e383bf1db20532e -
(The mv_cesa driver uses the scatterlist memory iterator, which uses flush_kernel_dcache_page() before kunmap()).
As described in [1], both flush_dcache_page() and flush_kernel_dcache_page() need to handle the following cases:
- page cache pages with no user space mapping
- page cache pages with user space mapping(s)
- flush the kernel mapping of anonymous pages
The last one is contradictory to documentation for flush_dcache_page(), but many uses in the kernel require it (drivers use it to flush modifications via the kernel mapping).
In the proposed patch the implementation of flush_kernel_dcache_page() follows the one of flush_dcache_page(), but only flushes the kernel mapping, not the user mapping(s).
The second patch optimizes these two functions for non-aliasing VIPT caches. On these the flush of the kernel mapping for the anon case is actually not needed. It may be needed to ensure I/D coherency, but this can be handled by __sync_icache_dcache() later and more effectively (similar to arm64, see [3]). Thus, we should be lazy as well here and just clear the PG_dcache_clean bit.
I could only test this on kirkwood (ARMv5, VIVT) and raspberry pi BCM2835 (ARMv6, PIPT / VIPT nonaliasing data cache). More testing would probably be a good thing... Especially, I could not test the non-aliasing cache case with a real driver using flush_kernel_dcache_page() (mv_cesa on dove could be a test case for this).
- Simon
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2012-August/113908.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2012-May/101795.html
[3] https://lkml.org/lkml/2012/9/12/121
Simon Baatz (2):
ARM: Handle user space mapped pages in flush_kernel_dcache_page
ARM: remove unnecessary flush of anon pages in
flush(_kernel)_dcache_page()
arch/arm/include/asm/cacheflush.h | 4 ++ arch/arm/mm/flush.c | 83 +++++++++++++++++++++++++++++-------- 2 files changed, 70 insertions(+), 17 deletions(-)
Signature · 1 line
1.7.9.5
PATCH v2 1/2 ARM: Handle user space mapped pages in flush_kernel_dcache_page
Commit f8b63c1 made flush_kernel_dcache_page() a no-op assuming that the pages it needs to handle are kernel mapped only. However, for example when doing direct I/O, pages with user space mappings may occur.
Thus, do lazy flushing like in flush_dcache_page() if there are no user space mappings. Otherwise, flush the kernel cache lines directly.
---
Changes in v2:
- flush_kernel_dcache_page() follows flush_dcache_page() now, except that it does not flush the user mappings
arch/arm/include/asm/cacheflush.h | 4 ++++ arch/arm/mm/flush.c | 39 +++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+)
arch/arm/include/asm/cacheflush.h +4 -0
@@ -307,6 +307,10 @@ static inline void flush_anon_page(struct vm_area_struct *vma,#define ARCH_HAS_FLUSH_KERNEL_DCACHE_PAGE static inline void flush_kernel_dcache_page (struct page *page) { +extern void __flush_kernel_dcache_page (struct page *); +/* highmem pages are always flushed upon kunmap already */ +if (!PageHighMem (page)) +__flush_kernel_dcache_page (page); }#define flush_dcache_mmap_lock (mapping) \
arch/arm/mm/flush.c +39 -0
@@ -192,6 +192,45 @@ void __flush_dcache_page(struct address_space *mapping, struct page *page) page->index << PAGE_CACHE_SHIFT); } +/* +* Ensure cache coherency for the kernel mapping of this page. +* +* 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 need to flush the +* dirty kernel cache lines directly. +* +* Note that we disable the lazy flush for SMP configurations where +* the cache maintenance operations are not automatically broadcasted. +* +* We can assume that the page is no high mem page, see +* flush_kernel_dcache_page. +*/ +void __flush_kernel_dcache_page (struct page *page) +{ +struct address_space *mapping; + +/* +* The zero page is never written to, so never has any dirty +* cache lines, and therefore never needs to be flushed. +*/ +if (page ==ZERO_PAGE (0 )) +return ; + + 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); + } +} +EXPORT_SYMBOL (__flush_kernel_dcache_page); +static void __flush_dcache_aliases (struct address_space *mapping,struct page *page) {struct mm_struct *mm = current->active_mm;
Signature · 1 line
1.7.9.5
PATCH 2/2 ARM: remove unnecessary flush of anon pages in flush(_kernel)_dcache_page()
In reply to Simon BaatzOn 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().
--- arch/arm/mm/flush.c | 60 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 35 insertions(+), 25 deletions(-)
arch/arm/mm/flush.c +35 -25
@@ -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);
Signature · 1 line
1.7.9.5
PATCH 2/2 ARM: remove unnecessary flush of anon pages in flush(_kernel)_dcache_page()
Quoted 9 lines · Simon Baatz
On non-aliasing VIPT D-caches, there is no need to flush the kernel mapping of anon pages in
flush_kernel_dcache_page()andflush_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 <[email protected]>Cc Catalin Marinas <[email protected]>Cc Russell King <[email protected]>
I would suggest you make this patch the first one rather than introducing __flush_kernel_dcache_page() and subsequently replacing its code.
Signature · 1 line
Catalin
PATCH 2/2 ARM: remove unnecessary flush of anon pages in flush(_kernel)_dcache_page()
Hi Catalin,
Quoted 7 lines · Catalin Marinas
Quoted 6 lines · Simon Baatz
On non-aliasing VIPT D-caches, there is no need to flush the kernel mapping of anon pages in
flush_kernel_dcache_page()andflush_dcache_page()directly. If the page is mapped as executable later, the necessary D/I-cache flush will be done in__sync_icache_dcache().
Quoted 7 lines
Quoted 3 lines
Signed-off-by Simon Baatz <[email protected]>Cc Catalin Marinas <[email protected]>Cc Russell King <[email protected]>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.
Quoted 1 line
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