Message-ID: <ZcNY8HuKHMguvdf9@FVFF77S0Q05N.cambridge.arm.com>
Date: Wed, 7 Feb 2024 10:18:24 +0000
From: Mark Rutland <mark.rutland@....com>
To: "Christoph Lameter (Ampere)" <cl@...two.org>
Cc: catalin.marinas@....com, Will Deacon <will@...nel.org>,
	Jonathan.Cameron@...wei.com, Matteo.Carlini@....com,
	Valentin.Schneider@....com, akpm@...ux-foundation.org,
	anshuman.khandual@....com, Eric Mackay <eric.mackay@...cle.com>,
	dave.kleikamp@...cle.com, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	linux@...linux.org.uk, robin.murphy@....com,
	vanshikonda@...amperecomputing.com, yang@...amperecomputing.com
Subject: Re: [PATCH REPOST v2] ARM64: Dynamically allocate cpumasks and
 increase supported CPUs to 512

Hi CHristoph,

On Tue, Feb 06, 2024 at 10:09:59AM -0800, Christoph Lameter (Ampere) wrote:
> Can we get this merged for 6.9? The patch has been around for awhile now.
>
> 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.

Can we please make this simpler, and clearer e.g.

  Currently defconfig selects NR_CPUS=256, but some vendors (e.g. Ampere
  Computing) are planning to ship systems with 512 CPUs. So that all CPUs on
  these systems can be used with defconfig, we'd like to bump NR_CPUS to 512.
  Therefore this patch increases the default NR_CPUS from 256 to 512.

  As increasing NR_CPUS will increase the size of cpumasks, there's a fear that
  this might have a significant impact on stack usage due to code which places
  cpumasks on the stack. To mitigate that concern, we can select
  CPUMASK_OFFSTACK. As that doesn't seem to be a problem today with
  NR_CPUS=256, we only select this when NR_CPUS > 256.

There's no need for all the other gunk.

However, can you please comment on the impact of this? e.g. 

* Does it make the kernel Image or vmlinux any bigger? 
 
  If you can build the kernel before/after this patch and dump the output of
  'ls -l arch/arm64/boot/Image' and 'size vmlinux', that would be great.

* Does this have any obvious impact on the memory usage of the kernel?

* Does this have any obvious performance impact? e.g. is a kernel build any
  slower/faster before/after this patch on a system with 256-or-fewer CPUs?

> Tested-by: Eric Mackay <eric.mackay@...cle.com>
> Signed-off-by: Christoph Lameter (Ampere) <cl@...ux.com>
> 
> ---
> 
> Original post: https://www.spinics.net/lists/linux-mm/msg369701.html
> 
> V1->V2
> 
> - Keep quotation marks
> - Remove whiltespace damage
> - Add tested by
> 
> 
> Index: linux/arch/arm64/Kconfig
> ===================================================================

Can you please use git format-patch? This is more painful than necessary to
apply with usual tools like b4 and git am, and the arm64 maintainers are much
more likely to pick up a patch when they don't have to go outside of their
usual workflow to do so...

> --- 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.
> +#

This described the semantic of CPUMASK_OFFSTACK, which is not specific to
arm64. If we need a comment, it should explain *why* we select this for more
than 256 CPUs specifically.

I think we can just delete this comment and rely on having that rationale in
the commit message. We can find that with git-log and git-blame.

With this comment gone, the patch itself looks fine to me, but as above the
commit message needs to be cleaned up and it'd be *really* helpful if you could
send this a git formatted patch.

Mark.

> +config CPUMASK_OFFSTACK
> +	def_bool y
> +	depends on NR_CPUS > 256
> 
>   config HOTPLUG_CPU
>   	bool "Support for hot-pluggable CPUs"
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Powered by blists - more mailing lists