[PATCH] flush icache before set_pte take6. [0/4]

[PATCH] flush icache before set_pte take6. [0/4]

[KAMEZAWA Hiroyuki]

flush icache before set_pte() for ia64 patch set.

patches are against 2.6.23-rc1.

Changes:
  - Added "Is Montecito?" check for optimization.

Thanks,
-Kame

[PATCH] flush icache before set_pte take6. [1/4] migration fix

[KAMEZAWA Hiroyuki]

In migration, a new page should be cache flushed before set_pte()
in some archs which have virtually-tagged cache..

V5 -> V6:
   * no changes (added new patches to the patch set)
V4 -> V5:
   * changed flush_icache_page to flush_cache_page.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

---
 mm/migrate.c |    1 +
 1 file changed, 1 insertion(+)

Index: linux-2.6.23-rc1.test/mm/migrate.c
===================================================================
--- linux-2.6.23-rc1.test.orig/mm/migrate.c
+++ linux-2.6.23-rc1.test/mm/migrate.c
@@ -172,6 +172,7 @@ 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);
+	flush_cache_page(vma, addr, pte_pfn(pte));
 	set_pte_at(mm, addr, ptep, pte);
 
 	if (PageAnon(new))

[PATCH] flush icache before set_pte take6. [2/4] sync icache dcache

[KAMEZAWA Hiroyuki]

flush icache for ia64 take4.
This patch is against 2.6.23-rc1.

Changes V5 -> V6:
  - no changes. (added new patches to the patch set)

Changes V4 -> V5:
  - removed sync_icache_dcache from do_wp_page() page reuse case.

Changes v3 -> v4:
  - avoid implementing flush_(i)cache_pages().
  - added sync_icache_dcache() call.
  - change Documentation/cachetlb.txt

Current ia64 kernel flushes icache by lazy_mmu_prot_update() *after*
set_pte(). This is wrong. This patch removes lazy_mmu_prot_update and
add sync_icache_dcache(). sync_icache_dcache() is called before set_pte()
if necessary and synchronize icache with dcache (fc.i instruction).

This patch fixes SIGILL problem on NFS/ia64.

About Icache-Dcache inconsistency in ia64
 - When the cache line is modified, Icache and Dcache are purged.

 - When I-cache misses, I-cache will access just the lower layer cache(memory).
   Then, If the lower_layer_cache is not up-to-date, I-cache will see
   old information. For avoiding this case, Icache-Dcache synchronization(fc.i)
   is necessary. (Icache-Dcache synchronization means making Dcache and lower
   layer unified cache(memory) consistent.)

Details:
 - In general, cache flushing macro are used for virtually tagged caches.
   IA64 has physically tagged caches but doesn't guarantee consistency 
   between Icache and Dcache. So, new macro, sync_icache_dcache() is added.
   This is NO-OP in other archs.
 - sync_icache_dcache() only works if pte is executable.
 - sync_icache_dcache must be called before set_pte().
 - A page which is consistent is marked as PG_arch_1.

About changes in generic codes:
 - do_wp_page() ....need to sync newly copied page.
	            Here, lazy_mmu_prot_update() was done before set_pte().
                    This was because SIGILL in JAVA was reported and quick
		    fix was applied.

 - do_anonymous_page() .... newly installed anon pages doesn't contains any
                    instruction when set_pte() is executed, icache-dcache
		    synchronization is not necessary.

 - __do_fault() .... need to sync newly-installed page.

 - handle_pte_fault() .... just changes access bit...then, no need to sync.

 - remove_migration_pte().... need to sync newly-installed page.

 
 - change_pte_range() .... need to sync icache-dcache. When a user writes
		     instruction into the page and modifies protection to be	
		     executable, it should be synced.

 - hugetlb_change_protection() .... Maybe cache will be expired...but
	                  it is safe to sync Icache before set_pte().

 - page_mkclean_one() .... no need to sync icache-dcache. There is no page
                           contents modification. And there is no protection 
			   change.

Thanks to Zoltan Menyhart for his advices.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

---
 Documentation/cachetlb.txt    |   11 +++++++----
 arch/ia64/mm/init.c           |    6 ++----
 include/asm-generic/pgtable.h |    8 ++++----
 include/asm-ia64/pgtable.h    |   15 ++++++++++-----
 mm/hugetlb.c                  |    3 +--
 mm/memory.c                   |    7 ++-----
 mm/migrate.c                  |    2 +-
 mm/mprotect.c                 |    2 +-
 mm/rmap.c                     |    1 -
 9 files changed, 28 insertions(+), 27 deletions(-)

Index: linux-2.6.23-rc1.test/include/asm-generic/pgtable.h
===================================================================
--- linux-2.6.23-rc1.test.orig/include/asm-generic/pgtable.h
+++ linux-2.6.23-rc1.test/include/asm-generic/pgtable.h
@@ -124,14 +124,14 @@ 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
 
+#ifndef __HAVE_ARCH_SYNC_ICACHE_DCACHE
+#define sync_icache_dcache(pte)		do {} while (0)
+#endif
+
 /*
  * A facility to provide lazy MMU batching.  This allows PTE updates and
  * page invalidations to be delayed until a call to leave lazy MMU mode
Index: linux-2.6.23-rc1.test/include/asm-ia64/pgtable.h
===================================================================
--- linux-2.6.23-rc1.test.orig/include/asm-ia64/pgtable.h
+++ linux-2.6.23-rc1.test/include/asm-ia64/pgtable.h
@@ -484,11 +484,18 @@ extern struct page *zero_page_memmap_ptr
 #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.
+ * IA-64 doesn't guarantee Icache is consistent with Dcache. For ensure
+ * Icache consistency, we have to synchronize them before setting pte
+ * as an executable pte.
  */
-extern void lazy_mmu_prot_update (pte_t pte);
+extern void __sync_icache_dcache(pte_t pte);
+static inline void sync_icache_dcache(pte_t pte)
+{
+	if (pte_exec(pte))
+		__sync_icache_dcache(pte);
+}
+#define __HAVE_ARCH_SYNC_ICACHE_DCACHE
+
 
 #define __HAVE_ARCH_PTEP_SET_ACCESS_FLAGS
 /*
@@ -578,7 +585,6 @@ extern void lazy_mmu_prot_update (pte_t 
 #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.23-rc1.test/Documentation/cachetlb.txt
===================================================================
--- linux-2.6.23-rc1.test.orig/Documentation/cachetlb.txt
+++ linux-2.6.23-rc1.test/Documentation/cachetlb.txt
@@ -133,11 +133,14 @@ changes occur:
 	The ia64 sn2 platform is one example of a platform
 	that uses this interface.
 
-8) void lazy_mmu_prot_update(pte_t pte)
-	This interface is called whenever the protection on
-	any user PTEs change.  This interface provides a notification
-	to architecture specific code to take appropriate action.
+8) void sync_icache_dcache(pte_t pte)
+	This interface is used for synchronize icache and dcache.
+	Even if the cache is physically tagged, some archs doesn't
+	guarantee consistency between I-cache and D-cache. In such arch,
+	we need to synchronize I-cache and D-cache before installing
+	executable pages.
 
+	This is used only for ia64 now.
 
 Next, we have the cache flushing interfaces.  In general, when Linux
 is changing an existing virtual-->physical mapping to a new value,
Index: linux-2.6.23-rc1.test/mm/memory.c
===================================================================
--- linux-2.6.23-rc1.test.orig/mm/memory.c
+++ linux-2.6.23-rc1.test/mm/memory.c
@@ -1699,7 +1699,6 @@ static int do_wp_page(struct mm_struct *
 		entry = maybe_mkwrite(pte_mkdirty(entry), vma);
 		if (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;
@@ -1741,7 +1740,7 @@ 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);
+		sync_icache_dcache(entry);
 		/*
 		 * Clear the pte entry and flush it first, before updating the
 		 * pte with the new entry. This will avoid a race condition
@@ -2286,7 +2285,6 @@ static int do_anonymous_page(struct mm_s
 
 	/* 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 0;
@@ -2419,6 +2417,7 @@ static int __do_fault(struct mm_struct *
 	if (likely(pte_same(*page_table, orig_pte))) {
 		flush_icache_page(vma, page);
 		entry = mk_pte(page, vma->vm_page_prot);
+		sync_icache_dcache(entry);
 		if (flags & FAULT_FLAG_WRITE)
 			entry = maybe_mkwrite(pte_mkdirty(entry), vma);
 		set_pte_at(mm, address, page_table, entry);
@@ -2437,7 +2436,6 @@ static int __do_fault(struct mm_struct *
 
 		/* no need to invalidate: a not-present page won't be cached */
 		update_mmu_cache(vma, address, entry);
-		lazy_mmu_prot_update(entry);
 	} else {
 		if (anon)
 			page_cache_release(page);
@@ -2611,7 +2609,6 @@ static inline int handle_pte_fault(struc
 	entry = pte_mkyoung(entry);
 	if (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.23-rc1.test/mm/migrate.c
===================================================================
--- linux-2.6.23-rc1.test.orig/mm/migrate.c
+++ linux-2.6.23-rc1.test/mm/migrate.c
@@ -173,6 +173,7 @@ static void remove_migration_pte(struct 
 	if (is_write_migration_entry(entry))
 		pte = pte_mkwrite(pte);
 	flush_cache_page(vma, addr, pte_pfn(pte));
+	sync_icache_dcache(pte);
 	set_pte_at(mm, addr, ptep, pte);
 
 	if (PageAnon(new))
@@ -182,7 +183,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.23-rc1.test/mm/hugetlb.c
===================================================================
--- linux-2.6.23-rc1.test.orig/mm/hugetlb.c
+++ linux-2.6.23-rc1.test/mm/hugetlb.c
@@ -352,7 +352,6 @@ static void set_huge_ptep_writable(struc
 	entry = pte_mkwrite(pte_mkdirty(*ptep));
 	if (ptep_set_access_flags(vma, address, ptep, entry, 1)) {
 		update_mmu_cache(vma, address, entry);
-		lazy_mmu_prot_update(entry);
 	}
 }
 
@@ -704,8 +703,8 @@ void hugetlb_change_protection(struct vm
 		if (!pte_none(*ptep)) {
 			pte = huge_ptep_get_and_clear(mm, address, ptep);
 			pte = pte_mkhuge(pte_modify(pte, newprot));
+			sync_icache_dcache(pte);
 			set_huge_pte_at(mm, address, ptep, pte);
-			lazy_mmu_prot_update(pte);
 		}
 	}
 	spin_unlock(&mm->page_table_lock);
Index: linux-2.6.23-rc1.test/arch/ia64/mm/init.c
===================================================================
--- linux-2.6.23-rc1.test.orig/arch/ia64/mm/init.c
+++ linux-2.6.23-rc1.test/arch/ia64/mm/init.c
@@ -53,16 +53,13 @@ EXPORT_SYMBOL(vmem_map);
 struct page *zero_page_memmap_ptr;	/* map entry for zero page */
 EXPORT_SYMBOL(zero_page_memmap_ptr);
 
-void
-lazy_mmu_prot_update (pte_t pte)
+void __sync_icache_dcache(pte_t pte)
 {
 	unsigned long addr;
 	struct page *page;
 	unsigned long order;
 
-	if (!pte_exec(pte))
-		return;				/* not an executable page... */
-
+	BUG_ON(!pte_exec(pte));
 	page = pte_page(pte);
 	addr = (unsigned long) page_address(page);
 
Index: linux-2.6.23-rc1.test/mm/mprotect.c
===================================================================
--- linux-2.6.23-rc1.test.orig/mm/mprotect.c
+++ linux-2.6.23-rc1.test/mm/mprotect.c
@@ -52,8 +52,8 @@ static void change_pte_range(struct mm_s
 			 */
 			if (dirty_accountable && pte_dirty(ptent))
 				ptent = pte_mkwrite(ptent);
+			sync_icache_dcache(ptent);
 			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.23-rc1.test/mm/rmap.c
===================================================================
--- linux-2.6.23-rc1.test.orig/mm/rmap.c
+++ linux-2.6.23-rc1.test/mm/rmap.c
@@ -436,7 +436,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;
 	}
 

[PATCH] flush icache before set_pte take6. [3/4] add montecito brand name

[KAMEZAWA Hiroyuki]

Add Brand name "Montecito" to cpuinfo.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

---
 arch/ia64/kernel/setup.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux-2.6.23-rc1.test/arch/ia64/kernel/setup.c
===================================================================
--- linux-2.6.23-rc1.test.orig/arch/ia64/kernel/setup.c
+++ linux-2.6.23-rc1.test/arch/ia64/kernel/setup.c
@@ -705,7 +705,8 @@ get_model_name(__u8 family, __u8 model)
 			case 0: memcpy(brand, "McKinley", 9); break;
 			case 1: memcpy(brand, "Madison", 8); break;
 			case 2: memcpy(brand, "Madison up to 9M cache", 23); break;
-		}
+		} else if (family == 0x20)
+			memcpy(brand, "Montecito", 10);
 	}
 	for (i = 0; i < MAX_BRANDS; i++)
 		if (strcmp(brandname[i], brand) == 0)

[PATCH] flush icache before set_pte take6. [4/4] optimization for cpus other than montecito

[KAMEZAWA Hiroyuki]

Add "L2 cache is separated? check flag" as read_mostly global variable.

This add one memory reference to global variable to page faults of "executable"
map in do_wp_page(page copy case), file-mapped page fault and some system calls
which does memory map changes. But not so bad as calling sync_icache_dcache in
architectures which doesn't need it.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>


---
 arch/ia64/kernel/setup.c   |    7 +++++++
 include/asm-ia64/pgtable.h |    3 ++-
 2 files changed, 9 insertions(+), 1 deletion(-)

Index: linux-2.6.23-rc1.test/arch/ia64/kernel/setup.c
===================================================================
--- linux-2.6.23-rc1.test.orig/arch/ia64/kernel/setup.c
+++ linux-2.6.23-rc1.test/arch/ia64/kernel/setup.c
@@ -106,6 +106,8 @@ struct io_space io_space[MAX_IO_SPACES];
 EXPORT_SYMBOL(io_space);
 unsigned int num_io_spaces;
 
+int separated_l2_icache_dcache __read_mostly;
+
 /*
  * "flush_icache_range()" needs to know what processor dependent stride size to use
  * when it makes i-cache(s) coherent with d-caches.
@@ -718,6 +720,11 @@ get_model_name(__u8 family, __u8 model)
 		printk(KERN_ERR
 		       "%s: Table overflow. Some processor model information will be missing\n",
 		       __FUNCTION__);
+	/* Montecito has separated L2 Icache and Dcache. This requires
+	   synchronize Icache and Dcache before set_pte() */
+	if (family == 0x20)
+		separated_l2_icache_dcache = 1;
+
 	return "Unknown";
 }
 
Index: linux-2.6.23-rc1.test/include/asm-ia64/pgtable.h
===================================================================
--- linux-2.6.23-rc1.test.orig/include/asm-ia64/pgtable.h
+++ linux-2.6.23-rc1.test/include/asm-ia64/pgtable.h
@@ -489,9 +489,10 @@ extern struct page *zero_page_memmap_ptr
  * as an executable pte.
  */
 extern void __sync_icache_dcache(pte_t pte);
+extern int separated_l2_icache_dcache;
 static inline void sync_icache_dcache(pte_t pte)
 {
-	if (pte_exec(pte))
+	if (pte_exec(pte) && separated_l2_icache_dcache)
 		__sync_icache_dcache(pte);
 }
 #define __HAVE_ARCH_SYNC_ICACHE_DCACHE

Re: [PATCH] flush icache before set_pte take6. [4/4] optimization for cpus other than montecito

[David Mosberger-Tang]

This seems crazy to me.  Flushing should occur according to the
*architecture*, not model-by-model.  Even if we happen to get "lucky"
on pre-Montecito CPUs, that doesn't justify such ugly hacks.  Or you
really want to debug this *again* come next CPU?

  --david

On 7/30/07, KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
>
> Add "L2 cache is separated? check flag" as read_mostly global variable.
>
> This add one memory reference to global variable to page faults of "executable"
> map in do_wp_page(page copy case), file-mapped page fault and some system calls
> which does memory map changes. But not so bad as calling sync_icache_dcache in
> architectures which doesn't need it.
>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>
>
> ---
>  arch/ia64/kernel/setup.c   |    7 +++++++
>  include/asm-ia64/pgtable.h |    3 ++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
>
> Index: linux-2.6.23-rc1.test/arch/ia64/kernel/setup.c
> ===================================================================
> --- linux-2.6.23-rc1.test.orig/arch/ia64/kernel/setup.c
> +++ linux-2.6.23-rc1.test/arch/ia64/kernel/setup.c
> @@ -106,6 +106,8 @@ struct io_space io_space[MAX_IO_SPACES];
>  EXPORT_SYMBOL(io_space);
>  unsigned int num_io_spaces;
>
> +int separated_l2_icache_dcache __read_mostly;
> +
>  /*
>   * "flush_icache_range()" needs to know what processor dependent stride size to use
>   * when it makes i-cache(s) coherent with d-caches.
> @@ -718,6 +720,11 @@ get_model_name(__u8 family, __u8 model)
>                 printk(KERN_ERR
>                        "%s: Table overflow. Some processor model information will be missing\n",
>                        __FUNCTION__);
> +       /* Montecito has separated L2 Icache and Dcache. This requires
> +          synchronize Icache and Dcache before set_pte() */
> +       if (family == 0x20)
> +               separated_l2_icache_dcache = 1;
> +
>         return "Unknown";
>  }
>
> Index: linux-2.6.23-rc1.test/include/asm-ia64/pgtable.h
> ===================================================================
> --- linux-2.6.23-rc1.test.orig/include/asm-ia64/pgtable.h
> +++ linux-2.6.23-rc1.test/include/asm-ia64/pgtable.h
> @@ -489,9 +489,10 @@ extern struct page *zero_page_memmap_ptr
>   * as an executable pte.
>   */
>  extern void __sync_icache_dcache(pte_t pte);
> +extern int separated_l2_icache_dcache;
>  static inline void sync_icache_dcache(pte_t pte)
>  {
> -       if (pte_exec(pte))
> +       if (pte_exec(pte) && separated_l2_icache_dcache)
>                 __sync_icache_dcache(pte);
>  }
>  #define __HAVE_ARCH_SYNC_ICACHE_DCACHE
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
Mosberger Consulting LLC, http://www.mosberger-consulting.com/

Re: [PATCH] flush icache before set_pte take6. [4/4] optimization for cpus other than montecito

[KAMEZAWA Hiroyuki]

On Mon, 30 Jul 2007 22:15:50 -0600
"David Mosberger-Tang" <dmosberger@gmail.com> wrote:

> This seems crazy to me.  Flushing should occur according to the
> *architecture*, not model-by-model.  Even if we happen to get "lucky"
> on pre-Montecito CPUs, that doesn't justify such ugly hacks.  

I'm not sure this can happen before Montecito because L1 was write-through
and L2 was mixed. 

> Or you really want to debug this *again* come next CPU?

No. 
I should add RFC to this patch. I just want to hear opinions.
This is why I separated this patch. I can drop this.

Thanks,
-Kame





 
>   --david
> 
> On 7/30/07, KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> >
> > Add "L2 cache is separated? check flag" as read_mostly global variable.
> >
> > This add one memory reference to global variable to page faults of "executable"
> > map in do_wp_page(page copy case), file-mapped page fault and some system calls
> > which does memory map changes. But not so bad as calling sync_icache_dcache in
> > architectures which doesn't need it.
> >
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> >
> >
> > ---
> >  arch/ia64/kernel/setup.c   |    7 +++++++
> >  include/asm-ia64/pgtable.h |    3 ++-
> >  2 files changed, 9 insertions(+), 1 deletion(-)
> >
> > Index: linux-2.6.23-rc1.test/arch/ia64/kernel/setup.c
> > ===================================================================
> > --- linux-2.6.23-rc1.test.orig/arch/ia64/kernel/setup.c
> > +++ linux-2.6.23-rc1.test/arch/ia64/kernel/setup.c
> > @@ -106,6 +106,8 @@ struct io_space io_space[MAX_IO_SPACES];
> >  EXPORT_SYMBOL(io_space);
> >  unsigned int num_io_spaces;
> >
> > +int separated_l2_icache_dcache __read_mostly;
> > +
> >  /*
> >   * "flush_icache_range()" needs to know what processor dependent stride size to use
> >   * when it makes i-cache(s) coherent with d-caches.
> > @@ -718,6 +720,11 @@ get_model_name(__u8 family, __u8 model)
> >                 printk(KERN_ERR
> >                        "%s: Table overflow. Some processor model information will be missing\n",
> >                        __FUNCTION__);
> > +       /* Montecito has separated L2 Icache and Dcache. This requires
> > +          synchronize Icache and Dcache before set_pte() */
> > +       if (family == 0x20)
> > +               separated_l2_icache_dcache = 1;
> > +
> >         return "Unknown";
> >  }
> >
> > Index: linux-2.6.23-rc1.test/include/asm-ia64/pgtable.h
> > ===================================================================
> > --- linux-2.6.23-rc1.test.orig/include/asm-ia64/pgtable.h
> > +++ linux-2.6.23-rc1.test/include/asm-ia64/pgtable.h
> > @@ -489,9 +489,10 @@ extern struct page *zero_page_memmap_ptr
> >   * as an executable pte.
> >   */
> >  extern void __sync_icache_dcache(pte_t pte);
> > +extern int separated_l2_icache_dcache;
> >  static inline void sync_icache_dcache(pte_t pte)
> >  {
> > -       if (pte_exec(pte))
> > +       if (pte_exec(pte) && separated_l2_icache_dcache)
> >                 __sync_icache_dcache(pte);
> >  }
> >  #define __HAVE_ARCH_SYNC_ICACHE_DCACHE
> >
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> 
> 
> -- 
> Mosberger Consulting LLC, http://www.mosberger-consulting.com/
> 

Re: [PATCH] flush icache before set_pte take6. [4/4] optimization for cpus other than montecito

[KAMEZAWA Hiroyuki]

On Tue, 31 Jul 2007 13:29:32 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> On Mon, 30 Jul 2007 22:15:50 -0600
> "David Mosberger-Tang" <dmosberger@gmail.com> wrote:
> 
> > This seems crazy to me.  Flushing should occur according to the
> > *architecture*, not model-by-model.  Even if we happen to get "lucky"
> > on pre-Montecito CPUs, that doesn't justify such ugly hacks.  
> 

BTW, how about the quality of patch[2/4] ?

If it is enough well, I want it to be merged....

please..
-Kame

Re: [PATCH] flush icache before set_pte take6. [4/4] optimization for cpus other than montecito

[Zoltan Menyhart]

David Mosberger-Tang wrote:
> This seems crazy to me.  Flushing should occur according to the
> *architecture*, not model-by-model.  Even if we happen to get "lucky"
> on pre-Montecito CPUs, that doesn't justify such ugly hacks.  Or you
> really want to debug this *again* come next CPU?
> 
>   --david

O.K. let's say we flush by default: the global flag is set.

We can have a (short) list of the CPU models which do not require
this flush.

If all of the CPUs are on the list then clear the global flag. And:

static inline void sync_icache_dcache(pte_t pte) {
       if (pte_exec(pte) && global_flag)
               __sync_icache_dcache(pte);
}

Thanks,

Zoltan

Re: [PATCH] flush icache before set_pte take6. [4/4] optimization for cpus other than montecito

[KAMEZAWA Hiroyuki]

On Tue, 31 Jul 2007 10:38:48 +0200
Zoltan Menyhart <Zoltan.Menyhart@bull.net> wrote:

> David Mosberger-Tang wrote:
> > This seems crazy to me.  Flushing should occur according to the
> > *architecture*, not model-by-model.  Even if we happen to get "lucky"
> > on pre-Montecito CPUs, that doesn't justify such ugly hacks.  Or you
> > really want to debug this *again* come next CPU?
> > 
> >   --david
> 
> O.K. let's say we flush by default: the global flag is set.
> 
> We can have a (short) list of the CPU models which do not require
> this flush.
> 
> If all of the CPUs are on the list then clear the global flag. And:
> 
> static inline void sync_icache_dcache(pte_t pte) {
>        if (pte_exec(pte) && global_flag)
>                __sync_icache_dcache(pte);
> }
> 

Could we discuss this in other thread as "optimization for some cpus" and
push bug-fix patches first ?

If take5 or part of take6(patch 1,2) are not acceptable, I'll continue this
work on -rc2.

Thanks,
-Kame

Re: [PATCH] flush icache before set_pte take6. [4/4] optimization for cpus other than montecito

[Zoltan Menyhart]

KAMEZAWA Hiroyuki wrote:

> Could we discuss this in other thread as "optimization for some cpus" and
> push bug-fix patches first ?
> 
> If take5 or part of take6(patch 1,2) are not acceptable, I'll continue this
> work on -rc2.
> 
> Thanks,
> -Kame

Sure.

Thanks,

Zoltan

RE: [PATCH] flush icache before set_pte take6. [3/4] add montecito brand name

[Luck, Tony]

+		} else if (family == 0x20)
+			memcpy(brand, "Montecito", 10);

NAK.

We don't really have names for the different cpu families. "Montecito"
is definitely not the right string to apply here (Montvale will also
have family == 0x20).  The old McKinley/Madison strings are intended
to provide some information for old processors where PAL did not
support PAL_BRAND_INFO.  All new cpus (from Montecito onward) will
support this PAL call, so there should be no need to add more code to
make names out of [family,model] tuples.

-Tony

RE: [PATCH] flush icache before set_pte take6. [4/4] optimization for cpus other than montecito

[Luck, Tony]

> This seems crazy to me.  Flushing should occur according to the
> *architecture*, not model-by-model.  Even if we happen to get "lucky"
> on pre-Montecito CPUs, that doesn't justify such ugly hacks.  Or you
> really want to debug this *again* come next CPU?

Ditto.  The only reason we should ever have model specific checks should
be to work around model specific errata (e.g. the McKinley Errata #9 code
in patch.c).

-Tony

RE: [PATCH] flush icache before set_pte take6. [4/4] optimization for cpus other than montecito

[Jim Hull]

Not just crazy, but wrong - this *can* happen on pre-Montecito.  Even though
L1D is write-through and L2 was mixed I/D, the L1 I-cache could contain
stale instrutions if there are missing flushes. I think the only reason this
has never been observed is that the L1I is so small (32K) that it's likely
that any stale data has been displaced.

 -- Jim

> -----Original Message-----
> From: linux-ia64-owner@vger.kernel.org 
> [mailto:linux-ia64-owner@vger.kernel.org] On Behalf Of 
> KAMEZAWA Hiroyuki
> Sent: Monday, July 30, 2007 9:30 PM
> To: David Mosberger-Tang
> Cc: LKML; linux-ia64@vger.kernel.org; tony.luck@intel.com; 
> Zoltan.Menyhart@bull.net; Christoph Lameter
> Subject: Re: [PATCH] flush icache before set_pte take6. [4/4] 
> optimization for cpus other than montecito
> 
> On Mon, 30 Jul 2007 22:15:50 -0600
> "David Mosberger-Tang" <dmosberger@gmail.com> wrote:
> 
> > This seems crazy to me.  Flushing should occur according to the
> > *architecture*, not model-by-model.  Even if we happen to 
> get "lucky"
> > on pre-Montecito CPUs, that doesn't justify such ugly hacks.  
> 
> I'm not sure this can happen before Montecito because L1 was 
> write-through
> and L2 was mixed. 
> 
> > Or you really want to debug this *again* come next CPU?
> 
> No. 
> I should add RFC to this patch. I just want to hear opinions.
> This is why I separated this patch. I can drop this.
> 
> Thanks,
> -Kame
> 
> 
> 
> 
> 
>  
> >   --david
> > 
> > On 7/30/07, KAMEZAWA Hiroyuki 
> <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> > >
> > > Add "L2 cache is separated? check flag" as read_mostly 
> global variable.
> > >
> > > This add one memory reference to global variable to page 
> faults of "executable"
> > > map in do_wp_page(page copy case), file-mapped page fault 
> and some system calls
> > > which does memory map changes. But not so bad as calling 
> sync_icache_dcache in
> > > architectures which doesn't need it.
> > >
> > > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > >
> > >
> > > ---
> > >  arch/ia64/kernel/setup.c   |    7 +++++++
> > >  include/asm-ia64/pgtable.h |    3 ++-
> > >  2 files changed, 9 insertions(+), 1 deletion(-)
> > >
> > > Index: linux-2.6.23-rc1.test/arch/ia64/kernel/setup.c
> > > 
> ===================================================================
> > > --- linux-2.6.23-rc1.test.orig/arch/ia64/kernel/setup.c
> > > +++ linux-2.6.23-rc1.test/arch/ia64/kernel/setup.c
> > > @@ -106,6 +106,8 @@ struct io_space io_space[MAX_IO_SPACES];
> > >  EXPORT_SYMBOL(io_space);
> > >  unsigned int num_io_spaces;
> > >
> > > +int separated_l2_icache_dcache __read_mostly;
> > > +
> > >  /*
> > >   * "flush_icache_range()" needs to know what processor 
> dependent stride size to use
> > >   * when it makes i-cache(s) coherent with d-caches.
> > > @@ -718,6 +720,11 @@ get_model_name(__u8 family, __u8 model)
> > >                 printk(KERN_ERR
> > >                        "%s: Table overflow. Some 
> processor model information will be missing\n",
> > >                        __FUNCTION__);
> > > +       /* Montecito has separated L2 Icache and Dcache. 
> This requires
> > > +          synchronize Icache and Dcache before set_pte() */
> > > +       if (family == 0x20)
> > > +               separated_l2_icache_dcache = 1;
> > > +
> > >         return "Unknown";
> > >  }
> > >
> > > Index: linux-2.6.23-rc1.test/include/asm-ia64/pgtable.h
> > > 
> ===================================================================
> > > --- linux-2.6.23-rc1.test.orig/include/asm-ia64/pgtable.h
> > > +++ linux-2.6.23-rc1.test/include/asm-ia64/pgtable.h
> > > @@ -489,9 +489,10 @@ extern struct page *zero_page_memmap_ptr
> > >   * as an executable pte.
> > >   */
> > >  extern void __sync_icache_dcache(pte_t pte);
> > > +extern int separated_l2_icache_dcache;
> > >  static inline void sync_icache_dcache(pte_t pte)
> > >  {
> > > -       if (pte_exec(pte))
> > > +       if (pte_exec(pte) && separated_l2_icache_dcache)
> > >                 __sync_icache_dcache(pte);
> > >  }
> > >  #define __HAVE_ARCH_SYNC_ICACHE_DCACHE
> > >
> > > -
> > > To unsubscribe from this list: send the line "unsubscribe 
> linux-ia64" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > >
> > 
> > 
> > -- 
> > Mosberger Consulting LLC, http://www.mosberger-consulting.com/
> > 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe 
> linux-ia64" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] flush icache before set_pte take6. [3/4] add montecito brand name

[KAMEZAWA Hiroyuki]

On Tue, 31 Jul 2007 09:39:58 -0700
"Luck, Tony" <tony.luck@intel.com> wrote:

> +		} else if (family == 0x20)
> +			memcpy(brand, "Montecito", 10);
> 
> NAK.
> 
> We don't really have names for the different cpu families. "Montecito"
> is definitely not the right string to apply here (Montvale will also
> have family == 0x20).  The old McKinley/Madison strings are intended
> to provide some information for old processors where PAL did not
> support PAL_BRAND_INFO.  All new cpus (from Montecito onward) will
> support this PAL call, so there should be no need to add more code to
> make names out of [family,model] tuples.
> 

Ok, please ignore patch [3/4] and [4/4]. 

-Kame

Re: [PATCH] flush icache before set_pte take6. [4/4] optimization for cpus other than montecito

[Zoltan Menyhart]

Jim Hull wrote:
> Not just crazy, but wrong - this *can* happen on pre-Montecito.  Even though
> L1D is write-through and L2 was mixed I/D, the L1 I-cache could contain
> stale instrutions if there are missing flushes.

I cannot agree with you.

In order to consider an L1 I-cache entry as valid, a corresponding
virtual -> physic address translation should be valid in one of the L1 ITLBs.

"See 6.1.1. Instruction TLBS" of the I2 Proc. Ref. Man. for SW Dev. & Opt.

You cannot have a valid L1 ITLB entry unless you have a corresponding valid
L2 ITLB entry.

When you remove a PTE (or switch off the exec bit) and you flush the L2 ITLB
matching the old translation (and you kill the corresponding L1 ITLBs),
you do invalidate the corresponding L1 I-cache entries.

Therefore CPU models without split L2 caches are safe.

Thanks,

Zoltan

Re: [PATCH] flush icache before set_pte take6. [4/4] optimization for cpus other than montecito

[Zoltan Menyhart]

Luck, Tony wrote:
>>This seems crazy to me.  Flushing should occur according to the
>>*architecture*, not model-by-model.  Even if we happen to get "lucky"
>>on pre-Montecito CPUs, that doesn't justify such ugly hacks.  Or you
>>really want to debug this *again* come next CPU?
> 
> 
> Ditto.  The only reason we should ever have model specific checks should
> be to work around model specific errata (e.g. the McKinley Errata #9 code
> in patch.c).

You do have model specific I cache semantics.
Not taking it into account will oblige you to flush in vain for the models
which do not require it. Why do you want to take this option?


Thanks,

Zoltan

Re: [PATCH] flush icache before set_pte take6. [4/4] optimization for cpus other than montecito

[David Mosberger-Tang]

On 8/1/07, Zoltan Menyhart <Zoltan.Menyhart@bull.net> wrote:

> You do have model specific I cache semantics.
> Not taking it into account will oblige you to flush in vain for the models
> which do not require it. Why do you want to take this option?

Given unlimited resources, your proposal makes perfect sense.  We
could have a Linux version for Merced, one for McKinley, one for
Madison, etc., etc.

(Un)fortunately, resources are limited and with that constraint in
place, rather than spending lots of time optimizing the kernel for
particular idiosyncrasies of a CPU model, it is generally much better
to optimize it for the things the hardware designers promised us would
stay the same across CPU models (i.e., the "architecture").  Sure, it
may mean that on occasion certain things are slightly slower than they
could be but it does have the decided advantage of letting the
maintainers sleep at night... ;-)  Moreover, higher-level
optimizations usually have much higher payoff, so even though you may
do things a bit more slowly at the lowest level, you may end up with a
faster system overall because you were able to spend more time
optimizing at a higher level.

  --david