[libvirt] [PATCH] Implement CPU topology support for QEMU driver

[Daniel P. Berrange]

On Wed, Jan 13, 2010 at 09:47:00AM +0100, Jiri Denemark wrote:
> > >      ADD_ARG_LIT("-smp");
> > > -    ADD_ARG_LIT(vcpus);
> > > +    if ((qemuCmdFlags & QEMUD_CMD_FLAG_SMP_TOPOLOGY) && def->cpu) {
> > > +        virBuffer buf = VIR_BUFFER_INITIALIZER;
> > > +
> > > +        virBufferVSprintf(&buf, "%lu", def->vcpus);
> > > +
> > > +        if (def->cpu->sockets > 0)
> > > +            virBufferVSprintf(&buf, ",sockets=%u", def->cpu->sockets);
> > > +
> > > +        if (def->cpu->cores > 0)
> > > +            virBufferVSprintf(&buf, ",cores=%u", def->cpu->cores);
> > > +
> > > +        if (def->cpu->threads > 0)
> > > +            virBufferVSprintf(&buf, ",threads=%u", def->cpu->threads);
> > > +
> > > +        if (virBufferError(&buf)) {
> > > +            virBufferFreeAndReset(&buf);
> > > +            goto no_memory;
> > > +        }
> > > +
> > > +        ADD_ARG(virBufferContentAndReset(&buf));
> > > +    }
> > > +    else {
> > > +        char vcpus[50];
> > > +        snprintf(vcpus, sizeof(vcpus), "%lu", def->vcpus);
> > > +        ADD_ARG_LIT(vcpus);
> > > +    }
> > 
> > 
> > Can you split this piece of code out into a separate method with a
> > signature like
> > 
> >   static char *qemuBuildCPUStr(virDomainDefPtr def, int qemuCmdFlags);
> > 
> > The main qemuBuildCommandLine method is getting out of control :-)
> 
> With pleasure.
> 
> > Also, in the case where 'def->cpu' is NULL, but QEMUD_CMD_FLAG_SMP_TOPOLOGY
> > is available, I think we should explicitly set an arg based on
> > 
> >    sockets=def->vcpus
> >    cores=1
> >    threads=1
> > 
> > so that we avoid relying on any changable QEMU default values.
> 
> Makes sense. I was also considering a check for vcpus == sockets * cores *
> threads, but I'm not totally convinced it is a good idea.

You don't want todo that because it is fine to have a topology that is
not fully populated with CPUs

> 
> > > +    do {
> > > +        if (*p == '\0' || *p == ',')
> > > +            goto syntax;
> > > +
> > > +        if ((next = strchr(p, ',')))
> > > +            next++;
> > > +
> > > +        if (c_isdigit(*p)) {
> > > +            int n;
> > > +            if (virStrToLong_i(p, &end, 10, &n) < 0 ||
> > > +                !end || (*end != ',' && *end != '\0'))
> > > +                goto syntax;
> > > +            dom->vcpus = n;
> > > +        } else {
> > > +            int i;
> > > +            int n = -1;
> > > +            for (i = 0; i < ARRAY_CARDINALITY(options); i++) {
> > > +                if (STRPREFIX(p, options[i])) {
> > > +                    p += strlen(options[i]);
> > > +                    if (virStrToLong_i(p, &end, 10, &n) < 0 ||
> > > +                        !end || (*end != ',' && *end != '\0'))
> > > +                        goto syntax;
> > > +                    topology[i] = n;
> > > +                    break;
> > > +                }
> > > +            }
> > > +
> > > +            if (n < 0)
> > > +                goto syntax;
> > > +        }
> > > +    } while ((p = next));
> > 
> > If you strip off the initial  CPU count value, then you can use
> > 
> >    qemuParseCommandLineKeywords
> > 
> > to parse the reset of the  name=value  args.
> 
> Ah, cool, I didn't know about it. I guess because it's not used very often in
> the code. BTW, would it make sense to extend the function to (optionally)
> support parsing of any comma-separated list such as keyword,name=value,...?
> The "keyword" parts would result in NULL value in retvalues array and the
> "name=value" parts would behave normally.

Yes, there are a couple of places where that might be useful.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|