Re: [libvirt] [PATCH v2 2/3] docs, schema, conf: Add support for setting scheduler parameters of guest threads

On 10.02.2015 16:35, Martin Kletzander wrote: > In order for QEMU vCPU (and other) threads to run with RT scheduler, > libvirt needs to take care of that so QEMU doesn't have to run privileged. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1178986 > > Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; > --- > docs/formatdomain.html.in | 16 ++ > docs/schemas/domaincommon.rng | 44 +++++ > src/conf/domain_conf.c | 183 ++++++++++++++++++++- > src/conf/domain_conf.h | 24 +++ > .../qemuxml2argv-cputune-iothreadsched-toomuch.xml | 38 +++++ > .../qemuxml2argv-cputune-iothreadsched.xml | 39 +++++ > .../qemuxml2argv-cputune-vcpusched-overlap.xml | 38 +++++ > tests/qemuxml2argvtest.c | 2 + > .../qemuxml2xmlout-cputune-iothreadsched.xml | 39 +++++ > tests/qemuxml2xmltest.c | 1 + > 10 files changed, 423 insertions(+), 1 deletion(-) > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cputune-iothreadsched-toomuch.xml > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cputune-iothreadsched.xml > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cputune-vcpusched-overlap.xml > create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreadsched.xml > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index d8144ea..3381dfe 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -550,6 +550,8 @@ > &lt;quota&gt;-1&lt;/quota&gt; > &lt;emulator_period&gt;1000000&lt;/emulator_period&gt; > &lt;emulator_quota&gt;-1&lt;/emulator_quota&gt; > + &lt;vcpusched vcpus='0-4,^3' scheduler='fifo' priority='1'/&gt; > + &lt;iothreadsched iothreads='2' scheduler='batch'/&gt; > &lt;/cputune&gt; > ... > &lt;/domain&gt; > @@ -652,6 +654,20 @@ > <span class="since">Only QEMU driver support since 0.10.0</span> > </dd> > > + <dt><code>vcpusched</code> and <code>iothreadsched</code></dt> > + <dd> > + The optional <code>vcpusched</code> elements specifie the scheduler s/specifie/specifies/ And maybe s/scheduler/scheduler type/?

> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index d467dce..98766dc 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -815,10 +815,54 @@ > </attribute> > </element> > </zeroOrMore> > + <zeroOrMore> > + <element name="vcpusched"> > + <optional> > + <attribute name="vcpus"> > + <ref name='cpuset'/> > + </attribute> > + </optional> > + <ref name="schedparam"/> > + </element> > + </zeroOrMore> > + <zeroOrMore> > + <element name="iothreadsched"> > + <optional> > + <attribute name="iothreads"> > + <ref name='cpuset'/> > + </attribute> > + </optional> > + <ref name="schedparam"/> > + </element> > + </zeroOrMore> > </interleave> > </element> > </define> > > + <define name="schedparam"> > + <choice> > + <group> > + <attribute name="scheduler"> > + <choice> > + <value>batch</value> > + <value>idle</value> > + </choice> > + </attribute> > + </group> > + <group> > + <attribute name="scheduler"> > + <choice> > + <value>fifo</value> > + <value>rr</value> > + </choice> > + </attribute> > + <attribute name="priority"> > + <ref name="unsignedShort"/> > + </attribute> > + </group> > + </choice> > + </define> > + Have we returned to the rule where each new XML extension requires ACK from at least one of Daniels?

> @@ -13139,6 +13218,77 @@ virDomainDefParseXML(xmlDocPtr xml, > } > VIR_FREE(nodes); > > + if ((n = virXPathNodeSet("./cputune/vcpusched", ctxt, &nodes)) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("cannot extract vcpusched nodes")); > + goto error; > + } > + if (n) { > + if (n > def->maxvcpus) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("too many vcpusched nodes in cputune")); > + goto error; > + } > + > + if (VIR_ALLOC_N(def->cputune.vcpusched, n) < 0) > + goto error; > + def->cputune.nvcpusched = n; > + > + for (i = 0; i < def->cputune.nvcpusched; i++) { > + if (virDomainThreadSchedParse(nodes[i], > + 0, def->maxvcpus - 1, > + "vcpus", > + &def->cputune.vcpusched[i]) < 0) > + goto error; > + > + for (j = 0; j < i; j++) { > + if (virBitmapOverlaps(def->cputune.vcpusched[i].ids, > + def->cputune.vcpusched[j].ids)) { > + virReportError(VIR_ERR_XML_DETAIL, "%s", > + _("vcpusched attributes 'vcpus' " > + "must not overlap")); > + goto error; > + } > + } > + } > + } > + VIR_FREE(nodes); > + > + if ((n = virXPathNodeSet("./cputune/iothreadsched", ctxt, &nodes)) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("cannot extract iothreadsched nodes")); > + goto error; > + } > + if (n) { > + if (n > def->iothreads) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("too many iothreadsched nodes in cputune")); > + goto error; > + } > + > + if (VIR_ALLOC_N(def->cputune.iothreadsched, n) < 0) > + goto error; > + def->cputune.niothreadsched = n; > + > + for (i = 0; i < def->cputune.niothreadsched; i++) { > + if (virDomainThreadSchedParse(nodes[i], > + 1, def->iothreads, > + "iothreads", > + &def->cputune.iothreadsched[i]) < 0) > + goto error; > + > + for (j = 0; j < i; j++) { > + if (virBitmapOverlaps(def->cputune.iothreadsched[i].ids, > + def->cputune.iothreadsched[j].ids)) { > + virReportError(VIR_ERR_XML_DETAIL, "%s", > + _("iothreadsched attributes 'iothreads' " > + "must not overlap")); > + goto error; > + } > + } > + } > + } > + VIR_FREE(nodes); Once parsed, do we also need a check in virDomainDefCheckABIStability()? Or can the sched parameters change on migration? I guess they can, but seems like you dig more into this area than me recently.

> @@ -2844,6 +2867,7 @@ VIR_ENUM_DECL(virDomainRNGModel) > VIR_ENUM_DECL(virDomainRNGBackend) > VIR_ENUM_DECL(virDomainTPMModel) > VIR_ENUM_DECL(virDomainTPMBackend) > +VIR_ENUM_DECL(virDomainThreadSched) Yet another ENUM_DECL without corresponding libvirt_private.syms change.

> /* from libvirt.h */ > VIR_ENUM_DECL(virDomainState) > VIR_ENUM_DECL(virDomainNostateReason) ACK with all of that fixed. Michal