| Message ID | 20210702100739.13672-4-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:36PM +0800, Yanan Wang wrote: > It's possible that dies parameter is explicitly specified as "dies=0" > in the cmdline, if so we will wrongly calculate the other ommited > parameters such as "sockets = maxcpus / (dies * cores * threads);" > with a zeroed dies value. > > So perform zero-check (default the value to 1 if zeroed) for -smp dies > before using it to calculate other parameters. OK, dies=0 may make some sense for a user that doesn't want to describe dies. Reviewed-by: Andrew Jones <drjones@redhat.com> > > Fixes: 1b45842203540 (vl.c: Add -smp, dies=* command line support and update doc) > Signed-off-by: Yanan Wang <wangyanan55@huawei.com> > --- > hw/i386/pc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index a44511c937..93d1f12a49 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -714,12 +714,14 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err > { > unsigned cpus = config->has_cpus ? config->cpus : 0; > unsigned sockets = config->has_sockets ? config->sockets : 0; > - unsigned dies = config->has_dies ? config->dies : 1; > + unsigned dies = config->has_dies ? config->dies : 0; > unsigned cores = config->has_cores ? config->cores : 0; > unsigned threads = config->has_threads ? config->threads : 0; > unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0; > > /* compute missing values, prefer sockets over cores over threads */ > + dies = dies > 0 ? dies : 1; > + > if (cpus == 0 || sockets == 0) { > cores = cores > 0 ? cores : 1; > threads = threads > 0 ? threads : 1; > -- > 2.19.1 >
On Mon, Jul 12, 2021 at 05:04:04PM +0200, Andrew Jones wrote: > On Fri, Jul 02, 2021 at 06:07:36PM +0800, Yanan Wang wrote: > > It's possible that dies parameter is explicitly specified as "dies=0" > > in the cmdline, if so we will wrongly calculate the other ommited > > parameters such as "sockets = maxcpus / (dies * cores * threads);" > > with a zeroed dies value. > > > > So perform zero-check (default the value to 1 if zeroed) for -smp dies > > before using it to calculate other parameters. > > OK, dies=0 may make some sense for a user that doesn't want to describe > dies. > > Reviewed-by: Andrew Jones <drjones@redhat.com> But... this is the pc smp parse function, not the general one, so maybe my comment from patch 1 should apply here as well, which is, that dies=0 should be an error rather than silently changed to dies=1. > > > > > Fixes: 1b45842203540 (vl.c: Add -smp, dies=* command line support and update doc) > > Signed-off-by: Yanan Wang <wangyanan55@huawei.com> > > --- > > hw/i386/pc.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index a44511c937..93d1f12a49 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -714,12 +714,14 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err > > { > > unsigned cpus = config->has_cpus ? config->cpus : 0; > > unsigned sockets = config->has_sockets ? config->sockets : 0; > > - unsigned dies = config->has_dies ? config->dies : 1; > > + unsigned dies = config->has_dies ? config->dies : 0; > > unsigned cores = config->has_cores ? config->cores : 0; > > unsigned threads = config->has_threads ? config->threads : 0; > > unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0; > > > > /* compute missing values, prefer sockets over cores over threads */ > > + dies = dies > 0 ? dies : 1; > > + > > if (cpus == 0 || sockets == 0) { > > cores = cores > 0 ? cores : 1; > > threads = threads > 0 ? threads : 1; > > -- > > 2.19.1 > >
On Mon, Jul 12, 2021 at 05:05:43PM +0200, Andrew Jones wrote: > On Mon, Jul 12, 2021 at 05:04:04PM +0200, Andrew Jones wrote: > > On Fri, Jul 02, 2021 at 06:07:36PM +0800, Yanan Wang wrote: > > > It's possible that dies parameter is explicitly specified as "dies=0" > > > in the cmdline, if so we will wrongly calculate the other ommited > > > parameters such as "sockets = maxcpus / (dies * cores * threads);" > > > with a zeroed dies value. > > > > > > So perform zero-check (default the value to 1 if zeroed) for -smp dies > > > before using it to calculate other parameters. > > > > OK, dies=0 may make some sense for a user that doesn't want to describe > > dies. > > > > Reviewed-by: Andrew Jones <drjones@redhat.com> > > But... this is the pc smp parse function, not the general one, so maybe my > comment from patch 1 should apply here as well, which is, that dies=0 > should be an error rather than silently changed to dies=1. Also, after reading Daniel's comment on a later patch, I think anything=0 should just be an error, even for the general parser. Thanks, drew > > > > > > > > > Fixes: 1b45842203540 (vl.c: Add -smp, dies=* command line support and update doc) > > > Signed-off-by: Yanan Wang <wangyanan55@huawei.com> > > > --- > > > hw/i386/pc.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > > index a44511c937..93d1f12a49 100644 > > > --- a/hw/i386/pc.c > > > +++ b/hw/i386/pc.c > > > @@ -714,12 +714,14 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err > > > { > > > unsigned cpus = config->has_cpus ? config->cpus : 0; > > > unsigned sockets = config->has_sockets ? config->sockets : 0; > > > - unsigned dies = config->has_dies ? config->dies : 1; > > > + unsigned dies = config->has_dies ? config->dies : 0; > > > unsigned cores = config->has_cores ? config->cores : 0; > > > unsigned threads = config->has_threads ? config->threads : 0; > > > unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0; > > > > > > /* compute missing values, prefer sockets over cores over threads */ > > > + dies = dies > 0 ? dies : 1; > > > + > > > if (cpus == 0 || sockets == 0) { > > > cores = cores > 0 ? cores : 1; > > > threads = threads > 0 ? threads : 1; > > > -- > > > 2.19.1 > > >
On 2021/7/12 23:27, Andrew Jones wrote: > On Mon, Jul 12, 2021 at 05:05:43PM +0200, Andrew Jones wrote: >> On Mon, Jul 12, 2021 at 05:04:04PM +0200, Andrew Jones wrote: >>> On Fri, Jul 02, 2021 at 06:07:36PM +0800, Yanan Wang wrote: >>>> It's possible that dies parameter is explicitly specified as "dies=0" >>>> in the cmdline, if so we will wrongly calculate the other ommited >>>> parameters such as "sockets = maxcpus / (dies * cores * threads);" >>>> with a zeroed dies value. >>>> >>>> So perform zero-check (default the value to 1 if zeroed) for -smp dies >>>> before using it to calculate other parameters. >>> OK, dies=0 may make some sense for a user that doesn't want to describe >>> dies. >>> >>> Reviewed-by: Andrew Jones <drjones@redhat.com> >> But... this is the pc smp parse function, not the general one, so maybe my >> comment from patch 1 should apply here as well, which is, that dies=0 >> should be an error rather than silently changed to dies=1. > Also, after reading Daniel's comment on a later patch, I think anything=0 > should just be an error, even for the general parser. > Ok, after reading comments in patch #1, I also agree with this solution. We can reject the "anything = 0" configuration in advance in function machine_smp_parse(). I will do it in v2. BTW, I have converted smp_parse() into a generic helper allowing future extension for new arch-specific parameters, as you have suggested. So that we don't need to maintain duplicated parsing code any more. Thanks, Yanan . > >>>> Fixes: 1b45842203540 (vl.c: Add -smp, dies=* command line support and update doc) >>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com> >>>> --- >>>> hw/i386/pc.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >>>> index a44511c937..93d1f12a49 100644 >>>> --- a/hw/i386/pc.c >>>> +++ b/hw/i386/pc.c >>>> @@ -714,12 +714,14 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err >>>> { >>>> unsigned cpus = config->has_cpus ? config->cpus : 0; >>>> unsigned sockets = config->has_sockets ? config->sockets : 0; >>>> - unsigned dies = config->has_dies ? config->dies : 1; >>>> + unsigned dies = config->has_dies ? config->dies : 0; >>>> unsigned cores = config->has_cores ? config->cores : 0; >>>> unsigned threads = config->has_threads ? config->threads : 0; >>>> unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0; >>>> >>>> /* compute missing values, prefer sockets over cores over threads */ >>>> + dies = dies > 0 ? dies : 1; >>>> + >>>> if (cpus == 0 || sockets == 0) { >>>> cores = cores > 0 ? cores : 1; >>>> threads = threads > 0 ? threads : 1; >>>> -- >>>> 2.19.1 >>>> > .
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index a44511c937..93d1f12a49 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -714,12 +714,14 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err { unsigned cpus = config->has_cpus ? config->cpus : 0; unsigned sockets = config->has_sockets ? config->sockets : 0; - unsigned dies = config->has_dies ? config->dies : 1; + unsigned dies = config->has_dies ? config->dies : 0; unsigned cores = config->has_cores ? config->cores : 0; unsigned threads = config->has_threads ? config->threads : 0; unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0; /* compute missing values, prefer sockets over cores over threads */ + dies = dies > 0 ? dies : 1; + if (cpus == 0 || sockets == 0) { cores = cores > 0 ? cores : 1; threads = threads > 0 ? threads : 1;
It's possible that dies parameter is explicitly specified as "dies=0" in the cmdline, if so we will wrongly calculate the other ommited parameters such as "sockets = maxcpus / (dies * cores * threads);" with a zeroed dies value. So perform zero-check (default the value to 1 if zeroed) for -smp dies before using it to calculate other parameters. Fixes: 1b45842203540 (vl.c: Add -smp, dies=* command line support and update doc) Signed-off-by: Yanan Wang <wangyanan55@huawei.com> --- hw/i386/pc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)