1. Patchew
  2. QEMU
  3. [PATCH for-6.2 v2 00/11] machine: smp parsing fixes and improvement
  4. View patch

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

Yanan Wang posted 11 patches 4 years, 6 months ago
Maintainers: Cornelia Huck <cohuck@redhat.com>, Greg Kurz <groug@kaod.org>, Halil Pasic <pasic@linux.ibm.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, David Gibson <david@gibson.dropbear.id.au>, Christian Borntraeger <borntraeger@de.ibm.com>, Markus Armbruster <armbru@redhat.com>, Eric Blake <eblake@redhat.com>, Thomas Huth <thuth@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, "Michael S. Tsirkin" <mst@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, David Hildenbrand <david@redhat.com>
There is a newer version of this series
[PATCH for-6.2 v2 05/11] machine: Improve the error reporting of smp parsing
Posted by Yanan Wang 4 years, 6 months ago 
We totally have two requirements for a valid SMP configuration:
the sum of "sockets * dies * cores * threads" must represent all
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".
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;
-    }
-
     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
Re: [PATCH for-6.2 v2 05/11] machine: Improve the error reporting of smp parsing
Posted by Andrew Jones 4 years, 6 months ago 
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.

> -
>      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
Re: [PATCH for-6.2 v2 05/11] machine: Improve the error reporting of smp parsing
Posted by wangyanan (Y) 4 years, 6 months ago 
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
>
> .
Re: [PATCH for-6.2 v2 05/11] machine: Improve the error reporting of smp parsing
Posted by Andrew Jones 4 years, 6 months ago 
On Thu, Jul 22, 2021 at 04:10:32PM +0800, wangyanan (Y) wrote:
> 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 ?

sure

> 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;
> }

OK by me

Thanks,
drew

> 
> 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
> > 
> > .
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.