memcg: cache obj_stock by memcg, not by objcg pointer
Subsystems (3)
Shakeel Butt
maintainerPATCH 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
@@ -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
Shakeel Butt
maintainerCc 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 singleobj_cgroupinto one per NUMA node, but the per-CPUobj_stock_pcpstill keyscached_objcgby 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_bytesare 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 viaREAD_ONCE(objcg->memcg)directly: pointer-compare only, no deref, so thercu_read_lockcontract onobj_cgroup_memcg()does not apply.In the same-memcg refill path also fold the incoming objcg's
nr_charged_bytesinto the stock; otherwise sub-page residue accumulates on whichever sibling was cached at drain time andobj_cgroup_release()silently drops it, leaking up tonr_node_ids* (PAGE_SIZE- 1) bytes per memcg lifecycle from thepage_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
Reported-by kernel test robot <[email protected]>Fixes 01b9da291c49 ("mm: memcontrol: convert objcg to be per-memcg per-node type")Signed-off-by Shakeel Butt <[email protected]>Debugged-by Qi Zheng <[email protected]>Tested-by Qi Zheng <[email protected]>
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.
Shakeel Butt
maintainerQuoted 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 singleobj_cgroupinto one per NUMA node, but the per-CPUobj_stock_pcpstill keyscached_objcgby 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_bytesare 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 viaREAD_ONCE(objcg->memcg)directly: pointer-compare only, no deref, so thercu_read_lockcontract onobj_cgroup_memcg()does not apply.In the same-memcg refill path also fold the incoming objcg's
nr_charged_bytesinto the stock; otherwise sub-page residue accumulates on whichever sibling was cached at drain time andobj_cgroup_release()silently drops it, leaking up tonr_node_ids* (PAGE_SIZE- 1) bytes per memcg lifecycle from thepage_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
Reported-by kernel test robot <[email protected]>Fixes 01b9da291c49 ("mm: memcontrol: convert objcg to be per-memcg per-node type")Signed-off-by Shakeel Butt <[email protected]>Debugged-by Qi Zheng <[email protected]>Tested-by Qi Zheng <[email protected]>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->memcgin 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_pcpwhich 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.
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?