On Wed, Mar 9, 2022 at 3:40 PM Yang Shi <shy828301@xxxxxxxxx> wrote:
>
> On Tue, Mar 8, 2022 at 1:35 PM Zach O'Keefe <zokeefe@xxxxxxxxxx> wrote:
> >
> > When scanning an anon pmd to see if it's eligible for collapse, return
> > SCAN_PAGE_COMPOUND if the pmd already maps a thp. This is consistent
> > with handling when scanning file-backed memory.
>
> I'm not quite keen that we have to keep anon consistent with file for
> this case. SCAN_PAGE_COMPOUND typically means the page is compound
> page, but PTE mapped.
>
Good point.
> And even SCAN_PMD_NULL is not returned every time when mm_find_pmd()
> returns NULL. In addition, SCAN_PMD_NULL seems ambiguous to me. The
> khugepaged actually sees non-present (migration) entry or trans huge
> entry, so may rename it to SCAN_PMD_NOT_SUITABLE?
>
Sorry, I'm not sure I understand the suggestion here. What this patch
would like to do, is to identify what pmds map thps. This will be
important later, since if a user requests a collapse of an
already-collapsed region, we want to return successfully (even if no
work to be done).
Maybe there should be a SCAN_PMD_MAPPED used here instead? Just to not
overload SCAN_PAGE_COMPOUND?
Though, note that when MADV_COLLAPSE supports file-backed memory, a
similar check for pmd-mapping will need to be made on the file-side of
things.
> >
> > Signed-off-by: Zach O'Keefe <zokeefe@xxxxxxxxxx>
> > ---
> > mm/khugepaged.c | 41 +++++++++++++++++++++++++++++++++++------
> > 1 file changed, 35 insertions(+), 6 deletions(-)
> >
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index ecbd3fc41c80..403578161a3b 100644
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -1011,6 +1011,38 @@ static int hugepage_vma_revalidate(struct mm_struct *mm, unsigned long address,
> > vm_flags_ignore, vmap);
> > }
> >
> > +/*
> > + * If returning NULL (meaning the pmd isn't mapped, isn't present, or thp),
> > + * write the reason to *result.
> > + */
> > +static pmd_t *find_pmd_or_thp_or_none(struct mm_struct *mm,
> > + unsigned long address,
> > + int *result)
> > +{
> > + pmd_t *pmd = mm_find_pmd_raw(mm, address);
> > + pmd_t pmde;
> > +
> > + if (!pmd) {
> > + *result = SCAN_PMD_NULL;
> > + return NULL;
> > + }
> > +
> > + pmde = pmd_read_atomic(pmd);
> > +
> > +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> > + /* See comments in pmd_none_or_trans_huge_or_clear_bad() */
> > + barrier();
> > +#endif
> > + if (!pmd_present(pmde) || !pmd_none(pmde)) {
> > + *result = SCAN_PMD_NULL;
> > + return NULL;
> > + } else if (pmd_trans_huge(pmde)) {
> > + *result = SCAN_PAGE_COMPOUND;
> > + return NULL;
> > + }
> > + return pmd;
> > +}
> > +
> > /*
> > * Bring missing pages in from swap, to complete THP collapse.
> > * Only done if khugepaged_scan_pmd believes it is worthwhile.
> > @@ -1212,9 +1244,8 @@ static void collapse_huge_page(struct mm_struct *mm,
> > goto out_nolock;
> > }
> >
> > - pmd = mm_find_pmd(mm, address);
> > + pmd = find_pmd_or_thp_or_none(mm, address, &result);
> > if (!pmd) {
> > - result = SCAN_PMD_NULL;
> > mmap_read_unlock(mm);
> > goto out_nolock;
> > }
> > @@ -1287,11 +1318,9 @@ static void scan_pmd(struct mm_struct *mm,
> > mmap_assert_locked(mm);
> > VM_BUG_ON(address & ~HPAGE_PMD_MASK);
> >
> > - pmd = mm_find_pmd(mm, address);
> > - if (!pmd) {
> > - scan_result->result = SCAN_PMD_NULL;
> > + pmd = find_pmd_or_thp_or_none(mm, address, &scan_result->result);
> > + if (!pmd)
> > goto out;
> > - }
> >
> > memset(cc->node_load, 0, sizeof(cc->node_load));
> > pte = pte_offset_map_lock(mm, pmd, address, &ptl);
> > --
> > 2.35.1.616.g0bdcbb4464-goog
> >
