| Message ID | 794a1211-630b-3ee5-55a3-c06f10df1490@linux.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM64: Dynamically allocate cpumasks and increase supported CPUs to 512 | expand |
On Thu, Dec 14, 2023 at 04:05:56PM -0800, Christoph Lameter (Ampere) wrote: > Index: linux/arch/arm64/Kconfig > =================================================================== > --- linux.orig/arch/arm64/Kconfig > +++ linux/arch/arm64/Kconfig > @@ -1407,7 +1407,21 @@ config SCHED_SMT > config NR_CPUS > int "Maximum number of CPUs (2-4096)" > range 2 4096 I think your mailer got to your patch and messed up the white space. There are two spaces before each of these lines rather than the usual one. > - default "256" > + default 512 > + > +# > +# Determines the placement of cpumasks. > +# > +# With CPUMASK_OFFSTACK the cpumasks are dynamically allocated. > +# Useful for machines with lots of core because it avoids increasing > +# the size of many of the data structures in the kernel. > +# > +# If this is off then the cpumasks have a static sizes and are > +# embedded within data structures. > +# > +config CPUMASK_OFFSTACK > + def_bool y > + depends on NR_CPUS > 256 Should that be ">= 256" ? > > config HOTPLUG_CPU > bool "Support for hot-pluggable CPUs" Same here.
Whitespace issues aside, I have applied the patch on top of kernel 6.1.55 and tested on both a dual-socket Ampere Altra machine with < 256 CPUs, and a dual-socket AmpereOne machine with > 256 CPUs. Works as expected, with all CPUs visible and functional. > config NR_CPUS > int "Maximum number of CPUs (2-4096)" > range 2 4096 > - default "256" > + default 512 Nit: the new default value should be in quotation marks, if we want to be pedantic Tested-by: Eric Mackay <eric.mackay@oracle.com>
On 2024/1/15 23:39, Russell King (Oracle) wrote: > On Thu, Dec 14, 2023 at 04:05:56PM -0800, Christoph Lameter (Ampere) wrote: >> Index: linux/arch/arm64/Kconfig >> =================================================================== >> --- linux.orig/arch/arm64/Kconfig >> +++ linux/arch/arm64/Kconfig >> @@ -1407,7 +1407,21 @@ config SCHED_SMT >> config NR_CPUS >> int "Maximum number of CPUs (2-4096)" >> range 2 4096 > > I think your mailer got to your patch and messed up the white space. > There are two spaces before each of these lines rather than the usual > one. > >> - default "256" >> + default 512 >> + >> +# >> +# Determines the placement of cpumasks. >> +# >> +# With CPUMASK_OFFSTACK the cpumasks are dynamically allocated. >> +# Useful for machines with lots of core because it avoids increasing >> +# the size of many of the data structures in the kernel. >> +# >> +# If this is off then the cpumasks have a static sizes and are >> +# embedded within data structures. >> +# >> +config CPUMASK_OFFSTACK >> + def_bool y >> + depends on NR_CPUS > 256 > > Should that be ">= 256" ? Maybe just select CPUMASK_OFFSTACK if NR_CPUS >= 256, But could we just make CPUMASK_OFFSTACK configurable and let user/distro to enable it? diff --git a/lib/Kconfig b/lib/Kconfig index 5ddda7c2ed9b..4254be5aa843 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -535,7 +535,9 @@ config CHECK_SIGNATURE bool config CPUMASK_OFFSTACK - bool "Force CPU masks off stack" if DEBUG_PER_CPU_MAPS + bool "Force CPU masks off stack" + depends on SMP + default n help Use dynamic allocation for cpumask_var_t, instead of putting them on the stack. This is a bit more expensive, but avoids > >> >> config HOTPLUG_CPU >> bool "Support for hot-pluggable CPUs" > > Same here. >
On Tue, Jan 16, 2024 at 03:10:26PM +0800, Kefeng Wang wrote: > On 2024/1/15 23:39, Russell King (Oracle) wrote: > > On Thu, Dec 14, 2023 at 04:05:56PM -0800, Christoph Lameter (Ampere) wrote: > > > Index: linux/arch/arm64/Kconfig > > > =================================================================== > > > --- linux.orig/arch/arm64/Kconfig > > > +++ linux/arch/arm64/Kconfig > > > @@ -1407,7 +1407,21 @@ config SCHED_SMT > > > config NR_CPUS > > > int "Maximum number of CPUs (2-4096)" > > > range 2 4096 > > > > I think your mailer got to your patch and messed up the white space. > > There are two spaces before each of these lines rather than the usual > > one. > > > > > - default "256" > > > + default 512 > > > + > > > +# > > > +# Determines the placement of cpumasks. > > > +# > > > +# With CPUMASK_OFFSTACK the cpumasks are dynamically allocated. > > > +# Useful for machines with lots of core because it avoids increasing > > > +# the size of many of the data structures in the kernel. > > > +# > > > +# If this is off then the cpumasks have a static sizes and are > > > +# embedded within data structures. > > > +# > > > +config CPUMASK_OFFSTACK > > > + def_bool y > > > + depends on NR_CPUS > 256 > > > > Should that be ">= 256" ? > > Maybe just select CPUMASK_OFFSTACK if NR_CPUS >= 256, > > > But could we just make CPUMASK_OFFSTACK configurable and let user/distro > to enable it? > > diff --git a/lib/Kconfig b/lib/Kconfig > index 5ddda7c2ed9b..4254be5aa843 100644 > --- a/lib/Kconfig > +++ b/lib/Kconfig > @@ -535,7 +535,9 @@ config CHECK_SIGNATURE > bool > > config CPUMASK_OFFSTACK > - bool "Force CPU masks off stack" if DEBUG_PER_CPU_MAPS > + bool "Force CPU masks off stack" > + depends on SMP > + default n Please. No. There is no point in defining a default of n. The default default is n. Therefore, specifying a default of n is utterly redundant as the option will still default to n and just adds clutter to Kconfig files.
On Mon, Jan 15, 2024 at 03:59:11PM -0800, Eric Mackay wrote: > Whitespace issues aside, I have applied the patch on top of kernel 6.1.55 and tested on both a dual-socket Ampere Altra machine with < 256 CPUs, and a dual-socket AmpereOne machine with > 256 CPUs. Works as expected, with all CPUs visible and functional. > > > config NR_CPUS > > int "Maximum number of CPUs (2-4096)" > > range 2 4096 > > - default "256" > > + default 512 > > Nit: the new default value should be in quotation marks, if we want to be pedantic I can't find anything that requires the quotes - and as "range" doesn't for consistency it seems that default shouldn't either. There's nothing in the documentation that indicates quotes should be used, and looking at the code, it's just treated as a string. The only thing that quotes seem to do is to ensure that whitespace will be included.
On Mon, Jan 15, 2024 at 03:39:00PM +0000, Russell King (Oracle) wrote: > On Thu, Dec 14, 2023 at 04:05:56PM -0800, Christoph Lameter (Ampere) wrote: > > +# Determines the placement of cpumasks. > > +# > > +# With CPUMASK_OFFSTACK the cpumasks are dynamically allocated. > > +# Useful for machines with lots of core because it avoids increasing > > +# the size of many of the data structures in the kernel. > > +# > > +# If this is off then the cpumasks have a static sizes and are > > +# embedded within data structures. > > +# > > +config CPUMASK_OFFSTACK > > + def_bool y > > + depends on NR_CPUS > 256 > > Should that be ">= 256" ? I don't think that ">= 256" makes sense. Note that since the cpumasks are arrays of unsigned long, they're chunked into groups of 64 bits: 2 to 64 cpus: 1 x unsigned long => 8 bytes 65 to 128 cpus: 2 x unsigned long => 16 bytes 129 to 192 cpus: 3 x unsigned long => 24 bytes 193 to 256 cpus: 4 x unsigned long => 32 bytes 257 to 320 cpus: 5 x unsigned long => 40 bytes ... and so if a mask for 256 CPUs is too big to go in the stack, so is any mask for 193+ CPUs, and so ">= 256" should be clamped down to ">= 193" or "> 192". The boundary should be just after a multiple of 64. How did we choose 256 specifically? I note that x86-64 allows 512 CPUs before requiring CPUMASK_OFFSTACK, and I see that powerpc selects CPUMASK_OFFSTACK when NR_CPUS >= 8192. Mark.
On Tue, Jan 16, 2024 1:08:20PM +0000, Mark Rutland wrote: > On Mon, Jan 15, 2024 at 03:39:00PM +0000, Russell King (Oracle) wrote: > > On Thu, Dec 14, 2023 at 04:05:56PM -0800, Christoph Lameter (Ampere) wrote: > > > +# Determines the placement of cpumasks. > > > +# > > > +# With CPUMASK_OFFSTACK the cpumasks are dynamically allocated. > > > +# Useful for machines with lots of core because it avoids increasing > > > +# the size of many of the data structures in the kernel. > > > +# > > > +# If this is off then the cpumasks have a static sizes and are > > > +# embedded within data structures. > > > +# > > > +config CPUMASK_OFFSTACK > > > + def_bool y > > > + depends on NR_CPUS > 256 > > > > Should that be ">= 256" ? > > I don't think that ">= 256" makes sense. Note that since the cpumasks are > arrays of unsigned long, they're chunked into groups of 64 bits: > > 2 to 64 cpus: 1 x unsigned long => 8 bytes > 65 to 128 cpus: 2 x unsigned long => 16 bytes > 129 to 192 cpus: 3 x unsigned long => 24 bytes > 193 to 256 cpus: 4 x unsigned long => 32 bytes > 257 to 320 cpus: 5 x unsigned long => 40 bytes > > ... and so if a mask for 256 CPUs is too big to go in the stack, so is any mask > for 193+ CPUs, and so ">= 256" should be clamped down to ">= 193" or "> 192". > The boundary should be just after a multiple of 64. > > How did we choose 256 specifically? I note that x86-64 allows 512 CPUs before > requiring CPUMASK_OFFSTACK, and I see that powerpc selects CPUMASK_OFFSTACK > when NR_CPUS >= 8192. > > Mark. The suggestion for >= 256 may have been a zero-index/one-index mixup. It seems > 256 was chosen as the cutoff simply because it preserves existing behavior. The patch description seems to imply there was pushback from distro maintainers on just increasing the default NR_CPUS. The existing default value of 256 is probably already a strain for smaller ARM systems, to which x86-64 isn't a reasonable comparison. I'm not sure what the reaction to increasing from 64 to 256 in 2019 was like, but picking a pivot point for CPUMASK_OFFSTACK beyond 256 may skew the balance even less in favor of smaller ARM systems.
On Tue, Jan 16, 2024 11:24:18PM +0000, Russell King (Oracle) wrote: > On Mon, Jan 15, 2024 at 03:59:11PM -0800, Eric Mackay wrote: > > Whitespace issues aside, I have applied the patch on top of kernel 6.1.55 and tested on both a dual-socket Ampere Altra machine with < 256 CPUs, and a dual-socket AmpereOne machine with > 256 CPUs. Works as expected, with all CPUs visible and functional. > > > > > config NR_CPUS > > > int "Maximum number of CPUs (2-4096)" > > > range 2 4096 > > > - default "256" > > > + default 512 > > > > Nit: the new default value should be in quotation marks, if we want to be pedantic > > I can't find anything that requires the quotes - and as "range" doesn't > for consistency it seems that default shouldn't either. There's nothing > in the documentation that indicates quotes should be used, and looking > at the code, it's just treated as a string. The only thing that quotes > seem to do is to ensure that whitespace will be included. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! Ack. I withdraw my nit.
On Tue, 16 Jan 2024, Eric Mackay wrote: > > It seems > 256 was chosen as the cutoff simply because it preserves existing behavior. > The patch description seems to imply there was pushback from distro maintainers on just increasing > the default NR_CPUS. Yup that was it.
On Tue, 16 Jan 2024, Eric Mackay wrote:
> Ack. I withdraw my nit.
Alright. Are we done and should I post V2?
On Wed, Jan 17, 2024 12:01:13PM -0800, Christoph Lameter (Ampere) wrote: > On Tue, 16 Jan 2024, Eric Mackay wrote: > > > Ack. I withdraw my nit. > > Alright. Are we done and should I post V2? Yes, please go ahead
Index: linux/arch/arm64/Kconfig =================================================================== --- linux.orig/arch/arm64/Kconfig +++ linux/arch/arm64/Kconfig @@ -1407,7 +1407,21 @@ config SCHED_SMT config NR_CPUS int "Maximum number of CPUs (2-4096)" range 2 4096 - default "256" + default 512 + +# +# Determines the placement of cpumasks. +# +# With CPUMASK_OFFSTACK the cpumasks are dynamically allocated. +# Useful for machines with lots of core because it avoids increasing +# the size of many of the data structures in the kernel. +# +# If this is off then the cpumasks have a static sizes and are +# embedded within data structures. +# +config CPUMASK_OFFSTACK + def_bool y + depends on NR_CPUS > 256 config HOTPLUG_CPU bool "Support for hot-pluggable CPUs"
Ampere Computing develops high end ARM processor that support an ever increasing number of processors. The default 256 processors are not enough for our newer products. The default is used by distros and therefore our customers cannot use distro kernels because the number of processors is not supported. One of the objections against earlier patches to increase the limit was that the memory use becomes too high. There is a feature called CPUMASK_OFFSTACK that configures the cpumasks in the kernel to be dynamically allocated. This was used in the X86 architecture in the past to enable support for larger CPU configurations up to 8k cpus. With that is becomes possible to dynamically size the allocation of the cpu bitmaps depending on the quantity of processors detected on bootup. This patch enables that logic if more than 256 processors are configured and increases the default to 512 processors. Further increases may be needed if ARM processor vendors start supporting more processors. Given the current inflationary trends in core counts from multiple processor manufacturers this may occur. Signed-off-by: Christoph Lameter (Ampere) <cl@linux.com> --- This was discussed earlier and this updated patch was proposed at https://lore.kernel.org/linux-arm-kernel/6a854175-5f89-c754-17b8-deda18447f1f@gentwo.org/T/