[libvirt] [PATCH v3 0/6] Implement Add/Del IOThreads
v2: http://www.redhat.com/archives/libvir-list/2015-April/msg00426.html Differences: Patch 1 (old patch 3): - virDomainIOThreadIDDefParseXML: - Fix error message for <id> (was element, change to attribute for <iothread> element) - Silently bump the 'def->iothreads' count if there are a more <iothreadid> elements found than def->iothreads defined rather than causing an error - virDomainIOThreadIDAdd - Just use VIR_APPEND_ELEMENT - no need for empty array check - Remove virDomainIOThreadIDIsDuplicate - Since it was doing essentially the same as virDomainIOThreadIDFind - Replace callers of IDIsDuplicate with the IDFind - Remove 'name' from _virDomainIOThreadIDDef causes ripple effect... - formatdomain.html.in and docs/schemas/domaincommon.rng - No need for virDomainIOThreadIDDefFree - Change of parameters to virDomainIOThreadIDAdd - Remove parse in virDomainIOThreadIDDefParseXML - Remove format in virDomainDefFormatInternal - Remove virDomainIOThreadIDDefFree - Unnecessary since 'name' is removed Patch 2 (merged old patch 4 & 5) - Add 'thread_id' to virDomainIOThreadIDDef and remove from _qemuDomainObjPrivate. Adjust qemuDomainObjPrivateFree, qemuDomainObjPrivateXMLFormat, and qemuDomainObjPrivateXMLParse to remove the the iothreadpids usage. Rely on saved domain XML for the 'live' data. - Use vm->def->niothreadids instead of priv->niothreadpids - Remove a couple of (now) unnecessary checks where priv->[n]iothreadpids was referenced. Now we now that at XMLParse we've created the [vm->]def->iothreadids with enough elements to satisfy the maximum of def->iothreads and def->niothreadids, so no need to check if it exists like we had to for priv->iothreadpids Patch 3 (old patch 6) - Remove 'name' from virDomainAddIOThread - Fix comments in virDomainIOThreadAdd and virDomainIOThreadDel - Remove if ((unsigned short) iothread_id != iothread_id) check Patch 4 (old patch 7) - Remove 'name' from remote_domain_add_iothread_args and remote_domain_add_iothread_args Patch 5 (old patch 8) - Split qemuDomainHotplugIOThread into qemuDomainHotplugAddIOThread and qemuDomainHotplugDelIOThread - Removed extraneous comments - Moved active domain check to after job starts and only for LIVE - Removed the virResetLastError if qemuMonitorGetIOThreads fails - Didn't touch the Numatune change - that can be resolved separately - For the Add case, there was a comment about already having the data which while true, we don't know where in the returned qemu iothreads list the new IOThread was added, so we have to find it in the list so we can get the thread_id so save once we create the element in the iothreadids list. - Moved where the virDomainLiveConfigHelperMethod was called - For an inactive config - we can add iothreads and iothreadids, when/if someone starts the guest, then they'll get the bad news if the right emulator wasn't available. - Handling LIVE before CONFIG just seemed to be the norm for commands that I looked at, so I left it that way. Patch 6 (old patch 9) - Remove "name" (avoided the one comment from the review) John Ferlan (6): conf: Add new domain XML element 'iothreadids' qemu: Use domain iothreadids to IOThread's 'thread_id' 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 | 30 ++ 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 | 190 ++++++++- src/conf/domain_conf.h | 16 + src/driver-hypervisor.h | 12 + src/libvirt-domain.c | 118 ++++++ src/libvirt_private.syms | 4 + src/libvirt_public.syms | 6 + 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 | 466 ++++++++++++++++++++- src/qemu/qemu_process.c | 37 +- src/remote/remote_driver.c | 2 + src/remote/remote_protocol.x | 30 +- src/remote_protocol-structs | 12 + .../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 ++ 29 files changed, 1239 insertions(+), 102 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 | 190 +++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 15 ++++ src/libvirt_private.syms | 3 + 5 files changed, 248 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 03fd541..4bd32bd 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -675,6 +675,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 4d7e3c9..e86b7e2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2113,6 +2113,23 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int npin) return NULL; } + +static void +virDomainIOThreadIDDefArrayFree(virDomainIOThreadIDDefPtr *def, + int nids) +{ + size_t i; + + if (!def) + return; + + for (i = 0; i < nids; i++) + VIR_FREE(def[i]); + + VIR_FREE(def); +} + + void virDomainPinDefFree(virDomainPinDefPtr def) { @@ -2310,6 +2327,8 @@ void virDomainDefFree(virDomainDefPtr def) virCPUDefFree(def->cpu); + virDomainIOThreadIDDefArrayFree(def->iothreadids, def->niothreadids); + virDomainPinDefArrayFree(def->cputune.vcpupin, def->cputune.nvcpupin); virDomainPinDefFree(def->cputune.emulatorpin); @@ -3304,6 +3323,18 @@ 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)) { + if (virDomainIOThreadIDAdd(def, iothread_id) < 0) + return -1; + } + iothread_id++; + } + } + if (virDomainDefGetMemoryInitial(def) == 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Memory size must be specified via <memory> or in the " @@ -13173,6 +13204,56 @@ 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; + } + + iothrid->defined = true; + + cleanup: + VIR_FREE(tmp); + ctxt->node = oldnode; + return iothrid; + + error: + VIR_FREE(iothrid); + goto cleanup; +} + + /* Parse the XML definition for a vcpupin * * vcpupin has the form of @@ -13948,6 +14029,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, MAX(n, def->iothreads)) < 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); + VIR_FREE(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) { @@ -17324,6 +17431,66 @@ 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; +} + +int +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 -1; + } + + if (VIR_ALLOC(iothrid) < 0) + goto error; + + iothrid->iothread_id = iothread_id; + + if (VIR_APPEND_ELEMENT(def->iothreadids, def->niothreadids, iothrid) < 0) + goto error; + + return 0; + + error: + VIR_FREE(iothrid); + return -1; +} + +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) { + VIR_FREE(def->iothreadids[n]); + VIR_DELETE_ELEMENT(def->iothreadids, n, def->niothreadids); + return; + } + } +} + /* Check if vcpupin with same id already exists. */ bool virDomainPinIsDuplicate(virDomainPinDefPtr *def, @@ -20666,8 +20833,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); + /* If we parsed the iothreadids, then write out those that we parsed */ + for (i = 0; i < def->niothreadids; i++) { + if (def->iothreadids[i]->defined) + break; + } + if (i < def->niothreadids) { + virBufferAddLit(buf, "<iothreadids>\n"); + virBufferAdjustIndent(buf, 2); + for (i = 0; i < def->niothreadids; i++) { + if (!def->iothreadids[i]->defined) + 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 e6fa3c9..9e7bdf9 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2041,6 +2041,14 @@ struct _virDomainHugePage { unsigned long long size; /* hugepage size in KiB */ }; +typedef struct _virDomainIOThreadIDDef virDomainIOThreadIDDef; +typedef virDomainIOThreadIDDef *virDomainIOThreadIDDefPtr; + +struct _virDomainIOThreadIDDef { + bool defined; + unsigned int iothread_id; +}; + typedef struct _virDomainCputune virDomainCputune; typedef virDomainCputune *virDomainCputunePtr; @@ -2132,6 +2140,8 @@ struct _virDomainDef { virBitmapPtr cpumask; unsigned int iothreads; + size_t niothreadids; + virDomainIOThreadIDDefPtr *iothreadids; virDomainCputune cputune; @@ -2590,6 +2600,11 @@ bool virDomainDefCheckABIStability(virDomainDefPtr src, int virDomainDefAddImplicitControllers(virDomainDefPtr def); +virDomainIOThreadIDDefPtr +virDomainIOThreadIDFind(virDomainDefPtr def, unsigned int iothread_id); +int 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 7166283..12fd9e9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -326,6 +326,9 @@ virDomainHubTypeToString; virDomainHypervTypeFromString; virDomainHypervTypeToString; virDomainInputDefFree; +virDomainIOThreadIDAdd; +virDomainIOThreadIDDel; +virDomainIOThreadIDFind; virDomainLeaseDefFree; virDomainLeaseIndex; virDomainLeaseInsert; -- 2.1.0
On Tue, Apr 14, 2015 at 21:18:21 -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 | 190 +++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 15 ++++ src/libvirt_private.syms | 3 + 5 files changed, 248 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 03fd541..4bd32bd 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -675,6 +675,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 4d7e3c9..e86b7e2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2113,6 +2113,23 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int npin) return NULL; }
+ +static void +virDomainIOThreadIDDefArrayFree(virDomainIOThreadIDDefPtr *def, + int nids) +{ + size_t i; + + if (!def) + return; + + for (i = 0; i < nids; i++) + VIR_FREE(def[i]); + + VIR_FREE(def); +} + + void virDomainPinDefFree(virDomainPinDefPtr def) { @@ -2310,6 +2327,8 @@ void virDomainDefFree(virDomainDefPtr def)
virCPUDefFree(def->cpu);
+ virDomainIOThreadIDDefArrayFree(def->iothreadids, def->niothreadids); + virDomainPinDefArrayFree(def->cputune.vcpupin, def->cputune.nvcpupin);
virDomainPinDefFree(def->cputune.emulatorpin); @@ -3304,6 +3323,18 @@ 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)) { + if (virDomainIOThreadIDAdd(def, iothread_id) < 0)
This code is _adding_ fields for every iothread that was not specified in <iothreadids> but is implied by <iothreads>, but before that you've allocated all the structures in [1].
+ return -1; + } + iothread_id++; + } + } + if (virDomainDefGetMemoryInitial(def) == 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Memory size must be specified via <memory> or in the " @@ -13173,6 +13204,56 @@ 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; + } + + iothrid->defined = true; + + cleanup: + VIR_FREE(tmp); + ctxt->node = oldnode; + return iothrid; + + error: + VIR_FREE(iothrid); + goto cleanup; +} + + /* Parse the XML definition for a vcpupin * * vcpupin has the form of @@ -13948,6 +14029,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, MAX(n, def->iothreads)) < 0) + goto error;
[1]
+ + 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); + VIR_FREE(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) { @@ -17324,6 +17431,66 @@ 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; +} + +int +virDomainIOThreadIDAdd(virDomainDefPtr def, + unsigned int iothread_id)
If this function returned the pointer to the added structure you'd be able to use it later to modify other aspects of it.
+{ + virDomainIOThreadIDDefPtr iothrid = NULL; + + if (virDomainIOThreadIDFind(def, iothread_id)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot duplicate iothread_id '%u' in iothreadids"), + iothread_id); + return -1; + } + + if (VIR_ALLOC(iothrid) < 0) + goto error; + + iothrid->iothread_id = iothread_id; + + if (VIR_APPEND_ELEMENT(def->iothreadids, def->niothreadids, iothrid) < 0) + goto error; + + return 0; + + error: + VIR_FREE(iothrid); + return -1; +} + +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) { + VIR_FREE(def->iothreadids[n]);
This could use a common freeing function, so that once you add more data you won't have to tweak it.
+ VIR_DELETE_ELEMENT(def->iothreadids, n, def->niothreadids); + return; + } + } +} + /* Check if vcpupin with same id already exists. */ bool virDomainPinIsDuplicate(virDomainPinDefPtr *def, @@ -20666,8 +20833,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); + /* If we parsed the iothreadids, then write out those that we parsed */
Seems rather obvious.
+ for (i = 0; i < def->niothreadids; i++) { + if (def->iothreadids[i]->defined) + break; + } + if (i < def->niothreadids) { + virBufferAddLit(buf, "<iothreadids>\n"); + virBufferAdjustIndent(buf, 2); + for (i = 0; i < def->niothreadids; i++) { + if (!def->iothreadids[i]->defined) + 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 e6fa3c9..9e7bdf9 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2041,6 +2041,14 @@ struct _virDomainHugePage { unsigned long long size; /* hugepage size in KiB */ };
+typedef struct _virDomainIOThreadIDDef virDomainIOThreadIDDef; +typedef virDomainIOThreadIDDef *virDomainIOThreadIDDefPtr; + +struct _virDomainIOThreadIDDef { + bool defined;
You may want to invert this field, since the case where you add the thread structs due to the fact that <iohreads> was more than the count of entries is less common than other places that add the struct.
+ unsigned int iothread_id; +}; + typedef struct _virDomainCputune virDomainCputune; typedef virDomainCputune *virDomainCputunePtr;
@@ -2132,6 +2140,8 @@ struct _virDomainDef { virBitmapPtr cpumask;
unsigned int iothreads; + size_t niothreadids; + virDomainIOThreadIDDefPtr *iothreadids;
virDomainCputune cputune;
@@ -2590,6 +2600,11 @@ bool virDomainDefCheckABIStability(virDomainDefPtr src,
int virDomainDefAddImplicitControllers(virDomainDefPtr def);
+virDomainIOThreadIDDefPtr +virDomainIOThreadIDFind(virDomainDefPtr def, unsigned int iothread_id);
Usually the return type is on the same line and the argument list is broken if it doesn't fit.
+int 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,
On 04/21/2015 10:08 AM, Peter Krempa wrote:
On Tue, Apr 14, 2015 at 21:18:21 -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 | 190 +++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 15 ++++ src/libvirt_private.syms | 3 + 5 files changed, 248 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 03fd541..4bd32bd 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -675,6 +675,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 4d7e3c9..e86b7e2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2113,6 +2113,23 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int npin) return NULL; }
+ +static void +virDomainIOThreadIDDefArrayFree(virDomainIOThreadIDDefPtr *def, + int nids) +{ + size_t i; + + if (!def) + return; + + for (i = 0; i < nids; i++) + VIR_FREE(def[i]); + + VIR_FREE(def); +} + + void virDomainPinDefFree(virDomainPinDefPtr def) { @@ -2310,6 +2327,8 @@ void virDomainDefFree(virDomainDefPtr def)
virCPUDefFree(def->cpu);
+ virDomainIOThreadIDDefArrayFree(def->iothreadids, def->niothreadids); + virDomainPinDefArrayFree(def->cputune.vcpupin, def->cputune.nvcpupin);
virDomainPinDefFree(def->cputune.emulatorpin); @@ -3304,6 +3323,18 @@ 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)) { + if (virDomainIOThreadIDAdd(def, iothread_id) < 0)
This code is _adding_ fields for every iothread that was not specified in <iothreadids> but is implied by <iothreads>, but before that you've allocated all the structures in [1].
Hmm... correct - of course the allocation in [1] was suggested by previous code review.
+ return -1; + } + iothread_id++; + } + } + if (virDomainDefGetMemoryInitial(def) == 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Memory size must be specified via <memory> or in the " @@ -13173,6 +13204,56 @@ 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; + } + + iothrid->defined = true; + + cleanup: + VIR_FREE(tmp); + ctxt->node = oldnode; + return iothrid; + + error: + VIR_FREE(iothrid); + goto cleanup; +} + + /* Parse the XML definition for a vcpupin * * vcpupin has the form of @@ -13948,6 +14029,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, MAX(n, def->iothreads)) < 0) + goto error;
[1]
Yes, the MAX(n, def->iothreads) was the prior code review suggestion... So if I 'revert' back to just 'n', then let the above code handle the rest, then these two issues should be resolved.
+ + 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); + VIR_FREE(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) { @@ -17324,6 +17431,66 @@ 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; +} + +int +virDomainIOThreadIDAdd(virDomainDefPtr def, + unsigned int iothread_id)
If this function returned the pointer to the added structure you'd be able to use it later to modify other aspects of it.
OK seems reasonable... had to change to using VIR_APPEND_ELEMENT_COPY too
+{ + virDomainIOThreadIDDefPtr iothrid = NULL; + + if (virDomainIOThreadIDFind(def, iothread_id)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot duplicate iothread_id '%u' in iothreadids"), + iothread_id); + return -1; + } + + if (VIR_ALLOC(iothrid) < 0) + goto error; + + iothrid->iothread_id = iothread_id; + + if (VIR_APPEND_ELEMENT(def->iothreadids, def->niothreadids, iothrid) < 0) + goto error; + + return 0; + + error: + VIR_FREE(iothrid); + return -1; +} + +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) { + VIR_FREE(def->iothreadids[n]);
This could use a common freeing function, so that once you add more data you won't have to tweak it.
So you'd prefer a 'common' free function that just calls VIR_FREE() - that just seems like excessive overhead, but that's fine.
+ VIR_DELETE_ELEMENT(def->iothreadids, n, def->niothreadids); + return; + } + } +} + /* Check if vcpupin with same id already exists. */ bool virDomainPinIsDuplicate(virDomainPinDefPtr *def, @@ -20666,8 +20833,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); + /* If we parsed the iothreadids, then write out those that we parsed */
Seems rather obvious.
As a reviewer for now, sure... But what about someone who reads this code 6, 12, 18 months from now and wonders why the extra loop... I can adjust the comment to be : "Only print out iothreadids if we read at least one"
+ for (i = 0; i < def->niothreadids; i++) { + if (def->iothreadids[i]->defined) + break; + } + if (i < def->niothreadids) { + virBufferAddLit(buf, "<iothreadids>\n"); + virBufferAdjustIndent(buf, 2); + for (i = 0; i < def->niothreadids; i++) { + if (!def->iothreadids[i]->defined) + 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 e6fa3c9..9e7bdf9 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2041,6 +2041,14 @@ struct _virDomainHugePage { unsigned long long size; /* hugepage size in KiB */ };
+typedef struct _virDomainIOThreadIDDef virDomainIOThreadIDDef; +typedef virDomainIOThreadIDDef *virDomainIOThreadIDDefPtr; + +struct _virDomainIOThreadIDDef { + bool defined;
You may want to invert this field, since the case where you add the thread structs due to the fact that <iohreads> was more than the count of entries is less common than other places that add the struct.
Made for an awkward looking: for (i = 0; i < def->niothreadids; i++) { if (!def->iothreadids[i]->undefined) break; }
+ unsigned int iothread_id; +}; + typedef struct _virDomainCputune virDomainCputune; typedef virDomainCputune *virDomainCputunePtr;
@@ -2132,6 +2140,8 @@ struct _virDomainDef { virBitmapPtr cpumask;
unsigned int iothreads; + size_t niothreadids; + virDomainIOThreadIDDefPtr *iothreadids;
virDomainCputune cputune;
@@ -2590,6 +2600,11 @@ bool virDomainDefCheckABIStability(virDomainDefPtr src,
int virDomainDefAddImplicitControllers(virDomainDefPtr def);
+virDomainIOThreadIDDefPtr +virDomainIOThreadIDFind(virDomainDefPtr def, unsigned int iothread_id);
Usually the return type is on the same line and the argument list is broken if it doesn't fit.
OK - perhaps a relic of previous code where even the first argument made the line too long - been too long to remember for sure.
+int 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,
Which gives the following to be squashed in (trying to make progress on at least the first two patches which compile and pass tests): diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 82c36a4..3ff06b6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2115,6 +2115,14 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int npin) } +void +virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def) +{ + if (def) + VIR_FREE(def); +} + + static void virDomainIOThreadIDDefArrayFree(virDomainIOThreadIDDefPtr *def, int nids) @@ -2125,7 +2133,7 @@ virDomainIOThreadIDDefArrayFree(virDomainIOThreadIDDefPtr *def, return; for (i = 0; i < nids; i++) - VIR_FREE(def[i]); + virDomainIOThreadIDDefFree(def[i]); VIR_FREE(def); } @@ -3322,8 +3330,11 @@ virDomainDefPostParseInternal(virDomainDefPtr def, unsigned int iothread_id = 1; while (def->niothreadids != def->iothreads) { if (!virDomainIOThreadIDFind(def, iothread_id)) { - if (virDomainIOThreadIDAdd(def, iothread_id) < 0) + virDomainIOThreadIDDefPtr iothrid; + + if (!(iothrid = virDomainIOThreadIDAdd(def, iothread_id))) return -1; + iothrid->undefined = true; } iothread_id++; } @@ -13216,15 +13227,13 @@ virDomainIOThreadIDDefParseXML(xmlNodePtr node, goto error; } - iothrid->defined = true; - cleanup: VIR_FREE(tmp); ctxt->node = oldnode; return iothrid; error: - VIR_FREE(iothrid); + virDomainIOThreadIDDefFree(iothrid); goto cleanup; } @@ -14042,7 +14051,7 @@ virDomainDefParseXML(xmlDocPtr xml, if (n > def->iothreads) def->iothreads = n; - if (n && VIR_ALLOC_N(def->iothreadids, MAX(n, def->iothreads)) < 0) + if (n && VIR_ALLOC_N(def->iothreadids, n) < 0) goto error; for (i = 0; i < n; i++) { @@ -14054,7 +14063,7 @@ virDomainDefParseXML(xmlDocPtr xml, virReportError(VIR_ERR_XML_ERROR, _("duplicate iothread id '%u' found"), iothrid->iothread_id); - VIR_FREE(iothrid); + virDomainIOThreadIDDefFree(iothrid); goto error; } def->iothreadids[def->niothreadids++] = iothrid; @@ -17362,7 +17371,7 @@ virDomainIOThreadIDFind(virDomainDefPtr def, return NULL; } -int +virDomainIOThreadIDDefPtr virDomainIOThreadIDAdd(virDomainDefPtr def, unsigned int iothread_id) { @@ -17372,7 +17381,7 @@ virDomainIOThreadIDAdd(virDomainDefPtr def, virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot duplicate iothread_id '%u' in iothreadids"), iothread_id); - return -1; + return NULL; } if (VIR_ALLOC(iothrid) < 0) @@ -17380,14 +17389,15 @@ virDomainIOThreadIDAdd(virDomainDefPtr def, iothrid->iothread_id = iothread_id; - if (VIR_APPEND_ELEMENT(def->iothreadids, def->niothreadids, iothrid) < 0) + if (VIR_APPEND_ELEMENT_COPY(def->iothreadids, def->niothreadids, + iothrid) < 0) goto error; - return 0; + return iothrid; error: - VIR_FREE(iothrid); - return -1; + virDomainIOThreadIDDefFree(iothrid); + return NULL; } void @@ -17398,7 +17408,7 @@ virDomainIOThreadIDDel(virDomainDefPtr def, for (n = 0; n < def->niothreadids; n++) { if (def->iothreadids[n]->iothread_id == iothread_id) { - VIR_FREE(def->iothreadids[n]); + virDomainIOThreadIDDefFree(def->iothreadids[n]); VIR_DELETE_ELEMENT(def->iothreadids, n, def->niothreadids); return; } @@ -20749,16 +20759,16 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (def->iothreads > 0) { virBufferAsprintf(buf, "<iothreads>%u</iothreads>\n", def->iothreads); - /* If we parsed the iothreadids, then write out those that we parsed */ + /* Only print out iothreadids if we read at least one */ for (i = 0; i < def->niothreadids; i++) { - if (def->iothreadids[i]->defined) + if (!def->iothreadids[i]->undefined) break; } if (i < def->niothreadids) { virBufferAddLit(buf, "<iothreadids>\n"); virBufferAdjustIndent(buf, 2); for (i = 0; i < def->niothreadids; i++) { - if (!def->iothreadids[i]->defined) + if (def->iothreadids[i]->undefined) continue; virBufferAsprintf(buf, "<iothread id='%u'/>\n", def->iothreadids[i]->iothread_id); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 01dc1f9..32de301 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2058,10 +2058,12 @@ typedef struct _virDomainIOThreadIDDef virDomainIOThreadIDDef; typedef virDomainIOThreadIDDef *virDomainIOThreadIDDefPtr; struct _virDomainIOThreadIDDef { - bool defined; + bool undefined; unsigned int iothread_id; }; +void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def); + typedef struct _virDomainCputune virDomainCputune; typedef virDomainCputune *virDomainCputunePtr; @@ -2612,9 +2614,10 @@ bool virDomainDefCheckABIStability(virDomainDefPtr src, int virDomainDefAddImplicitControllers(virDomainDefPtr def); -virDomainIOThreadIDDefPtr -virDomainIOThreadIDFind(virDomainDefPtr def, unsigned int iothread_id); -int virDomainIOThreadIDAdd(virDomainDefPtr def, unsigned int iothread_id); +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); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 649ed8f..ac35f1b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -325,6 +325,7 @@ virDomainHypervTypeFromString; virDomainHypervTypeToString; virDomainInputDefFree; virDomainIOThreadIDAdd; +virDomainIOThreadIDDefFree; virDomainIOThreadIDDel; virDomainIOThreadIDFind; virDomainLeaseDefFree;
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 9e7bdf9..fb5f2e1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2047,6 +2047,7 @@ typedef virDomainIOThreadIDDef *virDomainIOThreadIDDefPtr; struct _virDomainIOThreadIDDef { bool defined; unsigned int iothread_id; + int thread_id; }; typedef struct _virDomainCputune virDomainCputune; 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 e7e0937..7ae3d8e 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 603360f..9348bd1 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 3225abb..2642350 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -158,9 +158,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 f5a3ef9..008258f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5834,14 +5834,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; @@ -5896,19 +5898,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; @@ -6052,16 +6054,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; } @@ -6099,8 +6096,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); @@ -10049,8 +10045,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 d9611c9..e0b3ce2 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; @@ -2447,7 +2451,6 @@ qemuProcessSetEmulatorAffinity(virDomainObjPtr vm) static int qemuProcessSetIOThreadsAffinity(virDomainObjPtr vm) { - qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr def = vm->def; virDomainPinDefPtr pininfo; size_t i; @@ -2456,20 +2459,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; @@ -2519,8 +2517,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; @@ -5308,8 +5307,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 8d0a4aa..57de961 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1229,6 +1229,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 5a5812f..09f6d8d 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
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 7be4219..21b06b4 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 b275d86..b2b5a13 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8210,6 +8210,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 d90e6b5..c0b3eff 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; }; @@ -5643,5 +5655,21 @@ enum remote_procedure { * @generate: none * @acl: domain:read */ - REMOTE_PROC_DOMAIN_INTERFACE_ADDRESSES = 353 + REMOTE_PROC_DOMAIN_INTERFACE_ADDRESSES = 353, + + /** + * @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 = 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_DEL_IOTHREAD = 355 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index e614f77..95b9bdb 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; }; @@ -3017,4 +3027,6 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_GET_IOTHREAD_INFO = 351, REMOTE_PROC_DOMAIN_PIN_IOTHREAD = 352, REMOTE_PROC_DOMAIN_INTERFACE_ADDRESSES = 353, + REMOTE_PROC_DOMAIN_ADD_IOTHREAD = 354, + REMOTE_PROC_DOMAIN_DEL_IOTHREAD = 355, }; -- 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 | 431 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 447 insertions(+) diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 159ebf5..92fcdd3 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 12fd9e9..37e1fbb 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -129,6 +129,7 @@ virDomainAuditDisk; virDomainAuditFS; virDomainAuditHostdev; virDomainAuditInit; +virDomainAuditIOThread; virDomainAuditMemory; virDomainAuditNet; virDomainAuditNetDevice; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 008258f..f42d4fb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6179,6 +6179,435 @@ 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; + unsigned int idval = 0; + 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 (qemuDomainParseIOThreadAlias(new_iothreads[idx]->name, &idval) < 0) + goto cleanup; + if (iothread_id == idval) + break; + } + + if (idval != iothread_id) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find new IOThread '%u' in QEMU monitor."), + iothread_id); + goto cleanup; + } + + if (virDomainIOThreadIDAdd(vm->def, iothread_id) < 0) + goto cleanup; + + if (!(iothrid = virDomainIOThreadIDFind(vm->def, iothread_id))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find just added IOThread '%u'"), + 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 (qemuDomainHotplugAddPin(vm->def->cpumask, iothread_id, + &vm->def->cputune.iothreadspin, + &vm->def->cputune.niothreadspin) < 0) + + goto cleanup; + + if (qemuDomainHotplugPinThread(vm->def->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); + if (cgroup_iothread) + 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; + char *mem_mask = 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; + + /* After hotplugging the IOThreads we need to re-detect the + * IOThreads thread_id's, adjust the cgroups, thread affinity, + * and 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 (VIR_DELETE_ELEMENT(vm->def->iothreadids, idx, + vm->def->niothreadids) < 0) + goto cleanup; + + if (qemuDomainDelCgroupForThread(priv->cgroup, + VIR_CGROUP_THREAD_IOTHREAD, + iothread_id) < 0) + goto cleanup; + + virDomainPinDel(&vm->def->cputune.iothreadspin, + &vm->def->cputune.niothreadspin, + iothread_id); + + 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); + 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 (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) < 0) + goto endjob; + + /* Nothing to do in iothreadspin list (that's a separate command) */ + + persistentDef->iothreads++; + } else { + if (!virDomainIOThreadIDFind(persistentDef, iothread_id)) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot find IOThread '%u' in persistent " + "iothreadids"), + iothread_id); + goto cleanup; + } + + virDomainIOThreadIDDel(persistentDef, iothread_id); + virDomainPinDel(&persistentDef->cputune.iothreadspin, + &persistentDef->cputune.niothreadspin, + 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 path '%s'"), + iothread_id, + NULLSTR(vm->def->disks[i]->src->path)); + 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; @@ -19910,6 +20339,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 14, 2015 at 21:18:25 -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 | 431 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 447 insertions(+)
...
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 008258f..f42d4fb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6179,6 +6179,435 @@ 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; + unsigned int idval = 0; + 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)
Since we are not doing any fancy iothread naming, this function can parse the iothread IDs from the alias right away ... [1]
+ 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 + */
The message seems obvious when looking at the code.
+ for (idx = 0; idx < new_niothreads; idx++) { + if (qemuDomainParseIOThreadAlias(new_iothreads[idx]->name, &idval) < 0)
... [1] so that you don't have to do it manually.
+ goto cleanup; + if (iothread_id == idval) + break; + } + + if (idval != iothread_id) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find new IOThread '%u' in QEMU monitor."), + iothread_id); + goto cleanup; + } + + if (virDomainIOThreadIDAdd(vm->def, iothread_id) < 0)
virDomainIOThreadIDAdd could return the pointer to the created item ...
+ goto cleanup; + + if (!(iothrid = virDomainIOThreadIDFind(vm->def, iothread_id))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find just added IOThread '%u'"), + iothread_id);
So that you don't have to look it up right after adding it.
+ 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) {
Automatic NUMA placement(priv->autoCpuset) needs to be taken into account too.
+ if (qemuDomainHotplugAddPin(vm->def->cpumask, iothread_id, + &vm->def->cputune.iothreadspin, + &vm->def->cputune.niothreadspin) < 0) + + goto cleanup; + + if (qemuDomainHotplugPinThread(vm->def->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)
qemuProcessSetSchedParams won't do anything since the new thread doesn't have any scheduler assigned.
+ 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); + if (cgroup_iothread) + virCgroupFree(&cgroup_iothread);
virCgroupFree() handles NULL just fine.
+ 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; + char *mem_mask = 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; + + /* After hotplugging the IOThreads we need to re-detect the + * IOThreads thread_id's, adjust the cgroups, thread affinity, + * and the vm->def->iothreadids list. + */
You've removed the thread here, so thread affinity was destroyed by the thread exitting.
+ 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)
Why do you need the memory node mask when you are deleting the cgroup?
+ goto cleanup; + + if (VIR_DELETE_ELEMENT(vm->def->iothreadids, idx, + vm->def->niothreadids) < 0) + goto cleanup;
You've added virDomainIOThreadIDDel
+ + if (qemuDomainDelCgroupForThread(priv->cgroup, + VIR_CGROUP_THREAD_IOTHREAD, + iothread_id) < 0) + goto cleanup; + + virDomainPinDel(&vm->def->cputune.iothreadspin, + &vm->def->cputune.niothreadspin, + iothread_id); + + 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); + 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 (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0,
Wrong cgroup type. Additionally qemuDomainHotplugAddIOThread() will add the thread so adding it here doesn't make sense.
+ 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) < 0) + goto endjob; + + /* Nothing to do in iothreadspin list (that's a separate command) */ + + persistentDef->iothreads++; + } else { + if (!virDomainIOThreadIDFind(persistentDef, iothread_id)) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot find IOThread '%u' in persistent " + "iothreadids"), + iothread_id); + goto cleanup; + } + + virDomainIOThreadIDDel(persistentDef, iothread_id); + virDomainPinDel(&persistentDef->cputune.iothreadspin, + &persistentDef->cputune.niothreadspin, + iothread_id);
This is the reason why I've requested in the previous review that the pinning information would be merged into the iothread data structure. You then would not have to synchronise two data structures.
+ 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 path '%s'"), + iothread_id, + NULLSTR(vm->def->disks[i]->src->path));
Alternatively you can use vm->def->disks[i]->dst which should be always set.
+ 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;
On Tue, Apr 21, 2015 at 15:45:48 +0200, Peter Krempa wrote:
On Tue, Apr 14, 2015 at 21:18:25 -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 | 431 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 447 insertions(+)
...
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 008258f..f42d4fb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6179,6 +6179,435 @@ qemuDomainPinIOThread(virDomainPtr dom,
...
+ + if (virDomainIOThreadIDAdd(vm->def, iothread_id) < 0)
virDomainIOThreadIDAdd could return the pointer to the created item ...
+ goto cleanup; + + if (!(iothrid = virDomainIOThreadIDFind(vm->def, iothread_id))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find just added IOThread '%u'"), + iothread_id);
So that you don't have to look it up right after adding it.
+ goto cleanup; + } + + iothrid->thread_id = new_iothreads[idx]->thread_id;
You are also not marking the thread structure as 'defined' and thus wouldn't format it later ...
+ + /* 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) {
Automatic NUMA placement(priv->autoCpuset) needs to be taken into account too.
+ if (qemuDomainHotplugAddPin(vm->def->cpumask, iothread_id, + &vm->def->cputune.iothreadspin, + &vm->def->cputune.niothreadspin) < 0) + + goto cleanup; + + if (qemuDomainHotplugPinThread(vm->def->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)
qemuProcessSetSchedParams won't do anything since the new thread doesn't have any scheduler assigned.
+ 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); + if (cgroup_iothread) + virCgroupFree(&cgroup_iothread);
virCgroupFree() handles NULL just fine.
+ VIR_FREE(alias); + return ret; + + exit_monitor: + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; +}
Peter
+ goto cleanup; + } + + iothrid->thread_id = new_iothreads[idx]->thread_id;
You are also not marking the thread structure as 'defined' and thus wouldn't format it later ...
Changed to 'undefined' now, so this is unnecessary (although perhaps 'autofill' is a better term.... John
On 04/21/2015 09:45 AM, Peter Krempa wrote:
On Tue, Apr 14, 2015 at 21:18:25 -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 | 431 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 447 insertions(+)
...
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 008258f..f42d4fb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6179,6 +6179,435 @@ 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; + unsigned int idval = 0; + 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)
Since we are not doing any fancy iothread naming, this function can parse the iothread IDs from the alias right away ... [1]
+ 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 + */
The message seems obvious when looking at the code.
+ for (idx = 0; idx < new_niothreads; idx++) { + if (qemuDomainParseIOThreadAlias(new_iothreads[idx]->name, &idval) < 0)
... [1] so that you don't have to do it manually.
IOW: if (STREQ(alias, new_iothreads[idx]->name)) break
+ goto cleanup; + if (iothread_id == idval) + break; + } + + if (idval != iothread_id) {
And of course idx != new_niothreads, plus removing 'idval'
+ virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find new IOThread '%u' in QEMU monitor."), + iothread_id); + goto cleanup; + } + + if (virDomainIOThreadIDAdd(vm->def, iothread_id) < 0)
virDomainIOThreadIDAdd could return the pointer to the created item ...
OK, now we have if (!(iothrid = virDomainIOThreadIDAdd(vm->def, iothread_id))) goto cleanup; iothrid->thread_id = new_iothreads[idx]->thread_id;
+ goto cleanup; + + if (!(iothrid = virDomainIOThreadIDFind(vm->def, iothread_id))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find just added IOThread '%u'"), + iothread_id);
So that you don't have to look it up right after adding it.
+ 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) {
Automatic NUMA placement(priv->autoCpuset) needs to be taken into account too.
Meaning? There's a reference for that in qemuDomainAddCgroupForThread. Obviously I'm following the model of qemuDomainHotplugVcpus when "if (nvcpus > oldvcpus) {"
+ if (qemuDomainHotplugAddPin(vm->def->cpumask, iothread_id, + &vm->def->cputune.iothreadspin, + &vm->def->cputune.niothreadspin) < 0) + + goto cleanup; + + if (qemuDomainHotplugPinThread(vm->def->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)
qemuProcessSetSchedParams won't do anything since the new thread doesn't have any scheduler assigned.
So it's wrong in qemuDomainHotplugVcpus ? Is someone else fixing that? Is the expectation that I just remove this?
+ 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); + if (cgroup_iothread) + virCgroupFree(&cgroup_iothread);
virCgroupFree() handles NULL just fine.
OK - again copied from qemuDomainHotplugVcpus
+ 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; + char *mem_mask = 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; + + /* After hotplugging the IOThreads we need to re-detect the + * IOThreads thread_id's, adjust the cgroups, thread affinity, + * and the vm->def->iothreadids list. + */
You've removed the thread here, so thread affinity was destroyed by the thread exitting.
relic of me cut-n-paste from formerly combined code. removed the comment completely, no replacement.
+ 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)
Why do you need the memory node mask when you are deleting the cgroup?
Oh right - relic of the copy... gone.
+ goto cleanup; + + if (VIR_DELETE_ELEMENT(vm->def->iothreadids, idx, + vm->def->niothreadids) < 0) + goto cleanup;
You've added virDomainIOThreadIDDel
yep, change to : virDomainIOThreadIDDel(vm->def, iothread_id);
+ + if (qemuDomainDelCgroupForThread(priv->cgroup, + VIR_CGROUP_THREAD_IOTHREAD, + iothread_id) < 0) + goto cleanup; + + virDomainPinDel(&vm->def->cputune.iothreadspin, + &vm->def->cputune.niothreadspin, + iothread_id); + + 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); + 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 (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0,
Wrong cgroup type. Additionally qemuDomainHotplugAddIOThread() will add the thread so adding it here doesn't make sense.
See qemuDomainSetVcpusFlags, it too uses VIR_CGROUP_THREAD_EMULATOR so this is no different... In fact I suppose I could have created some sort of common API, but didn't. One thing I just noticed - commit id '6cf1e11' added a "&& virNumaIsAvailable()" check in qemuDomainSetVcpusFlags, so I suppose to follow that - I can add the extra check. Beyond that I'm not sure what you want.
+ 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) < 0) + goto endjob; + + /* Nothing to do in iothreadspin list (that's a separate command) */ + + persistentDef->iothreads++; + } else { + if (!virDomainIOThreadIDFind(persistentDef, iothread_id)) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot find IOThread '%u' in persistent " + "iothreadids"), + iothread_id); + goto cleanup; + } + + virDomainIOThreadIDDel(persistentDef, iothread_id); + virDomainPinDel(&persistentDef->cputune.iothreadspin, + &persistentDef->cputune.niothreadspin, + iothread_id);
This is the reason why I've requested in the previous review that the pinning information would be merged into the iothread data structure. You then would not have to synchronise two data structures.
Understood ... I started down that path, but then got bogged down in cputune information inside iothreadid's and the difference with the vCPU code. So I kept it this way to reduce the number of changes. I think it's worthy of being done, but I hope a follow-up patch will be acceptable.
+ 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 path '%s'"), + iothread_id, + NULLSTR(vm->def->disks[i]->src->path));
Alternatively you can use vm->def->disks[i]->dst which should be always set.
OK
+ 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;
So with all that the changes that would be sqashed in: diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5d3c7fc..e1fdeb5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6188,7 +6188,6 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver, unsigned int exp_niothreads = vm->def->iothreads; int new_niothreads = 0; qemuMonitorIOThreadInfoPtr *new_iothreads = NULL; - unsigned int idval = 0; virCgroupPtr cgroup_iothread = NULL; char *mem_mask = NULL; virDomainIOThreadIDDefPtr iothrid; @@ -6244,29 +6243,20 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver, * in the QEMU IOThread list, so we can add it to our iothreadids list */ for (idx = 0; idx < new_niothreads; idx++) { - if (qemuDomainParseIOThreadAlias(new_iothreads[idx]->name, &idval) < 0) - goto cleanup; - if (iothread_id == idval) + if (STREQ(new_iothreads[idx]->name, alias)) break; } - if (idval != iothread_id) { + if (idx != new_niothreads) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot find new IOThread '%u' in QEMU monitor."), iothread_id); goto cleanup; } - if (virDomainIOThreadIDAdd(vm->def, iothread_id) < 0) + if (!(iothrid = virDomainIOThreadIDAdd(vm->def, iothread_id))) goto cleanup; - if (!(iothrid = virDomainIOThreadIDFind(vm->def, iothread_id))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot find just added IOThread '%u'"), - iothread_id); - goto cleanup; - } - iothrid->thread_id = new_iothreads[idx]->thread_id; /* Add IOThread to cgroup if present */ @@ -6309,8 +6299,7 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver, VIR_FREE(mem_mask); virDomainAuditIOThread(vm, orig_niothreads, new_niothreads, "update", rc == 0); - if (cgroup_iothread) - virCgroupFree(&cgroup_iothread); + virCgroupFree(&cgroup_iothread); VIR_FREE(alias); return ret; @@ -6333,7 +6322,6 @@ qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver, unsigned int exp_niothreads = vm->def->iothreads; int new_niothreads = 0; qemuMonitorIOThreadInfoPtr *new_iothreads = NULL; - char *mem_mask = NULL; /* Normally would use virDomainIOThreadIDFind, but we need the index * from whence to delete for later... @@ -6360,10 +6348,6 @@ qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver, 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 the vm->def->iothreadids list. - */ if ((new_niothreads = qemuMonitorGetIOThreads(priv->mon, &new_iothreads)) < 0) goto exit_monitor; @@ -6381,16 +6365,7 @@ qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver, } 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 (VIR_DELETE_ELEMENT(vm->def->iothreadids, idx, - vm->def->niothreadids) < 0) - goto cleanup; + virDomainIOThreadIDDel(vm->def, iothread_id); if (qemuDomainDelCgroupForThread(priv->cgroup, VIR_CGROUP_THREAD_IOTHREAD, @@ -6409,7 +6384,6 @@ qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver, qemuMonitorIOThreadInfoFree(new_iothreads[idx]); VIR_FREE(new_iothreads); } - VIR_FREE(mem_mask); virDomainAuditIOThread(vm, orig_niothreads, new_niothreads, "update", rc == 0); VIR_FREE(alias); @@ -6464,19 +6438,21 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver, goto endjob; } - if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0, - false, &cgroup_temp) < 0) - 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 = virNumaGetHostNodeset())) + goto endjob; - if (!(all_nodes_str = virBitmapFormat(all_nodes))) - 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 (virCgroupGetCpusetMems(cgroup_temp, &mem_mask) < 0 || + virCgroupSetCpusetMems(cgroup_temp, all_nodes_str) < 0) + goto endjob; + } if (add) { if (qemuDomainHotplugAddIOThread(driver, vm, iothread_id) < 0) @@ -6492,7 +6468,7 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver, if (flags & VIR_DOMAIN_AFFECT_CONFIG) { if (add) { - if (virDomainIOThreadIDAdd(persistentDef, iothread_id) < 0) + if (!virDomainIOThreadIDAdd(persistentDef, iothread_id)) goto endjob; /* Nothing to do in iothreadspin list (that's a separate command) */ @@ -6590,8 +6566,7 @@ qemuDomainDelIOThread(virDomainPtr dom, virReportError(VIR_ERR_INVALID_ARG, _("cannot remove IOThread %u since it " "is being used by disk path '%s'"), - iothread_id, - NULLSTR(vm->def->disks[i]->src->path)); + iothread_id, vm->def->disks[i]->dst); goto cleanup; } }
...
This is the reason why I've requested in the previous review that the pinning information would be merged into the iothread data structure. You then would not have to synchronise two data structures.
Understood ... I started down that path, but then got bogged down in cputune information inside iothreadid's and the difference with the vCPU code. So I kept it this way to reduce the number of changes.
I think it's worthy of being done, but I hope a follow-up patch will be acceptable.
ug... starting looking at it again with the "existing" view of iothreadid's fresh in mind and it seems I'll have to jump in during this series because there's a check in virDomainIOThreadPinDefParseXML for pin->id > iothreads, which won't be true once "Add" hits, so I'll work it in now <sigh> John
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 bc6054a..680424f 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6883,6 +6883,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[] = { @@ -12812,6 +12964,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
participants (2)
-
John Ferlan -
Peter Krempa