Re: [PATCH] fix x86_64-mm-cpa-cache-flush.patch in 2.6.22-rc4-mm2

[Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>]

> Seems that Andi has changed
> ftp://ftp.firstfloor.org/pub/ak/x86_64/quilt/patches/cpa-cache-flush so
> hopefully these fixes will not be needed any more..
> 

Hrm, actually, something is very wrong with this patch. Please see
comments below.


...
> Index: linux/arch/x86_64/mm/pageattr.c
> ===================================================================
> --- linux.orig/arch/x86_64/mm/pageattr.c
> +++ linux/arch/x86_64/mm/pageattr.c
> @@ -234,7 +235,10 @@ void global_flush_tlb(void)
>  	flush_map(&l);
>  
>  	list_for_each_entry_safe(pg, next, &l, lru) {
> +		if (page_private(pg) != 0)
> +			continue;
>  		ClearPagePrivate(pg);
> +		clear_bit(PG_arch_1, &pg->flags);


First issue:

clear_bit should be before the if (page_private(pg) != 0). As a result,
in the current patch, the flush won't be done the second time flags
are changed in a page, because the bit would only be cleared when each
pte within the page are set back to normal page attributes.

I would also suggest putting a list_del before the if (page_private(pg)
!= 0). It is not required, but would make sure the pointers are
poisoned.

>  		__free_page(pg);
>  	} 
>  } 
> Index: linux/arch/i386/mm/pageattr.c
> ===================================================================
> --- linux.orig/arch/i386/mm/pageattr.c
> +++ linux/arch/i386/mm/pageattr.c
> @@ -82,7 +82,7 @@ static void flush_kernel_map(void *arg)
>  	struct page *p;
>  
>  	/* High level code is not ready for clflush yet */
> -	if (0 && cpu_has_clflush) {
> +	if (cpu_has_clflush) {
>  		list_for_each_entry (p, lh, lru)
>  			cache_flush_page(p);
>  	} else if (boot_cpu_data.x86_model >= 4)
> @@ -136,6 +136,12 @@ static inline void revert_page(struct pa
>  			    ref_prot));
>  }
>  
> +static inline void save_page(struct page *kpte_page)
> +{
> +	if (!test_and_set_bit(PG_arch_1, &kpte_page->flags))
> +		list_add(&kpte_page->lru, &df_list);
> +}
> +
>  static int
>  __change_page_attr(struct page *page, pgprot_t prot)
>  { 
> @@ -150,6 +156,9 @@ __change_page_attr(struct page *page, pg
>  	if (!kpte)
>  		return -EINVAL;
>  	kpte_page = virt_to_page(kpte);
> +	BUG_ON(PageLRU(kpte_page));
> +	BUG_ON(PageCompound(kpte_page));
> +
>  	if (pgprot_val(prot) != pgprot_val(PAGE_KERNEL)) { 
>  		if (!pte_huge(*kpte)) {
>  			set_pte_atomic(kpte, mk_pte(page, prot)); 
> @@ -179,11 +188,11 @@ __change_page_attr(struct page *page, pg
>  	 * time (not via split_large_page) and in turn we must not
>  	 * replace it with a largepage.
>  	 */
> +
> +	save_page(kpte_page);
>  	if (!PageReserved(kpte_page)) {
>  		if (cpu_has_pse && (page_private(kpte_page) == 0)) {
> -			ClearPagePrivate(kpte_page);
>  			paravirt_release_pt(page_to_pfn(kpte_page));
> -			list_add(&kpte_page->lru, &df_list);
>  			revert_page(kpte_page, address);
>  		}
>  	}
> @@ -236,6 +245,10 @@ void global_flush_tlb(void)
>  	spin_unlock_irq(&cpa_lock);
>  	flush_map(&l);
>  	list_for_each_entry_safe(pg, next, &l, lru) {
> +		if (page_private(pg) != 0)
> +			continue;
> +		ClearPagePrivate(pg);
> +		clear_bit(PG_arch_1, &pg->flags);

Same two issues pointed for x86_64 applies here: clear_bit and a
list_del should be put before the if statement.

Mathieu

>  		__free_page(pg);
>  	}
>  }


-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68