Re: [PATCH v5 06/11] thp: change split_huge_page_pmd() interface

["Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5713 bytes --]

On Wed, Nov 14, 2012 at 03:22:03PM -0800, David Rientjes wrote:
> On Wed, 7 Nov 2012, Kirill A. Shutemov wrote:
> 
> > diff --git a/Documentation/vm/transhuge.txt b/Documentation/vm/transhuge.txt
> > index f734bb2..677a599 100644
> > --- a/Documentation/vm/transhuge.txt
> > +++ b/Documentation/vm/transhuge.txt
> > @@ -276,7 +276,7 @@ unaffected. libhugetlbfs will also work fine as usual.
> >  == Graceful fallback ==
> >  
> >  Code walking pagetables but unware about huge pmds can simply call
> > -split_huge_page_pmd(mm, pmd) where the pmd is the one returned by
> > +split_huge_page_pmd(vma, pmd, addr) where the pmd is the one returned by
> >  pmd_offset. It's trivial to make the code transparent hugepage aware
> >  by just grepping for "pmd_offset" and adding split_huge_page_pmd where
> >  missing after pmd_offset returns the pmd. Thanks to the graceful
> > @@ -299,7 +299,7 @@ diff --git a/mm/mremap.c b/mm/mremap.c
> >  		return NULL;
> >  
> >  	pmd = pmd_offset(pud, addr);
> > -+	split_huge_page_pmd(mm, pmd);
> > ++	split_huge_page_pmd(vma, pmd, addr);
> >  	if (pmd_none_or_clear_bad(pmd))
> >  		return NULL;
> >  
> > diff --git a/arch/x86/kernel/vm86_32.c b/arch/x86/kernel/vm86_32.c
> > index 5c9687b..1dfe69c 100644
> > --- a/arch/x86/kernel/vm86_32.c
> > +++ b/arch/x86/kernel/vm86_32.c
> > @@ -182,7 +182,7 @@ static void mark_screen_rdonly(struct mm_struct *mm)
> >  	if (pud_none_or_clear_bad(pud))
> >  		goto out;
> >  	pmd = pmd_offset(pud, 0xA0000);
> > -	split_huge_page_pmd(mm, pmd);
> > +	split_huge_page_pmd_mm(mm, 0xA0000, pmd);
> >  	if (pmd_none_or_clear_bad(pmd))
> >  		goto out;
> >  	pte = pte_offset_map_lock(mm, pmd, 0xA0000, &ptl);
> 
> Why not be consistent and make this split_huge_page_pmd_mm(mm, pmd, addr)?
> 
> > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > index 90c63f9..291a0d1 100644
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> > @@ -643,7 +643,7 @@ static int clear_refs_pte_range(pmd_t *pmd, unsigned long addr,
> >  	spinlock_t *ptl;
> >  	struct page *page;
> >  
> > -	split_huge_page_pmd(walk->mm, pmd);
> > +	split_huge_page_pmd(vma, addr, pmd);
> 
> Ah, it's because the change to the documentation is wrong: the format is 
> actually split_huge_page_pmd(vma, addr, pmd).

Thanks, will fix.

> >  	if (pmd_trans_unstable(pmd))
> >  		return 0;
> >  
> > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> > index b31cb7d..856f080 100644
> > --- a/include/linux/huge_mm.h
> > +++ b/include/linux/huge_mm.h
> > @@ -91,12 +91,14 @@ extern int handle_pte_fault(struct mm_struct *mm,
> >  			    struct vm_area_struct *vma, unsigned long address,
> >  			    pte_t *pte, pmd_t *pmd, unsigned int flags);
> >  extern int split_huge_page(struct page *page);
> > -extern void __split_huge_page_pmd(struct mm_struct *mm, pmd_t *pmd);
> > -#define split_huge_page_pmd(__mm, __pmd)				\
> > +extern void __split_huge_page_pmd(struct vm_area_struct *vma,
> > +		unsigned long address, pmd_t *pmd);
> > +#define split_huge_page_pmd(__vma, __address, __pmd)			\
> >  	do {								\
> >  		pmd_t *____pmd = (__pmd);				\
> >  		if (unlikely(pmd_trans_huge(*____pmd)))			\
> > -			__split_huge_page_pmd(__mm, ____pmd);		\
> > +			__split_huge_page_pmd(__vma, __address,		\
> > +					____pmd);			\
> >  	}  while (0)
> >  #define wait_split_huge_page(__anon_vma, __pmd)				\
> >  	do {								\
> > @@ -106,6 +108,8 @@ extern void __split_huge_page_pmd(struct mm_struct *mm, pmd_t *pmd);
> >  		BUG_ON(pmd_trans_splitting(*____pmd) ||			\
> >  		       pmd_trans_huge(*____pmd));			\
> >  	} while (0)
> > +extern void split_huge_page_pmd_mm(struct mm_struct *mm, unsigned long address,
> > +		pmd_t *pmd);
> >  #if HPAGE_PMD_ORDER > MAX_ORDER
> >  #error "hugepages can't be allocated by the buddy allocator"
> >  #endif
> > @@ -173,10 +177,12 @@ static inline int split_huge_page(struct page *page)
> >  {
> >  	return 0;
> >  }
> > -#define split_huge_page_pmd(__mm, __pmd)	\
> > +#define split_huge_page_pmd(__vma, __address, __pmd)	\
> >  	do { } while (0)
> >  #define wait_split_huge_page(__anon_vma, __pmd)	\
> >  	do { } while (0)
> > +#define split_huge_page_pmd_mm(__mm, __address, __pmd)	\
> > +	do { } while (0)
> >  #define compound_trans_head(page) compound_head(page)
> >  static inline int hugepage_madvise(struct vm_area_struct *vma,
> >  				   unsigned long *vm_flags, int advice)
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 05490b3..90e651c 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -2509,19 +2509,23 @@ static int khugepaged(void *none)
> >  	return 0;
> >  }
> >  
> > -void __split_huge_page_pmd(struct mm_struct *mm, pmd_t *pmd)
> > +void __split_huge_page_pmd(struct vm_area_struct *vma, unsigned long address,
> > +		pmd_t *pmd)
> >  {
> >  	struct page *page;
> > +	unsigned long haddr = address & HPAGE_PMD_MASK;
> > 
> 
> Just do
> 
> 	struct mm_struct *mm = vma->vm_mm;
> 
> here and it makes everything else simpler.

Okay.

> > -	spin_lock(&mm->page_table_lock);
> > +	BUG_ON(vma->vm_start > haddr || vma->vm_end < haddr + HPAGE_PMD_SIZE);
> > +
> > +	spin_lock(&vma->vm_mm->page_table_lock);
> >  	if (unlikely(!pmd_trans_huge(*pmd))) {
> > -		spin_unlock(&mm->page_table_lock);
> > +		spin_unlock(&vma->vm_mm->page_table_lock);
> >  		return;
> >  	}
> >  	page = pmd_page(*pmd);
> >  	VM_BUG_ON(!page_count(page));
> >  	get_page(page);
> > -	spin_unlock(&mm->page_table_lock);
> > +	spin_unlock(&vma->vm_mm->page_table_lock);
> >  
> >  	split_huge_page(page);
> >  

-- 
 Kirill A. Shutemov

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]