Skip to contentSkip to conversation

memcg: optimize memcg_rstat_updated

PATCH Superseded Shakeel Butt 2 participants 3 messages Apr 9, 2025 – Apr 10, 2025

Subsystems (3)
Memory management cgroup memcg

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Shakeel Butt

author maintainer
#0 lore raw

PATCH 1/1 memcg: optimize memcg_rstat_updated

Currently the kernel maintains the stats updates per-memcg which is needed to implement stats flushing threshold. On the update side, the update is added to the per-cpu per-memcg update of the given memcg and all of its ancestors. However when the given memcg has passed the flushing threshold, all of its ancestors should have passed the threshold as well. There is no need to traverse up the memcg tree to maintain the stats updates.

Perf profile collected from our fleet shows that memcg_rstat_updated is one of the most expensive memcg function i.e. a lot of cumulative CPU is being spent on it. So, even small micro optimizations matter a lot.

Signed-off-by Shakeel Butt <[email protected]>
---
 mm/memcontrol.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)
mm/memcontrol.c +9 -7
index 421740f1bcdc..ea3e40e589df 100644
@@ -585,18 +585,20 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
 	cgroup_rstat_updated(memcg->css.cgroup, cpu);
 	statc = this_cpu_ptr(memcg->vmstats_percpu);
 	for (; statc; statc = statc->parent) {
+		/*
+		 * If @memcg is already flushable then all its ancestors are
+		 * flushable as well and also there is no need to increase
+		 * stats_updates.
+		 */
+		if (!memcg_vmstats_needs_flush(statc->vmstats))
+			break;
+
 		stats_updates = READ_ONCE(statc->stats_updates) + abs(val);
 		WRITE_ONCE(statc->stats_updates, stats_updates);
 		if (stats_updates < MEMCG_CHARGE_BATCH)
 			continue;
 
-		/*
-		 * If @memcg is already flush-able, increasing stats_updates is
-		 * redundant. Avoid the overhead of the atomic update.
-		 */
-		if (!memcg_vmstats_needs_flush(statc->vmstats))
-			atomic64_add(stats_updates,
-				     &statc->vmstats->stats_updates);
+		atomic64_add(stats_updates, &statc->vmstats->stats_updates);
 		WRITE_ONCE(statc->stats_updates, 0);
 	}
 }
Signature · 1 line
2.47.1
#1 lore raw
Quoted 23 lines · Shakeel Butt

Currently the kernel maintains the stats updates per-memcg which is needed to implement stats flushing threshold. On the update side, the update is added to the per-cpu per-memcg update of the given memcg and all of its ancestors. However when the given memcg has passed the flushing threshold, all of its ancestors should have passed the threshold as well. There is no need to traverse up the memcg tree to maintain the stats updates.

Perf profile collected from our fleet shows that memcg_rstat_updated is one of the most expensive memcg function i.e. a lot of cumulative CPU is being spent on it. So, even small micro optimizations matter a lot.

Signed-off-by Shakeel Butt<[email protected]>
---
  mm/memcontrol.c | 16 +++++++++-------
  1 file changed, 9 insertions(+), 7 deletions(-)
mm/memcontrol.c +0 -0
index 421740f1bcdc..ea3e40e589df 100644
@@ -585,18 +585,20 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
  	cgroup_rstat_updated(memcg->css.cgroup, cpu);

mm/memcontrol.c static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val) · line 595

Quoted 10 lines
mm/memcontrol.c +8 -0
  	statc = this_cpu_ptr(memcg->vmstats_percpu);
  	for (; statc; statc = statc->parent) {
+		/*
+		 * If @memcg is already flushable then all its ancestors are
+		 * flushable as well and also there is no need to increase
+		 * stats_updates.
+		 */
+		if (!memcg_vmstats_needs_flush(statc->vmstats))
+			break;
+

Do you mean "if (memcg_vmstats_needs_flush(statc->vmstats))"?

Cheers, Longman

Quoted 16 lines
mm/memcontrol.c +1 -7
  		stats_updates = READ_ONCE(statc->stats_updates) + abs(val);
  		WRITE_ONCE(statc->stats_updates, stats_updates);
  		if (stats_updates < MEMCG_CHARGE_BATCH)
  			continue;
 
-		/*
-		 * If @memcg is already flush-able, increasing stats_updates is
-		 * redundant. Avoid the overhead of the atomic update.
-		 */
-		if (!memcg_vmstats_needs_flush(statc->vmstats))
-			atomic64_add(stats_updates,
-				     &statc->vmstats->stats_updates);
+		atomic64_add(stats_updates, &statc->vmstats->stats_updates);
  		WRITE_ONCE(statc->stats_updates, 0);
  	}
  }
author maintainer
#2 lore raw
Quoted 28 lines · Waiman Long
Quoted 27 lines · Shakeel Butt

Currently the kernel maintains the stats updates per-memcg which is needed to implement stats flushing threshold. On the update side, the update is added to the per-cpu per-memcg update of the given memcg and all of its ancestors. However when the given memcg has passed the flushing threshold, all of its ancestors should have passed the threshold as well. There is no need to traverse up the memcg tree to maintain the stats updates.

Perf profile collected from our fleet shows that memcg_rstat_updated is one of the most expensive memcg function i.e. a lot of cumulative CPU is being spent on it. So, even small micro optimizations matter a lot.

Signed-off-by Shakeel Butt<[email protected]>
---
  mm/memcontrol.c | 16 +++++++++-------
  1 file changed, 9 insertions(+), 7 deletions(-)
mm/memcontrol.c +2 -0
index 421740f1bcdc..ea3e40e589df 100644
@@ -585,18 +585,20 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
  	cgroup_rstat_updated(memcg->css.cgroup, cpu);
  	statc = this_cpu_ptr(memcg->vmstats_percpu);
  	for (; statc; statc = statc->parent) {
+		/*
+		 * If @memcg is already flushable then all its ancestors are
Quoted 9 lines

mm/memcontrol.c static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val) · line 595

Quoted 6 lines
mm/memcontrol.c +6 -0
+		 * flushable as well and also there is no need to increase
+		 * stats_updates.
+		 */
+		if (!memcg_vmstats_needs_flush(statc->vmstats))
+			break;
+

Do you mean "if (memcg_vmstats_needs_flush(statc->vmstats))"?

Yup you are right, thanks for catching this. I will send a v2.