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.
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
