Re: [PATCH] xen/arm: Allow setting the number of CPUs to activate at runtime

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 23.05.2022 12:05, Julien Grall wrote:
> Hi,
> 
> On 23/05/2022 10:13, Michal Orzel wrote:
>> Introduce a command line parameter "maxcpus" on Arm to allow adjusting
>> the number of CPUs to activate.
> 
> The current definition "maxcpus" is not really suitable for big.LITTLE 
> systems as you have no flexibility to say how many types of each cores you 
> want to boot.
> 
> Instead, Xen will pick-up the first CPUs it parsed from the firmware tables.
> 
> 
> So what's your use-case/target?
> 
- use cases where we have no big little (although even on big.LITTLE limiting 
this number makes sense if we do not care about the types)
- debug cases where we want to set maxcpus=1

>> Currently the limit is defined by the
>> config option CONFIG_NR_CPUS. Such parameter already exists on x86.
>>
>> Define a parameter "maxcpus" and a corresponding static variable
>> max_cpus in Arm smpboot.c. Modify function smp_get_max_cpus to take
>> max_cpus as a limit and to return proper unsigned int instead of int.
>>
>> Take the opportunity to remove redundant variable cpus from start_xen
>> function and to directly assign the return value from smp_get_max_cpus
>> to nr_cpu_ids (global variable in Xen used to store the number of CPUs
>> actually activated).
>>
>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>> ---
>> max_cpus is also defined in x86 setup.c. It would be possible to join
>> these definitions in xen/common/cpu.c. However in that case, max_cpus
>> would become global which is not really what we want.
> 
> If we move the global variable, then I would also expect to move the parsing 
> parsing (i.e. smp_get_max_cpus()). So why would max_cpus end up to be global? 
> Is it because the x86 would continue to use it?
> 
Yes, that would involve more x86 modifications that actual Arm coding. That is 
why I wanted to avoid it.

>> There is already
>> global nr_cpu_ids used everywhere and max_cpus being used only in x86
>> start_xen and Arm smp_get_max_cpus should be kept local. Also there are
>> already lots of places in Xen using max_cpus (local versions) and that
>> would start to be hard to read (variable shadowing).
> 
> We should avoid variable shadowing.
> 
>> ---
>>   docs/misc/xen-command-line.pandoc |  2 +-
>>   xen/arch/arm/include/asm/smp.h    |  2 +-
>>   xen/arch/arm/setup.c              | 10 ++++------
>>   xen/arch/arm/smpboot.c            | 18 ++++++++++++------
>>   4 files changed, 18 insertions(+), 14 deletions(-)
> 
> The patch looks ok to me (see one coding style comment below). I haven't 
> acked it because I am waiting to get more input on your use-cases.
> 
> 
> [...]
> 
>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>> index 9bb32a301a..22fede6600 100644
>> --- a/xen/arch/arm/smpboot.c
>> +++ b/xen/arch/arm/smpboot.c
>> @@ -43,6 +43,10 @@ cpumask_t cpu_possible_map;
>>     struct cpuinfo_arm cpu_data[NR_CPUS];
>>   +/* maxcpus: maximum number of CPUs to activate. */
>> +static unsigned int __initdata max_cpus;
>> +integer_param("maxcpus", max_cpus);
>> +
>>   /* CPU logical map: map xen cpuid to an MPIDR */
>>   register_t __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = 
>> MPIDR_INVALID };
>>   @@ -277,16 +281,18 @@ void __init smp_init_cpus(void)
>>                       "unless the cpu affinity of all domains is 
>> specified.\n");
>>   }
>>   -int __init
>> -smp_get_max_cpus (void)
>> +unsigned int __init smp_get_max_cpus(void)
>>   {
>> -    int i, max_cpus = 0;
>> +    unsigned int i, cpus = 0;
>> +
>> +    if ( ( !max_cpus ) || ( max_cpus > nr_cpu_ids ) )
> 
> Coding style: We don't add space in the inner parentheses. I.e:
Noted, thanks.

> 
> if ( (!max_cpus) || (max_cpus > nr_cpu_ids) )
> 
>> +        max_cpus = nr_cpu_ids;
>>   -    for ( i = 0; i < nr_cpu_ids; i++ )
>> +    for ( i = 0; i < max_cpus; i++ )
>>           if ( cpu_possible(i) )
>> -            max_cpus++;
>> +            cpus++;
>>   -    return max_cpus;
>> +    return cpus;
>>   }
>>     void __init
> 

Cheers,
Michal