On Wed, Mar 9, 2022 at 6:06 PM Yang Shi <shy828301@xxxxxxxxx> wrote:
>
> On Wed, Mar 9, 2022 at 4:46 PM Zach O'Keefe <zokeefe@xxxxxxxxxx> wrote:
> >
> > 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).
>
> Makes sense.
>
> >
> > Maybe there should be a SCAN_PMD_MAPPED used here instead? Just to not
> > overload SCAN_PAGE_COMPOUND?
>
> I see. SCAN_PMD_MAPPED sounds more self-explained and suitable IMHO.
>
>
Makes sense not to conflate the two. Thanks for the feedback!
>
> >
> > 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
> > > >
