[PATCH v2 0/7] memcg: make memcg stats irq safe

[PATCH v2 0/7] memcg: make memcg stats irq safe

[Shakeel Butt]

This series converts memcg stats to be irq safe i.e. memcg stats can be
updated in any context (task, softirq or hardirq) without disabling the
irqs. This is still not nmi-safe on all architectures but after this
series converting memcg charging and stats nmi-safe will be easier.

Changes since v1:
-----------------
1. Rebased on mm-new.
2. Cleanups in first patch as suggested by Vlastimil.

Changes since RFC[1]:
--------------------
1. Rebased on next-20250513 (mm tree has some conflicts with cgroup
   tree).
2. Does not depend of nmi-safe memcg series [2].
3. Made memcg_rstat_updated re-entrant using this_cpu_* ops as suggested
   by Vlastimil.
4. Fixes some spelling mistakes as suggested by Vlastimil.
5. Rearranged the 6th and 7th patch as suggested by Vlastimil.

Link: http://lore.kernel.org/20250513031316.2147548-1-shakeel.butt@linux.dev [1]
Link: http://lore.kernel.org/20250509232859.657525-1-shakeel.butt@linux.dev [2]

Shakeel Butt (7):
  memcg: memcg_rstat_updated re-entrant safe against irqs
  memcg: move preempt disable to callers of memcg_rstat_updated
  memcg: make mod_memcg_state re-entrant safe against irqs
  memcg: make count_memcg_events re-entrant safe against irqs
  memcg: make __mod_memcg_lruvec_state re-entrant safe against irqs
  memcg: no stock lock for cpu hot-unplug
  memcg: objcg stock trylock without irq disabling

 include/linux/memcontrol.h |  41 +--------
 mm/memcontrol-v1.c         |   6 +-
 mm/memcontrol.c            | 170 +++++++++++++++----------------------
 mm/swap.c                  |   8 +-
 mm/vmscan.c                |  14 +--
 5 files changed, 86 insertions(+), 153 deletions(-)

-- 
2.47.1

[PATCH v2 1/7] memcg: memcg_rstat_updated re-entrant safe against irqs

[Shakeel Butt]

The function memcg_rstat_updated() is used to track the memcg stats
updates for optimizing the flushes. At the moment, it is not re-entrant
safe and the callers disabled irqs before calling. However to achieve
the goal of updating memcg stats without irqs, memcg_rstat_updated()
needs to be re-entrant safe against irqs.

This patch makes memcg_rstat_updated() re-entrant safe using this_cpu_*
ops. On archs with CONFIG_ARCH_HAS_NMI_SAFE_THIS_CPU_OPS, this patch is
also making memcg_rstat_updated() nmi safe.

Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/memcontrol.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 89476a71a18d..2464a58fbf17 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -505,8 +505,8 @@ struct memcg_vmstats_percpu {
 	unsigned int			stats_updates;
 
 	/* Cached pointers for fast iteration in memcg_rstat_updated() */
-	struct memcg_vmstats_percpu	*parent;
-	struct memcg_vmstats		*vmstats;
+	struct memcg_vmstats_percpu __percpu	*parent_pcpu;
+	struct memcg_vmstats			*vmstats;
 
 	/* The above should fit a single cacheline for memcg_rstat_updated() */
 
@@ -588,16 +588,21 @@ static bool memcg_vmstats_needs_flush(struct memcg_vmstats *vmstats)
 
 static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
 {
+	struct memcg_vmstats_percpu __percpu *statc_pcpu;
 	struct memcg_vmstats_percpu *statc;
-	int cpu = smp_processor_id();
+	int cpu;
 	unsigned int stats_updates;
 
 	if (!val)
 		return;
 
+	/* Don't assume callers have preemption disabled. */
+	cpu = get_cpu();
+
 	cgroup_rstat_updated(memcg->css.cgroup, cpu);
-	statc = this_cpu_ptr(memcg->vmstats_percpu);
-	for (; statc; statc = statc->parent) {
+	statc_pcpu = memcg->vmstats_percpu;
+	for (; statc_pcpu; statc_pcpu = statc->parent_pcpu) {
+		statc = this_cpu_ptr(statc_pcpu);
 		/*
 		 * If @memcg is already flushable then all its ancestors are
 		 * flushable as well and also there is no need to increase
@@ -606,14 +611,15 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
 		if (memcg_vmstats_needs_flush(statc->vmstats))
 			break;
 
-		stats_updates = READ_ONCE(statc->stats_updates) + abs(val);
-		WRITE_ONCE(statc->stats_updates, stats_updates);
+		stats_updates = this_cpu_add_return(statc_pcpu->stats_updates,
+						    abs(val));
 		if (stats_updates < MEMCG_CHARGE_BATCH)
 			continue;
 
+		stats_updates = this_cpu_xchg(statc_pcpu->stats_updates, 0);
 		atomic64_add(stats_updates, &statc->vmstats->stats_updates);
-		WRITE_ONCE(statc->stats_updates, 0);
 	}
+	put_cpu();
 }
 
 static void __mem_cgroup_flush_stats(struct mem_cgroup *memcg, bool force)
@@ -3691,7 +3697,7 @@ static void mem_cgroup_free(struct mem_cgroup *memcg)
 
 static struct mem_cgroup *mem_cgroup_alloc(struct mem_cgroup *parent)
 {
-	struct memcg_vmstats_percpu *statc, *pstatc;
+	struct memcg_vmstats_percpu *statc, __percpu *pstatc_pcpu;
 	struct mem_cgroup *memcg;
 	int node, cpu;
 	int __maybe_unused i;
@@ -3722,9 +3728,9 @@ static struct mem_cgroup *mem_cgroup_alloc(struct mem_cgroup *parent)
 
 	for_each_possible_cpu(cpu) {
 		if (parent)
-			pstatc = per_cpu_ptr(parent->vmstats_percpu, cpu);
+			pstatc_pcpu = parent->vmstats_percpu;
 		statc = per_cpu_ptr(memcg->vmstats_percpu, cpu);
-		statc->parent = parent ? pstatc : NULL;
+		statc->parent_pcpu = parent ? pstatc_pcpu : NULL;
 		statc->vmstats = memcg->vmstats;
 	}
 
-- 
2.47.1

[PATCH v2 2/7] memcg: move preempt disable to callers of memcg_rstat_updated

[Shakeel Butt]

Let's move the explicit preempt disable code to the callers of
memcg_rstat_updated and also remove the memcg_stats_lock and related
functions which ensures the callers of stats update functions have
disabled preemption because now the stats update functions are
explicitly disabling preemption.

Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/memcontrol.c | 74 +++++++++++++------------------------------------
 1 file changed, 19 insertions(+), 55 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 2464a58fbf17..1750d86012f3 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -557,48 +557,22 @@ static u64 flush_last_time;
 
 #define FLUSH_TIME (2UL*HZ)
 
-/*
- * Accessors to ensure that preemption is disabled on PREEMPT_RT because it can
- * not rely on this as part of an acquired spinlock_t lock. These functions are
- * never used in hardirq context on PREEMPT_RT and therefore disabling preemtion
- * is sufficient.
- */
-static void memcg_stats_lock(void)
-{
-	preempt_disable_nested();
-	VM_WARN_ON_IRQS_ENABLED();
-}
-
-static void __memcg_stats_lock(void)
-{
-	preempt_disable_nested();
-}
-
-static void memcg_stats_unlock(void)
-{
-	preempt_enable_nested();
-}
-
-
 static bool memcg_vmstats_needs_flush(struct memcg_vmstats *vmstats)
 {
 	return atomic64_read(&vmstats->stats_updates) >
 		MEMCG_CHARGE_BATCH * num_online_cpus();
 }
 
-static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
+static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val,
+				       int cpu)
 {
 	struct memcg_vmstats_percpu __percpu *statc_pcpu;
 	struct memcg_vmstats_percpu *statc;
-	int cpu;
 	unsigned int stats_updates;
 
 	if (!val)
 		return;
 
-	/* Don't assume callers have preemption disabled. */
-	cpu = get_cpu();
-
 	cgroup_rstat_updated(memcg->css.cgroup, cpu);
 	statc_pcpu = memcg->vmstats_percpu;
 	for (; statc_pcpu; statc_pcpu = statc->parent_pcpu) {
@@ -619,7 +593,6 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
 		stats_updates = this_cpu_xchg(statc_pcpu->stats_updates, 0);
 		atomic64_add(stats_updates, &statc->vmstats->stats_updates);
 	}
-	put_cpu();
 }
 
 static void __mem_cgroup_flush_stats(struct mem_cgroup *memcg, bool force)
@@ -717,6 +690,7 @@ void __mod_memcg_state(struct mem_cgroup *memcg, enum memcg_stat_item idx,
 		       int val)
 {
 	int i = memcg_stats_index(idx);
+	int cpu;
 
 	if (mem_cgroup_disabled())
 		return;
@@ -724,12 +698,14 @@ void __mod_memcg_state(struct mem_cgroup *memcg, enum memcg_stat_item idx,
 	if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, idx))
 		return;
 
-	memcg_stats_lock();
+	cpu = get_cpu();
+
 	__this_cpu_add(memcg->vmstats_percpu->state[i], val);
 	val = memcg_state_val_in_pages(idx, val);
-	memcg_rstat_updated(memcg, val);
+	memcg_rstat_updated(memcg, val, cpu);
 	trace_mod_memcg_state(memcg, idx, val);
-	memcg_stats_unlock();
+
+	put_cpu();
 }
 
 #ifdef CONFIG_MEMCG_V1
@@ -758,6 +734,7 @@ static void __mod_memcg_lruvec_state(struct lruvec *lruvec,
 	struct mem_cgroup_per_node *pn;
 	struct mem_cgroup *memcg;
 	int i = memcg_stats_index(idx);
+	int cpu;
 
 	if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, idx))
 		return;
@@ -765,24 +742,7 @@ static void __mod_memcg_lruvec_state(struct lruvec *lruvec,
 	pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec);
 	memcg = pn->memcg;
 
-	/*
-	 * The caller from rmap relies on disabled preemption because they never
-	 * update their counter from in-interrupt context. For these two
-	 * counters we check that the update is never performed from an
-	 * interrupt context while other caller need to have disabled interrupt.
-	 */
-	__memcg_stats_lock();
-	if (IS_ENABLED(CONFIG_DEBUG_VM)) {
-		switch (idx) {
-		case NR_ANON_MAPPED:
-		case NR_FILE_MAPPED:
-		case NR_ANON_THPS:
-			WARN_ON_ONCE(!in_task());
-			break;
-		default:
-			VM_WARN_ON_IRQS_ENABLED();
-		}
-	}
+	cpu = get_cpu();
 
 	/* Update memcg */
 	__this_cpu_add(memcg->vmstats_percpu->state[i], val);
@@ -791,9 +751,10 @@ static void __mod_memcg_lruvec_state(struct lruvec *lruvec,
 	__this_cpu_add(pn->lruvec_stats_percpu->state[i], val);
 
 	val = memcg_state_val_in_pages(idx, val);
-	memcg_rstat_updated(memcg, val);
+	memcg_rstat_updated(memcg, val, cpu);
 	trace_mod_memcg_lruvec_state(memcg, idx, val);
-	memcg_stats_unlock();
+
+	put_cpu();
 }
 
 /**
@@ -873,6 +834,7 @@ void __count_memcg_events(struct mem_cgroup *memcg, enum vm_event_item idx,
 			  unsigned long count)
 {
 	int i = memcg_events_index(idx);
+	int cpu;
 
 	if (mem_cgroup_disabled())
 		return;
@@ -880,11 +842,13 @@ void __count_memcg_events(struct mem_cgroup *memcg, enum vm_event_item idx,
 	if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, idx))
 		return;
 
-	memcg_stats_lock();
+	cpu = get_cpu();
+
 	__this_cpu_add(memcg->vmstats_percpu->events[i], count);
-	memcg_rstat_updated(memcg, count);
+	memcg_rstat_updated(memcg, count, cpu);
 	trace_count_memcg_events(memcg, idx, count);
-	memcg_stats_unlock();
+
+	put_cpu();
 }
 
 unsigned long memcg_events(struct mem_cgroup *memcg, int event)
-- 
2.47.1

[PATCH v2 3/7] memcg: make mod_memcg_state re-entrant safe against irqs

[Shakeel Butt]

Let's make mod_memcg_state re-entrant safe against irqs. The only thing
needed is to convert the usage of __this_cpu_add() to this_cpu_add().
In addition, with re-entrant safety, there is no need to disable irqs.

mod_memcg_state() is not safe against nmi, so let's add warning if
someone tries to call it in nmi context.

Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
---
 include/linux/memcontrol.h | 20 ++------------------
 mm/memcontrol.c            |  8 ++++----
 2 files changed, 6 insertions(+), 22 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 9ed75f82b858..92861ff3c43f 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -903,19 +903,9 @@ struct mem_cgroup *mem_cgroup_get_oom_group(struct task_struct *victim,
 					    struct mem_cgroup *oom_domain);
 void mem_cgroup_print_oom_group(struct mem_cgroup *memcg);
 
-void __mod_memcg_state(struct mem_cgroup *memcg, enum memcg_stat_item idx,
-		       int val);
-
 /* idx can be of type enum memcg_stat_item or node_stat_item */
-static inline void mod_memcg_state(struct mem_cgroup *memcg,
-				   enum memcg_stat_item idx, int val)
-{
-	unsigned long flags;
-
-	local_irq_save(flags);
-	__mod_memcg_state(memcg, idx, val);
-	local_irq_restore(flags);
-}
+void mod_memcg_state(struct mem_cgroup *memcg,
+		     enum memcg_stat_item idx, int val);
 
 static inline void mod_memcg_page_state(struct page *page,
 					enum memcg_stat_item idx, int val)
@@ -1375,12 +1365,6 @@ static inline void mem_cgroup_print_oom_group(struct mem_cgroup *memcg)
 {
 }
 
-static inline void __mod_memcg_state(struct mem_cgroup *memcg,
-				     enum memcg_stat_item idx,
-				     int nr)
-{
-}
-
 static inline void mod_memcg_state(struct mem_cgroup *memcg,
 				   enum memcg_stat_item idx,
 				   int nr)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 1750d86012f3..c5a835071610 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -681,12 +681,12 @@ static int memcg_state_val_in_pages(int idx, int val)
 }
 
 /**
- * __mod_memcg_state - update cgroup memory statistics
+ * mod_memcg_state - update cgroup memory statistics
  * @memcg: the memory cgroup
  * @idx: the stat item - can be enum memcg_stat_item or enum node_stat_item
  * @val: delta to add to the counter, can be negative
  */
-void __mod_memcg_state(struct mem_cgroup *memcg, enum memcg_stat_item idx,
+void mod_memcg_state(struct mem_cgroup *memcg, enum memcg_stat_item idx,
 		       int val)
 {
 	int i = memcg_stats_index(idx);
@@ -700,7 +700,7 @@ void __mod_memcg_state(struct mem_cgroup *memcg, enum memcg_stat_item idx,
 
 	cpu = get_cpu();
 
-	__this_cpu_add(memcg->vmstats_percpu->state[i], val);
+	this_cpu_add(memcg->vmstats_percpu->state[i], val);
 	val = memcg_state_val_in_pages(idx, val);
 	memcg_rstat_updated(memcg, val, cpu);
 	trace_mod_memcg_state(memcg, idx, val);
@@ -2920,7 +2920,7 @@ static void drain_obj_stock(struct obj_stock_pcp *stock)
 
 			memcg = get_mem_cgroup_from_objcg(old);
 
-			__mod_memcg_state(memcg, MEMCG_KMEM, -nr_pages);
+			mod_memcg_state(memcg, MEMCG_KMEM, -nr_pages);
 			memcg1_account_kmem(memcg, -nr_pages);
 			if (!mem_cgroup_is_root(memcg))
 				memcg_uncharge(memcg, nr_pages);
-- 
2.47.1

[PATCH v2 4/7] memcg: make count_memcg_events re-entrant safe against irqs

[Shakeel Butt]

Let's make count_memcg_events re-entrant safe against irqs. The only
thing needed is to convert the usage of __this_cpu_add() to
this_cpu_add(). In addition, with re-entrant safety, there is no need
to disable irqs. Also add warnings for in_nmi() as it is not safe
against nmi context.

Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
---
 include/linux/memcontrol.h | 21 ++-------------------
 mm/memcontrol-v1.c         |  6 +++---
 mm/memcontrol.c            |  6 +++---
 mm/swap.c                  |  8 ++++----
 mm/vmscan.c                | 14 +++++++-------
 5 files changed, 19 insertions(+), 36 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 92861ff3c43f..f7848f73f41c 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -942,19 +942,8 @@ static inline void mod_lruvec_kmem_state(void *p, enum node_stat_item idx,
 	local_irq_restore(flags);
 }
 
-void __count_memcg_events(struct mem_cgroup *memcg, enum vm_event_item idx,
-			  unsigned long count);
-
-static inline void count_memcg_events(struct mem_cgroup *memcg,
-				      enum vm_event_item idx,
-				      unsigned long count)
-{
-	unsigned long flags;
-
-	local_irq_save(flags);
-	__count_memcg_events(memcg, idx, count);
-	local_irq_restore(flags);
-}
+void count_memcg_events(struct mem_cgroup *memcg, enum vm_event_item idx,
+			unsigned long count);
 
 static inline void count_memcg_folio_events(struct folio *folio,
 		enum vm_event_item idx, unsigned long nr)
@@ -1418,12 +1407,6 @@ static inline void mod_lruvec_kmem_state(void *p, enum node_stat_item idx,
 }
 
 static inline void count_memcg_events(struct mem_cgroup *memcg,
-				      enum vm_event_item idx,
-				      unsigned long count)
-{
-}
-
-static inline void __count_memcg_events(struct mem_cgroup *memcg,
 					enum vm_event_item idx,
 					unsigned long count)
 {
diff --git a/mm/memcontrol-v1.c b/mm/memcontrol-v1.c
index 54c49cbfc968..4b94731305b9 100644
--- a/mm/memcontrol-v1.c
+++ b/mm/memcontrol-v1.c
@@ -512,9 +512,9 @@ static void memcg1_charge_statistics(struct mem_cgroup *memcg, int nr_pages)
 {
 	/* pagein of a big page is an event. So, ignore page size */
 	if (nr_pages > 0)
-		__count_memcg_events(memcg, PGPGIN, 1);
+		count_memcg_events(memcg, PGPGIN, 1);
 	else {
-		__count_memcg_events(memcg, PGPGOUT, 1);
+		count_memcg_events(memcg, PGPGOUT, 1);
 		nr_pages = -nr_pages; /* for event */
 	}
 
@@ -689,7 +689,7 @@ void memcg1_uncharge_batch(struct mem_cgroup *memcg, unsigned long pgpgout,
 	unsigned long flags;
 
 	local_irq_save(flags);
-	__count_memcg_events(memcg, PGPGOUT, pgpgout);
+	count_memcg_events(memcg, PGPGOUT, pgpgout);
 	__this_cpu_add(memcg->events_percpu->nr_page_events, nr_memory);
 	memcg1_check_events(memcg, nid);
 	local_irq_restore(flags);
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index c5a835071610..0923072386c2 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -825,12 +825,12 @@ void __mod_lruvec_kmem_state(void *p, enum node_stat_item idx, int val)
 }
 
 /**
- * __count_memcg_events - account VM events in a cgroup
+ * count_memcg_events - account VM events in a cgroup
  * @memcg: the memory cgroup
  * @idx: the event item
  * @count: the number of events that occurred
  */
-void __count_memcg_events(struct mem_cgroup *memcg, enum vm_event_item idx,
+void count_memcg_events(struct mem_cgroup *memcg, enum vm_event_item idx,
 			  unsigned long count)
 {
 	int i = memcg_events_index(idx);
@@ -844,7 +844,7 @@ void __count_memcg_events(struct mem_cgroup *memcg, enum vm_event_item idx,
 
 	cpu = get_cpu();
 
-	__this_cpu_add(memcg->vmstats_percpu->events[i], count);
+	this_cpu_add(memcg->vmstats_percpu->events[i], count);
 	memcg_rstat_updated(memcg, count, cpu);
 	trace_count_memcg_events(memcg, idx, count);
 
diff --git a/mm/swap.c b/mm/swap.c
index 77b2d5997873..4fc322f7111a 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -309,7 +309,7 @@ static void lru_activate(struct lruvec *lruvec, struct folio *folio)
 	trace_mm_lru_activate(folio);
 
 	__count_vm_events(PGACTIVATE, nr_pages);
-	__count_memcg_events(lruvec_memcg(lruvec), PGACTIVATE, nr_pages);
+	count_memcg_events(lruvec_memcg(lruvec), PGACTIVATE, nr_pages);
 }
 
 #ifdef CONFIG_SMP
@@ -581,7 +581,7 @@ static void lru_deactivate_file(struct lruvec *lruvec, struct folio *folio)
 
 	if (active) {
 		__count_vm_events(PGDEACTIVATE, nr_pages);
-		__count_memcg_events(lruvec_memcg(lruvec), PGDEACTIVATE,
+		count_memcg_events(lruvec_memcg(lruvec), PGDEACTIVATE,
 				     nr_pages);
 	}
 }
@@ -599,7 +599,7 @@ static void lru_deactivate(struct lruvec *lruvec, struct folio *folio)
 	lruvec_add_folio(lruvec, folio);
 
 	__count_vm_events(PGDEACTIVATE, nr_pages);
-	__count_memcg_events(lruvec_memcg(lruvec), PGDEACTIVATE, nr_pages);
+	count_memcg_events(lruvec_memcg(lruvec), PGDEACTIVATE, nr_pages);
 }
 
 static void lru_lazyfree(struct lruvec *lruvec, struct folio *folio)
@@ -625,7 +625,7 @@ static void lru_lazyfree(struct lruvec *lruvec, struct folio *folio)
 	lruvec_add_folio(lruvec, folio);
 
 	__count_vm_events(PGLAZYFREE, nr_pages);
-	__count_memcg_events(lruvec_memcg(lruvec), PGLAZYFREE, nr_pages);
+	count_memcg_events(lruvec_memcg(lruvec), PGLAZYFREE, nr_pages);
 }
 
 /*
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 5efd939d8c76..f86d264558f5 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2028,7 +2028,7 @@ static unsigned long shrink_inactive_list(unsigned long nr_to_scan,
 	item = PGSCAN_KSWAPD + reclaimer_offset(sc);
 	if (!cgroup_reclaim(sc))
 		__count_vm_events(item, nr_scanned);
-	__count_memcg_events(lruvec_memcg(lruvec), item, nr_scanned);
+	count_memcg_events(lruvec_memcg(lruvec), item, nr_scanned);
 	__count_vm_events(PGSCAN_ANON + file, nr_scanned);
 
 	spin_unlock_irq(&lruvec->lru_lock);
@@ -2048,7 +2048,7 @@ static unsigned long shrink_inactive_list(unsigned long nr_to_scan,
 	item = PGSTEAL_KSWAPD + reclaimer_offset(sc);
 	if (!cgroup_reclaim(sc))
 		__count_vm_events(item, nr_reclaimed);
-	__count_memcg_events(lruvec_memcg(lruvec), item, nr_reclaimed);
+	count_memcg_events(lruvec_memcg(lruvec), item, nr_reclaimed);
 	__count_vm_events(PGSTEAL_ANON + file, nr_reclaimed);
 	spin_unlock_irq(&lruvec->lru_lock);
 
@@ -2138,7 +2138,7 @@ static void shrink_active_list(unsigned long nr_to_scan,
 
 	if (!cgroup_reclaim(sc))
 		__count_vm_events(PGREFILL, nr_scanned);
-	__count_memcg_events(lruvec_memcg(lruvec), PGREFILL, nr_scanned);
+	count_memcg_events(lruvec_memcg(lruvec), PGREFILL, nr_scanned);
 
 	spin_unlock_irq(&lruvec->lru_lock);
 
@@ -2195,7 +2195,7 @@ static void shrink_active_list(unsigned long nr_to_scan,
 	nr_deactivate = move_folios_to_lru(lruvec, &l_inactive);
 
 	__count_vm_events(PGDEACTIVATE, nr_deactivate);
-	__count_memcg_events(lruvec_memcg(lruvec), PGDEACTIVATE, nr_deactivate);
+	count_memcg_events(lruvec_memcg(lruvec), PGDEACTIVATE, nr_deactivate);
 
 	__mod_node_page_state(pgdat, NR_ISOLATED_ANON + file, -nr_taken);
 	spin_unlock_irq(&lruvec->lru_lock);
@@ -4616,8 +4616,8 @@ static int scan_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
 		__count_vm_events(item, isolated);
 		__count_vm_events(PGREFILL, sorted);
 	}
-	__count_memcg_events(memcg, item, isolated);
-	__count_memcg_events(memcg, PGREFILL, sorted);
+	count_memcg_events(memcg, item, isolated);
+	count_memcg_events(memcg, PGREFILL, sorted);
 	__count_vm_events(PGSCAN_ANON + type, isolated);
 	trace_mm_vmscan_lru_isolate(sc->reclaim_idx, sc->order, MAX_LRU_BATCH,
 				scanned, skipped, isolated,
@@ -4769,7 +4769,7 @@ static int evict_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
 	item = PGSTEAL_KSWAPD + reclaimer_offset(sc);
 	if (!cgroup_reclaim(sc))
 		__count_vm_events(item, reclaimed);
-	__count_memcg_events(memcg, item, reclaimed);
+	count_memcg_events(memcg, item, reclaimed);
 	__count_vm_events(PGSTEAL_ANON + type, reclaimed);
 
 	spin_unlock_irq(&lruvec->lru_lock);
-- 
2.47.1

[PATCH v2 5/7] memcg: make __mod_memcg_lruvec_state re-entrant safe against irqs

[Shakeel Butt]

Let's make __mod_memcg_lruvec_state re-entrant safe and name it
mod_memcg_lruvec_state(). The only thing needed is to convert the usage
of __this_cpu_add() to this_cpu_add(). There are two callers of
mod_memcg_lruvec_state() and one of them i.e. __mod_objcg_mlstate() will
be re-entrant safe as well, so, rename it mod_objcg_mlstate(). The last
caller __mod_lruvec_state() still calls __mod_node_page_state() which is
not re-entrant safe yet, so keep it as is.

Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/memcontrol.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 0923072386c2..1071db0b1df8 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -727,7 +727,7 @@ unsigned long memcg_page_state_local(struct mem_cgroup *memcg, int idx)
 }
 #endif
 
-static void __mod_memcg_lruvec_state(struct lruvec *lruvec,
+static void mod_memcg_lruvec_state(struct lruvec *lruvec,
 				     enum node_stat_item idx,
 				     int val)
 {
@@ -745,10 +745,10 @@ static void __mod_memcg_lruvec_state(struct lruvec *lruvec,
 	cpu = get_cpu();
 
 	/* Update memcg */
-	__this_cpu_add(memcg->vmstats_percpu->state[i], val);
+	this_cpu_add(memcg->vmstats_percpu->state[i], val);
 
 	/* Update lruvec */
-	__this_cpu_add(pn->lruvec_stats_percpu->state[i], val);
+	this_cpu_add(pn->lruvec_stats_percpu->state[i], val);
 
 	val = memcg_state_val_in_pages(idx, val);
 	memcg_rstat_updated(memcg, val, cpu);
@@ -775,7 +775,7 @@ void __mod_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx,
 
 	/* Update memcg and lruvec */
 	if (!mem_cgroup_disabled())
-		__mod_memcg_lruvec_state(lruvec, idx, val);
+		mod_memcg_lruvec_state(lruvec, idx, val);
 }
 
 void __lruvec_stat_mod_folio(struct folio *folio, enum node_stat_item idx,
@@ -2527,7 +2527,7 @@ static void commit_charge(struct folio *folio, struct mem_cgroup *memcg)
 	folio->memcg_data = (unsigned long)memcg;
 }
 
-static inline void __mod_objcg_mlstate(struct obj_cgroup *objcg,
+static inline void mod_objcg_mlstate(struct obj_cgroup *objcg,
 				       struct pglist_data *pgdat,
 				       enum node_stat_item idx, int nr)
 {
@@ -2537,7 +2537,7 @@ static inline void __mod_objcg_mlstate(struct obj_cgroup *objcg,
 	rcu_read_lock();
 	memcg = obj_cgroup_memcg(objcg);
 	lruvec = mem_cgroup_lruvec(memcg, pgdat);
-	__mod_memcg_lruvec_state(lruvec, idx, nr);
+	mod_memcg_lruvec_state(lruvec, idx, nr);
 	rcu_read_unlock();
 }
 
@@ -2847,12 +2847,12 @@ static void __account_obj_stock(struct obj_cgroup *objcg,
 		struct pglist_data *oldpg = stock->cached_pgdat;
 
 		if (stock->nr_slab_reclaimable_b) {
-			__mod_objcg_mlstate(objcg, oldpg, NR_SLAB_RECLAIMABLE_B,
+			mod_objcg_mlstate(objcg, oldpg, NR_SLAB_RECLAIMABLE_B,
 					  stock->nr_slab_reclaimable_b);
 			stock->nr_slab_reclaimable_b = 0;
 		}
 		if (stock->nr_slab_unreclaimable_b) {
-			__mod_objcg_mlstate(objcg, oldpg, NR_SLAB_UNRECLAIMABLE_B,
+			mod_objcg_mlstate(objcg, oldpg, NR_SLAB_UNRECLAIMABLE_B,
 					  stock->nr_slab_unreclaimable_b);
 			stock->nr_slab_unreclaimable_b = 0;
 		}
@@ -2878,7 +2878,7 @@ static void __account_obj_stock(struct obj_cgroup *objcg,
 		}
 	}
 	if (nr)
-		__mod_objcg_mlstate(objcg, pgdat, idx, nr);
+		mod_objcg_mlstate(objcg, pgdat, idx, nr);
 }
 
 static bool consume_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes,
@@ -2947,13 +2947,13 @@ static void drain_obj_stock(struct obj_stock_pcp *stock)
 	 */
 	if (stock->nr_slab_reclaimable_b || stock->nr_slab_unreclaimable_b) {
 		if (stock->nr_slab_reclaimable_b) {
-			__mod_objcg_mlstate(old, stock->cached_pgdat,
+			mod_objcg_mlstate(old, stock->cached_pgdat,
 					  NR_SLAB_RECLAIMABLE_B,
 					  stock->nr_slab_reclaimable_b);
 			stock->nr_slab_reclaimable_b = 0;
 		}
 		if (stock->nr_slab_unreclaimable_b) {
-			__mod_objcg_mlstate(old, stock->cached_pgdat,
+			mod_objcg_mlstate(old, stock->cached_pgdat,
 					  NR_SLAB_UNRECLAIMABLE_B,
 					  stock->nr_slab_unreclaimable_b);
 			stock->nr_slab_unreclaimable_b = 0;
-- 
2.47.1

[PATCH v2 6/7] memcg: no stock lock for cpu hot-unplug

[Shakeel Butt]

Previously on the cpu hot-unplug, the kernel would call
drain_obj_stock() with objcg local lock. However local lock was not
needed as the stock which was accessed belongs to a dead cpu but we kept
it there to disable irqs as drain_obj_stock() may call
mod_objcg_mlstate() which required irqs disabled. However there is no
need to disable irqs now for mod_objcg_mlstate(), so we can remove the
local lock altogether from cpu hot-unplug path.

Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/memcontrol.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 1071db0b1df8..04d756be708b 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2025,17 +2025,8 @@ void drain_all_stock(struct mem_cgroup *root_memcg)
 
 static int memcg_hotplug_cpu_dead(unsigned int cpu)
 {
-	struct obj_stock_pcp *obj_st;
-	unsigned long flags;
-
-	obj_st = &per_cpu(obj_stock, cpu);
-
-	/* drain_obj_stock requires objstock.lock */
-	local_lock_irqsave(&obj_stock.lock, flags);
-	drain_obj_stock(obj_st);
-	local_unlock_irqrestore(&obj_stock.lock, flags);
-
 	/* no need for the local lock */
+	drain_obj_stock(&per_cpu(obj_stock, cpu));
 	drain_stock_fully(&per_cpu(memcg_stock, cpu));
 
 	return 0;
-- 
2.47.1

[PATCH v2 7/7] memcg: objcg stock trylock without irq disabling

[Shakeel Butt]

There is no need to disable irqs to use objcg per-cpu stock, so let's
just not do that but consume_obj_stock() and refill_obj_stock() will
need to use trylock instead to avoid deadlock against irq. One
consequence of this change is that the charge request from irq context
may take slowpath more often but it should be rare.

Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/memcontrol.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 04d756be708b..e17b698f6243 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1882,18 +1882,17 @@ static void drain_local_memcg_stock(struct work_struct *dummy)
 static void drain_local_obj_stock(struct work_struct *dummy)
 {
 	struct obj_stock_pcp *stock;
-	unsigned long flags;
 
 	if (WARN_ONCE(!in_task(), "drain in non-task context"))
 		return;
 
-	local_lock_irqsave(&obj_stock.lock, flags);
+	local_lock(&obj_stock.lock);
 
 	stock = this_cpu_ptr(&obj_stock);
 	drain_obj_stock(stock);
 	clear_bit(FLUSHING_CACHED_CHARGE, &stock->flags);
 
-	local_unlock_irqrestore(&obj_stock.lock, flags);
+	local_unlock(&obj_stock.lock);
 }
 
 static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
@@ -2876,10 +2875,10 @@ static bool consume_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes,
 			      struct pglist_data *pgdat, enum node_stat_item idx)
 {
 	struct obj_stock_pcp *stock;
-	unsigned long flags;
 	bool ret = false;
 
-	local_lock_irqsave(&obj_stock.lock, flags);
+	if (!local_trylock(&obj_stock.lock))
+		return ret;
 
 	stock = this_cpu_ptr(&obj_stock);
 	if (objcg == READ_ONCE(stock->cached_objcg) && stock->nr_bytes >= nr_bytes) {
@@ -2890,7 +2889,7 @@ static bool consume_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes,
 			__account_obj_stock(objcg, stock, nr_bytes, pgdat, idx);
 	}
 
-	local_unlock_irqrestore(&obj_stock.lock, flags);
+	local_unlock(&obj_stock.lock);
 
 	return ret;
 }
@@ -2979,10 +2978,16 @@ static void refill_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes,
 		enum node_stat_item idx)
 {
 	struct obj_stock_pcp *stock;
-	unsigned long flags;
 	unsigned int nr_pages = 0;
 
-	local_lock_irqsave(&obj_stock.lock, flags);
+	if (!local_trylock(&obj_stock.lock)) {
+		if (pgdat)
+			mod_objcg_mlstate(objcg, pgdat, idx, nr_bytes);
+		nr_pages = nr_bytes >> PAGE_SHIFT;
+		nr_bytes = nr_bytes & (PAGE_SIZE - 1);
+		atomic_add(nr_bytes, &objcg->nr_charged_bytes);
+		goto out;
+	}
 
 	stock = this_cpu_ptr(&obj_stock);
 	if (READ_ONCE(stock->cached_objcg) != objcg) { /* reset if necessary */
@@ -3004,8 +3009,8 @@ static void refill_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes,
 		stock->nr_bytes &= (PAGE_SIZE - 1);
 	}
 
-	local_unlock_irqrestore(&obj_stock.lock, flags);
-
+	local_unlock(&obj_stock.lock);
+out:
 	if (nr_pages)
 		obj_cgroup_uncharge_pages(objcg, nr_pages);
 }
-- 
2.47.1

Re: [PATCH v2 1/7] memcg: memcg_rstat_updated re-entrant safe against irqs

[Lorenzo Stoakes]

Shakeel - This breaks the build in mm-new for me:

  CC      mm/pt_reclaim.o
In file included from ./arch/x86/include/asm/rmwcc.h:5,
                 from ./arch/x86/include/asm/bitops.h:18,
                 from ./include/linux/bitops.h:68,
                 from ./include/linux/radix-tree.h:11,
                 from ./include/linux/idr.h:15,
                 from ./include/linux/cgroup-defs.h:13,
                 from mm/memcontrol.c:28:
mm/memcontrol.c: In function ‘mem_cgroup_alloc’:
./arch/x86/include/asm/percpu.h:39:45: error: expected identifier or ‘(’ before ‘__seg_gs’
   39 | #define __percpu_seg_override   CONCATENATE(__seg_, __percpu_seg)
      |                                             ^~~~~~
./include/linux/args.h:25:24: note: in definition of macro ‘__CONCAT’
   25 | #define __CONCAT(a, b) a ## b
      |                        ^
./arch/x86/include/asm/percpu.h:39:33: note: in expansion of macro ‘CONCATENATE’
   39 | #define __percpu_seg_override   CONCATENATE(__seg_, __percpu_seg)
      |                                 ^~~~~~~~~~~
./arch/x86/include/asm/percpu.h:93:33: note: in expansion of macro ‘__percpu_seg_override’
   93 | # define __percpu_qual          __percpu_seg_override
      |                                 ^~~~~~~~~~~~~~~~~~~~~
././include/linux/compiler_types.h:60:25: note: in expansion of macro ‘__percpu_qual’
   60 | # define __percpu       __percpu_qual BTF_TYPE_TAG(percpu)
      |                         ^~~~~~~~~~~~~
mm/memcontrol.c:3700:45: note: in expansion of macro ‘__percpu’
 3700 |         struct memcg_vmstats_percpu *statc, __percpu *pstatc_pcpu;
      |                                             ^~~~~~~~
mm/memcontrol.c:3731:25: error: ‘pstatc_pcpu’ undeclared (first use in this function); did you mean ‘kstat_cpu’?
 3731 |                         pstatc_pcpu = parent->vmstats_percpu;
      |                         ^~~~~~~~~~~
      |                         kstat_cpu
mm/memcontrol.c:3731:25: note: each undeclared identifier is reported only once for each function it appears in

The __percpu macro seems to be a bit screwy with comma-delimited decls, as it
seems that putting this on its own line fixes this problem:


From 28275e5d054506746d310cf5ebd1fafdb0881dba Mon Sep 17 00:00:00 2001
From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Date: Thu, 15 May 2025 13:43:46 +0100
Subject: [PATCH] fix

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 mm/memcontrol.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 2464a58fbf17..40fcc2259e5f 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3697,7 +3697,8 @@ static void mem_cgroup_free(struct mem_cgroup *memcg)

 static struct mem_cgroup *mem_cgroup_alloc(struct mem_cgroup *parent)
 {
-	struct memcg_vmstats_percpu *statc, __percpu *pstatc_pcpu;
+	struct memcg_vmstats_percpu *statc;
+	struct memcg_vmstats_percpu __percpu *pstatc_pcpu;
 	struct mem_cgroup *memcg;
 	int node, cpu;
 	int __maybe_unused i;
--
2.49.0


I have duplicated this again at the end of this mail for easy application.

Could we get this fix in or drop the series so the build is fixed for
mm-new? Thanks!


On Wed, May 14, 2025 at 11:41:52AM -0700, Shakeel Butt wrote:
> The function memcg_rstat_updated() is used to track the memcg stats
> updates for optimizing the flushes. At the moment, it is not re-entrant
> safe and the callers disabled irqs before calling. However to achieve
> the goal of updating memcg stats without irqs, memcg_rstat_updated()
> needs to be re-entrant safe against irqs.
>
> This patch makes memcg_rstat_updated() re-entrant safe using this_cpu_*
> ops. On archs with CONFIG_ARCH_HAS_NMI_SAFE_THIS_CPU_OPS, this patch is
> also making memcg_rstat_updated() nmi safe.
>
> Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
> Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  mm/memcontrol.c | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 89476a71a18d..2464a58fbf17 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -505,8 +505,8 @@ struct memcg_vmstats_percpu {
>  	unsigned int			stats_updates;
>
>  	/* Cached pointers for fast iteration in memcg_rstat_updated() */
> -	struct memcg_vmstats_percpu	*parent;
> -	struct memcg_vmstats		*vmstats;
> +	struct memcg_vmstats_percpu __percpu	*parent_pcpu;
> +	struct memcg_vmstats			*vmstats;
>
>  	/* The above should fit a single cacheline for memcg_rstat_updated() */
>
> @@ -588,16 +588,21 @@ static bool memcg_vmstats_needs_flush(struct memcg_vmstats *vmstats)
>
>  static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
>  {
> +	struct memcg_vmstats_percpu __percpu *statc_pcpu;
>  	struct memcg_vmstats_percpu *statc;
> -	int cpu = smp_processor_id();
> +	int cpu;
>  	unsigned int stats_updates;
>
>  	if (!val)
>  		return;
>
> +	/* Don't assume callers have preemption disabled. */
> +	cpu = get_cpu();
> +
>  	cgroup_rstat_updated(memcg->css.cgroup, cpu);
> -	statc = this_cpu_ptr(memcg->vmstats_percpu);
> -	for (; statc; statc = statc->parent) {
> +	statc_pcpu = memcg->vmstats_percpu;
> +	for (; statc_pcpu; statc_pcpu = statc->parent_pcpu) {
> +		statc = this_cpu_ptr(statc_pcpu);
>  		/*
>  		 * If @memcg is already flushable then all its ancestors are
>  		 * flushable as well and also there is no need to increase
> @@ -606,14 +611,15 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
>  		if (memcg_vmstats_needs_flush(statc->vmstats))
>  			break;
>
> -		stats_updates = READ_ONCE(statc->stats_updates) + abs(val);
> -		WRITE_ONCE(statc->stats_updates, stats_updates);
> +		stats_updates = this_cpu_add_return(statc_pcpu->stats_updates,
> +						    abs(val));
>  		if (stats_updates < MEMCG_CHARGE_BATCH)
>  			continue;
>
> +		stats_updates = this_cpu_xchg(statc_pcpu->stats_updates, 0);
>  		atomic64_add(stats_updates, &statc->vmstats->stats_updates);
> -		WRITE_ONCE(statc->stats_updates, 0);
>  	}
> +	put_cpu();
>  }
>
>  static void __mem_cgroup_flush_stats(struct mem_cgroup *memcg, bool force)
> @@ -3691,7 +3697,7 @@ static void mem_cgroup_free(struct mem_cgroup *memcg)
>
>  static struct mem_cgroup *mem_cgroup_alloc(struct mem_cgroup *parent)
>  {
> -	struct memcg_vmstats_percpu *statc, *pstatc;
> +	struct memcg_vmstats_percpu *statc, __percpu *pstatc_pcpu;
>  	struct mem_cgroup *memcg;
>  	int node, cpu;
>  	int __maybe_unused i;
> @@ -3722,9 +3728,9 @@ static struct mem_cgroup *mem_cgroup_alloc(struct mem_cgroup *parent)
>
>  	for_each_possible_cpu(cpu) {
>  		if (parent)
> -			pstatc = per_cpu_ptr(parent->vmstats_percpu, cpu);
> +			pstatc_pcpu = parent->vmstats_percpu;
>  		statc = per_cpu_ptr(memcg->vmstats_percpu, cpu);
> -		statc->parent = parent ? pstatc : NULL;
> +		statc->parent_pcpu = parent ? pstatc_pcpu : NULL;
>  		statc->vmstats = memcg->vmstats;
>  	}
>
> --
> 2.47.1
>
>

----8<----
From 28275e5d054506746d310cf5ebd1fafdb0881dba Mon Sep 17 00:00:00 2001
From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Date: Thu, 15 May 2025 13:43:46 +0100
Subject: [PATCH] fix

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 mm/memcontrol.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 2464a58fbf17..40fcc2259e5f 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3697,7 +3697,8 @@ static void mem_cgroup_free(struct mem_cgroup *memcg)

 static struct mem_cgroup *mem_cgroup_alloc(struct mem_cgroup *parent)
 {
-	struct memcg_vmstats_percpu *statc, __percpu *pstatc_pcpu;
+	struct memcg_vmstats_percpu *statc;
+	struct memcg_vmstats_percpu __percpu *pstatc_pcpu;
 	struct mem_cgroup *memcg;
 	int node, cpu;
 	int __maybe_unused i;
--
2.49.0

Re: [PATCH v2 1/7] memcg: memcg_rstat_updated re-entrant safe against irqs

[Shakeel Butt]

On Thu, May 15, 2025 at 5:47 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> Shakeel - This breaks the build in mm-new for me:
>
>   CC      mm/pt_reclaim.o
> In file included from ./arch/x86/include/asm/rmwcc.h:5,
>                  from ./arch/x86/include/asm/bitops.h:18,
>                  from ./include/linux/bitops.h:68,
>                  from ./include/linux/radix-tree.h:11,
>                  from ./include/linux/idr.h:15,
>                  from ./include/linux/cgroup-defs.h:13,
>                  from mm/memcontrol.c:28:
> mm/memcontrol.c: In function ‘mem_cgroup_alloc’:
> ./arch/x86/include/asm/percpu.h:39:45: error: expected identifier or ‘(’ before ‘__seg_gs’
>    39 | #define __percpu_seg_override   CONCATENATE(__seg_, __percpu_seg)
>       |                                             ^~~~~~
> ./include/linux/args.h:25:24: note: in definition of macro ‘__CONCAT’
>    25 | #define __CONCAT(a, b) a ## b
>       |                        ^
> ./arch/x86/include/asm/percpu.h:39:33: note: in expansion of macro ‘CONCATENATE’
>    39 | #define __percpu_seg_override   CONCATENATE(__seg_, __percpu_seg)
>       |                                 ^~~~~~~~~~~
> ./arch/x86/include/asm/percpu.h:93:33: note: in expansion of macro ‘__percpu_seg_override’
>    93 | # define __percpu_qual          __percpu_seg_override
>       |                                 ^~~~~~~~~~~~~~~~~~~~~
> ././include/linux/compiler_types.h:60:25: note: in expansion of macro ‘__percpu_qual’
>    60 | # define __percpu       __percpu_qual BTF_TYPE_TAG(percpu)
>       |                         ^~~~~~~~~~~~~
> mm/memcontrol.c:3700:45: note: in expansion of macro ‘__percpu’
>  3700 |         struct memcg_vmstats_percpu *statc, __percpu *pstatc_pcpu;
>       |                                             ^~~~~~~~
> mm/memcontrol.c:3731:25: error: ‘pstatc_pcpu’ undeclared (first use in this function); did you mean ‘kstat_cpu’?
>  3731 |                         pstatc_pcpu = parent->vmstats_percpu;
>       |                         ^~~~~~~~~~~
>       |                         kstat_cpu
> mm/memcontrol.c:3731:25: note: each undeclared identifier is reported only once for each function it appears in
>
> The __percpu macro seems to be a bit screwy with comma-delimited decls, as it
> seems that putting this on its own line fixes this problem:
>

Which compiler (and version) is this? Thanks for the fix.

Re: [PATCH v2 1/7] memcg: memcg_rstat_updated re-entrant safe against irqs

[Lorenzo Stoakes]

On Thu, May 15, 2025 at 07:31:09AM -0700, Shakeel Butt wrote:
> On Thu, May 15, 2025 at 5:47 AM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > Shakeel - This breaks the build in mm-new for me:
> >
> >   CC      mm/pt_reclaim.o
> > In file included from ./arch/x86/include/asm/rmwcc.h:5,
> >                  from ./arch/x86/include/asm/bitops.h:18,
> >                  from ./include/linux/bitops.h:68,
> >                  from ./include/linux/radix-tree.h:11,
> >                  from ./include/linux/idr.h:15,
> >                  from ./include/linux/cgroup-defs.h:13,
> >                  from mm/memcontrol.c:28:
> > mm/memcontrol.c: In function ‘mem_cgroup_alloc’:
> > ./arch/x86/include/asm/percpu.h:39:45: error: expected identifier or ‘(’ before ‘__seg_gs’
> >    39 | #define __percpu_seg_override   CONCATENATE(__seg_, __percpu_seg)
> >       |                                             ^~~~~~
> > ./include/linux/args.h:25:24: note: in definition of macro ‘__CONCAT’
> >    25 | #define __CONCAT(a, b) a ## b
> >       |                        ^
> > ./arch/x86/include/asm/percpu.h:39:33: note: in expansion of macro ‘CONCATENATE’
> >    39 | #define __percpu_seg_override   CONCATENATE(__seg_, __percpu_seg)
> >       |                                 ^~~~~~~~~~~
> > ./arch/x86/include/asm/percpu.h:93:33: note: in expansion of macro ‘__percpu_seg_override’
> >    93 | # define __percpu_qual          __percpu_seg_override
> >       |                                 ^~~~~~~~~~~~~~~~~~~~~
> > ././include/linux/compiler_types.h:60:25: note: in expansion of macro ‘__percpu_qual’
> >    60 | # define __percpu       __percpu_qual BTF_TYPE_TAG(percpu)
> >       |                         ^~~~~~~~~~~~~
> > mm/memcontrol.c:3700:45: note: in expansion of macro ‘__percpu’
> >  3700 |         struct memcg_vmstats_percpu *statc, __percpu *pstatc_pcpu;
> >       |                                             ^~~~~~~~
> > mm/memcontrol.c:3731:25: error: ‘pstatc_pcpu’ undeclared (first use in this function); did you mean ‘kstat_cpu’?
> >  3731 |                         pstatc_pcpu = parent->vmstats_percpu;
> >       |                         ^~~~~~~~~~~
> >       |                         kstat_cpu
> > mm/memcontrol.c:3731:25: note: each undeclared identifier is reported only once for each function it appears in
> >
> > The __percpu macro seems to be a bit screwy with comma-delimited decls, as it
> > seems that putting this on its own line fixes this problem:
> >
>
> Which compiler (and version) is this? Thanks for the fix.

gcc 15, but apparently 13, 14 also fail. It seems independent of config.

Re: [PATCH v2 1/7] memcg: memcg_rstat_updated re-entrant safe against irqs

[Vlastimil Babka]

On 5/15/25 16:31, Shakeel Butt wrote:
> On Thu, May 15, 2025 at 5:47 AM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
>>
>> Shakeel - This breaks the build in mm-new for me:
>>
>>   CC      mm/pt_reclaim.o
>> In file included from ./arch/x86/include/asm/rmwcc.h:5,
>>                  from ./arch/x86/include/asm/bitops.h:18,
>>                  from ./include/linux/bitops.h:68,
>>                  from ./include/linux/radix-tree.h:11,
>>                  from ./include/linux/idr.h:15,
>>                  from ./include/linux/cgroup-defs.h:13,
>>                  from mm/memcontrol.c:28:
>> mm/memcontrol.c: In function ‘mem_cgroup_alloc’:
>> ./arch/x86/include/asm/percpu.h:39:45: error: expected identifier or ‘(’ before ‘__seg_gs’
>>    39 | #define __percpu_seg_override   CONCATENATE(__seg_, __percpu_seg)
>>       |                                             ^~~~~~
>> ./include/linux/args.h:25:24: note: in definition of macro ‘__CONCAT’
>>    25 | #define __CONCAT(a, b) a ## b
>>       |                        ^
>> ./arch/x86/include/asm/percpu.h:39:33: note: in expansion of macro ‘CONCATENATE’
>>    39 | #define __percpu_seg_override   CONCATENATE(__seg_, __percpu_seg)
>>       |                                 ^~~~~~~~~~~
>> ./arch/x86/include/asm/percpu.h:93:33: note: in expansion of macro ‘__percpu_seg_override’
>>    93 | # define __percpu_qual          __percpu_seg_override
>>       |                                 ^~~~~~~~~~~~~~~~~~~~~
>> ././include/linux/compiler_types.h:60:25: note: in expansion of macro ‘__percpu_qual’
>>    60 | # define __percpu       __percpu_qual BTF_TYPE_TAG(percpu)
>>       |                         ^~~~~~~~~~~~~
>> mm/memcontrol.c:3700:45: note: in expansion of macro ‘__percpu’
>>  3700 |         struct memcg_vmstats_percpu *statc, __percpu *pstatc_pcpu;
>>       |                                             ^~~~~~~~
>> mm/memcontrol.c:3731:25: error: ‘pstatc_pcpu’ undeclared (first use in this function); did you mean ‘kstat_cpu’?
>>  3731 |                         pstatc_pcpu = parent->vmstats_percpu;
>>       |                         ^~~~~~~~~~~
>>       |                         kstat_cpu
>> mm/memcontrol.c:3731:25: note: each undeclared identifier is reported only once for each function it appears in
>>
>> The __percpu macro seems to be a bit screwy with comma-delimited decls, as it
>> seems that putting this on its own line fixes this problem:
>>
> 
> Which compiler (and version) is this? Thanks for the fix.

Hm right I see the same errors with gcc 7, 13, 14, 15 but not with clang.

Re: [PATCH v2 1/7] memcg: memcg_rstat_updated re-entrant safe against irqs

[Shakeel Butt]

On Thu, May 15, 2025 at 04:57:10PM +0200, Vlastimil Babka wrote:
> On 5/15/25 16:31, Shakeel Butt wrote:
> > On Thu, May 15, 2025 at 5:47 AM Lorenzo Stoakes
> > <lorenzo.stoakes@oracle.com> wrote:
> >>
> >> Shakeel - This breaks the build in mm-new for me:
> >>
> >>   CC      mm/pt_reclaim.o
> >> In file included from ./arch/x86/include/asm/rmwcc.h:5,
> >>                  from ./arch/x86/include/asm/bitops.h:18,
> >>                  from ./include/linux/bitops.h:68,
> >>                  from ./include/linux/radix-tree.h:11,
> >>                  from ./include/linux/idr.h:15,
> >>                  from ./include/linux/cgroup-defs.h:13,
> >>                  from mm/memcontrol.c:28:
> >> mm/memcontrol.c: In function ‘mem_cgroup_alloc’:
> >> ./arch/x86/include/asm/percpu.h:39:45: error: expected identifier or ‘(’ before ‘__seg_gs’
> >>    39 | #define __percpu_seg_override   CONCATENATE(__seg_, __percpu_seg)
> >>       |                                             ^~~~~~
> >> ./include/linux/args.h:25:24: note: in definition of macro ‘__CONCAT’
> >>    25 | #define __CONCAT(a, b) a ## b
> >>       |                        ^
> >> ./arch/x86/include/asm/percpu.h:39:33: note: in expansion of macro ‘CONCATENATE’
> >>    39 | #define __percpu_seg_override   CONCATENATE(__seg_, __percpu_seg)
> >>       |                                 ^~~~~~~~~~~
> >> ./arch/x86/include/asm/percpu.h:93:33: note: in expansion of macro ‘__percpu_seg_override’
> >>    93 | # define __percpu_qual          __percpu_seg_override
> >>       |                                 ^~~~~~~~~~~~~~~~~~~~~
> >> ././include/linux/compiler_types.h:60:25: note: in expansion of macro ‘__percpu_qual’
> >>    60 | # define __percpu       __percpu_qual BTF_TYPE_TAG(percpu)
> >>       |                         ^~~~~~~~~~~~~
> >> mm/memcontrol.c:3700:45: note: in expansion of macro ‘__percpu’
> >>  3700 |         struct memcg_vmstats_percpu *statc, __percpu *pstatc_pcpu;
> >>       |                                             ^~~~~~~~
> >> mm/memcontrol.c:3731:25: error: ‘pstatc_pcpu’ undeclared (first use in this function); did you mean ‘kstat_cpu’?
> >>  3731 |                         pstatc_pcpu = parent->vmstats_percpu;
> >>       |                         ^~~~~~~~~~~
> >>       |                         kstat_cpu
> >> mm/memcontrol.c:3731:25: note: each undeclared identifier is reported only once for each function it appears in
> >>
> >> The __percpu macro seems to be a bit screwy with comma-delimited decls, as it
> >> seems that putting this on its own line fixes this problem:
> >>
> > 
> > Which compiler (and version) is this? Thanks for the fix.
> 
> Hm right I see the same errors with gcc 7, 13, 14, 15 but not with clang.

It seems to work with gcc 11.5.0, so weird.

Re: [PATCH v2 1/7] memcg: memcg_rstat_updated re-entrant safe against irqs

[Shakeel Butt]

On Thu, May 15, 2025 at 03:53:17PM +0100, Lorenzo Stoakes wrote:
> On Thu, May 15, 2025 at 07:31:09AM -0700, Shakeel Butt wrote:
> > On Thu, May 15, 2025 at 5:47 AM Lorenzo Stoakes
> > <lorenzo.stoakes@oracle.com> wrote:
> > >
> > > Shakeel - This breaks the build in mm-new for me:
> > >
> > >   CC      mm/pt_reclaim.o
> > > In file included from ./arch/x86/include/asm/rmwcc.h:5,
> > >                  from ./arch/x86/include/asm/bitops.h:18,
> > >                  from ./include/linux/bitops.h:68,
> > >                  from ./include/linux/radix-tree.h:11,
> > >                  from ./include/linux/idr.h:15,
> > >                  from ./include/linux/cgroup-defs.h:13,
> > >                  from mm/memcontrol.c:28:
> > > mm/memcontrol.c: In function ‘mem_cgroup_alloc’:
> > > ./arch/x86/include/asm/percpu.h:39:45: error: expected identifier or ‘(’ before ‘__seg_gs’
> > >    39 | #define __percpu_seg_override   CONCATENATE(__seg_, __percpu_seg)
> > >       |                                             ^~~~~~
> > > ./include/linux/args.h:25:24: note: in definition of macro ‘__CONCAT’
> > >    25 | #define __CONCAT(a, b) a ## b
> > >       |                        ^
> > > ./arch/x86/include/asm/percpu.h:39:33: note: in expansion of macro ‘CONCATENATE’
> > >    39 | #define __percpu_seg_override   CONCATENATE(__seg_, __percpu_seg)
> > >       |                                 ^~~~~~~~~~~
> > > ./arch/x86/include/asm/percpu.h:93:33: note: in expansion of macro ‘__percpu_seg_override’
> > >    93 | # define __percpu_qual          __percpu_seg_override
> > >       |                                 ^~~~~~~~~~~~~~~~~~~~~
> > > ././include/linux/compiler_types.h:60:25: note: in expansion of macro ‘__percpu_qual’
> > >    60 | # define __percpu       __percpu_qual BTF_TYPE_TAG(percpu)
> > >       |                         ^~~~~~~~~~~~~
> > > mm/memcontrol.c:3700:45: note: in expansion of macro ‘__percpu’
> > >  3700 |         struct memcg_vmstats_percpu *statc, __percpu *pstatc_pcpu;
> > >       |                                             ^~~~~~~~
> > > mm/memcontrol.c:3731:25: error: ‘pstatc_pcpu’ undeclared (first use in this function); did you mean ‘kstat_cpu’?
> > >  3731 |                         pstatc_pcpu = parent->vmstats_percpu;
> > >       |                         ^~~~~~~~~~~
> > >       |                         kstat_cpu
> > > mm/memcontrol.c:3731:25: note: each undeclared identifier is reported only once for each function it appears in
> > >
> > > The __percpu macro seems to be a bit screwy with comma-delimited decls, as it
> > > seems that putting this on its own line fixes this problem:
> > >
> >
> > Which compiler (and version) is this? Thanks for the fix.
> 
> gcc 15, but apparently 13, 14 also fail. It seems independent of config.

Thanks, somehow it works with gcc 11.5.0.

Re: [PATCH v2 1/7] memcg: memcg_rstat_updated re-entrant safe against irqs

[Lorenzo Stoakes]

On Thu, May 15, 2025 at 08:22:04AM -0700, Shakeel Butt wrote:
> On Thu, May 15, 2025 at 03:53:17PM +0100, Lorenzo Stoakes wrote:
> > On Thu, May 15, 2025 at 07:31:09AM -0700, Shakeel Butt wrote:
> > > On Thu, May 15, 2025 at 5:47 AM Lorenzo Stoakes
> > > <lorenzo.stoakes@oracle.com> wrote:
> > > >
> > > > Shakeel - This breaks the build in mm-new for me:
> > > >
> > > >   CC      mm/pt_reclaim.o
> > > > In file included from ./arch/x86/include/asm/rmwcc.h:5,
> > > >                  from ./arch/x86/include/asm/bitops.h:18,
> > > >                  from ./include/linux/bitops.h:68,
> > > >                  from ./include/linux/radix-tree.h:11,
> > > >                  from ./include/linux/idr.h:15,
> > > >                  from ./include/linux/cgroup-defs.h:13,
> > > >                  from mm/memcontrol.c:28:
> > > > mm/memcontrol.c: In function ‘mem_cgroup_alloc’:
> > > > ./arch/x86/include/asm/percpu.h:39:45: error: expected identifier or ‘(’ before ‘__seg_gs’
> > > >    39 | #define __percpu_seg_override   CONCATENATE(__seg_, __percpu_seg)
> > > >       |                                             ^~~~~~
> > > > ./include/linux/args.h:25:24: note: in definition of macro ‘__CONCAT’
> > > >    25 | #define __CONCAT(a, b) a ## b
> > > >       |                        ^
> > > > ./arch/x86/include/asm/percpu.h:39:33: note: in expansion of macro ‘CONCATENATE’
> > > >    39 | #define __percpu_seg_override   CONCATENATE(__seg_, __percpu_seg)
> > > >       |                                 ^~~~~~~~~~~
> > > > ./arch/x86/include/asm/percpu.h:93:33: note: in expansion of macro ‘__percpu_seg_override’
> > > >    93 | # define __percpu_qual          __percpu_seg_override
> > > >       |                                 ^~~~~~~~~~~~~~~~~~~~~
> > > > ././include/linux/compiler_types.h:60:25: note: in expansion of macro ‘__percpu_qual’
> > > >    60 | # define __percpu       __percpu_qual BTF_TYPE_TAG(percpu)
> > > >       |                         ^~~~~~~~~~~~~
> > > > mm/memcontrol.c:3700:45: note: in expansion of macro ‘__percpu’
> > > >  3700 |         struct memcg_vmstats_percpu *statc, __percpu *pstatc_pcpu;
> > > >       |                                             ^~~~~~~~
> > > > mm/memcontrol.c:3731:25: error: ‘pstatc_pcpu’ undeclared (first use in this function); did you mean ‘kstat_cpu’?
> > > >  3731 |                         pstatc_pcpu = parent->vmstats_percpu;
> > > >       |                         ^~~~~~~~~~~
> > > >       |                         kstat_cpu
> > > > mm/memcontrol.c:3731:25: note: each undeclared identifier is reported only once for each function it appears in
> > > >
> > > > The __percpu macro seems to be a bit screwy with comma-delimited decls, as it
> > > > seems that putting this on its own line fixes this problem:
> > > >
> > >
> > > Which compiler (and version) is this? Thanks for the fix.
> >
> > gcc 15, but apparently 13, 14 also fail. It seems independent of config.
>
> Thanks, somehow it works with gcc 11.5.0.

That is... both extremely bizarre, and VERY gnu... haha

Re: [PATCH v2 1/7] memcg: memcg_rstat_updated re-entrant safe against irqs

[Alexei Starovoitov]

On Thu, May 15, 2025 at 5:47 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> ----8<----
> From 28275e5d054506746d310cf5ebd1fafdb0881dba Mon Sep 17 00:00:00 2001
> From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Date: Thu, 15 May 2025 13:43:46 +0100
> Subject: [PATCH] fix
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
>  mm/memcontrol.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 2464a58fbf17..40fcc2259e5f 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3697,7 +3697,8 @@ static void mem_cgroup_free(struct mem_cgroup *memcg)
>
>  static struct mem_cgroup *mem_cgroup_alloc(struct mem_cgroup *parent)
>  {
> -       struct memcg_vmstats_percpu *statc, __percpu *pstatc_pcpu;
> +       struct memcg_vmstats_percpu *statc;
> +       struct memcg_vmstats_percpu __percpu *pstatc_pcpu;
>         struct mem_cgroup *memcg;
>         int node, cpu;
>         int __maybe_unused i;
> --
> 2.49.0

Tested-by: Alexei Starovoitov <ast@kernel.org>

Andrew,

Please pick up Lorenzo's fix into mm-new.