Skip to contentSkip to conversation

arm64: Kconfig: Don't select CPUMASK_OFFSTACK with 64K pages

PATCH In review Breno Leitao 2 participants 6 messages Oct 7 – Oct 30 arm64

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 arm64: Kconfig: Don't select CPUMASK_OFFSTACK with 64K pages

arm64 selects CPUMASK_OFFSTACK when NR_CPUS is above 256, which keeps cpumask_var_t off the kernel stack. With 64K pages the kernel stack is 64K as well, we have enough room to keep cpumask on the stack, avoiding extra alocation and possible performance penalties.

Skip the select when ARM64_64K_PAGES is set. A 64K kernel can then raise NR_CPUS above 256 and keep cpumask_var_t as an inline array. 4K and 16K page kernels are unchanged.

---
Based on Will presentation at LPC, I've decided to have a look at Meta's stack usage, and I found that this might be an small optimization!?

---
 arch/arm64/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/Kconfig +1 -1
index 5f2db2cecf9772..b73371327d2a47 100644
@@ -111,7 +111,7 @@ config ARM64
 	select CLONE_BACKWARDS
 	select COMMON_CLK
 	select CPU_PM if (SUSPEND || CPU_IDLE)
-	select CPUMASK_OFFSTACK if NR_CPUS > 256
+	select CPUMASK_OFFSTACK if NR_CPUS > 256 && !ARM64_64K_PAGES
 	select DCACHE_WORD_ACCESS
 	select HAVE_EXTRA_IPI_TRACEPOINTS
 	select DYNAMIC_FTRACE if FUNCTION_TRACER

---
base-commit: eea3fef32a9cf36abcb5975a5a594e4135a6b026
change-id: 20261007-arm_stack-fa76c2d0d69a

Best regards,

Signature · 1 line
Breno Leitao <[email protected]>
#1 lore raw
Quoted 22 lines · Breno Leitao

arm64 selects CPUMASK_OFFSTACK when NR_CPUS is above 256, which keeps cpumask_var_t off the kernel stack. With 64K pages the kernel stack is 64K as well, we have enough room to keep cpumask on the stack, avoiding extra alocation and possible performance penalties.

Skip the select when ARM64_64K_PAGES is set. A 64K kernel can then raise NR_CPUS above 256 and keep cpumask_var_t as an inline array. 4K and 16K page kernels are unchanged.

---
Based on Will presentation at LPC, I've decided to have a look at Meta's stack usage, and I found that this might be an small optimization!?

---
 arch/arm64/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/Kconfig +0 -0
index 5f2db2cecf9772..b73371327d2a47 100644

arch/arm64/Kconfig config ARM64 · line 114

Quoted 5 lines
arch/arm64/Kconfig +1 -1
@@ -111,7 +111,7 @@ config ARM64
 	select CLONE_BACKWARDS
 	select COMMON_CLK
 	select CPU_PM if (SUSPEND || CPU_IDLE)
-	select CPUMASK_OFFSTACK if NR_CPUS > 256
+	select CPUMASK_OFFSTACK if NR_CPUS > 256 && !ARM64_64K_PAGES

Does this one sound sane?

maintainer
#2 lore raw
Quoted 18 lines · Breno Leitao
Quoted 17 lines · Breno Leitao

arm64 selects CPUMASK_OFFSTACK when NR_CPUS is above 256, which keeps cpumask_var_t off the kernel stack. With 64K pages the kernel stack is 64K as well, we have enough room to keep cpumask on the stack, avoiding extra alocation and possible performance penalties.

Skip the select when ARM64_64K_PAGES is set. A 64K kernel can then raise NR_CPUS above 256 and keep cpumask_var_t as an inline array. 4K and 16K page kernels are unchanged.

---
Based on Will presentation at LPC, I've decided to have a look at Meta's stack usage, and I found that this might be an small optimization!?

---
 arch/arm64/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 12 lines

arch/arm64/Kconfig config ARM64 · line 114

Quoted 10 lines
arch/arm64/Kconfig +1 -1
index 5f2db2cecf9772..b73371327d2a47 100644
@@ -111,7 +111,7 @@ config ARM64
 	select CLONE_BACKWARDS
 	select COMMON_CLK
 	select CPU_PM if (SUSPEND || CPU_IDLE)
-	select CPUMASK_OFFSTACK if NR_CPUS > 256
+	select CPUMASK_OFFSTACK if NR_CPUS > 256 && !ARM64_64K_PAGES

Does this one sound sane?

It doesn't make an awful lot of sense to me, to be honest. The page size and the number of CPUs are two independent variables, so tying them together like this looks a little odd.

Will

#3 lore raw

Hello Will,

Quoted 32 lines · Will Deacon
Quoted 18 lines · Breno Leitao
Quoted 17 lines · Breno Leitao

arm64 selects CPUMASK_OFFSTACK when NR_CPUS is above 256, which keeps cpumask_var_t off the kernel stack. With 64K pages the kernel stack is 64K as well, we have enough room to keep cpumask on the stack, avoiding extra alocation and possible performance penalties.

Skip the select when ARM64_64K_PAGES is set. A 64K kernel can then raise NR_CPUS above 256 and keep cpumask_var_t as an inline array. 4K and 16K page kernels are unchanged.

---
Based on Will presentation at LPC, I've decided to have a look at Meta's stack usage, and I found that this might be an small optimization!?

---
 arch/arm64/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 12 lines

arch/arm64/Kconfig config ARM64 · line 114

Quoted 10 lines
arch/arm64/Kconfig +1 -1
index 5f2db2cecf9772..b73371327d2a47 100644
@@ -111,7 +111,7 @@ config ARM64
 	select CLONE_BACKWARDS
 	select COMMON_CLK
 	select CPU_PM if (SUSPEND || CPU_IDLE)
-	select CPUMASK_OFFSTACK if NR_CPUS > 256
+	select CPUMASK_OFFSTACK if NR_CPUS > 256 && !ARM64_64K_PAGES

Does this one sound sane?

Quoted 3 lines

It doesn't make an awful lot of sense to me, to be honest. The page size and the number of CPUs are two independent variables, so tying them together like this looks a little odd.

Sure, but with 64K pages the kernel stack is _huge_. Why do we still need to keep the cpumask off the stack frame in that case?

maintainer
#4 lore raw
Quoted 35 lines · Breno Leitao

Hello Will,

Quoted 32 lines · Will Deacon
Quoted 30 lines · Breno Leitao
Quoted 22 lines · Breno Leitao

arm64 selects CPUMASK_OFFSTACK when NR_CPUS is above 256, which keeps cpumask_var_t off the kernel stack. With 64K pages the kernel stack is 64K as well, we have enough room to keep cpumask on the stack, avoiding extra alocation and possible performance penalties.

Skip the select when ARM64_64K_PAGES is set. A 64K kernel can then raise NR_CPUS above 256 and keep cpumask_var_t as an inline array. 4K and 16K page kernels are unchanged.

---
Based on Will presentation at LPC, I've decided to have a look at Meta's stack usage, and I found that this might be an small optimization!?

---
 arch/arm64/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/Kconfig +0 -0
index 5f2db2cecf9772..b73371327d2a47 100644

arch/arm64/Kconfig config ARM64 · line 114

Quoted 5 lines
arch/arm64/Kconfig +1 -1
@@ -111,7 +111,7 @@ config ARM64
 	select CLONE_BACKWARDS
 	select COMMON_CLK
 	select CPU_PM if (SUSPEND || CPU_IDLE)
-	select CPUMASK_OFFSTACK if NR_CPUS > 256
+	select CPUMASK_OFFSTACK if NR_CPUS > 256 && !ARM64_64K_PAGES

Does this one sound sane?

Quoted 6 lines
Quoted 3 lines

It doesn't make an awful lot of sense to me, to be honest. The page size and the number of CPUs are two independent variables, so tying them together like this looks a little odd.

Sure, but with 64K pages the kernel stack is _huge_. Why do we still need to keep the cpumask off the stack frame in that case?

Because you might have thousands of CPUs? Because copying these things by value could be expensive?

Will

#5 lore raw
Quoted 1 line · Will Deacon

Because you might have thousands of CPUs?

I checked that case. I built defconfig with 64K pages and NR_CPUS=4096, with and without this patch, and compared clang's -fstack-usage output.

With the patch, 62 functions grow by 512 bytes or more. The worst are in
cpuset: update_prstate(), cpuset_write_resmask() and cpuset_handle_hotplug() grow by ~1.5K, to 1648 bytes. Nothing crosses FRAME_WARN, and the largest frame in the build goes from 1312 to 1648 bytes, out of a 64K stack.

I believe this higher frame usage is still way better than the kmalloc at function invocation (the reason for this patch), but honestly I don't have real numbers, and I am happy to take them, if it is worth.

Quoted 1 line

Because copying these things by value could be expensive?

Hmm, I _think_ having the cpumask on the stack or not doesn't change how it is passed. cpumask_var_t is either a pointer or a one-element array, which decays to a pointer, so callees get a struct cpumask * in both cases.

Which copies do you have in mind?

Thanks,
--breno

--
PS: I am not pushing on this patch at all, I am just trying to understand the reasoning and learn.