Re: [RFC PATCH v2 2/4] hw/arm/virt: Parse -smp cluster parameter in virt_smp_parse

["wangyanan (Y)" <wangyanan55@huawei.com>]

Hi Drew,

On 2021/4/28 18:31, Andrew Jones wrote:
> On Tue, Apr 13, 2021 at 04:31:45PM +0800, Yanan Wang wrote:
>> There is a separate function virt_smp_parse() in hw/virt/arm.c used
>> to parse cpu topology for the ARM machines. So add parsing of -smp
>> cluster parameter in it, then total number of logical cpus will be
>> calculated like: max_cpus = sockets * clusters * cores * threads.
>>
>> In virt_smp_parse(), the computing logic of missing values prefers
>> cores over sockets over threads. And for compatibility, the value
>> of clusters will be set as default 1 if not explicitly specified.
>>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>>   hw/arm/virt.c | 32 ++++++++++++++++++--------------
>>   1 file changed, 18 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 57ef961cb5..51797628db 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -2639,35 +2639,38 @@ static void virt_smp_parse(MachineState *ms, QemuOpts *opts)
>>       if (opts) {
>>           unsigned cpus = qemu_opt_get_number(opts, "cpus", 0);
>>           unsigned sockets = qemu_opt_get_number(opts, "sockets", 0);
>> +        unsigned clusters = qemu_opt_get_number(opts, "clusters", 1);
>>           unsigned cores = qemu_opt_get_number(opts, "cores", 0);
>>           unsigned threads = qemu_opt_get_number(opts, "threads", 0);
>> +        VirtMachineState *vms = VIRT_MACHINE(ms);
>>   
>>           /*
>> -         * Compute missing values; prefer cores over sockets and
>> -         * sockets over threads.
>> +         * Compute missing values; prefer cores over sockets and sockets
>> +         * over threads. For compatibility, value of clusters will have
>> +         * been set as default 1 if not explicitly specified.
>>            */
>>           if (cpus == 0 || cores == 0) {
>>               sockets = sockets > 0 ? sockets : 1;
>>               threads = threads > 0 ? threads : 1;
>>               if (cpus == 0) {
>>                   cores = cores > 0 ? cores : 1;
>> -                cpus = cores * threads * sockets;
>> +                cpus = sockets * clusters * cores * threads;
>>               } else {
>>                   ms->smp.max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
>> -                cores = ms->smp.max_cpus / (sockets * threads);
>> +                cores = ms->smp.max_cpus / (sockets * clusters * threads);
>>               }
>>           } else if (sockets == 0) {
>>               threads = threads > 0 ? threads : 1;
>> -            sockets = cpus / (cores * threads);
>> +            sockets = cpus / (clusters * cores * threads);
>>               sockets = sockets > 0 ? sockets : 1;
> If we initialize clusters to zero instead of one and add lines in
> 'cpus == 0 || cores == 0' and 'sockets == 0' like
> 'clusters = clusters > 0 ? clusters : 1' as needed, then I think we can
> add
>
>   } else if (clusters == 0) {
>       threads = threads > 0 ? threads : 1;
>       clusters = cpus / (sockets * cores * thread);
>       clusters = clusters > 0 ? clusters : 1;
>   }
>
> here.
I have thought about this kind of format before, but there is a little bit
difference between these two ways. Let's chose the better and more
reasonable one of the two.

Way A currently in this patch:
If value of clusters is not explicitly specified in -smp command line, 
we assume
that users don't want to support clusters, for compatibility we 
initialized the
value to 1. So that with cmdline "-smp cpus=24, sockets=2, cores=6", we will
parse out the topology description like below:
cpus=24, sockets=2, clusters=1, cores=6, threads=2

Way B that you suggested for this patch:
Whether value of clusters is explicitly specified in -smp command line 
or not,
we assume that clusters are supported and calculate the value. So that with
cmdline "-smp cpus=24, sockets=2, cores=6", we will parse out the topology
description like below:
cpus =24, sockets=2, clusters=2, cores=6, threads=1

But I think maybe we should not assume too much about what users think
through the -smp command line. We should just assume that all levels of
cpu topology are supported and calculate them, and users should be more
careful if they want to get the expected results with not so complete 
cmdline.
If I'm right, then Way B should be better. :)

Thanks,
Yanan
>>           } else if (threads == 0) {
>> -            threads = cpus / (cores * sockets);
>> +            threads = cpus / (sockets * clusters * cores);
>>               threads = threads > 0 ? threads : 1;
>> -        } else if (sockets * cores * threads < cpus) {
>> +        } else if (sockets * clusters * cores * threads < cpus) {
>>               error_report("cpu topology: "
>> -                         "sockets (%u) * cores (%u) * threads (%u) < "
>> -                         "smp_cpus (%u)",
>> -                         sockets, cores, threads, cpus);
>> +                         "sockets (%u) * clusters (%u) * cores (%u) * "
>> +                         "threads (%u) < smp_cpus (%u)",
>> +                         sockets, clusters, cores, threads, cpus);
>>               exit(1);
>>           }
>>   
>> @@ -2678,11 +2681,11 @@ static void virt_smp_parse(MachineState *ms, QemuOpts *opts)
>>               exit(1);
>>           }
>>   
>> -        if (sockets * cores * threads != ms->smp.max_cpus) {
>> +        if (sockets * clusters * cores * threads != ms->smp.max_cpus) {
>>               error_report("cpu topology: "
>> -                         "sockets (%u) * cores (%u) * threads (%u)"
>> -                         "!= maxcpus (%u)",
>> -                         sockets, cores, threads,
>> +                         "sockets (%u) * clusters (%u) * cores (%u) * "
>> +                         "threads (%u) != maxcpus (%u)",
>> +                         sockets, clusters, cores, threads,
>>                            ms->smp.max_cpus);
>>               exit(1);
>>           }
>> @@ -2691,6 +2694,7 @@ static void virt_smp_parse(MachineState *ms, QemuOpts *opts)
>>           ms->smp.cores = cores;
>>           ms->smp.threads = threads;
>>           ms->smp.sockets = sockets;
>> +        vms->smp_clusters = clusters;
>>       }
>>   
>>       if (ms->smp.cpus > 1) {
>> -- 
>> 2.19.1
>>
> Thanks,
> drew
>
> .