Re: [PATCH 5/7] khugepaged: Allow to collapse PTE-mapped compound pages

[Yang Shi]

On Sat, Mar 28, 2020 at 5:27 AM Kirill A. Shutemov <kirill@xxxxxxxxxxxxx> wrote:
>
> On Fri, Mar 27, 2020 at 06:09:38PM -0700, Yang Shi wrote:
> > On Fri, Mar 27, 2020 at 5:34 PM Kirill A. Shutemov <kirill@xxxxxxxxxxxxx> wrote:
> > >
> > > On Fri, Mar 27, 2020 at 11:53:57AM -0700, Yang Shi wrote:
> > > > On Fri, Mar 27, 2020 at 10:06 AM Kirill A. Shutemov
> > > > <kirill@xxxxxxxxxxxxx> wrote:
> > > > >
> > > > > We can collapse PTE-mapped compound pages. We only need to avoid
> > > > > handling them more than once: lock/unlock page only once if it's present
> > > > > in the PMD range multiple times as it handled on compound level. The
> > > > > same goes for LRU isolation and putpack.
> > > > >
> > > > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> > > > > ---
> > > > >  mm/khugepaged.c | 41 +++++++++++++++++++++++++++++++----------
> > > > >  1 file changed, 31 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > > > > index b47edfe57f7b..c8c2c463095c 100644
> > > > > --- a/mm/khugepaged.c
> > > > > +++ b/mm/khugepaged.c
> > > > > @@ -515,6 +515,17 @@ void __khugepaged_exit(struct mm_struct *mm)
> > > > >
> > > > >  static void release_pte_page(struct page *page)
> > > > >  {
> > > > > +       /*
> > > > > +        * We need to unlock and put compound page on LRU only once.
> > > > > +        * The rest of the pages have to be locked and not on LRU here.
> > > > > +        */
> > > > > +       VM_BUG_ON_PAGE(!PageCompound(page) &&
> > > > > +                       (!PageLocked(page) && PageLRU(page)), page);
> > > > > +
> > > > > +       if (!PageLocked(page))
> > > > > +               return;
> > > > > +
> > > > > +       page = compound_head(page);
> > > > >         dec_node_page_state(page, NR_ISOLATED_ANON + page_is_file_cache(page));
> > > >
> > > > We need count in the number of base pages. The same counter is
> > > > modified by vmscan in base page unit.
> > >
> > > Is it though? Where?
> >
> > __mod_node_page_state(pgdat, NR_ISOLATED_ANON + file, nr_taken) in
> > vmscan.c, here nr_taken is nr_compound(page), so if it is THP the
> > number would be 512.
>
> Could you point a particular codepath?

shrink_inactive_list ->
        nr_taken = isolate_lru_pages()
        __mod_node_page_state(pgdat, NR_ISOLATED_ANON + file, nr_taken);

Then in isolate_lru_pages() :
        nr_pages = compound_nr(page);
        ...
        switch (__isolate_lru_page(page, mode)) {
                case 0:
                        nr_taken += nr_pages;

>
> > So in both inc and dec path of collapse PTE mapped THP, we should mod
> > nr_compound(page) too.
>
> I disagree. Compound page is represented by single entry on LRU, so it has
> to be counted once.

It was not a problem without THP swap. But with THP swap we saw
pgsteal_{kswapd|direct} may be greater than pgscan_{kswapd|direct} if
we count THP as one page.

Please refer to the below commit:

commit 98879b3b9edc1604f2d1a6686576ef4d08ed3310
Author: Yang Shi <yang.shi@xxxxxxxxxxxxxxxxx>
Date:   Thu Jul 11 20:59:30 2019 -0700

    mm: vmscan: correct some vmscan counters for THP swapout

    Commit bd4c82c22c36 ("mm, THP, swap: delay splitting THP after swapped
    out"), THP can be swapped out in a whole.  But, nr_reclaimed and some
    other vm counters still get inc'ed by one even though a whole THP (512
    pages) gets swapped out.

    This doesn't make too much sense to memory reclaim.

    For example, direct reclaim may just need reclaim SWAP_CLUSTER_MAX
    pages, reclaiming one THP could fulfill it.  But, if nr_reclaimed is not
    increased correctly, direct reclaim may just waste time to reclaim more
    pages, SWAP_CLUSTER_MAX * 512 pages in worst case.

    And, it may cause pgsteal_{kswapd|direct} is greater than
    pgscan_{kswapd|direct}, like the below:

    pgsteal_kswapd 122933
    pgsteal_direct 26600225
    pgscan_kswapd 174153
    pgscan_direct 14678312

    nr_reclaimed and nr_scanned must be fixed in parallel otherwise it would
    break some page reclaim logic, e.g.

    vmpressure: this looks at the scanned/reclaimed ratio so it won't change
    semantics as long as scanned & reclaimed are fixed in parallel.

    compaction/reclaim: compaction wants a certain number of physical pages
    freed up before going back to compacting.

    kswapd priority raising: kswapd raises priority if we scan fewer pages
    than the reclaim target (which itself is obviously expressed in order-0
    pages).  As a result, kswapd can falsely raise its aggressiveness even
    when it's making great progress.

    Other than nr_scanned and nr_reclaimed, some other counters, e.g.
    pgactivate, nr_skipped, nr_ref_keep and nr_unmap_fail need to be fixed too
    since they are user visible via cgroup, /proc/vmstat or trace points,
    otherwise they would be underreported.

    When isolating pages from LRUs, nr_taken has been accounted in base page,
    but nr_scanned and nr_skipped are still accounted in THP.  It doesn't make
    too much sense too since this may cause trace point underreport the
    numbers as well.

    So accounting those counters in base page instead of accounting THP as one
    page.

    nr_dirty, nr_unqueued_dirty, nr_congested and nr_writeback are used by
    file cache, so they are not impacted by THP swap.

    This change may result in lower steal/scan ratio in some cases since THP
    may get split during page reclaim, then a part of tail pages get reclaimed
    instead of the whole 512 pages, but nr_scanned is accounted by 512,
    particularly for direct reclaim.  But, this should be not a significant
    issue.


So, since we count THP in base page unit in vmscan path, so the same
counter should be updated in base page unit in other path as well
IMHO.


>
> --
>  Kirill A. Shutemov