Skip to contentSkip to conversation

memcg: cache obj_stock by memcg, not by objcg pointer

PATCH Superseded Shakeel Butt 1 participant 3 messages May 17 – May 18

Subsystems (3)

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Shakeel Butt

author maintainer
#0 lore raw

PATCH v2 1/1 memcg: cache obj_stock by memcg, not by objcg pointer

Commit 01b9da291c49 ("mm: memcontrol: convert objcg to be per-memcg per-node type") split a memcg's single obj_cgroup into one per NUMA node, but the per-CPU obj_stock_pcp still keys cached_objcg by pointer. Cross-NUMA workloads now see a drain on every refill and a miss on every consume that targets a sibling per-node objcg of the same memcg, producing the 67.7% stress-ng switch-mq regression reported by LKP.

stock->nr_bytes are fungible across per-node objcgs of one memcg. Treat the cache as keyed by memcg in __consume_obj_stock() and __refill_obj_stock() so siblings share the reserve. Compare via READ_ONCE(objcg->memcg) directly: pointer-compare only, no deref, so the rcu_read_lock contract on obj_cgroup_memcg() does not apply.

In the same-memcg refill path also fold the incoming objcg's nr_charged_bytes into the stock; otherwise sub-page residue accumulates on whichever sibling was cached at drain time and obj_cgroup_release() silently drops it, leaking up to nr_node_ids * (PAGE_SIZE - 1) bytes per memcg lifecycle from the page_counter. This issue was reported by Sashiko.

Update the now-stale invariant comment on __account_obj_stock().

Qi Zheng built a specialized reproducer [1] for the corner case and confirmed the fix.

---

Changes since v1:
- Fix the rcu warning (Sashiko).
- Fix the page counter possible underflow warning (Sashiko).

 mm/memcontrol.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)
mm/memcontrol.c +22 -3
index d978e18b9b2d..e22ffa3b3319 100644
@@ -3152,7 +3152,12 @@ static void unlock_stock(struct obj_stock_pcp *stock)
 		local_unlock(&obj_stock.lock);
 }
 
-/* Call after __refill_obj_stock() to ensure stock->cached_objg == objcg */
+/*
+ * Call after __consume_obj_stock() / __refill_obj_stock(). The stock may be
+ * cached for a sibling per-node objcg of the same memcg; in that case the
+ * vmstat batching slot does not match objcg and we fall through to the
+ * direct path.
+ */
 static void __account_obj_stock(struct obj_cgroup *objcg,
 				struct obj_stock_pcp *stock, int nr,
 				struct pglist_data *pgdat, enum node_stat_item idx)
@@ -3210,7 +3215,11 @@ static bool __consume_obj_stock(struct obj_cgroup *objcg,
 				struct obj_stock_pcp *stock,
 				unsigned int nr_bytes)
 {
-	if (objcg == READ_ONCE(stock->cached_objcg) &&
+	struct obj_cgroup *cached = READ_ONCE(stock->cached_objcg);
+
+	/* Sibling per-node objcgs share the reserve. */
+	if ((cached == objcg ||
+	     (cached && READ_ONCE(cached->memcg) == READ_ONCE(objcg->memcg))) &&
 	    stock->nr_bytes >= nr_bytes) {
 		stock->nr_bytes -= nr_bytes;
 		return true;
@@ -3318,6 +3327,7 @@ static void __refill_obj_stock(struct obj_cgroup *objcg,
 			       unsigned int nr_bytes,
 			       bool allow_uncharge)
 {
+	struct obj_cgroup *cached;
 	unsigned int nr_pages = 0;
 
 	if (!stock) {
@@ -3327,7 +3337,11 @@ static void __refill_obj_stock(struct obj_cgroup *objcg,
 		goto out;
 	}
 
-	if (READ_ONCE(stock->cached_objcg) != objcg) { /* reset if necessary */
+	cached = READ_ONCE(stock->cached_objcg);
+	if (cached == objcg)
+		goto add_bytes;
+	/* Direct READ_ONCE due to just pointer comparison. */
+	if (!cached || READ_ONCE(cached->memcg) != READ_ONCE(objcg->memcg)) {
 		drain_obj_stock(stock);
 		obj_cgroup_get(objcg);
 		stock->nr_bytes = atomic_read(&objcg->nr_charged_bytes)
@@ -3335,7 +3349,12 @@ static void __refill_obj_stock(struct obj_cgroup *objcg,
 		WRITE_ONCE(stock->cached_objcg, objcg);
 
 		allow_uncharge = true;	/* Allow uncharge when objcg changes */
+	} else if (atomic_read(&objcg->nr_charged_bytes)) {
+		/* Fold sibling's stranded ncb into stock; else release leaks it. */
+		stock->nr_bytes += atomic_xchg(&objcg->nr_charged_bytes, 0);
+		allow_uncharge = true;
 	}
+add_bytes:
 	stock->nr_bytes += nr_bytes;
 
 	if (allow_uncharge && (stock->nr_bytes > PAGE_SIZE)) {
Signature · 1 line
2.53.0-Meta
author maintainer
#1 lore raw

Cc Alex, Joshua (since they are working on making per-num kmem accounting work)

Quoted 26 lines · Shakeel Butt

Commit 01b9da291c49 ("mm: memcontrol: convert objcg to be per-memcg per-node type") split a memcg's single obj_cgroup into one per NUMA node, but the per-CPU obj_stock_pcp still keys cached_objcg by pointer. Cross-NUMA workloads now see a drain on every refill and a miss on every consume that targets a sibling per-node objcg of the same memcg, producing the 67.7% stress-ng switch-mq regression reported by LKP.

stock->nr_bytes are fungible across per-node objcgs of one memcg. Treat the cache as keyed by memcg in __consume_obj_stock() and __refill_obj_stock() so siblings share the reserve. Compare via READ_ONCE(objcg->memcg) directly: pointer-compare only, no deref, so the rcu_read_lock contract on obj_cgroup_memcg() does not apply.

In the same-memcg refill path also fold the incoming objcg's nr_charged_bytes into the stock; otherwise sub-page residue accumulates on whichever sibling was cached at drain time and obj_cgroup_release() silently drops it, leaking up to nr_node_ids * (PAGE_SIZE - 1) bytes per memcg lifecycle from the page_counter. This issue was reported by Sashiko.

Update the now-stale invariant comment on __account_obj_stock().

Qi Zheng built a specialized reproducer [1] for the corner case and confirmed the fix.

Quoted 7 lines

Sashiko [1] reported two issues. First one seems benign but the second one is real. However I think we need to take a step back and rethink on how to solve this issue in more future proof way.

It seems like Alex and Joshua are working on enabling per-node kmem accounting and that would need accurate per-numa association for each per-node objcg. So, checking objcg->memcg in consume and refill, would go against the per-node kmem accounting.

One way to fix the regression and be future proof is to follow the approach we have for memcg_stock_pcp which is multiple per-cpu objcg stocks. We will need to test it more and depending on the additional code complexity, we will need to decide to backport it to 7.2 or not.

[1] https://sashiko.dev/#/patchset/[email protected]?part=1

author maintainer
#2 lore raw
Quoted 46 lines · Shakeel Butt

Cc Alex, Joshua (since they are working on making per-num kmem accounting work)

Quoted 26 lines · Shakeel Butt

Commit 01b9da291c49 ("mm: memcontrol: convert objcg to be per-memcg per-node type") split a memcg's single obj_cgroup into one per NUMA node, but the per-CPU obj_stock_pcp still keys cached_objcg by pointer. Cross-NUMA workloads now see a drain on every refill and a miss on every consume that targets a sibling per-node objcg of the same memcg, producing the 67.7% stress-ng switch-mq regression reported by LKP.

stock->nr_bytes are fungible across per-node objcgs of one memcg. Treat the cache as keyed by memcg in __consume_obj_stock() and __refill_obj_stock() so siblings share the reserve. Compare via READ_ONCE(objcg->memcg) directly: pointer-compare only, no deref, so the rcu_read_lock contract on obj_cgroup_memcg() does not apply.

In the same-memcg refill path also fold the incoming objcg's nr_charged_bytes into the stock; otherwise sub-page residue accumulates on whichever sibling was cached at drain time and obj_cgroup_release() silently drops it, leaking up to nr_node_ids * (PAGE_SIZE - 1) bytes per memcg lifecycle from the page_counter. This issue was reported by Sashiko.

Update the now-stale invariant comment on __account_obj_stock().

Qi Zheng built a specialized reproducer [1] for the corner case and confirmed the fix.

Quoted 7 lines

Sashiko [1] reported two issues. First one seems benign but the second one is real. However I think we need to take a step back and rethink on how to solve this issue in more future proof way.

It seems like Alex and Joshua are working on enabling per-node kmem accounting and that would need accurate per-numa association for each per-node objcg. So, checking objcg->memcg in consume and refill, would go against the per-node kmem accounting.

Quoted 8 lines

One way to fix the regression and be future proof is to follow the approach we have for memcg_stock_pcp which is multiple per-cpu objcg stocks. We will need to test it more and depending on the additional code complexity, we will need to decide to backport it to 7.2 or not.

[1] https://sashiko.dev/#/patchset/[email protected]?part=1

Previously I had prototyped the multiple per-cpu objcg stocks (when I worked on multi-memcg percpu stock) which I just rebased on latest linux-next and sent [1].

That patch is additional 100 LOC. For upstreaming, I will break it up into at least 4 patches. However I am questioning about backporting them to 7.1. One thing I can do is fix whatever sashiko is asking for and send v3 which can be ported to 7.1 and then later for 7.2+, revert this short term fix and send out the multiple objcg patch series.

Any concerns?

[1] http://lore.kernel.org/[email protected]