| Message ID | 1400095122-24736-1-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
Ping? Who can get this applied? Without this, the machine->max_cpus limit is ignored when using cpu-add. On Wed, May 14, 2014 at 04:18:42PM -0300, Eduardo Habkost wrote: > If a given machine have max_cpus set, not just smp_cpus needs to be > limited, but the total number of CPUs (considering CPU hotplug) for the > machine. > > We also had yet another max_cpus limit check at smp_parse(), ensuring > that max_cpus < MAX_CPUMASK_BITS. > > This patch unifies the machine->max_cpus and MAX_CPUMASK_BITS checks, > and also moves the (smp_cpus <= max_cpus) outside smp_parse(), to keep > all checks in the same place. > > With those changes, the new code ensures that: > > 1 <= smp_cpus <= max_cpus <= machine->max_cpus <= MAX_CPUMASK_BITS > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > Cc: Peter Maydell <peter.maydell@linaro.org> > Cc: Andreas Färber <afaerber@suse.de> > Cc: Igor Mammedov <imammedo@redhat.com> > Cc: Marcelo Tosatti <mtosatti@redhat.com> > --- > Changes v1 -> v2: > * v1 was: [PATCH] vl.c: Check max_cpus limit instead of smp_cpus > * Unify machine->max_cpus and MAX_CPUMASK_BITS check > * Move all checks outside smp_parse() > * Add assert() lines ensuring the results are consistent > * s/machine/machine_class/, after rebase to latest qemu.git > (commit 6b342cc) > --- > vl.c | 26 ++++++++++++++------------ > 1 file changed, 14 insertions(+), 12 deletions(-) > > diff --git a/vl.c b/vl.c > index 709d8cd..749abbd 100644 > --- a/vl.c > +++ b/vl.c > @@ -1425,15 +1425,6 @@ static void smp_parse(QemuOpts *opts) > max_cpus = smp_cpus; > } > > - if (max_cpus > MAX_CPUMASK_BITS) { > - fprintf(stderr, "Unsupported number of maxcpus\n"); > - exit(1); > - } > - if (max_cpus < smp_cpus) { > - fprintf(stderr, "maxcpus must be equal to or greater than smp\n"); > - exit(1); > - } > - > } > > static void configure_realtime(QemuOpts *opts) > @@ -4054,13 +4045,24 @@ int main(int argc, char **argv, char **envp) > smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL)); > > machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */ > - if (smp_cpus > machine_class->max_cpus) { > - fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus " > - "supported by machine `%s' (%d)\n", smp_cpus, > + machine_class->max_cpus = MIN(machine_class->max_cpus, MAX_CPUMASK_BITS); > + > + if (max_cpus < smp_cpus) { > + fprintf(stderr, "maxcpus must be equal to or greater than smp\n"); > + exit(1); > + } > + if (max_cpus > machine_class->max_cpus) { > + fprintf(stderr, "Total number of CPUs (%d), exceeds maximum " > + "supported by machine `%s' (%d)\n", max_cpus, > machine_class->name, machine_class->max_cpus); > exit(1); > } > > + assert(1 <= smp_cpus); > + assert(smp_cpus <= max_cpus); > + assert(max_cpus <= machine_class->max_cpus); > + assert(machine_class->max_cpus <= MAX_CPUMASK_BITS); > + > /* > * Get the default machine options from the machine if it is not already > * specified either by the configuration file or by the command line. > -- > 1.9.0 > >
On Wed, 14 May 2014 16:18:42 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > If a given machine have max_cpus set, not just smp_cpus needs to be > limited, but the total number of CPUs (considering CPU hotplug) for the > machine. > > We also had yet another max_cpus limit check at smp_parse(), ensuring > that max_cpus < MAX_CPUMASK_BITS. > > This patch unifies the machine->max_cpus and MAX_CPUMASK_BITS checks, > and also moves the (smp_cpus <= max_cpus) outside smp_parse(), to keep > all checks in the same place. > > With those changes, the new code ensures that: > > 1 <= smp_cpus <= max_cpus <= machine->max_cpus <= MAX_CPUMASK_BITS > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > Cc: Peter Maydell <peter.maydell@linaro.org> > Cc: Andreas Färber <afaerber@suse.de> > Cc: Igor Mammedov <imammedo@redhat.com> > Cc: Marcelo Tosatti <mtosatti@redhat.com> > --- > Changes v1 -> v2: > * v1 was: [PATCH] vl.c: Check max_cpus limit instead of smp_cpus > * Unify machine->max_cpus and MAX_CPUMASK_BITS check > * Move all checks outside smp_parse() > * Add assert() lines ensuring the results are consistent > * s/machine/machine_class/, after rebase to latest qemu.git > (commit 6b342cc) > --- > vl.c | 26 ++++++++++++++------------ > 1 file changed, 14 insertions(+), 12 deletions(-) > > diff --git a/vl.c b/vl.c > index 709d8cd..749abbd 100644 > --- a/vl.c > +++ b/vl.c > @@ -1425,15 +1425,6 @@ static void smp_parse(QemuOpts *opts) > max_cpus = smp_cpus; > } > > - if (max_cpus > MAX_CPUMASK_BITS) { > - fprintf(stderr, "Unsupported number of maxcpus\n"); > - exit(1); > - } > - if (max_cpus < smp_cpus) { > - fprintf(stderr, "maxcpus must be equal to or greater than smp\n"); > - exit(1); > - } > - > } > > static void configure_realtime(QemuOpts *opts) > @@ -4054,13 +4045,24 @@ int main(int argc, char **argv, char **envp) > smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL)); > > machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */ > - if (smp_cpus > machine_class->max_cpus) { > - fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus " > - "supported by machine `%s' (%d)\n", smp_cpus, > + machine_class->max_cpus = MIN(machine_class->max_cpus, MAX_CPUMASK_BITS); > + > + if (max_cpus < smp_cpus) { > + fprintf(stderr, "maxcpus must be equal to or greater than smp\n"); > + exit(1); > + } > + if (max_cpus > machine_class->max_cpus) { > + fprintf(stderr, "Total number of CPUs (%d), exceeds maximum " > + "supported by machine `%s' (%d)\n", max_cpus, > machine_class->name, machine_class->max_cpus); > exit(1); > } > > + assert(1 <= smp_cpus); > + assert(smp_cpus <= max_cpus); > + assert(max_cpus <= machine_class->max_cpus); > + assert(machine_class->max_cpus <= MAX_CPUMASK_BITS); It would be better complement some assertions with checks with proper error reporting. also try to test following combos: -smp 0,maxcpus=0 -smp -1 -smp 0,maxcpus=-1 -smp 1,maxcpus=0xFFFFFFFF we need to handle values as unsigned at least and maybe also ban 0 as valid one. The rest seems to work as expected. > + > /* > * Get the default machine options from the machine if it is not already > * specified either by the configuration file or by the command line. > -- > 1.9.0 >
On Fri, May 30, 2014 at 09:50:33AM +0200, Igor Mammedov wrote: > On Wed, 14 May 2014 16:18:42 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > If a given machine have max_cpus set, not just smp_cpus needs to be > > limited, but the total number of CPUs (considering CPU hotplug) for the > > machine. > > > > We also had yet another max_cpus limit check at smp_parse(), ensuring > > that max_cpus < MAX_CPUMASK_BITS. > > > > This patch unifies the machine->max_cpus and MAX_CPUMASK_BITS checks, > > and also moves the (smp_cpus <= max_cpus) outside smp_parse(), to keep > > all checks in the same place. > > > > With those changes, the new code ensures that: > > > > 1 <= smp_cpus <= max_cpus <= machine->max_cpus <= MAX_CPUMASK_BITS > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > Cc: Peter Maydell <peter.maydell@linaro.org> > > Cc: Andreas Färber <afaerber@suse.de> > > Cc: Igor Mammedov <imammedo@redhat.com> > > Cc: Marcelo Tosatti <mtosatti@redhat.com> > > --- > > Changes v1 -> v2: > > * v1 was: [PATCH] vl.c: Check max_cpus limit instead of smp_cpus > > * Unify machine->max_cpus and MAX_CPUMASK_BITS check > > * Move all checks outside smp_parse() > > * Add assert() lines ensuring the results are consistent > > * s/machine/machine_class/, after rebase to latest qemu.git > > (commit 6b342cc) > > --- > > vl.c | 26 ++++++++++++++------------ > > 1 file changed, 14 insertions(+), 12 deletions(-) > > > > diff --git a/vl.c b/vl.c > > index 709d8cd..749abbd 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -1425,15 +1425,6 @@ static void smp_parse(QemuOpts *opts) > > max_cpus = smp_cpus; > > } > > > > - if (max_cpus > MAX_CPUMASK_BITS) { > > - fprintf(stderr, "Unsupported number of maxcpus\n"); > > - exit(1); > > - } > > - if (max_cpus < smp_cpus) { > > - fprintf(stderr, "maxcpus must be equal to or greater than smp\n"); > > - exit(1); > > - } > > - > > } > > > > static void configure_realtime(QemuOpts *opts) > > @@ -4054,13 +4045,24 @@ int main(int argc, char **argv, char **envp) > > smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL)); > > > > machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */ > > - if (smp_cpus > machine_class->max_cpus) { > > - fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus " > > - "supported by machine `%s' (%d)\n", smp_cpus, > > + machine_class->max_cpus = MIN(machine_class->max_cpus, MAX_CPUMASK_BITS); > > + > > + if (max_cpus < smp_cpus) { > > + fprintf(stderr, "maxcpus must be equal to or greater than smp\n"); > > + exit(1); > > + } > > + if (max_cpus > machine_class->max_cpus) { > > + fprintf(stderr, "Total number of CPUs (%d), exceeds maximum " > > + "supported by machine `%s' (%d)\n", max_cpus, > > machine_class->name, machine_class->max_cpus); > > exit(1); > > } > > > > + assert(1 <= smp_cpus); > > + assert(smp_cpus <= max_cpus); > > + assert(max_cpus <= machine_class->max_cpus); > > + assert(machine_class->max_cpus <= MAX_CPUMASK_BITS); > It would be better complement some assertions with checks with proper error > reporting. Absolutely. I was assuming all assert()s above have corresponding error checks/reporting. If they can be triggered in any way, that's a bug. > > also try to test following combos: > -smp 0,maxcpus=0 This gets translated to cpus=0,sockets=0,cores=0,threads=0. sockets,cores,threads become 1 when they are 0. cpus becomes sockets*cores*threads when it is zero. So, this gets translated to smp_cpus=1. That's an existing feature. Arguably confusing (and we may change it), but not a bug. > -smp -1 assert() is triggered, that's a bug. Thanks for finding it. (And that was the whole point of adding the assertions: to detect bugs.) > -smp 0,maxcpus=-1 $ ./install/bin/qemu-system-x86_64 0,maxcpus=0 -vnc :0 -S -monitor stdio -smp 0,maxcpus=-1 maxcpus must be equal to or greater than smp $ That's correct behavior. > -smp 1,maxcpus=0xFFFFFFFF $ ./install/bin/qemu-system-x86_64 0,maxcpus=0 -vnc :0 -S -monitor stdio -smp 1,maxcpus=0xFFFFFFFF maxcpus must be equal to or greater than smp $ The QemuOpts API can't (and never could) differentiate MAX_UINT from -1. Arguably a confusing feature (and we may try to change it), but not a bug. And the only way to change this behavior is by changing the QemuOpts API. > > we need to handle values as unsigned at least and maybe also ban 0 as valid > one. We translate "0" to "use the default". A confusing existing feature we may want to drop, but not a bug. > > The rest seems to work as expected. Thanks!
On Mon, Jun 02, 2014 at 12:51:53PM -0300, Eduardo Habkost wrote: [...] > > -smp -1 > > assert() is triggered, that's a bug. Thanks for finding it. > > (And that was the whole point of adding the assertions: to detect bugs.) This is fixed by: [PATCH] vl.c: Check -smp option ranges before setting int globals submitted yesterday.
diff --git a/vl.c b/vl.c index 709d8cd..749abbd 100644 --- a/vl.c +++ b/vl.c @@ -1425,15 +1425,6 @@ static void smp_parse(QemuOpts *opts) max_cpus = smp_cpus; } - if (max_cpus > MAX_CPUMASK_BITS) { - fprintf(stderr, "Unsupported number of maxcpus\n"); - exit(1); - } - if (max_cpus < smp_cpus) { - fprintf(stderr, "maxcpus must be equal to or greater than smp\n"); - exit(1); - } - } static void configure_realtime(QemuOpts *opts) @@ -4054,13 +4045,24 @@ int main(int argc, char **argv, char **envp) smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL)); machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */ - if (smp_cpus > machine_class->max_cpus) { - fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus " - "supported by machine `%s' (%d)\n", smp_cpus, + machine_class->max_cpus = MIN(machine_class->max_cpus, MAX_CPUMASK_BITS); + + if (max_cpus < smp_cpus) { + fprintf(stderr, "maxcpus must be equal to or greater than smp\n"); + exit(1); + } + if (max_cpus > machine_class->max_cpus) { + fprintf(stderr, "Total number of CPUs (%d), exceeds maximum " + "supported by machine `%s' (%d)\n", max_cpus, machine_class->name, machine_class->max_cpus); exit(1); } + assert(1 <= smp_cpus); + assert(smp_cpus <= max_cpus); + assert(max_cpus <= machine_class->max_cpus); + assert(machine_class->max_cpus <= MAX_CPUMASK_BITS); + /* * Get the default machine options from the machine if it is not already * specified either by the configuration file or by the command line.
If a given machine have max_cpus set, not just smp_cpus needs to be limited, but the total number of CPUs (considering CPU hotplug) for the machine. We also had yet another max_cpus limit check at smp_parse(), ensuring that max_cpus < MAX_CPUMASK_BITS. This patch unifies the machine->max_cpus and MAX_CPUMASK_BITS checks, and also moves the (smp_cpus <= max_cpus) outside smp_parse(), to keep all checks in the same place. With those changes, the new code ensures that: 1 <= smp_cpus <= max_cpus <= machine->max_cpus <= MAX_CPUMASK_BITS Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> Cc: Peter Maydell <peter.maydell@linaro.org> Cc: Andreas Färber <afaerber@suse.de> Cc: Igor Mammedov <imammedo@redhat.com> Cc: Marcelo Tosatti <mtosatti@redhat.com> --- Changes v1 -> v2: * v1 was: [PATCH] vl.c: Check max_cpus limit instead of smp_cpus * Unify machine->max_cpus and MAX_CPUMASK_BITS check * Move all checks outside smp_parse() * Add assert() lines ensuring the results are consistent * s/machine/machine_class/, after rebase to latest qemu.git (commit 6b342cc) --- vl.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)