Re: [Qemu-devel] [PATCH v2 RESEND] vl.c: Unify MAX_CPUMASK_BITS and machine->max_cpus checks

[Eduardo Habkost]

On Fri, May 30, 2014 at 09:50:33AM +0200, Igor Mammedov wrote:
> On Wed, 14 May 2014 16:18:42 -0300
> Eduardo Habkost <address@hidden> 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 <address@hidden>
> > Cc: Peter Maydell <address@hidden>
> > Cc: Andreas Färber <address@hidden>
> > Cc: Igor Mammedov <address@hidden>
> > Cc: Marcelo Tosatti <address@hidden>
> > ---
> > 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!

-- 
Eduardo