| Message ID | 20220510203222.24246-7-shy828301@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Make khugepaged collapse readonly FS THP more consistent | expand |
On Tue, 10 May 2022 13:32:20 -0700 Yang Shi <shy828301@gmail.com> wrote: > The hugepage_vma_check() could be reused by khugepaged_enter() and > khugepaged_enter_vma_merge(), but it is static in khugepaged.c. > Make it non-static and declare it in khugepaged.h. > > .. > > @@ -508,20 +508,13 @@ void __khugepaged_enter(struct mm_struct *mm) > void khugepaged_enter_vma_merge(struct vm_area_struct *vma, > unsigned long vm_flags) > { > - unsigned long hstart, hend; > - > - /* > - * khugepaged only supports read-only files for non-shmem files. > - * khugepaged does not yet work on special mappings. And > - * file-private shmem THP is not supported. > - */ > - if (!hugepage_vma_check(vma, vm_flags)) > - return; > - > - hstart = (vma->vm_start + ~HPAGE_PMD_MASK) & HPAGE_PMD_MASK; > - hend = vma->vm_end & HPAGE_PMD_MASK; > - if (hstart < hend) > - khugepaged_enter(vma, vm_flags); > + if (!test_bit(MMF_VM_HUGEPAGE, &vma->vm_mm->flags) && > + khugepaged_enabled() && > + (((vma->vm_start + ~HPAGE_PMD_MASK) & HPAGE_PMD_MASK) < > + (vma->vm_end & HPAGE_PMD_MASK))) { Reviewing these bounds-checking tests is so hard :( Can we simplify? > + if (hugepage_vma_check(vma, vm_flags)) > + __khugepaged_enter(vma->vm_mm); > + } > } void khugepaged_enter_vma(struct vm_area_struct *vma, unsigned long vm_flags) { if (test_bit(MMF_VM_HUGEPAGE, &vma->vm_mm->flags)) return; if (!khugepaged_enabled()) return; if (round_up(vma->vm_start, HPAGE_PMD_SIZE) >= (vma->vm_end & HPAGE_PMD_MASK)) return; /* vma is too small */ if (!hugepage_vma_check(vma, vm_flags)) return; __khugepaged_enter(vma->vm_mm); } Also, it might be slightly faster to have checked MMF_VM_HUGEPAGE before khugepaged_enabled(), but it looks odd. And it might be slower, too - more pointer chasing. I wish someone would document hugepage_vma_check().
On Tue, May 10, 2022 at 2:05 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Tue, 10 May 2022 13:32:20 -0700 Yang Shi <shy828301@gmail.com> wrote: > > > The hugepage_vma_check() could be reused by khugepaged_enter() and > > khugepaged_enter_vma_merge(), but it is static in khugepaged.c. > > Make it non-static and declare it in khugepaged.h. > > > > .. > > > > @@ -508,20 +508,13 @@ void __khugepaged_enter(struct mm_struct *mm) > > void khugepaged_enter_vma_merge(struct vm_area_struct *vma, > > unsigned long vm_flags) > > { > > - unsigned long hstart, hend; > > - > > - /* > > - * khugepaged only supports read-only files for non-shmem files. > > - * khugepaged does not yet work on special mappings. And > > - * file-private shmem THP is not supported. > > - */ > > - if (!hugepage_vma_check(vma, vm_flags)) > > - return; > > - > > - hstart = (vma->vm_start + ~HPAGE_PMD_MASK) & HPAGE_PMD_MASK; > > - hend = vma->vm_end & HPAGE_PMD_MASK; > > - if (hstart < hend) > > - khugepaged_enter(vma, vm_flags); > > + if (!test_bit(MMF_VM_HUGEPAGE, &vma->vm_mm->flags) && > > + khugepaged_enabled() && > > + (((vma->vm_start + ~HPAGE_PMD_MASK) & HPAGE_PMD_MASK) < > > + (vma->vm_end & HPAGE_PMD_MASK))) { > > Reviewing these bounds-checking tests is so hard :( Can we simplify? Yeah, I think they can be moved into a helper with a more descriptive name. > > > + if (hugepage_vma_check(vma, vm_flags)) > > + __khugepaged_enter(vma->vm_mm); > > + } > > } > > void khugepaged_enter_vma(struct vm_area_struct *vma, > unsigned long vm_flags) > { > if (test_bit(MMF_VM_HUGEPAGE, &vma->vm_mm->flags)) > return; > if (!khugepaged_enabled()) > return; > if (round_up(vma->vm_start, HPAGE_PMD_SIZE) >= > (vma->vm_end & HPAGE_PMD_MASK)) > return; /* vma is too small */ > if (!hugepage_vma_check(vma, vm_flags)) > return; > __khugepaged_enter(vma->vm_mm); > } > > > Also, it might be slightly faster to have checked MMF_VM_HUGEPAGE > before khugepaged_enabled(), but it looks odd. And it might be slower, > too - more pointer chasing. I think most configurations have always or madvise mode set (khugepaged_enabled() return true), so having checked MMF_VM_HUGEPAGE before khugepaged_enabled() seems slightly better, but anyway it should not have measurable effect IMHO. > > I wish someone would document hugepage_vma_check(). I will clean up all the stuff further in a new patchset, for example, trying to consolidate all the different hugepage_suitable/enabled/active checks.
diff --git a/include/linux/khugepaged.h b/include/linux/khugepaged.h index 0423d3619f26..c340f6bb39d6 100644 --- a/include/linux/khugepaged.h +++ b/include/linux/khugepaged.h @@ -3,8 +3,6 @@ #define _LINUX_KHUGEPAGED_H #include <linux/sched/coredump.h> /* MMF_VM_HUGEPAGE */ -#include <linux/shmem_fs.h> - #ifdef CONFIG_TRANSPARENT_HUGEPAGE extern struct attribute_group khugepaged_attr_group; @@ -12,6 +10,8 @@ extern struct attribute_group khugepaged_attr_group; extern int khugepaged_init(void); extern void khugepaged_destroy(void); extern int start_stop_khugepaged(void); +extern bool hugepage_vma_check(struct vm_area_struct *vma, + unsigned long vm_flags); extern void __khugepaged_enter(struct mm_struct *mm); extern void __khugepaged_exit(struct mm_struct *mm); extern void khugepaged_enter_vma_merge(struct vm_area_struct *vma, @@ -55,13 +55,11 @@ static inline void khugepaged_exit(struct mm_struct *mm) static inline void khugepaged_enter(struct vm_area_struct *vma, unsigned long vm_flags) { - if (!test_bit(MMF_VM_HUGEPAGE, &vma->vm_mm->flags)) - if ((khugepaged_always() || - (shmem_file(vma->vm_file) && shmem_huge_enabled(vma)) || - (khugepaged_req_madv() && (vm_flags & VM_HUGEPAGE))) && - !(vm_flags & VM_NOHUGEPAGE) && - !test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags)) + if (!test_bit(MMF_VM_HUGEPAGE, &vma->vm_mm->flags) && + khugepaged_enabled()) { + if (hugepage_vma_check(vma, vm_flags)) __khugepaged_enter(vma->vm_mm); + } } #else /* CONFIG_TRANSPARENT_HUGEPAGE */ static inline void khugepaged_fork(struct mm_struct *mm, struct mm_struct *oldmm) diff --git a/mm/khugepaged.c b/mm/khugepaged.c index 7815218ab960..dec449339964 100644 --- a/mm/khugepaged.c +++ b/mm/khugepaged.c @@ -437,8 +437,8 @@ static inline int khugepaged_test_exit(struct mm_struct *mm) return atomic_read(&mm->mm_users) == 0; } -static bool hugepage_vma_check(struct vm_area_struct *vma, - unsigned long vm_flags) +bool hugepage_vma_check(struct vm_area_struct *vma, + unsigned long vm_flags) { if (!transhuge_vma_enabled(vma, vm_flags)) return false; @@ -508,20 +508,13 @@ void __khugepaged_enter(struct mm_struct *mm) void khugepaged_enter_vma_merge(struct vm_area_struct *vma, unsigned long vm_flags) { - unsigned long hstart, hend; - - /* - * khugepaged only supports read-only files for non-shmem files. - * khugepaged does not yet work on special mappings. And - * file-private shmem THP is not supported. - */ - if (!hugepage_vma_check(vma, vm_flags)) - return; - - hstart = (vma->vm_start + ~HPAGE_PMD_MASK) & HPAGE_PMD_MASK; - hend = vma->vm_end & HPAGE_PMD_MASK; - if (hstart < hend) - khugepaged_enter(vma, vm_flags); + if (!test_bit(MMF_VM_HUGEPAGE, &vma->vm_mm->flags) && + khugepaged_enabled() && + (((vma->vm_start + ~HPAGE_PMD_MASK) & HPAGE_PMD_MASK) < + (vma->vm_end & HPAGE_PMD_MASK))) { + if (hugepage_vma_check(vma, vm_flags)) + __khugepaged_enter(vma->vm_mm); + } } void __khugepaged_exit(struct mm_struct *mm)
The hugepage_vma_check() could be reused by khugepaged_enter() and khugepaged_enter_vma_merge(), but it is static in khugepaged.c. Make it non-static and declare it in khugepaged.h. Suggested-by: Vlastimil Babka <vbabka@suse.cz> Signed-off-by: Yang Shi <shy828301@gmail.com> --- include/linux/khugepaged.h | 14 ++++++-------- mm/khugepaged.c | 25 +++++++++---------------- 2 files changed, 15 insertions(+), 24 deletions(-)