Re: [PATCH] add command line init_start_cpus

[Andrew Morton <akpm@linux-foundation.org>]

On Sat, 13 Dec 2008 22:06:49 -0800 Yinghai Lu <yinghai@kernel.org> wrote:

> 
> Impact: new command line
> 
> so could select cpus to be started during init stage
> 

Please always always always provide a description of the *reason* for
making a change to the kernel.  For patches other than bugfixes, this
is the most important part of the patch, because without a reason, why
should we even look at the code changes?

> 
> ---
>  Documentation/kernel-parameters.txt |   14 ++++++++++++++
>  init/main.c                         |   29 ++++++++++++++++++++++++++++-
>  2 files changed, 42 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6/Documentation/kernel-parameters.txt
> ===================================================================
> --- linux-2.6.orig/Documentation/kernel-parameters.txt
> +++ linux-2.6/Documentation/kernel-parameters.txt
> @@ -918,6 +918,20 @@ and is between 256 and 4096 characters.
>  		forcesac
>  		soft
>  
> +	init_start_cpus=	[KNL,SMP] set CPUs that will be started during
> +			init stage.
> +			Format:
> +			<cpu number>,...,<cpu number>
> +			or
> +			<cpu number>-<cpu number>
> +			(must be a positive range in ascending order)
> +			or a mixture
> +			<cpu number>,...,<cpu number>-<cpu number>
> +
> +			This option can be used to specify one or more CPUs
> +			to be started during init stage.
> +			<cpu number> begins at 0 and the maximum value is
> +			"number of CPUs in system - 1".

I can't even begin to imagine why would we want this feature.

>  	intel_iommu=	[DMAR] Intel IOMMU driver (DMAR) option
>  		off
> Index: linux-2.6/init/main.c
> ===================================================================
> --- linux-2.6.orig/init/main.c
> +++ linux-2.6/init/main.c

Can we put this all somewhere else? init/main.c is a random dumping ground.

Perhaps kernel/cpu.c would be appropriate.  There's a bunch of other
cpumasky stuff in init/main.c which could also be moved into cpu.c
(and tidied up - it's a godawful ifdef mess).

> @@ -413,11 +413,35 @@ static void __init setup_per_cpu_areas(v
>  }
>  #endif /* CONFIG_HAVE_SETUP_PER_CPU_AREA */
>  
> +static __initdata cpumask_var_t cpu_init_start_mask;
> +
> +/* Setup the mask of cpus configured for init start */
> +static int __init init_start_cpu_setup(char *str)
> +{
> +	cpulist_parse(str, *cpu_init_start_mask);
> +
> +	return 1;
> +}
> +
> +__setup("init_start_cpus=", init_start_cpu_setup);
> +
>  /* Called by boot processor to activate the rest. */
>  static void __init smp_init(void)
>  {
>  	unsigned int cpu;
> +	struct cpumask *start_mask;
> +
> +	if (cpumask_empty(cpu_init_start_mask))
> +		start_mask = &cpu_present_map;
> +	else {
> +		start_mask = kmalloc(cpumask_size(), GFP_KERNEL);

Why is this dynamically allocated?  I'd have though that a simple
__initdata static store would suit?


> +		if (!start_mask)
> +			start_mask = &cpu_present_map;
> +		else
> +			cpumask_and(start_mask, cpu_init_start_mask,
> +					 &cpu_present_map);
> +	}
>  	/*
>  	 * Set up the current CPU as possible to migrate to.
>  	 * The other ones will be done by cpu_up/cpu_down()
> @@ -426,13 +450,16 @@ static void __init smp_init(void)
>  	cpu_set(cpu, cpu_active_map);
>  
>  	/* FIXME: This should be done in userspace --RR */
> -	for_each_present_cpu(cpu) {
> +	for_each_cpu_mask_nr(cpu, *start_mask) {
>  		if (num_online_cpus() >= setup_max_cpus)
>  			break;
>  		if (!cpu_online(cpu))
>  			cpu_up(cpu);
>  	}
>  
> +	if (start_mask != &cpu_present_map)
> +		kfree(start_mask);
> +