| Message ID | 1399493987-14285-1-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 7 May 2014 21:19, 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. > > As smp_parse() already ensures smp_cpus <= max_cpus, we just need to > check if max_cpus exceeds the limit. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > vl.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/vl.c b/vl.c > index 236f95e..e317cc9 100644 > --- a/vl.c > +++ b/vl.c > @@ -4039,9 +4039,9 @@ int main(int argc, char **argv, char **envp) > smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL)); > > machine->max_cpus = machine->max_cpus ?: 1; /* Default to UP */ > - if (smp_cpus > machine->max_cpus) { > - fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus " > - "supported by machine `%s' (%d)\n", smp_cpus, machine->name, > + if (max_cpus > machine->max_cpus) { > + fprintf(stderr, "Total number of CPUs (%d), exceeds maximum " > + "supported by machine `%s' (%d)\n", max_cpus, machine->name, > machine->max_cpus); > exit(1); This is confusing. What is max_cpus and why do we not just set max_cpus = machine->max_cpus ? thanks -- PMM
Am 07.05.2014 23:48, schrieb Peter Maydell: > On 7 May 2014 21:19, 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. >> >> As smp_parse() already ensures smp_cpus <= max_cpus, we just need to >> check if max_cpus exceeds the limit. >> >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >> --- >> vl.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/vl.c b/vl.c >> index 236f95e..e317cc9 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -4039,9 +4039,9 @@ int main(int argc, char **argv, char **envp) >> smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL)); >> >> machine->max_cpus = machine->max_cpus ?: 1; /* Default to UP */ >> - if (smp_cpus > machine->max_cpus) { >> - fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus " >> - "supported by machine `%s' (%d)\n", smp_cpus, machine->name, >> + if (max_cpus > machine->max_cpus) { >> + fprintf(stderr, "Total number of CPUs (%d), exceeds maximum " >> + "supported by machine `%s' (%d)\n", max_cpus, machine->name, >> machine->max_cpus); >> exit(1); > > > This is confusing. What is max_cpus Specified via -smp, defaults to actual CPUs. > and why do we not > just set max_cpus = machine->max_cpus ? For the PC, machine->max_cpus specifies the technical (APIC/ACPI/...) upper limit, whereas -smp limits it to saner values. If you think KVM, then many machines today will have less than the maximum number of CPUs and you may want to run multiple VMs and you may not want their vCPUs competing too much for existing CPUs, so a limit makes sense. An interesting question for the 2.x era would be whether we may want to change the default from no hot-add to machine maximum - does recent libvirt specify max_cpus by default? Regards, Andreas
On 7 May 2014 23:20, Andreas Färber <afaerber@suse.de> wrote: > Am 07.05.2014 23:48, schrieb Peter Maydell: >> This is confusing. What is max_cpus > > Specified via -smp, defaults to actual CPUs. I thought that was smp_cpus ... >> and why do we not >> just set max_cpus = machine->max_cpus ? > > For the PC, machine->max_cpus specifies the technical (APIC/ACPI/...) > upper limit, whereas -smp limits it to saner values. > > If you think KVM, then many machines today will have less than the > maximum number of CPUs and you may want to run multiple VMs and you may > not want their vCPUs competing too much for existing CPUs, so a limit > makes sense. That would imply that we want max_cpus = MIN(command_line_max_cpus, machine->max_cpus); thanks -- PMM
On Thu, May 08, 2014 at 09:26:36AM +0100, Peter Maydell wrote: > On 7 May 2014 23:20, Andreas Färber <afaerber@suse.de> wrote: > > Am 07.05.2014 23:48, schrieb Peter Maydell: > >> This is confusing. What is max_cpus > > > > Specified via -smp, defaults to actual CPUs. > > I thought that was smp_cpus ... smp_cpus is the first argument to -smp. max_cpus is the "maxcpus" option of -smp. > > >> and why do we not > >> just set max_cpus = machine->max_cpus ? > > > > For the PC, machine->max_cpus specifies the technical (APIC/ACPI/...) > > upper limit, whereas -smp limits it to saner values. > > > > If you think KVM, then many machines today will have less than the > > maximum number of CPUs and you may want to run multiple VMs and you may > > not want their vCPUs competing too much for existing CPUs, so a limit > > makes sense. > > That would imply that we want > max_cpus = MIN(command_line_max_cpus, machine->max_cpus); Please don't do this. If the maxcpus value from the command-line is not supported, QEMU should abort instead of silently ignoring the value provided by the user.
On 8 May 2014 16:00, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, May 08, 2014 at 09:26:36AM +0100, Peter Maydell wrote: >> On 7 May 2014 23:20, Andreas Färber <afaerber@suse.de> wrote: >> > Am 07.05.2014 23:48, schrieb Peter Maydell: >> >> This is confusing. What is max_cpus >> > >> > Specified via -smp, defaults to actual CPUs. >> >> I thought that was smp_cpus ... > > smp_cpus is the first argument to -smp. max_cpus is the "maxcpus" option > of -smp. > Please don't do this. If the maxcpus value from the command-line is not > supported, QEMU should abort instead of silently ignoring the value > provided by the user. Ah, I see. This code is confusing because we've split the handling of user errors in the -smp argument between the smp_parse() function and this bit of inline code. Ideally we should put all the error handling in one place... thanks -- PMM
Am 08.05.2014 10:26, schrieb Peter Maydell: > On 7 May 2014 23:20, Andreas Färber <afaerber@suse.de> wrote: >> Am 07.05.2014 23:48, schrieb Peter Maydell: >>> This is confusing. What is max_cpus >> >> Specified via -smp, defaults to actual CPUs. > > I thought that was smp_cpus ... smp_cpus determines how many CPUs are cold-added in a loop in the machine init functions. max_cpus - smp_cpus is the amount of CPUs that may be hot-added later. Andreas
On Thu, May 08, 2014 at 04:07:54PM +0100, Peter Maydell wrote: > On 8 May 2014 16:00, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Thu, May 08, 2014 at 09:26:36AM +0100, Peter Maydell wrote: > >> On 7 May 2014 23:20, Andreas Färber <afaerber@suse.de> wrote: > >> > Am 07.05.2014 23:48, schrieb Peter Maydell: > >> >> This is confusing. What is max_cpus > >> > > >> > Specified via -smp, defaults to actual CPUs. > >> > >> I thought that was smp_cpus ... > > > > smp_cpus is the first argument to -smp. max_cpus is the "maxcpus" option > > of -smp. > > > Please don't do this. If the maxcpus value from the command-line is not > > supported, QEMU should abort instead of silently ignoring the value > > provided by the user. > > Ah, I see. This code is confusing because we've split the > handling of user errors in the -smp argument between > the smp_parse() function and this bit of inline code. > Ideally we should put all the error handling in one > place... The split makes sense to me: smp_parse() is just for basic parsing that doesn't depend on any state other than the provided QemuOpts parameter, and can run at any point in time. The machine->max_cpus check, on the other hand depend on the machine object to be created, and even changes the machine->max_cpus value. It is more dependent on ordering of initialization. To me, it makes sense to have it outside smp_parse(). But while looking at that code, I noticed that we can at least make the MAX_CPUMASK_BITS check and the machine->max_cpus check a single one. I will send a new patch.
diff --git a/vl.c b/vl.c index 236f95e..e317cc9 100644 --- a/vl.c +++ b/vl.c @@ -4039,9 +4039,9 @@ int main(int argc, char **argv, char **envp) smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL)); machine->max_cpus = machine->max_cpus ?: 1; /* Default to UP */ - if (smp_cpus > machine->max_cpus) { - fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus " - "supported by machine `%s' (%d)\n", smp_cpus, machine->name, + if (max_cpus > machine->max_cpus) { + fprintf(stderr, "Total number of CPUs (%d), exceeds maximum " + "supported by machine `%s' (%d)\n", max_cpus, machine->name, machine->max_cpus); exit(1); }
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. As smp_parse() already ensures smp_cpus <= max_cpus, we just need to check if max_cpus exceeds the limit. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- vl.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)