| Message ID | 20220228235741.102941-3-shy828301@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Make khugepaged collapse readonly FS THP more consistent | expand |
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@gmail.com> > --- > 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? 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) >
On Tue, Mar 1, 2022 at 1:07 AM Miaohe Lin <linmiaohe@huawei.com> 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@gmail.com> > > --- > > 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) > > > >
On 2022/3/3 2:43, Yang Shi wrote: > On Tue, Mar 1, 2022 at 1:07 AM Miaohe Lin <linmiaohe@huawei.com> 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@gmail.com> >>> --- >>> 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. Sounds reasonable to me. Khugepaged shouldn't collapse VM_NO_KHUGEPAGED vmas. Reviewed-by: Miaohe Lin <linmiaohe@huawei.com> Thanks. > >> >> 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) >>> >> >> > . >
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; + 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)
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@gmail.com> --- mm/khugepaged.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)