Re: [libvirt] [PATCH v5 05/10] conf: Adjust the iothreadsched expectations

On 04/27/2015 10:28 AM, Peter Krempa wrote: > On Fri, Apr 24, 2015 at 12:05:57 -0400, John Ferlan wrote: >> With iothreadid's allowing any 'id' value for an iothread_id, the >> iothreadsched code needs a slight adjustment to allow for "any" >> unsigned int value in order to create the bitmap of ids that will >> have scheduler adjustments. Adjusted the doc description as well. >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> docs/formatdomain.html.in | 16 ++++++++++++---- >> src/conf/domain_conf.c | 2 +- >> .../qemuxml2argv-cputune-iothreadsched-toomuch.xml | 3 ++- >> 3 files changed, 15 insertions(+), 6 deletions(-) >> >> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in >> index 7af6bd7..0767a2a 100644 >> --- a/docs/formatdomain.html.in >> +++ b/docs/formatdomain.html.in >> @@ -691,10 +691,18 @@ >> type (values <code>batch</code>, <code>idle</code>, <code>fifo</code>, >> <code>rr</code>) for particular vCPU/IOThread threads (based on >> <code>vcpus</code> and <code>iothreads</code>, leaving out >> - <code>vcpus</code>/<code>iothreads</code> sets the default). For >> - real-time schedulers (<code>fifo</code>, <code>rr</code>), priority must >> - be specified as well (and is ignored for non-real-time ones). The value >> - range for the priority depends on the host kernel (usually 1-99). >> + <code>vcpus</code>/<code>iothreads</code> sets the default). Valid >> + <code>vcpus</code> values start at 0 through one less than the >> + number of vCPU's defined for the domain. Valid <code>iothreads</code> >> + values are described in the <code>iothreadids</code> >> + <a href="#elementsIOThreadsAllocation"><code>description</code></a>. >> + If no <code>iothreadids</code> are defined, then libvirt numbers >> + IOThreads from 1 to the number of <code>iothreads</code> available >> + for the domain. For real-time schedulers (<code>fifo</code>, >> + <code>rr</code>), priority must real-time schedulers >> + (<code>fifo</code>, <code>rr</code>), priority must be specified as >> + well (and is ignored for non-real-time ones). The value range >> + for the priority depends on the host kernel (usually 1-99). >> <span class="since">Since 1.2.13</span> >> </dd> >> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >> index 129908d..9d4c916 100644 >> --- a/src/conf/domain_conf.c >> +++ b/src/conf/domain_conf.c >> @@ -14348,7 +14348,7 @@ virDomainDefParseXML(xmlDocPtr xml, >> >> for (i = 0; i < def->cputune.niothreadsched; i++) { >> if (virDomainThreadSchedParse(nodes[i], >> - 1, def->iothreads, >> + 1, UINT_MAX, >> "iothreads", >> &def->cputune.iothreadsched[i]) < 0) >> goto error; > > I think this patch should also add code that checks that the provided > scheduler info is provided only for valid iothread IDs. > Yuck... I know you eschew inline diffs, but it's just easier if nothing else just to make progress:

index b6a8129..7da94bb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14348,12 +14348,24 @@ virDomainDefParseXML(xmlDocPtr xml, def->cputune.niothreadsched = n; for (i = 0; i < def->cputune.niothreadsched; i++) { + ssize_t pos = -1; + if (virDomainThreadSchedParse(nodes[i], 1, UINT_MAX, "iothreads", &def->cputune.iothreadsched[i]) < 0) goto error; + while ((pos = virBitmapNextSetBit(def->cputune.iothreadsched[i].ids, + pos)) > -1) { + if (!virDomainIOThreadIDFind(def, pos)) { + virReportError(VIR_ERR_XML_DETAIL, "%s", + _("iothreadsched attribute 'iothreads' " + "uses undefined iothread ids")); + goto error;

+ } + } + for (j = 0; j < i; j++) { if (virBitmapOverlaps(def->cputune.iothreadsched[i].ids, def->cputune.iothreadsched[j].ids)) {