| Message ID | 20210404153311.1460106-3-yanfei.xu@windriver.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: khugepaged: cleanup and a minor tuning in THP | expand |
On Sun, Apr 4, 2021 at 8:33 AM <yanfei.xu@windriver.com> wrote: > > From: Yanfei Xu <yanfei.xu@windriver.com> > > We could check MMF_DISABLE_THP ahead of iterating over all of vma. > Otherwise if some mm_struct contain a large number of vma, there will > be amounts meaningless cpu cycles cost. > > BTW, drop an unnecessary cond_resched(), because there is a another > cond_resched() followed it and no consumed invocation between them. > > Signed-off-by: Yanfei Xu <yanfei.xu@windriver.com> > --- > mm/khugepaged.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > index 2efe1d0c92ed..c293ec4a94ea 100644 > --- a/mm/khugepaged.c > +++ b/mm/khugepaged.c > @@ -2094,6 +2094,8 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages, > */ > if (unlikely(!mmap_read_trylock(mm))) > goto breakouterloop_mmap_lock; > + if (test_bit(MMF_DISABLE_THP, &mm->flags)) > + goto breakouterloop_mmap_lock; It is fine to check this flag. But mmap_lock has been acquired so you should jump to breakouterloop. > if (likely(!khugepaged_test_exit(mm))) > vma = find_vma(mm, khugepaged_scan.address); > > @@ -2101,7 +2103,6 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages, > for (; vma; vma = vma->vm_next) { > unsigned long hstart, hend; > > - cond_resched(); I don't have a strong opinion for removing this cond_resched(). But IIUC khugepaged is a best effort job there is no harm to keep it IMHO. > if (unlikely(khugepaged_test_exit(mm))) { > progress++; > break; > -- > 2.27.0 > >
On 4/6/21 2:20 AM, Yang Shi wrote: > [Please note: This e-mail is from an EXTERNAL e-mail address] > > On Sun, Apr 4, 2021 at 8:33 AM <yanfei.xu@windriver.com> wrote: >> >> From: Yanfei Xu <yanfei.xu@windriver.com> >> >> We could check MMF_DISABLE_THP ahead of iterating over all of vma. >> Otherwise if some mm_struct contain a large number of vma, there will >> be amounts meaningless cpu cycles cost. >> >> BTW, drop an unnecessary cond_resched(), because there is a another >> cond_resched() followed it and no consumed invocation between them. >> >> Signed-off-by: Yanfei Xu <yanfei.xu@windriver.com> >> --- >> mm/khugepaged.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/mm/khugepaged.c b/mm/khugepaged.c >> index 2efe1d0c92ed..c293ec4a94ea 100644 >> --- a/mm/khugepaged.c >> +++ b/mm/khugepaged.c >> @@ -2094,6 +2094,8 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages, >> */ >> if (unlikely(!mmap_read_trylock(mm))) >> goto breakouterloop_mmap_lock; >> + if (test_bit(MMF_DISABLE_THP, &mm->flags)) >> + goto breakouterloop_mmap_lock; > > It is fine to check this flag. But mmap_lock has been acquired so you > should jump to breakouterloop. Oops! It's my fault. Thank you for pointing out this. Will fix it in v2. > >> if (likely(!khugepaged_test_exit(mm))) >> vma = find_vma(mm, khugepaged_scan.address); >> >> @@ -2101,7 +2103,6 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages, >> for (; vma; vma = vma->vm_next) { >> unsigned long hstart, hend; >> >> - cond_resched(); > > I don't have a strong opinion for removing this cond_resched(). But > IIUC khugepaged is a best effort job there is no harm to keep it IMHO. > Yes, keeping it is no harm. But I think we should add it when we need. Look at the blow codes, there are only some simple check between these two cond_resched(). And we still have some cond_resched() in the khugepaged_scan_file() and khugepaged_scan_pmd() which is the actual wrok about collapsing. So I think it is unnecessary. :) for (; vma; vma = vma->vm_next) { unsigned long hstart, hend; cond_resched(); //here if (unlikely(khugepaged_test_exit(mm))) { progress++; break; } if (!hugepage_vma_check(vma, vma->vm_flags)) { skip: progress++; continue; } hstart = ALIGN(vma->vm_start, HPAGE_PMD_SIZE); hend = ALIGN_DOWN(vma->vm_end, HPAGE_PMD_SIZE); if (hstart >= hend) goto skip; if (khugepaged_scan.address > hend) goto skip; if (khugepaged_scan.address < hstart) khugepaged_scan.address = hstart; VM_BUG_ON(!IS_ALIGNED(khugepaged_scan.address, HPAGE_PMD_SIZE)); if (shmem_file(vma->vm_file) && !shmem_huge_enabled(vma)) goto skip; while (khugepaged_scan.address < hend) { int ret; cond_resched(); //here >> if (unlikely(khugepaged_test_exit(mm))) { >> progress++; >> break; >> -- >> 2.27.0 >> >>
On 4/6/21 10:51 AM, Xu, Yanfei wrote: > > > On 4/6/21 2:20 AM, Yang Shi wrote: >> [Please note: This e-mail is from an EXTERNAL e-mail address] >> >> On Sun, Apr 4, 2021 at 8:33 AM <yanfei.xu@windriver.com> wrote: >>> >>> From: Yanfei Xu <yanfei.xu@windriver.com> >>> >>> We could check MMF_DISABLE_THP ahead of iterating over all of vma. >>> Otherwise if some mm_struct contain a large number of vma, there will >>> be amounts meaningless cpu cycles cost. >>> >>> BTW, drop an unnecessary cond_resched(), because there is a another >>> cond_resched() followed it and no consumed invocation between them. >>> >>> Signed-off-by: Yanfei Xu <yanfei.xu@windriver.com> >>> --- >>> mm/khugepaged.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c >>> index 2efe1d0c92ed..c293ec4a94ea 100644 >>> --- a/mm/khugepaged.c >>> +++ b/mm/khugepaged.c >>> @@ -2094,6 +2094,8 @@ static unsigned int >>> khugepaged_scan_mm_slot(unsigned int pages, >>> */ >>> if (unlikely(!mmap_read_trylock(mm))) >>> goto breakouterloop_mmap_lock; >>> + if (test_bit(MMF_DISABLE_THP, &mm->flags)) >>> + goto breakouterloop_mmap_lock; >> >> It is fine to check this flag. But mmap_lock has been acquired so you >> should jump to breakouterloop. > > Oops! It's my fault. Thank you for pointing out this. > Will fix it in v2. > >> >>> if (likely(!khugepaged_test_exit(mm))) >>> vma = find_vma(mm, khugepaged_scan.address); >>> >>> @@ -2101,7 +2103,6 @@ static unsigned int >>> khugepaged_scan_mm_slot(unsigned int pages, >>> for (; vma; vma = vma->vm_next) { >>> unsigned long hstart, hend; >>> >>> - cond_resched(); >> >> I don't have a strong opinion for removing this cond_resched(). But >> IIUC khugepaged is a best effort job there is no harm to keep it IMHO. >> > > Yes, keeping it is no harm. But I think we should add it when we need. > Look at the blow codes, there are only some simple check between these > two cond_resched(). And we still have some cond_resched() in the > khugepaged_scan_file() and khugepaged_scan_pmd() which is the actual > wrok about collapsing. So I think it is unnecessary. :) > BTW, the original author add this cond_resched() might be worry about the hugepage_vma_check() always return false due to the MMF_DISABLE_THP. But now we have moved it out of the for loop of iterating vma. um.. That is my guess.. Thanks, Yanfei > for (; vma; vma = vma->vm_next) { > unsigned long hstart, hend; > > cond_resched(); //here > if (unlikely(khugepaged_test_exit(mm))) { > progress++; > break; > } > if (!hugepage_vma_check(vma, vma->vm_flags)) { > skip: > progress++; > continue; > } > hstart = ALIGN(vma->vm_start, HPAGE_PMD_SIZE); > hend = ALIGN_DOWN(vma->vm_end, HPAGE_PMD_SIZE); > if (hstart >= hend) > goto skip; > if (khugepaged_scan.address > hend) > goto skip; > if (khugepaged_scan.address < hstart) > khugepaged_scan.address = hstart; > VM_BUG_ON(!IS_ALIGNED(khugepaged_scan.address, > HPAGE_PMD_SIZE)); > > if (shmem_file(vma->vm_file) && !shmem_huge_enabled(vma)) > goto skip; > > while (khugepaged_scan.address < hend) { > int ret; > cond_resched(); //here > > >>> if (unlikely(khugepaged_test_exit(mm))) { >>> progress++; >>> break; >>> -- >>> 2.27.0 >>> >>>
On Mon, Apr 5, 2021 at 8:05 PM Xu, Yanfei <yanfei.xu@windriver.com> wrote: > > > > On 4/6/21 10:51 AM, Xu, Yanfei wrote: > > > > > > On 4/6/21 2:20 AM, Yang Shi wrote: > >> [Please note: This e-mail is from an EXTERNAL e-mail address] > >> > >> On Sun, Apr 4, 2021 at 8:33 AM <yanfei.xu@windriver.com> wrote: > >>> > >>> From: Yanfei Xu <yanfei.xu@windriver.com> > >>> > >>> We could check MMF_DISABLE_THP ahead of iterating over all of vma. > >>> Otherwise if some mm_struct contain a large number of vma, there will > >>> be amounts meaningless cpu cycles cost. > >>> > >>> BTW, drop an unnecessary cond_resched(), because there is a another > >>> cond_resched() followed it and no consumed invocation between them. > >>> > >>> Signed-off-by: Yanfei Xu <yanfei.xu@windriver.com> > >>> --- > >>> mm/khugepaged.c | 3 ++- > >>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c > >>> index 2efe1d0c92ed..c293ec4a94ea 100644 > >>> --- a/mm/khugepaged.c > >>> +++ b/mm/khugepaged.c > >>> @@ -2094,6 +2094,8 @@ static unsigned int > >>> khugepaged_scan_mm_slot(unsigned int pages, > >>> */ > >>> if (unlikely(!mmap_read_trylock(mm))) > >>> goto breakouterloop_mmap_lock; > >>> + if (test_bit(MMF_DISABLE_THP, &mm->flags)) > >>> + goto breakouterloop_mmap_lock; > >> > >> It is fine to check this flag. But mmap_lock has been acquired so you > >> should jump to breakouterloop. > > > > Oops! It's my fault. Thank you for pointing out this. > > Will fix it in v2. > > > >> > >>> if (likely(!khugepaged_test_exit(mm))) > >>> vma = find_vma(mm, khugepaged_scan.address); > >>> > >>> @@ -2101,7 +2103,6 @@ static unsigned int > >>> khugepaged_scan_mm_slot(unsigned int pages, > >>> for (; vma; vma = vma->vm_next) { > >>> unsigned long hstart, hend; > >>> > >>> - cond_resched(); > >> > >> I don't have a strong opinion for removing this cond_resched(). But > >> IIUC khugepaged is a best effort job there is no harm to keep it IMHO. > >> > > > > Yes, keeping it is no harm. But I think we should add it when we need. > > Look at the blow codes, there are only some simple check between these > > two cond_resched(). And we still have some cond_resched() in the > > khugepaged_scan_file() and khugepaged_scan_pmd() which is the actual > > wrok about collapsing. So I think it is unnecessary. :) > > > > BTW, the original author add this cond_resched() might be worry about > the hugepage_vma_check() always return false due to the MMF_DISABLE_THP. > But now we have moved it out of the for loop of iterating vma. A little bit of archeology showed the cond_resched() was there in the first place even before MMF_DISABLE_THP was introduced. > > um.. That is my guess.. > > Thanks, > Yanfei > > > for (; vma; vma = vma->vm_next) { > > unsigned long hstart, hend; > > > > cond_resched(); //here > > if (unlikely(khugepaged_test_exit(mm))) { > > progress++; > > break; > > } > > if (!hugepage_vma_check(vma, vma->vm_flags)) { > > skip: > > progress++; > > continue; > > } > > hstart = ALIGN(vma->vm_start, HPAGE_PMD_SIZE); > > hend = ALIGN_DOWN(vma->vm_end, HPAGE_PMD_SIZE); > > if (hstart >= hend) > > goto skip; > > if (khugepaged_scan.address > hend) > > goto skip; > > if (khugepaged_scan.address < hstart) > > khugepaged_scan.address = hstart; > > VM_BUG_ON(!IS_ALIGNED(khugepaged_scan.address, > > HPAGE_PMD_SIZE)); > > > > if (shmem_file(vma->vm_file) && !shmem_huge_enabled(vma)) > > goto skip; > > > > while (khugepaged_scan.address < hend) { > > int ret; > > cond_resched(); //here > > > > > >>> if (unlikely(khugepaged_test_exit(mm))) { > >>> progress++; > >>> break; > >>> -- > >>> 2.27.0 > >>> > >>>
diff --git a/mm/khugepaged.c b/mm/khugepaged.c index 2efe1d0c92ed..c293ec4a94ea 100644 --- a/mm/khugepaged.c +++ b/mm/khugepaged.c @@ -2094,6 +2094,8 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages, */ if (unlikely(!mmap_read_trylock(mm))) goto breakouterloop_mmap_lock; + if (test_bit(MMF_DISABLE_THP, &mm->flags)) + goto breakouterloop_mmap_lock; if (likely(!khugepaged_test_exit(mm))) vma = find_vma(mm, khugepaged_scan.address); @@ -2101,7 +2103,6 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages, for (; vma; vma = vma->vm_next) { unsigned long hstart, hend; - cond_resched(); if (unlikely(khugepaged_test_exit(mm))) { progress++; break;