Re: [RFC PATCH 2/6] machine: Perform zero-check for the computed value of sockets

[wangyanan (Y)]

On 2021/7/12 23:30, Daniel P. Berrangé wrote:

On Mon, Jul 12, 2021 at 05:00:48PM +0200, Andrew Jones wrote:

On Fri, Jul 02, 2021 at 06:07:35PM +0800, Yanan Wang wrote:

We currently perform zero-check (default the value to 1 if zeroed)
for the computed values of cores/threads, to make sure they are at
least 1. For consistency, we probably should also default sockets
to 1 if the computed value is zero. Note that this won't affect
any existing working cmdlines but will improve the error reporting
of the invalid ones such as "-smp 8,maxcpus=9,cores=10,threads=1".

How does this help error checking? If the user input values that compute a
fractional (rounded down to zero with integer division) value, then we'll
catch that with the sockets*cores*threads == maxcpus test now, but we may
not catch that after this patch.

Since we're having alot of debates about what should be valid scenarios
vs invalid scenarios, I think this points towards introducing a tests
for the smp_parse function, that enumerates both the correct and
incorrect scenarios based on the current implementation.

I think so! Actually I'm already working on a simple QEMU unit test
(test-smp-parse.c) for smp parsing and plan to post it in v2. But it
will directly test the finally improved generic parser.

Thanks,
Yanan
.

This series should then update the test cases for scenarios that we
think are currently wrongly handled.

Thanks,
drew

Signed-off-by: Yanan Wang <wangyana...@huawei.com>
---
  hw/core/machine.c | 1 +
  hw/i386/pc.c      | 1 +
  2 files changed, 2 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index f17bbe3275..1e194677cd 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -761,6 +761,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration 
*config, Error **errp)
          } else {
              maxcpus = maxcpus > 0 ? maxcpus : cpus;
              sockets = maxcpus / (cores * threads);
+            sockets = sockets > 0 ? sockets : 1;
          }
      } else if (cores == 0) {
          threads = threads > 0 ? threads : 1;
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index a9b22fdc01..a44511c937 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -729,6 +729,7 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration 
*config, Error **err
          } else {
              maxcpus = maxcpus > 0 ? maxcpus : cpus;
              sockets = maxcpus / (dies * cores * threads);
+            sockets = sockets > 0 ? sockets : 1;
          }
      } else if (cores == 0) {
          threads = threads > 0 ? threads : 1;
--
2.19.1

Regards,
Daniel