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
