| Message ID | 20210719032043.25416-6-wangyanan55@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | machine: smp parsing fixes and improvement | expand |
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
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 > > .
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 > > > > . >
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;
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(-)