Re: [PATCH for-6.2 v2 04/11] machine: Use the computed parameters to calculate omitted cpus

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

On 2021/7/20 0:42, Andrew Jones wrote:
> On Mon, Jul 19, 2021 at 11:20:36AM +0800, Yanan Wang wrote:
>> Currently we directly calculate the omitted cpus based on the already
>> provided parameters. This makes some cmdlines like:
>>    -smp maxcpus=16
>>    -smp sockets=2,maxcpus=16
>>    -smp sockets=2,dies=2,maxcpus=16
>>    -smp sockets=2,cores=4,maxcpus=16
>> not work. We should probably use the computed paramters to calculate
>> cpus when maxcpus is provided while cpus is omitted, which will make
>> above configs start to work.
>>
>> Note: change in this patch won't affect any existing working cmdlines
>> but allows more incomplete configs to be valid.
>>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>>   hw/core/machine.c | 17 +++++++++--------
>>   1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index c9f15b15a5..668f0a1553 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -767,26 +767,27 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>>           return;
>>       }
>>   
>> -    /* compute missing values, prefer sockets over cores over threads */
>>       maxcpus = maxcpus > 0 ? maxcpus : cpus;
>>   
>> -    if (cpus == 0) {
>> -        sockets = sockets > 0 ? sockets : 1;
>> -        cores = cores > 0 ? cores : 1;
>> -        threads = threads > 0 ? threads : 1;
>> -        cpus = sockets * dies * cores * threads;
>> -        maxcpus = maxcpus > 0 ? maxcpus : cpus;
>> -    } else if (sockets == 0) {
>> +    /* compute missing values, prefer sockets over cores over threads */
>> +    if (sockets == 0) {
>>           cores = cores > 0 ? cores : 1;
>>           threads = threads > 0 ? threads : 1;
>>           sockets = maxcpus / (dies * cores * threads);
>> +        sockets = sockets > 0 ? sockets : 1;
>>       } else if (cores == 0) {
>>           threads = threads > 0 ? threads : 1;
>>           cores = maxcpus / (sockets * dies * threads);
>> +        cores = cores > 0 ? cores : 1;
>>       } else if (threads == 0) {
>>           threads = maxcpus / (sockets * dies * cores);
>> +        threads = threads > 0 ? threads : 1;
>>       }
> I didn't think we wanted this rounding which this patch adds back into
> cores and threads and now also sockets.
Firstly, I think we can agree that with or without the rounding, the invalid
configs will still be reported as invalid. So the only difference is in 
the err
message (either report 0 or 1 of a fractional parameter). :)

I added back the rounding because this patch indeed need it, we start
to use the computed parameters to calculate cpus, so we have to ensure
that the computed parameters are at least 1. If both cpus and maxcpus
are omitted (e.g. -smp sockets=16), without the rounding we will get
zeroed cpus and maxcpus, and with the rounding we will get valid result
like "cpus=16,sockets=16,cores=1,threads=1,maxcpus=16".

If a "0" or "1" in the error message doesn't make so much difference as
assumed for the error reporting, I suggest that we probably can keep the
rounding which makes the parser code concise.
>>   
>> +    /* use the computed parameters to calculate the omitted cpus */
>> +    cpus = cpus > 0 ? cpus : sockets * dies * cores * threads;
>> +    maxcpus = maxcpus > 0 ? maxcpus : cpus;
> It doesn't really matter, but I think I'd rather write this like
>
>   maxcpus = maxcpus > 0 ? maxcpus : sockets * dies * cores * threads;
>   cpus = cpus > 0 ? cpus : maxcpus;
Yes, there is no functional difference. But I think maybe we'd better keep
some consistence with the "maxcpus = maxcpus > 0 ? maxcpus : cpus"
at top this function and also with the smp doc in qemu-option.hx i.e.
"If omitted the maximum number of CPUs will be set to match the initial
CPU count" ?

Thanks,
Yanan
.
>> +
>>       if (sockets * dies * cores * threads < cpus) {
>>           g_autofree char *dies_msg = g_strdup_printf(
>>               mc->smp_dies_supported ? " * dies (%u)" : "", dies);
>> -- 
>> 2.19.1
>>
> Thanks,
> drew
>
> .