Re: [PATCH for-6.2 v2 05/11] machine: Improve the error reporting of smp parsing

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

On 2021/7/20 0:53, Andrew Jones wrote:
> On Mon, Jul 19, 2021 at 11:20:37AM +0800, Yanan Wang wrote:
>> We totally have two requirements for a valid SMP configuration:
> s/totally//
>
>> the sum of "sockets * dies * cores * threads" must represent all
> the product
>
>> the possible cpus, i.e., max_cpus, and must include the initial
>> present cpus, i.e., smp_cpus.
>>
>> We only need to ensure "sockets * dies * cores * threads == maxcpus"
>> at first and then ensure "sockets * dies * cores * threads >= cpus".
> Or, "maxcpus >= cpus"
>
>> With a reasonable order of the sanity-check, we can simplify the
>> error reporting code.
>>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>>   hw/core/machine.c | 25 ++++++++++---------------
>>   1 file changed, 10 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 668f0a1553..8b4d07d3fc 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -788,21 +788,6 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>>       cpus = cpus > 0 ? cpus : sockets * dies * cores * threads;
>>       maxcpus = maxcpus > 0 ? maxcpus : cpus;
>>   
>> -    if (sockets * dies * cores * threads < cpus) {
>> -        g_autofree char *dies_msg = g_strdup_printf(
>> -            mc->smp_dies_supported ? " * dies (%u)" : "", dies);
>> -        error_setg(errp, "cpu topology: "
>> -                   "sockets (%u)%s * cores (%u) * threads (%u) < "
>> -                   "smp_cpus (%u)",
>> -                   sockets, dies_msg, cores, threads, cpus);
>> -        return;
>> -    }
>> -
>> -    if (maxcpus < cpus) {
>> -        error_setg(errp, "maxcpus must be equal to or greater than smp");
>> -        return;
>> -    }
> This may be redundant when determining a valid config, but by checking it
> separately we can provide a more useful error message.
Yes, this message is more useful. Can we also report the exact values of the
parameters within this error message ? How about the following:

if (sockets * cores * threads != maxcpus) {
     error_setg("product of the topology must be equal to maxcpus"
                "sockets (%u) * cores (%u) * threads (%u)"
                "!= maxcpus (%u)",
                sockets, cores, threads, maxcpus);
return;
}

if (maxcpus < cpus) {
     error_setg("maxcpus must be equal to or greater than smp:"
                "sockets (%u) * cores (%u) * threads (%u)"
                "== maxcpus (%u) < smp_cpus (%u)",
                sockets, cores, threads, maxcpus, cpus);
return;
}

Thanks,
Yanan
.
>> -
>>       if (sockets * dies * cores * threads != maxcpus) {
>>           g_autofree char *dies_msg = g_strdup_printf(
>>               mc->smp_dies_supported ? " * dies (%u)" : "", dies);
>> @@ -814,6 +799,16 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>>           return;
>>       }
>>   
>> +    if (sockets * dies * cores * threads < cpus) {
>> +        g_autofree char *dies_msg = g_strdup_printf(
>> +            mc->smp_dies_supported ? " * dies (%u)" : "", dies);
>> +        error_setg(errp, "Invalid CPU topology: "
>> +                   "sockets (%u)%s * cores (%u) * threads (%u) < "
>> +                   "smp_cpus (%u)",
>> +                   sockets, dies_msg, cores, threads, cpus);
>> +        return;
>> +    }
>> +
>>       ms->smp.cpus = cpus;
>>       ms->smp.sockets = sockets;
>>       ms->smp.dies = dies;
>> -- 
>> 2.19.1
>>
> I'm not sure we need this patch.
>
> Thanks,
> drew
>
> .