memcg: cleanup the memcg stats interfaces

The memcg stats are safe against irq (and nmi) context and thus does not require disabling irqs. However some code paths for memcg stats also update the node level stats and use irq unsafe interface and thus require the users to disable irqs. However node level stats, on architectures with HAVE_CMPXCHG_LOCAL (all major ones), has interface which does not require irq disabling. Let's move memcg stats code to start using that interface for node level stats.

Signed-off-by Shakeel Butt <[email protected]>

---

The memcg stats are safe against irq (and nmi) context and thus does not require disabling irqs. However some code paths for memcg stats also update the node level stats and use irq unsafe interface and thus require the users to disable irqs. However node level stats, on architectures with HAVE_CMPXCHG_LOCAL (all major ones), has interface which does not require irq disabling. Let's move memcg stats code to start using that interface for node level stats.

Signed-off-by Shakeel Butt <[email protected]>

The memcg stats are safe against irq (and nmi) context and thus does not require disabling irqs. However some code paths for memcg stats also update the node level stats and use irq unsafe interface and thus require the users to disable irqs. However node level stats, on architectures with HAVE_CMPXCHG_LOCAL (all major ones), has interface which does not require irq disabling. Let's move memcg stats code to start using that interface for node level stats.

Signed-off-by Shakeel Butt <[email protected]>

---

 include/linux/memcontrol.h | 2 +-
 include/linux/vmstat.h     | 4 ++--
 mm/memcontrol.c            | 6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

include/linux/memcontrol.h +1 -1

index 8c0f15e5978f..f82fac2fd988 100644
@@ -1408,7 +1408,7 @@ static inline void __mod_lruvec_kmem_state(void *p, enum node_stat_item idx,
 {
 	struct page *page = virt_to_head_page(p);
 
-	__mod_node_page_state(page_pgdat(page), idx, val);
+	mod_node_page_state(page_pgdat(page), idx, val);
 }
 
 static inline void mod_lruvec_kmem_state(void *p, enum node_stat_item idx,

include/linux/vmstat.h +1 -1

index c287998908bf..11a37aaa4dd9 100644
@@ -557,7 +557,7 @@ static inline void mod_lruvec_page_state(struct page *page,
 static inline void __mod_lruvec_state(struct lruvec *lruvec,
 				      enum node_stat_item idx, int val)
 {
-	__mod_node_page_state(lruvec_pgdat(lruvec), idx, val);
+	mod_node_page_state(lruvec_pgdat(lruvec), idx, val);
 }
 
 static inline void mod_lruvec_state(struct lruvec *lruvec,

include/linux/vmstat.h +0 -0

 static inline void lruvec_stat_mod_folio(struct folio *folio,

mm/memcontrol.c +3 -3

index 025da46d9959..f4b8a6414ed3 100644
@@ -770,7 +770,7 @@ void __mod_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx,
 			int val)
 {
 	/* Update node */
-	__mod_node_page_state(lruvec_pgdat(lruvec), idx, val);
+	mod_node_page_state(lruvec_pgdat(lruvec), idx, val);
 
 	/* Update memcg and lruvec */
 	if (!mem_cgroup_disabled())

@@ -789,7 +789,7 @@ void __lruvec_stat_mod_folio(struct folio *folio, enum node_stat_item idx,
 	/* Untracked pages have no memcg, no lruvec. Update only the node */
 	if (!memcg) {
 		rcu_read_unlock();
-		__mod_node_page_state(pgdat, idx, val);
+		mod_node_page_state(pgdat, idx, val);
 		return;
 	}
 

@@ -815,7 +815,7 @@ void __mod_lruvec_kmem_state(void *p, enum node_stat_item idx, int val)
 	 * vmstats to keep it correct for the root memcg.
 	 */
 	if (!memcg) {
-		__mod_node_page_state(pgdat, idx, val);
+		mod_node_page_state(pgdat, idx, val);
 	} else {
 		lruvec = mem_cgroup_lruvec(memcg, pgdat);
 		__mod_lruvec_state(lruvec, idx, val);

Quoted 8 lines · Shakeel Butt

The memcg stats are safe against irq (and nmi) context and thus does not require disabling irqs. However some code paths for memcg stats also update the node level stats and use irq unsafe interface and thus require the users to disable irqs. However node level stats, on architectures with HAVE_CMPXCHG_LOCAL (all major ones), has interface which does not require irq disabling. Let's move memcg stats code to start using that interface for node level stats.

Quoted 2 lines

Signed-off-by Shakeel Butt <[email protected]>

---

Hello Shakeel,

We are seeing a regression in micromm/munmap benchmark with this patch, on arm64 - the benchmark mmmaps a lot of memory, memsets it, and measures the time taken to munmap. Please see below if my understanding of this patch is correct.

Quoted 13 lines · Dev Jain

Quoted 10 lines · Shakeel Butt

The memcg stats are safe against irq (and nmi) context and thus does not require disabling irqs. However some code paths for memcg stats also update the node level stats and use irq unsafe interface and thus require the users to disable irqs. However node level stats, on architectures with HAVE_CMPXCHG_LOCAL (all major ones), has interface which does not require irq disabling. Let's move memcg stats code to start using that interface for node level stats.

Signed-off-by Shakeel Butt <[email protected]>

---

Hello Shakeel,

Quoted 3 lines

We are seeing a regression in micromm/munmap benchmark with this patch, on arm64 - the benchmark mmmaps a lot of memory, memsets it, and measures the time taken to munmap. Please see below if my understanding of this patch is correct.

Thanks for the report. Are you seeing regression in just the benchmark or some real workload as well? Also how much regression are you seeing? I have a kernel rebot regression report [1] for this patch as well which says 2.6% regression and thus it was on the back-burner for now. I will take look at this again soon.

[1] https://lore.kernel.org/all/[email protected]/

Quoted 8 lines

Quoted 8 lines

Hello Shakeel,

We are seeing a regression in micromm/munmap benchmark with this patch, on arm64 - the benchmark mmmaps a lot of memory, memsets it, and measures the time taken to munmap. Please see below if my understanding of this patch is correct.

Quoted 6 lines

Thanks for the report. Are you seeing regression in just the benchmark or some real workload as well? Also how much regression are you seeing? I have a kernel rebot regression report [1] for this patch as well which says 2.6% regression and thus it was on the back-burner for now. I will take look at this again soon.

The munmap regression is ~24%. Haven't observed a regression in any other benchmark yet.

Quoted 12 lines

Quoted 12 lines

Quoted 6 lines

Hello Shakeel,

We are seeing a regression in micromm/munmap benchmark with this patch, on arm64 - the benchmark mmmaps a lot of memory, memsets it, and measures the time taken to munmap. Please see below if my understanding of this patch is correct.

Thanks for the report. Are you seeing regression in just the benchmark or some real workload as well? Also how much regression are you seeing? I have a kernel rebot regression report [1] for this patch as well which says 2.6% regression and thus it was on the back-burner for now. I will take look at this again soon.

Quoted 2 lines

The munmap regression is ~24%. Haven't observed a regression in any other benchmark yet.

Please share the code/benchmark which shows such regression, also if you can share the perf profile, that would be awesome.

Quoted 12 lines · Shakeel Butt

Quoted 12 lines

Quoted 12 lines

Quoted 6 lines

Hello Shakeel,

We are seeing a regression in micromm/munmap benchmark with this patch, on arm64 - the benchmark mmmaps a lot of memory, memsets it, and measures the time taken to munmap. Please see below if my understanding of this patch is correct.

Thanks for the report. Are you seeing regression in just the benchmark or some real workload as well? Also how much regression are you seeing? I have a kernel rebot regression report [1] for this patch as well which says 2.6% regression and thus it was on the back-burner for now. I will take look at this again soon.

Quoted 4 lines

Quoted 2 lines

The munmap regression is ~24%. Haven't observed a regression in any other benchmark yet.

Please share the code/benchmark which shows such regression, also if you can share the perf profile, that would be awesome.

https://gitlab.arm.com/tooling/fastpath/-/blob/main/containers/microbench/micromm.c
You can run this with
./micromm 0 munmap 10

Don't have a perf profile, I measured the time taken by above command, with and without the patch.

Quoted 15 lines · Dev Jain

Quoted 14 lines · Shakeel Butt

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

Hello Shakeel,

We are seeing a regression in micromm/munmap benchmark with this patch, on arm64 - the benchmark mmmaps a lot of memory, memsets it, and measures the time taken to munmap. Please see below if my understanding of this patch is correct.

Quoted 6 lines

Thanks for the report. Are you seeing regression in just the benchmark or some real workload as well? Also how much regression are you seeing? I have a kernel rebot regression report [1] for this patch as well which says 2.6% regression and thus it was on the back-burner for now. I will take look at this again soon.

The munmap regression is ~24%. Haven't observed a regression in any other benchmark yet.

Quoted 9 lines

Quoted 2 lines

Please share the code/benchmark which shows such regression, also if you can share the perf profile, that would be awesome.

https://gitlab.arm.com/tooling/fastpath/-/blob/main/containers/microbench/micromm.c
You can run this with
./micromm 0 munmap 10

Don't have a perf profile, I measured the time taken by above command, with and without the patch.

Hi Dev, can you please try the following patch?

From 40155feca7e7bc846800ab8449735bdb03164d6d Mon Sep 17 00:00:00 2001
From: Shakeel Butt <[email protected]>
Date: Wed, 4 Feb 2026 08:46:08 -0800
Subject: [PATCH] vmstat: use preempt disable instead of try_cmpxchg

Signed-off-by Shakeel Butt <[email protected]>

---
 include/linux/mmzone.h |  2 +-
 mm/vmstat.c            | 58 ++++++++++++++++++------------------------
 2 files changed, 26 insertions(+), 34 deletions(-)

include/linux/mmzone.h +1 -1

index 3e51190a55e4..499cd53efdd6 100644
@@ -776,7 +776,7 @@ struct per_cpu_zonestat {
 
 struct per_cpu_nodestat {
 	s8 stat_threshold;
-	s8 vm_node_stat_diff[NR_VM_NODE_STAT_ITEMS];
+	long vm_node_stat_diff[NR_VM_NODE_STAT_ITEMS];
 };
 
 #endif /* !__GENERATING_BOUNDS.H */

mm/vmstat.c +24 -32

index 86b14b0f77b5..0930695597bb 100644
@@ -377,7 +377,7 @@ void __mod_node_page_state(struct pglist_data *pgdat, enum node_stat_item item,
 				long delta)
 {
 	struct per_cpu_nodestat __percpu *pcp = pgdat->per_cpu_nodestats;
-	s8 __percpu *p = pcp->vm_node_stat_diff + item;
+	long __percpu *p = pcp->vm_node_stat_diff + item;
 	long x;
 	long t;
 

@@ -456,8 +456,8 @@ void __inc_zone_state(struct zone *zone, enum zone_stat_item item)
 void __inc_node_state(struct pglist_data *pgdat, enum node_stat_item item)
 {
 	struct per_cpu_nodestat __percpu *pcp = pgdat->per_cpu_nodestats;
-	s8 __percpu *p = pcp->vm_node_stat_diff + item;
-	s8 v, t;
+	long __percpu *p = pcp->vm_node_stat_diff + item;
+	long v, t;
 
 	VM_WARN_ON_ONCE(vmstat_item_in_bytes(item));
 

@@ -467,7 +467,7 @@ void __inc_node_state(struct pglist_data *pgdat, enum node_stat_item item)
 	v = __this_cpu_inc_return(*p);
 	t = __this_cpu_read(pcp->stat_threshold);
 	if (unlikely(v > t)) {
-		s8 overstep = t >> 1;
+		long overstep = t >> 1;
 
 		node_page_state_add(v + overstep, pgdat, item);
 		__this_cpu_write(*p, -overstep);

@@ -512,8 +512,8 @@ void __dec_zone_state(struct zone *zone, enum zone_stat_item item)
 void __dec_node_state(struct pglist_data *pgdat, enum node_stat_item item)
 {
 	struct per_cpu_nodestat __percpu *pcp = pgdat->per_cpu_nodestats;
-	s8 __percpu *p = pcp->vm_node_stat_diff + item;
-	s8 v, t;
+	long __percpu *p = pcp->vm_node_stat_diff + item;
+	long v, t;
 
 	VM_WARN_ON_ONCE(vmstat_item_in_bytes(item));
 

@@ -523,7 +523,7 @@ void __dec_node_state(struct pglist_data *pgdat, enum node_stat_item item)
 	v = __this_cpu_dec_return(*p);
 	t = __this_cpu_read(pcp->stat_threshold);
 	if (unlikely(v < - t)) {
-		s8 overstep = t >> 1;
+		long overstep = t >> 1;
 
 		node_page_state_add(v - overstep, pgdat, item);
 		__this_cpu_write(*p, overstep);

@@ -619,9 +619,8 @@ static inline void mod_node_state(struct pglist_data *pgdat,
        enum node_stat_item item, int delta, int overstep_mode)
 {
 	struct per_cpu_nodestat __percpu *pcp = pgdat->per_cpu_nodestats;
-	s8 __percpu *p = pcp->vm_node_stat_diff + item;
-	long n, t, z;
-	s8 o;
+	long __percpu *p = pcp->vm_node_stat_diff + item;
+	long o, n, t, z;
 
 	if (vmstat_item_in_bytes(item)) {
 		/*

@@ -634,32 +633,25 @@ static inline void mod_node_state(struct pglist_data *pgdat,
 		delta >>= PAGE_SHIFT;
 	}
 
+	preempt_disable();
+
 	o = this_cpu_read(*p);
-	do {
-		z = 0;  /* overflow to node counters */
+	n = o + delta;
 
-		/*
-		 * The fetching of the stat_threshold is racy. We may apply
-		 * a counter threshold to the wrong the cpu if we get
-		 * rescheduled while executing here. However, the next
-		 * counter update will apply the threshold again and
-		 * therefore bring the counter under the threshold again.
-		 *
-		 * Most of the time the thresholds are the same anyways
-		 * for all cpus in a node.
-		 */
-		t = this_cpu_read(pcp->stat_threshold);
+	t = this_cpu_read(pcp->stat_threshold);
+	z = 0;
 
-		n = delta + (long)o;
+	if (abs(n) > t) {
+		int os = overstep_mode * (t >> 1);
 
-		if (abs(n) > t) {
-			int os = overstep_mode * (t >> 1) ;
+		/* Overflow must be added to node counters */
+		z = n + os;
+		n = -os;
+	}
 
-			/* Overflow must be added to node counters */
-			z = n + os;
-			n = -os;
-		}
-	} while (!this_cpu_try_cmpxchg(*p, &o, n));
+	this_cpu_add(*p, n - o);
+
+	preempt_enable();
 
 	if (z)
 		node_page_state_add(z, pgdat, item);

@@ -866,7 +858,7 @@ static bool refresh_cpu_vm_stats(bool do_pagesets)
 		struct per_cpu_nodestat __percpu *p = pgdat->per_cpu_nodestats;
 
 		for (i = 0; i < NR_VM_NODE_STAT_ITEMS; i++) {
-			int v;
+			long v;
 
 			v = this_cpu_xchg(p->vm_node_stat_diff[i], 0);
 			if (v) {

Quoted 35 lines · Shakeel Butt

Quoted 15 lines · Dev Jain

Quoted 14 lines · Shakeel Butt

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

Hello Shakeel,

We are seeing a regression in micromm/munmap benchmark with this patch, on arm64 - the benchmark mmmaps a lot of memory, memsets it, and measures the time taken to munmap. Please see below if my understanding of this patch is correct.

Quoted 6 lines

Thanks for the report. Are you seeing regression in just the benchmark or some real workload as well? Also how much regression are you seeing? I have a kernel rebot regression report [1] for this patch as well which says 2.6% regression and thus it was on the back-burner for now. I will take look at this again soon.

The munmap regression is ~24%. Haven't observed a regression in any other benchmark yet.

Quoted 9 lines

Quoted 2 lines

Please share the code/benchmark which shows such regression, also if you can share the perf profile, that would be awesome.

https://gitlab.arm.com/tooling/fastpath/-/blob/main/containers/microbench/micromm.c
You can run this with
./micromm 0 munmap 10

Don't have a perf profile, I measured the time taken by above command, with and without the patch.

Hi Dev, can you please try the following patch?

From 40155feca7e7bc846800ab8449735bdb03164d6d Mon Sep 17 00:00:00 2001
From: Shakeel Butt <[email protected]>
Date: Wed, 4 Feb 2026 08:46:08 -0800
Subject: [PATCH] vmstat: use preempt disable instead of try_cmpxchg

Signed-off-by Shakeel Butt <[email protected]>

---

Thanks for looking into this.

But this doesn't solve it :( preempt_disable() contains a compiler barrier, probably that's why.

Also can you confirm whether my analysis of the regression was correct? Because if it was, then this diff looks wrong - AFAIU preempt_disable() won't stop an irq handler from interrupting the execution, so this will introduce a bug for code paths running in irq context.

Quoted 38 lines · Shakeel Butt

On Mon, Feb 02, 2026 at 02:23:54PM +0530, Dev Jain wrote:
On 02/02/26 10:24 am, Shakeel Butt wrote:
Hello Shakeel,

We are seeing a regression in micromm/munmap benchmark with this patch, on arm64 - the benchmark mmmaps a lot of memory, memsets it, and measures the time taken to munmap. Please see below if my understanding of this patch is correct.

Thanks for the report. Are you seeing regression in just the benchmark or some real workload as well? Also how much regression are you seeing? I have a kernel rebot regression report [1] for this patch as well which says 2.6% regression and thus it was on the back-burner for now. I will take look at this again soon.

The munmap regression is ~24%. Haven't observed a regression in any other benchmark yet.
Please share the code/benchmark which shows such regression, also if you can share the perf profile, that would be awesome.
https://gitlab.arm.com/tooling/fastpath/-/blob/main/containers/microbench/micromm.c
You can run this with
./micromm 0 munmap 10

Don't have a perf profile, I measured the time taken by above command, with and without the patch.

Hi Dev, can you please try the following patch?

From 40155feca7e7bc846800ab8449735bdb03164d6d Mon Sep 17 00:00:00 2001
From: Shakeel Butt <[email protected]>
Date: Wed, 4 Feb 2026 08:46:08 -0800
Subject: [PATCH] vmstat: use preempt disable instead of try_cmpxchg

Signed-off-by Shakeel Butt <[email protected]>

---

[...snip...]

Quoted 6 lines

Thanks for looking into this.

But this doesn't solve it :( preempt_disable() contains a compiler barrier, probably that's why.

I think the reason why it doesn't solve the regression is because of how arm64 implements this_cpu_add_8() and this_cpu_try_cmpxchg_8().

On arm64, IIUC both this_cpu_try_cmpxchg_8() and this_cpu_add_8() are implemented using LL/SC instructions or LSE atomics (if supported).

See:
- this_cpu_add_8()
-> __percpu_add_case_64
(which is generated from PERCPU_OP)

- this_cpu_try_cmpxchg_8()
-> __cpu_fallback_try_cmpxchg(..., this_cpu_cmpxchg_8) -> this_cpu_cmpxchg_8()
-> cmpxchg_relaxed()
-> raw_cmpxchg_relaxed()
-> arch_cmpxchg_relaxed()
-> __cmpxchg_wrapper()
-> __cmpxchg_case_64()
-> __lse_ll_sc_body(_cmpxchg_case_64, ...)

Quoted 6 lines

Also can you confirm whether my analysis of the regression was correct?
Because if it was, then this diff looks wrong - AFAIU preempt_disable() won't stop an irq handler from interrupting the execution, so this will introduce a bug for code paths running in irq context.

I was worried about the correctness too, but this_cpu_add() is safe against IRQs and so the stat will be _eventually_ consistent?

Ofc it's so confusing! Maybe I'm the one confused.

Quoted 56 lines · Dev Jain

Quoted 37 lines · Shakeel Butt

On Mon, Feb 02, 2026 at 02:23:54PM +0530, Dev Jain wrote:
On 02/02/26 10:24 am, Shakeel Butt wrote:
Hello Shakeel,

We are seeing a regression in micromm/munmap benchmark with this patch, on arm64 - the benchmark mmmaps a lot of memory, memsets it, and measures the time taken to munmap. Please see below if my understanding of this patch is correct.

Thanks for the report. Are you seeing regression in just the benchmark or some real workload as well? Also how much regression are you seeing? I have a kernel rebot regression report [1] for this patch as well which says 2.6% regression and thus it was on the back-burner for now. I will take look at this again soon.

The munmap regression is ~24%. Haven't observed a regression in any other benchmark yet.
Please share the code/benchmark which shows such regression, also if you can share the perf profile, that would be awesome.
https://gitlab.arm.com/tooling/fastpath/-/blob/main/containers/microbench/micromm.c
You can run this with
./micromm 0 munmap 10

Don't have a perf profile, I measured the time taken by above command, with and without the patch.

Hi Dev, can you please try the following patch?

From 40155feca7e7bc846800ab8449735bdb03164d6d Mon Sep 17 00:00:00 2001
From: Shakeel Butt <[email protected]>
Date: Wed, 4 Feb 2026 08:46:08 -0800
Subject: [PATCH] vmstat: use preempt disable instead of try_cmpxchg

Signed-off-by Shakeel Butt <[email protected]>

---

[...snip...]

Quoted 5 lines

Thanks for looking into this.

But this doesn't solve it :( preempt_disable() contains a compiler barrier, probably that's why.

I think the reason why it doesn't solve the regression is because of how arm64 implements this_cpu_add_8() and this_cpu_try_cmpxchg_8().

On arm64, IIUC both this_cpu_try_cmpxchg_8() and this_cpu_add_8() are implemented using LL/SC instructions or LSE atomics (if supported).

See:
- this_cpu_add_8()
-> __percpu_add_case_64
(which is generated from PERCPU_OP)

Quoted 10 lines

- this_cpu_try_cmpxchg_8()
-> __cpu_fallback_try_cmpxchg(..., this_cpu_cmpxchg_8) -> this_cpu_cmpxchg_8()
-> cmpxchg_relaxed()
-> raw_cmpxchg_relaxed()
-> arch_cmpxchg_relaxed()
-> __cmpxchg_wrapper()
-> __cmpxchg_case_64()
-> __lse_ll_sc_body(_cmpxchg_case_64, ...)

Oh so it is arm64 specific issue. I tested on x86-64 machine and it solves the little regression it had before. So, on arm64 all this_cpu_ops i.e. without double underscore, uses LL/SC instructions.

Need more thought on this.

Quoted 5 lines

Quoted 5 lines

Also can you confirm whether my analysis of the regression was correct?
Because if it was, then this diff looks wrong - AFAIU preempt_disable() won't stop an irq handler from interrupting the execution, so this will introduce a bug for code paths running in irq context.

Quoted 4 lines

I was worried about the correctness too, but this_cpu_add() is safe against IRQs and so the stat will be _eventually_ consistent?

Ofc it's so confusing! Maybe I'm the one confused.

Yeah there is no issue with proposed patch as it is making the function re-entrant safe.

Quoted 16 lines

Quoted 2 lines

Oh so it is arm64 specific issue. I tested on x86-64 machine and it solves the little regression it had before. So, on arm64 all this_cpu_ops i.e. without double underscore, uses LL/SC instructions.

Need more thought on this.

Quoted 8 lines

Quoted 5 lines

Also can you confirm whether my analysis of the regression was correct?
Because if it was, then this diff looks wrong - AFAIU preempt_disable() won't stop an irq handler from interrupting the execution, so this will introduce a bug for code paths running in irq context.

I was worried about the correctness too, but this_cpu_add() is safe against IRQs and so the stat will be _eventually_ consistent?

Quoted 3 lines

Quoted 1 line

Ofc it's so confusing! Maybe I'm the one confused.

Yeah there is no issue with proposed patch as it is making the function re-entrant safe.

Ah yes, this_cpu_add() does the addition in one shot without read-modify-write.

I am still puzzled whether the original patch was a bug fix or an optimization.

The patch description says that node stat updation uses irq unsafe interface. Therefore, we had foo() calling __foo() nested with local_irq_save/restore. But there were code paths which directly called __foo() - so, your patch fixes a bug right

On Tue, Feb 10, 2026 at 01:08:49PM +0530, Dev Jain wrote:
[...]

Quoted 14 lines

Quoted 14 lines

Oh so it is arm64 specific issue. I tested on x86-64 machine and it solves the little regression it had before. So, on arm64 all this_cpu_ops i.e. without double underscore, uses LL/SC instructions.

Need more thought on this.

Quoted 8 lines

Quoted 5 lines

Also can you confirm whether my analysis of the regression was correct?
Because if it was, then this diff looks wrong - AFAIU preempt_disable() won't stop an irq handler from interrupting the execution, so this will introduce a bug for code paths running in irq context.

I was worried about the correctness too, but this_cpu_add() is safe against IRQs and so the stat will be _eventually_ consistent?

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Ofc it's so confusing! Maybe I'm the one confused.

Yeah there is no issue with proposed patch as it is making the function re-entrant safe.

Ah yes, this_cpu_add() does the addition in one shot without read-modify-write.

I am still puzzled whether the original patch was a bug fix or an optimization.

The original patch was a cleanup patch. The memcg stats update functions were already irq/nmi safe without disabling irqs and that patch did the same for the numa stats. Though it seems like that is causing regression for arm64 as this_cpu* ops are expensive on arm64.

Quoted 3 lines

The patch description says that node stat updation uses irq unsafe interface. Therefore, we had foo() calling __foo() nested with local_irq_save/restore. But there were code paths which directly called __foo() - so, your patch fixes a bug right

No, those places were already disabling irqs and should be fine.

I am working on adding batched stats update functionality in the hope that will fix the regression.

Quoted 27 lines · Shakeel Butt

On Tue, Feb 10, 2026 at 01:08:49PM +0530, Dev Jain wrote:
[...]

Quoted 14 lines

Quoted 14 lines

Oh so it is arm64 specific issue. I tested on x86-64 machine and it solves the little regression it had before. So, on arm64 all this_cpu_ops i.e. without double underscore, uses LL/SC instructions.

Need more thought on this.

Quoted 8 lines

Quoted 5 lines

Also can you confirm whether my analysis of the regression was correct?
Because if it was, then this diff looks wrong - AFAIU preempt_disable() won't stop an irq handler from interrupting the execution, so this will introduce a bug for code paths running in irq context.

I was worried about the correctness too, but this_cpu_add() is safe against IRQs and so the stat will be _eventually_ consistent?

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Ofc it's so confusing! Maybe I'm the one confused.

Yeah there is no issue with proposed patch as it is making the function re-entrant safe.

Ah yes, this_cpu_add() does the addition in one shot without read-modify-write.

I am still puzzled whether the original patch was a bug fix or an optimization.

The original patch was a cleanup patch. The memcg stats update functions were already irq/nmi safe without disabling irqs and that patch did the same for the numa stats. Though it seems like that is causing regression for arm64 as this_cpu* ops are expensive on arm64.

Quoted 4 lines

Quoted 3 lines

The patch description says that node stat updation uses irq unsafe interface. Therefore, we had foo() calling __foo() nested with local_irq_save/restore. But there were code paths which directly called __foo() - so, your patch fixes a bug right

No, those places were already disabling irqs and should be fine.

Please correct me if I am missing something here. Simply putting an if (!irqs_disabled()) -> dump_stack() in __lruvec_stat_mod_folio, before calling __mod_node_page_state, reveals:

[ 6.486375] Call trace:
[ 6.486376] show_stack+0x20/0x38 (C)
[ 6.486379] dump_stack_lvl+0x74/0x90
[ 6.486382] dump_stack+0x18/0x28
[ 6.486383] __lruvec_stat_mod_folio+0x160/0x180
[ 6.486385] folio_add_file_rmap_ptes+0x128/0x480
[ 6.486388] set_pte_range+0xe8/0x320
[ 6.486389] finish_fault+0x260/0x508
[ 6.486390] do_fault+0x2d0/0x598
[ 6.486391] __handle_mm_fault+0x398/0xb60
[ 6.486393] handle_mm_fault+0x15c/0x298
[ 6.486394] __get_user_pages+0x204/0xb88
[ 6.486395] populate_vma_page_range+0xbc/0x1b8
[ 6.486396] __mm_populate+0xcc/0x1e0
[ 6.486397] __arm64_sys_mlockall+0x1d4/0x1f8
[ 6.486398] invoke_syscall+0x50/0x120
[ 6.486399] el0_svc_common.constprop.0+0x48/0xf0
[ 6.486400] do_el0_svc+0x24/0x38
[ 6.486400] el0_svc+0x34/0xf0
[ 6.486402] el0t_64_sync_handler+0xa0/0xe8
[ 6.486404] el0t_64_sync+0x198/0x1a0

Indeed finish_fault() takes a PTL spin lock without irq disablement.

Quoted 2 lines

I am working on adding batched stats update functionality in the hope that will fix the regression.

Thanks! FYI, I have zeroed in the issue on to preempt_disable(). Dropping this from _pcpu_protect_return solves the regression.

Unlike x86, arm64 does a preempt_disable
when doing this_cpu_*. On a cursory look it seems like this is unnecessary - since we are doing preempt_enable() immediately after reading the pointer, CPU migration is possible anyways, so there is nothing to be gained by reading pcpu pointer with preemption disabled. I am investigating whether we can simply drop this in general.

We also thought that initially, but there's a sbutle race that can occur, and so we added code to disable preemption in commit:

f3eab7184ddcd486 ("arm64: percpu: Make this_cpu accessors pre-empt safe")

The problem on arm64 is that our atomics take a single base register, and we have to generate the percpu address with separate instructions from the atomic itself. That means we can get preempted between address generation and the atomic, which is problematic for sequences like:

	// Thread-A			// Thread-B

	this_cpu_add(var)
					local_irq_disable(flags)
					...
					v = __this_cpu_read(var);
					v = some_function(v);
					__this_cpu_write(var, v);
					...
					local_irq_restore(flags)

... which can unexpectedly race as:

	// Thread-A			// Thread-B

	< generate CPU X addr >
	< preempted >

					< scheduled on CPU X >
					local_irq_disable(flags);
					v = __this_cpu_read(var);

	< scheduled on CPU Y >
	< add to CPU X's var >
					v = some_function(v);
					__this_cpu_write(var, v);
					local_irq_restore(flags);

... and hence we lose an update to a percpu variable.

Quoted 60 lines · Dev Jain

Quoted 27 lines · Shakeel Butt

On Tue, Feb 10, 2026 at 01:08:49PM +0530, Dev Jain wrote:
[...]

Quoted 14 lines

Quoted 14 lines

Oh so it is arm64 specific issue. I tested on x86-64 machine and it solves the little regression it had before. So, on arm64 all this_cpu_ops i.e. without double underscore, uses LL/SC instructions.

Need more thought on this.

Quoted 8 lines

Quoted 5 lines

Also can you confirm whether my analysis of the regression was correct?
Because if it was, then this diff looks wrong - AFAIU preempt_disable() won't stop an irq handler from interrupting the execution, so this will introduce a bug for code paths running in irq context.

I was worried about the correctness too, but this_cpu_add() is safe against IRQs and so the stat will be _eventually_ consistent?

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Ofc it's so confusing! Maybe I'm the one confused.

Yeah there is no issue with proposed patch as it is making the function re-entrant safe.

Ah yes, this_cpu_add() does the addition in one shot without read-modify-write.

I am still puzzled whether the original patch was a bug fix or an optimization.

The original patch was a cleanup patch. The memcg stats update functions were already irq/nmi safe without disabling irqs and that patch did the same for the numa stats. Though it seems like that is causing regression for arm64 as this_cpu* ops are expensive on arm64.

Quoted 4 lines

Quoted 3 lines

The patch description says that node stat updation uses irq unsafe interface. Therefore, we had foo() calling __foo() nested with local_irq_save/restore. But there were code paths which directly called __foo() - so, your patch fixes a bug right

No, those places were already disabling irqs and should be fine.

Please correct me if I am missing something here. Simply putting an if (!irqs_disabled()) -> dump_stack() in __lruvec_stat_mod_folio, before calling __mod_node_page_state, reveals:

[ 6.486375] Call trace:
[ 6.486376] show_stack+0x20/0x38 (C)
[ 6.486379] dump_stack_lvl+0x74/0x90
[ 6.486382] dump_stack+0x18/0x28
[ 6.486383] __lruvec_stat_mod_folio+0x160/0x180
[ 6.486385] folio_add_file_rmap_ptes+0x128/0x480
[ 6.486388] set_pte_range+0xe8/0x320
[ 6.486389] finish_fault+0x260/0x508
[ 6.486390] do_fault+0x2d0/0x598
[ 6.486391] __handle_mm_fault+0x398/0xb60
[ 6.486393] handle_mm_fault+0x15c/0x298
[ 6.486394] __get_user_pages+0x204/0xb88
[ 6.486395] populate_vma_page_range+0xbc/0x1b8
[ 6.486396] __mm_populate+0xcc/0x1e0
[ 6.486397] __arm64_sys_mlockall+0x1d4/0x1f8
[ 6.486398] invoke_syscall+0x50/0x120
[ 6.486399] el0_svc_common.constprop.0+0x48/0xf0
[ 6.486400] do_el0_svc+0x24/0x38
[ 6.486400] el0_svc+0x34/0xf0
[ 6.486402] el0t_64_sync_handler+0xa0/0xe8
[ 6.486404] el0t_64_sync+0x198/0x1a0

Quoted 1 line

Indeed finish_fault() takes a PTL spin lock without irq disablement.

That indeed looks incorrect to me.
I was assuming __foo() is always called with IRQs disabled!

Quoted 6 lines

Quoted 3 lines

I am working on adding batched stats update functionality in the hope that will fix the regression.

Thanks! FYI, I have zeroed in the issue on to preempt_disable(). Dropping this from _pcpu_protect_return solves the regression.

That's interesting, why is the cost of preempt disable/enable so high?

Quoted 5 lines

Unlike x86, arm64 does a preempt_disable
when doing this_cpu_*. On a cursory look it seems like this is unnecessary - since we are doing preempt_enable() immediately after reading the pointer, CPU migration is possible anyways, so there is nothing to be gained by reading pcpu pointer with preemption disabled. I am investigating whether we can simply drop this in general.

... so, removing preempt disable _in general_ is probably not a good idea.

[1] https://lore.kernel.org/all/[email protected]

Signature · 2 lines

Cheers,
Harry / Hyeonggon

Quoted 61 lines · Harry Yoo

Quoted 60 lines · Dev Jain

Quoted 27 lines · Shakeel Butt

On Tue, Feb 10, 2026 at 01:08:49PM +0530, Dev Jain wrote:
[...]

Quoted 14 lines

Quoted 14 lines

Oh so it is arm64 specific issue. I tested on x86-64 machine and it solves the little regression it had before. So, on arm64 all this_cpu_ops i.e. without double underscore, uses LL/SC instructions.

Need more thought on this.

Quoted 8 lines

Quoted 5 lines

Also can you confirm whether my analysis of the regression was correct?
Because if it was, then this diff looks wrong - AFAIU preempt_disable() won't stop an irq handler from interrupting the execution, so this will introduce a bug for code paths running in irq context.

I was worried about the correctness too, but this_cpu_add() is safe against IRQs and so the stat will be _eventually_ consistent?

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Ofc it's so confusing! Maybe I'm the one confused.

Yeah there is no issue with proposed patch as it is making the function re-entrant safe.

Ah yes, this_cpu_add() does the addition in one shot without read-modify-write.

I am still puzzled whether the original patch was a bug fix or an optimization.

The original patch was a cleanup patch. The memcg stats update functions were already irq/nmi safe without disabling irqs and that patch did the same for the numa stats. Though it seems like that is causing regression for arm64 as this_cpu* ops are expensive on arm64.

Quoted 4 lines

Quoted 3 lines

The patch description says that node stat updation uses irq unsafe interface. Therefore, we had foo() calling __foo() nested with local_irq_save/restore. But there were code paths which directly called __foo() - so, your patch fixes a bug right

No, those places were already disabling irqs and should be fine.

Please correct me if I am missing something here. Simply putting an if (!irqs_disabled()) -> dump_stack() in __lruvec_stat_mod_folio, before calling __mod_node_page_state, reveals:

[ 6.486375] Call trace:
[ 6.486376] show_stack+0x20/0x38 (C)
[ 6.486379] dump_stack_lvl+0x74/0x90
[ 6.486382] dump_stack+0x18/0x28
[ 6.486383] __lruvec_stat_mod_folio+0x160/0x180
[ 6.486385] folio_add_file_rmap_ptes+0x128/0x480
[ 6.486388] set_pte_range+0xe8/0x320
[ 6.486389] finish_fault+0x260/0x508
[ 6.486390] do_fault+0x2d0/0x598
[ 6.486391] __handle_mm_fault+0x398/0xb60
[ 6.486393] handle_mm_fault+0x15c/0x298
[ 6.486394] __get_user_pages+0x204/0xb88
[ 6.486395] populate_vma_page_range+0xbc/0x1b8
[ 6.486396] __mm_populate+0xcc/0x1e0
[ 6.486397] __arm64_sys_mlockall+0x1d4/0x1f8
[ 6.486398] invoke_syscall+0x50/0x120
[ 6.486399] el0_svc_common.constprop.0+0x48/0xf0
[ 6.486400] do_el0_svc+0x24/0x38
[ 6.486400] el0_svc+0x34/0xf0
[ 6.486402] el0t_64_sync_handler+0xa0/0xe8
[ 6.486404] el0t_64_sync+0x198/0x1a0

Quoted 4 lines

Quoted 1 line

Indeed finish_fault() takes a PTL spin lock without irq disablement.

That indeed looks incorrect to me.
I was assuming __foo() is always called with IRQs disabled!

Not necessarily. For stats which never get updated in IRQ context, can be updated using __foo() with just premption disabled.

Quoted 59 lines · Harry Yoo

Quoted 58 lines · Dev Jain

Quoted 27 lines · Shakeel Butt

On Tue, Feb 10, 2026 at 01:08:49PM +0530, Dev Jain wrote:
[...]

Quoted 14 lines

Quoted 14 lines

Oh so it is arm64 specific issue. I tested on x86-64 machine and it solves the little regression it had before. So, on arm64 all this_cpu_ops i.e. without double underscore, uses LL/SC instructions.

Need more thought on this.

Quoted 8 lines

Quoted 5 lines

Also can you confirm whether my analysis of the regression was correct?
Because if it was, then this diff looks wrong - AFAIU preempt_disable() won't stop an irq handler from interrupting the execution, so this will introduce a bug for code paths running in irq context.

I was worried about the correctness too, but this_cpu_add() is safe against IRQs and so the stat will be _eventually_ consistent?

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Ofc it's so confusing! Maybe I'm the one confused.

Yeah there is no issue with proposed patch as it is making the function re-entrant safe.

Ah yes, this_cpu_add() does the addition in one shot without read-modify-write.

I am still puzzled whether the original patch was a bug fix or an optimization.

The original patch was a cleanup patch. The memcg stats update functions were already irq/nmi safe without disabling irqs and that patch did the same for the numa stats. Though it seems like that is causing regression for arm64 as this_cpu* ops are expensive on arm64.

Quoted 4 lines

Quoted 3 lines

The patch description says that node stat updation uses irq unsafe interface. Therefore, we had foo() calling __foo() nested with local_irq_save/restore. But there were code paths which directly called __foo() - so, your patch fixes a bug right

No, those places were already disabling irqs and should be fine.

Please correct me if I am missing something here. Simply putting an if (!irqs_disabled()) -> dump_stack() in __lruvec_stat_mod_folio, before calling __mod_node_page_state, reveals:

[ 6.486375] Call trace:
[ 6.486376] show_stack+0x20/0x38 (C)
[ 6.486379] dump_stack_lvl+0x74/0x90
[ 6.486382] dump_stack+0x18/0x28
[ 6.486383] __lruvec_stat_mod_folio+0x160/0x180
[ 6.486385] folio_add_file_rmap_ptes+0x128/0x480
[ 6.486388] set_pte_range+0xe8/0x320
[ 6.486389] finish_fault+0x260/0x508
[ 6.486390] do_fault+0x2d0/0x598
[ 6.486391] __handle_mm_fault+0x398/0xb60
[ 6.486393] handle_mm_fault+0x15c/0x298
[ 6.486394] __get_user_pages+0x204/0xb88
[ 6.486395] populate_vma_page_range+0xbc/0x1b8
[ 6.486396] __mm_populate+0xcc/0x1e0
[ 6.486397] __arm64_sys_mlockall+0x1d4/0x1f8
[ 6.486398] invoke_syscall+0x50/0x120
[ 6.486399] el0_svc_common.constprop.0+0x48/0xf0
[ 6.486400] do_el0_svc+0x24/0x38
[ 6.486400] el0_svc+0x34/0xf0
[ 6.486402] el0t_64_sync_handler+0xa0/0xe8
[ 6.486404] el0t_64_sync+0x198/0x1a0

Quoted 3 lines

Quoted 1 line

Indeed finish_fault() takes a PTL spin lock without irq disablement.

That indeed looks incorrect to me.
I was assuming __foo() is always called with IRQs disabled!

Not necessarily. For stats which never get updated in IRQ context, can be updated using __foo() with just premption disabled.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

I am working on adding batched stats update functionality in the hope that will fix the regression.

Thanks! FYI, I have zeroed in the issue on to preempt_disable(). Dropping this from _pcpu_protect_return solves the regression.

That's interesting, why is the cost of preempt disable/enable so high?

What made you (Dev) so convinced that preempt_disable is that expensive.

Quoted 6 lines

Quoted 6 lines

Unlike x86, arm64 does a preempt_disable
when doing this_cpu_*. On a cursory look it seems like this is unnecessary - since we are doing preempt_enable() immediately after reading the pointer, CPU migration is possible anyways, so there is nothing to be gained by reading pcpu pointer with preemption disabled. I am investigating whether we can simply drop this in general.

[...]

Quoted 2 lines

... so, removing preempt disable _in general_ is probably not a good idea.

Yup, I agree here.

Quoted 6 lines

[1] https://lore.kernel.org/all/[email protected]

Signature · 2 lines

Cheers,
Harry / Hyeonggon

Quoted 58 lines · Dev Jain

Quoted 27 lines · Shakeel Butt

On Tue, Feb 10, 2026 at 01:08:49PM +0530, Dev Jain wrote:
[...]

Quoted 14 lines

Quoted 14 lines

Oh so it is arm64 specific issue. I tested on x86-64 machine and it solves the little regression it had before. So, on arm64 all this_cpu_ops i.e. without double underscore, uses LL/SC instructions.

Need more thought on this.

Quoted 8 lines

Quoted 5 lines

Also can you confirm whether my analysis of the regression was correct?
Because if it was, then this diff looks wrong - AFAIU preempt_disable() won't stop an irq handler from interrupting the execution, so this will introduce a bug for code paths running in irq context.

I was worried about the correctness too, but this_cpu_add() is safe against IRQs and so the stat will be _eventually_ consistent?

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Ofc it's so confusing! Maybe I'm the one confused.

Yeah there is no issue with proposed patch as it is making the function re-entrant safe.

Ah yes, this_cpu_add() does the addition in one shot without read-modify-write.

I am still puzzled whether the original patch was a bug fix or an optimization.

The original patch was a cleanup patch. The memcg stats update functions were already irq/nmi safe without disabling irqs and that patch did the same for the numa stats. Though it seems like that is causing regression for arm64 as this_cpu* ops are expensive on arm64.

Quoted 4 lines

Quoted 3 lines

The patch description says that node stat updation uses irq unsafe interface. Therefore, we had foo() calling __foo() nested with local_irq_save/restore. But there were code paths which directly called __foo() - so, your patch fixes a bug right

No, those places were already disabling irqs and should be fine.

Please correct me if I am missing something here. Simply putting an if (!irqs_disabled()) -> dump_stack() in __lruvec_stat_mod_folio, before calling __mod_node_page_state, reveals:

[ 6.486375] Call trace:
[ 6.486376] show_stack+0x20/0x38 (C)
[ 6.486379] dump_stack_lvl+0x74/0x90
[ 6.486382] dump_stack+0x18/0x28
[ 6.486383] __lruvec_stat_mod_folio+0x160/0x180
[ 6.486385] folio_add_file_rmap_ptes+0x128/0x480
[ 6.486388] set_pte_range+0xe8/0x320
[ 6.486389] finish_fault+0x260/0x508
[ 6.486390] do_fault+0x2d0/0x598
[ 6.486391] __handle_mm_fault+0x398/0xb60
[ 6.486393] handle_mm_fault+0x15c/0x298
[ 6.486394] __get_user_pages+0x204/0xb88
[ 6.486395] populate_vma_page_range+0xbc/0x1b8
[ 6.486396] __mm_populate+0xcc/0x1e0
[ 6.486397] __arm64_sys_mlockall+0x1d4/0x1f8
[ 6.486398] invoke_syscall+0x50/0x120
[ 6.486399] el0_svc_common.constprop.0+0x48/0xf0
[ 6.486400] do_el0_svc+0x24/0x38
[ 6.486400] el0_svc+0x34/0xf0
[ 6.486402] el0t_64_sync_handler+0xa0/0xe8
[ 6.486404] el0t_64_sync+0x198/0x1a0

Quoted 1 line

Indeed finish_fault() takes a PTL spin lock without irq disablement.

That indeed looks incorrect to me.
I was assuming __foo() is always called with IRQs disabled!

Quoted 4 lines

Quoted 2 lines

I am working on adding batched stats update functionality in the hope that will fix the regression.

Thanks! FYI, I have zeroed in the issue on to preempt_disable(). Dropping this from _pcpu_protect_return solves the regression.

That's interesting, why is the cost of preempt disable/enable so high?

Quoted 5 lines

Unlike x86, arm64 does a preempt_disable
when doing this_cpu_*. On a cursory look it seems like this is unnecessary - since we are doing preempt_enable() immediately after reading the pointer, CPU migration is possible anyways, so there is nothing to be gained by reading pcpu pointer with preemption disabled. I am investigating whether we can simply drop this in general.

Let me quote an old email from Mark Rutland [1]:

Quoted 39 lines

We also thought that initially, but there's a sbutle race that can occur, and so we added code to disable preemption in commit:

f3eab7184ddcd486 ("arm64: percpu: Make this_cpu accessors pre-empt safe")

The problem on arm64 is that our atomics take a single base register, and we have to generate the percpu address with separate instructions from the atomic itself. That means we can get preempted between address generation and the atomic, which is problematic for sequences like:

	// Thread-A			// Thread-B

	this_cpu_add(var)
					local_irq_disable(flags)
					...
					v = __this_cpu_read(var);
					v = some_function(v);
					__this_cpu_write(var, v);
					...
					local_irq_restore(flags)

... which can unexpectedly race as:

	// Thread-A			// Thread-B

	< generate CPU X addr >
	< preempted >

					< scheduled on CPU X >
					local_irq_disable(flags);
					v = __this_cpu_read(var);

	< scheduled on CPU Y >
	< add to CPU X's var >
					v = some_function(v);
					__this_cpu_write(var, v);
					local_irq_restore(flags);

Quoted 1 line

... and hence we lose an update to a percpu variable.

... so, removing preempt disable _in general_ is probably not a good idea.

[1] https://lore.kernel.org/all/[email protected]

Quoted 59 lines · Dev Jain

Quoted 38 lines · Shakeel Butt

On Mon, Feb 02, 2026 at 02:23:54PM +0530, Dev Jain wrote:
On 02/02/26 10:24 am, Shakeel Butt wrote:
Hello Shakeel,

We are seeing a regression in micromm/munmap benchmark with this patch, on arm64 - the benchmark mmmaps a lot of memory, memsets it, and measures the time taken to munmap. Please see below if my understanding of this patch is correct.

Thanks for the report. Are you seeing regression in just the benchmark or some real workload as well? Also how much regression are you seeing? I have a kernel rebot regression report [1] for this patch as well which says 2.6% regression and thus it was on the back-burner for now. I will take look at this again soon.

The munmap regression is ~24%. Haven't observed a regression in any other benchmark yet.
Please share the code/benchmark which shows such regression, also if you can share the perf profile, that would be awesome.
https://gitlab.arm.com/tooling/fastpath/-/blob/main/containers/microbench/micromm.c
You can run this with
./micromm 0 munmap 10

Don't have a perf profile, I measured the time taken by above command, with and without the patch.

Hi Dev, can you please try the following patch?

From 40155feca7e7bc846800ab8449735bdb03164d6d Mon Sep 17 00:00:00 2001
From: Shakeel Butt <[email protected]>
Date: Wed, 4 Feb 2026 08:46:08 -0800
Subject: [PATCH] vmstat: use preempt disable instead of try_cmpxchg

Signed-off-by Shakeel Butt <[email protected]>

---

[...snip...]

Quoted 6 lines

Thanks for looking into this.

But this doesn't solve it :( preempt_disable() contains a compiler barrier, probably that's why.

I think the reason why it doesn't solve the regression is because of how arm64 implements this_cpu_add_8() and this_cpu_try_cmpxchg_8().

On arm64, IIUC both this_cpu_try_cmpxchg_8() and this_cpu_add_8() are implemented using LL/SC instructions or LSE atomics (if supported).

See:
- this_cpu_add_8()
-> __percpu_add_case_64
(which is generated from PERCPU_OP)

Quoted 10 lines

- this_cpu_try_cmpxchg_8()
-> __cpu_fallback_try_cmpxchg(..., this_cpu_cmpxchg_8) -> this_cpu_cmpxchg_8()
-> cmpxchg_relaxed()
-> raw_cmpxchg_relaxed()
-> arch_cmpxchg_relaxed()
-> __cmpxchg_wrapper()
-> __cmpxchg_case_64()
-> __lse_ll_sc_body(_cmpxchg_case_64, ...)

Oh so it is arm64 specific issue. I tested on x86-64 machine and it solves the little regression it had before. So, on arm64 all this_cpu_ops i.e. without double underscore, uses LL/SC instructions.

Need more thought on this.

The __mod_lruvec_kmem_state is already safe against irqs, so there is no need to have a separate interface (i.e. mod_lruvec_kmem_state) which wraps calls to it with irq disabling and reenabling. Let's rename __mod_lruvec_kmem_state to mod_lruvec_kmem_state.

Signed-off-by Shakeel Butt <[email protected]>

---

The __mod_lruvec_kmem_state is already safe against irqs, so there is no need to have a separate interface (i.e. mod_lruvec_kmem_state) which wraps calls to it with irq disabling and reenabling. Let's rename __mod_lruvec_kmem_state to mod_lruvec_kmem_state.

Signed-off-by Shakeel Butt <[email protected]>

---
  include/linux/memcontrol.h | 28 +++++-----------------------
  mm/memcontrol.c            |  2 +-
  mm/workingset.c            |  2 +-
  3 files changed, 7 insertions(+), 25 deletions(-)

include/linux/memcontrol.h +1 -19

index f82fac2fd988..1384a9d305e1 100644
@@ -957,17 +957,7 @@ unsigned long lruvec_page_state_local(struct lruvec *lruvec,
  void mem_cgroup_flush_stats(struct mem_cgroup *memcg);
  void mem_cgroup_flush_stats_ratelimited(struct mem_cgroup *memcg);
 
-void __mod_lruvec_kmem_state(void *p, enum node_stat_item idx, int val);
-
-static inline void mod_lruvec_kmem_state(void *p, enum node_stat_item idx,
-					 int val)
-{
-	unsigned long flags;
-
-	local_irq_save(flags);
-	__mod_lruvec_kmem_state(p, idx, val);
-	local_irq_restore(flags);
-}
+void mod_lruvec_kmem_state(void *p, enum node_stat_item idx, int val);
 
  void count_memcg_events(struct mem_cgroup *memcg, enum vm_event_item idx,
  			unsigned long count);

@@ -1403,14 +1393,6 @@ static inline void mem_cgroup_flush_stats_ratelimited(struct mem_cgroup *memcg)
  {
  }
 
-static inline void __mod_lruvec_kmem_state(void *p, enum node_stat_item idx,
-					   int val)
-{
-	struct page *page = virt_to_head_page(p);
-
-	mod_node_page_state(page_pgdat(page), idx, val);
-}
-
  static inline void mod_lruvec_kmem_state(void *p, enum node_stat_item idx,
  					 int val)
  {

include/linux/memcontrol.h +0 -0

  static inline struct lruvec *parent_lruvec(struct lruvec *lruvec)

mm/memcontrol.c +1 -1

index f4b8a6414ed3..3a59d3ee92a7 100644
@@ -799,7 +799,7 @@ void __lruvec_stat_mod_folio(struct folio *folio, enum node_stat_item idx,
  }
  EXPORT_SYMBOL(__lruvec_stat_mod_folio);
 
-void __mod_lruvec_kmem_state(void *p, enum node_stat_item idx, int val)
+void mod_lruvec_kmem_state(void *p, enum node_stat_item idx, int val)
  {
  	pg_data_t *pgdat = page_pgdat(virt_to_page(p));
  	struct mem_cgroup *memcg;

mm/workingset.c +1 -1

index d32dc2e02a61..892f6fe94ea9 100644
@@ -749,7 +749,7 @@ static enum lru_status shadow_lru_isolate(struct list_head *item,
  	if (WARN_ON_ONCE(node->count != node->nr_values))
  		goto out_invalid;
  	xa_delete_node(node, workingset_update_node);
-	__inc_lruvec_kmem_state(node, WORKINGSET_NODERECLAIM);
+	inc_lruvec_kmem_state(node, WORKINGSET_NODERECLAIM);
 
  out_invalid:
  	xa_unlock_irq(&mapping->i_pages);

The __mod_lruvec_kmem_state is already safe against irqs, so there is no need to have a separate interface (i.e. mod_lruvec_kmem_state) which wraps calls to it with irq disabling and reenabling. Let's rename __mod_lruvec_kmem_state to mod_lruvec_kmem_state.

Signed-off-by Shakeel Butt <[email protected]>

The __mod_lruvec_state is already safe against irqs, so there is no need to have a separate interface (i.e. mod_lruvec_state) which wraps calls to it with irq disabling and reenabling. Let's rename __mod_lruvec_state to mod_lruvec_state.

Signed-off-by Shakeel Butt <[email protected]>

---

The __mod_lruvec_state is already safe against irqs, so there is no need to have a separate interface (i.e. mod_lruvec_state) which wraps calls to it with irq disabling and reenabling. Let's rename __mod_lruvec_state to mod_lruvec_state.

Signed-off-by Shakeel Butt <[email protected]>

The __lruvec_stat_mod_folio is already safe against irqs, so there is no need to have a separate interface (i.e. lruvec_stat_mod_folio) which wraps calls to it with irq disabling and reenabling. Let's rename __lruvec_stat_mod_folio to lruvec_stat_mod_folio.

Signed-off-by Shakeel Butt <[email protected]>

---

The __lruvec_stat_mod_folio is already safe against irqs, so there is no need to have a separate interface (i.e. lruvec_stat_mod_folio) which wraps calls to it with irq disabling and reenabling. Let's rename __lruvec_stat_mod_folio to lruvec_stat_mod_folio.

Signed-off-by Shakeel Butt <[email protected]>

The memcg stats are safe against irq (and nmi) context and thus does not require disabling irqs. However for some stats which are also maintained at node level, it is using irq unsafe interface and thus requiring the users to still disables irqs or use interfaces which explicitly disables irqs. Let's move memcg code to use irq safe node level stats function which is already optimized for architectures with HAVE_CMPXCHG_LOCAL (all major ones), so there will not be any performance penalty for its usage.

Shakeel Butt (4):
memcg: use mod_node_page_state to update stats
memcg: remove __mod_lruvec_kmem_state
memcg: remove __mod_lruvec_state
memcg: remove __lruvec_stat_mod_folio

 include/linux/memcontrol.h | 28 ++++------------------
 include/linux/mm_inline.h  |  2 +-
 include/linux/vmstat.h     | 48 ++------------------------------------
 mm/filemap.c               | 20 ++++++++--------
 mm/huge_memory.c           |  4 ++--
 mm/khugepaged.c            |  8 +++----
 mm/memcontrol.c            | 20 ++++++++--------
 mm/migrate.c               | 20 ++++++++--------
 mm/page-writeback.c        |  2 +-
 mm/rmap.c                  |  4 ++--
 mm/shmem.c                 |  6 ++---
 mm/vmscan.c                |  4 ++--
 mm/workingset.c            |  2 +-
 13 files changed, 53 insertions(+), 115 deletions(-)

Signature · 1 line

2.47.3

Quoted 8 lines · Shakeel Butt

The memcg stats are safe against irq (and nmi) context and thus does not require disabling irqs. However for some stats which are also maintained at node level, it is using irq unsafe interface and thus requiring the users to still disables irqs or use interfaces which explicitly disables irqs. Let's move memcg code to use irq safe node level stats function which is already optimized for architectures with HAVE_CMPXCHG_LOCAL (all major ones), so there will not be any performance penalty for its usage.

Are you or Qi planning a follow-up that converts spin_lock_irq() to spin_lock() in places where they disabled IRQs was just to update vmstat?

Qi's zombie memcg series will depends on that work I guess..

Hi,

Quoted 3 lines

Are you or Qi planning a follow-up that converts spin_lock_irq() to spin_lock() in places where they disabled IRQs was just to update vmstat?

Perhaps this change could be implemented together in [PATCH 1/4]?

Of course, it's also reasonable to make it a separate patch. If we choose this method, I’m fine with either me or Shakeel doing it.

Quoted 2 lines

Qi's zombie memcg series will depends on that work I guess..

Yes, and there are other places that also need to be converted, such as __folio_migrate_mapping().

Quoted 2 lines · Qi Zheng

Hi,

[...]

Quoted 4 lines

Quoted 3 lines

Are you or Qi planning a follow-up that converts spin_lock_irq() to spin_lock() in places where they disabled IRQs was just to update vmstat?

Quoted 5 lines

Perhaps this change could be implemented together in [PATCH 1/4]?

Of course, it's also reasonable to make it a separate patch. If we choose this method, I’m fine with either me or Shakeel doing it.

Let's do it separately as I wanted to keep the memcg related changes self-contained.

Qi, can you please take a stab at that?

Quoted 6 lines

Quoted 3 lines

Qi's zombie memcg series will depends on that work I guess..

Yes, and there are other places that also need to be converted, such as __folio_migrate_mapping().

I see __mod_zone_page_state() usage in __folio_migrate_mapping() and using the same reasoning we can convert it to use mod_zone_page_state(). Where else do you need to do these conversions (other than __folio_migrate_mapping)?

Hi Shakeel,

Quoted 8 lines · Shakeel Butt

Quoted 2 lines · Qi Zheng

Hi,

[...]

Quoted 4 lines

Quoted 3 lines

Are you or Qi planning a follow-up that converts spin_lock_irq() to spin_lock() in places where they disabled IRQs was just to update vmstat?

Quoted 8 lines

Quoted 5 lines

Perhaps this change could be implemented together in [PATCH 1/4]?

Of course, it's also reasonable to make it a separate patch. If we choose this method, I’m fine with either me or Shakeel doing it.

Let's do it separately as I wanted to keep the memcg related changes self-contained.

OK.

Quoted 2 lines

Qi, can you please take a stab at that?

Sure, I will do it.

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

Qi's zombie memcg series will depends on that work I guess..

Yes, and there are other places that also need to be converted, such as __folio_migrate_mapping().

Quoted 4 lines

I see __mod_zone_page_state() usage in __folio_migrate_mapping() and using the same reasoning we can convert it to use mod_zone_page_state(). Where else do you need to do these conversions (other than __folio_migrate_mapping)?

I mean converting these places to use spin_lock() instead of spin_lock_irq().

Quoted 39 lines · Qi Zheng

Hi Shakeel,

Quoted 8 lines · Shakeel Butt

Quoted 2 lines · Qi Zheng

Hi,

[...]

Quoted 4 lines

Quoted 3 lines

Are you or Qi planning a follow-up that converts spin_lock_irq() to spin_lock() in places where they disabled IRQs was just to update vmstat?

Quoted 8 lines

Quoted 5 lines

Perhaps this change could be implemented together in [PATCH 1/4]?

Of course, it's also reasonable to make it a separate patch. If we choose this method, I’m fine with either me or Shakeel doing it.

Let's do it separately as I wanted to keep the memcg related changes self-contained.

OK.

Quoted 2 lines

Qi, can you please take a stab at that?

Sure, I will do it.

Quoted 11 lines

Quoted 6 lines

Quoted 3 lines

Qi's zombie memcg series will depends on that work I guess..

Yes, and there are other places that also need to be converted, such as __folio_migrate_mapping().

I see __mod_zone_page_state() usage in __folio_migrate_mapping() and using the same reasoning we can convert it to use mod_zone_page_state(). Where else do you need to do these conversions (other than __folio_migrate_mapping)?

Quoted 2 lines

I mean converting these places to use spin_lock() instead of spin_lock_irq().

For only stats, right?

Quoted 40 lines · Shakeel Butt

Quoted 39 lines · Qi Zheng

Hi Shakeel,

Quoted 8 lines · Shakeel Butt

Quoted 2 lines · Qi Zheng

Hi,

[...]

Quoted 4 lines

Quoted 3 lines

Are you or Qi planning a follow-up that converts spin_lock_irq() to spin_lock() in places where they disabled IRQs was just to update vmstat?

Quoted 8 lines

Quoted 5 lines

Perhaps this change could be implemented together in [PATCH 1/4]?

Of course, it's also reasonable to make it a separate patch. If we choose this method, I’m fine with either me or Shakeel doing it.

Let's do it separately as I wanted to keep the memcg related changes self-contained.

OK.

Quoted 2 lines

Qi, can you please take a stab at that?

Sure, I will do it.

Quoted 11 lines

Quoted 6 lines

Quoted 3 lines

Qi's zombie memcg series will depends on that work I guess..

Yes, and there are other places that also need to be converted, such as __folio_migrate_mapping().

I see __mod_zone_page_state() usage in __folio_migrate_mapping() and using the same reasoning we can convert it to use mod_zone_page_state(). Where else do you need to do these conversions (other than __folio_migrate_mapping)?

Quoted 4 lines

Quoted 2 lines

I mean converting these places to use spin_lock() instead of spin_lock_irq().

For only stats, right?

Right, for thoes places where they disabled IRQs was just to update vmstat.

Quoted 46 lines · Qi Zheng

Quoted 43 lines · Shakeel Butt

Quoted 41 lines · Qi Zheng

Hi Shakeel,

Quoted 8 lines · Shakeel Butt

Quoted 2 lines · Qi Zheng

Hi,

[...]

Quoted 4 lines

Quoted 3 lines

Are you or Qi planning a follow-up that converts spin_lock_irq() to spin_lock() in places where they disabled IRQs was just to update vmstat?

Quoted 8 lines

Quoted 5 lines

Perhaps this change could be implemented together in [PATCH 1/4]?

Of course, it's also reasonable to make it a separate patch. If we choose this method, I’m fine with either me or Shakeel doing it.

Let's do it separately as I wanted to keep the memcg related changes self-contained.

OK.

Quoted 2 lines

Qi, can you please take a stab at that?

Sure, I will do it.

Quoted 11 lines

Quoted 6 lines

Quoted 3 lines

Qi's zombie memcg series will depends on that work I guess..

Yes, and there are other places that also need to be converted, such as __folio_migrate_mapping().

I see __mod_zone_page_state() usage in __folio_migrate_mapping() and using the same reasoning we can convert it to use mod_zone_page_state(). Where else do you need to do these conversions (other than __folio_migrate_mapping)?

I mean converting these places to use spin_lock() instead of spin_lock_irq().

Quoted 4 lines

Quoted 1 line

For only stats, right?

Right, for thoes places where they disabled IRQs was just to update vmstat.

...Or if they disabled IRQs for other reasons as well, we can still move vmstat update code outside the IRQ disabled region.

Hi Shakeel,

Quoted 10 lines · Shakeel Butt

Quoted 2 lines · Qi Zheng

Hi,

[...]

Quoted 6 lines

Quoted 3 lines

Are you or Qi planning a follow-up that converts spin_lock_irq() to spin_lock() in places where they disabled IRQs was just to update vmstat?

Perhaps this change could be implemented together in [PATCH 1/4]?

Quoted 6 lines

Quoted 3 lines

Of course, it's also reasonable to make it a separate patch. If we choose this method, I’m fine with either me or Shakeel doing it.

Let's do it separately as I wanted to keep the memcg related changes self-contained.

OK.

Quoted 1 line

Qi, can you please take a stab at that?

Sure, I will do it.

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

Qi's zombie memcg series will depends on that work I guess..

Yes, and there are other places that also need to be converted, such as __folio_migrate_mapping().

Quoted 4 lines

I see __mod_zone_page_state() usage in __folio_migrate_mapping() and using the same reasoning we can convert it to use mod_zone_page_state(). Where else do you need to do these conversions (other than __folio_migrate_mapping)?

I mean converting these places to use spin_lock() instead of spin_lock_irq().

Quoted 2 lines

I mean converting these places to use spin_lock() instead of spin_lock_irq().

Just one thing I noticed while looking at __folio_migrate_mapping()...

- xas_lock_irq() -> xas_unlock() -> local_irq_enable()
- swap_cluster_get_and_lock_irq() -> swap_cluster_unlock() -> local_irq_enable()

is wrong because spin_lock_irq() doesn't disable IRQ on PREEMPT_RT.

Not 100% sure if it would be benign or lead to actual bugs that need to be fixed in -stable kernels.

Cc'ing RT folks again :)

The memcg stats are safe against irq (and nmi) context and thus does not require disabling irqs. However for some stats which are also maintained at node level, it is using irq unsafe interface and thus requiring the users to still disables irqs or use interfaces which explicitly disables irqs. Let's move memcg code to use irq safe node level stats function which is already optimized for architectures with HAVE_CMPXCHG_LOCAL (all major ones), so there will not be any performance penalty for its usage.

Shakeel Butt (4):
memcg: use mod_node_page_state to update stats
memcg: remove __mod_lruvec_kmem_state
memcg: remove __mod_lruvec_state
memcg: remove __lruvec_stat_mod_folio

  include/linux/memcontrol.h | 28 ++++------------------
  include/linux/mm_inline.h  |  2 +-
  include/linux/vmstat.h     | 48 ++------------------------------------
  mm/filemap.c               | 20 ++++++++--------
  mm/huge_memory.c           |  4 ++--
  mm/khugepaged.c            |  8 +++----
  mm/memcontrol.c            | 20 ++++++++--------
  mm/migrate.c               | 20 ++++++++--------
  mm/page-writeback.c        |  2 +-
  mm/rmap.c                  |  4 ++--
  mm/shmem.c                 |  6 ++---
  mm/vmscan.c                |  4 ++--
  mm/workingset.c            |  2 +-
  13 files changed, 53 insertions(+), 115 deletions(-)

Hi Shakeel,

Quoted 8 lines · Shakeel Butt

The memcg stats are safe against irq (and nmi) context and thus does not require disabling irqs. However for some stats which are also maintained at node level, it is using irq unsafe interface and thus requiring the users to still disables irqs or use interfaces which explicitly disables irqs. Let's move memcg code to use irq safe node level stats function which is already optimized for architectures with HAVE_CMPXCHG_LOCAL (all major ones), so there will not be any performance penalty for its usage.

Generally, places that call __mod_lruvec_state() also call __mod_zone_page_state(), and it also has the corresponding optimized version (mod_zone_page_state()). It seems necessary to clean that up as well, so that those disabling-IRQs that are only used for updating vmstat can be removed.

Quoted 12 lines · Qi Zheng

Hi Shakeel,

Quoted 8 lines · Shakeel Butt

The memcg stats are safe against irq (and nmi) context and thus does not require disabling irqs. However for some stats which are also maintained at node level, it is using irq unsafe interface and thus requiring the users to still disables irqs or use interfaces which explicitly disables irqs. Let's move memcg code to use irq safe node level stats function which is already optimized for architectures with HAVE_CMPXCHG_LOCAL (all major ones), so there will not be any performance penalty for its usage.

Quoted 5 lines

Generally, places that call __mod_lruvec_state() also call __mod_zone_page_state(), and it also has the corresponding optimized version (mod_zone_page_state()). It seems necessary to clean that up as well, so that those disabling-IRQs that are only used for updating vmstat can be removed.

I agree, please take a stab at that.

The memcg stats are safe against irq (and nmi) context and thus does not require disabling irqs. However for some stats which are also maintained at node level, it is using irq unsafe interface and thus requiring the users to still disables irqs or use interfaces which explicitly disables irqs. Let's move memcg code to use irq safe node level stats function which is already optimized for architectures with HAVE_CMPXCHG_LOCAL (all major ones), so there will not be any performance penalty for its usage.

Shakeel Butt (4):
memcg: use mod_node_page_state to update stats
memcg: remove __mod_lruvec_kmem_state
memcg: remove __mod_lruvec_state
memcg: remove __lruvec_stat_mod_folio

 include/linux/memcontrol.h | 28 ++++------------------
 include/linux/mm_inline.h  |  2 +-
 include/linux/vmstat.h     | 48 ++------------------------------------
 mm/filemap.c               | 20 ++++++++--------
 mm/huge_memory.c           |  4 ++--
 mm/khugepaged.c            |  8 +++----
 mm/memcontrol.c            | 20 ++++++++--------
 mm/migrate.c               | 20 ++++++++--------
 mm/page-writeback.c        |  2 +-
 mm/rmap.c                  |  4 ++--
 mm/shmem.c                 |  6 ++---
 mm/vmscan.c                |  4 ++--
 mm/workingset.c            |  2 +-
 13 files changed, 53 insertions(+), 115 deletions(-)

The memcg stats are safe against irq (and nmi) context and thus does not require disabling irqs. However for some stats which are also maintained at node level, it is using irq unsafe interface and thus requiring the users to still disables irqs or use interfaces which explicitly disables irqs. Let's move memcg code to use irq safe node level stats function which is already optimized for architectures with HAVE_CMPXCHG_LOCAL (all major ones), so there will not be any performance penalty for its usage.

Quoted 8 lines · Shakeel Butt

The memcg stats are safe against irq (and nmi) context and thus does not require disabling irqs. However for some stats which are also maintained at node level, it is using irq unsafe interface and thus requiring the users to still disables irqs or use interfaces which explicitly disables irqs. Let's move memcg code to use irq safe node level stats function which is already optimized for architectures with HAVE_CMPXCHG_LOCAL (all major ones), so there will not be any performance penalty for its usage.

Do you have any production data for this or it's theory-based?

In general I feel we need a benchmark focused on memcg stats:
there was a number of performance improvements and regressions in this code over last years, so a dedicated benchmark can help with measuring them.

Nice cleanup btw, thanks!

The memcg stats are safe against irq (and nmi) context and thus does not require disabling irqs. However for some stats which are also maintained at node level, it is using irq unsafe interface and thus requiring the users to still disables irqs or use interfaces which explicitly disables irqs. Let's move memcg code to use irq safe node level stats function which is already optimized for architectures with HAVE_CMPXCHG_LOCAL (all major ones), so there will not be any performance penalty for its usage.

Shakeel Butt (4):
memcg: use mod_node_page_state to update stats
memcg: remove __mod_lruvec_kmem_state
memcg: remove __mod_lruvec_state
memcg: remove __lruvec_stat_mod_folio

 include/linux/memcontrol.h | 28 ++++------------------
 include/linux/mm_inline.h  |  2 +-
 include/linux/vmstat.h     | 48 ++------------------------------------
 mm/filemap.c               | 20 ++++++++--------
 mm/huge_memory.c           |  4 ++--
 mm/khugepaged.c            |  8 +++----
 mm/memcontrol.c            | 20 ++++++++--------
 mm/migrate.c               | 20 ++++++++--------
 mm/page-writeback.c        |  2 +-
 mm/rmap.c                  |  4 ++--
 mm/shmem.c                 |  6 ++---
 mm/vmscan.c                |  4 ++--
 mm/workingset.c            |  2 +-
 13 files changed, 53 insertions(+), 115 deletions(-)

Signature · 1 line

2.47.3