| Message ID | 20241216165105.56185-2-dev.jain@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | khugepaged: Asynchronous mTHP collapse | expand |
On Mon, Dec 16, 2024 at 10:20:54PM +0530, Dev Jain wrote: > -static int hpage_collapse_scan_pmd(struct mm_struct *mm, > +static int hpage_collapse_scan_ptes(struct mm_struct *mm, i don't think this is necessary at all. you're scanning a pmd. you might not be scanning in order to collapse to a pmd, but pmd is the level you're scanning at.
On 17/12/24 9:48 am, Matthew Wilcox wrote: > On Mon, Dec 16, 2024 at 10:20:54PM +0530, Dev Jain wrote: >> -static int hpage_collapse_scan_pmd(struct mm_struct *mm, >> +static int hpage_collapse_scan_ptes(struct mm_struct *mm, > i don't think this is necessary at all. you're scanning a pmd. > you might not be scanning in order to collapse to a pmd, but pmd > is the level you're scanning at. I did that if at all in the review process we note that we need to even drop down the starting scan order, so that we do not skip smaller VMAs. I kinda agree with you so till that time I have no problem reverting. >
On 17/12/2024 04:18, Matthew Wilcox wrote: > On Mon, Dec 16, 2024 at 10:20:54PM +0530, Dev Jain wrote: >> -static int hpage_collapse_scan_pmd(struct mm_struct *mm, >> +static int hpage_collapse_scan_ptes(struct mm_struct *mm, > > i don't think this is necessary at all. you're scanning a pmd. > you might not be scanning in order to collapse to a pmd, but pmd > is the level you're scanning at. > Sorry Matthew, I don't really understand this statement. Prior to the change we were scanning all PTE entries in a PTE table with the aim of collapsing to a PMD entry. After the change we are scanning some PTE entries in a PTE table with the aim of collapsing to either to a multi-PTE-mapped folio or a single-PMD-mapped folio. So personally I think "scan_pmd" was a misnomer even before the change - we are scanning the ptes.
On 17 Dec 2024, at 1:43, Ryan Roberts wrote: > On 17/12/2024 04:18, Matthew Wilcox wrote: >> On Mon, Dec 16, 2024 at 10:20:54PM +0530, Dev Jain wrote: >>> -static int hpage_collapse_scan_pmd(struct mm_struct *mm, >>> +static int hpage_collapse_scan_ptes(struct mm_struct *mm, >> >> i don't think this is necessary at all. you're scanning a pmd. >> you might not be scanning in order to collapse to a pmd, but pmd >> is the level you're scanning at. >> > > Sorry Matthew, I don't really understand this statement. Prior to the change we > were scanning all PTE entries in a PTE table with the aim of collapsing to a PMD > entry. After the change we are scanning some PTE entries in a PTE table with the > aim of collapsing to either to a multi-PTE-mapped folio or a single-PMD-mapped > folio. > > So personally I think "scan_pmd" was a misnomer even before the change - we are > scanning the ptes. But there are still a lot of scan_pmd code in the function, for example, VM_BUG_ON(address & ~HPAGE_PMD_MASK), _pte < pte + HPAGE_PMD_NR in the function. These need to be changed along with function renaming. If after the change only a subset of PTEs are scanned within a PMD, maybe a scan_range parameter can be added. Best Regards, Yan, Zi
On 17/12/2024 18:11, Zi Yan wrote: > On 17 Dec 2024, at 1:43, Ryan Roberts wrote: > >> On 17/12/2024 04:18, Matthew Wilcox wrote: >>> On Mon, Dec 16, 2024 at 10:20:54PM +0530, Dev Jain wrote: >>>> -static int hpage_collapse_scan_pmd(struct mm_struct *mm, >>>> +static int hpage_collapse_scan_ptes(struct mm_struct *mm, >>> >>> i don't think this is necessary at all. you're scanning a pmd. >>> you might not be scanning in order to collapse to a pmd, but pmd >>> is the level you're scanning at. >>> >> >> Sorry Matthew, I don't really understand this statement. Prior to the change we >> were scanning all PTE entries in a PTE table with the aim of collapsing to a PMD >> entry. After the change we are scanning some PTE entries in a PTE table with the >> aim of collapsing to either to a multi-PTE-mapped folio or a single-PMD-mapped >> folio. >> >> So personally I think "scan_pmd" was a misnomer even before the change - we are >> scanning the ptes. > > But there are still a lot of scan_pmd code in the function, for example, > VM_BUG_ON(address & ~HPAGE_PMD_MASK), _pte < pte + HPAGE_PMD_NR in the function. > These need to be changed along with function renaming. If after the change only > a subset of PTEs are scanned within a PMD, maybe a scan_range parameter can be > added. Oh I see; I think your and Matthew's point is that we are scanning a "PMD-entry's worth of PTEs". Looking at it like that, I guess I understand why "scan_pmd" makes sense. Fair enough. > > Best Regards, > Yan, Zi
diff --git a/mm/khugepaged.c b/mm/khugepaged.c index 99dc995aac11..95643e6e5f31 100644 --- a/mm/khugepaged.c +++ b/mm/khugepaged.c @@ -605,7 +605,7 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma, folio = page_folio(page); VM_BUG_ON_FOLIO(!folio_test_anon(folio), folio); - /* See hpage_collapse_scan_pmd(). */ + /* See hpage_collapse_scan_ptes(). */ if (folio_likely_mapped_shared(folio)) { ++shared; if (cc->is_khugepaged && @@ -991,7 +991,7 @@ static int check_pmd_still_valid(struct mm_struct *mm, /* * Bring missing pages in from swap, to complete THP collapse. - * Only done if hpage_collapse_scan_pmd believes it is worthwhile. + * Only done if hpage_collapse_scan_ptes believes it is worthwhile. * * Called and returns without pte mapped or spinlocks held. * Returns result: if not SCAN_SUCCEED, mmap_lock has been released. @@ -1263,7 +1263,7 @@ static int collapse_huge_page(struct mm_struct *mm, unsigned long address, return result; } -static int hpage_collapse_scan_pmd(struct mm_struct *mm, +static int hpage_collapse_scan_ptes(struct mm_struct *mm, struct vm_area_struct *vma, unsigned long address, bool *mmap_locked, struct collapse_control *cc) @@ -2457,7 +2457,7 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages, int *result, mmap_read_unlock(mm); } } else { - *result = hpage_collapse_scan_pmd(mm, vma, + *result = hpage_collapse_scan_ptes(mm, vma, khugepaged_scan.address, &mmap_locked, cc); } @@ -2792,7 +2792,7 @@ int madvise_collapse(struct vm_area_struct *vma, struct vm_area_struct **prev, cc); fput(file); } else { - result = hpage_collapse_scan_pmd(mm, vma, addr, + result = hpage_collapse_scan_ptes(mm, vma, addr, &mmap_locked, cc); } if (!mmap_locked)
Rename prior to generalizing the collapse function. Signed-off-by: Dev Jain <dev.jain@arm.com> --- mm/khugepaged.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)