public inbox for linux-arch@vger.kernel.org
 help / color / mirror / Atom feed
* [RFC] flush_cache_page in kernel/ptrace.c
@ 2004-09-09 21:56 Russell King
  2004-09-10  0:06 ` Linus Torvalds
  0 siblings, 1 reply; 5+ messages in thread
From: Russell King @ 2004-09-09 21:56 UTC (permalink / raw)
  To: linux-arch, Linus Torvalds, Andrew Morton

Hi,

(This has turned into something of a rambling message, sorry.)

I'm presently looking at sorting out cache issues on ARM VIPT aliasing
caches, and I'm concerned with the above usage of flush_cache_page().

At present, flush_cache_page() is used to handle the case where we
unmap a page or alter the page permissions on the target page with
one exception - access_process_vm().  Based upon the former, the
decision to implement this function is:

	do we need to flush the cache when we unmap or change
	the mapping permissions?

However, access_process_vm() also includes into this:

	or we need to ensure cache coherency between the kernel
	and user space mapping of this page.

I argue that the use of flush_cache_page() here in the generic
code is wrong, and if an architecture wishes to use it for this
purpose, it should do so within it's architecture private
implementation of copy_to_user_page() and copy_from_user_page().

Further, I wish to question why access_process_vm() is different
from the case where a user binary has a shared file mmapping and
another process writes data into that file.  In this case we
make no attempt to call flush_cache_page() prior to writing the
page cache page, yet we do ultimately expect the same behaviour.


As for why... In the case of kernel/ptrace.c, the VIPT cache I'm
looking at would need:

- flush_cache_page would have to set up a temporary mapping of
  the same cache colour as the user mapping of the page, then
  flush the cache line entries in that user coherent mapping,
  and tear it down.

- copy_*_user_page() reads/writes the kernel mapping, and then
  uses flush_dcache_page() to ensure coherency

- flush_dcache_page() then flushes the kernel mapping, and then
  tries to find a user space mapping of the page, or will setup
  it's own mapping and flush that.

IOW, flush_cache_page() and flush_dcache_page() are both grossly
inefficient and overhanded for what needs to be done here, which
can be done far more effectively inside an appropriate
copy_*_user_page() implementation:

- map the page at an user coherent mapping
- flush cache lines for the affected region in the user coherent
  and kernel mappings
- copy data to/from kernel mapping and, if copy_to_user_page,
  ensure it's written back
- tear down user coherent mapping


Rambling on a bit, what this then gives me is a sane and clean
usage model for flush_cache_page().  At this point, all users
of flush_cache_page() have the PFN for the page to be flushed
readily available.

Why is this important?  Because on ARM VIPT caches, to be able
to flush cache lines, we need to have a mapping setup which
corresponds to the cache colour of the desired lines to be
flushed - which in turn means that flush_cache_page() may need
to create its own mapping, especially if it's being asked to
operate on a page outside our currently visible MM space.

Why can't the page tables be walked to obtain this information?
Check out pte_offset_map() / pte_offset_map_nested() - it would
be a violation of the semantics of these to call either inside
flush_dcache_page().

Comments?


(And am I hating these aliasing VIPT writeback caches yet?  Most
 certainly I am.)

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [RFC] flush_cache_page in kernel/ptrace.c
@ 2004-09-23 14:05 Russell King
  0 siblings, 0 replies; 5+ messages in thread
From: Russell King @ 2004-09-23 14:05 UTC (permalink / raw)
  To: Andrew Morton, Linus Torvalds, linux-arch

Since there appears to be almost zero interest from architecture
people in this (except jejb who seems to be solving a similar
problem, but has okayed this), can we merge it?

----- Forwarded message from Russell King <rmk@arm.linux.org.uk> -----

Date:	Fri, 17 Sep 2004 16:11:47 +0100
From:	Russell King <rmk@arm.linux.org.uk>
To:	linux-arch@vger.kernel.org, Andrew Morton <akpm@osdl.org>
Subject: Re: [RFC] flush_cache_page in kernel/ptrace.c

On Fri, Sep 10, 2004 at 04:23:30PM +0100, Russell King wrote:
> That's fine by me - please send your patches my way. 8)

Here's a patch which I've put together to achieve the first stage.
We remove flush_cache_page() from kernel/ptrace.c, adding it to
the arch-specific copy_{to,from}_user_page() where flush_cache_page
is non-empty.

Can architecture people test and check this patch please?

Thanks.

===== Documentation/cachetlb.txt 1.11 vs edited =====
--- 1.11/Documentation/cachetlb.txt	2004-07-11 01:14:00 +01:00
+++ edited/Documentation/cachetlb.txt	2004-09-17 14:50:31 +01:00
@@ -354,10 +354,6 @@
 	of arbitrary user pages (f.e. for ptrace()) it will use
 	these two routines.
 
-	The page has been kmap()'d, and flush_cache_page() has
-	just been called for the user mapping of this page (if
-	necessary).
-
 	Any necessary cache flushing or other coherency operations
 	that need to occur should happen here.  If the processor's
 	instruction cache does not snoop cpu stores, it is very
===== kernel/ptrace.c 1.36 vs edited =====
--- 1.36/kernel/ptrace.c	2004-09-08 07:32:59 +01:00
+++ edited/kernel/ptrace.c	2004-09-17 14:39:19 +01:00
@@ -202,8 +202,6 @@
 		if (bytes > PAGE_SIZE-offset)
 			bytes = PAGE_SIZE-offset;
 
-		flush_cache_page(vma, addr);
-
 		maddr = kmap(page);
 		if (write) {
 			copy_to_user_page(vma, page, addr,
===== include/asm-arm/cacheflush.h 1.15 vs edited =====
--- 1.15/include/asm-arm/cacheflush.h	2004-09-07 14:43:06 +01:00
+++ edited/include/asm-arm/cacheflush.h	2004-09-17 15:15:48 +01:00
@@ -237,11 +237,17 @@
  * space" model to handle this.
  */
 #define copy_to_user_page(vma, page, vaddr, dst, src, len) \
-do { memcpy(dst, src, len); \
-     flush_icache_user_range(vma, page, vaddr, len); \
-} while (0)
+	do {					\
+		flush_cache_page(vma, vaddr);	\
+		memcpy(dst, src, len);		\
+		flush_dcache_page(page);	\
+	} while (0)
+
 #define copy_from_user_page(vma, page, vaddr, dst, src, len) \
-	memcpy(dst, src, len)
+	do {					\
+		flush_cache_page(vma, vaddr);	\
+		memcpy(dst, src, len);		\
+	} while (0)
 
 /*
  * Convert calls to our calling convention.
===== include/asm-m68k/cacheflush.h 1.9 vs edited =====
--- 1.9/include/asm-m68k/cacheflush.h	2004-05-22 22:56:28 +01:00
+++ edited/include/asm-m68k/cacheflush.h	2004-09-17 15:15:21 +01:00
@@ -132,10 +132,18 @@
 #define flush_dcache_mmap_unlock(mapping)	do { } while (0)
 #define flush_icache_page(vma, page)	__flush_page_to_ram(page_address(page))
 #define flush_icache_user_range(vma,pg,adr,len)	do { } while (0)
+
 #define copy_to_user_page(vma, page, vaddr, dst, src, len) \
-	memcpy(dst, src, len)
+	do {					\
+		flush_cache_page(vma, vaddr);	\
+		memcpy(dst, src, len);		\
+	} while (0)
+
 #define copy_from_user_page(vma, page, vaddr, dst, src, len) \
-	memcpy(dst, src, len)
+	do {					\
+		flush_cache_page(vma, vaddr);	\
+		memcpy(dst, src, len);		\
+	} while (0)
 
 extern void flush_icache_range(unsigned long address, unsigned long endaddr);
 
===== include/asm-parisc/cacheflush.h 1.11 vs edited =====
--- 1.11/include/asm-parisc/cacheflush.h	2004-08-13 09:23:36 +01:00
+++ edited/include/asm-parisc/cacheflush.h	2004-09-17 16:04:02 +01:00
@@ -77,11 +77,17 @@
 #define flush_icache_range(s,e)		do { flush_kernel_dcache_range_asm(s,e); flush_kernel_icache_range_asm(s,e); } while (0)
 
 #define copy_to_user_page(vma, page, vaddr, dst, src, len) \
-do { memcpy(dst, src, len); \
-     flush_kernel_dcache_range_asm((unsigned long)dst, (unsigned long)dst + len); \
+do { \
+	flush_cache_page(vma, vaddr); \
+	memcpy(dst, src, len); \
+	flush_kernel_dcache_range_asm((unsigned long)dst, (unsigned long)dst + len); \
 } while (0)
+
 #define copy_from_user_page(vma, page, vaddr, dst, src, len) \
-	memcpy(dst, src, len)
+do { \
+	flush_cache_page(vma, vaddr); \
+	memcpy(dst, src, len); \
+} while (0)
 
 static inline void flush_cache_range(struct vm_area_struct *vma,
 		unsigned long start, unsigned long end)
===== include/asm-sh64/cacheflush.h 1.1 vs edited =====
--- 1.1/include/asm-sh64/cacheflush.h	2004-06-29 15:44:46 +01:00
+++ edited/include/asm-sh64/cacheflush.h	2004-09-17 15:31:42 +01:00
@@ -30,13 +30,17 @@
 #define flush_icache_page(vma, page)	do { } while (0)
 
 #define copy_to_user_page(vma, page, vaddr, dst, src, len) \
-do { memcpy(dst, src, len); \
-     flush_icache_user_range(vma, page, vaddr, len); \
-} while (0)
+	do {							\
+		flush_cache_page(vma, vaddr);			\
+		memcpy(dst, src, len);				\
+		flush_icache_user_range(vma, page, vaddr, len);	\
+	} while (0)
 
 #define copy_from_user_page(vma, page, vaddr, dst, src, len) \
-	memcpy(dst, src, len)
-
+	do {							\
+		flush_cache_page(vma, vaddr);			\
+		memcpy(dst, src, len);				\
+	} while (0)
 
 #endif /* __ASSEMBLY__ */
 
===== include/asm-sh/cacheflush.h 1.2 vs edited =====
--- 1.2/include/asm-sh/cacheflush.h	2003-10-02 08:11:59 +01:00
+++ edited/include/asm-sh/cacheflush.h	2004-09-17 15:29:45 +01:00
@@ -14,10 +14,16 @@
 #define flush_cache_vunmap(start, end)		flush_cache_all()
 
 #define copy_to_user_page(vma, page, vaddr, dst, src, len) \
-do { memcpy(dst, src, len); \
-     flush_icache_user_range(vma, page, vaddr, len); \
-} while (0)
+	do {							\
+		flush_cache_page(vma, vaddr);			\
+		memcpy(dst, src, len);				\
+		flush_icache_user_range(vma, page, vaddr, len);	\
+	} while (0)
+
 #define copy_from_user_page(vma, page, vaddr, dst, src, len) \
-	memcpy(dst, src, len)
+	do {							\
+		flush_cache_page(vma, vaddr);			\
+		memcpy(dst, src, len);				\
+	} while (0)
 
 #endif /* __ASM_SH_CACHEFLUSH_H */
===== include/asm-sparc64/cacheflush.h 1.6 vs edited =====
--- 1.6/include/asm-sparc64/cacheflush.h	2004-08-07 07:01:03 +01:00
+++ edited/include/asm-sparc64/cacheflush.h	2004-09-17 15:58:35 +01:00
@@ -38,9 +38,16 @@
 #define flush_icache_user_range(vma,pg,adr,len)	do { } while (0)
 
 #define copy_to_user_page(vma, page, vaddr, dst, src, len) \
-	memcpy(dst, src, len)
+	do {					\
+		flush_cache_page(vma, vaddr);	\
+		memcpy(dst, src, len);		\
+	} while (0)
+
 #define copy_from_user_page(vma, page, vaddr, dst, src, len) \
-	memcpy(dst, src, len)
+	do {					\
+		flush_cache_page(vma, vaddr);	\
+		memcpy(dst, src, len);		\
+	} while (0)
 
 extern void flush_dcache_page(struct page *page);
 #define flush_dcache_mmap_lock(mapping)		do { } while (0)
===== include/asm-sparc/cacheflush.h 1.5 vs edited =====
--- 1.5/include/asm-sparc/cacheflush.h	2004-05-22 22:56:28 +01:00
+++ edited/include/asm-sparc/cacheflush.h	2004-09-17 15:57:49 +01:00
@@ -57,9 +57,15 @@
 #define flush_icache_user_range(vma,pg,adr,len)	do { } while (0)
 
 #define copy_to_user_page(vma, page, vaddr, dst, src, len) \
-	memcpy(dst, src, len)
+	do {					\
+		flush_cache_page(vma, vaddr);	\
+		memcpy(dst, src, len);		\
+	} while (0)
 #define copy_from_user_page(vma, page, vaddr, dst, src, len) \
-	memcpy(dst, src, len)
+	do {					\
+		flush_cache_page(vma, vaddr);	\
+		memcpy(dst, src, len);		\
+	} while (0)
 
 BTFIXUPDEF_CALL(void, __flush_page_to_ram, unsigned long)
 BTFIXUPDEF_CALL(void, flush_sig_insns, struct mm_struct *, unsigned long)


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

----- End forwarded message -----

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2004-09-23 14:05 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-09-09 21:56 [RFC] flush_cache_page in kernel/ptrace.c Russell King
2004-09-10  0:06 ` Linus Torvalds
2004-09-10 15:23   ` Russell King
2004-09-17 15:11     ` Russell King
  -- strict thread matches above, loose matches on Subject: below --
2004-09-23 14:05 Russell King

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox