Re: [patch 3/5]thp: add tlb_remove_pmd_tlb_entry

[Andrea Arcangeli]

On Tue, Oct 25, 2011 at 10:59:31AM +0800, Shaohua Li wrote:
> Index: linux/arch/x86/include/asm/tlb.h
> ===================================================================
> --- linux.orig/arch/x86/include/asm/tlb.h	2011-10-25 09:00:39.000000000 +0800
> +++ linux/arch/x86/include/asm/tlb.h	2011-10-25 09:02:52.000000000 +0800
> @@ -4,6 +4,7 @@
>  #define tlb_start_vma(tlb, vma) do { } while (0)
>  #define tlb_end_vma(tlb, vma) do { } while (0)
>  #define __tlb_remove_tlb_entry(tlb, ptep, address) do { } while (0)
> +#define __tlb_remove_pmd_tlb_entry(tlb, pmdp, address) do { } while (0)
>  #define tlb_flush(tlb) flush_tlb_mm((tlb)->mm)

This is superfluous, it's already define below as noop.

>  
>  #include <asm-generic/tlb.h>
> Index: linux/include/asm-generic/tlb.h
> ===================================================================
> --- linux.orig/include/asm-generic/tlb.h	2011-10-25 09:00:23.000000000 +0800
> +++ linux/include/asm-generic/tlb.h	2011-10-25 09:18:01.000000000 +0800
> @@ -139,6 +139,16 @@ static inline void tlb_remove_page(struc
>  		__tlb_remove_tlb_entry(tlb, ptep, address);	\
>  	} while (0)
>  
> +#ifndef __tlb_remove_pmd_tlb_entry
> +#define __tlb_remove_pmd_tlb_entry(tlb, pmdp, address) do {} while(0)
> +#endif
> +
> +#define tlb_remove_pmd_tlb_entry(tlb, pmdp, address)		\
> +	do {							\
> +		tlb->need_flush = 1;				\
> +		__tlb_remove_pmd_tlb_entry(tlb, pmdp, address);	\
> +	} while (0)

this looks weird, why do we set need_flush = 1 again, considering that
we're doing tlb_remove_page() just a few lines later (which also sets
tlb->need_flush = 1).

Ok that other archs may need the __tlb_remove_pmd_tlb_entry to be
called (and I've no idea why), but the need_flush = 1 seems
unnecessary.

Why other archs need the __tlb_remove_pmd_tlb_entry to be called?

One way to go would be to change the tlb->need_flush = 1 in
__tlb_remove_page to a VM_BUG_ON(!tlb->need_flush) and then we keep it
above and we add the __tlb_remove_pmd_tlb_entry call.

Or is there any place where __tlb_remove_page is called without a
tlb_remove_*tlb_entry being called before it?

In any case the VM_BUG_ON will verify this.

> Index: linux/include/linux/huge_mm.h
> ===================================================================
> --- linux.orig/include/linux/huge_mm.h	2011-10-25 09:07:12.000000000 +0800
> +++ linux/include/linux/huge_mm.h	2011-10-25 09:07:44.000000000 +0800
> @@ -18,7 +18,7 @@ extern struct page *follow_trans_huge_pm
>  					  unsigned int flags);
>  extern int zap_huge_pmd(struct mmu_gather *tlb,
>  			struct vm_area_struct *vma,
> -			pmd_t *pmd);
> +			pmd_t *pmd, unsigned long addr);
>  extern int mincore_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
>  			unsigned long addr, unsigned long end,
>  			unsigned char *vec);
> Index: linux/mm/huge_memory.c
> ===================================================================
> --- linux.orig/mm/huge_memory.c	2011-10-25 09:00:07.000000000 +0800
> +++ linux/mm/huge_memory.c	2011-10-25 09:06:55.000000000 +0800
> @@ -1005,7 +1005,7 @@ out:
>  }
>  
>  int zap_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
> -		 pmd_t *pmd)
> +		 pmd_t *pmd, unsigned long addr)
>  {
>  	int ret = 0;
>  
> @@ -1021,6 +1021,7 @@ int zap_huge_pmd(struct mmu_gather *tlb,
>  			pgtable = get_pmd_huge_pte(tlb->mm);
>  			page = pmd_page(*pmd);
>  			pmd_clear(pmd);
> +			tlb_remove_pmd_tlb_entry(tlb, pmd, addr);
>  			page_remove_rmap(page);
>  			VM_BUG_ON(page_mapcount(page) < 0);
>  			add_mm_counter(tlb->mm, MM_ANONPAGES, -HPAGE_PMD_NR);
> Index: linux/mm/memory.c
> ===================================================================
> --- linux.orig/mm/memory.c	2011-10-25 09:07:49.000000000 +0800
> +++ linux/mm/memory.c	2011-10-25 09:08:29.000000000 +0800
> @@ -1231,7 +1231,7 @@ static inline unsigned long zap_pmd_rang
>  			if (next-addr != HPAGE_PMD_SIZE) {
>  				VM_BUG_ON(!rwsem_is_locked(&tlb->mm->mmap_sem));
>  				split_huge_page_pmd(vma->vm_mm, pmd);
> -			} else if (zap_huge_pmd(tlb, vma, pmd))
> +			} else if (zap_huge_pmd(tlb, vma, pmd, addr))
>  				continue;
>  			/* fall through */
>  		}

Rest looks ok.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/