aio: fix D-cache aliasing issues

PATCH In review Helge Deller 5 participants 17 messages Nov 15, 2013 – Nov 18, 2013

Subsystems (3)

VFS PA-RISC architecture Aio

lore

Share & tools

lore

Helge Deller

author maintainer

2013-11-15 22:05 UTC #0 lore raw

PATCH 1/1 aio: fix D-cache aliasing issues

When a user page mapping is released via kunmap*() functions, the D-cache needs to be flushed via flush_dcache_page() to avoid D-cache aliasing issues.

This patch fixes aio on the parisc platform (and probably others).

Signed-off-by Helge Deller <[email protected]>

To: Benjamin LaHaise <[email protected]>
To: [email protected]
Cc: [email protected] # 3.12+

fs/aio.c +3 -0

index 823efcb..2181821 100644
@@ -514,6 +514,7 @@ static void free_ioctx(struct work_struct *work)
 		atomic_add(avail, &ctx->reqs_available);
 		ring->head = ring->tail;
 		kunmap_atomic(ring);
+		flush_dcache_page(ctx->ring_pages[0]);
 
 		if (atomic_read(&ctx->reqs_available) >= ctx->nr_events - 1)
 			break;

@@ -568,6 +569,7 @@ static int ioctx_add_table(struct kioctx *ctx, struct mm_struct *mm)
 					ring = kmap_atomic(ctx->ring_pages[0]);
 					ring->id = ctx->id;
 					kunmap_atomic(ring);
+					flush_dcache_page(ctx->ring_pages[0]);
 					return 0;
 				}

@@ -1032,6 +1034,7 @@ static long aio_read_events_ring(struct kioctx *ctx,
 	head = ring->head;
 	tail = ring->tail;
 	kunmap_atomic(ring);
+	flush_dcache_page(ctx->ring_pages[0]);
 
 	pr_debug("h%u t%u m%u\n", head, tail, ctx->nr_events);

James Bottomley

maintainer

2013-11-15 22:42 UTC #1 lore raw

Quoted 4 lines · Helge Deller

When a user page mapping is released via kunmap*() functions, the D-cache needs to be flushed via flush_dcache_page() to avoid D-cache aliasing issues.
This patch fixes aio on the parisc platform (and probably others).

This should be flush_kernel_dcache_page(). flush_dcache_page() is for full coherency but for unmap, we know the page was coherent going in and may have been modified by the kernel, so only the kernel view needs to be sync'd. Technically, by the kernel API, the flush should be done *before* unmapping. This would have mattered on parisc until we did flush via tmpalias which means we no-longer care if the mapping for the flush exists or not because we always recreate it via the tmpalias pages.

James

Quoted 38 lines

Signed-off-by Helge Deller <[email protected]>

To: Benjamin LaHaise <[email protected]>
To: [email protected]
Cc: [email protected] # 3.12+

fs/aio.c +3 -1

index 823efcb..2181821 100644
@@ -514,6 +514,7 @@ static void free_ioctx(struct work_struct *work)
 		atomic_add(avail, &ctx->reqs_available);
 		ring->head = ring->tail;
 		kunmap_atomic(ring);
+		flush_dcache_page(ctx->ring_pages[0]);
 
 		if (atomic_read(&ctx->reqs_available) >= ctx->nr_events - 1)
 			break;

@@ -568,6 +569,7 @@ static int ioctx_add_table(struct kioctx *ctx, struct mm_struct *mm)
 					ring = kmap_atomic(ctx->ring_pages[0]);
 					ring->id = ctx->id;
 					kunmap_atomic(ring);
+					flush_dcache_page(ctx->ring_pages[0]);
 					return 0;
 				}

@@ -1032,6 +1034,7 @@ static long aio_read_events_ring(struct kioctx *ctx,
 	head = ring->head;
 	tail = ring->tail;
 	kunmap_atomic(ring);
+	flush_dcache_page(ctx->ring_pages[0]);
 
 	pr_debug("h%u t%u m%u\n", head, tail, ctx->nr_events);
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Simon Baatz

2013-11-16 20:07 UTC #2 lore raw

Quoted 6 lines · James Bottomley

Quoted 4 lines · Helge Deller
When a user page mapping is released via kunmap*() functions, the D-cache needs to be flushed via flush_dcache_page() to avoid D-cache aliasing issues.
This patch fixes aio on the parisc platform (and probably others).

Quoted 8 lines

This should be flush_kernel_dcache_page(). flush_dcache_page() is for full coherency but for unmap, we know the page was coherent going in and may have been modified by the kernel, so only the kernel view needs to be sync'd. Technically, by the kernel API, the flush should be done *before* unmapping. This would have mattered on parisc until we did flush via tmpalias which means we no-longer care if the mapping for the flush exists or not because we always recreate it via the tmpalias pages.

On ARM, flush_kernel_dcache_page() actually assumes that the page is mapped. It avoids double flushing of highmem pages by not flushing in those cases where kunmap_atomic() already takes care of flushing.

- Simon

Benjamin LaHaise

maintainer

2013-11-16 20:09 UTC #3 lore raw

Quoted 16 lines · Simon Baatz

Quoted 6 lines · James Bottomley
Quoted 4 lines · Helge Deller
When a user page mapping is released via kunmap*() functions, the D-cache needs to be flushed via flush_dcache_page() to avoid D-cache aliasing issues.
This patch fixes aio on the parisc platform (and probably others).
Quoted 8 lines
This should be flush_kernel_dcache_page(). flush_dcache_page() is for full coherency but for unmap, we know the page was coherent going in and may have been modified by the kernel, so only the kernel view needs to be sync'd. Technically, by the kernel API, the flush should be done *before* unmapping. This would have mattered on parisc until we did flush via tmpalias which means we no-longer care if the mapping for the flush exists or not because we always recreate it via the tmpalias pages.

Quoted 3 lines

On ARM, flush_kernel_dcache_page() actually assumes that the page is mapped. It avoids double flushing of highmem pages by not flushing in those cases where kunmap_atomic() already takes care of flushing.

Helge -- are you going to resubmit a version of this patch that makes the recommended change?

-ben

Quoted 3 lines

- Simon

Signature · 7 lines

"Thought is the essence of where you are now."

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to [email protected].  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"[email protected]">[email protected]</a>

Helge Deller

author maintainer

2013-11-17 21:23 UTC #4 lore raw

Quoted 17 lines · Benjamin LaHaise

Quoted 16 lines · Simon Baatz
Quoted 14 lines · James Bottomley
Quoted 4 lines · Helge Deller
When a user page mapping is released via kunmap*() functions, the D-cache needs to be flushed via flush_dcache_page() to avoid D-cache aliasing issues.
This patch fixes aio on the parisc platform (and probably others).
This should be flush_kernel_dcache_page(). flush_dcache_page() is for full coherency but for unmap, we know the page was coherent going in and may have been modified by the kernel, so only the kernel view needs to be sync'd. Technically, by the kernel API, the flush should be done *before* unmapping. This would have mattered on parisc until we did flush via tmpalias which means we no-longer care if the mapping for the flush exists or not because we always recreate it via the tmpalias pages.

Quoted 6 lines

Quoted 3 lines
On ARM, flush_kernel_dcache_page() actually assumes that the page is mapped. It avoids double flushing of highmem pages by not flushing in those cases where kunmap_atomic() already takes care of flushing.
Helge -- are you going to resubmit a version of this patch that makes the recommended change?

Sure, I'll do. May need some time for testing the various machine types though. Maybe in the end you can drop my patch since we might be able to fix it in the parisc arch code.

@James, Dave: The patch I sent here was not used on the C8000. The C8000 (PA8800) works out of the box (and it uses the flush_kernel_dcache_page() function which is provided by kunmap()). It seems we require the flush_kernel_dcache_page() on the other machines too?

Helge

James Bottomley

maintainer

2013-11-17 21:49 UTC #5 lore raw

Quoted 25 lines · Helge Deller

Quoted 17 lines · Benjamin LaHaise
Quoted 16 lines · Simon Baatz
Quoted 14 lines · James Bottomley
Quoted 4 lines · Helge Deller
When a user page mapping is released via kunmap*() functions, the D-cache needs to be flushed via flush_dcache_page() to avoid D-cache aliasing issues.
This patch fixes aio on the parisc platform (and probably others).
This should be flush_kernel_dcache_page(). flush_dcache_page() is for full coherency but for unmap, we know the page was coherent going in and may have been modified by the kernel, so only the kernel view needs to be sync'd. Technically, by the kernel API, the flush should be done *before* unmapping. This would have mattered on parisc until we did flush via tmpalias which means we no-longer care if the mapping for the flush exists or not because we always recreate it via the tmpalias pages.
Quoted 6 lines
Quoted 3 lines
On ARM, flush_kernel_dcache_page() actually assumes that the page is mapped. It avoids double flushing of highmem pages by not flushing in those cases where kunmap_atomic() already takes care of flushing.
Helge -- are you going to resubmit a version of this patch that makes the recommended change?

Quoted 8 lines

Sure, I'll do. May need some time for testing the various machine types though. Maybe in the end you can drop my patch since we might be able to fix it in the parisc arch code.
@James, Dave: The patch I sent here was not used on the C8000. The C8000 (PA8800) works out of the box (and it uses the flush_kernel_dcache_page() function which is provided by kunmap()). It seems we require the flush_kernel_dcache_page() on the other machines too?

Well, this is why I think our in-kernel API (not the parisc one) for kmap/kunmap is faulty. I can't see any valid use of kmap/kunmap where you don't need to flush. Either you kmapped for reading, in which case you need to flush user space before the kernel access, or you kmapped for writing, in which case you need to flush user space just before mapping (eliminate any dirty user line) and the kernel cache before unmap (so the modification becomes visible in main memory). I could see an argument that flushing all the user mappings is expensive so only do it if necessary, but I think it looks to be always necessary: even if you're writing only, which means purging the lines above the user mappings, most hardware will panic (HPMC) if it sees inequivalent aliases with clashing dirty lines.

I think the reason for the complexity is issues on ARM. The user space cache lines have to remain empty until the kunmap. On parisc we can ensure this by purging the tlb entries to prevent speculative move in (if the user deliberately accesses while we're modifying, it's caveat emptor), but I don't believe ARM can control this speculation, so they always need to re-invalidate all of user space before unmap just to eliminate speculated lines ... in any case, that could still be done as part of the kunmap API.

James

Helge Deller

author maintainer

2013-11-17 23:47 UTC #6 lore raw

* James Bottomley <[email protected]>:

Quoted 30 lines · Helge Deller

Quoted 29 lines · Benjamin LaHaise
Quoted 17 lines · Simon Baatz
Quoted 16 lines · James Bottomley
Quoted 14 lines · Helge Deller
Quoted 4 lines
When a user page mapping is released via kunmap*() functions, the D-cache needs to be flushed via flush_dcache_page() to avoid D-cache aliasing issues.
This patch fixes aio on the parisc platform (and probably others).
This should be flush_kernel_dcache_page(). flush_dcache_page() is for full coherency but for unmap, we know the page was coherent going in and may have been modified by the kernel, so only the kernel view needs to be sync'd. Technically, by the kernel API, the flush should be done *before* unmapping. This would have mattered on parisc until we did flush via tmpalias which means we no-longer care if the mapping for the flush exists or not because we always recreate it via the tmpalias pages.
Quoted 6 lines
Quoted 3 lines
On ARM, flush_kernel_dcache_page() actually assumes that the page is mapped. It avoids double flushing of highmem pages by not flushing in those cases where kunmap_atomic() already takes care of flushing.
Helge -- are you going to resubmit a version of this patch that makes the recommended change?
Sure, I'll do. May need some time for testing the various machine types though. Maybe in the end you can drop my patch since we might be able to fix it in the parisc arch code.

Quoted 8 lines

Quoted 4 lines
@James, Dave: The patch I sent here was not used on the C8000. The C8000 (PA8800) works out of the box (and it uses the flush_kernel_dcache_page() function which is provided by kunmap()). It seems we require the flush_kernel_dcache_page() on the other machines too?
Well, this is why I think our in-kernel API (not the parisc one) for kmap/kunmap is faulty. I can't see any valid use of kmap/kunmap where you don't need to flush.

Maybe adding a flag to kmap(), e.g. READ_ONLY, WRITE_ONLY or similiar?

Quoted 5 lines

Either you kmapped for reading, in which case
you need to flush user space before the kernel access, or you kmapped for writing, in which case you need to flush user space just before mapping (eliminate any dirty user line) and the kernel cache before unmap (so the modification becomes visible in main memory).

That's exactly what Dave told me in a private mail earlier.

Below is a patch to the parisc arch code which is originally from Dave. I only changed kunmap_parisc() to always call flush_kernel_dcache_page_addr(addr) - independend if we are on a PA8800/8900 CPU or not - which is what you proposed in your original mail. And yes, this flush_kernel_dcache_page() fixes the aio problems on all my machines (32- and 64bit kernels, PA7300LC, PA8700, ...).

Quoted 6 lines

I could see
an argument that flushing all the user mappings is expensive so only do it if necessary, but I think it looks to be always necessary: even if you're writing only, which means purging the lines above the user mappings, most hardware will panic (HPMC) if it sees inequivalent aliases with clashing dirty lines.

arch/parisc/include/asm/cacheflush.h +16 -5

index f0e2784..ce6d605 100644
@@ -43,6 +43,8 @@ static inline void flush_kernel_dcache_page(struct page *page)
 	flush_kernel_dcache_page_addr(page_address(page));
 }
 
+void flush_user_dcache_page(struct page *page);
+
 #define flush_kernel_dcache_range(start,size) \
 	flush_kernel_dcache_range_asm((start), (start)+(size));
 /* vmap range flushes and invalidates.  Architecturally, we don't need

@@ -125,18 +127,27 @@ flush_anon_page(struct vm_area_struct *vma, struct page *page, unsigned long vma
 void mark_rodata_ro(void);
 #endif
 
-#ifdef CONFIG_PA8X00
-/* Only pa8800, pa8900 needs this */
-
 #include <asm/kmap_types.h>
 
 #define ARCH_HAS_KMAP
 
-void kunmap_parisc(void *addr);
+static inline void kmap_parisc(struct page *page)
+{
+	if (parisc_requires_coherency())
+		flush_user_dcache_page(page);
+}
+
+static inline void kunmap_parisc(void *addr)
+{
+	/* always flush kernel dcache, independed of
+	 * parisc_requires_coherency(). Needed on CPUs < PA8800/PA8900 */
+	flush_kernel_dcache_page_addr(addr);
+}
 
 static inline void *kmap(struct page *page)
 {
 	might_sleep();
+	kmap_parisc(page);
 	return page_address(page);
 }

@@ -148,6 +159,7 @@ static inline void kunmap(struct page *page)
 static inline void *kmap_atomic(struct page *page)
 {
 	pagefault_disable();
+	kmap_parisc(page);
 	return page_address(page);
 }

@@ -160,7 +172,6 @@ static inline void __kunmap_atomic(void *addr)
 #define kmap_atomic_prot(page, prot)	kmap_atomic(page)
 #define kmap_atomic_pfn(pfn)	kmap_atomic(pfn_to_page(pfn))
 #define kmap_atomic_to_page(ptr)	virt_to_page(ptr)
-#endif
 
 #endif /* _PARISC_CACHEFLUSH_H */

arch/parisc/kernel/cache.c +15 ~~-17~~

index c035673..a7ee7b7 100644
@@ -283,7 +283,7 @@ __flush_cache_page(struct vm_area_struct *vma, unsigned long vmaddr,
 	preempt_enable();
 }
 
-void flush_dcache_page(struct page *page)
+void flush_user_dcache_page(struct page *page)
 {
 	struct address_space *mapping = page_mapping(page);
 	struct vm_area_struct *mpnt;

@@ -291,13 +291,6 @@ void flush_dcache_page(struct page *page)
 	unsigned long addr, old_addr = 0;
 	pgoff_t pgoff;
 
-	if (mapping && !mapping_mapped(mapping)) {
-		set_bit(PG_dcache_dirty, &page->flags);
-		return;
-	}
-
-	flush_kernel_dcache_page(page);
-
 	if (!mapping)
 		return;

@@ -332,6 +325,20 @@ void flush_dcache_page(struct page *page)
 	}
 	flush_dcache_mmap_unlock(mapping);
 }
+EXPORT_SYMBOL(flush_user_dcache_page);
+
+void flush_dcache_page(struct page *page)
+{
+	struct address_space *mapping = page_mapping(page);
+
+	if (mapping && !mapping_mapped(mapping)) {
+		set_bit(PG_dcache_dirty, &page->flags);
+		return;
+	}
+
+	flush_kernel_dcache_page(page);
+	flush_user_dcache_page(page);
+}
 EXPORT_SYMBOL(flush_dcache_page);
 
 /* Defined in arch/parisc/kernel/pacache.S */

@@ -413,16 +420,6 @@ void copy_user_page(void *vto, void *vfrom, unsigned long vaddr,
 }
 EXPORT_SYMBOL(copy_user_page);
 
-#ifdef CONFIG_PA8X00
-
-void kunmap_parisc(void *addr)
-{
-	if (parisc_requires_coherency())
-		flush_kernel_dcache_page_addr(addr);
-}
-EXPORT_SYMBOL(kunmap_parisc);
-#endif

James Bottomley

maintainer

2013-11-18 00:52 UTC #7 lore raw

Quoted 72 lines · Helge Deller

* James Bottomley <[email protected]>:
Quoted 30 lines · Helge Deller
Quoted 29 lines · Benjamin LaHaise
Quoted 17 lines · Simon Baatz
Quoted 16 lines · James Bottomley
Quoted 14 lines · Helge Deller
Quoted 4 lines
When a user page mapping is released via kunmap*() functions, the D-cache needs to be flushed via flush_dcache_page() to avoid D-cache aliasing issues.
This patch fixes aio on the parisc platform (and probably others).
This should be flush_kernel_dcache_page(). flush_dcache_page() is for full coherency but for unmap, we know the page was coherent going in and may have been modified by the kernel, so only the kernel view needs to be sync'd. Technically, by the kernel API, the flush should be done *before* unmapping. This would have mattered on parisc until we did flush via tmpalias which means we no-longer care if the mapping for the flush exists or not because we always recreate it via the tmpalias pages.
Quoted 6 lines
Quoted 3 lines
On ARM, flush_kernel_dcache_page() actually assumes that the page is mapped. It avoids double flushing of highmem pages by not flushing in those cases where kunmap_atomic() already takes care of flushing.
Helge -- are you going to resubmit a version of this patch that makes the recommended change?
Sure, I'll do. May need some time for testing the various machine types though. Maybe in the end you can drop my patch since we might be able to fix it in the parisc arch code.
Quoted 8 lines
Quoted 4 lines
@James, Dave: The patch I sent here was not used on the C8000. The C8000 (PA8800) works out of the box (and it uses the flush_kernel_dcache_page() function which is provided by kunmap()). It seems we require the flush_kernel_dcache_page() on the other machines too?
Well, this is why I think our in-kernel API (not the parisc one) for kmap/kunmap is faulty. I can't see any valid use of kmap/kunmap where you don't need to flush.
Maybe adding a flag to kmap(), e.g. READ_ONLY, WRITE_ONLY or similiar?
Quoted 5 lines
Either you kmapped for reading, in which case
you need to flush user space before the kernel access, or you kmapped for writing, in which case you need to flush user space just before mapping (eliminate any dirty user line) and the kernel cache before unmap (so the modification becomes visible in main memory).
That's exactly what Dave told me in a private mail earlier.
Below is a patch to the parisc arch code which is originally from Dave. I only changed kunmap_parisc() to always call flush_kernel_dcache_page_addr(addr) - independend if we are on a PA8800/8900 CPU or not - which is what you proposed in your original mail. And yes, this flush_kernel_dcache_page() fixes the aio problems on all my machines (32- and 64bit kernels, PA7300LC, PA8700, ...).
Quoted 6 lines
I could see
an argument that flushing all the user mappings is expensive so only do it if necessary, but I think it looks to be always necessary: even if you're writing only, which means purging the lines above the user mappings, most hardware will panic (HPMC) if it sees inequivalent aliases with clashing dirty lines.
arch/parisc/include/asm/cacheflush.h +0 -0
index f0e2784..ce6d605 100644

arch/parisc/include/asm/cacheflush.h static inline void flush_kernel_dcache_page(struct page *page) · line 47

Quoted 5 lines

arch/parisc/include/asm/cacheflush.h +2 -0

@@ -43,6 +43,8 @@ static inline void flush_kernel_dcache_page(struct page *page)
 	flush_kernel_dcache_page_addr(page_address(page));
 }
 
+void flush_user_dcache_page(struct page *page);
+

The split into flush_kernel.. and flush_user.. is pointless. We have no use for a flush_user_.. API because it's not part of the standard linux one. Plus it's going to confuse those who come after us no end because now we're different from every other architecture.

Quoted 10 lines

arch/parisc/include/asm/cacheflush.h +0 -3

 #define flush_kernel_dcache_range(start,size) \
 	flush_kernel_dcache_range_asm((start), (start)+(size));
 /* vmap range flushes and invalidates.  Architecturally, we don't need

@@ -125,18 +127,27 @@ flush_anon_page(struct vm_area_struct *vma, struct page *page, unsigned long vma
 void mark_rodata_ro(void);
 #endif
 
-#ifdef CONFIG_PA8X00
-/* Only pa8800, pa8900 needs this */
-

arch/parisc/include/asm/cacheflush.h flush_anon_page(struct vm_area_struct *vma, struct page *page, unsigned long vma · line 138

Quoted 10 lines

arch/parisc/include/asm/cacheflush.h +5 -1

 #include <asm/kmap_types.h>
 
 #define ARCH_HAS_KMAP
 
-void kunmap_parisc(void *addr);
+static inline void kmap_parisc(struct page *page)
+{
+	if (parisc_requires_coherency())
+		flush_user_dcache_page(page);
+}

No ... if we're genuinely moving coherency into kmap/kunmap, this has to become

	flush_dcache_page(page);

unconditionally.

James

John David Anglin

2013-11-18 01:15 UTC #8 lore raw

Quoted 7 lines · James Bottomley

arch/parisc/include/asm/cacheflush.h static inline void flush_kernel_dcache_page(struct page *page) · line 47
Quoted 2 lines
arch/parisc/include/asm/cacheflush.h +2 -0
+void flush_user_dcache_page(struct page *page);
+
The split into flush_kernel.. and flush_user.. is pointless. We have no use for a flush_user_.. API because it's not part of the standard linux one. Plus it's going to confuse those who come after us no end because now we're different from every other architecture.

I introduced it because it seemed to be what was needed on parisc when we map a kernel page on PA8800/PA8900. We need this to ensure what the kernel sees is up to date and also to ensure that we don't create inequivalent aliases between the user and kernel mappings.

Quoted 26 lines

Quoted 16 lines
#define flush_kernel_dcache_range(start,size) \
	flush_kernel_dcache_range_asm((start), (start)+(size));
/* vmap range flushes and invalidates. Architecturally, we don't need
(quoted diff) +0 -0
@@ -125,18 +127,27 @@ flush_anon_page(struct vm_area_struct *vma,  
struct page *page, unsigned long vma
void mark_rodata_ro(void);
#endif
arch/parisc/include/asm/cacheflush.h +0 -3
-#ifdef CONFIG_PA8X00
-/* Only pa8800, pa8900 needs this */
-
#include <asm/kmap_types.h>
#define ARCH_HAS_KMAP
arch/parisc/include/asm/cacheflush.h flush_anon_page(struct vm_area_struct *vma, struct page *page, unsigned long vma · line 138
Quoted 6 lines
arch/parisc/include/asm/cacheflush.h +5 -1
-void kunmap_parisc(void *addr);
+static inline void kmap_parisc(struct page *page)
+{
+	if (parisc_requires_coherency())
+		flush_user_dcache_page(page);
+}
No ... if we're genuinely moving coherency into kmap/kunmap, this has to become

Quoted 3 lines

	flush_dcache_page(page);
unconditionally.

There are two things about flush_dcache_page(page):

1) The flush could be deferred. I don't think we want that.
2) It seems to unnecessarily call flush_kernel_dcache_page().

Dave
--
John David Anglin [email protected]

James Bottomley

maintainer

2013-11-18 01:50 UTC #9 lore raw

Quoted 47 lines · John David Anglin

Quoted 10 lines · James Bottomley
arch/parisc/include/asm/cacheflush.h static inline void flush_kernel_dcache_page(struct page *page) · line 47
Quoted 2 lines
arch/parisc/include/asm/cacheflush.h +2 -0
+void flush_user_dcache_page(struct page *page);
+
The split into flush_kernel.. and flush_user.. is pointless. We have no
use for a flush_user_.. API because it's not part of the standard linux
one. Plus it's going to confuse those who come after us no end because
now we're different from every other architecture.
I introduced it because it seemed to be what was needed on parisc when we map a kernel page on PA8800/PA8900. We need this to ensure what the kernel sees is up to date and also to ensure that we don't create inequivalent
aliases between the user and kernel mappings.
Quoted 28 lines
Quoted 17 lines
#define flush_kernel_dcache_range(start,size) \
	flush_kernel_dcache_range_asm((start), (start)+(size));
/* vmap range flushes and invalidates. Architecturally, we don't need
(quoted diff) +0 -0
@@ -125,18 +127,27 @@ flush_anon_page(struct vm_area_struct *vma,  
struct page *page, unsigned long vma
void mark_rodata_ro(void);
#endif
arch/parisc/include/asm/cacheflush.h +0 -3
-#ifdef CONFIG_PA8X00
-/* Only pa8800, pa8900 needs this */
-
#include <asm/kmap_types.h>
#define ARCH_HAS_KMAP
arch/parisc/include/asm/cacheflush.h flush_anon_page(struct vm_area_struct *vma, struct page *page, unsigned long vma · line 138
Quoted 6 lines
arch/parisc/include/asm/cacheflush.h +5 -1
-void kunmap_parisc(void *addr);
+static inline void kmap_parisc(struct page *page)
+{
+	if (parisc_requires_coherency())
+		flush_user_dcache_page(page);
+}
No ... if we're genuinely moving coherency into kmap/kunmap, this has to
become

Quoted 7 lines

Quoted 3 lines
	flush_dcache_page(page);
unconditionally.
There are two things about flush_dcache_page(page):
1) The flush could be deferred. I don't think we want that.

Our deferred flush logic seems a bit bogus to me. We set a flag that causes a kernel page flush in invalidate_kernel_vmap_range(). However, when that function is called we checked this on a page by page bases and if set we flush the page then we then go and do a dcache flush on the entire range ... that's got to be wrong. I also can't really see what the check for mapping && !mapping_mapped() is about. Mapping != NULL means shared mappings with user space !mapping_mapped() means no file backing (so they must be some form of anonymous vma). Even if we got the deferred logic right, I don't think we can avoid flushing in the anon_vma case

Quoted 1 line

2) It seems to unnecessarily call flush_kernel_dcache_page().

Remember flush on parisc is flush and invalidate. We do need to invalidate the kernel page in flush_dcache_page() just in case there are any read only speculated cache lines above it. I agree there shouldn't be any dirty cache lines above it in kernel space.

James

John David Anglin

2013-11-18 01:03 UTC #10 lore raw

In reply to Helge Deller

Quoted 3 lines · Helge Deller

I only changed kunmap_parisc() to always call flush_kernel_dcache_page_addr(addr) - independend if we are on a PA8800/8900 CPU or not - which is what you proposed in your original mail.

In which case the flushes in clear_user_page() and copy_user_page() aren't needed.

Dave
--
John David Anglin [email protected]

Simon Baatz

2013-11-18 10:54 UTC #11 lore raw

In reply to Helge Deller

Hi Helge,

Quoted 22 lines · Helge Deller

Quoted 21 lines · Benjamin LaHaise
Quoted 19 lines · Simon Baatz
Quoted 6 lines · James Bottomley
Quoted 4 lines · Helge Deller
When a user page mapping is released via kunmap*() functions, the D-cache needs to be flushed via flush_dcache_page() to avoid D-cache aliasing issues.
This patch fixes aio on the parisc platform (and probably others).
Quoted 8 lines
This should be flush_kernel_dcache_page(). flush_dcache_page() is for full coherency but for unmap, we know the page was coherent going in and may have been modified by the kernel, so only the kernel view needs to be sync'd. Technically, by the kernel API, the flush should be done *before* unmapping. This would have mattered on parisc until we did flush via tmpalias which means we no-longer care if the mapping for the flush exists or not because we always recreate it via the tmpalias pages.
On ARM, flush_kernel_dcache_page() actually assumes that the page is mapped. It avoids double flushing of highmem pages by not flushing in those cases where kunmap_atomic() already takes care of flushing.

Quoted 6 lines

Quoted 2 lines
Helge -- are you going to resubmit a version of this patch that makes the recommended change?
Sure, I'll do. May need some time for testing the various machine types though. Maybe in the end you can drop my patch since we might be able to fix it in the parisc arch code.

Could you provide me with the test case(s) you are running to reproduce the problem? I could test this on aliasing D-cache on ARM as well. Since kmap/kunmap do not flush in general on ARM, we might need the explicit flushes here.

- Simon

Helge Deller

author maintainer

2013-11-18 11:24 UTC #12 lore raw

Quoted 5 lines

Hi Helge,
Could you provide me with the test case(s) you are running to reproduce the problem? I could test this on aliasing D-cache on ARM as well. Since kmap/kunmap do not flush in general on ARM, we might need the explicit flushes here.

Just google for aio-stress.c (e.g. https://github.com/repoforge/rpms/blob/master/specs/aio-stress/aio-stress.c).
And the aio01 / aio02 testcases from the Linux Test Project (https://github.com/linux-test-project/ltp).

Helge

James Bottomley

maintainer

2013-11-16 22:06 UTC #13 lore raw

In reply to Simon Baatz

Quoted 16 lines · Simon Baatz

Quoted 6 lines · James Bottomley
Quoted 4 lines · Helge Deller
When a user page mapping is released via kunmap*() functions, the D-cache needs to be flushed via flush_dcache_page() to avoid D-cache aliasing issues.
This patch fixes aio on the parisc platform (and probably others).
Quoted 8 lines
This should be flush_kernel_dcache_page(). flush_dcache_page() is for full coherency but for unmap, we know the page was coherent going in and may have been modified by the kernel, so only the kernel view needs to be sync'd. Technically, by the kernel API, the flush should be done *before* unmapping. This would have mattered on parisc until we did flush via tmpalias which means we no-longer care if the mapping for the flush exists or not because we always recreate it via the tmpalias pages.

Quoted 3 lines

On ARM, flush_kernel_dcache_page() actually assumes that the page is mapped. It avoids double flushing of highmem pages by not flushing in those cases where kunmap_atomic() already takes care of flushing.

On Parisc, kmap/kunmap is currently a nop. However, if we ever implemented highmem, we would also need to flush before we unmap, which is why the flush needs to go before the kunmap.

I've got to say on all of this, we've implemented the most inane set of primitives. It would be much easier if kmap/kunmap just took care of all of this. kmap should bring userspace into coherence (because why else would we be kmapping) and kunmap should force coherence on the kernel address ... and then no-one would need to worry when to and when not to flush.

James

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in the body to [email protected]. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"[email protected]">[email protected]</a>

John David Anglin

2013-11-16 22:32 UTC #14 lore raw

Quoted 17 lines · James Bottomley

Quoted 16 lines · Simon Baatz
Quoted 14 lines · James Bottomley
Quoted 4 lines · Helge Deller
When a user page mapping is released via kunmap*() functions, the D-cache needs to be flushed via flush_dcache_page() to avoid D-cache aliasing issues.
This patch fixes aio on the parisc platform (and probably others).
This should be flush_kernel_dcache_page(). flush_dcache_page() is for full coherency but for unmap, we know the page was coherent going in and may have been modified by the kernel, so only the kernel view needs to be sync'd. Technically, by the kernel API, the flush should be done *before* unmapping. This would have mattered on parisc until we did flush via tmpalias which means we no-longer care if the mapping for the flush exists or not because we always recreate it via the tmpalias pages.

Quoted 7 lines

Quoted 3 lines
On ARM, flush_kernel_dcache_page() actually assumes that the page is mapped. It avoids double flushing of highmem pages by not flushing in those cases where kunmap_atomic() already takes care of flushing.
On Parisc, kmap/kunmap is currently a nop. However, if we ever implemented highmem, we would also need to flush before we unmap, which is why the flush needs to go before the kunmap.

Not quite. On PA8800/PA8900, we currently do a flush in kunmap.

I'm fairly certain from discussion with Helge that he saw this bug on a C8000 with PA8800 processor. In that case, adding a call to flush_kernel_dcache_page() just duplicates the call in kunmap(). Could be wrong though.

The problem is the coherence with userspace. That's why flush_dcache_page() is called in several places in this file. I agree with you that this should be done before.

Quoted 7 lines

I've got to say on all of this, we've implemented the most inane set of primitives. It would be much easier if kmap/kunmap just took care of all of this. kmap should bring userspace into coherence (because why else would we be kmapping) and kunmap should force coherence on the kernel address ... and then no-one would need to worry when to and when not to flush.

I'm working on adding a flush to our kmap code to bring userspace into coherence on PA8800/PA8900. It's ugly because it requires looping over vma's as we do in in flush_dcache_page().

Dave
--
John David Anglin [email protected]

James Bottomley

maintainer

2013-11-16 22:37 UTC #15 lore raw

Quoted 34 lines · John David Anglin

Quoted 23 lines · James Bottomley
Quoted 22 lines · Simon Baatz
Quoted 20 lines · James Bottomley
Quoted 6 lines · Helge Deller
When a user page mapping is released via kunmap*() functions, the D-cache needs
to be flushed via flush_dcache_page() to avoid D-cache aliasing issues.
This patch fixes aio on the parisc platform (and probably others).
This should be flush_kernel_dcache_page(). flush_dcache_page() is for
full coherency but for unmap, we know the page was coherent going in and
may have been modified by the kernel, so only the kernel view needs to
be sync'd. Technically, by the kernel API, the flush should be done *before* unmapping. This would have mattered on parisc until we did flush via tmpalias which means we no-longer care if the mapping for the
flush exists or not because we always recreate it via the tmpalias pages.
Quoted 8 lines
Quoted 3 lines
On ARM, flush_kernel_dcache_page() actually assumes that the page is mapped. It avoids double flushing of highmem pages by not flushing in those cases where kunmap_atomic() already takes care of flushing.
On Parisc, kmap/kunmap is currently a nop. However, if we ever implemented highmem, we would also need to flush before we unmap, which
is why the flush needs to go before the kunmap.

Quoted 7 lines

Not quite. On PA8800/PA8900, we currently do a flush in kunmap.
I'm fairly certain from discussion with Helge that he saw this bug on a C8000
with PA8800 processor. In that case, adding a call to flush_kernel_dcache_page()
just duplicates the call in kunmap().

Yes, if that's true it won't be the solution.

Quoted 7 lines

Could be wrong though.
The problem is the coherence with userspace. That's why flush_dcache_page()
is called in several places in this file. I agree with you that this should be done
before.

If we need coherence with userspace, we can't do it as we unmap because by then we've used the data in the kernel. We have to do it as we kmap, because that's when we're preparing to use the data. So the solution, if this is the problem, would be flush_dcache_page() just after kmap.

James

John David Anglin

2013-11-16 22:49 UTC #16 lore raw

Quoted 37 lines · James Bottomley

Quoted 36 lines · John David Anglin
Quoted 23 lines · James Bottomley
Quoted 22 lines · Simon Baatz
Quoted 20 lines · James Bottomley
Quoted 6 lines · Helge Deller
When a user page mapping is released via kunmap*() functions, the D-cache needs
to be flushed via flush_dcache_page() to avoid D-cache aliasing issues.
This patch fixes aio on the parisc platform (and probably others).
This should be flush_kernel_dcache_page(). flush_dcache_page() is for
full coherency but for unmap, we know the page was coherent going in and
may have been modified by the kernel, so only the kernel view needs to
be sync'd. Technically, by the kernel API, the flush should be done *before* unmapping. This would have mattered on parisc until we did flush via tmpalias which means we no-longer care if the mapping for the
flush exists or not because we always recreate it via the tmpalias pages.
Quoted 8 lines
Quoted 3 lines
On ARM, flush_kernel_dcache_page() actually assumes that the page is mapped. It avoids double flushing of highmem pages by not flushing in those cases where kunmap_atomic() already takes care of flushing.
On Parisc, kmap/kunmap is currently a nop. However, if we ever implemented highmem, we would also need to flush before we unmap, which
is why the flush needs to go before the kunmap.
Not quite. On PA8800/PA8900, we currently do a flush in kunmap.

Quoted 7 lines

Quoted 5 lines
I'm fairly certain from discussion with Helge that he saw this bug on a C8000
with PA8800 processor. In that case, adding a call to flush_kernel_dcache_page()
just duplicates the call in kunmap().
Yes, if that's true it won't be the solution.

The calls may be needed for the two cases where the page is changed, but I don't know how to avoid double flush on PA8800/PA8900 without changing the primatives.

Dave
--
John David Anglin [email protected]