[PATCH] mm: memcontrol: skip propagate percpu vmstat values to current memcg

[PATCH] mm: memcontrol: skip propagate percpu vmstat values to current memcg

[Muchun Song]

The current memcg will be freed soon, so updating it's vmstat and
vmevent values is pointless. Just skip updating it.

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/memcontrol.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index c0a90767e264..597fbe9acf93 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3700,7 +3700,7 @@ static void memcg_flush_percpu_vmstats(struct mem_cgroup *memcg)
 		for (i = 0; i < MEMCG_NR_STAT; i++)
 			stat[i] += per_cpu(memcg->vmstats_percpu->stat[i], cpu);
 
-	for (mi = memcg; mi; mi = parent_mem_cgroup(mi))
+	for (mi = parent_mem_cgroup(memcg); mi; mi = parent_mem_cgroup(mi))
 		for (i = 0; i < MEMCG_NR_STAT; i++)
 			atomic_long_add(stat[i], &mi->vmstats[i]);
 
@@ -3716,7 +3716,8 @@ static void memcg_flush_percpu_vmstats(struct mem_cgroup *memcg)
 				stat[i] += per_cpu(
 					pn->lruvec_stat_cpu->count[i], cpu);
 
-		for (pi = pn; pi; pi = parent_nodeinfo(pi, node))
+		for (pi = parent_nodeinfo(pn, node); pi;
+		     pi = parent_nodeinfo(pi, node))
 			for (i = 0; i < NR_VM_NODE_STAT_ITEMS; i++)
 				atomic_long_add(stat[i], &pi->lruvec_stat[i]);
 	}
@@ -3736,7 +3737,7 @@ static void memcg_flush_percpu_vmevents(struct mem_cgroup *memcg)
 			events[i] += per_cpu(memcg->vmstats_percpu->events[i],
 					     cpu);
 
-	for (mi = memcg; mi; mi = parent_mem_cgroup(mi))
+	for (mi = parent_mem_cgroup(memcg); mi; mi = parent_mem_cgroup(mi))
 		for (i = 0; i < NR_VM_EVENT_ITEMS; i++)
 			atomic_long_add(events[i], &mi->vmevents[i]);
 }
-- 
2.11.0

Re: [PATCH] mm: memcontrol: skip propagate percpu vmstat values to current memcg

[Johannes Weiner]

On Tue, Jan 19, 2021 at 01:27:44PM +0800, Muchun Song wrote:
> The current memcg will be freed soon, so updating it's vmstat and
> vmevent values is pointless. Just skip updating it.
> 
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>

Oof, that's pretty subtle! Somebody trying to refactor that code for
other purposes may not immediately notice that optimization and add
potentially tedious bugs.

How much does this save? Cgroup creation and deletion isn't really
considered a hot path. It takes global locks and such...

Re: [PATCH] mm: memcontrol: skip propagate percpu vmstat values to current memcg

[Michal Hocko]

On Tue 19-01-21 11:58:42, Johannes Weiner wrote:
> On Tue, Jan 19, 2021 at 01:27:44PM +0800, Muchun Song wrote:
> > The current memcg will be freed soon, so updating it's vmstat and
> > vmevent values is pointless. Just skip updating it.
> > 
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> 
> Oof, that's pretty subtle! Somebody trying to refactor that code for
> other purposes may not immediately notice that optimization and add
> potentially tedious bugs.

Absolutely agreed!

> How much does this save? Cgroup creation and deletion isn't really
> considered a hot path. It takes global locks and such...

This is not the first time when an (micro)optimization is posted without
any data showing benefit or otherwise appealing justification. Sigh.

-- 
Michal Hocko
SUSE Labs