7:40 a.m.
On Mon, Jan 18, 2016 at 18:06:22 -0500, John Ferlan wrote:
> > @@ -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.
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.
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