Messages in this thread | | | Subject | Re: [PATCH 2/2] mm: khugepaged: check MMF_DISABLE_THP ahead of iterating over vmas | From | "Xu, Yanfei" <> | Date | Tue, 6 Apr 2021 11:04:53 +0800 |
| |
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 >>> >>>
| |