[libvirt] [PATCH v4 0/9] Implement Add/Del IOThreads
v3 here: http://www.redhat.com/archives/libvir-list/2015-April/msg00621.html Changes since v3 Patch 1: * Add back in and use virDomainIOThreadIDDefFree * Change _virDomainIOThreadIDDef 'undefined' to 'autofill' * Adjust virDomainDefParseXML to only allocate what's defined allowing virDomainDefPostParseInternal to Add in IOThreads which weren't defined and set the 'autofill' boolean * Have virDomainIOThreadIDAdd return a virDomainIOThreadIDDefPtr Patch 2: No changes Patch 3: (NEW) * Discussed in prior review regarding making virDomainPinIsDuplicate a static function (needed to move it) Patch 4: (NEW) * Move the iothreadspin data (e.g., the cpumask) into _virDomainIOThreadIDDef * Kept the 'niothreadspin' and manipulated as necessary since there was code to write out <cputune> data that I didn't want to reinvent/rototill just to search through iothreadids for a cpumask * Adjusted virDomainIOThreadPinDefParseXML to handle storing the cpumask in the right iothreadsid[] entry. If not found (it may not be since the virDomainDefPostParseInternal hasn't run), then create an autofill version of an iothreadids entry. * Remove/adjust a lot of code that used to handle iothreadspin Patch 5: (NEW) * Slight adjustment for iothreadsched to allow for "any" id value. This code stores iothread id's as bitmap entries, so it didn't have the same issues as the iothreadspin code Patch 6-7: Unchanged Patch 8: * Adjusted the search for the new thread code to use existing alias * Use the virDomainIOThreadIDAdd returned pointer * Comment adjustments from code review * Removal of erroneously cut-n-pasted code in Delete path * Use the ->dst for the message * Changes based on having cpumask in the iothrid data Patch 9: No changes John Ferlan (9): conf: Add new domain XML element 'iothreadids' qemu: Use domain iothreadids to IOThread's 'thread_id' conf: Move virDomainPinIsDuplicate and make static Move iothreadspin information into iothreadids conf: Adjust the iothreadsched expectations Implement virDomainAddIOThread and virDomainDelIOThread remote: Add support for AddIOThread and DelIOThread qemu: Add support to Add/Delete IOThreads virsh: Add iothreadadd and iothreaddel commands docs/formatdomain.html.in | 46 +- docs/schemas/domaincommon.rng | 12 + include/libvirt/libvirt-domain.h | 6 + src/conf/domain_audit.c | 9 + src/conf/domain_audit.h | 6 + src/conf/domain_conf.c | 340 ++++++++++---- src/conf/domain_conf.h | 25 +- src/driver-hypervisor.h | 12 + src/libvirt-domain.c | 118 +++++ src/libvirt_private.syms | 6 +- src/libvirt_public.syms | 6 + src/qemu/qemu_cgroup.c | 31 +- src/qemu/qemu_command.c | 38 +- src/qemu/qemu_command.h | 3 + src/qemu/qemu_domain.c | 36 -- src/qemu/qemu_domain.h | 3 - src/qemu/qemu_driver.c | 511 ++++++++++++++++++--- src/qemu/qemu_process.c | 40 +- src/remote/remote_driver.c | 2 + src/remote/remote_protocol.x | 30 +- src/remote_protocol-structs | 12 + .../qemuxml2argv-cputune-iothreadsched-toomuch.xml | 1 + .../qemuxml2argv-iothreads-ids-partial.args | 10 + .../qemuxml2argv-iothreads-ids-partial.xml | 33 ++ .../qemuxml2argv-iothreads-ids.args | 8 + .../qemuxml2argv-iothreads-ids.xml | 33 ++ tests/qemuxml2argvtest.c | 2 + tests/qemuxml2xmltest.c | 2 + tools/virsh-domain.c | 164 +++++++ tools/virsh.pod | 31 ++ 30 files changed, 1320 insertions(+), 256 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids-partial.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids-partial.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids.xml -- 2.1.0
Adding a new XML element 'iothreadids' in order to allow defining specific IOThread ID's rather than relying on the algorithm to assign IOThread ID's starting at 1 and incrementing to iothreads count. This will allow future patches to be able to add new IOThreads by a specific iothread_id and of course delete any exisiting IOThread. Each iothreadids element will have 'n' <iothread> children elements which will have attribute "id". The "id" will allow for definition of any "valid" (eg > 0) iothread_id value. On input, if any <iothreadids> <iothread>'s are provided, they will be marked so that we only print out what we read in. On input, if no <iothreadids> are provided, the PostParse code will self generate a list of ID's starting at 1 and going to the number of iothreads defined for the domain (just like the current algorithm numbering scheme). A future patch will rework the existing algorithm to make use of the iothreadids list. On output, only print out the <iothreadids> if they were read in. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 30 +++++++ docs/schemas/domaincommon.rng | 12 +++ src/conf/domain_conf.c | 200 +++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 18 ++++ src/libvirt_private.syms | 4 + 5 files changed, 262 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index e921749..518f7c5 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -521,6 +521,18 @@ ... </domain> </pre> +<pre> +<domain> + ... + <iothreadids> + <iothread id="2"/> + <iothread id="4"/> + <iothread id="6"/> + <iothread id="8"/> + </iothreadids> + ... +</domain> +</pre> <dl> <dt><code>iothreads</code></dt> @@ -530,7 +542,25 @@ virtio-blk-pci and virtio-blk-ccw target storage devices. There should be only 1 or 2 IOThreads per host CPU. There may be more than one supported device assigned to each IOThread. + <span class="since">Since 1.2.8</span> </dd> + <dt><code>iothreadids</code></dt> + <dd> + The optional <code>iothreadids</code> element provides the capability + to specifically define the IOThread ID's for the domain. By default, + IOThread ID's are sequentially numbered starting from 1 through the + number of <code>iothreads</code> defined for the domain. The + <code>id</code> attribute is used to define the IOThread ID. The + <code>id</code> attribute must be a positive integer greater than 0. + If there are less <code>iothreadids</code> defined than + <code>iothreads</code> defined for the domain, then libvirt will + sequentially fill <code>iothreadids</code> starting at 1 avoiding + any predefined <code>id</code>. If there are more + <code>iothreadids</code> defined than <code>iothreads</code> + defined for the domain, then the <code>iothreads</code> value + will be adjusted accordingly. + <span class="since">Since 1.2.15</span> + </dd> </dl> <h3><a name="elementsCPUTuning">CPU Tuning</a></h3> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 19461f5..7072954 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -539,6 +539,18 @@ </optional> <optional> + <element name="iothreadids"> + <zeroOrMore> + <element name="iothread"> + <attribute name="id"> + <ref name="unsignedInt"/> + </attribute> + </element> + </zeroOrMore> + </element> + </optional> + + <optional> <ref name="blkiotune"/> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 479b4c2..da1bb7e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2114,6 +2114,31 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int npin) return NULL; } + +void +virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def) +{ + if (def) + VIR_FREE(def); +} + + +static void +virDomainIOThreadIDDefArrayFree(virDomainIOThreadIDDefPtr *def, + int nids) +{ + size_t i; + + if (!def) + return; + + for (i = 0; i < nids; i++) + virDomainIOThreadIDDefFree(def[i]); + + VIR_FREE(def); +} + + void virDomainPinDefFree(virDomainPinDefPtr def) { @@ -2310,6 +2335,8 @@ void virDomainDefFree(virDomainDefPtr def) virCPUDefFree(def->cpu); + virDomainIOThreadIDDefArrayFree(def->iothreadids, def->niothreadids); + virDomainPinDefArrayFree(def->cputune.vcpupin, def->cputune.nvcpupin); virDomainPinDefFree(def->cputune.emulatorpin); @@ -3298,6 +3325,21 @@ virDomainDefPostParseInternal(virDomainDefPtr def, return -1; } + /* Fully populate the IOThread ID list */ + if (def->iothreads && def->iothreads != def->niothreadids) { + unsigned int iothread_id = 1; + while (def->niothreadids != def->iothreads) { + if (!virDomainIOThreadIDFind(def, iothread_id)) { + virDomainIOThreadIDDefPtr iothrid; + + if (!(iothrid = virDomainIOThreadIDAdd(def, iothread_id))) + return -1; + iothrid->autofill = true; + } + iothread_id++; + } + } + if (virDomainDefGetMemoryInitial(def) == 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Memory size must be specified via <memory> or in the " @@ -13148,6 +13190,54 @@ virDomainIdmapDefParseXML(xmlXPathContextPtr ctxt, return idmap; } +/* Parse the XML definition for an IOThread ID + * + * Format is : + * + * <iothreads>4</iothreads> + * <iothreadids> + * <iothread id='1'/> + * <iothread id='3'/> + * <iothread id='5'/> + * <iothread id='7'/> + * </iothreadids> + */ +static virDomainIOThreadIDDefPtr +virDomainIOThreadIDDefParseXML(xmlNodePtr node, + xmlXPathContextPtr ctxt) +{ + virDomainIOThreadIDDefPtr iothrid; + xmlNodePtr oldnode = ctxt->node; + char *tmp = NULL; + + if (VIR_ALLOC(iothrid) < 0) + return NULL; + + ctxt->node = node; + + if (!(tmp = virXPathString("string(./@id)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing 'id' attribute in <iothread> element")); + goto error; + } + if (virStrToLong_uip(tmp, NULL, 10, &iothrid->iothread_id) < 0 || + iothrid->iothread_id == 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid iothread 'id' value '%s'"), tmp); + goto error; + } + + cleanup: + VIR_FREE(tmp); + ctxt->node = oldnode; + return iothrid; + + error: + virDomainIOThreadIDDefFree(iothrid); + goto cleanup; +} + + /* Parse the XML definition for a vcpupin * * vcpupin has the form of @@ -13954,6 +14044,32 @@ virDomainDefParseXML(xmlDocPtr xml, } VIR_FREE(tmp); + /* Extract any iothread id's defined */ + if ((n = virXPathNodeSet("./iothreadids/iothread", ctxt, &nodes)) < 0) + goto error; + + if (n > def->iothreads) + def->iothreads = n; + + if (n && VIR_ALLOC_N(def->iothreadids, n) < 0) + goto error; + + for (i = 0; i < n; i++) { + virDomainIOThreadIDDefPtr iothrid = NULL; + if (!(iothrid = virDomainIOThreadIDDefParseXML(nodes[i], ctxt))) + goto error; + + if (virDomainIOThreadIDFind(def, iothrid->iothread_id)) { + virReportError(VIR_ERR_XML_ERROR, + _("duplicate iothread id '%u' found"), + iothrid->iothread_id); + virDomainIOThreadIDDefFree(iothrid); + goto error; + } + def->iothreadids[def->niothreadids++] = iothrid; + } + VIR_FREE(nodes); + /* Extract cpu tunables. */ if ((n = virXPathULong("string(./cputune/shares[1])", ctxt, &def->cputune.shares)) < -1) { @@ -17238,6 +17354,67 @@ virDomainDefAddImplicitControllers(virDomainDefPtr def) return 0; } +virDomainIOThreadIDDefPtr +virDomainIOThreadIDFind(virDomainDefPtr def, + unsigned int iothread_id) +{ + size_t i; + + if (!def->iothreadids || !def->niothreadids) + return NULL; + + for (i = 0; i < def->niothreadids; i++) { + if (iothread_id == def->iothreadids[i]->iothread_id) + return def->iothreadids[i]; + } + + return NULL; +} + +virDomainIOThreadIDDefPtr +virDomainIOThreadIDAdd(virDomainDefPtr def, + unsigned int iothread_id) +{ + virDomainIOThreadIDDefPtr iothrid = NULL; + + if (virDomainIOThreadIDFind(def, iothread_id)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot duplicate iothread_id '%u' in iothreadids"), + iothread_id); + return NULL; + } + + if (VIR_ALLOC(iothrid) < 0) + goto error; + + iothrid->iothread_id = iothread_id; + + if (VIR_APPEND_ELEMENT_COPY(def->iothreadids, def->niothreadids, + iothrid) < 0) + goto error; + + return iothrid; + + error: + virDomainIOThreadIDDefFree(iothrid); + return NULL; +} + +void +virDomainIOThreadIDDel(virDomainDefPtr def, + unsigned int iothread_id) +{ + int n; + + for (n = 0; n < def->niothreadids; n++) { + if (def->iothreadids[n]->iothread_id == iothread_id) { + virDomainIOThreadIDDefFree(def->iothreadids[n]); + VIR_DELETE_ELEMENT(def->iothreadids, n, def->niothreadids); + return; + } + } +} + /* Check if vcpupin with same id already exists. */ bool virDomainPinIsDuplicate(virDomainPinDefPtr *def, @@ -20579,8 +20756,27 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAsprintf(buf, " current='%u'", def->vcpus); virBufferAsprintf(buf, ">%u</vcpu>\n", def->maxvcpus); - if (def->iothreads > 0) - virBufferAsprintf(buf, "<iothreads>%u</iothreads>\n", def->iothreads); + if (def->iothreads > 0) { + virBufferAsprintf(buf, "<iothreads>%u</iothreads>\n", + def->iothreads); + /* Only print out iothreadids if we read at least one */ + for (i = 0; i < def->niothreadids; i++) { + if (!def->iothreadids[i]->autofill) + break; + } + if (i < def->niothreadids) { + virBufferAddLit(buf, "<iothreadids>\n"); + virBufferAdjustIndent(buf, 2); + for (i = 0; i < def->niothreadids; i++) { + if (def->iothreadids[i]->autofill) + continue; + virBufferAsprintf(buf, "<iothread id='%u'/>\n", + def->iothreadids[i]->iothread_id); + } + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</iothreadids>\n"); + } + } if (def->cputune.sharesSpecified || (def->cputune.nvcpupin && !virDomainIsAllVcpupinInherited(def)) || diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 25d3ee6..d4229eb 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2054,6 +2054,16 @@ struct _virDomainHugePage { unsigned long long size; /* hugepage size in KiB */ }; +typedef struct _virDomainIOThreadIDDef virDomainIOThreadIDDef; +typedef virDomainIOThreadIDDef *virDomainIOThreadIDDefPtr; + +struct _virDomainIOThreadIDDef { + bool autofill; + unsigned int iothread_id; +}; + +void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def); + typedef struct _virDomainCputune virDomainCputune; typedef virDomainCputune *virDomainCputunePtr; @@ -2145,6 +2155,8 @@ struct _virDomainDef { virBitmapPtr cpumask; unsigned int iothreads; + size_t niothreadids; + virDomainIOThreadIDDefPtr *iothreadids; virDomainCputune cputune; @@ -2602,6 +2614,12 @@ bool virDomainDefCheckABIStability(virDomainDefPtr src, int virDomainDefAddImplicitControllers(virDomainDefPtr def); +virDomainIOThreadIDDefPtr virDomainIOThreadIDFind(virDomainDefPtr def, + unsigned int iothread_id); +virDomainIOThreadIDDefPtr virDomainIOThreadIDAdd(virDomainDefPtr def, + unsigned int iothread_id); +void virDomainIOThreadIDDel(virDomainDefPtr def, unsigned int iothread_id); + unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags); char *virDomainDefFormat(virDomainDefPtr def, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8c50ea2..77f6797 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -324,6 +324,10 @@ virDomainHubTypeToString; virDomainHypervTypeFromString; virDomainHypervTypeToString; virDomainInputDefFree; +virDomainIOThreadIDAdd; +virDomainIOThreadIDDefFree; +virDomainIOThreadIDDel; +virDomainIOThreadIDFind; virDomainLeaseDefFree; virDomainLeaseIndex; virDomainLeaseInsert; -- 2.1.0
On Tue, Apr 21, 2015 at 19:31:22 -0400, John Ferlan wrote:
Adding a new XML element 'iothreadids' in order to allow defining specific IOThread ID's rather than relying on the algorithm to assign IOThread ID's starting at 1 and incrementing to iothreads count.
This will allow future patches to be able to add new IOThreads by a specific iothread_id and of course delete any exisiting IOThread.
Each iothreadids element will have 'n' <iothread> children elements which will have attribute "id". The "id" will allow for definition of any "valid" (eg > 0) iothread_id value.
On input, if any <iothreadids> <iothread>'s are provided, they will be marked so that we only print out what we read in.
On input, if no <iothreadids> are provided, the PostParse code will self generate a list of ID's starting at 1 and going to the number of iothreads defined for the domain (just like the current algorithm numbering scheme). A future patch will rework the existing algorithm to make use of the iothreadids list.
On output, only print out the <iothreadids> if they were read in.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 30 +++++++ docs/schemas/domaincommon.rng | 12 +++ src/conf/domain_conf.c | 200 +++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 18 ++++ src/libvirt_private.syms | 4 + 5 files changed, 262 insertions(+), 2 deletions(-)
...
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 479b4c2..da1bb7e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2114,6 +2114,31 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int npin) return NULL; }
+ +void +virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def) +{ + if (def) + VIR_FREE(def);
This breaks syntax check.
+} + + +static void +virDomainIOThreadIDDefArrayFree(virDomainIOThreadIDDefPtr *def, + int nids) +{ + size_t i; + + if (!def) + return; + + for (i = 0; i < nids; i++) + virDomainIOThreadIDDefFree(def[i]); + + VIR_FREE(def); +} + + void virDomainPinDefFree(virDomainPinDefPtr def) {
....
@@ -3298,6 +3325,21 @@ virDomainDefPostParseInternal(virDomainDefPtr def, return -1; }
+ /* Fully populate the IOThread ID list */ + if (def->iothreads && def->iothreads != def->niothreadids) { + unsigned int iothread_id = 1; + while (def->niothreadids != def->iothreads) { + if (!virDomainIOThreadIDFind(def, iothread_id)) { + virDomainIOThreadIDDefPtr iothrid; + + if (!(iothrid = virDomainIOThreadIDAdd(def, iothread_id))) + return -1;
Unfortunately, fixing the iothread list after you parse iothread pinning in patch 4 makes a loophole where you might force arbitrary iothread ID without using <iothreadids>. This code will probably need to be moved after the parsing code, despite the fact that the postparse callback is better place to do such checks.
+ iothrid->autofill = true; + } + iothread_id++; + } + } + if (virDomainDefGetMemoryInitial(def) == 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Memory size must be specified via <memory> or in the "
The rest of this patch looks good, but I'd like to see the above part fixed before my final ACK. Peter
...
....
@@ -3298,6 +3325,21 @@ virDomainDefPostParseInternal(virDomainDefPtr def, return -1; }
+ /* Fully populate the IOThread ID list */ + if (def->iothreads && def->iothreads != def->niothreadids) { + unsigned int iothread_id = 1; + while (def->niothreadids != def->iothreads) { + if (!virDomainIOThreadIDFind(def, iothread_id)) { + virDomainIOThreadIDDefPtr iothrid; + + if (!(iothrid = virDomainIOThreadIDAdd(def, iothread_id))) + return -1;
Unfortunately, fixing the iothread list after you parse iothread pinning in patch 4 makes a loophole where you might force arbitrary iothread ID without using <iothreadids>.
This code will probably need to be moved after the parsing code, despite the fact that the postparse callback is better place to do such checks.
+ iothrid->autofill = true; + } + iothread_id++; + } + } + if (virDomainDefGetMemoryInitial(def) == 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Memory size must be specified via <memory> or in the "
The rest of this patch looks good, but I'd like to see the above part fixed before my final ACK.
I'll move it and post v5 for 1/9 here as well as 4/9 I haven't yet got down to 8/9 I'd like to get this in before code freeze... Tks - John
Add 'thread_id' to the virDomainIOThreadIDDef as a means to store the 'thread_id' as returned from the live qemu monitor data. Remove the iothreadpids list from _qemuDomainObjPrivate and replace with the new iothreadids 'thread_id' element. Rather than use the default numbering scheme of 1..number of iothreads defined for the domain, use the iothreadid's list for the iothread_id Since iothreadids list keeps track of the iothread_id's, these are now used in place of the many places where a for loop would "know" that the ID was "+ 1" from the array element. The new tests ensure usage of the <iothreadid> values for an exact number of iothreads and the usage of a smaller number of <iothreadid> values than iothreads that exist (and usage of the default numbering scheme). Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.h | 1 + src/qemu/qemu_cgroup.c | 22 ++++++------- src/qemu/qemu_command.c | 38 +++++++++++++++++----- src/qemu/qemu_command.h | 3 ++ src/qemu/qemu_domain.c | 36 -------------------- src/qemu/qemu_domain.h | 3 -- src/qemu/qemu_driver.c | 35 +++++++++----------- src/qemu/qemu_process.c | 37 ++++++++++----------- .../qemuxml2argv-iothreads-ids-partial.args | 10 ++++++ .../qemuxml2argv-iothreads-ids-partial.xml | 33 +++++++++++++++++++ .../qemuxml2argv-iothreads-ids.args | 8 +++++ .../qemuxml2argv-iothreads-ids.xml | 33 +++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ tests/qemuxml2xmltest.c | 2 ++ 14 files changed, 164 insertions(+), 99 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids-partial.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids-partial.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids.xml diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index d4229eb..2d53a8d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2060,6 +2060,7 @@ typedef virDomainIOThreadIDDef *virDomainIOThreadIDDefPtr; struct _virDomainIOThreadIDDef { bool autofill; unsigned int iothread_id; + int thread_id; }; void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def); diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index e83342d..cdf0aaf 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -802,8 +802,9 @@ qemuRestoreCgroupState(virDomainObjPtr vm) virCgroupFree(&cgroup_temp); } - for (i = 0; i < priv->niothreadpids; i++) { - if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_IOTHREAD, i + 1, + for (i = 0; i < vm->def->niothreadids; i++) { + if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_IOTHREAD, + vm->def->iothreadids[i]->iothread_id, false, &cgroup_temp) < 0 || virCgroupSetCpusetMemoryMigrate(cgroup_temp, true) < 0 || virCgroupGetCpusetMems(cgroup_temp, &nodeset) < 0 || @@ -1175,11 +1176,6 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) if (priv->cgroup == NULL) return 0; - if (def->iothreads && priv->niothreadpids == 0) { - VIR_WARN("Unable to get iothreads' pids."); - return 0; - } - if (virDomainNumatuneGetMode(vm->def->numa, -1) == VIR_DOMAIN_NUMATUNE_MEM_STRICT && virDomainNumatuneMaybeFormatNodeset(vm->def->numa, @@ -1187,16 +1183,18 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) &mem_mask, -1) < 0) goto cleanup; - for (i = 0; i < priv->niothreadpids; i++) { + for (i = 0; i < def->niothreadids; i++) { /* IOThreads are numbered 1..n, although the array is 0..n-1, * so we will account for that here */ - if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_IOTHREAD, i + 1, + if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_IOTHREAD, + def->iothreadids[i]->iothread_id, true, &cgroup_iothread) < 0) goto cleanup; /* move the thread for iothread to sub dir */ - if (virCgroupAddTask(cgroup_iothread, priv->iothreadpids[i]) < 0) + if (virCgroupAddTask(cgroup_iothread, + def->iothreadids[i]->thread_id) < 0) goto cleanup; if (period || quota) { @@ -1221,8 +1219,8 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) /* specific cpu mask */ for (j = 0; j < def->cputune.niothreadspin; j++) { - /* IOThreads are numbered/named 1..n */ - if (def->cputune.iothreadspin[j]->id == i + 1) { + if (def->cputune.iothreadspin[j]->id == + def->iothreadids[i]->iothread_id) { cpumask = def->cputune.iothreadspin[j]->cpumask; break; } diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 29b876e..cc96d5b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -678,6 +678,24 @@ qemuOpenVhostNet(virDomainDefPtr def, } int +qemuDomainParseIOThreadAlias(char *alias, + unsigned int *iothread_id) +{ + unsigned int idval; + + if (virStrToLong_ui(alias + strlen("iothread"), + NULL, 10, &idval) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to find iothread id for '%s'"), + alias); + return -1; + } + + *iothread_id = idval; + return 0; +} + +int qemuNetworkPrepareDevices(virDomainDefPtr def) { int ret = -1; @@ -3985,11 +4003,11 @@ qemuCheckIOThreads(virDomainDefPtr def, return false; } - /* Value larger than iothreads available? */ - if (disk->iothread > def->iothreads) { + /* Can we find the disk iothread in the iothreadid list? */ + if (!virDomainIOThreadIDFind(def, disk->iothread)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Disk iothread '%u' invalid only %u IOThreads"), - disk->iothread, def->iothreads); + _("Disk iothread '%u' not defined in iothreadid"), + disk->iothread); return false; } @@ -4209,6 +4227,7 @@ qemuBuildDriveDevStr(virDomainDefPtr def, if (disk->iothread) virBufferAsprintf(&opt, ",iothread=iothread%u", disk->iothread); } + qemuBuildIoEventFdStr(&opt, disk->ioeventfd, qemuCaps); if (disk->event_idx && virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_EVENT_IDX)) { @@ -8794,14 +8813,15 @@ qemuBuildCommandLine(virConnectPtr conn, if (def->iothreads > 0 && virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { - /* Create named iothread objects starting with 1. These may be used + /* Create iothread objects using the defined iothreadids list + * and the defined id and name from the list. These may be used * by a disk definition which will associate to an iothread by - * supplying a value of 1 up to the number of iothreads available - * (since 0 would indicate to not use the feature). + * supplying a value of an id from the list */ - for (i = 1; i <= def->iothreads; i++) { + for (i = 0; i < def->niothreadids; i++) { virCommandAddArg(cmd, "-object"); - virCommandAddArgFormat(cmd, "iothread,id=iothread%zu", i); + virCommandAddArgFormat(cmd, "iothread,id=iothread%u", + def->iothreadids[i]->iothread_id); } } diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index a29db41..538ccdf 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -238,6 +238,9 @@ int qemuOpenVhostNet(virDomainDefPtr def, int *vhostfd, size_t *vhostfdSize); +int qemuDomainParseIOThreadAlias(char *alias, + unsigned int *iothread_id); + int qemuNetworkPrepareDevices(virDomainDefPtr def); /* diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1368386..ce5d1d2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -440,7 +440,6 @@ qemuDomainObjPrivateFree(void *data) virDomainChrSourceDefFree(priv->monConfig); qemuDomainObjFreeJob(priv); VIR_FREE(priv->vcpupids); - VIR_FREE(priv->iothreadpids); VIR_FREE(priv->lockState); VIR_FREE(priv->origname); @@ -501,18 +500,6 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf, void *data) virBufferAddLit(buf, "</vcpus>\n"); } - if (priv->niothreadpids) { - size_t i; - virBufferAddLit(buf, "<iothreads>\n"); - virBufferAdjustIndent(buf, 2); - for (i = 0; i < priv->niothreadpids; i++) { - virBufferAsprintf(buf, "<iothread pid='%d'/>\n", - priv->iothreadpids[i]); - } - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</iothreads>\n"); - } - if (priv->qemuCaps) { size_t i; virBufferAddLit(buf, "<qemuCaps>\n"); @@ -633,29 +620,6 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, void *data) VIR_FREE(nodes); } - n = virXPathNodeSet("./iothreads/iothread", ctxt, &nodes); - if (n < 0) - goto error; - if (n) { - priv->niothreadpids = n; - if (VIR_REALLOC_N(priv->iothreadpids, priv->niothreadpids) < 0) - goto error; - - for (i = 0; i < n; i++) { - char *pidstr = virXMLPropString(nodes[i], "pid"); - if (!pidstr) - goto error; - - if (virStrToLong_i(pidstr, NULL, 10, - &(priv->iothreadpids[i])) < 0) { - VIR_FREE(pidstr); - goto error; - } - VIR_FREE(pidstr); - } - VIR_FREE(nodes); - } - if ((n = virXPathNodeSet("./qemuCaps/flag", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse qemu capabilities flags")); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 6bea7c7..1b80848 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -152,9 +152,6 @@ struct _qemuDomainObjPrivate { int nvcpupids; int *vcpupids; - int niothreadpids; - int *iothreadpids; - virDomainPCIAddressSetPtr pciaddrs; virDomainCCWAddressSetPtr ccwaddrs; virDomainVirtioSerialAddrSetPtr vioserialaddrs; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6fc9696..0f95cc7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5829,14 +5829,16 @@ qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver, goto endjob; for (i = 0; i < niothreads; i++) { + unsigned int iothread_id; virBitmapPtr map = NULL; - if (VIR_ALLOC(info_ret[i]) < 0) + if (qemuDomainParseIOThreadAlias(iothreads[i]->name, + &iothread_id) < 0) goto endjob; - if (virStrToLong_ui(iothreads[i]->name + strlen("iothread"), NULL, 10, - &info_ret[i]->iothread_id) < 0) + if (VIR_ALLOC(info_ret[i]) < 0) goto endjob; + info_ret[i]->iothread_id = iothread_id; if (virProcessGetAffinity(iothreads[i]->thread_id, &map, hostcpus) < 0) goto endjob; @@ -5891,19 +5893,19 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef, if (VIR_ALLOC_N(info_ret, targetDef->iothreads) < 0) goto cleanup; - for (i = 0; i < targetDef->iothreads; i++) { + for (i = 0; i < targetDef->niothreadids; i++) { virDomainPinDefPtr pininfo; if (VIR_ALLOC(info_ret[i]) < 0) goto cleanup; - /* IOThreads being counting at 1 */ - info_ret[i]->iothread_id = i + 1; + /* IOThread ID's are taken from the iothreadids list */ + info_ret[i]->iothread_id = targetDef->iothreadids[i]->iothread_id; /* Initialize the cpumap */ pininfo = virDomainPinFind(targetDef->cputune.iothreadspin, targetDef->cputune.niothreadspin, - i + 1); + targetDef->iothreadids[i]->iothread_id); if (!pininfo) { if (targetDef->cpumask) { cpumask = targetDef->cpumask; @@ -6047,16 +6049,11 @@ qemuDomainPinIOThread(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (priv->iothreadpids == NULL) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("IOThread affinity is not supported")); - goto endjob; - } + virDomainIOThreadIDDefPtr iothrid; - if (iothread_id > priv->niothreadpids) { + if (!(iothrid = virDomainIOThreadIDFind(vm->def, iothread_id))) { virReportError(VIR_ERR_INVALID_ARG, - _("iothread value out of range %d > %d"), - iothread_id, priv->niothreadpids); + _("iothread value %d not found"), iothread_id); goto endjob; } @@ -6094,8 +6091,7 @@ qemuDomainPinIOThread(virDomainPtr dom, goto endjob; } } else { - if (virProcessSetAffinity(priv->iothreadpids[iothread_id - 1], - pcpumap) < 0) { + if (virProcessSetAffinity(iothrid->thread_id, pcpumap) < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, _("failed to set cpu affinity for IOThread %d"), iothread_id); @@ -10066,8 +10062,9 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm, virCgroupFree(&cgroup_temp); } - for (i = 0; i < priv->niothreadpids; i++) { - if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_IOTHREAD, i + 1, + for (i = 0; i < vm->def->niothreadids; i++) { + if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_IOTHREAD, + vm->def->iothreadids[i]->iothread_id, false, &cgroup_temp) < 0 || virCgroupSetCpusetMems(cgroup_temp, nodeset_str) < 0) goto cleanup; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 276837e..717d113 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2267,12 +2267,16 @@ qemuProcessDetectIOThreadPIDs(virQEMUDriverPtr driver, goto cleanup; } - if (VIR_ALLOC_N(priv->iothreadpids, niothreads) < 0) - goto cleanup; - priv->niothreadpids = niothreads; + for (i = 0; i < vm->def->niothreadids; i++) { + unsigned int iothread_id; - for (i = 0; i < priv->niothreadpids; i++) - priv->iothreadpids[i] = iothreads[i]->thread_id; + if (qemuDomainParseIOThreadAlias(iothreads[i]->name, + &iothread_id) < 0) + goto cleanup; + + vm->def->iothreadids[i]->thread_id = iothreads[i]->thread_id; + vm->def->iothreadids[i]->iothread_id = iothread_id; + } ret = 0; @@ -2453,7 +2457,6 @@ qemuProcessSetEmulatorAffinity(virDomainObjPtr vm) static int qemuProcessSetIOThreadsAffinity(virDomainObjPtr vm) { - qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr def = vm->def; virDomainPinDefPtr pininfo; size_t i; @@ -2462,20 +2465,15 @@ qemuProcessSetIOThreadsAffinity(virDomainObjPtr vm) if (!def->cputune.niothreadspin) return 0; - if (priv->iothreadpids == NULL) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("IOThread affinity is not supported")); - return -1; - } - - for (i = 0; i < def->iothreads; i++) { - /* set affinity only for existing vcpus */ + for (i = 0; i < def->niothreadids; i++) { + /* set affinity only for existing iothreads */ if (!(pininfo = virDomainPinFind(def->cputune.iothreadspin, def->cputune.niothreadspin, - i + 1))) + def->iothreadids[i]->iothread_id))) continue; - if (virProcessSetAffinity(priv->iothreadpids[i], pininfo->cpumask) < 0) + if (virProcessSetAffinity(def->iothreadids[i]->thread_id, + pininfo->cpumask) < 0) goto cleanup; } ret = 0; @@ -2525,8 +2523,9 @@ qemuProcessSetSchedulers(virDomainObjPtr vm) return -1; } - for (i = 0; i < priv->niothreadpids; i++) { - if (qemuProcessSetSchedParams(i + 1, priv->iothreadpids[i], + for (i = 0; i < vm->def->niothreadids; i++) { + if (qemuProcessSetSchedParams(vm->def->iothreadids[i]->iothread_id, + vm->def->iothreadids[i]->thread_id, vm->def->cputune.niothreadsched, vm->def->cputune.iothreadsched) < 0) return -1; @@ -5314,8 +5313,6 @@ void qemuProcessStop(virQEMUDriverPtr driver, virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason); VIR_FREE(priv->vcpupids); priv->nvcpupids = 0; - VIR_FREE(priv->iothreadpids); - priv->niothreadpids = 0; virObjectUnref(priv->qemuCaps); priv->qemuCaps = NULL; VIR_FREE(priv->pidfile); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids-partial.args b/tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids-partial.args new file mode 100644 index 0000000..444cd17 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids-partial.args @@ -0,0 +1,10 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S -M \ +pc -m 214 -smp 2 \ +-object iothread,id=iothread5 \ +-object iothread,id=iothread6 \ +-object iothread,id=iothread1 \ +-object iothread,id=iothread2 \ +-nographic -monitor \ +unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -hda \ +/dev/HostVG/QEMUGuest1 -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids-partial.xml b/tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids-partial.xml new file mode 100644 index 0000000..c631677 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids-partial.xml @@ -0,0 +1,33 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>2</vcpu> + <iothreads>4</iothreads> + <iothreadids> + <iothread id='5'/> + <iothread id='6'/> + </iothreadids> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids.args b/tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids.args new file mode 100644 index 0000000..68998f6 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids.args @@ -0,0 +1,8 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S -M \ +pc -m 214 -smp 2 \ +-object iothread,id=iothread2 \ +-object iothread,id=iothread4 \ +-nographic -monitor \ +unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -hda \ +/dev/HostVG/QEMUGuest1 -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids.xml b/tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids.xml new file mode 100644 index 0000000..d70e74b --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids.xml @@ -0,0 +1,33 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>2</vcpu> + <iothreads>2</iothreads> + <iothreadids> + <iothread id='2'/> + <iothread id='4'/> + </iothreadids> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index acd6126..e51b24b 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1230,6 +1230,8 @@ mymain(void) DO_TEST("smp", QEMU_CAPS_SMP_TOPOLOGY); DO_TEST("iothreads", QEMU_CAPS_OBJECT_IOTHREAD); + DO_TEST("iothreads-ids", QEMU_CAPS_OBJECT_IOTHREAD); + DO_TEST("iothreads-ids-partial", QEMU_CAPS_OBJECT_IOTHREAD); DO_TEST("iothreads-disk", QEMU_CAPS_OBJECT_IOTHREAD, QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE); DO_TEST("iothreads-disk-virtio-ccw", QEMU_CAPS_OBJECT_IOTHREAD, QEMU_CAPS_DEVICE, diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index bce04b2..d418776 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -493,6 +493,8 @@ mymain(void) DO_TEST("smp"); DO_TEST("iothreads"); + DO_TEST("iothreads-ids"); + DO_TEST("iothreads-ids-partial"); DO_TEST_DIFFERENT("cputune-iothreads"); DO_TEST("iothreads-disk"); DO_TEST("iothreads-disk-virtio-ccw"); -- 2.1.0
Since it's only ever referenced in domain_conf.c, make the function static, but also will need to move it to somewhere before it's referenced rather than forward referencing it. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 38 +++++++++++++++++++------------------- src/conf/domain_conf.h | 4 ---- src/libvirt_private.syms | 1 - 3 files changed, 19 insertions(+), 24 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index da1bb7e..bd25d52 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13238,6 +13238,25 @@ virDomainIOThreadIDDefParseXML(xmlNodePtr node, } +/* Check if pin with same id already exists. */ +static bool +virDomainPinIsDuplicate(virDomainPinDefPtr *def, + int npin, + int id) +{ + size_t i; + + if (!def || !npin) + return false; + + for (i = 0; i < npin; i++) { + if (def[i]->id == id) + return true; + } + + return false; +} + /* Parse the XML definition for a vcpupin * * vcpupin has the form of @@ -17415,25 +17434,6 @@ virDomainIOThreadIDDel(virDomainDefPtr def, } } -/* Check if vcpupin with same id already exists. */ -bool -virDomainPinIsDuplicate(virDomainPinDefPtr *def, - int npin, - int id) -{ - size_t i; - - if (!def || !npin) - return false; - - for (i = 0; i < npin; i++) { - if (def[i]->id == id) - return true; - } - - return false; -} - virDomainPinDefPtr virDomainPinFind(virDomainPinDefPtr *def, int npin, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 2d53a8d..cc98f3d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1947,10 +1947,6 @@ void virDomainPinDefArrayFree(virDomainPinDefPtr *def, int npin); virDomainPinDefPtr *virDomainPinDefCopy(virDomainPinDefPtr *src, int npin); -bool virDomainPinIsDuplicate(virDomainPinDefPtr *def, - int npin, - int id); - virDomainPinDefPtr virDomainPinFind(virDomainPinDefPtr *def, int npin, int id); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 77f6797..a0f4152 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -413,7 +413,6 @@ virDomainPinDefCopy; virDomainPinDefFree; virDomainPinDel; virDomainPinFind; -virDomainPinIsDuplicate; virDomainPMSuspendedReasonTypeFromString; virDomainPMSuspendedReasonTypeToString; virDomainRedirdevBusTypeFromString; -- 2.1.0
On Tue, Apr 21, 2015 at 19:31:24 -0400, John Ferlan wrote:
Since it's only ever referenced in domain_conf.c, make the function static, but also will need to move it to somewhere before it's referenced rather than forward referencing it.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 38 +++++++++++++++++++------------------- src/conf/domain_conf.h | 4 ---- src/libvirt_private.syms | 1 - 3 files changed, 19 insertions(+), 24 deletions(-)
ACK, Peter
Remove the iothreadspin array from cputune and replace with a cpumask to be stored in the iothreadids list Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 10 ++-- src/conf/domain_conf.c | 118 +++++++++++++++++++++------------------------- src/conf/domain_conf.h | 2 +- src/qemu/qemu_cgroup.c | 13 ++--- src/qemu/qemu_driver.c | 79 +++++++++---------------------- src/qemu/qemu_process.c | 7 +-- 6 files changed, 86 insertions(+), 143 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 518f7c5..7af6bd7 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -624,11 +624,11 @@ and attribute <code>cpuset</code> of element <code>vcpu</code> is not specified, the IOThreads are pinned to all the physical CPUs by default. There are two required attributes, the attribute - <code>iothread</code> specifies the IOThread id and the attribute - <code>cpuset</code> specifying which physical CPUs to pin to. The - <code>iothread</code> value begins at "1" through the number of - <a href="#elementsIOThreadsAllocation"><code>iothreads</code></a> - allocated to the domain. A value of "0" is not permitted. + <code>iothread</code> specifies the IOThread ID and the attribute + <code>cpuset</code> specifying which physical CPUs to pin to. See + the <code>iothreadids</code> + <a href="#elementsIOThreadsAllocation"><code>description</code></a> + for valid <code>iothread</code> values. <span class="since">Since 1.2.9</span> </dd> <dt><code>shares</code></dt> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bd25d52..969e56f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2118,8 +2118,10 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int npin) void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def) { - if (def) + if (def) { + virBitmapFree(def->cpumask); VIR_FREE(def); + } } @@ -2341,9 +2343,6 @@ void virDomainDefFree(virDomainDefPtr def) virDomainPinDefFree(def->cputune.emulatorpin); - virDomainPinDefArrayFree(def->cputune.iothreadspin, - def->cputune.niothreadspin); - for (i = 0; i < def->cputune.nvcpusched; i++) virBitmapFree(def->cputune.vcpusched[i].ids); VIR_FREE(def->cputune.vcpusched); @@ -13316,74 +13315,77 @@ virDomainVcpuPinDefParseXML(xmlNodePtr node, * and an iothreadspin has the form * <iothreadpin iothread='1' cpuset='2'/> */ -static virDomainPinDefPtr +static int virDomainIOThreadPinDefParseXML(xmlNodePtr node, xmlXPathContextPtr ctxt, - int iothreads) + virDomainDefPtr def) { - virDomainPinDefPtr def; + int ret = -1; + virDomainIOThreadIDDefPtr iothrid; + virBitmapPtr cpumask; xmlNodePtr oldnode = ctxt->node; unsigned int iothreadid; char *tmp = NULL; - if (VIR_ALLOC(def) < 0) - return NULL; - ctxt->node = node; if (!(tmp = virXPathString("string(./@iothread)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing iothread id in iothreadpin")); - goto error; + goto cleanup; } if (virStrToLong_uip(tmp, NULL, 10, &iothreadid) < 0) { virReportError(VIR_ERR_XML_ERROR, _("invalid setting for iothread '%s'"), tmp); - goto error; + goto cleanup; } VIR_FREE(tmp); if (iothreadid == 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("zero is an invalid iothread id value")); - goto error; - } - - /* IOThreads are numbered "iothread1...iothread<n>", where - * "n" is the iothreads value */ - if (iothreadid > iothreads) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("iothread id must not exceed iothreads")); - goto error; + goto cleanup; } - def->id = iothreadid; - if (!(tmp = virXMLPropString(node, "cpuset"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing cpuset for iothreadpin")); - goto error; + goto cleanup; } - if (virBitmapParse(tmp, 0, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) - goto error; + if (virBitmapParse(tmp, 0, &cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) + goto cleanup; - if (virBitmapIsAllClear(def->cpumask)) { + if (virBitmapIsAllClear(cpumask)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid value of 'cpuset': %s"), tmp); - goto error; + goto cleanup; + } + + if (!(iothrid = virDomainIOThreadIDFind(def, iothreadid))) { + if (!(iothrid = virDomainIOThreadIDAdd(def, iothreadid))) + goto cleanup; + iothrid->autofill = true; + } + + if (iothrid->cpumask) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("duplicate iothreadpin for same iothread '%u'"), + iothreadid); + goto cleanup; } + iothrid->cpumask = cpumask; + cpumask = NULL; + ret = 0; + cleanup: VIR_FREE(tmp); + virBitmapFree(cpumask); ctxt->node = oldnode; - return def; - - error: - VIR_FREE(def); - goto cleanup; + return ret; } @@ -14250,28 +14252,11 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } - if (n && VIR_ALLOC_N(def->cputune.iothreadspin, n) < 0) - goto error; - for (i = 0; i < n; i++) { - virDomainPinDefPtr iothreadpin = NULL; - iothreadpin = virDomainIOThreadPinDefParseXML(nodes[i], ctxt, - def->iothreads); - if (!iothreadpin) + if (virDomainIOThreadPinDefParseXML(nodes[i], ctxt, def) < 0) goto error; - - if (virDomainPinIsDuplicate(def->cputune.iothreadspin, - def->cputune.niothreadspin, - iothreadpin->id)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("duplicate iothreadpin for same iothread")); - virDomainPinDefFree(iothreadpin); - goto error; - } - - def->cputune.iothreadspin[def->cputune.niothreadspin++] = - iothreadpin; } + def->cputune.niothreadspin = n; VIR_FREE(nodes); if ((n = virXPathNodeSet("./cputune/vcpusched", ctxt, &nodes)) < 0) { @@ -14384,7 +14369,7 @@ virDomainDefParseXML(xmlDocPtr xml, if (virDomainNumatuneHasPlacementAuto(def->numa) && !def->cpumask && !def->cputune.vcpupin && - !def->cputune.emulatorpin && !def->cputune.iothreadspin) + !def->cputune.emulatorpin && !def->cputune.niothreadspin) def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO; if ((n = virXPathNodeSet("./resource", ctxt, &nodes)) < 0) { @@ -20837,20 +20822,25 @@ virDomainDefFormatInternal(virDomainDefPtr def, VIR_FREE(cpumask); } - for (i = 0; i < def->cputune.niothreadspin; i++) { - char *cpumask; - /* Ignore the iothreadpin which inherit from "cpuset of "<vcpu>." */ - if (virBitmapEqual(def->cpumask, def->cputune.iothreadspin[i]->cpumask)) - continue; + if (def->cputune.niothreadspin) { + for (i = 0; i < def->niothreadids; i++) { + char *cpumask; - virBufferAsprintf(buf, "<iothreadpin iothread='%u' ", - def->cputune.iothreadspin[i]->id); + /* Ignore iothreadids with no cpumask or a cpumask that + * inherits from "cpuset of "<vcpu>." */ + if (!def->iothreadids[i]->cpumask || + virBitmapEqual(def->cpumask, def->iothreadids[i]->cpumask)) + continue; - if (!(cpumask = virBitmapFormat(def->cputune.iothreadspin[i]->cpumask))) - goto error; + virBufferAsprintf(buf, "<iothreadpin iothread='%u' ", + def->iothreadids[i]->iothread_id); - virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask); - VIR_FREE(cpumask); + if (!(cpumask = virBitmapFormat(def->iothreadids[i]->cpumask))) + goto error; + + virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask); + VIR_FREE(cpumask); + } } for (i = 0; i < def->cputune.nvcpusched; i++) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index cc98f3d..c71e1b8 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2057,6 +2057,7 @@ struct _virDomainIOThreadIDDef { bool autofill; unsigned int iothread_id; int thread_id; + virBitmapPtr cpumask; }; void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def); @@ -2075,7 +2076,6 @@ struct _virDomainCputune { virDomainPinDefPtr *vcpupin; virDomainPinDefPtr emulatorpin; size_t niothreadspin; - virDomainPinDefPtr *iothreadspin; size_t nvcpusched; virDomainThreadSchedParamPtr vcpusched; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index cdf0aaf..5282449 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1149,7 +1149,7 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) virCgroupPtr cgroup_iothread = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr def = vm->def; - size_t i, j; + size_t i; unsigned long long period = vm->def->cputune.period; long long quota = vm->def->cputune.quota; char *mem_mask = NULL; @@ -1214,18 +1214,11 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) /* default cpu masks */ if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) cpumask = priv->autoCpuset; + else if (def->iothreadids[i]->cpumask) + cpumask = def->iothreadids[i]->cpumask; else cpumask = def->cpumask; - /* specific cpu mask */ - for (j = 0; j < def->cputune.niothreadspin; j++) { - if (def->cputune.iothreadspin[j]->id == - def->iothreadids[i]->iothread_id) { - cpumask = def->cputune.iothreadspin[j]->cpumask; - break; - } - } - if (cpumask && qemuSetupCgroupCpusetCpus(cgroup_iothread, cpumask) < 0) goto cleanup; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0f95cc7..ee13d08 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5894,19 +5894,14 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef, goto cleanup; for (i = 0; i < targetDef->niothreadids; i++) { - virDomainPinDefPtr pininfo; - if (VIR_ALLOC(info_ret[i]) < 0) goto cleanup; /* IOThread ID's are taken from the iothreadids list */ info_ret[i]->iothread_id = targetDef->iothreadids[i]->iothread_id; - /* Initialize the cpumap */ - pininfo = virDomainPinFind(targetDef->cputune.iothreadspin, - targetDef->cputune.niothreadspin, - targetDef->iothreadids[i]->iothread_id); - if (!pininfo) { + cpumask = targetDef->iothreadids[i]->cpumask; + if (!cpumask) { if (targetDef->cpumask) { cpumask = targetDef->cpumask; } else { @@ -5915,8 +5910,6 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef, virBitmapSetAll(bitmap); cpumask = bitmap; } - } else { - cpumask = pininfo->cpumask; } if (virBitmapToData(cpumask, &info_ret[i]->cpumap, &info_ret[i]->cpumaplen) < 0) @@ -5999,8 +5992,6 @@ qemuDomainPinIOThread(virDomainPtr dom, virDomainDefPtr persistentDef = NULL; virBitmapPtr pcpumap = NULL; qemuDomainObjPrivatePtr priv; - virDomainPinDefPtr *newIOThreadsPin = NULL; - size_t newIOThreadsPinNum = 0; virCgroupPtr cgroup_iothread = NULL; virObjectEventPtr event = NULL; char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = ""; @@ -6050,33 +6041,21 @@ qemuDomainPinIOThread(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_LIVE) { virDomainIOThreadIDDefPtr iothrid; + virBitmapPtr cpumask; if (!(iothrid = virDomainIOThreadIDFind(vm->def, iothread_id))) { virReportError(VIR_ERR_INVALID_ARG, - _("iothread value %d not found"), iothread_id); + _("iothreadid %d not found"), iothread_id); goto endjob; } - if (vm->def->cputune.iothreadspin) { - newIOThreadsPin = - virDomainPinDefCopy(vm->def->cputune.iothreadspin, - vm->def->cputune.niothreadspin); - if (!newIOThreadsPin) - goto endjob; - - newIOThreadsPinNum = vm->def->cputune.niothreadspin; - } else { - if (VIR_ALLOC(newIOThreadsPin) < 0) - goto endjob; - newIOThreadsPinNum = 0; - } - - if (virDomainPinAdd(&newIOThreadsPin, &newIOThreadsPinNum, - cpumap, maplen, iothread_id) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update iothreadspin")); + if (!(cpumask = virBitmapNewData(cpumap, maplen))) goto endjob; - } + + if (!iothrid->cpumask) + vm->def->cputune.niothreadspin++; + virBitmapFree(iothrid->cpumask); + iothrid->cpumask = cpumask; /* Configure the corresponding cpuset cgroup before set affinity. */ if (virCgroupHasController(priv->cgroup, @@ -6099,14 +6078,6 @@ qemuDomainPinIOThread(virDomainPtr dom, } } - if (vm->def->cputune.iothreadspin) - virDomainPinDefArrayFree(vm->def->cputune.iothreadspin, - vm->def->cputune.niothreadspin); - - vm->def->cputune.iothreadspin = newIOThreadsPin; - vm->def->cputune.niothreadspin = newIOThreadsPinNum; - newIOThreadsPin = NULL; - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto endjob; @@ -6124,31 +6095,25 @@ qemuDomainPinIOThread(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + virDomainIOThreadIDDefPtr iothrid; + virBitmapPtr cpumask; + /* Coverity didn't realize that targetDef must be set if we got here. */ sa_assert(persistentDef); - if (iothread_id > persistentDef->iothreads) { + if (!(iothrid = virDomainIOThreadIDFind(persistentDef, iothread_id))) { virReportError(VIR_ERR_INVALID_ARG, - _("iothread value out of range %d > %d"), - iothread_id, persistentDef->iothreads); + _("iothreadid %d not found"), iothread_id); goto endjob; } - if (!persistentDef->cputune.iothreadspin) { - if (VIR_ALLOC(persistentDef->cputune.iothreadspin) < 0) - goto endjob; - persistentDef->cputune.niothreadspin = 0; - } - if (virDomainPinAdd(&persistentDef->cputune.iothreadspin, - &persistentDef->cputune.niothreadspin, - cpumap, - maplen, - iothread_id) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update or add iothreadspin xml " - "of a persistent domain")); + if (!(cpumask = virBitmapNewData(cpumap, maplen))) goto endjob; - } + + if (!iothrid->cpumask) + persistentDef->cputune.niothreadspin++; + virBitmapFree(iothrid->cpumask); + iothrid->cpumask = cpumask; ret = virDomainSaveConfig(cfg->configDir, persistentDef); goto endjob; @@ -6160,8 +6125,6 @@ qemuDomainPinIOThread(virDomainPtr dom, qemuDomainObjEndJob(driver, vm); cleanup: - if (newIOThreadsPin) - virDomainPinDefArrayFree(newIOThreadsPin, newIOThreadsPinNum); if (cgroup_iothread) virCgroupFree(&cgroup_iothread); if (event) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 717d113..09263e9 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2458,7 +2458,6 @@ static int qemuProcessSetIOThreadsAffinity(virDomainObjPtr vm) { virDomainDefPtr def = vm->def; - virDomainPinDefPtr pininfo; size_t i; int ret = -1; @@ -2467,13 +2466,11 @@ qemuProcessSetIOThreadsAffinity(virDomainObjPtr vm) for (i = 0; i < def->niothreadids; i++) { /* set affinity only for existing iothreads */ - if (!(pininfo = virDomainPinFind(def->cputune.iothreadspin, - def->cputune.niothreadspin, - def->iothreadids[i]->iothread_id))) + if (!def->iothreadids[i]->cpumask) continue; if (virProcessSetAffinity(def->iothreadids[i]->thread_id, - pininfo->cpumask) < 0) + def->iothreadids[i]->cpumask) < 0) goto cleanup; } ret = 0; -- 2.1.0
On Tue, Apr 21, 2015 at 19:31:25 -0400, John Ferlan wrote:
Remove the iothreadspin array from cputune and replace with a cpumask to be stored in the iothreadids list
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 10 ++-- src/conf/domain_conf.c | 118 +++++++++++++++++++++------------------------- src/conf/domain_conf.h | 2 +- src/qemu/qemu_cgroup.c | 13 ++--- src/qemu/qemu_driver.c | 79 +++++++++---------------------- src/qemu/qemu_process.c | 7 +-- 6 files changed, 86 insertions(+), 143 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 518f7c5..7af6bd7 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -624,11 +624,11 @@ and attribute <code>cpuset</code> of element <code>vcpu</code> is not specified, the IOThreads are pinned to all the physical CPUs by default. There are two required attributes, the attribute - <code>iothread</code> specifies the IOThread id and the attribute - <code>cpuset</code> specifying which physical CPUs to pin to. The - <code>iothread</code> value begins at "1" through the number of - <a href="#elementsIOThreadsAllocation"><code>iothreads</code></a> - allocated to the domain. A value of "0" is not permitted. + <code>iothread</code> specifies the IOThread ID and the attribute + <code>cpuset</code> specifying which physical CPUs to pin to. See + the <code>iothreadids</code> + <a href="#elementsIOThreadsAllocation"><code>description</code></a> + for valid <code>iothread</code> values.
This description is fair enough. It hints that the implicit ones are numbered too.
<span class="since">Since 1.2.9</span> </dd> <dt><code>shares</code></dt> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bd25d52..969e56f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2118,8 +2118,10 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int npin) void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def) { - if (def) + if (def) { + virBitmapFree(def->cpumask); VIR_FREE(def);
This fixes the syntax check failure from 1/9.
+ } }
@@ -2341,9 +2343,6 @@ void virDomainDefFree(virDomainDefPtr def)
virDomainPinDefFree(def->cputune.emulatorpin);
- virDomainPinDefArrayFree(def->cputune.iothreadspin, - def->cputune.niothreadspin); - for (i = 0; i < def->cputune.nvcpusched; i++) virBitmapFree(def->cputune.vcpusched[i].ids); VIR_FREE(def->cputune.vcpusched); @@ -13316,74 +13315,77 @@ virDomainVcpuPinDefParseXML(xmlNodePtr node, * and an iothreadspin has the form * <iothreadpin iothread='1' cpuset='2'/> */ -static virDomainPinDefPtr +static int virDomainIOThreadPinDefParseXML(xmlNodePtr node, xmlXPathContextPtr ctxt, - int iothreads) + virDomainDefPtr def) { - virDomainPinDefPtr def; + int ret = -1; + virDomainIOThreadIDDefPtr iothrid; + virBitmapPtr cpumask; xmlNodePtr oldnode = ctxt->node; unsigned int iothreadid; char *tmp = NULL;
- if (VIR_ALLOC(def) < 0) - return NULL; - ctxt->node = node;
if (!(tmp = virXPathString("string(./@iothread)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing iothread id in iothreadpin")); - goto error; + goto cleanup; }
if (virStrToLong_uip(tmp, NULL, 10, &iothreadid) < 0) { virReportError(VIR_ERR_XML_ERROR, _("invalid setting for iothread '%s'"), tmp); - goto error; + goto cleanup; } VIR_FREE(tmp);
if (iothreadid == 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("zero is an invalid iothread id value")); - goto error; - } - - /* IOThreads are numbered "iothread1...iothread<n>", where - * "n" is the iothreads value */ - if (iothreadid > iothreads) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("iothread id must not exceed iothreads"));
[1]
- goto error; + goto cleanup; }
- def->id = iothreadid; - if (!(tmp = virXMLPropString(node, "cpuset"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing cpuset for iothreadpin")); - goto error; + goto cleanup; }
- if (virBitmapParse(tmp, 0, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) - goto error; + if (virBitmapParse(tmp, 0, &cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) + goto cleanup;
- if (virBitmapIsAllClear(def->cpumask)) { + if (virBitmapIsAllClear(cpumask)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid value of 'cpuset': %s"), tmp); - goto error; + goto cleanup; + } + + if (!(iothrid = virDomainIOThreadIDFind(def, iothreadid))) { + if (!(iothrid = virDomainIOThreadIDAdd(def, iothreadid)))
Why are you adding a iothread definition here? This code is executed after the iothread info should already be parsed so adding a thread here doesn't make sense. The section here should ressemble the check [1] that you've removed.
+ goto cleanup; + iothrid->autofill = true; + } + + if (iothrid->cpumask) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("duplicate iothreadpin for same iothread '%u'"), + iothreadid); + goto cleanup; }
+ iothrid->cpumask = cpumask; + cpumask = NULL; + ret = 0; + cleanup: VIR_FREE(tmp); + virBitmapFree(cpumask); ctxt->node = oldnode; - return def; - - error: - VIR_FREE(def); - goto cleanup; + return ret; }
@@ -14250,28 +14252,11 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; }
- if (n && VIR_ALLOC_N(def->cputune.iothreadspin, n) < 0) - goto error; - for (i = 0; i < n; i++) { - virDomainPinDefPtr iothreadpin = NULL; - iothreadpin = virDomainIOThreadPinDefParseXML(nodes[i], ctxt, - def->iothreads); - if (!iothreadpin) + if (virDomainIOThreadPinDefParseXML(nodes[i], ctxt, def) < 0) goto error; - - if (virDomainPinIsDuplicate(def->cputune.iothreadspin, - def->cputune.niothreadspin, - iothreadpin->id)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("duplicate iothreadpin for same iothread")); - virDomainPinDefFree(iothreadpin); - goto error; - } - - def->cputune.iothreadspin[def->cputune.niothreadspin++] = - iothreadpin; } + def->cputune.niothreadspin = n;
This variable should be removed along with the iothread pinning structure, shouldn't it?
VIR_FREE(nodes);
if ((n = virXPathNodeSet("./cputune/vcpusched", ctxt, &nodes)) < 0) { @@ -14384,7 +14369,7 @@ virDomainDefParseXML(xmlDocPtr xml,
if (virDomainNumatuneHasPlacementAuto(def->numa) && !def->cpumask && !def->cputune.vcpupin && - !def->cputune.emulatorpin && !def->cputune.iothreadspin) + !def->cputune.emulatorpin && !def->cputune.niothreadspin)
This could be replaced by a function that checks whether any of the threads has pinning info present rather than counting the pinning info.
def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO;
if ((n = virXPathNodeSet("./resource", ctxt, &nodes)) < 0) { @@ -20837,20 +20822,25 @@ virDomainDefFormatInternal(virDomainDefPtr def, VIR_FREE(cpumask); }
- for (i = 0; i < def->cputune.niothreadspin; i++) { - char *cpumask; - /* Ignore the iothreadpin which inherit from "cpuset of "<vcpu>." */ - if (virBitmapEqual(def->cpumask, def->cputune.iothreadspin[i]->cpumask)) - continue; + if (def->cputune.niothreadspin) {
The check above is useless. If neither of the iothreads has pin info, it would be skipped ...
+ for (i = 0; i < def->niothreadids; i++) { + char *cpumask;
- virBufferAsprintf(buf, "<iothreadpin iothread='%u' ", - def->cputune.iothreadspin[i]->id); + /* Ignore iothreadids with no cpumask or a cpumask that + * inherits from "cpuset of "<vcpu>." */ + if (!def->iothreadids[i]->cpumask || + virBitmapEqual(def->cpumask, def->iothreadids[i]->cpumask)) + continue;
... here. Also why are you explicitly rejecting cpumask that is equal to the default one? If a user explicitly specifies it that way then the info would be lost. Additionally, def->cpumask here is used on the iothread automatically without filling it to the structure if it's used so there's no need to do that.
- if (!(cpumask = virBitmapFormat(def->cputune.iothreadspin[i]->cpumask))) - goto error; + virBufferAsprintf(buf, "<iothreadpin iothread='%u' ", + def->iothreadids[i]->iothread_id);
- virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask); - VIR_FREE(cpumask); + if (!(cpumask = virBitmapFormat(def->iothreadids[i]->cpumask))) + goto error; + + virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask); + VIR_FREE(cpumask); + } }
for (i = 0; i < def->cputune.nvcpusched; i++) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index cc98f3d..c71e1b8 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2057,6 +2057,7 @@ struct _virDomainIOThreadIDDef { bool autofill; unsigned int iothread_id; int thread_id; + virBitmapPtr cpumask; };
void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def); @@ -2075,7 +2076,6 @@ struct _virDomainCputune { virDomainPinDefPtr *vcpupin; virDomainPinDefPtr emulatorpin; size_t niothreadspin;
This one should be removed too.
- virDomainPinDefPtr *iothreadspin;
size_t nvcpusched; virDomainThreadSchedParamPtr vcpusched; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index cdf0aaf..5282449 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1149,7 +1149,7 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) virCgroupPtr cgroup_iothread = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr def = vm->def; - size_t i, j; + size_t i; unsigned long long period = vm->def->cputune.period; long long quota = vm->def->cputune.quota; char *mem_mask = NULL; @@ -1214,18 +1214,11 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) /* default cpu masks */ if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) cpumask = priv->autoCpuset; + else if (def->iothreadids[i]->cpumask) + cpumask = def->iothreadids[i]->cpumask;
This has to be the first case. Even if VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO is specified, it cannot override what the user configured pinning.
else cpumask = def->cpumask;
- /* specific cpu mask */ - for (j = 0; j < def->cputune.niothreadspin; j++) { - if (def->cputune.iothreadspin[j]->id == - def->iothreadids[i]->iothread_id) { - cpumask = def->cputune.iothreadspin[j]->cpumask; - break; - }
Finally, this can be removed!
- } - if (cpumask && qemuSetupCgroupCpusetCpus(cgroup_iothread, cpumask) < 0) goto cleanup; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0f95cc7..ee13d08 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5894,19 +5894,14 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef, goto cleanup;
for (i = 0; i < targetDef->niothreadids; i++) { - virDomainPinDefPtr pininfo; - if (VIR_ALLOC(info_ret[i]) < 0) goto cleanup;
/* IOThread ID's are taken from the iothreadids list */ info_ret[i]->iothread_id = targetDef->iothreadids[i]->iothread_id;
- /* Initialize the cpumap */ - pininfo = virDomainPinFind(targetDef->cputune.iothreadspin, - targetDef->cputune.niothreadspin, - targetDef->iothreadids[i]->iothread_id); - if (!pininfo) { + cpumask = targetDef->iothreadids[i]->cpumask; + if (!cpumask) { if (targetDef->cpumask) { cpumask = targetDef->cpumask; } else { @@ -5915,8 +5910,6 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef, virBitmapSetAll(bitmap); cpumask = bitmap; } - } else { - cpumask = pininfo->cpumask; } if (virBitmapToData(cpumask, &info_ret[i]->cpumap, &info_ret[i]->cpumaplen) < 0) @@ -5999,8 +5992,6 @@ qemuDomainPinIOThread(virDomainPtr dom, virDomainDefPtr persistentDef = NULL; virBitmapPtr pcpumap = NULL; qemuDomainObjPrivatePtr priv; - virDomainPinDefPtr *newIOThreadsPin = NULL; - size_t newIOThreadsPinNum = 0; virCgroupPtr cgroup_iothread = NULL; virObjectEventPtr event = NULL; char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = ""; @@ -6050,33 +6041,21 @@ qemuDomainPinIOThread(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_LIVE) {
virDomainIOThreadIDDefPtr iothrid; + virBitmapPtr cpumask;
if (!(iothrid = virDomainIOThreadIDFind(vm->def, iothread_id))) { virReportError(VIR_ERR_INVALID_ARG, - _("iothread value %d not found"), iothread_id); + _("iothreadid %d not found"), iothread_id);
I'd compromise between those two. "iothread %d not foud"
goto endjob; }
- if (vm->def->cputune.iothreadspin) { - newIOThreadsPin = - virDomainPinDefCopy(vm->def->cputune.iothreadspin, - vm->def->cputune.niothreadspin); - if (!newIOThreadsPin) - goto endjob; - - newIOThreadsPinNum = vm->def->cputune.niothreadspin; - } else { - if (VIR_ALLOC(newIOThreadsPin) < 0) - goto endjob; - newIOThreadsPinNum = 0; - } - - if (virDomainPinAdd(&newIOThreadsPin, &newIOThreadsPinNum, - cpumap, maplen, iothread_id) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update iothreadspin")); + if (!(cpumask = virBitmapNewData(cpumap, maplen))) goto endjob; - } + + if (!iothrid->cpumask) + vm->def->cputune.niothreadspin++;
This should go. If you add a function that checks whether pinning s specified it will not be needed.
+ virBitmapFree(iothrid->cpumask); + iothrid->cpumask = cpumask;
/* Configure the corresponding cpuset cgroup before set affinity. */ if (virCgroupHasController(priv->cgroup, @@ -6099,14 +6078,6 @@ qemuDomainPinIOThread(virDomainPtr dom, } }
- if (vm->def->cputune.iothreadspin) - virDomainPinDefArrayFree(vm->def->cputune.iothreadspin, - vm->def->cputune.niothreadspin); - - vm->def->cputune.iothreadspin = newIOThreadsPin; - vm->def->cputune.niothreadspin = newIOThreadsPinNum; - newIOThreadsPin = NULL; - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto endjob;
@@ -6124,31 +6095,25 @@ qemuDomainPinIOThread(virDomainPtr dom, }
if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + virDomainIOThreadIDDefPtr iothrid; + virBitmapPtr cpumask; + /* Coverity didn't realize that targetDef must be set if we got here. */ sa_assert(persistentDef);
- if (iothread_id > persistentDef->iothreads) { + if (!(iothrid = virDomainIOThreadIDFind(persistentDef, iothread_id))) { virReportError(VIR_ERR_INVALID_ARG, - _("iothread value out of range %d > %d"), - iothread_id, persistentDef->iothreads); + _("iothreadid %d not found"), iothread_id); goto endjob; }
- if (!persistentDef->cputune.iothreadspin) { - if (VIR_ALLOC(persistentDef->cputune.iothreadspin) < 0) - goto endjob; - persistentDef->cputune.niothreadspin = 0; - } - if (virDomainPinAdd(&persistentDef->cputune.iothreadspin, - &persistentDef->cputune.niothreadspin, - cpumap, - maplen, - iothread_id) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update or add iothreadspin xml " - "of a persistent domain")); + if (!(cpumask = virBitmapNewData(cpumap, maplen))) goto endjob; - } + + if (!iothrid->cpumask) + persistentDef->cputune.niothreadspin++;
Again, this should not be necessary.
+ virBitmapFree(iothrid->cpumask); + iothrid->cpumask = cpumask;
ret = virDomainSaveConfig(cfg->configDir, persistentDef); goto endjob;
The code looks much better now, but this patch has still some nits. Peter
On 04/23/2015 08:02 AM, Peter Krempa wrote:
On Tue, Apr 21, 2015 at 19:31:25 -0400, John Ferlan wrote:
Remove the iothreadspin array from cputune and replace with a cpumask to be stored in the iothreadids list
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 10 ++-- src/conf/domain_conf.c | 118 +++++++++++++++++++++------------------------- src/conf/domain_conf.h | 2 +- src/qemu/qemu_cgroup.c | 13 ++--- src/qemu/qemu_driver.c | 79 +++++++++---------------------- src/qemu/qemu_process.c | 7 +-- 6 files changed, 86 insertions(+), 143 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 518f7c5..7af6bd7 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -624,11 +624,11 @@ and attribute <code>cpuset</code> of element <code>vcpu</code> is not specified, the IOThreads are pinned to all the physical CPUs by default. There are two required attributes, the attribute - <code>iothread</code> specifies the IOThread id and the attribute - <code>cpuset</code> specifying which physical CPUs to pin to. The - <code>iothread</code> value begins at "1" through the number of - <a href="#elementsIOThreadsAllocation"><code>iothreads</code></a> - allocated to the domain. A value of "0" is not permitted. + <code>iothread</code> specifies the IOThread ID and the attribute + <code>cpuset</code> specifying which physical CPUs to pin to. See + the <code>iothreadids</code> + <a href="#elementsIOThreadsAllocation"><code>description</code></a> + for valid <code>iothread</code> values.
This description is fair enough. It hints that the implicit ones are numbered too.
<span class="since">Since 1.2.9</span> </dd> <dt><code>shares</code></dt> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bd25d52..969e56f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2118,8 +2118,10 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int npin) void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def) { - if (def) + if (def) { + virBitmapFree(def->cpumask); VIR_FREE(def);
This fixes the syntax check failure from 1/9.
Byproduct of rewrites... I've change 1/9 to be (so that each patch passes check/syntax-check) + if (!def) + return; + VIR_FREE(def); Which will change this to be: if (!def) return; + virBitmapFree(def->cpumask); VIR_FREE(def);
+ } }
@@ -2341,9 +2343,6 @@ void virDomainDefFree(virDomainDefPtr def)
virDomainPinDefFree(def->cputune.emulatorpin);
- virDomainPinDefArrayFree(def->cputune.iothreadspin, - def->cputune.niothreadspin); - for (i = 0; i < def->cputune.nvcpusched; i++) virBitmapFree(def->cputune.vcpusched[i].ids); VIR_FREE(def->cputune.vcpusched); @@ -13316,74 +13315,77 @@ virDomainVcpuPinDefParseXML(xmlNodePtr node, * and an iothreadspin has the form * <iothreadpin iothread='1' cpuset='2'/> */ -static virDomainPinDefPtr +static int virDomainIOThreadPinDefParseXML(xmlNodePtr node, xmlXPathContextPtr ctxt, - int iothreads) + virDomainDefPtr def) { - virDomainPinDefPtr def; + int ret = -1; + virDomainIOThreadIDDefPtr iothrid; + virBitmapPtr cpumask; xmlNodePtr oldnode = ctxt->node; unsigned int iothreadid; char *tmp = NULL;
- if (VIR_ALLOC(def) < 0) - return NULL; - ctxt->node = node;
if (!(tmp = virXPathString("string(./@iothread)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing iothread id in iothreadpin")); - goto error; + goto cleanup; }
if (virStrToLong_uip(tmp, NULL, 10, &iothreadid) < 0) { virReportError(VIR_ERR_XML_ERROR, _("invalid setting for iothread '%s'"), tmp); - goto error; + goto cleanup; } VIR_FREE(tmp);
if (iothreadid == 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("zero is an invalid iothread id value")); - goto error; - } - - /* IOThreads are numbered "iothread1...iothread<n>", where - * "n" is the iothreads value */ - if (iothreadid > iothreads) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("iothread id must not exceed iothreads"));
[1]
- goto error; + goto cleanup; }
- def->id = iothreadid; - if (!(tmp = virXMLPropString(node, "cpuset"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing cpuset for iothreadpin")); - goto error; + goto cleanup; }
- if (virBitmapParse(tmp, 0, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) - goto error; + if (virBitmapParse(tmp, 0, &cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) + goto cleanup;
- if (virBitmapIsAllClear(def->cpumask)) { + if (virBitmapIsAllClear(cpumask)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid value of 'cpuset': %s"), tmp); - goto error; + goto cleanup; + } + + if (!(iothrid = virDomainIOThreadIDFind(def, iothreadid))) { + if (!(iothrid = virDomainIOThreadIDAdd(def, iothreadid)))
Why are you adding a iothread definition here? This code is executed after the iothread info should already be parsed so adding a thread here doesn't make sense. The section here should ressemble the check [1] that you've removed.
Because you required that the iothreadpin information to be included in the iothreadid; however, if someone hasn't defined <iothreadids>'s, then there won't be any until the PostParse is called - thus we have to add one here so that we can store the pin information for an iothread This means someone could have: <iothreads>4</iothreads> <cputune> ... <iothreadpin iothread="1" cpuset="5,6"/> ... </cputune> NOTE: No <iothreadids> So I believe it stays as is
+ goto cleanup; + iothrid->autofill = true; + } + + if (iothrid->cpumask) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("duplicate iothreadpin for same iothread '%u'"), + iothreadid); + goto cleanup; }
+ iothrid->cpumask = cpumask; + cpumask = NULL; + ret = 0; + cleanup: VIR_FREE(tmp); + virBitmapFree(cpumask); ctxt->node = oldnode; - return def; - - error: - VIR_FREE(def); - goto cleanup; + return ret; }
@@ -14250,28 +14252,11 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; }
- if (n && VIR_ALLOC_N(def->cputune.iothreadspin, n) < 0) - goto error; - for (i = 0; i < n; i++) { - virDomainPinDefPtr iothreadpin = NULL; - iothreadpin = virDomainIOThreadPinDefParseXML(nodes[i], ctxt, - def->iothreads); - if (!iothreadpin) + if (virDomainIOThreadPinDefParseXML(nodes[i], ctxt, def) < 0) goto error; - - if (virDomainPinIsDuplicate(def->cputune.iothreadspin, - def->cputune.niothreadspin, - iothreadpin->id)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("duplicate iothreadpin for same iothread")); - virDomainPinDefFree(iothreadpin); - goto error; - } - - def->cputune.iothreadspin[def->cputune.niothreadspin++] = - iothreadpin; } + def->cputune.niothreadspin = n;
This variable should be removed along with the iothread pinning structure, shouldn't it?
OK
VIR_FREE(nodes);
if ((n = virXPathNodeSet("./cputune/vcpusched", ctxt, &nodes)) < 0) { @@ -14384,7 +14369,7 @@ virDomainDefParseXML(xmlDocPtr xml,
if (virDomainNumatuneHasPlacementAuto(def->numa) && !def->cpumask && !def->cputune.vcpupin && - !def->cputune.emulatorpin && !def->cputune.iothreadspin) + !def->cputune.emulatorpin && !def->cputune.niothreadspin)
This could be replaced by a function that checks whether any of the threads has pinning info present rather than counting the pinning info.
OK
def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO;
if ((n = virXPathNodeSet("./resource", ctxt, &nodes)) < 0) { @@ -20837,20 +20822,25 @@ virDomainDefFormatInternal(virDomainDefPtr def, VIR_FREE(cpumask); }
- for (i = 0; i < def->cputune.niothreadspin; i++) { - char *cpumask; - /* Ignore the iothreadpin which inherit from "cpuset of "<vcpu>." */ - if (virBitmapEqual(def->cpumask, def->cputune.iothreadspin[i]->cpumask)) - continue; + if (def->cputune.niothreadspin) {
The check above is useless. If neither of the iothreads has pin info, it would be skipped ...
yep
+ for (i = 0; i < def->niothreadids; i++) { + char *cpumask;
- virBufferAsprintf(buf, "<iothreadpin iothread='%u' ", - def->cputune.iothreadspin[i]->id); + /* Ignore iothreadids with no cpumask or a cpumask that + * inherits from "cpuset of "<vcpu>." */ + if (!def->iothreadids[i]->cpumask || + virBitmapEqual(def->cpumask, def->iothreadids[i]->cpumask)) + continue;
... here. Also why are you explicitly rejecting cpumask that is equal to the default one? If a user explicitly specifies it that way then the info would be lost. Additionally, def->cpumask here is used on the iothread automatically without filling it to the structure if it's used so there's no need to do that.
Prior review - it's already there - see commit id '938fb12f'.... I'm fairly certain I was specifically asked to do that... If it needs to change that should be a separate patch IMO... I think if you look a few lines above you'll see the same/similar check for vcpupin
- if (!(cpumask = virBitmapFormat(def->cputune.iothreadspin[i]->cpumask))) - goto error; + virBufferAsprintf(buf, "<iothreadpin iothread='%u' ", + def->iothreadids[i]->iothread_id);
- virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask); - VIR_FREE(cpumask); + if (!(cpumask = virBitmapFormat(def->iothreadids[i]->cpumask))) + goto error; + + virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask); + VIR_FREE(cpumask); + } }
for (i = 0; i < def->cputune.nvcpusched; i++) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index cc98f3d..c71e1b8 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2057,6 +2057,7 @@ struct _virDomainIOThreadIDDef { bool autofill; unsigned int iothread_id; int thread_id; + virBitmapPtr cpumask; };
void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def); @@ -2075,7 +2076,6 @@ struct _virDomainCputune { virDomainPinDefPtr *vcpupin; virDomainPinDefPtr emulatorpin; size_t niothreadspin;
This one should be removed too.
yep
- virDomainPinDefPtr *iothreadspin;
size_t nvcpusched; virDomainThreadSchedParamPtr vcpusched; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index cdf0aaf..5282449 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1149,7 +1149,7 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) virCgroupPtr cgroup_iothread = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr def = vm->def; - size_t i, j; + size_t i; unsigned long long period = vm->def->cputune.period; long long quota = vm->def->cputune.quota; char *mem_mask = NULL; @@ -1214,18 +1214,11 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) /* default cpu masks */ if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) cpumask = priv->autoCpuset; + else if (def->iothreadids[i]->cpumask) + cpumask = def->iothreadids[i]->cpumask;
This has to be the first case. Even if VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO is specified, it cannot override what the user configured pinning.
OK, but see qemuSetupCgroupForVcpu and qemuSetupCgroupForEmulator... Vcpu has one way of doing it and Emulator slightly different, but both check AUTO first. VCPU checks AUTO first, then sets to default. Then it checks the vcpupin and resets to whatever is there. Emulator checks AUTO first, then it checks if emulatorpin is set and uses that, and finally falls back to default if defined. I followed emulatorpin If I'm wrong, then emulator and vcpu are wrong, but I think fixing that is a separate patch.
else cpumask = def->cpumask;
- /* specific cpu mask */ - for (j = 0; j < def->cputune.niothreadspin; j++) { - if (def->cputune.iothreadspin[j]->id == - def->iothreadids[i]->iothread_id) { - cpumask = def->cputune.iothreadspin[j]->cpumask; - break; - }
Finally, this can be removed!
- } - if (cpumask && qemuSetupCgroupCpusetCpus(cgroup_iothread, cpumask) < 0) goto cleanup; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0f95cc7..ee13d08 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5894,19 +5894,14 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef, goto cleanup;
for (i = 0; i < targetDef->niothreadids; i++) { - virDomainPinDefPtr pininfo; - if (VIR_ALLOC(info_ret[i]) < 0) goto cleanup;
/* IOThread ID's are taken from the iothreadids list */ info_ret[i]->iothread_id = targetDef->iothreadids[i]->iothread_id;
- /* Initialize the cpumap */ - pininfo = virDomainPinFind(targetDef->cputune.iothreadspin, - targetDef->cputune.niothreadspin, - targetDef->iothreadids[i]->iothread_id); - if (!pininfo) { + cpumask = targetDef->iothreadids[i]->cpumask; + if (!cpumask) { if (targetDef->cpumask) { cpumask = targetDef->cpumask; } else { @@ -5915,8 +5910,6 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef, virBitmapSetAll(bitmap); cpumask = bitmap; } - } else { - cpumask = pininfo->cpumask; } if (virBitmapToData(cpumask, &info_ret[i]->cpumap, &info_ret[i]->cpumaplen) < 0) @@ -5999,8 +5992,6 @@ qemuDomainPinIOThread(virDomainPtr dom, virDomainDefPtr persistentDef = NULL; virBitmapPtr pcpumap = NULL; qemuDomainObjPrivatePtr priv; - virDomainPinDefPtr *newIOThreadsPin = NULL; - size_t newIOThreadsPinNum = 0; virCgroupPtr cgroup_iothread = NULL; virObjectEventPtr event = NULL; char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = ""; @@ -6050,33 +6041,21 @@ qemuDomainPinIOThread(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_LIVE) {
virDomainIOThreadIDDefPtr iothrid; + virBitmapPtr cpumask;
if (!(iothrid = virDomainIOThreadIDFind(vm->def, iothread_id))) { virReportError(VIR_ERR_INVALID_ARG, - _("iothread value %d not found"), iothread_id); + _("iothreadid %d not found"), iothread_id);
I'd compromise between those two.
"iothread %d not foud"
OK
goto endjob; }
- if (vm->def->cputune.iothreadspin) { - newIOThreadsPin = - virDomainPinDefCopy(vm->def->cputune.iothreadspin, - vm->def->cputune.niothreadspin); - if (!newIOThreadsPin) - goto endjob; - - newIOThreadsPinNum = vm->def->cputune.niothreadspin; - } else { - if (VIR_ALLOC(newIOThreadsPin) < 0) - goto endjob; - newIOThreadsPinNum = 0; - } - - if (virDomainPinAdd(&newIOThreadsPin, &newIOThreadsPinNum, - cpumap, maplen, iothread_id) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update iothreadspin")); + if (!(cpumask = virBitmapNewData(cpumap, maplen))) goto endjob; - } + + if (!iothrid->cpumask) + vm->def->cputune.niothreadspin++;
This should go. If you add a function that checks whether pinning s specified it will not be needed.
Yep
+ virBitmapFree(iothrid->cpumask); + iothrid->cpumask = cpumask;
/* Configure the corresponding cpuset cgroup before set affinity. */ if (virCgroupHasController(priv->cgroup, @@ -6099,14 +6078,6 @@ qemuDomainPinIOThread(virDomainPtr dom, } }
- if (vm->def->cputune.iothreadspin) - virDomainPinDefArrayFree(vm->def->cputune.iothreadspin, - vm->def->cputune.niothreadspin); - - vm->def->cputune.iothreadspin = newIOThreadsPin; - vm->def->cputune.niothreadspin = newIOThreadsPinNum; - newIOThreadsPin = NULL; - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto endjob;
@@ -6124,31 +6095,25 @@ qemuDomainPinIOThread(virDomainPtr dom, }
if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + virDomainIOThreadIDDefPtr iothrid; + virBitmapPtr cpumask; + /* Coverity didn't realize that targetDef must be set if we got here. */ sa_assert(persistentDef);
- if (iothread_id > persistentDef->iothreads) { + if (!(iothrid = virDomainIOThreadIDFind(persistentDef, iothread_id))) { virReportError(VIR_ERR_INVALID_ARG, - _("iothread value out of range %d > %d"), - iothread_id, persistentDef->iothreads); + _("iothreadid %d not found"), iothread_id); goto endjob; }
- if (!persistentDef->cputune.iothreadspin) { - if (VIR_ALLOC(persistentDef->cputune.iothreadspin) < 0) - goto endjob; - persistentDef->cputune.niothreadspin = 0; - } - if (virDomainPinAdd(&persistentDef->cputune.iothreadspin, - &persistentDef->cputune.niothreadspin, - cpumap, - maplen, - iothread_id) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update or add iothreadspin xml " - "of a persistent domain")); + if (!(cpumask = virBitmapNewData(cpumap, maplen))) goto endjob; - } + + if (!iothrid->cpumask) + persistentDef->cputune.niothreadspin++;
Again, this should not be necessary.
yep
+ virBitmapFree(iothrid->cpumask); + iothrid->cpumask = cpumask;
ret = virDomainSaveConfig(cfg->configDir, persistentDef); goto endjob;
The code looks much better now, but this patch has still some nits.
Peter
All the following changes to be squashed in: diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a191b02..9876557 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2115,6 +2115,19 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int npin) } +static bool +virDomainIOThreadIDArrayHasPin(virDomainDefPtr def) +{ + size_t i; + + for (i = 0; i < def->niothreadids; i++) { + if (def->iothreadids[i]->cpumask) + return true; + } + return false; +} + + void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def) { @@ -14256,7 +14269,6 @@ virDomainDefParseXML(xmlDocPtr xml, if (virDomainIOThreadPinDefParseXML(nodes[i], ctxt, def) < 0) goto error; } - def->cputune.niothreadspin = n; VIR_FREE(nodes); if ((n = virXPathNodeSet("./cputune/vcpusched", ctxt, &nodes)) < 0) { @@ -14369,7 +14381,8 @@ virDomainDefParseXML(xmlDocPtr xml, if (virDomainNumatuneHasPlacementAuto(def->numa) && !def->cpumask && !def->cputune.vcpupin && - !def->cputune.emulatorpin && !def->cputune.niothreadspin) + !def->cputune.emulatorpin && + !virDomainIOThreadIDArrayHasPin(def)) def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO; if ((n = virXPathNodeSet("./resource", ctxt, &nodes)) < 0) { @@ -20769,7 +20782,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, def->cputune.period || def->cputune.quota || def->cputune.emulatorpin || def->cputune.emulator_period || def->cputune.emulator_quota || - def->cputune.niothreadspin || + virDomainIOThreadIDArrayHasPin(def) || def->cputune.vcpusched || def->cputune.iothreadsched) { virBufferAddLit(buf, "<cputune>\n"); cputune = true; @@ -20823,25 +20836,23 @@ virDomainDefFormatInternal(virDomainDefPtr def, VIR_FREE(cpumask); } - if (def->cputune.niothreadspin) { - for (i = 0; i < def->niothreadids; i++) { - char *cpumask; + for (i = 0; i < def->niothreadids; i++) { + char *cpumask; - /* Ignore iothreadids with no cpumask or a cpumask that - * inherits from "cpuset of "<vcpu>." */ - if (!def->iothreadids[i]->cpumask || - virBitmapEqual(def->cpumask, def->iothreadids[i]->cpumask)) - continue; + /* Ignore iothreadids with no cpumask or a cpumask that + * inherits from "cpuset of "<vcpu>." */ + if (!def->iothreadids[i]->cpumask || + virBitmapEqual(def->cpumask, def->iothreadids[i]->cpumask)) + continue; - virBufferAsprintf(buf, "<iothreadpin iothread='%u' ", - def->iothreadids[i]->iothread_id); + virBufferAsprintf(buf, "<iothreadpin iothread='%u' ", + def->iothreadids[i]->iothread_id); - if (!(cpumask = virBitmapFormat(def->iothreadids[i]->cpumask))) - goto error; + if (!(cpumask = virBitmapFormat(def->iothreadids[i]->cpumask))) + goto error; - virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask); - VIR_FREE(cpumask); - } + virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask); + VIR_FREE(cpumask); } for (i = 0; i < def->cputune.nvcpusched; i++) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index c71e1b8..7daaeed 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2075,7 +2075,6 @@ struct _virDomainCputune { size_t nvcpupin; virDomainPinDefPtr *vcpupin; virDomainPinDefPtr emulatorpin; - size_t niothreadspin; size_t nvcpusched; virDomainThreadSchedParamPtr vcpusched; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6954cdd..f5216bd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6067,15 +6067,13 @@ qemuDomainPinIOThread(virDomainPtr dom, if (!(iothrid = virDomainIOThreadIDFind(vm->def, iothread_id))) { virReportError(VIR_ERR_INVALID_ARG, - _("iothreadid %d not found"), iothread_id); + _("iothread %d not found"), iothread_id); goto endjob; } if (!(cpumask = virBitmapNewData(cpumap, maplen))) goto endjob; - if (!iothrid->cpumask) - vm->def->cputune.niothreadspin++; virBitmapFree(iothrid->cpumask); iothrid->cpumask = cpumask; @@ -6132,8 +6130,6 @@ qemuDomainPinIOThread(virDomainPtr dom, if (!(cpumask = virBitmapNewData(cpumap, maplen))) goto endjob; - if (!iothrid->cpumask) - persistentDef->cputune.niothreadspin++; virBitmapFree(iothrid->cpumask); iothrid->cpumask = cpumask; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 09263e9..80d922c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2461,9 +2461,6 @@ qemuProcessSetIOThreadsAffinity(virDomainObjPtr vm) size_t i; int ret = -1; - if (!def->cputune.niothreadspin) - return 0; - for (i = 0; i < def->niothreadids; i++) { /* set affinity only for existing iothreads */ if (!def->iothreadids[i]->cpumask)
On Thu, Apr 23, 2015 at 08:58:26 -0400, John Ferlan wrote:
On 04/23/2015 08:02 AM, Peter Krempa wrote:
On Tue, Apr 21, 2015 at 19:31:25 -0400, John Ferlan wrote:
Remove the iothreadspin array from cputune and replace with a cpumask to be stored in the iothreadids list
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 10 ++-- src/conf/domain_conf.c | 118 +++++++++++++++++++++------------------------- src/conf/domain_conf.h | 2 +- src/qemu/qemu_cgroup.c | 13 ++--- src/qemu/qemu_driver.c | 79 +++++++++---------------------- src/qemu/qemu_process.c | 7 +-- 6 files changed, 86 insertions(+), 143 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 518f7c5..7af6bd7 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -624,11 +624,11 @@ and attribute <code>cpuset</code> of element <code>vcpu</code> is not specified, the IOThreads are pinned to all the physical CPUs by default. There are two required attributes, the attribute - <code>iothread</code> specifies the IOThread id and the attribute - <code>cpuset</code> specifying which physical CPUs to pin to. The - <code>iothread</code> value begins at "1" through the number of - <a href="#elementsIOThreadsAllocation"><code>iothreads</code></a> - allocated to the domain. A value of "0" is not permitted. + <code>iothread</code> specifies the IOThread ID and the attribute + <code>cpuset</code> specifying which physical CPUs to pin to. See + the <code>iothreadids</code> + <a href="#elementsIOThreadsAllocation"><code>description</code></a> + for valid <code>iothread</code> values.
This description is fair enough. It hints that the implicit ones are numbered too.
<span class="since">Since 1.2.9</span> </dd> <dt><code>shares</code></dt> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bd25d52..969e56f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2118,8 +2118,10 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int npin) void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def) { - if (def) + if (def) { + virBitmapFree(def->cpumask); VIR_FREE(def);
This fixes the syntax check failure from 1/9.
Byproduct of rewrites...
I've change 1/9 to be (so that each patch passes check/syntax-check)
+ if (!def) + return; + VIR_FREE(def);
Which will change this to be: if (!def) return; + virBitmapFree(def->cpumask); VIR_FREE(def);
+ } }
@@ -2341,9 +2343,6 @@ void virDomainDefFree(virDomainDefPtr def)
virDomainPinDefFree(def->cputune.emulatorpin);
- virDomainPinDefArrayFree(def->cputune.iothreadspin, - def->cputune.niothreadspin); - for (i = 0; i < def->cputune.nvcpusched; i++) virBitmapFree(def->cputune.vcpusched[i].ids); VIR_FREE(def->cputune.vcpusched); @@ -13316,74 +13315,77 @@ virDomainVcpuPinDefParseXML(xmlNodePtr node, * and an iothreadspin has the form * <iothreadpin iothread='1' cpuset='2'/> */ -static virDomainPinDefPtr +static int virDomainIOThreadPinDefParseXML(xmlNodePtr node, xmlXPathContextPtr ctxt, - int iothreads) + virDomainDefPtr def) { - virDomainPinDefPtr def; + int ret = -1; + virDomainIOThreadIDDefPtr iothrid; + virBitmapPtr cpumask; xmlNodePtr oldnode = ctxt->node; unsigned int iothreadid; char *tmp = NULL;
- if (VIR_ALLOC(def) < 0) - return NULL; - ctxt->node = node;
if (!(tmp = virXPathString("string(./@iothread)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing iothread id in iothreadpin")); - goto error; + goto cleanup; }
if (virStrToLong_uip(tmp, NULL, 10, &iothreadid) < 0) { virReportError(VIR_ERR_XML_ERROR, _("invalid setting for iothread '%s'"), tmp); - goto error; + goto cleanup; } VIR_FREE(tmp);
if (iothreadid == 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("zero is an invalid iothread id value")); - goto error; - } - - /* IOThreads are numbered "iothread1...iothread<n>", where - * "n" is the iothreads value */ - if (iothreadid > iothreads) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("iothread id must not exceed iothreads"));
[1]
- goto error; + goto cleanup; }
- def->id = iothreadid; - if (!(tmp = virXMLPropString(node, "cpuset"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing cpuset for iothreadpin")); - goto error; + goto cleanup; }
- if (virBitmapParse(tmp, 0, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) - goto error; + if (virBitmapParse(tmp, 0, &cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) + goto cleanup;
- if (virBitmapIsAllClear(def->cpumask)) { + if (virBitmapIsAllClear(cpumask)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid value of 'cpuset': %s"), tmp); - goto error; + goto cleanup; + } + + if (!(iothrid = virDomainIOThreadIDFind(def, iothreadid))) { + if (!(iothrid = virDomainIOThreadIDAdd(def, iothreadid)))
Why are you adding a iothread definition here? This code is executed after the iothread info should already be parsed so adding a thread here doesn't make sense. The section here should ressemble the check [1] that you've removed.
Because you required that the iothreadpin information to be included in the iothreadid; however, if someone hasn't defined <iothreadids>'s, then there won't be any until the PostParse is called - thus we have to add one here so that we can store the pin information for an iothread
Okay, I didn't see that because I've remembered a different version of patch 1 where you allocated the full list upfront. By the way, the approach in this series now has the following loophole: 1) Specify <iothreads> > nelemes(<iothreadids>) 2) Specify iothreadpin for thread 9999. Now one of the threads is added with id 9999 due to the code above. Thus I think that the PostParse callback code should probably be removed and the missing threads should be numbered right after <iohtreadids> is parsed so that this patch doesn't allow that.
This means someone could have:
<iothreads>4</iothreads> <cputune> ... <iothreadpin iothread="1" cpuset="5,6"/> ... </cputune>
NOTE: No <iothreadids>
So I believe it stays as is
+ goto cleanup; + iothrid->autofill = true; + } + + if (iothrid->cpumask) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("duplicate iothreadpin for same iothread '%u'"), + iothreadid); + goto cleanup; }
+ iothrid->cpumask = cpumask; + cpumask = NULL; + ret = 0; + cleanup: VIR_FREE(tmp); + virBitmapFree(cpumask); ctxt->node = oldnode; - return def; - - error: - VIR_FREE(def); - goto cleanup; + return ret; }
@@ -14250,28 +14252,11 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; }
- if (n && VIR_ALLOC_N(def->cputune.iothreadspin, n) < 0) - goto error; - for (i = 0; i < n; i++) { - virDomainPinDefPtr iothreadpin = NULL; - iothreadpin = virDomainIOThreadPinDefParseXML(nodes[i], ctxt, - def->iothreads); - if (!iothreadpin) + if (virDomainIOThreadPinDefParseXML(nodes[i], ctxt, def) < 0) goto error; - - if (virDomainPinIsDuplicate(def->cputune.iothreadspin, - def->cputune.niothreadspin, - iothreadpin->id)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("duplicate iothreadpin for same iothread")); - virDomainPinDefFree(iothreadpin); - goto error; - } - - def->cputune.iothreadspin[def->cputune.niothreadspin++] = - iothreadpin; } + def->cputune.niothreadspin = n;
This variable should be removed along with the iothread pinning structure, shouldn't it?
OK
VIR_FREE(nodes);
if ((n = virXPathNodeSet("./cputune/vcpusched", ctxt, &nodes)) < 0) { @@ -14384,7 +14369,7 @@ virDomainDefParseXML(xmlDocPtr xml,
if (virDomainNumatuneHasPlacementAuto(def->numa) && !def->cpumask && !def->cputune.vcpupin && - !def->cputune.emulatorpin && !def->cputune.iothreadspin) + !def->cputune.emulatorpin && !def->cputune.niothreadspin)
This could be replaced by a function that checks whether any of the threads has pinning info present rather than counting the pinning info.
OK
def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO;
if ((n = virXPathNodeSet("./resource", ctxt, &nodes)) < 0) { @@ -20837,20 +20822,25 @@ virDomainDefFormatInternal(virDomainDefPtr def, VIR_FREE(cpumask); }
- for (i = 0; i < def->cputune.niothreadspin; i++) { - char *cpumask; - /* Ignore the iothreadpin which inherit from "cpuset of "<vcpu>." */ - if (virBitmapEqual(def->cpumask, def->cputune.iothreadspin[i]->cpumask)) - continue; + if (def->cputune.niothreadspin) {
The check above is useless. If neither of the iothreads has pin info, it would be skipped ...
yep
+ for (i = 0; i < def->niothreadids; i++) { + char *cpumask;
- virBufferAsprintf(buf, "<iothreadpin iothread='%u' ", - def->cputune.iothreadspin[i]->id); + /* Ignore iothreadids with no cpumask or a cpumask that + * inherits from "cpuset of "<vcpu>." */ + if (!def->iothreadids[i]->cpumask || + virBitmapEqual(def->cpumask, def->iothreadids[i]->cpumask)) + continue;
... here. Also why are you explicitly rejecting cpumask that is equal to the default one? If a user explicitly specifies it that way then the info would be lost. Additionally, def->cpumask here is used on the iothread automatically without filling it to the structure if it's used so there's no need to do that.
Prior review - it's already there - see commit id '938fb12f'.... I'm fairly certain I was specifically asked to do that...
It doesn't make much sense though. The default pinning is used from def->cpumap if the specific is not present. If the specific pinning is removed it should be freed.
If it needs to change that should be a separate patch IMO... I think if you look a few lines above you'll see the same/similar check for vcpupin
- if (!(cpumask = virBitmapFormat(def->cputune.iothreadspin[i]->cpumask))) - goto error; + virBufferAsprintf(buf, "<iothreadpin iothread='%u' ", + def->iothreadids[i]->iothread_id);
- virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask); - VIR_FREE(cpumask); + if (!(cpumask = virBitmapFormat(def->iothreadids[i]->cpumask))) + goto error; + + virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask); + VIR_FREE(cpumask); + } }
for (i = 0; i < def->cputune.nvcpusched; i++) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index cc98f3d..c71e1b8 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2057,6 +2057,7 @@ struct _virDomainIOThreadIDDef { bool autofill; unsigned int iothread_id; int thread_id; + virBitmapPtr cpumask; };
void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def); @@ -2075,7 +2076,6 @@ struct _virDomainCputune { virDomainPinDefPtr *vcpupin; virDomainPinDefPtr emulatorpin; size_t niothreadspin;
This one should be removed too.
yep
- virDomainPinDefPtr *iothreadspin;
size_t nvcpusched; virDomainThreadSchedParamPtr vcpusched; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index cdf0aaf..5282449 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1149,7 +1149,7 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) virCgroupPtr cgroup_iothread = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr def = vm->def; - size_t i, j; + size_t i; unsigned long long period = vm->def->cputune.period; long long quota = vm->def->cputune.quota; char *mem_mask = NULL; @@ -1214,18 +1214,11 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) /* default cpu masks */ if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) cpumask = priv->autoCpuset; + else if (def->iothreadids[i]->cpumask) + cpumask = def->iothreadids[i]->cpumask;
This has to be the first case. Even if VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO is specified, it cannot override what the user configured pinning.
OK, but see qemuSetupCgroupForVcpu and qemuSetupCgroupForEmulator... Vcpu has one way of doing it and Emulator slightly different, but both check AUTO first.
VCPU checks AUTO first, then sets to default. Then it checks the vcpupin and resets to whatever is there.
Emulator checks AUTO first, then it checks if emulatorpin is set and uses that, and finally falls back to default if defined.
Um, I made a mistake when I refactored qemuSetupCgroupForEmulator. qemuSetupCgroupForVcpu is the right approach.
I followed emulatorpin
If I'm wrong, then emulator and vcpu are wrong, but I think fixing that is a separate patch.
yes.
else cpumask = def->cpumask;
- /* specific cpu mask */ - for (j = 0; j < def->cputune.niothreadspin; j++) { - if (def->cputune.iothreadspin[j]->id == - def->iothreadids[i]->iothread_id) { - cpumask = def->cputune.iothreadspin[j]->cpumask; - break; - }
Finally, this can be removed!
- } - if (cpumask && qemuSetupCgroupCpusetCpus(cgroup_iothread, cpumask) < 0) goto cleanup; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0f95cc7..ee13d08 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5894,19 +5894,14 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef, goto cleanup;
for (i = 0; i < targetDef->niothreadids; i++) { - virDomainPinDefPtr pininfo; - if (VIR_ALLOC(info_ret[i]) < 0) goto cleanup;
/* IOThread ID's are taken from the iothreadids list */ info_ret[i]->iothread_id = targetDef->iothreadids[i]->iothread_id;
- /* Initialize the cpumap */ - pininfo = virDomainPinFind(targetDef->cputune.iothreadspin, - targetDef->cputune.niothreadspin, - targetDef->iothreadids[i]->iothread_id); - if (!pininfo) { + cpumask = targetDef->iothreadids[i]->cpumask; + if (!cpumask) { if (targetDef->cpumask) { cpumask = targetDef->cpumask; } else { @@ -5915,8 +5910,6 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef, virBitmapSetAll(bitmap); cpumask = bitmap; } - } else { - cpumask = pininfo->cpumask; } if (virBitmapToData(cpumask, &info_ret[i]->cpumap, &info_ret[i]->cpumaplen) < 0) @@ -5999,8 +5992,6 @@ qemuDomainPinIOThread(virDomainPtr dom, virDomainDefPtr persistentDef = NULL; virBitmapPtr pcpumap = NULL; qemuDomainObjPrivatePtr priv; - virDomainPinDefPtr *newIOThreadsPin = NULL; - size_t newIOThreadsPinNum = 0; virCgroupPtr cgroup_iothread = NULL; virObjectEventPtr event = NULL; char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = ""; @@ -6050,33 +6041,21 @@ qemuDomainPinIOThread(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_LIVE) {
virDomainIOThreadIDDefPtr iothrid; + virBitmapPtr cpumask;
if (!(iothrid = virDomainIOThreadIDFind(vm->def, iothread_id))) { virReportError(VIR_ERR_INVALID_ARG, - _("iothread value %d not found"), iothread_id); + _("iothreadid %d not found"), iothread_id);
I'd compromise between those two.
"iothread %d not foud"
OK
goto endjob; }
- if (vm->def->cputune.iothreadspin) { - newIOThreadsPin = - virDomainPinDefCopy(vm->def->cputune.iothreadspin, - vm->def->cputune.niothreadspin); - if (!newIOThreadsPin) - goto endjob; - - newIOThreadsPinNum = vm->def->cputune.niothreadspin; - } else { - if (VIR_ALLOC(newIOThreadsPin) < 0) - goto endjob; - newIOThreadsPinNum = 0; - } - - if (virDomainPinAdd(&newIOThreadsPin, &newIOThreadsPinNum, - cpumap, maplen, iothread_id) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update iothreadspin")); + if (!(cpumask = virBitmapNewData(cpumap, maplen))) goto endjob; - } + + if (!iothrid->cpumask) + vm->def->cputune.niothreadspin++;
This should go. If you add a function that checks whether pinning s specified it will not be needed.
Yep
+ virBitmapFree(iothrid->cpumask); + iothrid->cpumask = cpumask;
/* Configure the corresponding cpuset cgroup before set affinity. */ if (virCgroupHasController(priv->cgroup, @@ -6099,14 +6078,6 @@ qemuDomainPinIOThread(virDomainPtr dom, } }
- if (vm->def->cputune.iothreadspin) - virDomainPinDefArrayFree(vm->def->cputune.iothreadspin, - vm->def->cputune.niothreadspin); - - vm->def->cputune.iothreadspin = newIOThreadsPin; - vm->def->cputune.niothreadspin = newIOThreadsPinNum; - newIOThreadsPin = NULL; - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto endjob;
@@ -6124,31 +6095,25 @@ qemuDomainPinIOThread(virDomainPtr dom, }
if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + virDomainIOThreadIDDefPtr iothrid; + virBitmapPtr cpumask; + /* Coverity didn't realize that targetDef must be set if we got here. */ sa_assert(persistentDef);
- if (iothread_id > persistentDef->iothreads) { + if (!(iothrid = virDomainIOThreadIDFind(persistentDef, iothread_id))) { virReportError(VIR_ERR_INVALID_ARG, - _("iothread value out of range %d > %d"), - iothread_id, persistentDef->iothreads); + _("iothreadid %d not found"), iothread_id); goto endjob; }
- if (!persistentDef->cputune.iothreadspin) { - if (VIR_ALLOC(persistentDef->cputune.iothreadspin) < 0) - goto endjob; - persistentDef->cputune.niothreadspin = 0; - } - if (virDomainPinAdd(&persistentDef->cputune.iothreadspin, - &persistentDef->cputune.niothreadspin, - cpumap, - maplen, - iothread_id) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update or add iothreadspin xml " - "of a persistent domain")); + if (!(cpumask = virBitmapNewData(cpumap, maplen))) goto endjob; - } + + if (!iothrid->cpumask) + persistentDef->cputune.niothreadspin++;
Again, this should not be necessary.
yep
+ virBitmapFree(iothrid->cpumask); + iothrid->cpumask = cpumask;
ret = virDomainSaveConfig(cfg->configDir, persistentDef); goto endjob;
The code looks much better now, but this patch has still some nits.
Peter
All the following changes to be squashed in:
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a191b02..9876557 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2115,6 +2115,19 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int npin)
Adding patches in plaintex in thunderbird or other client that breaks lines makes them unusable so I don't usually bother checking it.
}
Peter
...
+ + if (!(iothrid = virDomainIOThreadIDFind(def, iothreadid))) { + if (!(iothrid = virDomainIOThreadIDAdd(def, iothreadid)))
Why are you adding a iothread definition here? This code is executed after the iothread info should already be parsed so adding a thread here doesn't make sense. The section here should ressemble the check [1] that you've removed.
Because you required that the iothreadpin information to be included in the iothreadid; however, if someone hasn't defined <iothreadids>'s, then there won't be any until the PostParse is called - thus we have to add one here so that we can store the pin information for an iothread
Okay, I didn't see that because I've remembered a different version of patch 1 where you allocated the full list upfront.
By the way, the approach in this series now has the following loophole:
1) Specify <iothreads> > nelemes(<iothreadids>) 2) Specify iothreadpin for thread 9999.
Now one of the threads is added with id 9999 due to the code above.
Thus I think that the PostParse callback code should probably be removed and the missing threads should be numbered right after <iohtreadids> is parsed so that this patch doesn't allow that.
I've moved the code in patch 1 ...
- virBufferAsprintf(buf, "<iothreadpin iothread='%u' ", - def->cputune.iothreadspin[i]->id); + /* Ignore iothreadids with no cpumask or a cpumask that + * inherits from "cpuset of "<vcpu>." */ + if (!def->iothreadids[i]->cpumask || + virBitmapEqual(def->cpumask, def->iothreadids[i]->cpumask)) + continue;
... here. Also why are you explicitly rejecting cpumask that is equal to the default one? If a user explicitly specifies it that way then the info would be lost. Additionally, def->cpumask here is used on the iothread automatically without filling it to the structure if it's used so there's no need to do that.
Prior review - it's already there - see commit id '938fb12f'.... I'm fairly certain I was specifically asked to do that...
It doesn't make much sense though. The default pinning is used from def->cpumap if the specific is not present. If the specific pinning is removed it should be freed.
OK, but I contend this needs to be a separate patch after this series so that all 3 are fixed at once. ...
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index cdf0aaf..5282449 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1149,7 +1149,7 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) virCgroupPtr cgroup_iothread = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr def = vm->def; - size_t i, j; + size_t i; unsigned long long period = vm->def->cputune.period; long long quota = vm->def->cputune.quota; char *mem_mask = NULL; @@ -1214,18 +1214,11 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) /* default cpu masks */ if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) cpumask = priv->autoCpuset; + else if (def->iothreadids[i]->cpumask) + cpumask = def->iothreadids[i]->cpumask;
This has to be the first case. Even if VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO is specified, it cannot override what the user configured pinning.
OK, but see qemuSetupCgroupForVcpu and qemuSetupCgroupForEmulator... Vcpu has one way of doing it and Emulator slightly different, but both check AUTO first.
VCPU checks AUTO first, then sets to default. Then it checks the vcpupin and resets to whatever is there.
Emulator checks AUTO first, then it checks if emulatorpin is set and uses that, and finally falls back to default if defined.
Um, I made a mistake when I refactored qemuSetupCgroupForEmulator. qemuSetupCgroupForVcpu is the right approach.
I will follow ForVcpu and someone else can fix ForEmulator
I followed emulatorpin
If I'm wrong, then emulator and vcpu are wrong, but I think fixing that is a separate patch.
yes.
else cpumask = def->cpumask;
- /* specific cpu mask */ - for (j = 0; j < def->cputune.niothreadspin; j++) { - if (def->cputune.iothreadspin[j]->id == - def->iothreadids[i]->iothread_id) { - cpumask = def->cputune.iothreadspin[j]->cpumask; - break; - }
Finally, this can be removed!
- } - if (cpumask && qemuSetupCgroupCpusetCpus(cgroup_iothread, cpumask) < 0) goto cleanup; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0f95cc7..ee13d08 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5894,19 +5894,14 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef, goto cleanup;
for (i = 0; i < targetDef->niothreadids; i++) { - virDomainPinDefPtr pininfo; - if (VIR_ALLOC(info_ret[i]) < 0) goto cleanup;
/* IOThread ID's are taken from the iothreadids list */ info_ret[i]->iothread_id = targetDef->iothreadids[i]->iothread_id;
- /* Initialize the cpumap */ - pininfo = virDomainPinFind(targetDef->cputune.iothreadspin, - targetDef->cputune.niothreadspin, - targetDef->iothreadids[i]->iothread_id); - if (!pininfo) { + cpumask = targetDef->iothreadids[i]->cpumask; + if (!cpumask) { if (targetDef->cpumask) { cpumask = targetDef->cpumask; } else { @@ -5915,8 +5910,6 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef, virBitmapSetAll(bitmap); cpumask = bitmap; } - } else { - cpumask = pininfo->cpumask; } if (virBitmapToData(cpumask, &info_ret[i]->cpumap, &info_ret[i]->cpumaplen) < 0) @@ -5999,8 +5992,6 @@ qemuDomainPinIOThread(virDomainPtr dom, virDomainDefPtr persistentDef = NULL; virBitmapPtr pcpumap = NULL; qemuDomainObjPrivatePtr priv; - virDomainPinDefPtr *newIOThreadsPin = NULL; - size_t newIOThreadsPinNum = 0; virCgroupPtr cgroup_iothread = NULL; virObjectEventPtr event = NULL; char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = ""; @@ -6050,33 +6041,21 @@ qemuDomainPinIOThread(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_LIVE) {
virDomainIOThreadIDDefPtr iothrid; + virBitmapPtr cpumask;
if (!(iothrid = virDomainIOThreadIDFind(vm->def, iothread_id))) { virReportError(VIR_ERR_INVALID_ARG, - _("iothread value %d not found"), iothread_id); + _("iothreadid %d not found"), iothread_id);
I'd compromise between those two.
"iothread %d not foud"
OK
goto endjob; }
- if (vm->def->cputune.iothreadspin) { - newIOThreadsPin = - virDomainPinDefCopy(vm->def->cputune.iothreadspin, - vm->def->cputune.niothreadspin); - if (!newIOThreadsPin) - goto endjob; - - newIOThreadsPinNum = vm->def->cputune.niothreadspin; - } else { - if (VIR_ALLOC(newIOThreadsPin) < 0) - goto endjob; - newIOThreadsPinNum = 0; - } - - if (virDomainPinAdd(&newIOThreadsPin, &newIOThreadsPinNum, - cpumap, maplen, iothread_id) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update iothreadspin")); + if (!(cpumask = virBitmapNewData(cpumap, maplen))) goto endjob; - } + + if (!iothrid->cpumask) + vm->def->cputune.niothreadspin++;
This should go. If you add a function that checks whether pinning s specified it will not be needed.
Yep
+ virBitmapFree(iothrid->cpumask); + iothrid->cpumask = cpumask;
/* Configure the corresponding cpuset cgroup before set affinity. */ if (virCgroupHasController(priv->cgroup, @@ -6099,14 +6078,6 @@ qemuDomainPinIOThread(virDomainPtr dom, } }
- if (vm->def->cputune.iothreadspin) - virDomainPinDefArrayFree(vm->def->cputune.iothreadspin, - vm->def->cputune.niothreadspin); - - vm->def->cputune.iothreadspin = newIOThreadsPin; - vm->def->cputune.niothreadspin = newIOThreadsPinNum; - newIOThreadsPin = NULL; - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto endjob;
@@ -6124,31 +6095,25 @@ qemuDomainPinIOThread(virDomainPtr dom, }
if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + virDomainIOThreadIDDefPtr iothrid; + virBitmapPtr cpumask; + /* Coverity didn't realize that targetDef must be set if we got here. */ sa_assert(persistentDef);
- if (iothread_id > persistentDef->iothreads) { + if (!(iothrid = virDomainIOThreadIDFind(persistentDef, iothread_id))) { virReportError(VIR_ERR_INVALID_ARG, - _("iothread value out of range %d > %d"), - iothread_id, persistentDef->iothreads); + _("iothreadid %d not found"), iothread_id); goto endjob; }
- if (!persistentDef->cputune.iothreadspin) { - if (VIR_ALLOC(persistentDef->cputune.iothreadspin) < 0) - goto endjob; - persistentDef->cputune.niothreadspin = 0; - } - if (virDomainPinAdd(&persistentDef->cputune.iothreadspin, - &persistentDef->cputune.niothreadspin, - cpumap, - maplen, - iothread_id) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update or add iothreadspin xml " - "of a persistent domain")); + if (!(cpumask = virBitmapNewData(cpumap, maplen))) goto endjob; - } + + if (!iothrid->cpumask) + persistentDef->cputune.niothreadspin++;
Again, this should not be necessary.
yep
+ virBitmapFree(iothrid->cpumask); + iothrid->cpumask = cpumask;
ret = virDomainSaveConfig(cfg->configDir, persistentDef); goto endjob;
The code looks much better now, but this patch has still some nits.
Peter
All the following changes to be squashed in:
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a191b02..9876557 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2115,6 +2115,19 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int npin)
Adding patches in plaintex in thunderbird or other client that breaks lines makes them unusable so I don't usually bother checking it.
OK - I'll just resend a v5 with changes tks - John
Remove the iothreadspin array from cputune and replace with a cpumask to be stored in the iothreadids list. Adjust the test output because our printing goes in order of the iothreadids list now. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 10 +- src/conf/domain_conf.c | 112 ++++++++++----------- src/conf/domain_conf.h | 3 +- src/qemu/qemu_cgroup.c | 13 +-- src/qemu/qemu_driver.c | 75 ++++---------- src/qemu/qemu_process.c | 10 +- .../qemuxml2xmlout-cputune-iothreads.xml | 2 +- 7 files changed, 86 insertions(+), 139 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 518f7c5..7af6bd7 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -624,11 +624,11 @@ and attribute <code>cpuset</code> of element <code>vcpu</code> is not specified, the IOThreads are pinned to all the physical CPUs by default. There are two required attributes, the attribute - <code>iothread</code> specifies the IOThread id and the attribute - <code>cpuset</code> specifying which physical CPUs to pin to. The - <code>iothread</code> value begins at "1" through the number of - <a href="#elementsIOThreadsAllocation"><code>iothreads</code></a> - allocated to the domain. A value of "0" is not permitted. + <code>iothread</code> specifies the IOThread ID and the attribute + <code>cpuset</code> specifying which physical CPUs to pin to. See + the <code>iothreadids</code> + <a href="#elementsIOThreadsAllocation"><code>description</code></a> + for valid <code>iothread</code> values. <span class="since">Since 1.2.9</span> </dd> <dt><code>shares</code></dt> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 15514bb..083ded7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2115,11 +2115,25 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int npin) } +static bool +virDomainIOThreadIDArrayHasPin(virDomainDefPtr def) +{ + size_t i; + + for (i = 0; i < def->niothreadids; i++) { + if (def->iothreadids[i]->cpumask) + return true; + } + return false; +} + + void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def) { if (!def) return; + virBitmapFree(def->cpumask); VIR_FREE(def); } @@ -2342,9 +2356,6 @@ void virDomainDefFree(virDomainDefPtr def) virDomainPinDefFree(def->cputune.emulatorpin); - virDomainPinDefArrayFree(def->cputune.iothreadspin, - def->cputune.niothreadspin); - for (i = 0; i < def->cputune.nvcpusched; i++) virBitmapFree(def->cputune.vcpusched[i].ids); VIR_FREE(def->cputune.vcpusched); @@ -13302,74 +13313,77 @@ virDomainVcpuPinDefParseXML(xmlNodePtr node, * and an iothreadspin has the form * <iothreadpin iothread='1' cpuset='2'/> */ -static virDomainPinDefPtr +static int virDomainIOThreadPinDefParseXML(xmlNodePtr node, xmlXPathContextPtr ctxt, - int iothreads) + virDomainDefPtr def) { - virDomainPinDefPtr def; + int ret = -1; + virDomainIOThreadIDDefPtr iothrid; + virBitmapPtr cpumask; xmlNodePtr oldnode = ctxt->node; unsigned int iothreadid; char *tmp = NULL; - if (VIR_ALLOC(def) < 0) - return NULL; - ctxt->node = node; if (!(tmp = virXPathString("string(./@iothread)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing iothread id in iothreadpin")); - goto error; + goto cleanup; } if (virStrToLong_uip(tmp, NULL, 10, &iothreadid) < 0) { virReportError(VIR_ERR_XML_ERROR, _("invalid setting for iothread '%s'"), tmp); - goto error; + goto cleanup; } VIR_FREE(tmp); if (iothreadid == 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("zero is an invalid iothread id value")); - goto error; + goto cleanup; } - /* IOThreads are numbered "iothread1...iothread<n>", where - * "n" is the iothreads value */ - if (iothreadid > iothreads) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("iothread id must not exceed iothreads")); - goto error; + if (!(iothrid = virDomainIOThreadIDFind(def, iothreadid))) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Cannot find 'iothread' : %u"), + iothreadid); } - def->id = iothreadid; - if (!(tmp = virXMLPropString(node, "cpuset"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing cpuset for iothreadpin")); - goto error; + goto cleanup; } - if (virBitmapParse(tmp, 0, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) - goto error; + if (virBitmapParse(tmp, 0, &cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) + goto cleanup; - if (virBitmapIsAllClear(def->cpumask)) { + if (virBitmapIsAllClear(cpumask)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid value of 'cpuset': %s"), tmp); - goto error; + goto cleanup; } + if (iothrid->cpumask) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("duplicate iothreadpin for same iothread '%u'"), + iothreadid); + goto cleanup; + } + + iothrid->cpumask = cpumask; + cpumask = NULL; + ret = 0; + cleanup: VIR_FREE(tmp); + virBitmapFree(cpumask); ctxt->node = oldnode; - return def; - - error: - VIR_FREE(def); - goto cleanup; + return ret; } @@ -14253,27 +14267,9 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } - if (n && VIR_ALLOC_N(def->cputune.iothreadspin, n) < 0) - goto error; - for (i = 0; i < n; i++) { - virDomainPinDefPtr iothreadpin = NULL; - iothreadpin = virDomainIOThreadPinDefParseXML(nodes[i], ctxt, - def->iothreads); - if (!iothreadpin) + if (virDomainIOThreadPinDefParseXML(nodes[i], ctxt, def) < 0) goto error; - - if (virDomainPinIsDuplicate(def->cputune.iothreadspin, - def->cputune.niothreadspin, - iothreadpin->id)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("duplicate iothreadpin for same iothread")); - virDomainPinDefFree(iothreadpin); - goto error; - } - - def->cputune.iothreadspin[def->cputune.niothreadspin++] = - iothreadpin; } VIR_FREE(nodes); @@ -14387,7 +14383,8 @@ virDomainDefParseXML(xmlDocPtr xml, if (virDomainNumatuneHasPlacementAuto(def->numa) && !def->cpumask && !def->cputune.vcpupin && - !def->cputune.emulatorpin && !def->cputune.iothreadspin) + !def->cputune.emulatorpin && + !virDomainIOThreadIDArrayHasPin(def)) def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO; if ((n = virXPathNodeSet("./resource", ctxt, &nodes)) < 0) { @@ -20787,7 +20784,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, def->cputune.period || def->cputune.quota || def->cputune.emulatorpin || def->cputune.emulator_period || def->cputune.emulator_quota || - def->cputune.niothreadspin || + virDomainIOThreadIDArrayHasPin(def) || def->cputune.vcpusched || def->cputune.iothreadsched) { virBufferAddLit(buf, "<cputune>\n"); cputune = true; @@ -20841,16 +20838,19 @@ virDomainDefFormatInternal(virDomainDefPtr def, VIR_FREE(cpumask); } - for (i = 0; i < def->cputune.niothreadspin; i++) { + for (i = 0; i < def->niothreadids; i++) { char *cpumask; - /* Ignore the iothreadpin which inherit from "cpuset of "<vcpu>." */ - if (virBitmapEqual(def->cpumask, def->cputune.iothreadspin[i]->cpumask)) + + /* Ignore iothreadids with no cpumask or a cpumask that + * inherits from "cpuset of "<vcpu>." */ + if (!def->iothreadids[i]->cpumask || + virBitmapEqual(def->cpumask, def->iothreadids[i]->cpumask)) continue; virBufferAsprintf(buf, "<iothreadpin iothread='%u' ", - def->cputune.iothreadspin[i]->id); + def->iothreadids[i]->iothread_id); - if (!(cpumask = virBitmapFormat(def->cputune.iothreadspin[i]->cpumask))) + if (!(cpumask = virBitmapFormat(def->iothreadids[i]->cpumask))) goto error; virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index cc98f3d..7daaeed 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2057,6 +2057,7 @@ struct _virDomainIOThreadIDDef { bool autofill; unsigned int iothread_id; int thread_id; + virBitmapPtr cpumask; }; void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def); @@ -2074,8 +2075,6 @@ struct _virDomainCputune { size_t nvcpupin; virDomainPinDefPtr *vcpupin; virDomainPinDefPtr emulatorpin; - size_t niothreadspin; - virDomainPinDefPtr *iothreadspin; size_t nvcpusched; virDomainThreadSchedParamPtr vcpusched; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index cdf0aaf..1c7c03a 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1149,7 +1149,7 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) virCgroupPtr cgroup_iothread = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr def = vm->def; - size_t i, j; + size_t i; unsigned long long period = vm->def->cputune.period; long long quota = vm->def->cputune.quota; char *mem_mask = NULL; @@ -1217,14 +1217,9 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) else cpumask = def->cpumask; - /* specific cpu mask */ - for (j = 0; j < def->cputune.niothreadspin; j++) { - if (def->cputune.iothreadspin[j]->id == - def->iothreadids[i]->iothread_id) { - cpumask = def->cputune.iothreadspin[j]->cpumask; - break; - } - } + /* If it exists, override defaults */ + if (def->iothreadids[i]->cpumask) + cpumask = def->iothreadids[i]->cpumask; if (cpumask && qemuSetupCgroupCpusetCpus(cgroup_iothread, cpumask) < 0) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 098f0aa..f5216bd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5916,19 +5916,14 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef, goto cleanup; for (i = 0; i < targetDef->niothreadids; i++) { - virDomainPinDefPtr pininfo; - if (VIR_ALLOC(info_ret[i]) < 0) goto cleanup; /* IOThread ID's are taken from the iothreadids list */ info_ret[i]->iothread_id = targetDef->iothreadids[i]->iothread_id; - /* Initialize the cpumap */ - pininfo = virDomainPinFind(targetDef->cputune.iothreadspin, - targetDef->cputune.niothreadspin, - targetDef->iothreadids[i]->iothread_id); - if (!pininfo) { + cpumask = targetDef->iothreadids[i]->cpumask; + if (!cpumask) { if (targetDef->cpumask) { cpumask = targetDef->cpumask; } else { @@ -5937,8 +5932,6 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef, virBitmapSetAll(bitmap); cpumask = bitmap; } - } else { - cpumask = pininfo->cpumask; } if (virBitmapToData(cpumask, &info_ret[i]->cpumap, &info_ret[i]->cpumaplen) < 0) @@ -6021,8 +6014,6 @@ qemuDomainPinIOThread(virDomainPtr dom, virDomainDefPtr persistentDef = NULL; virBitmapPtr pcpumap = NULL; qemuDomainObjPrivatePtr priv; - virDomainPinDefPtr *newIOThreadsPin = NULL; - size_t newIOThreadsPinNum = 0; virCgroupPtr cgroup_iothread = NULL; virObjectEventPtr event = NULL; char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = ""; @@ -6072,33 +6063,19 @@ qemuDomainPinIOThread(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_LIVE) { virDomainIOThreadIDDefPtr iothrid; + virBitmapPtr cpumask; if (!(iothrid = virDomainIOThreadIDFind(vm->def, iothread_id))) { virReportError(VIR_ERR_INVALID_ARG, - _("iothread value %d not found"), iothread_id); + _("iothread %d not found"), iothread_id); goto endjob; } - if (vm->def->cputune.iothreadspin) { - newIOThreadsPin = - virDomainPinDefCopy(vm->def->cputune.iothreadspin, - vm->def->cputune.niothreadspin); - if (!newIOThreadsPin) - goto endjob; - - newIOThreadsPinNum = vm->def->cputune.niothreadspin; - } else { - if (VIR_ALLOC(newIOThreadsPin) < 0) - goto endjob; - newIOThreadsPinNum = 0; - } - - if (virDomainPinAdd(&newIOThreadsPin, &newIOThreadsPinNum, - cpumap, maplen, iothread_id) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update iothreadspin")); + if (!(cpumask = virBitmapNewData(cpumap, maplen))) goto endjob; - } + + virBitmapFree(iothrid->cpumask); + iothrid->cpumask = cpumask; /* Configure the corresponding cpuset cgroup before set affinity. */ if (virCgroupHasController(priv->cgroup, @@ -6121,14 +6098,6 @@ qemuDomainPinIOThread(virDomainPtr dom, } } - if (vm->def->cputune.iothreadspin) - virDomainPinDefArrayFree(vm->def->cputune.iothreadspin, - vm->def->cputune.niothreadspin); - - vm->def->cputune.iothreadspin = newIOThreadsPin; - vm->def->cputune.niothreadspin = newIOThreadsPinNum; - newIOThreadsPin = NULL; - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto endjob; @@ -6146,31 +6115,23 @@ qemuDomainPinIOThread(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + virDomainIOThreadIDDefPtr iothrid; + virBitmapPtr cpumask; + /* Coverity didn't realize that targetDef must be set if we got here. */ sa_assert(persistentDef); - if (iothread_id > persistentDef->iothreads) { + if (!(iothrid = virDomainIOThreadIDFind(persistentDef, iothread_id))) { virReportError(VIR_ERR_INVALID_ARG, - _("iothread value out of range %d > %d"), - iothread_id, persistentDef->iothreads); + _("iothreadid %d not found"), iothread_id); goto endjob; } - if (!persistentDef->cputune.iothreadspin) { - if (VIR_ALLOC(persistentDef->cputune.iothreadspin) < 0) - goto endjob; - persistentDef->cputune.niothreadspin = 0; - } - if (virDomainPinAdd(&persistentDef->cputune.iothreadspin, - &persistentDef->cputune.niothreadspin, - cpumap, - maplen, - iothread_id) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update or add iothreadspin xml " - "of a persistent domain")); + if (!(cpumask = virBitmapNewData(cpumap, maplen))) goto endjob; - } + + virBitmapFree(iothrid->cpumask); + iothrid->cpumask = cpumask; ret = virDomainSaveConfig(cfg->configDir, persistentDef); goto endjob; @@ -6182,8 +6143,6 @@ qemuDomainPinIOThread(virDomainPtr dom, qemuDomainObjEndJob(driver, vm); cleanup: - if (newIOThreadsPin) - virDomainPinDefArrayFree(newIOThreadsPin, newIOThreadsPinNum); if (cgroup_iothread) virCgroupFree(&cgroup_iothread); if (event) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 717d113..80d922c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2458,22 +2458,16 @@ static int qemuProcessSetIOThreadsAffinity(virDomainObjPtr vm) { virDomainDefPtr def = vm->def; - virDomainPinDefPtr pininfo; size_t i; int ret = -1; - if (!def->cputune.niothreadspin) - return 0; - for (i = 0; i < def->niothreadids; i++) { /* set affinity only for existing iothreads */ - if (!(pininfo = virDomainPinFind(def->cputune.iothreadspin, - def->cputune.niothreadspin, - def->iothreadids[i]->iothread_id))) + if (!def->iothreadids[i]->cpumask) continue; if (virProcessSetAffinity(def->iothreadids[i]->thread_id, - pininfo->cpumask) < 0) + def->iothreadids[i]->cpumask) < 0) goto cleanup; } ret = 0; diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreads.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreads.xml index 3684483..dc65564 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreads.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreads.xml @@ -12,8 +12,8 @@ <vcpupin vcpu='1' cpuset='1'/> <vcpupin vcpu='0' cpuset='0'/> <emulatorpin cpuset='1'/> - <iothreadpin iothread='2' cpuset='3'/> <iothreadpin iothread='1' cpuset='2'/> + <iothreadpin iothread='2' cpuset='3'/> </cputune> <os> <type arch='i686' machine='pc'>hvm</type> -- 2.1.0
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@redhat.com> --- docs/formatdomain.html.in | 6 +++++- src/conf/domain_conf.c | 2 +- .../qemuxml2argvdata/qemuxml2argv-cputune-iothreadsched-toomuch.xml | 1 + 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 7af6bd7..99ce298 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -691,7 +691,11 @@ 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 + <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>. 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). diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 969e56f..3e8551b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14313,7 +14313,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; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cputune-iothreadsched-toomuch.xml b/tests/qemuxml2argvdata/qemuxml2argv-cputune-iothreadsched-toomuch.xml index 1540969..97a5cde 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-cputune-iothreadsched-toomuch.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-cputune-iothreadsched-toomuch.xml @@ -13,6 +13,7 @@ <vcpupin vcpu='1' cpuset='1'/> <emulatorpin cpuset='1'/> <vcpusched vcpus='0-1' scheduler='fifo' priority='1'/> + <iothreadsched iothreads='1' scheduler='batch'/> <iothreadsched iothreads='2' scheduler='batch'/> </cputune> <os> -- 2.1.0
On Tue, Apr 21, 2015 at 19:31:26 -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@redhat.com> --- docs/formatdomain.html.in | 6 +++++- src/conf/domain_conf.c | 2 +- .../qemuxml2argvdata/qemuxml2argv-cputune-iothreadsched-toomuch.xml | 1 + 3 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 7af6bd7..99ce298 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -691,7 +691,11 @@ 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 + <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>. For
<iothreadids> don't include the implicit ones that were added by <iothreads>, but this code should be able to set them.
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). diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 969e56f..3e8551b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14313,7 +14313,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; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cputune-iothreadsched-toomuch.xml b/tests/qemuxml2argvdata/qemuxml2argv-cputune-iothreadsched-toomuch.xml index 1540969..97a5cde 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-cputune-iothreadsched-toomuch.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-cputune-iothreadsched-toomuch.xml @@ -13,6 +13,7 @@ <vcpupin vcpu='1' cpuset='1'/> <emulatorpin cpuset='1'/> <vcpusched vcpus='0-1' scheduler='fifo' priority='1'/> + <iothreadsched iothreads='1' scheduler='batch'/> <iothreadsched iothreads='2' scheduler='batch'/>
Both lines above are equivalent to <iothreadsched iothreads='1-2' scheduler='batch'/>. Change the scheduler type if there's a reason to have two lines.
</cputune> <os>
Peter
On 04/23/2015 07:24 AM, Peter Krempa wrote:
On Tue, Apr 21, 2015 at 19:31:26 -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@redhat.com> --- docs/formatdomain.html.in | 6 +++++- src/conf/domain_conf.c | 2 +- .../qemuxml2argvdata/qemuxml2argv-cputune-iothreadsched-toomuch.xml | 1 + 3 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 7af6bd7..99ce298 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -691,7 +691,11 @@ 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 + <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>. For
<iothreadids> don't include the implicit ones that were added by <iothreads>, but this code should be able to set them.
It can, but I'll add the following to help out (at least ranges are described now...): If no <code>iothreadids</code> are defined, then libvirt numbers IOThreads from 1 to the number of <code>iothreads</code> available for the domain.
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). diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 969e56f..3e8551b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14313,7 +14313,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; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cputune-iothreadsched-toomuch.xml b/tests/qemuxml2argvdata/qemuxml2argv-cputune-iothreadsched-toomuch.xml index 1540969..97a5cde 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-cputune-iothreadsched-toomuch.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-cputune-iothreadsched-toomuch.xml @@ -13,6 +13,7 @@ <vcpupin vcpu='1' cpuset='1'/> <emulatorpin cpuset='1'/> <vcpusched vcpus='0-1' scheduler='fifo' priority='1'/> + <iothreadsched iothreads='1' scheduler='batch'/> <iothreadsched iothreads='2' scheduler='batch'/>
Both lines above are equivalent to <iothreadsched iothreads='1-2' scheduler='batch'/>. Change the scheduler type if there's a reason to have two lines.
OK - the reason for two lines is the "assumption" change that the IOThreads are numbered 1..n. The existing test would have iothread_id=2 which would fail the > iothreads (of 1) check. The following doesn't change anything, <iothreadsched iothreads='1-2' scheduler='batch'/> so, the following is needed: - <iothreadsched iothreads='2' scheduler='batch'/> + <iothreadsched iothreads='1' scheduler='batch'/> + <iothreadsched iothreads='2' scheduler='fifo'/>
</cputune> <os>
Peter
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@redhat.com> --- 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 083ded7..846a744 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14327,7 +14327,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; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cputune-iothreadsched-toomuch.xml b/tests/qemuxml2argvdata/qemuxml2argv-cputune-iothreadsched-toomuch.xml index 1540969..7cae303 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-cputune-iothreadsched-toomuch.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-cputune-iothreadsched-toomuch.xml @@ -13,7 +13,8 @@ <vcpupin vcpu='1' cpuset='1'/> <emulatorpin cpuset='1'/> <vcpusched vcpus='0-1' scheduler='fifo' priority='1'/> - <iothreadsched iothreads='2' scheduler='batch'/> + <iothreadsched iothreads='1' scheduler='batch'/> + <iothreadsched iothreads='2' scheduler='fifo' priority='1'/> </cputune> <os> <type arch='i686' machine='pc'>hvm</type> -- 2.1.0
Add libvirt API's to manage adding and deleting IOThreads to/from the domain Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 6 ++ src/driver-hypervisor.h | 12 ++++ src/libvirt-domain.c | 118 +++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 6 ++ 4 files changed, 142 insertions(+) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 8a4fe53..528cfae 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1615,6 +1615,12 @@ int virDomainPinIOThread(virDomainPtr domain, unsigned char *cpumap, int maplen, unsigned int flags); +int virDomainAddIOThread(virDomainPtr domain, + unsigned int iothread_id, + unsigned int flags); +int virDomainDelIOThread(virDomainPtr domain, + unsigned int iothread_id, + unsigned int flags); /** * VIR_USE_CPU: diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h index 1b92460..8b8d031 100644 --- a/src/driver-hypervisor.h +++ b/src/driver-hypervisor.h @@ -393,6 +393,16 @@ typedef int unsigned int flags); typedef int +(*virDrvDomainAddIOThread)(virDomainPtr domain, + unsigned int iothread_id, + unsigned int flags); + +typedef int +(*virDrvDomainDelIOThread)(virDomainPtr domain, + unsigned int iothread_id, + unsigned int flags); + +typedef int (*virDrvDomainGetSecurityLabel)(virDomainPtr domain, virSecurityLabelPtr seclabel); @@ -1273,6 +1283,8 @@ struct _virHypervisorDriver { virDrvDomainGetMaxVcpus domainGetMaxVcpus; virDrvDomainGetIOThreadInfo domainGetIOThreadInfo; virDrvDomainPinIOThread domainPinIOThread; + virDrvDomainAddIOThread domainAddIOThread; + virDrvDomainDelIOThread domainDelIOThread; virDrvDomainGetSecurityLabel domainGetSecurityLabel; virDrvDomainGetSecurityLabelList domainGetSecurityLabelList; virDrvNodeGetSecurityModel nodeGetSecurityModel; diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index ccacdb4..ec5e2ff 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -8020,6 +8020,124 @@ virDomainPinIOThread(virDomainPtr domain, /** + * virDomainAddIOThread: + * @domain: a domain object + * @iothread_id: the specific IOThread ID value to add + * @flags: bitwise-OR of virDomainModificationImpact + * + * Dynamically add an IOThread to the domain. If @iothread_id is a positive + * non-zero value, then attempt to add the specific IOThread ID and error + * out if the iothread id already exists. + * + * Note that this call can fail if the underlying virtualization hypervisor + * does not support it or if growing the number is arbitrarily limited. + * This function requires privileged access to the hypervisor. + * + * @flags may include VIR_DOMAIN_AFFECT_LIVE or VIR_DOMAIN_AFFECT_CONFIG. + * Both flags may be set. + * If VIR_DOMAIN_AFFECT_LIVE is set, the change affects a running domain + * and may fail if domain is not alive. + * If VIR_DOMAIN_AFFECT_CONFIG is set, the change affects persistent state, + * and will fail for transient domains. If neither flag is specified (that is, + * @flags is VIR_DOMAIN_AFFECT_CURRENT), then an inactive domain modifies + * persistent setup, while an active domain is hypervisor-dependent on whether + * just live or both live and persistent state is changed. + * + * Returns 0 in case of success, -1 in case of failure. + */ +int +virDomainAddIOThread(virDomainPtr domain, + unsigned int iothread_id, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "iothread_id=%u, flags=%x", + iothread_id, flags); + + virResetLastError(); + + virCheckDomainReturn(domain, -1); + virCheckReadOnlyGoto(domain->conn->flags, error); + + conn = domain->conn; + + if (conn->driver->domainAddIOThread) { + int ret; + ret = conn->driver->domainAddIOThread(domain, iothread_id, flags); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + + error: + virDispatchError(domain->conn); + return -1; +} + + +/** + * virDomainDelIOThread: + * @domain: a domain object + * @iothread_id: the specific IOThread ID value to delete + * @flags: bitwise-OR of virDomainModificationImpact + * + * Dynamically delete an IOThread from the domain. The @iothread_id to be + * deleted must not have a resource associated with it and can be any of + * the currently valid IOThread ID's. + * + * Note that this call can fail if the underlying virtualization hypervisor + * does not support it or if reducing the number is arbitrarily limited. + * This function requires privileged access to the hypervisor. + * + * @flags may include VIR_DOMAIN_AFFECT_LIVE or VIR_DOMAIN_AFFECT_CONFIG. + * Both flags may be set. + * If VIR_DOMAIN_AFFECT_LIVE is set, the change affects a running domain + * and may fail if domain is not alive. + * If VIR_DOMAIN_AFFECT_CONFIG is set, the change affects persistent state, + * and will fail for transient domains. If neither flag is specified (that is, + * @flags is VIR_DOMAIN_AFFECT_CURRENT), then an inactive domain modifies + * persistent setup, while an active domain is hypervisor-dependent on whether + * just live or both live and persistent state is changed. + * + * Returns 0 in case of success, -1 in case of failure. + */ +int +virDomainDelIOThread(virDomainPtr domain, + unsigned int iothread_id, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "iothread_id=%u, flags=%x", iothread_id, flags); + + virResetLastError(); + + virCheckDomainReturn(domain, -1); + virCheckReadOnlyGoto(domain->conn->flags, error); + virCheckNonZeroArgGoto(iothread_id, error); + + conn = domain->conn; + + if (conn->driver->domainDelIOThread) { + int ret; + ret = conn->driver->domainDelIOThread(domain, iothread_id, flags); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + + error: + virDispatchError(domain->conn); + return -1; +} + + +/** * virDomainGetSecurityLabel: * @domain: a domain object * @seclabel: pointer to a virSecurityLabel structure diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 28347c6..ef3d2f0 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -704,4 +704,10 @@ LIBVIRT_1.2.14 { virDomainInterfaceFree; } LIBVIRT_1.2.12; +LIBVIRT_1.2.15 { + global: + virDomainAddIOThread; + virDomainDelIOThread; +} LIBVIRT_1.2.14; + # .... define new API here using predicted next version number .... -- 2.1.0
Add remote support for the add/delete IOThread API's Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/remote/remote_driver.c | 2 ++ src/remote/remote_protocol.x | 30 +++++++++++++++++++++++++++++- src/remote_protocol-structs | 12 ++++++++++++ 3 files changed, 43 insertions(+), 1 deletion(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 9c3b53f..31417e8 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8239,6 +8239,8 @@ static virHypervisorDriver hypervisor_driver = { .domainGetMaxVcpus = remoteDomainGetMaxVcpus, /* 0.3.0 */ .domainGetIOThreadInfo = remoteDomainGetIOThreadInfo, /* 1.2.14 */ .domainPinIOThread = remoteDomainPinIOThread, /* 1.2.14 */ + .domainAddIOThread = remoteDomainAddIOThread, /* 1.2.15 */ + .domainDelIOThread = remoteDomainDelIOThread, /* 1.2.15 */ .domainGetSecurityLabel = remoteDomainGetSecurityLabel, /* 0.6.1 */ .domainGetSecurityLabelList = remoteDomainGetSecurityLabelList, /* 0.10.0 */ .nodeGetSecurityModel = remoteNodeGetSecurityModel, /* 0.6.1 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index b02e58c..49b7ddd 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1212,6 +1212,18 @@ struct remote_domain_pin_iothread_args { unsigned int flags; }; +struct remote_domain_add_iothread_args { + remote_nonnull_domain dom; + unsigned int iothread_id; + unsigned int flags; +}; + +struct remote_domain_del_iothread_args { + remote_nonnull_domain dom; + unsigned int iothread_id; + unsigned int flags; +}; + struct remote_domain_get_security_label_args { remote_nonnull_domain dom; }; @@ -5655,5 +5667,21 @@ enum remote_procedure { * @generate: both * @acl: none */ - REMOTE_PROC_DOMAIN_EVENT_CALLBACK_DEVICE_ADDED = 354 + REMOTE_PROC_DOMAIN_EVENT_CALLBACK_DEVICE_ADDED = 354, + + /** + * @generate:both + * @acl: domain:write + * @acl: domain:save:!VIR_DOMAIN_AFFECT_CONFIG|VIR_DOMAIN_AFFECT_LIVE + * @acl: domain:save:VIR_DOMAIN_AFFECT_CONFIG + */ + REMOTE_PROC_DOMAIN_ADD_IOTHREAD = 355, + + /** + * @generate:both + * @acl: domain:write + * @acl: domain:save:!VIR_DOMAIN_AFFECT_CONFIG|VIR_DOMAIN_AFFECT_LIVE + * @acl: domain:save:VIR_DOMAIN_AFFECT_CONFIG + */ + REMOTE_PROC_DOMAIN_DEL_IOTHREAD = 356 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 2b6b47a..116b572 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -834,6 +834,16 @@ struct remote_domain_pin_iothread_args { } cpumap; u_int flags; }; +struct remote_domain_add_iothread_args { + remote_nonnull_domain dom; + u_int iothread_id; + u_int flags; +}; +struct remote_domain_del_iothread_args { + remote_nonnull_domain dom; + u_int iothread_id; + u_int flags; +}; struct remote_domain_get_security_label_args { remote_nonnull_domain dom; }; @@ -3023,4 +3033,6 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_PIN_IOTHREAD = 352, REMOTE_PROC_DOMAIN_INTERFACE_ADDRESSES = 353, REMOTE_PROC_DOMAIN_EVENT_CALLBACK_DEVICE_ADDED = 354, + REMOTE_PROC_DOMAIN_ADD_IOTHREAD = 355, + REMOTE_PROC_DOMAIN_DEL_IOTHREAD = 356, }; -- 2.1.0
We're about to allow IOThreads to be deleted, but an iothreadid may be included in some domain thread sched, so add a new API to allow removing an iothread from some entry. Then during the writing of the threadsched data and an additional check to determine whether the bitmap is all clear before writing it out. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 20 ++++++++++++++++++++ src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + 3 files changed, 22 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 846a744..bc975f3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17434,6 +17434,24 @@ virDomainIOThreadIDDel(virDomainDefPtr def, } } +void +virDomainIOThreadSchedDelId(virDomainDefPtr def, + unsigned int iothreadid) +{ + size_t i; + + if (!def->cputune.iothreadsched || !def->cputune.niothreadsched) + return; + + for (i = 0; i < def->cputune.niothreadsched; i++) { + if (virBitmapIsBitSet(def->cputune.iothreadsched[i].ids, iothreadid)) { + ignore_value(virBitmapClearBit(def->cputune.iothreadsched[i].ids, + iothreadid)); + return; + } + } +} + virDomainPinDefPtr virDomainPinFind(virDomainPinDefPtr *def, int npin, @@ -20876,6 +20894,8 @@ virDomainDefFormatInternal(virDomainDefPtr def, virDomainThreadSchedParamPtr sp = &def->cputune.iothreadsched[i]; char *ids = NULL; + if (virBitmapIsAllClear(sp->ids)) + continue; if (!(ids = virBitmapFormat(sp->ids))) goto error; virBufferAsprintf(buf, "<iothreadsched iothreads='%s' scheduler='%s'", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 7daaeed..4ae89fd 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2615,6 +2615,7 @@ virDomainIOThreadIDDefPtr virDomainIOThreadIDFind(virDomainDefPtr def, virDomainIOThreadIDDefPtr virDomainIOThreadIDAdd(virDomainDefPtr def, unsigned int iothread_id); void virDomainIOThreadIDDel(virDomainDefPtr def, unsigned int iothread_id); +void virDomainIOThreadSchedDelId(virDomainDefPtr def, unsigned int iothread_id); unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a0f4152..0877907 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -328,6 +328,7 @@ virDomainIOThreadIDAdd; virDomainIOThreadIDDefFree; virDomainIOThreadIDDel; virDomainIOThreadIDFind; +virDomainIOThreadSchedDelId; virDomainLeaseDefFree; virDomainLeaseIndex; virDomainLeaseInsert; -- 2.1.0
Add qemuDomainAddIOThread and qemuDomainDelIOThread in order to add or remove an IOThread to/from the host either for live or config optoins The implementation for the 'live' option will use the iothreadpids list in order to make decision, while the 'config' option will use the iothreadids list. Additionally, for deletion each may have to adjust the iothreadpin list. IOThreads are implemented by qmp objects, the code makes use of the existing qemuMonitorAddObject or qemuMonitorDelObject APIs. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_audit.c | 9 ++ src/conf/domain_audit.h | 6 + src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 401 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 417 insertions(+) diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 3e93d97..4ea10d2 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -790,6 +790,15 @@ virDomainAuditVcpu(virDomainObjPtr vm, return virDomainAuditResource(vm, "vcpu", oldvcpu, newvcpu, reason, success); } +void +virDomainAuditIOThread(virDomainObjPtr vm, + unsigned int oldiothread, unsigned int newiothread, + const char *reason, bool success) +{ + return virDomainAuditResource(vm, "iothread", oldiothread, newiothread, + reason, success); +} + static void virDomainAuditLifecycle(virDomainObjPtr vm, const char *op, const char *reason, bool success) diff --git a/src/conf/domain_audit.h b/src/conf/domain_audit.h index 4c1ef90..97dadca 100644 --- a/src/conf/domain_audit.h +++ b/src/conf/domain_audit.h @@ -102,6 +102,12 @@ void virDomainAuditVcpu(virDomainObjPtr vm, const char *reason, bool success) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); +void virDomainAuditIOThread(virDomainObjPtr vm, + unsigned int oldiothread, + unsigned int newiothread, + const char *reason, + bool success) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); void virDomainAuditSecurityLabel(virDomainObjPtr vm, bool success) ATTRIBUTE_NONNULL(1); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a0f4152..6f2cf40 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -127,6 +127,7 @@ virDomainAuditDisk; virDomainAuditFS; virDomainAuditHostdev; virDomainAuditInit; +virDomainAuditIOThread; virDomainAuditMemory; virDomainAuditNet; virDomainAuditNetDevice; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ee13d08..c34abc9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6137,6 +6137,405 @@ qemuDomainPinIOThread(virDomainPtr dom, return ret; } +static int +qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int iothread_id) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + char *alias = NULL; + size_t idx; + int rc = -1; + int ret = -1; + unsigned int orig_niothreads = vm->def->iothreads; + unsigned int exp_niothreads = vm->def->iothreads; + int new_niothreads = 0; + qemuMonitorIOThreadInfoPtr *new_iothreads = NULL; + virCgroupPtr cgroup_iothread = NULL; + char *mem_mask = NULL; + virDomainIOThreadIDDefPtr iothrid; + + if (virDomainIOThreadIDFind(vm->def, iothread_id)) { + virReportError(VIR_ERR_INVALID_ARG, + _("an IOThread is already using iothread_id '%u'"), + iothread_id); + goto cleanup; + } + + if (virAsprintf(&alias, "iothread%u", iothread_id) < 0) + return -1; + + qemuDomainObjEnterMonitor(driver, vm); + + rc = qemuMonitorAddObject(priv->mon, "iothread", alias, NULL); + exp_niothreads++; + if (rc < 0) + goto exit_monitor; + + /* After hotplugging the IOThreads we need to re-detect the + * IOThreads thread_id's, adjust the cgroups, thread affinity, + * and add the thread_id to the vm->def->iothreadids list. + */ + if ((new_niothreads = qemuMonitorGetIOThreads(priv->mon, + &new_iothreads)) < 0) + goto exit_monitor; + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + if (new_niothreads != exp_niothreads) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("got wrong number of IOThread ids from QEMU monitor. " + "got %d, wanted %d"), + new_niothreads, exp_niothreads); + vm->def->iothreads = new_niothreads; + goto cleanup; + } + vm->def->iothreads = exp_niothreads; + + if (virDomainNumatuneGetMode(vm->def->numa, -1) == + VIR_DOMAIN_NUMATUNE_MEM_STRICT && + virDomainNumatuneMaybeFormatNodeset(vm->def->numa, + priv->autoNodeset, + &mem_mask, -1) < 0) + goto cleanup; + + + /* + * If we've successfully added an IOThread, find out where we added it + * in the QEMU IOThread list, so we can add it to our iothreadids list + */ + for (idx = 0; idx < new_niothreads; idx++) { + if (STREQ(new_iothreads[idx]->name, alias)) + break; + } + + if (idx == new_niothreads) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find new IOThread '%u' in QEMU monitor."), + iothread_id); + goto cleanup; + } + + if (!(iothrid = virDomainIOThreadIDAdd(vm->def, iothread_id))) + goto cleanup; + + iothrid->thread_id = new_iothreads[idx]->thread_id; + + /* Add IOThread to cgroup if present */ + if (priv->cgroup) { + cgroup_iothread = + qemuDomainAddCgroupForThread(priv->cgroup, + VIR_CGROUP_THREAD_IOTHREAD, + iothread_id, mem_mask, + iothrid->thread_id); + if (!cgroup_iothread) + goto cleanup; + } + + /* Inherit def->cpuset */ + if (vm->def->cpumask) { + if (!(iothrid->cpumask = virBitmapNewCopy(vm->def->cpumask))) + goto cleanup; + vm->def->cputune.niothreadspin++; + + if (qemuDomainHotplugPinThread(iothrid->cpumask, iothread_id, + iothrid->thread_id, cgroup_iothread) < 0) + goto cleanup; + + if (qemuProcessSetSchedParams(iothread_id, iothrid->thread_id, + vm->def->cputune.niothreadsched, + vm->def->cputune.iothreadsched) < 0) + goto cleanup; + } + + ret = 0; + + cleanup: + if (new_iothreads) { + for (idx = 0; idx < new_niothreads; idx++) + qemuMonitorIOThreadInfoFree(new_iothreads[idx]); + VIR_FREE(new_iothreads); + } + VIR_FREE(mem_mask); + virDomainAuditIOThread(vm, orig_niothreads, new_niothreads, + "update", rc == 0); + virCgroupFree(&cgroup_iothread); + VIR_FREE(alias); + return ret; + + exit_monitor: + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; +} + +static int +qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int iothread_id) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + size_t idx; + char *alias = NULL; + int rc = -1; + int ret = -1; + unsigned int orig_niothreads = vm->def->iothreads; + unsigned int exp_niothreads = vm->def->iothreads; + int new_niothreads = 0; + qemuMonitorIOThreadInfoPtr *new_iothreads = NULL; + + /* Normally would use virDomainIOThreadIDFind, but we need the index + * from whence to delete for later... + */ + for (idx = 0; idx < vm->def->niothreadids; idx++) { + if (iothread_id == vm->def->iothreadids[idx]->iothread_id) + break; + } + + if (idx == vm->def->niothreadids) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot find IOThread '%u' in iothreadids list"), + iothread_id); + return -1; + } + + if (virAsprintf(&alias, "iothread%u", iothread_id) < 0) + return -1; + + qemuDomainObjEnterMonitor(driver, vm); + + rc = qemuMonitorDelObject(priv->mon, alias); + exp_niothreads--; + if (rc < 0) + goto exit_monitor; + + if ((new_niothreads = qemuMonitorGetIOThreads(priv->mon, + &new_iothreads)) < 0) + goto exit_monitor; + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + if (new_niothreads != exp_niothreads) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("got wrong number of IOThread ids from QEMU monitor. " + "got %d, wanted %d"), + new_niothreads, exp_niothreads); + vm->def->iothreads = new_niothreads; + goto cleanup; + } + vm->def->iothreads = exp_niothreads; + + if (vm->def->iothreadids[idx]->cpumask) + vm->def->cputune.niothreadspin--; + virDomainIOThreadIDDel(vm->def, iothread_id); + + if (qemuDomainDelCgroupForThread(priv->cgroup, + VIR_CGROUP_THREAD_IOTHREAD, + iothread_id) < 0) + goto cleanup; + + ret = 0; + + cleanup: + if (new_iothreads) { + for (idx = 0; idx < new_niothreads; idx++) + qemuMonitorIOThreadInfoFree(new_iothreads[idx]); + VIR_FREE(new_iothreads); + } + virDomainAuditIOThread(vm, orig_niothreads, new_niothreads, + "update", rc == 0); + VIR_FREE(alias); + return ret; + + exit_monitor: + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; +} + +static int +qemuDomainChgIOThread(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int iothread_id, + bool add, + unsigned int flags) +{ + virQEMUDriverConfigPtr cfg = NULL; + virCapsPtr caps = NULL; + qemuDomainObjPrivatePtr priv; + virCgroupPtr cgroup_temp = NULL; + virBitmapPtr all_nodes = NULL; + char *all_nodes_str = NULL; + char *mem_mask = NULL; + virDomainDefPtr persistentDef; + int ret = -1; + + cfg = virQEMUDriverGetConfig(driver); + + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + priv = vm->privateData; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, + &persistentDef) < 0) + goto endjob; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot change IOThreads for an inactive domain")); + goto endjob; + } + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IOThreads not supported with this binary")); + goto endjob; + } + + if (virNumaIsAvailable()) { + if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0, + false, &cgroup_temp) < 0) + goto endjob; + + if (!(all_nodes = virNumaGetHostNodeset())) + goto endjob; + + if (!(all_nodes_str = virBitmapFormat(all_nodes))) + goto endjob; + + if (virCgroupGetCpusetMems(cgroup_temp, &mem_mask) < 0 || + virCgroupSetCpusetMems(cgroup_temp, all_nodes_str) < 0) + goto endjob; + } + + if (add) { + if (qemuDomainHotplugAddIOThread(driver, vm, iothread_id) < 0) + goto endjob; + } else { + if (qemuDomainHotplugDelIOThread(driver, vm, iothread_id) < 0) + goto endjob; + } + + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + goto endjob; + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (add) { + if (!virDomainIOThreadIDAdd(persistentDef, iothread_id)) + goto endjob; + + persistentDef->iothreads++; + } else { + virDomainIOThreadIDDefPtr iothrid; + if (!(iothrid = virDomainIOThreadIDFind(persistentDef, + iothread_id))) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot find IOThread '%u' in persistent " + "iothreadids"), + iothread_id); + goto cleanup; + } + + if (iothrid->cpumask) + persistentDef->cputune.niothreadspin--; + virDomainIOThreadIDDel(persistentDef, iothread_id); + persistentDef->iothreads--; + } + + if (virDomainSaveConfig(cfg->configDir, persistentDef) < 0) + goto endjob; + } + + ret = 0; + + endjob: + if (mem_mask) { + virErrorPtr err = virSaveLastError(); + virCgroupSetCpusetMems(cgroup_temp, mem_mask); + virSetError(err); + virFreeError(err); + } + qemuDomainObjEndJob(driver, vm); + + cleanup: + VIR_FREE(mem_mask); + VIR_FREE(all_nodes_str); + virBitmapFree(all_nodes); + virCgroupFree(&cgroup_temp); + virObjectUnref(caps); + virObjectUnref(cfg); + return ret; +} + +static int +qemuDomainAddIOThread(virDomainPtr dom, + unsigned int iothread_id, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainAddIOThreadEnsureACL(dom->conn, vm->def, flags) < 0) + goto cleanup; + + ret = qemuDomainChgIOThread(driver, vm, iothread_id, true, flags); + + cleanup: + qemuDomObjEndAPI(&vm); + return ret; +} + + +static int +qemuDomainDelIOThread(virDomainPtr dom, + unsigned int iothread_id, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + int ret = -1; + size_t i; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainDelIOThreadEnsureACL(dom->conn, vm->def, flags) < 0) + goto cleanup; + + /* If there is a disk using the IOThread to be removed, then fail. */ + for (i = 0; i < vm->def->ndisks; i++) { + if (vm->def->disks[i]->iothread == iothread_id) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot remove IOThread %u since it " + "is being used by disk '%s'"), + iothread_id, vm->def->disks[i]->dst); + goto cleanup; + } + } + + ret = qemuDomainChgIOThread(driver, vm, iothread_id, false, flags); + + cleanup: + qemuDomObjEndAPI(&vm); + return ret; +} + static int qemuDomainGetSecurityLabel(virDomainPtr dom, virSecurityLabelPtr seclabel) { virQEMUDriverPtr driver = dom->conn->privateData; @@ -19896,6 +20295,8 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainGetMaxVcpus = qemuDomainGetMaxVcpus, /* 0.4.4 */ .domainGetIOThreadInfo = qemuDomainGetIOThreadInfo, /* 1.2.14 */ .domainPinIOThread = qemuDomainPinIOThread, /* 1.2.14 */ + .domainAddIOThread = qemuDomainAddIOThread, /* 1.2.15 */ + .domainDelIOThread = qemuDomainDelIOThread, /* 1.2.15 */ .domainGetSecurityLabel = qemuDomainGetSecurityLabel, /* 0.6.1 */ .domainGetSecurityLabelList = qemuDomainGetSecurityLabelList, /* 0.10.0 */ .nodeGetSecurityModel = qemuNodeGetSecurityModel, /* 0.6.1 */ -- 2.1.0
On Tue, Apr 21, 2015 at 19:31:29 -0400, John Ferlan wrote:
Add qemuDomainAddIOThread and qemuDomainDelIOThread in order to add or remove an IOThread to/from the host either for live or config optoins
The implementation for the 'live' option will use the iothreadpids list in order to make decision, while the 'config' option will use the iothreadids list. Additionally, for deletion each may have to adjust the iothreadpin list.
IOThreads are implemented by qmp objects, the code makes use of the existing qemuMonitorAddObject or qemuMonitorDelObject APIs.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_audit.c | 9 ++ src/conf/domain_audit.h | 6 + src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 401 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 417 insertions(+)
...
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ee13d08..c34abc9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6137,6 +6137,405 @@ qemuDomainPinIOThread(virDomainPtr dom, return ret; }
+static int +qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int iothread_id) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + char *alias = NULL; + size_t idx; + int rc = -1; + int ret = -1; + unsigned int orig_niothreads = vm->def->iothreads; + unsigned int exp_niothreads = vm->def->iothreads; + int new_niothreads = 0; + qemuMonitorIOThreadInfoPtr *new_iothreads = NULL; + virCgroupPtr cgroup_iothread = NULL; + char *mem_mask = NULL; + virDomainIOThreadIDDefPtr iothrid; + + if (virDomainIOThreadIDFind(vm->def, iothread_id)) { + virReportError(VIR_ERR_INVALID_ARG, + _("an IOThread is already using iothread_id '%u'"), + iothread_id); + goto cleanup; + } + + if (virAsprintf(&alias, "iothread%u", iothread_id) < 0) + return -1; + + qemuDomainObjEnterMonitor(driver, vm); + + rc = qemuMonitorAddObject(priv->mon, "iothread", alias, NULL); + exp_niothreads++; + if (rc < 0) + goto exit_monitor; + + /* After hotplugging the IOThreads we need to re-detect the + * IOThreads thread_id's, adjust the cgroups, thread affinity, + * and add the thread_id to the vm->def->iothreadids list. + */ + if ((new_niothreads = qemuMonitorGetIOThreads(priv->mon, + &new_iothreads)) < 0) + goto exit_monitor; + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + if (new_niothreads != exp_niothreads) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("got wrong number of IOThread ids from QEMU monitor. " + "got %d, wanted %d"), + new_niothreads, exp_niothreads); + vm->def->iothreads = new_niothreads; + goto cleanup; + } + vm->def->iothreads = exp_niothreads; + + if (virDomainNumatuneGetMode(vm->def->numa, -1) == + VIR_DOMAIN_NUMATUNE_MEM_STRICT && + virDomainNumatuneMaybeFormatNodeset(vm->def->numa, + priv->autoNodeset, + &mem_mask, -1) < 0) + goto cleanup; + + + /* + * If we've successfully added an IOThread, find out where we added it + * in the QEMU IOThread list, so we can add it to our iothreadids list + */ + for (idx = 0; idx < new_niothreads; idx++) { + if (STREQ(new_iothreads[idx]->name, alias)) + break; + } + + if (idx == new_niothreads) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find new IOThread '%u' in QEMU monitor."), + iothread_id); + goto cleanup; + } + + if (!(iothrid = virDomainIOThreadIDAdd(vm->def, iothread_id))) + goto cleanup; + + iothrid->thread_id = new_iothreads[idx]->thread_id; + + /* Add IOThread to cgroup if present */ + if (priv->cgroup) { + cgroup_iothread = + qemuDomainAddCgroupForThread(priv->cgroup, + VIR_CGROUP_THREAD_IOTHREAD, + iothread_id, mem_mask, + iothrid->thread_id); + if (!cgroup_iothread) + goto cleanup; + } + + /* Inherit def->cpuset */ + if (vm->def->cpumask) { + if (!(iothrid->cpumask = virBitmapNewCopy(vm->def->cpumask))) + goto cleanup;
Copying the existing default mask is not necessary here. You only have to set it now. It will be set automatically the next time the VM will start. Also you still didn't add the case where automatic numa placement is used.
+ vm->def->cputune.niothreadspin++;
As said, this shouldn't be necessary.
+ + if (qemuDomainHotplugPinThread(iothrid->cpumask, iothread_id, + iothrid->thread_id, cgroup_iothread) < 0) + goto cleanup; + + if (qemuProcessSetSchedParams(iothread_id, iothrid->thread_id, + vm->def->cputune.niothreadsched, + vm->def->cputune.iothreadsched) < 0) + goto cleanup;
As I've pointed out in my last review: <cite> qemuProcessSetSchedParams won't do anything since the new thread doesn't have any scheduler assigned. </cite> For your reply to that comment: So it's wrong in qemuDomainHotplugVcpus ? Is someone else fixing that? Well, yes. This isn't qemuDomainHotplugVcpus though so why to add the same mistake? Existing code is no golden standard for how things should be done.
+ } + + ret = 0; + + cleanup: + if (new_iothreads) { + for (idx = 0; idx < new_niothreads; idx++) + qemuMonitorIOThreadInfoFree(new_iothreads[idx]); + VIR_FREE(new_iothreads); + } + VIR_FREE(mem_mask); + virDomainAuditIOThread(vm, orig_niothreads, new_niothreads, + "update", rc == 0); + virCgroupFree(&cgroup_iothread); + VIR_FREE(alias); + return ret; + + exit_monitor: + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; +} + +static int +qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int iothread_id) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + size_t idx; + char *alias = NULL; + int rc = -1; + int ret = -1; + unsigned int orig_niothreads = vm->def->iothreads; + unsigned int exp_niothreads = vm->def->iothreads; + int new_niothreads = 0; + qemuMonitorIOThreadInfoPtr *new_iothreads = NULL; + + /* Normally would use virDomainIOThreadIDFind, but we need the index + * from whence to delete for later... + */ + for (idx = 0; idx < vm->def->niothreadids; idx++) { + if (iothread_id == vm->def->iothreadids[idx]->iothread_id) + break; + } + + if (idx == vm->def->niothreadids) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot find IOThread '%u' in iothreadids list"), + iothread_id); + return -1; + } + + if (virAsprintf(&alias, "iothread%u", iothread_id) < 0) + return -1; + + qemuDomainObjEnterMonitor(driver, vm); + + rc = qemuMonitorDelObject(priv->mon, alias); + exp_niothreads--; + if (rc < 0) + goto exit_monitor; + + if ((new_niothreads = qemuMonitorGetIOThreads(priv->mon, + &new_iothreads)) < 0) + goto exit_monitor; + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + if (new_niothreads != exp_niothreads) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("got wrong number of IOThread ids from QEMU monitor. " + "got %d, wanted %d"), + new_niothreads, exp_niothreads); + vm->def->iothreads = new_niothreads; + goto cleanup; + } + vm->def->iothreads = exp_niothreads; + + if (vm->def->iothreadids[idx]->cpumask) + vm->def->cputune.niothreadspin--;
This shouldn't be necessary after you fix the check for formating cputune.
+ virDomainIOThreadIDDel(vm->def, iothread_id); + + if (qemuDomainDelCgroupForThread(priv->cgroup, + VIR_CGROUP_THREAD_IOTHREAD, + iothread_id) < 0) + goto cleanup;
Here you also should remove the corresponding bit from the iothread scheduler list since you are removign the bit. Ideally you merge that info here too. Note that it won't be that easy since it's stored in an orthogonal fashion, which terribly sucks, but it would be great to have all the data in single place.
+ + ret = 0; + + cleanup: + if (new_iothreads) { + for (idx = 0; idx < new_niothreads; idx++) + qemuMonitorIOThreadInfoFree(new_iothreads[idx]); + VIR_FREE(new_iothreads); + } + virDomainAuditIOThread(vm, orig_niothreads, new_niothreads, + "update", rc == 0); + VIR_FREE(alias); + return ret; + + exit_monitor: + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; +} + +static int +qemuDomainChgIOThread(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int iothread_id, + bool add, + unsigned int flags) +{ + virQEMUDriverConfigPtr cfg = NULL; + virCapsPtr caps = NULL; + qemuDomainObjPrivatePtr priv; + virCgroupPtr cgroup_temp = NULL; + virBitmapPtr all_nodes = NULL; + char *all_nodes_str = NULL; + char *mem_mask = NULL; + virDomainDefPtr persistentDef; + int ret = -1; + + cfg = virQEMUDriverGetConfig(driver); + + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + priv = vm->privateData; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, + &persistentDef) < 0) + goto endjob; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot change IOThreads for an inactive domain")); + goto endjob; + } + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IOThreads not supported with this binary")); + goto endjob; + } + + if (virNumaIsAvailable()) { + if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0, + false, &cgroup_temp) < 0) + goto endjob; + + if (!(all_nodes = virNumaGetHostNodeset())) + goto endjob; + + if (!(all_nodes_str = virBitmapFormat(all_nodes))) + goto endjob; + + if (virCgroupGetCpusetMems(cgroup_temp, &mem_mask) < 0 || + virCgroupSetCpusetMems(cgroup_temp, all_nodes_str) < 0) + goto endjob;
One more example that blatant copying of the code is a terrible idea. This whole block was added to fix the following problem: commit e3435caf6af41748204e542dee13ede8441d88c0 Author: Martin Kletzander <mkletzan@redhat.com> Date: Fri Dec 12 15:36:45 2014 +0100 qemu: Fix hotplugging cpus with strict memory pinning When hot-plugging a VCPU into the guest, kvm needs to allocate some data from the DMA zone, which might be in a memory node that's not allowed in cpuset.mems. Basically the same problem as there was with starting the domain and due to which commit 7e72ac787848b7434c9359a57c1e2789d92350f8 exists. This patch just extends it to hotplugging as well. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1161540 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> Since it is apparently not necessary when doing iothread hotplug please remove it.
+ } + + if (add) { + if (qemuDomainHotplugAddIOThread(driver, vm, iothread_id) < 0) + goto endjob; + } else { + if (qemuDomainHotplugDelIOThread(driver, vm, iothread_id) < 0) + goto endjob; + } + + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + goto endjob; + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (add) { + if (!virDomainIOThreadIDAdd(persistentDef, iothread_id)) + goto endjob; + + persistentDef->iothreads++; + } else { + virDomainIOThreadIDDefPtr iothrid; + if (!(iothrid = virDomainIOThreadIDFind(persistentDef, + iothread_id))) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot find IOThread '%u' in persistent " + "iothreadids"), + iothread_id); + goto cleanup; + } + + if (iothrid->cpumask) + persistentDef->cputune.niothreadspin--; + virDomainIOThreadIDDel(persistentDef, iothread_id); + persistentDef->iothreads--; + } + + if (virDomainSaveConfig(cfg->configDir, persistentDef) < 0) + goto endjob; + } + + ret = 0; + + endjob: + if (mem_mask) { + virErrorPtr err = virSaveLastError(); + virCgroupSetCpusetMems(cgroup_temp, mem_mask); + virSetError(err); + virFreeError(err); + } + qemuDomainObjEndJob(driver, vm); + + cleanup: + VIR_FREE(mem_mask); + VIR_FREE(all_nodes_str); + virBitmapFree(all_nodes); + virCgroupFree(&cgroup_temp); + virObjectUnref(caps); + virObjectUnref(cfg); + return ret; +} +
Peter
On 04/23/2015 09:22 AM, Peter Krempa wrote:
On Tue, Apr 21, 2015 at 19:31:29 -0400, John Ferlan wrote:
Add qemuDomainAddIOThread and qemuDomainDelIOThread in order to add or remove an IOThread to/from the host either for live or config optoins
The implementation for the 'live' option will use the iothreadpids list in order to make decision, while the 'config' option will use the iothreadids list. Additionally, for deletion each may have to adjust the iothreadpin list.
IOThreads are implemented by qmp objects, the code makes use of the existing qemuMonitorAddObject or qemuMonitorDelObject APIs.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_audit.c | 9 ++ src/conf/domain_audit.h | 6 + src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 401 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 417 insertions(+)
...
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ee13d08..c34abc9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6137,6 +6137,405 @@ qemuDomainPinIOThread(virDomainPtr dom, return ret; }
+static int +qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int iothread_id) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + char *alias = NULL; + size_t idx; + int rc = -1; + int ret = -1; + unsigned int orig_niothreads = vm->def->iothreads; + unsigned int exp_niothreads = vm->def->iothreads; + int new_niothreads = 0; + qemuMonitorIOThreadInfoPtr *new_iothreads = NULL; + virCgroupPtr cgroup_iothread = NULL; + char *mem_mask = NULL; + virDomainIOThreadIDDefPtr iothrid; + + if (virDomainIOThreadIDFind(vm->def, iothread_id)) { + virReportError(VIR_ERR_INVALID_ARG, + _("an IOThread is already using iothread_id '%u'"), + iothread_id); + goto cleanup; + } + + if (virAsprintf(&alias, "iothread%u", iothread_id) < 0) + return -1; + + qemuDomainObjEnterMonitor(driver, vm); + + rc = qemuMonitorAddObject(priv->mon, "iothread", alias, NULL); + exp_niothreads++; + if (rc < 0) + goto exit_monitor; + + /* After hotplugging the IOThreads we need to re-detect the + * IOThreads thread_id's, adjust the cgroups, thread affinity, + * and add the thread_id to the vm->def->iothreadids list. + */ + if ((new_niothreads = qemuMonitorGetIOThreads(priv->mon, + &new_iothreads)) < 0) + goto exit_monitor; + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + if (new_niothreads != exp_niothreads) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("got wrong number of IOThread ids from QEMU monitor. " + "got %d, wanted %d"), + new_niothreads, exp_niothreads); + vm->def->iothreads = new_niothreads; + goto cleanup; + } + vm->def->iothreads = exp_niothreads; + + if (virDomainNumatuneGetMode(vm->def->numa, -1) == + VIR_DOMAIN_NUMATUNE_MEM_STRICT && + virDomainNumatuneMaybeFormatNodeset(vm->def->numa, + priv->autoNodeset, + &mem_mask, -1) < 0) + goto cleanup; + + + /* + * If we've successfully added an IOThread, find out where we added it + * in the QEMU IOThread list, so we can add it to our iothreadids list + */ + for (idx = 0; idx < new_niothreads; idx++) { + if (STREQ(new_iothreads[idx]->name, alias)) + break; + } + + if (idx == new_niothreads) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find new IOThread '%u' in QEMU monitor."), + iothread_id); + goto cleanup; + } + + if (!(iothrid = virDomainIOThreadIDAdd(vm->def, iothread_id))) + goto cleanup; + + iothrid->thread_id = new_iothreads[idx]->thread_id; + + /* Add IOThread to cgroup if present */ + if (priv->cgroup) { + cgroup_iothread = + qemuDomainAddCgroupForThread(priv->cgroup, + VIR_CGROUP_THREAD_IOTHREAD, + iothread_id, mem_mask, + iothrid->thread_id); + if (!cgroup_iothread) + goto cleanup; + } + + /* Inherit def->cpuset */ + if (vm->def->cpumask) { + if (!(iothrid->cpumask = virBitmapNewCopy(vm->def->cpumask))) + goto cleanup;
Copying the existing default mask is not necessary here. You only have to set it now. It will be set automatically the next time the VM will start.
Also you still didn't add the case where automatic numa placement is used.
Perhaps because I'm not clear what you're asking for. Obviously I'm comparing to HotplugVcpus
+ vm->def->cputune.niothreadspin++;
As said, this shouldn't be necessary.
It's been removed
+ + if (qemuDomainHotplugPinThread(iothrid->cpumask, iothread_id, + iothrid->thread_id, cgroup_iothread) < 0) + goto cleanup; + + if (qemuProcessSetSchedParams(iothread_id, iothrid->thread_id, + vm->def->cputune.niothreadsched, + vm->def->cputune.iothreadsched) < 0) + goto cleanup;
As I've pointed out in my last review: <cite> qemuProcessSetSchedParams won't do anything since the new thread doesn't have any scheduler assigned. </cite>
For your reply to that comment: So it's wrong in qemuDomainHotplugVcpus ? Is someone else fixing that?
Well, yes. This isn't qemuDomainHotplugVcpus though so why to add the same mistake? Existing code is no golden standard for how things should be done.
So if we know existing code is broken, shouldn't it be fixed? That way it prevents someone from doing something wrong. While existing code isn't a "golden standard" - it does provide a working example for others to follow if they're implementing something very similar to what currently exists. I don't mind removing it - fine, whatever, but is someone going to fix the HotplugVcpus as well? Or should I add a patch 10 that does that?
+ } + + ret = 0; + + cleanup: + if (new_iothreads) { + for (idx = 0; idx < new_niothreads; idx++) + qemuMonitorIOThreadInfoFree(new_iothreads[idx]); + VIR_FREE(new_iothreads); + } + VIR_FREE(mem_mask); + virDomainAuditIOThread(vm, orig_niothreads, new_niothreads, + "update", rc == 0); + virCgroupFree(&cgroup_iothread); + VIR_FREE(alias); + return ret; + + exit_monitor: + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; +} + +static int +qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int iothread_id) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + size_t idx; + char *alias = NULL; + int rc = -1; + int ret = -1; + unsigned int orig_niothreads = vm->def->iothreads; + unsigned int exp_niothreads = vm->def->iothreads; + int new_niothreads = 0; + qemuMonitorIOThreadInfoPtr *new_iothreads = NULL; + + /* Normally would use virDomainIOThreadIDFind, but we need the index + * from whence to delete for later... + */ + for (idx = 0; idx < vm->def->niothreadids; idx++) { + if (iothread_id == vm->def->iothreadids[idx]->iothread_id) + break; + } + + if (idx == vm->def->niothreadids) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot find IOThread '%u' in iothreadids list"), + iothread_id); + return -1; + } + + if (virAsprintf(&alias, "iothread%u", iothread_id) < 0) + return -1; + + qemuDomainObjEnterMonitor(driver, vm); + + rc = qemuMonitorDelObject(priv->mon, alias); + exp_niothreads--; + if (rc < 0) + goto exit_monitor; + + if ((new_niothreads = qemuMonitorGetIOThreads(priv->mon, + &new_iothreads)) < 0) + goto exit_monitor; + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + if (new_niothreads != exp_niothreads) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("got wrong number of IOThread ids from QEMU monitor. " + "got %d, wanted %d"), + new_niothreads, exp_niothreads); + vm->def->iothreads = new_niothreads; + goto cleanup; + } + vm->def->iothreads = exp_niothreads; + + if (vm->def->iothreadids[idx]->cpumask) + vm->def->cputune.niothreadspin--;
This shouldn't be necessary after you fix the check for formating cputune.
Right
+ virDomainIOThreadIDDel(vm->def, iothread_id); + + if (qemuDomainDelCgroupForThread(priv->cgroup, + VIR_CGROUP_THREAD_IOTHREAD, + iothread_id) < 0) + goto cleanup;
Here you also should remove the corresponding bit from the iothread scheduler list since you are removign the bit. Ideally you merge that info here too. Note that it won't be that easy since it's stored in an orthogonal fashion, which terribly sucks, but it would be great to have all the data in single place.
The design of *sched is less than optimal to say the least - it made a lot of assumptions and should have a way to remove bits as necessary... Since vcpu's cannot removed "yet" it hasn't mattered. I'll figure something out - it'll probably require a new API for domain_conf.c since the code will need to be used for both active and config
+ + ret = 0; + + cleanup: + if (new_iothreads) { + for (idx = 0; idx < new_niothreads; idx++) + qemuMonitorIOThreadInfoFree(new_iothreads[idx]); + VIR_FREE(new_iothreads); + } + virDomainAuditIOThread(vm, orig_niothreads, new_niothreads, + "update", rc == 0); + VIR_FREE(alias); + return ret; + + exit_monitor: + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; +} + +static int +qemuDomainChgIOThread(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int iothread_id, + bool add, + unsigned int flags) +{ + virQEMUDriverConfigPtr cfg = NULL; + virCapsPtr caps = NULL; + qemuDomainObjPrivatePtr priv; + virCgroupPtr cgroup_temp = NULL; + virBitmapPtr all_nodes = NULL; + char *all_nodes_str = NULL; + char *mem_mask = NULL; + virDomainDefPtr persistentDef; + int ret = -1; + + cfg = virQEMUDriverGetConfig(driver); + + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + priv = vm->privateData; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, + &persistentDef) < 0) + goto endjob; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot change IOThreads for an inactive domain")); + goto endjob; + } + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IOThreads not supported with this binary")); + goto endjob; + } + + if (virNumaIsAvailable()) { + if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0, + false, &cgroup_temp) < 0) + goto endjob; + + if (!(all_nodes = virNumaGetHostNodeset())) + goto endjob; + + if (!(all_nodes_str = virBitmapFormat(all_nodes))) + goto endjob; + + if (virCgroupGetCpusetMems(cgroup_temp, &mem_mask) < 0 || + virCgroupSetCpusetMems(cgroup_temp, all_nodes_str) < 0) + goto endjob;
One more example that blatant copying of the code is a terrible idea. This whole block was added to fix the following problem:
Hmm... perhaps a case where a few extra comments in the vCPU code would have helped the reader understand what was being done without ferreting around git log history to understand it. I'll remove it from the patch. John
commit e3435caf6af41748204e542dee13ede8441d88c0 Author: Martin Kletzander <mkletzan@redhat.com> Date: Fri Dec 12 15:36:45 2014 +0100
qemu: Fix hotplugging cpus with strict memory pinning
When hot-plugging a VCPU into the guest, kvm needs to allocate some data from the DMA zone, which might be in a memory node that's not allowed in cpuset.mems. Basically the same problem as there was with starting the domain and due to which commit 7e72ac787848b7434c9359a57c1e2789d92350f8 exists. This patch just extends it to hotplugging as well.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1161540
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Since it is apparently not necessary when doing iothread hotplug please remove it.
+ } + + if (add) { + if (qemuDomainHotplugAddIOThread(driver, vm, iothread_id) < 0) + goto endjob; + } else { + if (qemuDomainHotplugDelIOThread(driver, vm, iothread_id) < 0) + goto endjob; + } + + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + goto endjob; + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (add) { + if (!virDomainIOThreadIDAdd(persistentDef, iothread_id)) + goto endjob; + + persistentDef->iothreads++; + } else { + virDomainIOThreadIDDefPtr iothrid; + if (!(iothrid = virDomainIOThreadIDFind(persistentDef, + iothread_id))) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot find IOThread '%u' in persistent " + "iothreadids"), + iothread_id); + goto cleanup; + } + + if (iothrid->cpumask) + persistentDef->cputune.niothreadspin--; + virDomainIOThreadIDDel(persistentDef, iothread_id); + persistentDef->iothreads--; + } + + if (virDomainSaveConfig(cfg->configDir, persistentDef) < 0) + goto endjob; + } + + ret = 0; + + endjob: + if (mem_mask) { + virErrorPtr err = virSaveLastError(); + virCgroupSetCpusetMems(cgroup_temp, mem_mask); + virSetError(err); + virFreeError(err); + } + qemuDomainObjEndJob(driver, vm); + + cleanup: + VIR_FREE(mem_mask); + VIR_FREE(all_nodes_str); + virBitmapFree(all_nodes); + virCgroupFree(&cgroup_temp); + virObjectUnref(caps); + virObjectUnref(cfg); + return ret; +} +
Peter
Add qemuDomainAddIOThread and qemuDomainDelIOThread in order to add or remove an IOThread to/from the host either for live or config optoins The implementation for the 'live' option will use the iothreadpids list in order to make decision, while the 'config' option will use the iothreadids list. Additionally, for deletion each may have to adjust the iothreadpin list. IOThreads are implemented by qmp objects, the code makes use of the existing qemuMonitorAddObject or qemuMonitorDelObject APIs. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_audit.c | 9 ++ src/conf/domain_audit.h | 6 + src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 375 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 391 insertions(+) diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 3e93d97..4ea10d2 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -790,6 +790,15 @@ virDomainAuditVcpu(virDomainObjPtr vm, return virDomainAuditResource(vm, "vcpu", oldvcpu, newvcpu, reason, success); } +void +virDomainAuditIOThread(virDomainObjPtr vm, + unsigned int oldiothread, unsigned int newiothread, + const char *reason, bool success) +{ + return virDomainAuditResource(vm, "iothread", oldiothread, newiothread, + reason, success); +} + static void virDomainAuditLifecycle(virDomainObjPtr vm, const char *op, const char *reason, bool success) diff --git a/src/conf/domain_audit.h b/src/conf/domain_audit.h index 4c1ef90..97dadca 100644 --- a/src/conf/domain_audit.h +++ b/src/conf/domain_audit.h @@ -102,6 +102,12 @@ void virDomainAuditVcpu(virDomainObjPtr vm, const char *reason, bool success) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); +void virDomainAuditIOThread(virDomainObjPtr vm, + unsigned int oldiothread, + unsigned int newiothread, + const char *reason, + bool success) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); void virDomainAuditSecurityLabel(virDomainObjPtr vm, bool success) ATTRIBUTE_NONNULL(1); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0877907..2f72636 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -127,6 +127,7 @@ virDomainAuditDisk; virDomainAuditFS; virDomainAuditHostdev; virDomainAuditInit; +virDomainAuditIOThread; virDomainAuditMemory; virDomainAuditNet; virDomainAuditNetDevice; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f5216bd..56d953f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6155,6 +6155,379 @@ qemuDomainPinIOThread(virDomainPtr dom, return ret; } +static int +qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int iothread_id) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + char *alias = NULL; + size_t idx; + int rc = -1; + int ret = -1; + unsigned int orig_niothreads = vm->def->iothreads; + unsigned int exp_niothreads = vm->def->iothreads; + int new_niothreads = 0; + qemuMonitorIOThreadInfoPtr *new_iothreads = NULL; + virCgroupPtr cgroup_iothread = NULL; + char *mem_mask = NULL; + virDomainIOThreadIDDefPtr iothrid; + + if (virDomainIOThreadIDFind(vm->def, iothread_id)) { + virReportError(VIR_ERR_INVALID_ARG, + _("an IOThread is already using iothread_id '%u'"), + iothread_id); + goto cleanup; + } + + if (virAsprintf(&alias, "iothread%u", iothread_id) < 0) + return -1; + + qemuDomainObjEnterMonitor(driver, vm); + + rc = qemuMonitorAddObject(priv->mon, "iothread", alias, NULL); + exp_niothreads++; + if (rc < 0) + goto exit_monitor; + + /* After hotplugging the IOThreads we need to re-detect the + * IOThreads thread_id's, adjust the cgroups, thread affinity, + * and add the thread_id to the vm->def->iothreadids list. + */ + if ((new_niothreads = qemuMonitorGetIOThreads(priv->mon, + &new_iothreads)) < 0) + goto exit_monitor; + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + if (new_niothreads != exp_niothreads) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("got wrong number of IOThread ids from QEMU monitor. " + "got %d, wanted %d"), + new_niothreads, exp_niothreads); + vm->def->iothreads = new_niothreads; + goto cleanup; + } + vm->def->iothreads = exp_niothreads; + + if (virDomainNumatuneGetMode(vm->def->numa, -1) == + VIR_DOMAIN_NUMATUNE_MEM_STRICT && + virDomainNumatuneMaybeFormatNodeset(vm->def->numa, + priv->autoNodeset, + &mem_mask, -1) < 0) + goto cleanup; + + + /* + * If we've successfully added an IOThread, find out where we added it + * in the QEMU IOThread list, so we can add it to our iothreadids list + */ + for (idx = 0; idx < new_niothreads; idx++) { + if (STREQ(new_iothreads[idx]->name, alias)) + break; + } + + if (idx == new_niothreads) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find new IOThread '%u' in QEMU monitor."), + iothread_id); + goto cleanup; + } + + if (!(iothrid = virDomainIOThreadIDAdd(vm->def, iothread_id))) + goto cleanup; + + iothrid->thread_id = new_iothreads[idx]->thread_id; + + /* Add IOThread to cgroup if present */ + if (priv->cgroup) { + cgroup_iothread = + qemuDomainAddCgroupForThread(priv->cgroup, + VIR_CGROUP_THREAD_IOTHREAD, + iothread_id, mem_mask, + iothrid->thread_id); + if (!cgroup_iothread) + goto cleanup; + } + + /* Inherit def->cpuset */ + if (vm->def->cpumask) { + if (qemuDomainHotplugPinThread(vm->def->cpumask, iothread_id, + iothrid->thread_id, cgroup_iothread) < 0) + goto cleanup; + } + + ret = 0; + + cleanup: + if (new_iothreads) { + for (idx = 0; idx < new_niothreads; idx++) + qemuMonitorIOThreadInfoFree(new_iothreads[idx]); + VIR_FREE(new_iothreads); + } + VIR_FREE(mem_mask); + virDomainAuditIOThread(vm, orig_niothreads, new_niothreads, + "update", rc == 0); + virCgroupFree(&cgroup_iothread); + VIR_FREE(alias); + return ret; + + exit_monitor: + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; +} + +static int +qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int iothread_id) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + size_t idx; + char *alias = NULL; + int rc = -1; + int ret = -1; + unsigned int orig_niothreads = vm->def->iothreads; + unsigned int exp_niothreads = vm->def->iothreads; + int new_niothreads = 0; + qemuMonitorIOThreadInfoPtr *new_iothreads = NULL; + + /* Normally would use virDomainIOThreadIDFind, but we need the index + * from whence to delete for later... + */ + for (idx = 0; idx < vm->def->niothreadids; idx++) { + if (iothread_id == vm->def->iothreadids[idx]->iothread_id) + break; + } + + if (idx == vm->def->niothreadids) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot find IOThread '%u' in iothreadids list"), + iothread_id); + return -1; + } + + if (virAsprintf(&alias, "iothread%u", iothread_id) < 0) + return -1; + + qemuDomainObjEnterMonitor(driver, vm); + + rc = qemuMonitorDelObject(priv->mon, alias); + exp_niothreads--; + if (rc < 0) + goto exit_monitor; + + if ((new_niothreads = qemuMonitorGetIOThreads(priv->mon, + &new_iothreads)) < 0) + goto exit_monitor; + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + if (new_niothreads != exp_niothreads) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("got wrong number of IOThread ids from QEMU monitor. " + "got %d, wanted %d"), + new_niothreads, exp_niothreads); + vm->def->iothreads = new_niothreads; + goto cleanup; + } + vm->def->iothreads = exp_niothreads; + + virDomainIOThreadIDDel(vm->def, iothread_id); + + virDomainIOThreadSchedDelId(vm->def, iothread_id); + + if (qemuDomainDelCgroupForThread(priv->cgroup, + VIR_CGROUP_THREAD_IOTHREAD, + iothread_id) < 0) + goto cleanup; + + ret = 0; + + cleanup: + if (new_iothreads) { + for (idx = 0; idx < new_niothreads; idx++) + qemuMonitorIOThreadInfoFree(new_iothreads[idx]); + VIR_FREE(new_iothreads); + } + virDomainAuditIOThread(vm, orig_niothreads, new_niothreads, + "update", rc == 0); + VIR_FREE(alias); + return ret; + + exit_monitor: + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; +} + +static int +qemuDomainChgIOThread(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int iothread_id, + bool add, + unsigned int flags) +{ + virQEMUDriverConfigPtr cfg = NULL; + virCapsPtr caps = NULL; + qemuDomainObjPrivatePtr priv; + virCgroupPtr cgroup_temp = NULL; + virBitmapPtr all_nodes = NULL; + char *all_nodes_str = NULL; + char *mem_mask = NULL; + virDomainDefPtr persistentDef; + int ret = -1; + + cfg = virQEMUDriverGetConfig(driver); + + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + priv = vm->privateData; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, + &persistentDef) < 0) + goto endjob; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot change IOThreads for an inactive domain")); + goto endjob; + } + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IOThreads not supported with this binary")); + goto endjob; + } + + if (add) { + if (qemuDomainHotplugAddIOThread(driver, vm, iothread_id) < 0) + goto endjob; + } else { + if (qemuDomainHotplugDelIOThread(driver, vm, iothread_id) < 0) + goto endjob; + } + + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + goto endjob; + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (add) { + if (!virDomainIOThreadIDAdd(persistentDef, iothread_id)) + goto endjob; + + persistentDef->iothreads++; + } else { + virDomainIOThreadIDDefPtr iothrid; + if (!(iothrid = virDomainIOThreadIDFind(persistentDef, + iothread_id))) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot find IOThread '%u' in persistent " + "iothreadids"), + iothread_id); + goto cleanup; + } + + virDomainIOThreadIDDel(persistentDef, iothread_id); + virDomainIOThreadSchedDelId(persistentDef, iothread_id); + persistentDef->iothreads--; + } + + if (virDomainSaveConfig(cfg->configDir, persistentDef) < 0) + goto endjob; + } + + ret = 0; + + endjob: + if (mem_mask) { + virErrorPtr err = virSaveLastError(); + virCgroupSetCpusetMems(cgroup_temp, mem_mask); + virSetError(err); + virFreeError(err); + } + qemuDomainObjEndJob(driver, vm); + + cleanup: + VIR_FREE(mem_mask); + VIR_FREE(all_nodes_str); + virBitmapFree(all_nodes); + virCgroupFree(&cgroup_temp); + virObjectUnref(caps); + virObjectUnref(cfg); + return ret; +} + +static int +qemuDomainAddIOThread(virDomainPtr dom, + unsigned int iothread_id, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainAddIOThreadEnsureACL(dom->conn, vm->def, flags) < 0) + goto cleanup; + + ret = qemuDomainChgIOThread(driver, vm, iothread_id, true, flags); + + cleanup: + qemuDomObjEndAPI(&vm); + return ret; +} + + +static int +qemuDomainDelIOThread(virDomainPtr dom, + unsigned int iothread_id, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + int ret = -1; + size_t i; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainDelIOThreadEnsureACL(dom->conn, vm->def, flags) < 0) + goto cleanup; + + /* If there is a disk using the IOThread to be removed, then fail. */ + for (i = 0; i < vm->def->ndisks; i++) { + if (vm->def->disks[i]->iothread == iothread_id) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot remove IOThread %u since it " + "is being used by disk '%s'"), + iothread_id, vm->def->disks[i]->dst); + goto cleanup; + } + } + + ret = qemuDomainChgIOThread(driver, vm, iothread_id, false, flags); + + cleanup: + qemuDomObjEndAPI(&vm); + return ret; +} + static int qemuDomainGetSecurityLabel(virDomainPtr dom, virSecurityLabelPtr seclabel) { virQEMUDriverPtr driver = dom->conn->privateData; @@ -19914,6 +20287,8 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainGetMaxVcpus = qemuDomainGetMaxVcpus, /* 0.4.4 */ .domainGetIOThreadInfo = qemuDomainGetIOThreadInfo, /* 1.2.14 */ .domainPinIOThread = qemuDomainPinIOThread, /* 1.2.14 */ + .domainAddIOThread = qemuDomainAddIOThread, /* 1.2.15 */ + .domainDelIOThread = qemuDomainDelIOThread, /* 1.2.15 */ .domainGetSecurityLabel = qemuDomainGetSecurityLabel, /* 0.6.1 */ .domainGetSecurityLabelList = qemuDomainGetSecurityLabelList, /* 0.10.0 */ .nodeGetSecurityModel = qemuNodeGetSecurityModel, /* 0.6.1 */ -- 2.1.0
https://bugzilla.redhat.com/show_bug.cgi?id=1161617 Add command to allow adding and removing IOThreads from the domain including the configuration and live domain. $ virsh iothreadadd --help NAME iothreadadd - add an IOThread to the guest domain SYNOPSIS iothreadadd <domain> <id> [--config] [--live] [--current] DESCRIPTION Add an IOThread to the guest domain. OPTIONS [--domain] <string> domain name, id or uuid [--id] <number> iothread for the new IOThread --config affect next boot --live affect running domain --current affect current domain $ virsh iothreaddel --help NAME iothreaddel - delete an IOThread from the guest domain SYNOPSIS iothreaddel <domain> <id> [--config] [--live] [--current] DESCRIPTION Delete an IOThread from the guest domain. OPTIONS [--domain] <string> domain name, id or uuid [--id] <number> iothread_id for the IOThread to delete --config affect next boot --live affect running domain --current affect current domain Assuming a running $dom with multiple IOThreads assigned and that that the $dom has disks assigned to IOThread 1 and IOThread 2: $ virsh iothreadinfo $dom IOThread ID CPU Affinity --------------------------------------------------- 1 2 2 3 3 0-1 $ virsh iothreadadd $dom 1 error: invalid argument: an IOThread is already using iothread_id '1' in iothreadpids $ virsh iothreadadd $dom 1 --config error: invalid argument: an IOThread is already using iothread_id '1' in persistent iothreadids $ virsh iothreadadd $dom 4 $ virsh iothreadinfo $dom IOThread ID CPU Affinity --------------------------------------------------- 1 2 2 3 3 0-1 4 0-3 $ virsh iothreadinfo $dom --config IOThread ID CPU Affinity --------------------------------------------------- 1 2 2 3 3 0-1 $ virsh iothreadadd $dom 4 --config $ virsh iothreadinfo $dom --config IOThread ID CPU Affinity --------------------------------------------------- 1 2 2 3 3 0-1 4 0-3 Assuming the same original configuration $ virsh iothreaddel $dom 1 error: invalid argument: cannot remove IOThread 1 since it is being used by disk path '/home/vm-images/f18' $ virsh iothreaddel $dom 3 $ virsh iothreadinfo $dom IOThread ID CPU Affinity --------------------------------------------------- 1 2 2 3 $ virsh iothreadinfo $dom --config IOThread ID CPU Affinity --------------------------------------------------- 1 2 2 3 3 0-1 Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 164 +++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 31 ++++++++++ 2 files changed, 195 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 4f58f07..3c9cd64 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6880,6 +6880,158 @@ cmdIOThreadPin(vshControl *ctl, const vshCmd *cmd) } /* + * "iothreadadd" command + */ +static const vshCmdInfo info_iothreadadd[] = { + {.name = "help", + .data = N_("add an IOThread to the guest domain") + }, + {.name = "desc", + .data = N_("Add an IOThread to the guest domain.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_iothreadadd[] = { + {.name = "domain", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("domain name, id or uuid") + }, + {.name = "id", + .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ, + .help = N_("iothread for the new IOThread") + }, + {.name = "config", + .type = VSH_OT_BOOL, + .help = N_("affect next boot") + }, + {.name = "live", + .type = VSH_OT_BOOL, + .help = N_("affect running domain") + }, + {.name = "current", + .type = VSH_OT_BOOL, + .help = N_("affect current domain") + }, + {.name = NULL} +}; + +static bool +cmdIOThreadAdd(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom; + int iothread_id = 0; + bool ret = false; + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool current = vshCommandOptBool(cmd, "current"); + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (vshCommandOptInt(cmd, "id", &iothread_id) < 0 || iothread_id <= 0) { + vshError(ctl, _("Invalid IOThread id value: '%d'"), iothread_id); + goto cleanup; + } + + if (virDomainAddIOThread(dom, iothread_id, flags) < 0) + goto cleanup; + + ret = true; + + cleanup: + virDomainFree(dom); + return ret; +} + +/* + * "iothreaddel" command + */ +static const vshCmdInfo info_iothreaddel[] = { + {.name = "help", + .data = N_("delete an IOThread from the guest domain") + }, + {.name = "desc", + .data = N_("Delete an IOThread from the guest domain.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_iothreaddel[] = { + {.name = "domain", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("domain name, id or uuid") + }, + {.name = "id", + .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ, + .help = N_("iothread_id for the IOThread to delete") + }, + {.name = "config", + .type = VSH_OT_BOOL, + .help = N_("affect next boot") + }, + {.name = "live", + .type = VSH_OT_BOOL, + .help = N_("affect running domain") + }, + {.name = "current", + .type = VSH_OT_BOOL, + .help = N_("affect current domain") + }, + {.name = NULL} +}; + +static bool +cmdIOThreadDel(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom; + int iothread_id = 0; + bool ret = false; + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool current = vshCommandOptBool(cmd, "current"); + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (vshCommandOptInt(cmd, "id", &iothread_id) < 0 || iothread_id <= 0) { + vshError(ctl, _("Invalid IOThread id value: '%d'"), iothread_id); + goto cleanup; + } + + if (virDomainDelIOThread(dom, iothread_id, flags) < 0) + goto cleanup; + + ret = true; + + cleanup: + virDomainFree(dom); + return ret; +} + +/* * "cpu-compare" command */ static const vshCmdInfo info_cpu_compare[] = { @@ -12829,6 +12981,18 @@ const vshCmdDef domManagementCmds[] = { .info = info_iothreadpin, .flags = 0 }, + {.name = "iothreadadd", + .handler = cmdIOThreadAdd, + .opts = opts_iothreadadd, + .info = info_iothreadadd, + .flags = 0 + }, + {.name = "iothreaddel", + .handler = cmdIOThreadDel, + .opts = opts_iothreaddel, + .info = info_iothreaddel, + .flags = 0 + }, {.name = "send-key", .handler = cmdSendKey, .opts = opts_send_key, diff --git a/tools/virsh.pod b/tools/virsh.pod index d642a69..091abc6 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1416,6 +1416,37 @@ If no flag is specified, behavior is different depending on hypervisor. B<Note>: The expression is sequentially evaluated, so "0-15,^8" is identical to "9-14,0-7,15" but not identical to "^8,0-15". +=item B<iothreadadd> I<domain> I<iothread_id> +[[I<--config>] [I<--live>] | [I<--current>]] + +Add a new IOThread to the domain using the specified I<iothread_id>. +If the I<iothread_id> already exists, the command will fail. The +I<iothread_id> must be greater than zero. + +If I<--live> is specified, affect a running guest. If the guest is not +running an error is returned. +If I<--config> is specified, affect the next boot of a persistent guest. +If I<--current> is specified or I<--live> and I<--config> are not specified, +affect the current guest state. +Both I<--live> and I<--config> flags may be given, but if not flag is +specified, behavior is different depending on hypervisor. + +=item B<iothreaddel> I<domain> I<iothread_id> +[[I<--config>] [I<--live>] | [I<--current>]] + +Delete an IOThread from the domain using the specified I<iothread_id>. +If an IOThread is currently assigned to a disk resource such as via the +B<attach-disk> command, then the attempt to remove the IOThread will fail. +If the I<iothread_id> does not exist an error will occur. + +If I<--live> is specified, affect a running guest. If the guest is not +running an error is returned. +If I<--config> is specified, affect the next boot of a persistent guest. +If I<--current> is specified or I<--live> and I<--config> are not specified, +affect the current guest state. +Both I<--live> and I<--config> flags may be given, but if not flag is +specified, behavior is different depending on hypervisor. + =item B<managedsave> I<domain> [I<--bypass-cache>] [{I<--running> | I<--paused>}] [I<--verbose>] -- 2.1.0
Adding a new XML element 'iothreadids' in order to allow defining specific IOThread ID's rather than relying on the algorithm to assign IOThread ID's starting at 1 and incrementing to iothreads count. This will allow future patches to be able to add new IOThreads by a specific iothread_id and of course delete any exisiting IOThread. Each iothreadids element will have 'n' <iothread> children elements which will have attribute "id". The "id" will allow for definition of any "valid" (eg > 0) iothread_id value. On input, if any <iothreadids> <iothread>'s are provided, they will be marked so that we only print out what we read in. On input, if no <iothreadids> are provided, the PostParse code will self generate a list of ID's starting at 1 and going to the number of iothreads defined for the domain (just like the current algorithm numbering scheme). A future patch will rework the existing algorithm to make use of the iothreadids list. On output, only print out the <iothreadids> if they were read in. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 30 +++++++ docs/schemas/domaincommon.rng | 12 +++ src/conf/domain_conf.c | 203 +++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 18 ++++ src/libvirt_private.syms | 4 + 5 files changed, 265 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index e921749..518f7c5 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -521,6 +521,18 @@ ... </domain> </pre> +<pre> +<domain> + ... + <iothreadids> + <iothread id="2"/> + <iothread id="4"/> + <iothread id="6"/> + <iothread id="8"/> + </iothreadids> + ... +</domain> +</pre> <dl> <dt><code>iothreads</code></dt> @@ -530,7 +542,25 @@ virtio-blk-pci and virtio-blk-ccw target storage devices. There should be only 1 or 2 IOThreads per host CPU. There may be more than one supported device assigned to each IOThread. + <span class="since">Since 1.2.8</span> </dd> + <dt><code>iothreadids</code></dt> + <dd> + The optional <code>iothreadids</code> element provides the capability + to specifically define the IOThread ID's for the domain. By default, + IOThread ID's are sequentially numbered starting from 1 through the + number of <code>iothreads</code> defined for the domain. The + <code>id</code> attribute is used to define the IOThread ID. The + <code>id</code> attribute must be a positive integer greater than 0. + If there are less <code>iothreadids</code> defined than + <code>iothreads</code> defined for the domain, then libvirt will + sequentially fill <code>iothreadids</code> starting at 1 avoiding + any predefined <code>id</code>. If there are more + <code>iothreadids</code> defined than <code>iothreads</code> + defined for the domain, then the <code>iothreads</code> value + will be adjusted accordingly. + <span class="since">Since 1.2.15</span> + </dd> </dl> <h3><a name="elementsCPUTuning">CPU Tuning</a></h3> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 19461f5..7072954 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -539,6 +539,18 @@ </optional> <optional> + <element name="iothreadids"> + <zeroOrMore> + <element name="iothread"> + <attribute name="id"> + <ref name="unsignedInt"/> + </attribute> + </element> + </zeroOrMore> + </element> + </optional> + + <optional> <ref name="blkiotune"/> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9aad782..5c89388 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2114,6 +2114,32 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int npin) return NULL; } + +void +virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def) +{ + if (!def) + return; + VIR_FREE(def); +} + + +static void +virDomainIOThreadIDDefArrayFree(virDomainIOThreadIDDefPtr *def, + int nids) +{ + size_t i; + + if (!def) + return; + + for (i = 0; i < nids; i++) + virDomainIOThreadIDDefFree(def[i]); + + VIR_FREE(def); +} + + void virDomainPinDefFree(virDomainPinDefPtr def) { @@ -2310,6 +2336,8 @@ void virDomainDefFree(virDomainDefPtr def) virCPUDefFree(def->cpu); + virDomainIOThreadIDDefArrayFree(def->iothreadids, def->niothreadids); + virDomainPinDefArrayFree(def->cputune.vcpupin, def->cputune.nvcpupin); virDomainPinDefFree(def->cputune.emulatorpin); @@ -13148,6 +13176,54 @@ virDomainIdmapDefParseXML(xmlXPathContextPtr ctxt, return idmap; } +/* Parse the XML definition for an IOThread ID + * + * Format is : + * + * <iothreads>4</iothreads> + * <iothreadids> + * <iothread id='1'/> + * <iothread id='3'/> + * <iothread id='5'/> + * <iothread id='7'/> + * </iothreadids> + */ +static virDomainIOThreadIDDefPtr +virDomainIOThreadIDDefParseXML(xmlNodePtr node, + xmlXPathContextPtr ctxt) +{ + virDomainIOThreadIDDefPtr iothrid; + xmlNodePtr oldnode = ctxt->node; + char *tmp = NULL; + + if (VIR_ALLOC(iothrid) < 0) + return NULL; + + ctxt->node = node; + + if (!(tmp = virXPathString("string(./@id)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing 'id' attribute in <iothread> element")); + goto error; + } + if (virStrToLong_uip(tmp, NULL, 10, &iothrid->iothread_id) < 0 || + iothrid->iothread_id == 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid iothread 'id' value '%s'"), tmp); + goto error; + } + + cleanup: + VIR_FREE(tmp); + ctxt->node = oldnode; + return iothrid; + + error: + virDomainIOThreadIDDefFree(iothrid); + goto cleanup; +} + + /* Parse the XML definition for a vcpupin * * vcpupin has the form of @@ -13954,6 +14030,49 @@ virDomainDefParseXML(xmlDocPtr xml, } VIR_FREE(tmp); + /* Extract any iothread id's defined */ + if ((n = virXPathNodeSet("./iothreadids/iothread", ctxt, &nodes)) < 0) + goto error; + + if (n > def->iothreads) + def->iothreads = n; + + if (n && VIR_ALLOC_N(def->iothreadids, n) < 0) + goto error; + + for (i = 0; i < n; i++) { + virDomainIOThreadIDDefPtr iothrid = NULL; + if (!(iothrid = virDomainIOThreadIDDefParseXML(nodes[i], ctxt))) + goto error; + + if (virDomainIOThreadIDFind(def, iothrid->iothread_id)) { + virReportError(VIR_ERR_XML_ERROR, + _("duplicate iothread id '%u' found"), + iothrid->iothread_id); + virDomainIOThreadIDDefFree(iothrid); + goto error; + } + def->iothreadids[def->niothreadids++] = iothrid; + } + VIR_FREE(nodes); + + /* If no iothreadid's or not fully populated, let's finish the job + * here rather than in PostParseCallback + */ + if (def->iothreads && def->iothreads != def->niothreadids) { + unsigned int iothread_id = 1; + while (def->niothreadids != def->iothreads) { + if (!virDomainIOThreadIDFind(def, iothread_id)) { + virDomainIOThreadIDDefPtr iothrid; + + if (!(iothrid = virDomainIOThreadIDAdd(def, iothread_id))) + goto error; + iothrid->autofill = true; + } + iothread_id++; + } + } + /* Extract cpu tunables. */ if ((n = virXPathULong("string(./cputune/shares[1])", ctxt, &def->cputune.shares)) < -1) { @@ -17238,6 +17357,67 @@ virDomainDefAddImplicitControllers(virDomainDefPtr def) return 0; } +virDomainIOThreadIDDefPtr +virDomainIOThreadIDFind(virDomainDefPtr def, + unsigned int iothread_id) +{ + size_t i; + + if (!def->iothreadids || !def->niothreadids) + return NULL; + + for (i = 0; i < def->niothreadids; i++) { + if (iothread_id == def->iothreadids[i]->iothread_id) + return def->iothreadids[i]; + } + + return NULL; +} + +virDomainIOThreadIDDefPtr +virDomainIOThreadIDAdd(virDomainDefPtr def, + unsigned int iothread_id) +{ + virDomainIOThreadIDDefPtr iothrid = NULL; + + if (virDomainIOThreadIDFind(def, iothread_id)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot duplicate iothread_id '%u' in iothreadids"), + iothread_id); + return NULL; + } + + if (VIR_ALLOC(iothrid) < 0) + goto error; + + iothrid->iothread_id = iothread_id; + + if (VIR_APPEND_ELEMENT_COPY(def->iothreadids, def->niothreadids, + iothrid) < 0) + goto error; + + return iothrid; + + error: + virDomainIOThreadIDDefFree(iothrid); + return NULL; +} + +void +virDomainIOThreadIDDel(virDomainDefPtr def, + unsigned int iothread_id) +{ + int n; + + for (n = 0; n < def->niothreadids; n++) { + if (def->iothreadids[n]->iothread_id == iothread_id) { + virDomainIOThreadIDDefFree(def->iothreadids[n]); + VIR_DELETE_ELEMENT(def->iothreadids, n, def->niothreadids); + return; + } + } +} + /* Check if vcpupin with same id already exists. */ bool virDomainPinIsDuplicate(virDomainPinDefPtr *def, @@ -20580,8 +20760,27 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAsprintf(buf, " current='%u'", def->vcpus); virBufferAsprintf(buf, ">%u</vcpu>\n", def->maxvcpus); - if (def->iothreads > 0) - virBufferAsprintf(buf, "<iothreads>%u</iothreads>\n", def->iothreads); + if (def->iothreads > 0) { + virBufferAsprintf(buf, "<iothreads>%u</iothreads>\n", + def->iothreads); + /* Only print out iothreadids if we read at least one */ + for (i = 0; i < def->niothreadids; i++) { + if (!def->iothreadids[i]->autofill) + break; + } + if (i < def->niothreadids) { + virBufferAddLit(buf, "<iothreadids>\n"); + virBufferAdjustIndent(buf, 2); + for (i = 0; i < def->niothreadids; i++) { + if (def->iothreadids[i]->autofill) + continue; + virBufferAsprintf(buf, "<iothread id='%u'/>\n", + def->iothreadids[i]->iothread_id); + } + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</iothreadids>\n"); + } + } if (def->cputune.sharesSpecified || (def->cputune.nvcpupin && !virDomainIsAllVcpupinInherited(def)) || diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 25d3ee6..d4229eb 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2054,6 +2054,16 @@ struct _virDomainHugePage { unsigned long long size; /* hugepage size in KiB */ }; +typedef struct _virDomainIOThreadIDDef virDomainIOThreadIDDef; +typedef virDomainIOThreadIDDef *virDomainIOThreadIDDefPtr; + +struct _virDomainIOThreadIDDef { + bool autofill; + unsigned int iothread_id; +}; + +void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def); + typedef struct _virDomainCputune virDomainCputune; typedef virDomainCputune *virDomainCputunePtr; @@ -2145,6 +2155,8 @@ struct _virDomainDef { virBitmapPtr cpumask; unsigned int iothreads; + size_t niothreadids; + virDomainIOThreadIDDefPtr *iothreadids; virDomainCputune cputune; @@ -2602,6 +2614,12 @@ bool virDomainDefCheckABIStability(virDomainDefPtr src, int virDomainDefAddImplicitControllers(virDomainDefPtr def); +virDomainIOThreadIDDefPtr virDomainIOThreadIDFind(virDomainDefPtr def, + unsigned int iothread_id); +virDomainIOThreadIDDefPtr virDomainIOThreadIDAdd(virDomainDefPtr def, + unsigned int iothread_id); +void virDomainIOThreadIDDel(virDomainDefPtr def, unsigned int iothread_id); + unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags); char *virDomainDefFormat(virDomainDefPtr def, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8c50ea2..77f6797 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -324,6 +324,10 @@ virDomainHubTypeToString; virDomainHypervTypeFromString; virDomainHypervTypeToString; virDomainInputDefFree; +virDomainIOThreadIDAdd; +virDomainIOThreadIDDefFree; +virDomainIOThreadIDDel; +virDomainIOThreadIDFind; virDomainLeaseDefFree; virDomainLeaseIndex; virDomainLeaseInsert; -- 2.1.0
So obviously I messed something up with the threading on the responses - thought I had it right, but not quite I guess. Oh well - I think it's obvious what I was trying to do ;-) v5 updates: Patch 1/10 (forgot to edit the 10) * Fixes the syntax check * Moves the PostParse autofill of iothreadids right after the possible reading from XML Patch 4/9 * Add virDomainIOThreadIDArrayHasPin which will looking through the niothreadids[] array for any iothreadid[] with a cpumask defined * Fail in virDomainIOThreadPinDefParseXML if the iothreadid cannot be found * Remove [n]iothreadspin references * Left the cpumask checking to default - I think all should be handled in a later/separate patch * Fixed qemuSetupCgroupForIOThreads cpumask ordering Patch 5/9 * Adjusted the test to use 'fifo' *and* priority='1' for second entry (got a failure in a different test after my first pass) Patch 7.5/9 * NEW - Code that will remove the iothread_id bit from the ids mask. I did test this and it worked like I expected... Patch 8/9 * Not quite sure what was meant by auto numa placement so I didn't add that into the patch I sent, but if the following is what you meant, then I can squash it in (and define cpumask appropriately) + if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) + cpumask = priv->autoCpuset; + else + cpumask = vm->def->cpumask; + + if (cpumask) { + if (qemuDomainHotplugPinThread(cpumask, iothread_id, * Remove qemuProcessSetSchedParams * Remove niothreadspin * Remove the virNumaIsAvailable() hunk from qemuDomainChgIOThread Tks, John
participants (2)
-
John Ferlan -
Peter Krempa