Re: [v3 PATCH 1/2] mm: thp: make transhuge_vma_suitable available for anonymous THP

[Yang Shi]

On 7/17/19 12:43 PM, Hugh Dickins wrote:
> On Thu, 13 Jun 2019, Yang Shi wrote:
>
> > The transhuge_vma_suitable() was only available for shmem THP, but
> > anonymous THP has the same check except pgoff check.  And, it will be
> > used for THP eligible check in the later patch, so make it available for
> > all kind of THPs.  This also helps reduce code duplication slightly.
> >
> > Since anonymous THP doesn't have to check pgoff, so make pgoff check
> > shmem vma only.
> Yes, I think you are right to avoid the pgoff check on anonymous.
> I had originally thought that it would work out okay even with the
> pgoff check on anonymous, and usually it would: but could give the
> wrong answer on an mremap-moved anonymous area.
>
> > Cc: Hugh Dickins <hughd@xxxxxxxxxx>
> > Cc: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> > Cc: Michal Hocko <mhocko@xxxxxxxx>
> > Cc: Vlastimil Babka <vbabka@xxxxxxx>
> > Cc: David Rientjes <rientjes@xxxxxxxxxx>
> > Signed-off-by: Yang Shi <yang.shi@xxxxxxxxxxxxxxxxx>
> Almost Acked-by me, but there's one nit I'd much prefer to change:
> sorry for being such a late nuisance...
>
> > ---
> >   mm/huge_memory.c |  2 +-
> >   mm/internal.h    | 25 +++++++++++++++++++++++++
> >   mm/memory.c      | 13 -------------
> >   3 files changed, 26 insertions(+), 14 deletions(-)
> >
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 9f8bce9..4bc2552 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -691,7 +691,7 @@ vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf)
> >   	struct page *page;
> >   	unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
> >   
> > -	if (haddr < vma->vm_start || haddr + HPAGE_PMD_SIZE > vma->vm_end)
> > +	if (!transhuge_vma_suitable(vma, haddr))
> >   		return VM_FAULT_FALLBACK;
> >   	if (unlikely(anon_vma_prepare(vma)))
> >   		return VM_FAULT_OOM;
> > diff --git a/mm/internal.h b/mm/internal.h
> > index 9eeaf2b..7f096ba 100644
> > --- a/mm/internal.h
> > +++ b/mm/internal.h
> > @@ -555,4 +555,29 @@ static inline bool is_migrate_highatomic_page(struct page *page)
> >   
> >   void setup_zone_pageset(struct zone *zone);
> >   extern struct page *alloc_new_node_page(struct page *page, unsigned long node);
> > +
> > +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> > +#define HPAGE_CACHE_INDEX_MASK (HPAGE_PMD_NR - 1)
> > +static inline bool transhuge_vma_suitable(struct vm_area_struct *vma,
> > +		unsigned long haddr)
> > +{
> > +	/* Don't have to check pgoff for anonymous vma */
> > +	if (!vma_is_anonymous(vma)) {
> > +		if (((vma->vm_start >> PAGE_SHIFT) & HPAGE_CACHE_INDEX_MASK) !=
> > +			(vma->vm_pgoff & HPAGE_CACHE_INDEX_MASK))
> > +			return false;
> > +	}
> > +
> > +	if (haddr < vma->vm_start || haddr + HPAGE_PMD_SIZE > vma->vm_end)
> > +		return false;
> > +	return true;
> > +}
> > +#else
> > +static inline bool transhuge_vma_suitable(struct vma_area_struct *vma,
> > +		unsigned long haddr)
> > +{
> > +	return false;
> > +}
> > +#endif
> > +
> >   #endif	/* __MM_INTERNAL_H */
> ... maybe I'm just not much of a fan of mm/internal.h (where at last you
> find odd bits and pieces which you had expected to find elsewhere), and
> maybe others will disagree: but I'd say transhuge_vma_suitable() surely
> belongs in include/linux/huge_mm.h, near __transparent_hugepage_enabled().
>
> But then your correct use of vma_is_anonymous() gets more complicated:
> because that declaration is over in include/linux/mm.h; and although
> linux/mm.h includes linux/huge_mm.h, vma_is_anonymous() comes lower down.
>
> However... linux/mm.h's definition of vma_set_anonymous() comes higher
> up, and it would make perfect sense to move vma_is_anonymous up to just
> after vma_set_anonymous(), wouldn't it?  Should vma_is_shmem() and
> vma_is_stack_for_current() declarations move with it? Probably yes:
> they make more sense near vma_is_anonymous() than where they were.

Thanks for the thorough instructions. Will fix this in v4.

> Hugh
>
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 96f1d47..2286424 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -3205,19 +3205,6 @@ static vm_fault_t pte_alloc_one_map(struct vm_fault *vmf)
> >   }
> >   
> >   #ifdef CONFIG_TRANSPARENT_HUGE_PAGECACHE
> > -
> > -#define HPAGE_CACHE_INDEX_MASK (HPAGE_PMD_NR - 1)
> > -static inline bool transhuge_vma_suitable(struct vm_area_struct *vma,
> > -		unsigned long haddr)
> > -{
> > -	if (((vma->vm_start >> PAGE_SHIFT) & HPAGE_CACHE_INDEX_MASK) !=
> > -			(vma->vm_pgoff & HPAGE_CACHE_INDEX_MASK))
> > -		return false;
> > -	if (haddr < vma->vm_start || haddr + HPAGE_PMD_SIZE > vma->vm_end)
> > -		return false;
> > -	return true;
> > -}
> > -
> >   static void deposit_prealloc_pte(struct vm_fault *vmf)
> >   {
> >   	struct vm_area_struct *vma = vmf->vma;
> > --
> > 1.8.3.1