| Message ID | 1560401041-32207-2-git-send-email-yang.shi@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix false negative of shmem vma's THP eligibility | expand |
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. 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
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
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 */ 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;
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. 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> --- mm/huge_memory.c | 2 +- mm/internal.h | 25 +++++++++++++++++++++++++ mm/memory.c | 13 ------------- 3 files changed, 26 insertions(+), 14 deletions(-)