Skip to contentSkip to conversation

memcg: use round-robin victim selection in refill_stock

PATCH Applied Shakeel Butt 3 participants 3 messages May 21 – May 25

Subsystems (3)
Memory management cgroup memcg
lore
Share & tools
lore

Shakeel Butt

author maintainer
#0 lore raw

PATCH 1/1 memcg: use round-robin victim selection in refill_stock

Harry Yoo reported that get_random_u32_below() is not safe to call in the nmi context and memcg charge draining can happen in nmi context.

More specifically get_random_u32_below() is neither reentrant- nor
NMI-safe: it acquires a per-cpu local_lock via local_lock_irqsave() on the batched_entropy_u32 state. An NMI that lands on a CPU mid-update of the ChaCha batch state and recurses into the random subsystem would corrupt that state. The memcg_stock local_trylock prevents re-entry on the percpu stock itself, but cannot protect an unrelated subsystem's per-cpu lock.

Replace the random pick with a per-cpu round-robin counter stored in memcg_stock_pcp and serialized by the same local_trylock that already guards cached[] and nr_pages[]. No atomics, no random calls, no extra locks needed.

Fixes f735eebe55f8f ("memcg: multi-memcg percpu charge cache")
Reported-by Harry Yoo <[email protected]>
Closes https://lore.kernel.org/[email protected]/ open here
Signed-off-by Shakeel Butt <[email protected]>
---
 mm/memcontrol.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
mm/memcontrol.c +4 -1
index 0eb50e639f0a..6392a2704441 100644
@@ -2031,6 +2031,7 @@ struct memcg_stock_pcp {
 
 	struct work_struct work;
 	unsigned long flags;
+	uint8_t drain_idx;
 };
 
 static DEFINE_PER_CPU_ALIGNED(struct memcg_stock_pcp, memcg_stock) = {

@@ -2214,7 +2215,9 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
 	if (!success) {
 		i = empty_slot;
 		if (i == -1) {
-			i = get_random_u32_below(NR_MEMCG_STOCK);
+			i = stock->drain_idx++;
+			if (stock->drain_idx == NR_MEMCG_STOCK)
+				stock->drain_idx = 0;
 			drain_stock(stock, i);
 		}
 		css_get(&memcg->css);
Signature · 1 line
2.53.0-Meta
Acked-by on v1 1/1
#1 lore raw
Quoted 11 lines · Shakeel Butt

Harry Yoo reported that get_random_u32_below() is not safe to call in the nmi context and memcg charge draining can happen in nmi context.

More specifically get_random_u32_below() is neither reentrant- nor
NMI-safe: it acquires a per-cpu local_lock via local_lock_irqsave() on the batched_entropy_u32 state. An NMI that lands on a CPU mid-update of the ChaCha batch state and recurses into the random subsystem would corrupt that state. The memcg_stock local_trylock prevents re-entry on the percpu stock itself, but cannot protect an unrelated subsystem's per-cpu lock.

Quoted 6 lines

Replace the random pick with a per-cpu round-robin counter stored in memcg_stock_pcp and serialized by the same local_trylock that already guards cached[] and nr_pages[]. No atomics, no random calls, no extra locks needed.

Fixes f735eebe55f8f ("memcg: multi-memcg percpu charge cache")
Acked-by Harry Yoo (Oracle) <[email protected]>

and perhaps

Cc <[email protected]>

as it affects v6.18 (the latest LTS).

Thanks a lot for fixing it, Shakeel!

Quoted 30 lines
Reported-by Harry Yoo <[email protected]>
Closes https://lore.kernel.org/[email protected]/ open here
Signed-off-by Shakeel Butt <[email protected]>
---
  mm/memcontrol.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)
mm/memcontrol.c +4 -1
index 0eb50e639f0a..6392a2704441 100644
@@ -2031,6 +2031,7 @@ struct memcg_stock_pcp {
 
  	struct work_struct work;
  	unsigned long flags;
+	uint8_t drain_idx;
  };
 
  static DEFINE_PER_CPU_ALIGNED(struct memcg_stock_pcp, memcg_stock) = {

@@ -2214,7 +2215,9 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
  	if (!success) {
  		i = empty_slot;
  		if (i == -1) {
-			i = get_random_u32_below(NR_MEMCG_STOCK);
+			i = stock->drain_idx++;
+			if (stock->drain_idx == NR_MEMCG_STOCK)
+				stock->drain_idx = 0;
  			drain_stock(stock, i);
  		}
  		css_get(&memcg->css);
Signature · 2 lines
Cheers,
Harry / Hyeonggon
Acked-by on v1 1/1
#2 lore raw
In reply to Shakeel Butt
Quoted 16 lines · Shakeel Butt

Harry Yoo reported that get_random_u32_below() is not safe to call in the nmi context and memcg charge draining can happen in nmi context.

More specifically get_random_u32_below() is neither reentrant- nor
NMI-safe: it acquires a per-cpu local_lock via local_lock_irqsave() on the batched_entropy_u32 state. An NMI that lands on a CPU mid-update of the ChaCha batch state and recurses into the random subsystem would corrupt that state. The memcg_stock local_trylock prevents re-entry on the percpu stock itself, but cannot protect an unrelated subsystem's per-cpu lock.

Replace the random pick with a per-cpu round-robin counter stored in memcg_stock_pcp and serialized by the same local_trylock that already guards cached[] and nr_pages[]. No atomics, no random calls, no extra locks needed.

Quoted 4 lines
Fixes f735eebe55f8f ("memcg: multi-memcg percpu charge cache")
Reported-by Harry Yoo <[email protected]>
Closes https://lore.kernel.org/[email protected]/ open here
Signed-off-by Shakeel Butt <[email protected]>
Acked-by Michal Hocko <[email protected]>

Thanks!

Quoted 29 lines
---
 mm/memcontrol.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
mm/memcontrol.c +4 -1
index 0eb50e639f0a..6392a2704441 100644
@@ -2031,6 +2031,7 @@ struct memcg_stock_pcp {
 
 	struct work_struct work;
 	unsigned long flags;
+	uint8_t drain_idx;
 };
 
 static DEFINE_PER_CPU_ALIGNED(struct memcg_stock_pcp, memcg_stock) = {

@@ -2214,7 +2215,9 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
 	if (!success) {
 		i = empty_slot;
 		if (i == -1) {
-			i = get_random_u32_below(NR_MEMCG_STOCK);
+			i = stock->drain_idx++;
+			if (stock->drain_idx == NR_MEMCG_STOCK)
+				stock->drain_idx = 0;
 			drain_stock(stock, i);
 		}
 		css_get(&memcg->css);
Signature · 1 line
2.53.0-Meta
Signature · 2 lines
Michal Hocko
SUSE Labs