| Message ID | 20210702100739.13672-3-wangyanan55@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | machine: smp parsing fixes and improvement | expand |
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. Thanks, drew > > Signed-off-by: Yanan Wang <wangyanan55@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 >
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. This series should then update the test cases for scenarios that we think are currently wrongly handled. > > Thanks, > drew > > > > > Signed-off-by: Yanan Wang <wangyanan55@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
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 <wangyanan55@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
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;
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". Signed-off-by: Yanan Wang <wangyanan55@huawei.com> --- hw/core/machine.c | 1 + hw/i386/pc.c | 1 + 2 files changed, 2 insertions(+)