Skip to contentSkip to conversation

arm64: remove HAVE_CMPXCHG_LOCAL

PATCH In review Jisheng Zhang 8 participants 15 messages Feb 15 – Feb 23

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 arm64: remove HAVE_CMPXCHG_LOCAL

It turns out the generic disable/enable irq this_cpu_cmpxchg implementation is faster than LL/SC or lse implementation. Remove HAVE_CMPXCHG_LOCAL for better performance on arm64.

Tested on Quad 1.9GHZ CA55 platform:
average mod_node_page_state() cost decreases from 167ns to 103ns the spawn (30 duration) benchmark in unixbench is improved from 147494 lps to 150561 lps, improved by 2.1%

Tested on Quad 2.1GHZ CA73 platform:
average mod_node_page_state() cost decreases from 113ns to 85ns the spawn (30 duration) benchmark in unixbench is improved from 209844 lps to 212581 lps, improved by 1.3%

---
 arch/arm64/Kconfig              |  1 -
 arch/arm64/include/asm/percpu.h | 24 ------------------------
 2 files changed, 25 deletions(-)
arch/arm64/Kconfig +0 -1
index 38dba5f7e4d2..5e7e2e65d5a5 100644
@@ -205,7 +205,6 @@ config ARM64
 	select HAVE_EBPF_JIT
 	select HAVE_C_RECORDMCOUNT
 	select HAVE_CMPXCHG_DOUBLE
-	select HAVE_CMPXCHG_LOCAL
 	select HAVE_CONTEXT_TRACKING_USER
 	select HAVE_DEBUG_KMEMLEAK
 	select HAVE_DMA_CONTIGUOUS
arch/arm64/include/asm/percpu.h +0 -24
index b57b2bb00967..70ffe566cb4b 100644
@@ -232,30 +232,6 @@ PERCPU_RET_OP(add, add, ldadd)
 #define this_cpu_xchg_8(pcp, val)	\
 	_pcp_protect_return(xchg_relaxed, pcp, val)
 
-#define this_cpu_cmpxchg_1(pcp, o, n)	\
-	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
-#define this_cpu_cmpxchg_2(pcp, o, n)	\
-	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
-#define this_cpu_cmpxchg_4(pcp, o, n)	\
-	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
-#define this_cpu_cmpxchg_8(pcp, o, n)	\
-	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
-
-#define this_cpu_cmpxchg64(pcp, o, n)	this_cpu_cmpxchg_8(pcp, o, n)
-
-#define this_cpu_cmpxchg128(pcp, o, n)					\
-({									\
-	typedef typeof(pcp) pcp_op_T__;					\
-	u128 old__, new__, ret__;					\
-	pcp_op_T__ *ptr__;						\
-	old__ = o;							\
-	new__ = n;							\
-	preempt_disable_notrace();					\
-	ptr__ = raw_cpu_ptr(&(pcp));					\
-	ret__ = cmpxchg128_local((void *)ptr__, old__, new__);		\
-	preempt_enable_notrace();					\
-	ret__;								\
-})
 
 #ifdef __KVM_NVHE_HYPERVISOR__
 extern unsigned long __hyp_per_cpu_offset(unsigned int cpu);
Signature · 1 line
2.51.0
#1 lore raw
Quoted 9 lines · Jisheng Zhang

It turns out the generic disable/enable irq this_cpu_cmpxchg implementation is faster than LL/SC or lse implementation. Remove HAVE_CMPXCHG_LOCAL for better performance on arm64.

Tested on Quad 1.9GHZ CA55 platform:
average mod_node_page_state() cost decreases from 167ns to 103ns the spawn (30 duration) benchmark in unixbench is improved from 147494 lps to 150561 lps, improved by 2.1%

Quoted 7 lines

Tested on Quad 2.1GHZ CA73 platform:
average mod_node_page_state() cost decreases from 113ns to 85ns the spawn (30 duration) benchmark in unixbench is improved from 209844 lps to 212581 lps, improved by 1.3%

---

Thanks. This concurs with my investigation on [1]. The problem isn't really LL/SC/LSE but preempt_disable()/enable() in this_cpu_* [1, 2].

I think you should only remove the selection of the config, but keep the code? We may want to switch this on again if the real issue gets solved.

[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/CAHbLzkpcN-T8MH6=W3jCxcFj1gVZp8fRqe231yzZT-rV_E_org@mail.gmail.com/

Quoted 51 lines
 arch/arm64/Kconfig              |  1 -
 arch/arm64/include/asm/percpu.h | 24 ------------------------
 2 files changed, 25 deletions(-)
arch/arm64/Kconfig +0 -1
index 38dba5f7e4d2..5e7e2e65d5a5 100644
@@ -205,7 +205,6 @@ config ARM64
 	select HAVE_EBPF_JIT
 	select HAVE_C_RECORDMCOUNT
 	select HAVE_CMPXCHG_DOUBLE
-	select HAVE_CMPXCHG_LOCAL
 	select HAVE_CONTEXT_TRACKING_USER
 	select HAVE_DEBUG_KMEMLEAK
 	select HAVE_DMA_CONTIGUOUS
arch/arm64/include/asm/percpu.h +0 -24
index b57b2bb00967..70ffe566cb4b 100644
@@ -232,30 +232,6 @@ PERCPU_RET_OP(add, add, ldadd)
 #define this_cpu_xchg_8(pcp, val)	\
 	_pcp_protect_return(xchg_relaxed, pcp, val)
 
-#define this_cpu_cmpxchg_1(pcp, o, n)	\
-	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
-#define this_cpu_cmpxchg_2(pcp, o, n)	\
-	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
-#define this_cpu_cmpxchg_4(pcp, o, n)	\
-	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
-#define this_cpu_cmpxchg_8(pcp, o, n)	\
-	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
-
-#define this_cpu_cmpxchg64(pcp, o, n)	this_cpu_cmpxchg_8(pcp, o, n)
-
-#define this_cpu_cmpxchg128(pcp, o, n)					\
-({									\
-	typedef typeof(pcp) pcp_op_T__;					\
-	u128 old__, new__, ret__;					\
-	pcp_op_T__ *ptr__;						\
-	old__ = o;							\
-	new__ = n;							\
-	preempt_disable_notrace();					\
-	ptr__ = raw_cpu_ptr(&(pcp));					\
-	ret__ = cmpxchg128_local((void *)ptr__, old__, new__);		\
-	preempt_enable_notrace();					\
-	ret__;								\
-})
 
 #ifdef __KVM_NVHE_HYPERVISOR__
 extern unsigned long __hyp_per_cpu_offset(unsigned int cpu);
maintainer
#2 lore raw
In reply to Jisheng Zhang
Quoted 14 lines · Jisheng Zhang

It turns out the generic disable/enable irq this_cpu_cmpxchg implementation is faster than LL/SC or lse implementation. Remove HAVE_CMPXCHG_LOCAL for better performance on arm64.

Tested on Quad 1.9GHZ CA55 platform:
average mod_node_page_state() cost decreases from 167ns to 103ns the spawn (30 duration) benchmark in unixbench is improved from 147494 lps to 150561 lps, improved by 2.1%

Tested on Quad 2.1GHZ CA73 platform:
average mod_node_page_state() cost decreases from 113ns to 85ns the spawn (30 duration) benchmark in unixbench is improved from 209844 lps to 212581 lps, improved by 1.3%

Quoted 5 lines
---
 arch/arm64/Kconfig              |  1 -
 arch/arm64/include/asm/percpu.h | 24 ------------------------
 2 files changed, 25 deletions(-)

That is _entirely_ dependent on the system, so this isn't the right approach. I also don't think it's something we particularly want to micro-optimise to accomodate systems that suck at atomics.

Will

Quoted 50 lines
arch/arm64/Kconfig +0 -1
index 38dba5f7e4d2..5e7e2e65d5a5 100644
@@ -205,7 +205,6 @@ config ARM64
 	select HAVE_EBPF_JIT
 	select HAVE_C_RECORDMCOUNT
 	select HAVE_CMPXCHG_DOUBLE
-	select HAVE_CMPXCHG_LOCAL
 	select HAVE_CONTEXT_TRACKING_USER
 	select HAVE_DEBUG_KMEMLEAK
 	select HAVE_DMA_CONTIGUOUS
arch/arm64/include/asm/percpu.h +0 -24
index b57b2bb00967..70ffe566cb4b 100644
@@ -232,30 +232,6 @@ PERCPU_RET_OP(add, add, ldadd)
 #define this_cpu_xchg_8(pcp, val)	\
 	_pcp_protect_return(xchg_relaxed, pcp, val)
 
-#define this_cpu_cmpxchg_1(pcp, o, n)	\
-	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
-#define this_cpu_cmpxchg_2(pcp, o, n)	\
-	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
-#define this_cpu_cmpxchg_4(pcp, o, n)	\
-	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
-#define this_cpu_cmpxchg_8(pcp, o, n)	\
-	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
-
-#define this_cpu_cmpxchg64(pcp, o, n)	this_cpu_cmpxchg_8(pcp, o, n)
-
-#define this_cpu_cmpxchg128(pcp, o, n)					\
-({									\
-	typedef typeof(pcp) pcp_op_T__;					\
-	u128 old__, new__, ret__;					\
-	pcp_op_T__ *ptr__;						\
-	old__ = o;							\
-	new__ = n;							\
-	preempt_disable_notrace();					\
-	ptr__ = raw_cpu_ptr(&(pcp));					\
-	ret__ = cmpxchg128_local((void *)ptr__, old__, new__);		\
-	preempt_enable_notrace();					\
-	ret__;								\
-})
 
 #ifdef __KVM_NVHE_HYPERVISOR__
 extern unsigned long __hyp_per_cpu_offset(unsigned int cpu);
Signature · 1 line
2.51.0
#3 lore raw
Quoted 15 lines · Will Deacon
Quoted 14 lines · Jisheng Zhang

It turns out the generic disable/enable irq this_cpu_cmpxchg implementation is faster than LL/SC or lse implementation. Remove HAVE_CMPXCHG_LOCAL for better performance on arm64.

Tested on Quad 1.9GHZ CA55 platform:
average mod_node_page_state() cost decreases from 167ns to 103ns the spawn (30 duration) benchmark in unixbench is improved from 147494 lps to 150561 lps, improved by 2.1%

Tested on Quad 2.1GHZ CA73 platform:
average mod_node_page_state() cost decreases from 113ns to 85ns the spawn (30 duration) benchmark in unixbench is improved from 209844 lps to 212581 lps, improved by 1.3%

Quoted 8 lines
Quoted 5 lines
---
 arch/arm64/Kconfig              |  1 -
 arch/arm64/include/asm/percpu.h | 24 ------------------------
 2 files changed, 25 deletions(-)

That is _entirely_ dependent on the system, so this isn't the right approach. I also don't think it's something we particularly want to micro-optimise to accomodate systems that suck at atomics.

Hi Will,

As I mention in the other email, the suspect is not the atomics, but preempt_disable(). On Apple M3, the regression reported in [1] resolves by removing preempt_disable/enable in _pcp_protect_return. To prove this another way, I disabled CONFIG_ARM64_HAS_LSE_ATOMICS and the regression worsened, indicating that at least on Apple M3 the atomics are faster.

It may help to confirm this hypothesis on other hardware - perhaps Jisheng can test with this change on his hardware and confirm whether he gets the same performance improvement.

By coincidence, Yang Shi has been discussing the this_cpu_* overhead at [2].

[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/CAHbLzkpcN-T8MH6=W3jCxcFj1gVZp8fRqe231yzZT-rV_E_org@mail.gmail.com/

Quoted 53 lines

Will

Quoted 50 lines
arch/arm64/Kconfig +0 -1
index 38dba5f7e4d2..5e7e2e65d5a5 100644
@@ -205,7 +205,6 @@ config ARM64
 	select HAVE_EBPF_JIT
 	select HAVE_C_RECORDMCOUNT
 	select HAVE_CMPXCHG_DOUBLE
-	select HAVE_CMPXCHG_LOCAL
 	select HAVE_CONTEXT_TRACKING_USER
 	select HAVE_DEBUG_KMEMLEAK
 	select HAVE_DMA_CONTIGUOUS
arch/arm64/include/asm/percpu.h +0 -24
index b57b2bb00967..70ffe566cb4b 100644
@@ -232,30 +232,6 @@ PERCPU_RET_OP(add, add, ldadd)
 #define this_cpu_xchg_8(pcp, val)	\
 	_pcp_protect_return(xchg_relaxed, pcp, val)
 
-#define this_cpu_cmpxchg_1(pcp, o, n)	\
-	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
-#define this_cpu_cmpxchg_2(pcp, o, n)	\
-	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
-#define this_cpu_cmpxchg_4(pcp, o, n)	\
-	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
-#define this_cpu_cmpxchg_8(pcp, o, n)	\
-	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
-
-#define this_cpu_cmpxchg64(pcp, o, n)	this_cpu_cmpxchg_8(pcp, o, n)
-
-#define this_cpu_cmpxchg128(pcp, o, n)					\
-({									\
-	typedef typeof(pcp) pcp_op_T__;					\
-	u128 old__, new__, ret__;					\
-	pcp_op_T__ *ptr__;						\
-	old__ = o;							\
-	new__ = n;							\
-	preempt_disable_notrace();					\
-	ptr__ = raw_cpu_ptr(&(pcp));					\
-	ret__ = cmpxchg128_local((void *)ptr__, old__, new__);		\
-	preempt_enable_notrace();					\
-	ret__;								\
-})
 
 #ifdef __KVM_NVHE_HYPERVISOR__
 extern unsigned long __hyp_per_cpu_offset(unsigned int cpu);
Signature · 1 line
2.51.0
maintainer
#4 lore raw
Quoted 25 lines · Dev Jain
Quoted 15 lines · Will Deacon
Quoted 14 lines · Jisheng Zhang

It turns out the generic disable/enable irq this_cpu_cmpxchg implementation is faster than LL/SC or lse implementation. Remove HAVE_CMPXCHG_LOCAL for better performance on arm64.

Tested on Quad 1.9GHZ CA55 platform:
average mod_node_page_state() cost decreases from 167ns to 103ns the spawn (30 duration) benchmark in unixbench is improved from 147494 lps to 150561 lps, improved by 2.1%

Tested on Quad 2.1GHZ CA73 platform:
average mod_node_page_state() cost decreases from 113ns to 85ns the spawn (30 duration) benchmark in unixbench is improved from 209844 lps to 212581 lps, improved by 1.3%

Quoted 8 lines
Quoted 5 lines
---
 arch/arm64/Kconfig              |  1 -
 arch/arm64/include/asm/percpu.h | 24 ------------------------
 2 files changed, 25 deletions(-)

That is _entirely_ dependent on the system, so this isn't the right approach. I also don't think it's something we particularly want to micro-optimise to accomodate systems that suck at atomics.

Quoted 8 lines

Hi Will,

As I mention in the other email, the suspect is not the atomics, but preempt_disable(). On Apple M3, the regression reported in [1] resolves by removing preempt_disable/enable in _pcp_protect_return. To prove this another way, I disabled CONFIG_ARM64_HAS_LSE_ATOMICS and the regression worsened, indicating that at least on Apple M3 the atomics are faster.

Then why don't we replace the preempt disabling with local_irq_save() in the arm64 code and still use the LSE atomics?

IIUC (lots of macro indirection), the generic cmpxchg is not atomic, so another CPU is allowed to mess this up if it accesses current CPU's variable via per_cpu_ptr().

Signature · 1 line
Catalin
maintainer
#5 lore raw
Quoted 35 lines · Catalin Marinas
Quoted 25 lines · Dev Jain
Quoted 15 lines · Will Deacon
Quoted 14 lines · Jisheng Zhang

It turns out the generic disable/enable irq this_cpu_cmpxchg implementation is faster than LL/SC or lse implementation. Remove HAVE_CMPXCHG_LOCAL for better performance on arm64.

Tested on Quad 1.9GHZ CA55 platform:
average mod_node_page_state() cost decreases from 167ns to 103ns the spawn (30 duration) benchmark in unixbench is improved from 147494 lps to 150561 lps, improved by 2.1%

Tested on Quad 2.1GHZ CA73 platform:
average mod_node_page_state() cost decreases from 113ns to 85ns the spawn (30 duration) benchmark in unixbench is improved from 209844 lps to 212581 lps, improved by 1.3%

Quoted 8 lines
Quoted 5 lines
---
 arch/arm64/Kconfig              |  1 -
 arch/arm64/include/asm/percpu.h | 24 ------------------------
 2 files changed, 25 deletions(-)

That is _entirely_ dependent on the system, so this isn't the right approach. I also don't think it's something we particularly want to micro-optimise to accomodate systems that suck at atomics.

Quoted 8 lines

Hi Will,

As I mention in the other email, the suspect is not the atomics, but preempt_disable(). On Apple M3, the regression reported in [1] resolves by removing preempt_disable/enable in _pcp_protect_return. To prove this another way, I disabled CONFIG_ARM64_HAS_LSE_ATOMICS and the regression worsened, indicating that at least on Apple M3 the atomics are faster.

Quoted 2 lines

Then why don't we replace the preempt disabling with local_irq_save() in the arm64 code and still use the LSE atomics?

Even better, work on making preempt_disable() faster as it's used in many other places. Of course, if people want to hack the .config, they could also change the preemption mode...

Will

maintainer
#6 lore raw
Quoted 17 lines · Will Deacon
Quoted 16 lines · Catalin Marinas
Quoted 15 lines · Dev Jain
Quoted 14 lines · Will Deacon
Quoted 13 lines · Jisheng Zhang

It turns out the generic disable/enable irq this_cpu_cmpxchg implementation is faster than LL/SC or lse implementation. Remove HAVE_CMPXCHG_LOCAL for better performance on arm64.

Tested on Quad 1.9GHZ CA55 platform:
average mod_node_page_state() cost decreases from 167ns to 103ns the spawn (30 duration) benchmark in unixbench is improved from 147494 lps to 150561 lps, improved by 2.1%

Tested on Quad 2.1GHZ CA73 platform:
average mod_node_page_state() cost decreases from 113ns to 85ns the spawn (30 duration) benchmark in unixbench is improved from 209844 lps to 212581 lps, improved by 1.3%

[...]

Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
Quoted 3 lines

That is _entirely_ dependent on the system, so this isn't the right approach. I also don't think it's something we particularly want to micro-optimise to accomodate systems that suck at atomics.

As I mention in the other email, the suspect is not the atomics, but preempt_disable(). On Apple M3, the regression reported in [1] resolves by removing preempt_disable/enable in _pcp_protect_return. To prove this another way, I disabled CONFIG_ARM64_HAS_LSE_ATOMICS and the regression worsened, indicating that at least on Apple M3 the atomics are faster.

Quoted 5 lines
Quoted 2 lines

Then why don't we replace the preempt disabling with local_irq_save() in the arm64 code and still use the LSE atomics?

Even better, work on making preempt_disable() faster as it's used in many other places.

Yes, that would be good. It's the preempt_enable_notrace() path that ends up calling preempt_schedule_notrace() -> __schedule() pretty much unconditionally. Not sure what would go wrong but some simple change like this (can be done at a higher in the preempt macros to even avoid getting here):

kernel/sched/core.c +2 -2
index 854984967fe2..d9a5d6438303 100644
@@ -7119,7 +7119,7 @@ asmlinkage __visible void __sched notrace preempt_schedule_notrace(void)
 	if (likely(!preemptible()))
 		return;
 
-	do {
+	while (need_resched()) {
 		/*
 		 * Because the function tracer can trace preempt_count_sub()
 		 * and it also uses preempt_enable/disable_notrace(), if
@@ -7146,7 +7146,7 @@ asmlinkage __visible void __sched notrace preempt_schedule_notrace(void)
 
 		preempt_latency_stop(1);
 		preempt_enable_no_resched_notrace();
-	} while (need_resched());
+	}
 }
 EXPORT_SYMBOL_GPL(preempt_schedule_notrace);
 

Of course, changing the preemption model solves this by making the macros no-ops but I assume people want to keep preemption on.

Signature · 1 line
Catalin
#7 lore raw

Hello Catalin,

Quoted 3 lines · Catalin Marinas

Yes, that would be good. It's the preempt_enable_notrace() path that ends up calling preempt_schedule_notrace() -> __schedule() pretty much unconditionally.

What do you mean by unconditionally? We always check __preempt_count_dec_and_test() before calling into __schedule().

On x86, We use MSB of preempt_count to indicate a resched and set_preempt_need_resched() would just clear this MSB.

If the preempt_count() turns 0, we immediately go into schedule or or the next preempt_enable() -> __preempt_count_dec_and_test() would see the entire preempt_count being clear and will call into schedule.

The arm64 implementation seems to be doing something similar too with a separate "ti->preempt.need_resched" bit which is part of the "ti->preempt_count"'s union so it isn't really unconditional.

Quoted 9 lines

Not sure what would go wrong but some simple change
like this (can be done at a higher in the preempt macros to even avoid getting here):

kernel/sched/core.c +0 -0
index 854984967fe2..d9a5d6438303 100644

kernel/sched/core.c asmlinkage __visible void __sched notrace preempt_schedule_notrace(void) · line 7122

Quoted 5 lines
kernel/sched/core.c +1 -1
@@ -7119,7 +7119,7 @@ asmlinkage __visible void __sched notrace preempt_schedule_notrace(void)
 	if (likely(!preemptible()))
 		return;
 
-	do {
+	while (need_resched()) {

Essentially you are simply checking it twice now on entry since need_resched() state would have already been communicated by __preempt_count_dec_and_test().

Quoted 11 lines
kernel/sched/core.c +1 -1
 		/*
 		 * Because the function tracer can trace preempt_count_sub()
 		 * and it also uses preempt_enable/disable_notrace(), if
@@ -7146,7 +7146,7 @@ asmlinkage __visible void __sched notrace preempt_schedule_notrace(void)
 
 		preempt_latency_stop(1);
 		preempt_enable_no_resched_notrace();
-	} while (need_resched());
+	}
 }
 EXPORT_SYMBOL_GPL(preempt_schedule_notrace);
Signature · 2 lines
Thanks and Regards,
Prateek
maintainer
#8 lore raw

Hi Prateek,

Quoted 11 lines · K Prateek Nayak
Quoted 3 lines · Catalin Marinas

Yes, that would be good. It's the preempt_enable_notrace() path that ends up calling preempt_schedule_notrace() -> __schedule() pretty much unconditionally.

What do you mean by unconditionally? We always check __preempt_count_dec_and_test() before calling into __schedule().

On x86, We use MSB of preempt_count to indicate a resched and set_preempt_need_resched() would just clear this MSB.

Quoted 8 lines

If the preempt_count() turns 0, we immediately go into schedule or or the next preempt_enable() -> __preempt_count_dec_and_test() would see the entire preempt_count being clear and will call into schedule.

The arm64 implementation seems to be doing something similar too with a separate "ti->preempt.need_resched" bit which is part of the "ti->preempt_count"'s union so it isn't really unconditional.

Ah, yes, you are right. I got the polarity of need_resched in thread_info wrong (we should have named it no_need_to_resched).

So in the common case, the overhead is caused by the additional pointer chase and preempt_count update, on top of the cpu offset read. Not sure we can squeeze any more cycles out of these without some large overhaul like:

https://git.kernel.org/mark/c/84ee5f23f93d4a650e828f831da9ed29c54623c5

or Yang's per-CPU page tables. Well, there are more ideas like in-kernel restartable sequences but they move the overhead elsewhere.

Thanks.

Signature · 1 line
Catalin
#9 lore raw
In reply to Dev Jain
Quoted 2 lines · Dev Jain

By coincidence, Yang Shi has been discussing the this_cpu_* overhead at [2].

Yang Shi is on vacation but we have a patchset that removes preempt_enable/disable from this_cpu operations on ARM64.

The performance of cmpxchg varies by platform in use and with the kernel config. The measurements that I did 2 years ago indicated that the cmpxchg use with Ampere processors did not cause a regression.

Note that distro kernels often do not enable PREEMPT_FULL and therefore preempt_disable/enable overhead is not incurred in production systems.

PREEMPT_VOLUNTARY does not use preemption for this_cpu ops.

#10 lore raw
Quoted 9 lines · Christoph Lameter (Ampere)
Quoted 2 lines · Dev Jain

By coincidence, Yang Shi has been discussing the this_cpu_* overhead at [2].

Yang Shi is on vacation but we have a patchset that removes preempt_enable/disable from this_cpu operations on ARM64.

Quoted 8 lines

The performance of cmpxchg varies by platform in use and with the kernel config. The measurements that I did 2 years ago indicated that the cmpxchg use with Ampere processors did not cause a regression.

Note that distro kernels often do not enable PREEMPT_FULL and therefore preempt_disable/enable overhead is not incurred in production systems.

PREEMPT_VOLUNTARY does not use preemption for this_cpu ops.

Note that with commit 7dadeaa6e851 ("sched: Further restrict the preemption modes") at least PREEMPT_LAZY is enforced, which comes together with PREEMPT_BUILD and full preempt_disable()/preempt_enable() overhead for this_cpu_* ops for every "up-to-date" architecture (except x86).

PREEMPT_NONE and PREEMPT_VOLUNTARY are gone for those architectures.

#11 lore raw
In reply to Dev Jain
Quoted 17 lines · Dev Jain
Quoted 15 lines · Will Deacon
Quoted 14 lines · Jisheng Zhang

It turns out the generic disable/enable irq this_cpu_cmpxchg implementation is faster than LL/SC or lse implementation. Remove HAVE_CMPXCHG_LOCAL for better performance on arm64.

Tested on Quad 1.9GHZ CA55 platform:
average mod_node_page_state() cost decreases from 167ns to 103ns the spawn (30 duration) benchmark in unixbench is improved from 147494 lps to 150561 lps, improved by 2.1%

Tested on Quad 2.1GHZ CA73 platform:
average mod_node_page_state() cost decreases from 113ns to 85ns the spawn (30 duration) benchmark in unixbench is improved from 209844 lps to 212581 lps, improved by 1.3%

Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
---
 arch/arm64/Kconfig              |  1 -
 arch/arm64/include/asm/percpu.h | 24 ------------------------
 2 files changed, 25 deletions(-)

That is _entirely_ dependent on the system, so this isn't the right approach. I also don't think it's something we particularly want to micro-optimise to accomodate systems that suck at atomics.

Hi Will,

I read this as an implication that the cmpxchg_local version is better than generic disable/enable irq version on the newer arm64 systems. Is my understanding correct?

Quoted 10 lines

Hi Will,

As I mention in the other email, the suspect is not the atomics, but preempt_disable(). On Apple M3, the regression reported in [1] resolves by removing preempt_disable/enable in _pcp_protect_return. To prove this another way, I disabled CONFIG_ARM64_HAS_LSE_ATOMICS and the regression worsened, indicating that at least on Apple M3 the atomics are faster.

Quoted 3 lines

It may help to confirm this hypothesis on other hardware - perhaps Jisheng can test with this change on his hardware and confirm whether he gets the same performance improvement.

Hi Dev,

Thanks for the hints. I tried to remove the preempt_disable/enable from _pcp_protect_return, it improves, but the HAVE_CMPXCHG_LOCAL version is still worse than generic disable/enable irq version on CA55 and CA73.

Quoted 60 lines

By coincidence, Yang Shi has been discussing the this_cpu_* overhead at [2].

[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/CAHbLzkpcN-T8MH6=W3jCxcFj1gVZp8fRqe231yzZT-rV_E_org@mail.gmail.com/

Quoted 53 lines

Will

Quoted 50 lines
arch/arm64/Kconfig +0 -1
index 38dba5f7e4d2..5e7e2e65d5a5 100644
@@ -205,7 +205,6 @@ config ARM64
 	select HAVE_EBPF_JIT
 	select HAVE_C_RECORDMCOUNT
 	select HAVE_CMPXCHG_DOUBLE
-	select HAVE_CMPXCHG_LOCAL
 	select HAVE_CONTEXT_TRACKING_USER
 	select HAVE_DEBUG_KMEMLEAK
 	select HAVE_DMA_CONTIGUOUS
arch/arm64/include/asm/percpu.h +0 -24
index b57b2bb00967..70ffe566cb4b 100644
@@ -232,30 +232,6 @@ PERCPU_RET_OP(add, add, ldadd)
 #define this_cpu_xchg_8(pcp, val)	\
 	_pcp_protect_return(xchg_relaxed, pcp, val)
 
-#define this_cpu_cmpxchg_1(pcp, o, n)	\
-	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
-#define this_cpu_cmpxchg_2(pcp, o, n)	\
-	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
-#define this_cpu_cmpxchg_4(pcp, o, n)	\
-	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
-#define this_cpu_cmpxchg_8(pcp, o, n)	\
-	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
-
-#define this_cpu_cmpxchg64(pcp, o, n)	this_cpu_cmpxchg_8(pcp, o, n)
-
-#define this_cpu_cmpxchg128(pcp, o, n)					\
-({									\
-	typedef typeof(pcp) pcp_op_T__;					\
-	u128 old__, new__, ret__;					\
-	pcp_op_T__ *ptr__;						\
-	old__ = o;							\
-	new__ = n;							\
-	preempt_disable_notrace();					\
-	ptr__ = raw_cpu_ptr(&(pcp));					\
-	ret__ = cmpxchg128_local((void *)ptr__, old__, new__);		\
-	preempt_enable_notrace();					\
-	ret__;								\
-})
 
 #ifdef __KVM_NVHE_HYPERVISOR__
 extern unsigned long __hyp_per_cpu_offset(unsigned int cpu);
Signature · 1 line
2.51.0
#12 lore raw
In reply to Jisheng Zhang
Quoted 9 lines · Jisheng Zhang

It turns out the generic disable/enable irq this_cpu_cmpxchg implementation is faster than LL/SC or lse implementation. Remove HAVE_CMPXCHG_LOCAL for better performance on arm64.

Tested on Quad 1.9GHZ CA55 platform:
average mod_node_page_state() cost decreases from 167ns to 103ns the spawn (30 duration) benchmark in unixbench is improved from 147494 lps to 150561 lps, improved by 2.1%

Quoted 6 lines

Tested on Quad 2.1GHZ CA73 platform:
average mod_node_page_state() cost decreases from 113ns to 85ns the spawn (30 duration) benchmark in unixbench is improved from 209844 lps to 212581 lps, improved by 1.3%

Please note that mod_node_page_state() can be called in NMI context and generic disable/enable irq are not safe against NMIs (newer arm arch supports NMI).

#13 lore raw
Quoted 15 lines · Shakeel Butt
Quoted 14 lines · Jisheng Zhang

It turns out the generic disable/enable irq this_cpu_cmpxchg implementation is faster than LL/SC or lse implementation. Remove HAVE_CMPXCHG_LOCAL for better performance on arm64.

Tested on Quad 1.9GHZ CA55 platform:
average mod_node_page_state() cost decreases from 167ns to 103ns the spawn (30 duration) benchmark in unixbench is improved from 147494 lps to 150561 lps, improved by 2.1%

Tested on Quad 2.1GHZ CA73 platform:
average mod_node_page_state() cost decreases from 113ns to 85ns the spawn (30 duration) benchmark in unixbench is improved from 209844 lps to 212581 lps, improved by 1.3%

Quoted 5 lines
Quoted 1 line

Please note that mod_node_page_state() can be called in NMI context and generic disable/enable irq are not safe against NMIs (newer arm arch supports NMI).

hmm, interesting...

fgrep HAVE_NMI arch/*/Kconfig
then
fgrep HAVE_CMPXCHG_LOCAL arch/*/Kconfig

shows that only x86, arm64, s390 and loongarch are safe, while arm, powerpc and mips enable HAVE_NMI but missing HAVE_CMPXCHG_LOCAL, so they rely on generic generic disable/enable irq version, so you imply that these three arch are not safe considering mod_node_page_state() in NMI context.

#14 lore raw
Quoted 28 lines · Jisheng Zhang
Quoted 15 lines · Shakeel Butt
Quoted 14 lines · Jisheng Zhang

It turns out the generic disable/enable irq this_cpu_cmpxchg implementation is faster than LL/SC or lse implementation. Remove HAVE_CMPXCHG_LOCAL for better performance on arm64.

Tested on Quad 1.9GHZ CA55 platform:
average mod_node_page_state() cost decreases from 167ns to 103ns the spawn (30 duration) benchmark in unixbench is improved from 147494 lps to 150561 lps, improved by 2.1%

Tested on Quad 2.1GHZ CA73 platform:
average mod_node_page_state() cost decreases from 113ns to 85ns the spawn (30 duration) benchmark in unixbench is improved from 209844 lps to 212581 lps, improved by 1.3%

Quoted 5 lines
Quoted 1 line

Please note that mod_node_page_state() can be called in NMI context and generic disable/enable irq are not safe against NMIs (newer arm arch supports NMI).

hmm, interesting...

fgrep HAVE_NMI arch/*/Kconfig
then
fgrep HAVE_CMPXCHG_LOCAL arch/*/Kconfig

Quoted 5 lines

shows that only x86, arm64, s390 and loongarch are safe, while arm, powerpc and mips enable HAVE_NMI but missing HAVE_CMPXCHG_LOCAL, so they rely on generic generic disable/enable irq version, so you imply that these three arch are not safe considering mod_node_page_state() in NMI context.

Yes it seems like it. For memcg stats, we use ARCH_HAVE_NMI_SAFE_CMPXCHG and ARCH_HAS_NMI_SAFE_THIS_CPU_OPS config options to correctly handle the updates from NMI context. Maybe we need something similar for vmstat as well.

So arm, powerpc and mips does not have ARCH_HAS_NMI_SAFE_THIS_CPU_OPS but powerpc does have ARCH_HAVE_NMI_SAFE_CMPXCHG and arm has
it for CPU_V7, CPU_V7M & CPU_V6K models.

I wonder if we need to add complexity for these archs.