[BUGFIX][PATCH] DO flush icache before set_pte() on ia64.

[BUGFIX][PATCH] DO flush icache before set_pte() on ia64.

[KAMEZAWA Hiroyuki]

This is a experimental patch for fixing icache flush race of ia64(Montecito).

Problem Description:
Montecito, new ia64 processor, has separated L2 i-cache and d-cache,
and i-cache and d-cache is not consistent in automatic way.

L1 cache is also separated but L1 D-cache is write-through. Then, before
Montecito, any changes in L1-dcache is visible in L2-mixed-cache consistently.

Montecito has separated L2 cache and Mixed L3 cache. But...L2 D-cache is
*write back*. (See http://download.intel.com/design/Itanium2/manuals/
30806501.pdf section 2.3.3)

Assume : valid data is in L2 d-cache and old data in L3 mixed cache.
If write-back L2->L3 is delayed, at L2 i-cache miss cpu will fetch old data
in L3 mixed cache. 
By this, L2-icache-miss will read wrong instruction from L3-mixed cache.
(Just I think so, is this correct ?)

Anyway, there is SIGILL problem in NFS/ia64 and icache flush can fix
SIGILL problem (in our HPC team test.)

Following SIGILL issue occurs in current kernel.
(This was a discussion in this April)
- http://www.gelato.unsw.edu.au/archives/linux-ia64/0704/20323.html
Usual file systems uses DMA and it purges cache. But NFS uses copy-by-cpu.

This is HP-UX's errata comment:
- http://h50221.www5.hp.com/upassist/itrc_japan/assist2/patchdigest/PHKL_36120.html
(Sorry for Japanese page...but English comments also written. See PHKL_36120)

Now, I think icache should be flushed before set_pte().
This is a patch to try that.

1. remove all lazy_mmu_prot_update()...which is used by only ia64.
2. implements flush_cache_page()/flush_icache_page() for ia64.

Something unsure....
3. mprotect() flushes cache before removing pte. Is this sane ?
   I added flush_icache_range() before set_pte() here.

Any comments and advices ?
 
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

---
 arch/ia64/mm/init.c           |    7 +------
 include/asm-generic/pgtable.h |    4 ----
 include/asm-ia64/cacheflush.h |   24 ++++++++++++++++++++++--
 include/asm-ia64/pgtable.h    |    9 ---------
 mm/fremap.c                   |    1 -
 mm/memory.c                   |   13 ++++++-------
 mm/migrate.c                  |    6 +++++-
 mm/mprotect.c                 |   10 +++++++++-
 mm/rmap.c                     |    1 -
 9 files changed, 43 insertions(+), 32 deletions(-)

Index: linux-2.6.22-rc7/include/asm-ia64/cacheflush.h
===================================================================
--- linux-2.6.22-rc7.orig/include/asm-ia64/cacheflush.h
+++ linux-2.6.22-rc7/include/asm-ia64/cacheflush.h
@@ -10,18 +10,38 @@
 
 #include <asm/bitops.h>
 #include <asm/page.h>
+#include <linux/mm.h>
 
 /*
  * Cache flushing routines.  This is the kind of stuff that can be very expensive, so try
  * to avoid them whenever possible.
  */
+extern void __flush_icache_page_ia64(struct page *page);
 
 #define flush_cache_all()			do { } while (0)
 #define flush_cache_mm(mm)			do { } while (0)
 #define flush_cache_dup_mm(mm)			do { } while (0)
 #define flush_cache_range(vma, start, end)	do { } while (0)
-#define flush_cache_page(vma, vmaddr, pfn)	do { } while (0)
-#define flush_icache_page(vma,page)		do { } while (0)
+
+static inline void
+flush_cache_page(struct vm_area_struct *vma, unsigned long vmaddr,
+		unsigned long pfn)
+{
+	if (vma->vm_flags & VM_EXEC) {
+		struct page *page;
+		if (!pfn_valid(pfn))
+			return;
+		page = pfn_to_page(pfn);
+		__flush_icache_page_ia64(page);
+	}
+}
+
+static inline void
+flush_icache_page(struct vm_area_struct *vma,struct page *page) {
+	if (vma->vm_flags & VM_EXEC)
+		__flush_icache_page_ia64(page);
+}
+
 #define flush_cache_vmap(start, end)		do { } while (0)
 #define flush_cache_vunmap(start, end)		do { } while (0)
 
Index: linux-2.6.22-rc7/arch/ia64/mm/init.c
===================================================================
--- linux-2.6.22-rc7.orig/arch/ia64/mm/init.c
+++ linux-2.6.22-rc7/arch/ia64/mm/init.c
@@ -105,16 +105,11 @@ check_pgt_cache(void)
 }
 
 void
-lazy_mmu_prot_update (pte_t pte)
+__flush_icache_page_ia64 (struct page *page)
 {
 	unsigned long addr;
-	struct page *page;
 	unsigned long order;
 
-	if (!pte_exec(pte))
-		return;				/* not an executable page... */
-
-	page = pte_page(pte);
 	addr = (unsigned long) page_address(page);
 
 	if (test_bit(PG_arch_1, &page->flags))
Index: linux-2.6.22-rc7/include/asm-ia64/pgtable.h
===================================================================
--- linux-2.6.22-rc7.orig/include/asm-ia64/pgtable.h
+++ linux-2.6.22-rc7/include/asm-ia64/pgtable.h
@@ -151,7 +151,6 @@
 
 #include <linux/sched.h>	/* for mm_struct */
 #include <asm/bitops.h>
-#include <asm/cacheflush.h>
 #include <asm/mmu_context.h>
 #include <asm/processor.h>
 
@@ -506,13 +505,6 @@ extern struct page *zero_page_memmap_ptr
 #define HUGETLB_PGDIR_MASK	(~(HUGETLB_PGDIR_SIZE-1))
 #endif
 
-/*
- * IA-64 doesn't have any external MMU info: the page tables contain all the necessary
- * information.  However, we use this routine to take care of any (delayed) i-cache
- * flushing that may be necessary.
- */
-extern void lazy_mmu_prot_update (pte_t pte);
-
 #define __HAVE_ARCH_PTEP_SET_ACCESS_FLAGS
 /*
  * Update PTEP with ENTRY, which is guaranteed to be a less
@@ -593,7 +585,6 @@ do {											\
 #define __HAVE_ARCH_PTEP_SET_WRPROTECT
 #define __HAVE_ARCH_PTE_SAME
 #define __HAVE_ARCH_PGD_OFFSET_GATE
-#define __HAVE_ARCH_LAZY_MMU_PROT_UPDATE
 
 #ifndef CONFIG_PGTABLE_4
 #include <asm-generic/pgtable-nopud.h>
Index: linux-2.6.22-rc7/include/asm-generic/pgtable.h
===================================================================
--- linux-2.6.22-rc7.orig/include/asm-generic/pgtable.h
+++ linux-2.6.22-rc7/include/asm-generic/pgtable.h
@@ -154,10 +154,6 @@ static inline void ptep_set_wrprotect(st
 #define pgd_offset_gate(mm, addr)	pgd_offset(mm, addr)
 #endif
 
-#ifndef __HAVE_ARCH_LAZY_MMU_PROT_UPDATE
-#define lazy_mmu_prot_update(pte)	do { } while (0)
-#endif
-
 #ifndef __HAVE_ARCH_MOVE_PTE
 #define move_pte(pte, prot, old_addr, new_addr)	(pte)
 #endif
Index: linux-2.6.22-rc7/mm/memory.c
===================================================================
--- linux-2.6.22-rc7.orig/mm/memory.c
+++ linux-2.6.22-rc7/mm/memory.c
@@ -1599,7 +1599,6 @@ static int do_wp_page(struct mm_struct *
 		entry = maybe_mkwrite(pte_mkdirty(entry), vma);
 		ptep_set_access_flags(vma, address, page_table, entry, 1);
 		update_mmu_cache(vma, address, entry);
-		lazy_mmu_prot_update(entry);
 		ret |= VM_FAULT_WRITE;
 		goto unlock;
 	}
@@ -1640,7 +1639,6 @@ gotten:
 		flush_cache_page(vma, address, pte_pfn(orig_pte));
 		entry = mk_pte(new_page, vma->vm_page_prot);
 		entry = maybe_mkwrite(pte_mkdirty(entry), vma);
-		lazy_mmu_prot_update(entry);
 		/*
 		 * Clear the pte entry and flush it first, before updating the
 		 * pte with the new entry. This will avoid a race condition
@@ -2105,7 +2103,6 @@ static int do_swap_page(struct mm_struct
 
 	/* No need to invalidate - it was non-present before */
 	update_mmu_cache(vma, address, pte);
-	lazy_mmu_prot_update(pte);
 unlock:
 	pte_unmap_unlock(page_table, ptl);
 out:
@@ -2162,12 +2159,16 @@ static int do_anonymous_page(struct mm_s
 		inc_mm_counter(mm, file_rss);
 		page_add_file_rmap(page);
 	}
-
+	/*
+	 * new page is zero-filled, but we have to guarantee icache-dcache
+	 * synchronization before setting pte on some processor.
+	 */
+	if (write_access && (vma->vm_flags & VM_EXEC))
+		flush_icache_page(vma, page);
 	set_pte_at(mm, address, page_table, entry);
 
 	/* No need to invalidate - it was non-present before */
 	update_mmu_cache(vma, address, entry);
-	lazy_mmu_prot_update(entry);
 unlock:
 	pte_unmap_unlock(page_table, ptl);
 	return VM_FAULT_MINOR;
@@ -2312,7 +2313,6 @@ retry:
 
 	/* no need to invalidate: a not-present page shouldn't be cached */
 	update_mmu_cache(vma, address, entry);
-	lazy_mmu_prot_update(entry);
 unlock:
 	pte_unmap_unlock(page_table, ptl);
 	if (dirty_page) {
@@ -2470,7 +2470,6 @@ static inline int handle_pte_fault(struc
 	if (!pte_same(old_entry, entry)) {
 		ptep_set_access_flags(vma, address, pte, entry, write_access);
 		update_mmu_cache(vma, address, entry);
-		lazy_mmu_prot_update(entry);
 	} else {
 		/*
 		 * This is needed only for protection faults but the arch code
Index: linux-2.6.22-rc7/mm/migrate.c
===================================================================
--- linux-2.6.22-rc7.orig/mm/migrate.c
+++ linux-2.6.22-rc7/mm/migrate.c
@@ -172,6 +172,11 @@ static void remove_migration_pte(struct 
 	pte = pte_mkold(mk_pte(new, vma->vm_page_prot));
 	if (is_write_migration_entry(entry))
 		pte = pte_mkwrite(pte);
+	/*
+	 * If the processor doesn't guarantee icache-dicache synchronization.
+	 * We need to flush icache before set_pte.
+	 */
+	flush_icache_page(vma, new);
 	set_pte_at(mm, addr, ptep, pte);
 
 	if (PageAnon(new))
@@ -181,7 +186,6 @@ static void remove_migration_pte(struct 
 
 	/* No need to invalidate - it was non-present before */
 	update_mmu_cache(vma, addr, pte);
-	lazy_mmu_prot_update(pte);
 
 out:
 	pte_unmap_unlock(ptep, ptl);
Index: linux-2.6.22-rc7/mm/mprotect.c
===================================================================
--- linux-2.6.22-rc7.orig/mm/mprotect.c
+++ linux-2.6.22-rc7/mm/mprotect.c
@@ -52,8 +52,16 @@ static void change_pte_range(struct mm_s
 			 */
 			if (dirty_accountable && pte_dirty(ptent))
 				ptent = pte_mkwrite(ptent);
+#ifdef CONFIG_SMP
+			/* we already flushed cache before reach here.
+			 * But that flush was done before removing pte.
+			 * we confirm i-cache consitency here again.
+			 * This is rare case.
+			 */
+			if (pte_exec(ptent))
+				flush_icache_range(addr, addr + PAGE_SIZE);
+#endif
 			set_pte_at(mm, addr, pte, ptent);
-			lazy_mmu_prot_update(ptent);
 #ifdef CONFIG_MIGRATION
 		} else if (!pte_file(oldpte)) {
 			swp_entry_t entry = pte_to_swp_entry(oldpte);
Index: linux-2.6.22-rc7/mm/rmap.c
===================================================================
--- linux-2.6.22-rc7.orig/mm/rmap.c
+++ linux-2.6.22-rc7/mm/rmap.c
@@ -461,7 +461,6 @@ static int page_mkclean_one(struct page 
 		entry = pte_wrprotect(entry);
 		entry = pte_mkclean(entry);
 		set_pte_at(mm, address, pte, entry);
-		lazy_mmu_prot_update(entry);
 		ret = 1;
 	}
 
Index: linux-2.6.22-rc7/mm/fremap.c
===================================================================
--- linux-2.6.22-rc7.orig/mm/fremap.c
+++ linux-2.6.22-rc7/mm/fremap.c
@@ -83,7 +83,6 @@ int install_page(struct mm_struct *mm, s
 	set_pte_at(mm, addr, pte, pte_val);
 	page_add_file_rmap(page);
 	update_mmu_cache(vma, addr, pte_val);
-	lazy_mmu_prot_update(pte_val);
 	err = 0;
 unlock:
 	pte_unmap_unlock(pte, ptl);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX][PATCH] DO flush icache before set_pte() on ia64.

[Nick Piggin]

KAMEZAWA Hiroyuki wrote:
> This is a experimental patch for fixing icache flush race of ia64(Montecito).
> 
> Problem Description:
> Montecito, new ia64 processor, has separated L2 i-cache and d-cache,
> and i-cache and d-cache is not consistent in automatic way.
> 
> L1 cache is also separated but L1 D-cache is write-through. Then, before
> Montecito, any changes in L1-dcache is visible in L2-mixed-cache consistently.
> 
> Montecito has separated L2 cache and Mixed L3 cache. But...L2 D-cache is
> *write back*. (See http://download.intel.com/design/Itanium2/manuals/
> 30806501.pdf section 2.3.3)
> 
> Assume : valid data is in L2 d-cache and old data in L3 mixed cache.
> If write-back L2->L3 is delayed, at L2 i-cache miss cpu will fetch old data
> in L3 mixed cache. 
> By this, L2-icache-miss will read wrong instruction from L3-mixed cache.
> (Just I think so, is this correct ?)
> 
> Anyway, there is SIGILL problem in NFS/ia64 and icache flush can fix
> SIGILL problem (in our HPC team test.)
> 
> Following SIGILL issue occurs in current kernel.
> (This was a discussion in this April)
> - http://www.gelato.unsw.edu.au/archives/linux-ia64/0704/20323.html
> Usual file systems uses DMA and it purges cache. But NFS uses copy-by-cpu.
> 
> This is HP-UX's errata comment:
> - http://h50221.www5.hp.com/upassist/itrc_japan/assist2/patchdigest/PHKL_36120.html
> (Sorry for Japanese page...but English comments also written. See PHKL_36120)
> 
> Now, I think icache should be flushed before set_pte().
> This is a patch to try that.
> 
> 1. remove all lazy_mmu_prot_update()...which is used by only ia64.
> 2. implements flush_cache_page()/flush_icache_page() for ia64.
> 
> Something unsure....
> 3. mprotect() flushes cache before removing pte. Is this sane ?
>    I added flush_icache_range() before set_pte() here.
> 
> Any comments and advices ?

Thanks, this is the way I wanted to see it go in the generic code. (ie.
get rid of lazy_mmu_prot_uptdate and actually follow the cacheflush API
instead).

The only thing I noticed when I looked at the code is that some places
may not have flushed icache when they should have? Did you get them all?
Minor nitpick: you have one place where you test VM_EXEC before flushing,
but the flush routine itself contains the same test I think?

Regarding the ia64 code -- I'm not an expert so I can't say whether it
is the right thing to do or not. However I still can't work out what it's
rationale for the PG_arch_1 bit is, exactly. Does it assume that
flush_dcache_page sites would only ever be encountered by pages that are
not faulted in? A faulted in page kind of is "special" because it is
guaranteed uptodate, but is the ia64 arch code relying on that? Should it?
(there could definitely still be flush_dcache_page called on mapped pages,
but it should only be a subset of all possible sites -- I don't know if it
is too clean for ia64 cacheflush code to know that?). [*]

[*] all this is, as usual, predicated by the disclaimer that quirks in mm/
can result in mapped pages not being uptodate (in which case hell often
breaks loose in other ways anyway).

-- 
SUSE Labs, Novell Inc.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX][PATCH] DO flush icache before set_pte() on ia64.

[KAMEZAWA Hiroyuki]

On Wed, 04 Jul 2007 16:31:06 +1000
Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> The only thing I noticed when I looked at the code is that some places
> may not have flushed icache when they should have? Did you get them all?

I think that I added flush_icache_page() to the place where any flush_(i)cache_xxx
is not called and lazy_mmu_prot_update was used instead of them.
But I want good review, of course.

> Minor nitpick: you have one place where you test VM_EXEC before flushing,
> but the flush routine itself contains the same test I think?
> 
Ah, yes...in do_anonymous_page(). my mistake.

> Regarding the ia64 code -- I'm not an expert so I can't say whether it
> is the right thing to do or not. However I still can't work out what it's
> rationale for the PG_arch_1 bit is, exactly. Does it assume that
> flush_dcache_page sites would only ever be encountered by pages that are
> not faulted in? A faulted in page kind of is "special" because it is
> guaranteed uptodate, but is the ia64 arch code relying on that? Should it?

(I'm sorry if I misses point.)
ia64's D-cache is coherent but I-cache and D-cache is not coherent and any
invalidation against d-cache will invalidate I-cache.

In my understanding :
PG_arch_1 is used for showing "there is no inconsistent data on any level of
cache". PG_uptodate is used for showing "this page includes the newest data
and contents are valid."
...maybe not used for the same purpose.

BTW, a page filled by DMA should have PG_arch_1 :(

-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX][PATCH] DO flush icache before set_pte() on ia64.

[Nick Piggin]

KAMEZAWA Hiroyuki wrote:
> On Wed, 04 Jul 2007 16:31:06 +1000
> Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> 
>>The only thing I noticed when I looked at the code is that some places
>>may not have flushed icache when they should have? Did you get them all?
> 
> 
> I think that I added flush_icache_page() to the place where any flush_(i)cache_xxx
> is not called and lazy_mmu_prot_update was used instead of them.
> But I want good review, of course.
> 
> 
>>Minor nitpick: you have one place where you test VM_EXEC before flushing,
>>but the flush routine itself contains the same test I think?
>>
> 
> Ah, yes...in do_anonymous_page(). my mistake.
> 
> 
>>Regarding the ia64 code -- I'm not an expert so I can't say whether it
>>is the right thing to do or not. However I still can't work out what it's
>>rationale for the PG_arch_1 bit is, exactly. Does it assume that
>>flush_dcache_page sites would only ever be encountered by pages that are
>>not faulted in? A faulted in page kind of is "special" because it is
>>guaranteed uptodate, but is the ia64 arch code relying on that? Should it?
> 
> 
> (I'm sorry if I misses point.)
> ia64's D-cache is coherent but I-cache and D-cache is not coherent and any
> invalidation against d-cache will invalidate I-cache.
> 
> In my understanding :
> PG_arch_1 is used for showing "there is no inconsistent data on any level of
> cache". PG_uptodate is used for showing "this page includes the newest data
> and contents are valid."
> ...maybe not used for the same purpose.

I think that's right, but why is set_pte-time the critical point for the
flush? It is actually possible to write into an executable page via the
dcache *after* it has ptes pointing to it.

 From what I can work out, it is something like "at this point the page
should be uptodate, so at least the icache won't contain *inconsistent*
data, just old data which userspace should take care of flushing if it
modifies". Is that always true? Could the page get modified by means
other than a direct write(2)? And even in the case of a write(2) writer,
how do they know if another process is mapping that particular page for
exec at that time? Should they always flush? Flushing would require they
have a virtual address on the page to begin with anyway, doesn't it? So
they'd have to mmap it... phew.

I guess it is mostly safe because it is probably very uncommon to do
such a thing, and chances are no non-write(2) write activity happens to
a page after it is brought uptodate. But I don't know if that has been
audited. I would really like to see the kernel always manage all aspects
of its pagecache though. I realise performance considerations may make
this not always possible... but it might be possible to do efficiently
using mapcount these days?

Anyway, ignore my tangent if you like :) Your patch doesn't make any of
this worse, so I'm getting off topic.

So I think your patch is nice, but would need ia64 people to actually ack
it.


> BTW, a page filled by DMA should have PG_arch_1 :(

The consequences of not are superfluous flushes?

-- 
SUSE Labs, Novell Inc.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX][PATCH] DO flush icache before set_pte() on ia64.

[KAMEZAWA Hiroyuki]

On Thu, 05 Jul 2007 12:04:23 +1000
Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> > In my understanding :
> > PG_arch_1 is used for showing "there is no inconsistent data on any level of
> > cache". PG_uptodate is used for showing "this page includes the newest data
> > and contents are valid."
> > ...maybe not used for the same purpose.
> 
> I think that's right, but why is set_pte-time the critical point for the
> flush? It is actually possible to write into an executable page via the
> dcache *after* it has ptes pointing to it.
yes. I think there are 2 cases.
- copy-on-write case..... OS handles this.
- page is writable and app just rewrites it ...... the app should handle this.

> 
>  From what I can work out, it is something like "at this point the page
> should be uptodate, so at least the icache won't contain *inconsistent*
> data, just old data which userspace should take care of flushing if it
> modifies". Is that always true?
 
I think it's true. But, in this case, i-cache doesn't contain *incositent* data.
There are inconsistency between L2-Dcache and L3-mixed-cache. At L2-icache-miss,
a cpu fetches data from L3 cache.
This case seems defficult to be generalized...


> Could the page get modified by means
> other than a direct write(2)? And even in the case of a write(2) writer,
> how do they know if another process is mapping that particular page for
> exec at that time? Should they always flush? Flushing would require they
> have a virtual address on the page to begin with anyway, doesn't it? So
> they'd have to mmap it... phew.
> 
> I guess it is mostly safe because it is probably very uncommon to do
> such a thing, and chances are no non-write(2) write activity happens to
> a page after it is brought uptodate. But I don't know if that has been
> audited. I would really like to see the kernel always manage all aspects
> of its pagecache though. I realise performance considerations may make
> this not always possible... but it might be possible to do efficiently
> using mapcount these days?

generic_file_write() does flush_dcahe_page() but no flush_icache_page()...

Then..maybe this will be necessary...
==
	flush_dcache_page(page);
	if (page_mapcount(page) > 0 && page_is_mapped_as_text(page))
		flush_icache_page(page);
==
But I don't know whether write(2) to mapped text file is expected to work well.

> > BTW, a page filled by DMA should have PG_arch_1 :(
> 
> The consequences of not are superfluous flushes?
> 
Yes, DMA flushes all levels of cache.

Thanks,
-Kame



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX][PATCH] DO flush icache before set_pte() on ia64.

[Nick Piggin]

KAMEZAWA Hiroyuki wrote:
> On Thu, 05 Jul 2007 12:04:23 +1000
> Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> 
>>>In my understanding :
>>>PG_arch_1 is used for showing "there is no inconsistent data on any level of
>>>cache". PG_uptodate is used for showing "this page includes the newest data
>>>and contents are valid."
>>>...maybe not used for the same purpose.
>>
>>I think that's right, but why is set_pte-time the critical point for the
>>flush? It is actually possible to write into an executable page via the
>>dcache *after* it has ptes pointing to it.
> 
> yes. I think there are 2 cases.
> - copy-on-write case..... OS handles this.
> - page is writable and app just rewrites it ...... the app should handle this.

Well it may not be "the" app, but any app on the system might write to
a page shared by any other app. OK we could just add the vague qualifier
about "don't do stupid stuff", but that should be only AFTER it is
determined that handling the corner cases is too hard.


>> From what I can work out, it is something like "at this point the page
>>should be uptodate, so at least the icache won't contain *inconsistent*
>>data, just old data which userspace should take care of flushing if it
>>modifies". Is that always true?
> 
>  
> I think it's true. But, in this case, i-cache doesn't contain *incositent* data.
> There are inconsistency between L2-Dcache and L3-mixed-cache. At L2-icache-miss,
> a cpu fetches data from L3 cache.
> This case seems defficult to be generalized...

If there is something in the icache line that isn't the last data to
be stored at that address, isn't that inconsistent?


>>Could the page get modified by means
>>other than a direct write(2)? And even in the case of a write(2) writer,
>>how do they know if another process is mapping that particular page for
>>exec at that time? Should they always flush? Flushing would require they
>>have a virtual address on the page to begin with anyway, doesn't it? So
>>they'd have to mmap it... phew.
>>
>>I guess it is mostly safe because it is probably very uncommon to do
>>such a thing, and chances are no non-write(2) write activity happens to
>>a page after it is brought uptodate. But I don't know if that has been
>>audited. I would really like to see the kernel always manage all aspects
>>of its pagecache though. I realise performance considerations may make
>>this not always possible... but it might be possible to do efficiently
>>using mapcount these days?
> 
> 
> generic_file_write() does flush_dcahe_page() but no flush_icache_page()...
> 
> Then..maybe this will be necessary...
> ==
> 	flush_dcache_page(page);
> 	if (page_mapcount(page) > 0 && page_is_mapped_as_text(page))
> 		flush_icache_page(page);
> ==
> But I don't know whether write(2) to mapped text file is expected to work well.

The point of the flush_dcache is that you have just written to dcache via
the kernel virtual address and need to handle any aliases. So if you did
want to handle this with ia64, you would do it all in flush_dcache_page.

flush_icache_page AFAIKS allows you to attempt some funny tricks to avoid
flushing @ flush_dcache_page-time. It is better than the lazy_mmu_update
thingy (because it is actually done in the right place -- ie. *before* the
set_pte), however it is still pretty tricky.

-- 
SUSE Labs, Novell Inc.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX][PATCH] DO flush icache before set_pte() on ia64.

[KAMEZAWA Hiroyuki]

On Thu, 05 Jul 2007 13:19:41 +1000
Nick Piggin <nickpiggin@yahoo.com.au> wrote:

> >> From what I can work out, it is something like "at this point the page
> >>should be uptodate, so at least the icache won't contain *inconsistent*
> >>data, just old data which userspace should take care of flushing if it
> >>modifies". Is that always true?
> > 
> >  
> > I think it's true. But, in this case, i-cache doesn't contain *incositent* data.
> > There are inconsistency between L2-Dcache and L3-mixed-cache. At L2-icache-miss,
> > a cpu fetches data from L3 cache.
> > This case seems defficult to be generalized...
> 
> If there is something in the icache line that isn't the last data to
> be stored at that address, isn't that inconsistent?
> 
Hmm..do we have a chance to add do_flush_cache_if_not_filled_by_dma(page)
before SetPageUptodate(page) ?

-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX][PATCH] DO flush icache before set_pte() on ia64.

[Nick Piggin]

KAMEZAWA Hiroyuki wrote:
> On Thu, 05 Jul 2007 13:19:41 +1000
> Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> 
> 
>>>>From what I can work out, it is something like "at this point the page
>>>>should be uptodate, so at least the icache won't contain *inconsistent*
>>>>data, just old data which userspace should take care of flushing if it
>>>>modifies". Is that always true?
>>>
>>> 
>>>I think it's true. But, in this case, i-cache doesn't contain *incositent* data.
>>>There are inconsistency between L2-Dcache and L3-mixed-cache. At L2-icache-miss,
>>>a cpu fetches data from L3 cache.
>>>This case seems defficult to be generalized...
>>
>>If there is something in the icache line that isn't the last data to
>>be stored at that address, isn't that inconsistent?
>>
> 
> Hmm..do we have a chance to add do_flush_cache_if_not_filled_by_dma(page)
> before SetPageUptodate(page) ?

Well you shouldn't strictly need to, because only uptodate pages should
ever be mapped with set_pte, and if you do the flush_icache before the
set_pte, then you effectively get the same result.

And if that's the result you want, then I don't think the flush_icache
before set_pte is a *terrible* way to get it.

-- 
SUSE Labs, Novell Inc.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX][PATCH] DO flush icache before set_pte() on ia64.

[Mike Stroyan]

On Wed, Jul 04, 2007 at 03:05:04PM +0900, KAMEZAWA Hiroyuki wrote:
> This is a experimental patch for fixing icache flush race of ia64(Montecito).
> 
> Problem Description:
> Montecito, new ia64 processor, has separated L2 i-cache and d-cache,
> and i-cache and d-cache is not consistent in automatic way.
> 
> L1 cache is also separated but L1 D-cache is write-through. Then, before
> Montecito, any changes in L1-dcache is visible in L2-mixed-cache consistently.
> 
> Montecito has separated L2 cache and Mixed L3 cache. But...L2 D-cache is
> *write back*. (See http://download.intel.com/design/Itanium2/manuals/
> 30806501.pdf section 2.3.3)
> 
> Assume : valid data is in L2 d-cache and old data in L3 mixed cache.
> If write-back L2->L3 is delayed, at L2 i-cache miss cpu will fetch old data
> in L3 mixed cache. 
> By this, L2-icache-miss will read wrong instruction from L3-mixed cache.
> (Just I think so, is this correct ?)

  The L3 cache is involved in the HP-UX defect description because the
earlier HP-UX patch PHKL_33781 added flushing of the instruction cache
when an executable mapping was removed.  Linux never added that
unsuccessfull attempt at montecito cache coherency.  In the current
linux situation it can execute old cache lines straight from L2 icache.

> Now, I think icache should be flushed before set_pte().
> This is a patch to try that.
> 
> 1. remove all lazy_mmu_prot_update()...which is used by only ia64.
> 2. implements flush_cache_page()/flush_icache_page() for ia64.
> 
> Something unsure....
> 3. mprotect() flushes cache before removing pte. Is this sane ?
>    I added flush_icache_range() before set_pte() here.
> 
> Any comments and advices ?

  I am concerned about performance consequences.  With the change
from lazy_mmu_prot_update to __flush_icache_page_ia64 you dropped
the code that avoids icache flushes for non-executable pages.
Section 4.6.2 of David Mosberger and Stephane Eranian's
"ia-64 linux kernel design and implementation" goes into some
detail about the performance penalties avoided by limiting icache
flushes to executable pages and defering flushes until the first
fault for execution.

  Have you done any benchmarking to measure the performance
effect of these additional cache flushes?  It would be particularly
interesting to measure on large systems with many CPUs.  The fc.i
instruction needs to be broadcast to all CPUs in the system.

  The only defect that I see in the current implementation of
lazy_mmu_prot_update() is that it is called too late in some
functions that are already calling it.  Are your large changes
attempting to correct other defects?  Or are you simplifying
away potentially valuable code because you don't understand it?

>  
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
> ---
>  arch/ia64/mm/init.c           |    7 +------
>  include/asm-generic/pgtable.h |    4 ----
>  include/asm-ia64/cacheflush.h |   24 ++++++++++++++++++++++--
>  include/asm-ia64/pgtable.h    |    9 ---------
>  mm/fremap.c                   |    1 -
>  mm/memory.c                   |   13 ++++++-------
>  mm/migrate.c                  |    6 +++++-
>  mm/mprotect.c                 |   10 +++++++++-
>  mm/rmap.c                     |    1 -
>  9 files changed, 43 insertions(+), 32 deletions(-)

You don't seem to have removed the lazy_mmu_prot_update() calls from
mm/hugetlb.c.  Will that build with HUGETLBFS configured?

-- 
Mike Stroyan <mike@stroyan.net>

P.S.  I am retired from hp.  So the mike_stroyan@hp.com address that
      this was previously cc'd to no longer reaches me.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX][PATCH] DO flush icache before set_pte() on ia64.

[KAMEZAWA Hiroyuki]

On Thu, 5 Jul 2007 12:13:09 -0600
Mike Stroyan <mike@stroyan.net> wrote:
>   The L3 cache is involved in the HP-UX defect description because the
> earlier HP-UX patch PHKL_33781 added flushing of the instruction cache
> when an executable mapping was removed.  Linux never added that
> unsuccessfull attempt at montecito cache coherency.  In the current
> linux situation it can execute old cache lines straight from L2 icache.
> 
Hmm... I couldn't understand "why icache includes old lines in a new page."
This happens at
 - a file is newly loaded into page-cache.
 - only on NFS.
 - happens very *often* if the program is unlucky.

So I wrote my understainding as I think.

> > Now, I think icache should be flushed before set_pte().
> > This is a patch to try that.
> > 
> > 1. remove all lazy_mmu_prot_update()...which is used by only ia64.
> > 2. implements flush_cache_page()/flush_icache_page() for ia64.
> > 
> > Something unsure....
> > 3. mprotect() flushes cache before removing pte. Is this sane ?
> >    I added flush_icache_range() before set_pte() here.
> > 
> > Any comments and advices ?
> 
>   I am concerned about performance consequences.  With the change
> from lazy_mmu_prot_update to __flush_icache_page_ia64 you dropped
> the code that avoids icache flushes for non-executable pages.

Hmm? I added VM_EXEC check in flush_(d|i)cache_page(). Isn't it enough ?

> Section 4.6.2 of David Mosberger and Stephane Eranian's
> "ia-64 linux kernel design and implementation" goes into some
> detail about the performance penalties avoided by limiting icache
> flushes to executable pages and defering flushes until the first
> fault for execution.
> 
>   Have you done any benchmarking to measure the performance
> effect of these additional cache flushes?  It would be particularly
> interesting to measure on large systems with many CPUs.  The fc.i
> instruction needs to be broadcast to all CPUs in the system.

no benchmarks yet.

> 
>   The only defect that I see in the current implementation of
> lazy_mmu_prot_update() is that it is called too late in some
> functions that are already calling it.  Are your large changes
> attempting to correct other defects?  Or are you simplifying
> away potentially valuable code because you don't understand it?
> 
I know your *simple* patch in April wasn't included. So I wrote this.
In April thread, commenter's advices was "implement flush_icache_page()" I think.  
If you have a better patch, please post.

Thanks,
-Kame




--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX][PATCH] DO flush icache before set_pte() on ia64.

[KAMEZAWA Hiroyuki]

On Thu, 5 Jul 2007 12:13:09 -0600
Mike Stroyan <mike@stroyan.net> wrote:

> You don't seem to have removed the lazy_mmu_prot_update() calls from
> mm/hugetlb.c.  Will that build with HUGETLBFS configured?
> 
Thanks, it's my patch refresh miss... Sigh..

-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX][PATCH] DO flush icache before set_pte() on ia64.

[KAMEZAWA Hiroyuki]

On Fri, 6 Jul 2007 07:18:53 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> On Thu, 5 Jul 2007 12:13:09 -0600
> Mike Stroyan <mike@stroyan.net> wrote:
> >   The L3 cache is involved in the HP-UX defect description because the
> > earlier HP-UX patch PHKL_33781 added flushing of the instruction cache
> > when an executable mapping was removed.  Linux never added that
> > unsuccessfull attempt at montecito cache coherency.  In the current
> > linux situation it can execute old cache lines straight from L2 icache.
> > 
> Hmm... I couldn't understand "why icache includes old lines in a new page."
> This happens at
>  - a file is newly loaded into page-cache.
>  - only on NFS.
>  - happens very *often* if the program is unlucky.
> 
> So I wrote my understainding as I think.
> 
I'll remove reference to HP-UX in the next post. And rewrite all description.

> > 
> >   The only defect that I see in the current implementation of
> > lazy_mmu_prot_update() is that it is called too late in some
> > functions that are already calling it.  Are your large changes
> > attempting to correct other defects?  Or are you simplifying
> > away potentially valuable code because you don't understand it?
> > 
> I know your *simple* patch in April wasn't included. So I wrote this.
> In April thread, commenter's advices was "implement flush_icache_page()" I think.  
> If you have a better patch, please post.
> 
I'll check callers of lazy_mmu_prot_update() again and remove uncecessary calls.
But, basically, i-cache flush will be necessary when VM_EXEC is on. PG_arch_1 will
help us for optimization.

-Kame




--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX][PATCH] DO flush icache before set_pte() on ia64.

[Christoph Lameter]

I am a bit worried about the performance impact of all this flushing? What 
is the worst case scenario here?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX][PATCH] DO flush icache before set_pte() on ia64.

[KAMEZAWA Hiroyuki]

On Fri, 6 Jul 2007 10:32:29 -0700 (PDT)
Christoph Lameter <clameter@sgi.com> wrote:

> I am a bit worried about the performance impact of all this flushing? What 
> is the worst case scenario here?
> 

IMHO....
When a user set VM_EXEC to their anonymous memory intentionally and
does many page faults.

I myself don't think (file's) page cache flushing is not so heavy work because
PG_arch_1 guarantees that icache flushing occurs just once at the first 
read in the system.

Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>