Re: [RFC PATCH 09/14] mm/khugepaged: record SCAN_PAGE_COMPOUND when scan_pmd() finds THP

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
> > > >




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux