Re: [PATCH v19 18/20] mm/lru: replace pgdat lru_lock with lruvec lock

[Hugh Dickins <hughd@google.com>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 14033 bytes --]

On Wed, 20 Oct 2020, Alex Shi wrote:
> 在 2020/9/24 上午11:28, Alex Shi 写道:
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -273,6 +273,8 @@ enum lruvec_flags {
> >  };
> >  
> >  struct lruvec {
> > +	/* per lruvec lru_lock for memcg */
> > +	spinlock_t			lru_lock;
> >  	struct list_head		lists[NR_LRU_LISTS];
> >  	/*
> >  	 * These track the cost of reclaiming one LRU - file or anon -
> 
> Hi All,
> 
> Intel Rong Chen, LKP, report a big regression on this patch, about
> 12 ~ 32% performance drop on fio.read_iops and case-lru-file-mmap-read
> case on wide Intel machine with attached kernel config.

Many thanks for the stats that followed: but though I spent many hours
poring over them, I ended up (as all too usual for me) just drowning
in the numbers, less and less able to arrive at any conclusion.

Why is 141% such a common standard deviation? I presume an artifact
of trying to estimate a stddev from only 3 runs? I wanted to ask you
to increase the runs from 3 to 12: too many of the numbers looked
unsafe to rely upon. And perhaps to increase the runtime from 200s
to 1000s, since some of the entries (invalidate_mapping_pages) look
as if they come from test preparation rather than the test itself.
But don't bother unless someone else asks: I would only drown again!

I have come to a tentative conclusion below, I don't see those stats
contradicting it, but I cannot argue how they support it either.

> Hugh Dickins pointed it's a false sharing issue on the lru_lock.

Well, I did suggest that as a possibility at first, before finding
that the slab (or slob) cache is guaranteeing cacheline alignment to
the mem_cgroup_per_node, hence to the lruvec, hence to the lru_lock.
So, I could not then see any false sharing.

> And that could be fixed by move the lru_lock out of busy lists[]
> cacheline,

No, access to those lists[] is always under that lru_lock: they
belong together, and sharing a cacheline between them is very
efficient: that is good sharing, not false sharing.

> like the following patch:

Not really.

> 
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index a75e6d0effcb..58b21bffef95 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -272,9 +272,9 @@ enum lruvec_flags {
>  };
> 
>  struct lruvec {
> +       struct list_head                lists[NR_LRU_LISTS];
>         /* per lruvec lru_lock for memcg */
>         spinlock_t                      lru_lock;
> -       struct list_head                lists[NR_LRU_LISTS];
>         /*
>          * These track the cost of reclaiming one LRU - file or anon -
>          * over the other. As the observed cost of reclaiming one LRU

Aha, that's a different patch than you showed me before, and it's
wonderfully simple, but rather surprising.  This patch really works
to fix the reported regression?  That's great, so long as it doesn't
turn out to introduce regressions elsewhere.  If it works, it's so
simple that I strongly approve of it, even if I don't understand it.

But (I see it in the lruv20 branch of your github tree) I would much
prefer you to remove my name from the commit message: it's your patch,
all I contributed was misdirection and bafflement.

> 
> Although the problem fixed， But I still no idea of the reasons
> and the gut problem. Any comments for this?

Right, how to explain it?

Well (and I'm assuming a 64-byte cacheline - I do hope the machines
you've been testing on really have that, rather than some greater
size that the slab allocator is unaware of), with your patch in
the first cacheline of the struct lruvec is:

inactive_anon.next inactive_anon.prev active_anon.next active_anon.prev
inactive_file.next inactive_file.prev active_file.next active_file.prev

And the interesting second cacheline of the struct lruvec is:

unevictable.next   unevictable.prev   lru_lock         anon_cost
file_cost          nonresident_age    refaults[anon]   refaults[file]

(I hope I'm not missing some debug option in your config, which would
expand lru_lock from an int plus a pad int, to something much bigger.)

Of those fields, unevictable.next unevictable.prev anon_cost and
file_cost are accessed under lru_lock, so are good to share its
cacheline.  snapshot_refaults() does not take any lock when setting
refaults[2], and they are not atomic; shrink_node() does not take
any lock when reading them.  We could quite easily change the code
to require lru_lock to access them, but it looks to me as if all
that would do is add unnecessary overhead: I could be wrong, but
they don't look worrying to me.

The worrying one is atomic_long_t nonresident_age.

And the advice we would usually give is to keep that in a separate
cacheline from lru_lock and its domain.  But what your patch does is
the opposite: it moves lru_lock into the same cacheline as its "rival";
and testing (so far) has shown that to be a good choice.

That seems audacious and surprising to me, but not impossible:
if it works, go with it.  But I wouldn't know how to write the
justification for the commit.

The image I have in my mind is: trying to get through a door in a
howling gale, while holding a bunch of cacheline kites tied together.
By forcing lru_lock and nonresident_age into the same cacheline,
you're making sure there is one less kite that can blow away in
another direction before you get through the door.

And Yu Zhao (in offline mail) has already drawn our attention to the
inefficient way move_pages_to_lru() calls workingset_age_nonresident()
repeatedly, all while holding lru_lock.  That's not much of a problem
in the base tree, but with per-memcg lru_lock, and more than one level
in the memcg hierarchy, that gets to be nasty - atomically modifying
the cacheline of a parent lruvec while holding the child's lru_lock.
I don't believe there's any deadlock in that, but I would expect
increasingly regressive performance at scale, even with your patch.

Here's a patch I was testing last night, on top of 5.9 and yours: works
for me, but my scale is too small to tell how well it will work for you.
I did consider whether we could use lru_lock on nonresident_age instead
of it being an independent atomic: in other places that looks plausible,
but adding lru_lock into __remove_mapping() feels like a very bad idea.

I'm not saying this or similar patch needs to go in right now
(and we shall certainly want Johannes's opinion before pushing it),
but I think it is going to show up as needed quite soon.

[PATCH] mm/lru: minimize workingset_age_nonresident() interference

1. move_pages_to_lru() batch workingset_age_nonresident() (from Yu Zhao)
2. workingset_age_nonresident() after unlock of lru_lock (from Yu Zhao)
3. lru_note_cost_unlock_irq() to remove an unlock+lock of lru_lock
4. lru_note_cost_unlock_irq() include updates of nonresident_age
5. workingset_refault() use that OR workingset_age_nonresident()

Of those, I expect 1 & 2 (from Yu Zhao) to be the most significant,
increasingly so with more cpus, increasingly so with more levels in
the memcg hierarchy.

Signed-off-by: Hugh Dickins <hughd@google.com>
Cc: Yu Zhao <yuzhao@google.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
---

 include/linux/swap.h |    7 +++----
 mm/swap.c            |   26 ++++++++++++++++----------
 mm/vmscan.c          |   23 +++++++++++++----------
 mm/workingset.c      |   11 +++++++----
 4 files changed, 39 insertions(+), 28 deletions(-)

--- 5.9-alex/include/linux/swap.h	2020-10-24 12:25:17.290383664 -0700
+++ 5.9-hugh/include/linux/swap.h	2020-10-24 18:31:58.396795250 -0700
@@ -332,11 +332,10 @@ extern unsigned long nr_free_buffer_page
 /* Definition of global_zone_page_state not available yet */
 #define nr_free_pages() global_zone_page_state(NR_FREE_PAGES)
 
-
 /* linux/mm/swap.c */
-extern void lru_note_cost(struct lruvec *lruvec, bool file,
-			  unsigned int nr_pages);
-extern void lru_note_cost_page(struct page *);
+extern void lru_note_cost_unlock_irq(struct lruvec *lruvec, bool file,
+		unsigned long nr_pages, unsigned long nr_active)
+		__releases(lruvec->lru_lock);
 extern void lru_cache_add(struct page *);
 extern void activate_page(struct page *);
 extern void mark_page_accessed(struct page *);
--- 5.9-alex/mm/swap.c	2020-10-24 12:25:17.314383732 -0700
+++ 5.9-hugh/mm/swap.c	2020-10-24 18:31:32.524708589 -0700
@@ -259,12 +259,12 @@ void rotate_reclaimable_page(struct page
 	}
 }
 
-void lru_note_cost(struct lruvec *lruvec, bool file, unsigned int nr_pages)
+void lru_note_cost_unlock_irq(struct lruvec *lruvec, bool file,
+		unsigned long nr_pages, unsigned long nr_active)
 {
-	do {
+	for (;;) {
 		unsigned long lrusize;
 
-		spin_lock_irq(&lruvec->lru_lock);
 		/* Record cost event */
 		if (file)
 			lruvec->file_cost += nr_pages;
@@ -288,14 +288,20 @@ void lru_note_cost(struct lruvec *lruvec
 			lruvec->file_cost /= 2;
 			lruvec->anon_cost /= 2;
 		}
-		spin_unlock_irq(&lruvec->lru_lock);
-	} while ((lruvec = parent_lruvec(lruvec)));
-}
 
-void lru_note_cost_page(struct page *page)
-{
-	lru_note_cost(mem_cgroup_page_lruvec(page, page_pgdat(page)),
-		      page_is_file_lru(page), thp_nr_pages(page));
+		/*
+		 * Update nonresident_age like workingset_age_nonresident().
+		 * Better to drop lru_lock first? Or better to do that last?
+		 */
+		if (nr_active)
+			atomic_long_add(nr_active, &lruvec->nonresident_age);
+
+		spin_unlock_irq(&lruvec->lru_lock);
+		lruvec = parent_lruvec(lruvec);
+		if (!lruvec)
+			break;
+		spin_lock_irq(&lruvec->lru_lock);
+	}
 }
 
 static void __activate_page(struct page *page, struct lruvec *lruvec)
--- 5.9-alex/mm/vmscan.c	2020-10-24 12:25:17.318383744 -0700
+++ 5.9-hugh/mm/vmscan.c	2020-10-24 18:37:31.389910659 -0700
@@ -1818,10 +1818,11 @@ static int too_many_isolated(struct pgli
  * move_pages_to_lru() moves pages from private @list to appropriate LRU list.
  * On return, @list is reused as a list of pages to be freed by the caller.
  *
- * Returns the number of pages moved to the given lruvec.
+ * Returns the number of pages moved to the given lruvec,
+ * or the number of pages moved to the active lru if count_actives_only.
  */
 static unsigned noinline_for_stack move_pages_to_lru(struct lruvec *lruvec,
-						     struct list_head *list)
+		struct list_head *list, bool count_actives_only)
 {
 	int nr_pages, nr_moved = 0;
 	LIST_HEAD(pages_to_free);
@@ -1872,9 +1873,8 @@ static unsigned noinline_for_stack move_
 
 		update_lru_size(lruvec, lru, page_zonenum(page), nr_pages);
 		list_add(&page->lru, &lruvec->lists[lru]);
-		nr_moved += nr_pages;
-		if (PageActive(page))
-			workingset_age_nonresident(lruvec, nr_pages);
+		if (!count_actives_only || PageActive(page))
+			nr_moved += nr_pages;
 	}
 
 	/*
@@ -1909,6 +1909,7 @@ shrink_inactive_list(unsigned long nr_to
 	LIST_HEAD(page_list);
 	unsigned long nr_scanned;
 	unsigned int nr_reclaimed = 0;
+	unsigned int nr_active;
 	unsigned long nr_taken;
 	struct reclaim_stat stat;
 	bool file = is_file_lru(lru);
@@ -1952,7 +1953,7 @@ shrink_inactive_list(unsigned long nr_to
 				&stat, false);
 
 	spin_lock_irq(&lruvec->lru_lock);
-	move_pages_to_lru(lruvec, &page_list);
+	nr_active = move_pages_to_lru(lruvec, &page_list, true);
 
 	__mod_node_page_state(pgdat, NR_ISOLATED_ANON + file, -nr_taken);
 	item = current_is_kswapd() ? PGSTEAL_KSWAPD : PGSTEAL_DIRECT;
@@ -1960,9 +1961,8 @@ shrink_inactive_list(unsigned long nr_to
 		__count_vm_events(item, nr_reclaimed);
 	__count_memcg_events(lruvec_memcg(lruvec), item, nr_reclaimed);
 	__count_vm_events(PGSTEAL_ANON + file, nr_reclaimed);
-	spin_unlock_irq(&lruvec->lru_lock);
 
-	lru_note_cost(lruvec, file, stat.nr_pageout);
+	lru_note_cost_unlock_irq(lruvec, file, stat.nr_pageout, nr_active);
 	mem_cgroup_uncharge_list(&page_list);
 	free_unref_page_list(&page_list);
 
@@ -2089,8 +2089,8 @@ static void shrink_active_list(unsigned
 	 */
 	spin_lock_irq(&lruvec->lru_lock);
 
-	nr_activate = move_pages_to_lru(lruvec, &l_active);
-	nr_deactivate = move_pages_to_lru(lruvec, &l_inactive);
+	nr_activate = move_pages_to_lru(lruvec, &l_active, false);
+	nr_deactivate = move_pages_to_lru(lruvec, &l_inactive, false);
 	/* Keep all free pages in l_active list */
 	list_splice(&l_inactive, &l_active);
 
@@ -2100,6 +2100,9 @@ static void shrink_active_list(unsigned
 	__mod_node_page_state(pgdat, NR_ISOLATED_ANON + file, -nr_taken);
 	spin_unlock_irq(&lruvec->lru_lock);
 
+	if (nr_activate)
+		workingset_age_nonresident(lruvec, nr_activate);
+
 	mem_cgroup_uncharge_list(&l_active);
 	free_unref_page_list(&l_active);
 	trace_mm_vmscan_lru_shrink_active(pgdat->node_id, nr_taken, nr_activate,
--- 5.9-alex/mm/workingset.c	2020-10-24 12:25:17.322383755 -0700
+++ 5.9-hugh/mm/workingset.c	2020-10-24 16:55:08.733369734 -0700
@@ -291,6 +291,7 @@ void workingset_refault(struct page *pag
 	unsigned long eviction;
 	struct lruvec *lruvec;
 	unsigned long refault;
+	unsigned int nr_pages;
 	bool workingset;
 	int memcgid;
 
@@ -374,16 +375,18 @@ void workingset_refault(struct page *pag
 		goto out;
 
 	SetPageActive(page);
-	workingset_age_nonresident(lruvec, thp_nr_pages(page));
-	inc_lruvec_state(lruvec, WORKINGSET_ACTIVATE_BASE + file);
+	nr_pages = thp_nr_pages(page);
 
 	/* Page was active prior to eviction */
 	if (workingset) {
 		SetPageWorkingset(page);
-		/* XXX: Move to lru_cache_add() when it supports new vs putback */
-		lru_note_cost_page(page);
+		spin_lock_irq(&lruvec->lru_lock);
+		lru_note_cost_unlock_irq(lruvec, file, nr_pages, nr_pages);
 		inc_lruvec_state(lruvec, WORKINGSET_RESTORE_BASE + file);
+	} else {
+		workingset_age_nonresident(lruvec, nr_pages);
 	}
+	inc_lruvec_state(lruvec, WORKINGSET_ACTIVATE_BASE + file);
 out:
 	rcu_read_unlock();
 }