[Qemu-devel] [PATCH] vl.c: Check max_cpus limit instead of smp_cpus

[Qemu-devel] [PATCH] vl.c: Check max_cpus limit instead of smp_cpus

[Eduardo Habkost]

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);
     }
-- 
1.9.0

Re: [Qemu-devel] [PATCH] vl.c: Check max_cpus limit instead of smp_cpus

[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 and why do we not
just set max_cpus = machine->max_cpus ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] vl.c: Check max_cpus limit instead of smp_cpus

[Andreas Färber]

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] vl.c: Check max_cpus limit instead of smp_cpus

[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 ...

>> 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

Re: [Qemu-devel] [PATCH] vl.c: Check max_cpus limit instead of smp_cpus

[Eduardo Habkost]

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.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] vl.c: Check max_cpus limit instead of smp_cpus

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] vl.c: Check max_cpus limit instead of smp_cpus

[Andreas Färber]

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] vl.c: Check max_cpus limit instead of smp_cpus

[Eduardo Habkost]

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.

-- 
Eduardo