Re: [PATCH 2/8] mm: khugepaged: remove redundant check for VM_NO_KHUGEPAGED

[Yang Shi]

On Tue, Mar 1, 2022 at 1:07 AM Miaohe Lin <linmiaohe@xxxxxxxxxx> wrote:
>
> On 2022/3/1 7:57, Yang Shi wrote:
> > The hugepage_vma_check() called by khugepaged_enter_vma_merge() does
> > check VM_NO_KHUGEPAGED. Remove the check from caller and move the check
> > in hugepage_vma_check() up.
> >
> > More checks may be run for VM_NO_KHUGEPAGED vmas, but MADV_HUGEPAGE is
> > definitely not a hot path, so cleaner code does outweigh.
> >
> > Signed-off-by: Yang Shi <shy828301@xxxxxxxxx>
> > ---
> >  mm/khugepaged.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index 131492fd1148..82c71c6da9ce 100644
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -366,8 +366,7 @@ int hugepage_madvise(struct vm_area_struct *vma,
> >                * register it here without waiting a page fault that
> >                * may not happen any time soon.
> >                */
> > -             if (!(*vm_flags & VM_NO_KHUGEPAGED) &&
> > -                             khugepaged_enter_vma_merge(vma, *vm_flags))
> > +             if (khugepaged_enter_vma_merge(vma, *vm_flags))
> >                       return -ENOMEM;
> >               break;
> >       case MADV_NOHUGEPAGE:
> > @@ -446,6 +445,9 @@ static bool hugepage_vma_check(struct vm_area_struct *vma,
> >       if (!transhuge_vma_enabled(vma, vm_flags))
> >               return false;
> >
> > +     if (vm_flags & VM_NO_KHUGEPAGED)
> > +             return false;
> > +
>
> This patch does improve the readability. But I have a question.
> It seems VM_NO_KHUGEPAGED is not checked in the below if-condition:
>
>         /* Only regular file is valid */
>         if (IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) && vma->vm_file &&
>             (vm_flags & VM_EXEC)) {
>                 struct inode *inode = vma->vm_file->f_inode;
>
>                 return !inode_is_open_for_write(inode) &&
>                         S_ISREG(inode->i_mode);
>         }
>
> If we return false due to VM_NO_KHUGEPAGED here, it seems it will affect the
> return value of this CONFIG_READ_ONLY_THP_FOR_FS condition check.
> Or am I miss something?

Yes, it will return false instead of true if that file THP check is
true, but wasn't that old behavior actually problematic? Khugepaged
definitely can't collapse VM_NO_KHUGEPAGED vmas even though it
satisfies all the readonly file THP checks. With the old behavior
khugepaged may scan an exec file hugetlb vma IIUC although it will
fail later due to other page sanity checks, i.e. page compound check.

>
> Thanks.
>
> >       if (vma->vm_file && !IS_ALIGNED((vma->vm_start >> PAGE_SHIFT) -
> >                               vma->vm_pgoff, HPAGE_PMD_NR))
> >               return false;
> > @@ -471,7 +473,8 @@ static bool hugepage_vma_check(struct vm_area_struct *vma,
> >               return false;
> >       if (vma_is_temporary_stack(vma))
> >               return false;
> > -     return !(vm_flags & VM_NO_KHUGEPAGED);
> > +
> > +     return true;
> >  }
> >
> >  int __khugepaged_enter(struct mm_struct *mm)
> >
>
>