Re: [PATCH v2 3/5] mm/huge_memory.c: add missing read-only THP checking in transparent_hugepage_enabled()

[Yang Shi]

On Thu, Apr 29, 2021 at 7:57 AM David Hildenbrand <david@xxxxxxxxxx> wrote:
>
> On 29.04.21 15:26, Miaohe Lin wrote:
> > Since commit 99cb0dbd47a1 ("mm,thp: add read-only THP support for
> > (non-shmem) FS"), read-only THP file mapping is supported. But it
> > forgot to add checking for it in transparent_hugepage_enabled().
> > To fix it, we add checking for read-only THP file mapping and also
> > introduce helper transhuge_vma_enabled() to check whether thp is
> > enabled for specified vma to reduce duplicated code.
> >
> > Fixes: 99cb0dbd47a1 ("mm,thp: add read-only THP support for (non-shmem) FS")
> > Signed-off-by: Miaohe Lin <linmiaohe@xxxxxxxxxx>
> > ---
> >   include/linux/huge_mm.h | 21 +++++++++++++++++----
> >   mm/huge_memory.c        |  6 ++++++
> >   mm/khugepaged.c         |  4 +---
> >   mm/shmem.c              |  3 +--
> >   4 files changed, 25 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> > index 0a526f211fec..f460b74619fc 100644
> > --- a/include/linux/huge_mm.h
> > +++ b/include/linux/huge_mm.h
> > @@ -115,6 +115,16 @@ extern struct kobj_attribute shmem_enabled_attr;
> >
> >   extern unsigned long transparent_hugepage_flags;
> >
> > +static inline bool transhuge_vma_enabled(struct vm_area_struct *vma,
> > +                                       unsigned long vm_flags)
>
> You're passing the vma already, why do you pass vma->vm_flags
> separately? It's sufficient to pass in the vma only.

I do agree.

>
> > +{
> > +     /* Explicitly disabled through madvise. */
> > +     if ((vm_flags & VM_NOHUGEPAGE) ||
> > +         test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
> > +             return false;
> > +     return true;
> > +}
> > +
> >   /*
> >    * to be used on vmas which are known to support THP.
> >    * Use transparent_hugepage_enabled otherwise
> > @@ -128,15 +138,12 @@ static inline bool __transparent_hugepage_enabled(struct vm_area_struct *vma)
> >       if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_NEVER_DAX))
> >               return false;
> >
> > -     if (vma->vm_flags & VM_NOHUGEPAGE)
> > +     if (!transhuge_vma_enabled(vma, vma->vm_flags))
> >               return false;
> >
> >       if (vma_is_temporary_stack(vma))
> >               return false;
> >
> > -     if (test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
> > -             return false;
> > -
> >       if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_FLAG))
> >               return true;
> >
> > @@ -362,6 +369,12 @@ static inline bool transhuge_vma_suitable(struct vm_area_struct *vma,
> >       return false;
> >   }
> >
> > +static inline bool transhuge_vma_enabled(struct vm_area_struct *vma,
> > +                                       unsigned long vm_flags)
> > +{
> > +     return false;
> > +}
> > +
> >   static inline void prep_transhuge_page(struct page *page) {}
> >
> >   static inline bool is_transparent_hugepage(struct page *page)
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 76ca1eb2a223..e24a96de2e37 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -68,12 +68,18 @@ bool transparent_hugepage_enabled(struct vm_area_struct *vma)
> >       /* The addr is used to check if the vma size fits */
> >       unsigned long addr = (vma->vm_end & HPAGE_PMD_MASK) - HPAGE_PMD_SIZE;
> >
> > +     if (!transhuge_vma_enabled(vma, vma->vm_flags))
> > +             return false;
> >       if (!transhuge_vma_suitable(vma, addr))
> >               return false;
> >       if (vma_is_anonymous(vma))
> >               return __transparent_hugepage_enabled(vma);
> >       if (vma_is_shmem(vma))
> >               return shmem_huge_enabled(vma);
> > +     if (IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) && vma->vm_file &&
> > +         !inode_is_open_for_write(vma->vm_file->f_inode) &&
> > +         (vma->vm_flags & VM_EXEC))
> > +             return true;
>
> Nit: I'm really wondering why we have 3 different functions that sound
> like they are doing the same thing
>
> transparent_hugepage_enabled(vma)

This one is called by smap code only which does check everything (for
both anonymous and shmem) to decide if allocating THP is possible.
Miaohe Lin's patch adds check for readonly FS THP.

> transhuge_vma_enabled()

This is the helper added by Miaohe Lin in this patch. It checks
VM_NOHUGEPAGE and MMF_DISABLE flags. It helps eliminate some duplicate
code. And this check is called at a couple of different places.

> transhuge_vma_suitable()

This one checks if vma is aligned properly. It may be better to rename
it to transhuge_vma_aligned(). It seems this check is just called by
page fault handler.

>
> Which check belongs where? Does it really have to be that complicated?
>
> >
> >       return false;
> >   }
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index 6c0185fdd815..d97b20fad6e8 100644
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -442,9 +442,7 @@ static inline int khugepaged_test_exit(struct mm_struct *mm)
> >   static bool hugepage_vma_check(struct vm_area_struct *vma,
> >                              unsigned long vm_flags)
> >   {
> > -     /* Explicitly disabled through madvise. */
> > -     if ((vm_flags & VM_NOHUGEPAGE) ||
> > -         test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
> > +     if (!transhuge_vma_enabled(vma, vm_flags))
> >               return false;
> >
> >       /* Enabled via shmem mount options or sysfs settings. */
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index a08cedefbfaa..1dcbec313c70 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -4032,8 +4032,7 @@ bool shmem_huge_enabled(struct vm_area_struct *vma)
> >       loff_t i_size;
> >       pgoff_t off;
> >
> > -     if ((vma->vm_flags & VM_NOHUGEPAGE) ||
> > -         test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
> > +     if (!transhuge_vma_enabled(vma, vma->vm_flags))
> >               return false;
> >       if (shmem_huge == SHMEM_HUGE_FORCE)
> >               return true;
> >
>
>
> --
> Thanks,
>
> David / dhildenb
>
>