Messages in this thread | | | Subject | Re: [v3 PATCH 1/2] mm: thp: make transhuge_vma_suitable available for anonymous THP | From | Yang Shi <> | Date | Wed, 17 Jul 2019 14:03:55 -0700 |
| |
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@google.com> >> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> >> Cc: Michal Hocko <mhocko@suse.com> >> Cc: Vlastimil Babka <vbabka@suse.cz> >> Cc: David Rientjes <rientjes@google.com> >> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> > 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
| |