[libvirt] [PATCH 29/34] conf: Don't store vcpusched orthogonally to other vcpu info

[Peter Krempa]

On Mon, Jan 18, 2016 at 18:06:22 -0500, John Ferlan wrote:
> On 01/14/2016 11:27 AM, Peter Krempa wrote:

[...]

> > +virDomainDefGetVcpuSched(virDomainDefPtr def,
> > +                         unsigned int vcpu)
> > +{
> > +    virDomainVcpuInfoPtr vcpuinfo;
> > +
> > +    if (!(vcpuinfo = virDomainDefGetVcpu(def, vcpu)))
> > +        return NULL;
> 
> Does there need to be a vcpuinfo->online check?  (aka OnlineVcpuMap)

No. If you look carefully you'll notice that the scheduler can be
specified also for offline vCPUs and is applied when cpus are onlined.

I'm planing to do the same for the pinning bitmaps in part 3.

> 
> Will the caller need to differentiate?  I know this is the parsing
> code... just thinking while typing especially for non-static function.
> Later thoughts made me think this should be static for parse...

Yes it's not used anywhere else. I don't remember why I didn't make it
static.

> 


> > @@ -14592,6 +14601,55 @@ virDomainSchedulerParse(xmlNodePtr node,
> > 
> > 
> >  static int
> > +virDomainThreadSchedParseHelper(xmlNodePtr node,
> > +                                const char *name,
> > +                                virDomainThreadSchedParamPtr (*func)(virDomainDefPtr, unsigned int),
> > +                                virDomainDefPtr def)
> > +{
> > +    ssize_t next = -1;
> > +    virBitmapPtr map = NULL;
> > +    virDomainThreadSchedParamPtr sched;
> > +    virProcessSchedPolicy policy;
> > +    int priority;
> > +    int ret = -1;
> > +
> > +    if (!(map = virDomainSchedulerParse(node, name, &policy, &priority)))
> > +        goto cleanup;
> > +
> 
> Replacing the virBitmapOverlaps...
> 
> > +    while ((next = virBitmapNextSetBit(map, next)) > -1) {
> > +        if (!(sched = func(def, next)))
> > +            goto cleanup;
> 
> Could this be 'continue;' ?  That is, is the data required?  I'm
> thinking primarily of the vcpu->online == false case...

No, it's invalid to specify it for a non-existant object, but valid for
offline one.

> 
> > +
> > +        if (sched->policy != VIR_PROC_POLICY_NONE) {
> > +            virReportError(VIR_ERR_XML_DETAIL,
> > +                           _("%ssched attributes 'vcpus' must not overlap"),
> 
> Since the code will be shared in patch 30, change message to:
> 
> "%sched attribute '%s' must not overlap",

Indeed, I forgot to fix this.

> 
> using 'name' for both %s params. Similar to virDomainFormatSchedDef has
> done things.
> 
> > +                           name);
> > +            goto cleanup;

[...]


> > @@ -21504,6 +21543,128 @@ virDomainDefHasCapabilitiesFeatures(virDomainDefPtr def)
> > 
> > 
> 
> Probably a good place to note the arguments, but specifically that
> "name" is used to generate the XML "<vcpusched vcpus='..." or
> "<iothreadsched iothreads='..."
> 
> >  static int
> > +virDomainFormatSchedDef(virDomainDefPtr def,
> > +                        virBufferPtr buf,
> > +                        const char *name,
> > +                        virDomainThreadSchedParamPtr (*func)(virDomainDefPtr, unsigned int),
> > +                        virBitmapPtr resourceMap)
> > +

[...]

> > +            virBufferAddLit(buf, "/>\n");
> > +
> > +            /* subtract all vCPUs that were already found */
> > +            virBitmapSubtract(schedMap, currentMap);
> > +        }
> > +    }
> 
> This is one heckuva loop - I hope others can look as well because my
> eyes and brain decided to run in the other direction ;-)

Well the loop is complex because the original design is orthogonal. If
it was designed properly, it would be quite a lot simpler.

> 
> > +
> > +    ret = 0;
> > +
> > + cleanup:
> > +    virBitmapFree(schedMap);
> > +    virBitmapFree(prioMap);
> > +    return ret;
> > +}
> > +
> > +
> > +static int
> > +virDomainFormatVcpuSchedDef(virDomainDefPtr def,
> > +                            virBufferPtr buf)
> > +{
> > +    virBitmapPtr allcpumap;
> > +    int ret;
> > +
> > +    if (virDomainDefGetVcpusMax(def) == 0)
> > +        return 0;
> 
> Hmm... a zero for maxvcpus...  In patch 2 we disallow machines with 0
> vcpus online... Just a strange check.

Just in qemu. Some other hypervisors think 0 is the right way to
describe maximum host vcpus which would make this crash.

> 
> > +
> > +    if (!(allcpumap = virBitmapNew(virDomainDefGetVcpusMax(def))))
> 
> use of same function - although I assume the compiler will optimize that
> for you anyway...
> 
> > +        return -1;
> > +
> > +    virBitmapSetAll(allcpumap);
> > +
> > +    ret = virDomainFormatSchedDef(def, buf, "vcpu", virDomainDefGetVcpuSched,
> > +                                  allcpumap);
> 
> This differs slightly from Parse which uses "vcpus" - I'm wondering if

The parser historically used "vcpus" or "iothreads" I kept it there that
way but I don't like it so I made this as it should be.

> it should be consistent.  At the very least documented...

I'll probably change the parser code to drop the extra s which can be
constructed internally.

Peter

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160129/f995a7cf/attachment-0001.sig>