Re: [libvirt] [PATCH v4 4/9] Move iothreadspin information into iothreadids

On Tue, Apr 21, 2015 at 19:31:25 -0400, John Ferlan wrote:

Remove the iothreadspin array from cputune and replace with a cpumask to be stored in the iothreadids list

Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 10 ++-- src/conf/domain_conf.c | 118 +++++++++++++++++++++------------------------- src/conf/domain_conf.h | 2 +- src/qemu/qemu_cgroup.c | 13 ++--- src/qemu/qemu_driver.c | 79 +++++++++---------------------- src/qemu/qemu_process.c | 7 +-- 6 files changed, 86 insertions(+), 143 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 518f7c5..7af6bd7 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -624,11 +624,11 @@ and attribute <code>cpuset</code> of element <code>vcpu</code> is not specified, the IOThreads are pinned to all the physical CPUs by default. There are two required attributes, the attribute - <code>iothread</code> specifies the IOThread id and the attribute - <code>cpuset</code> specifying which physical CPUs to pin to. The - <code>iothread</code> value begins at "1" through the number of - <a href="#elementsIOThreadsAllocation"><code>iothreads</code></a> - allocated to the domain. A value of "0" is not permitted. + <code>iothread</code> specifies the IOThread ID and the attribute + <code>cpuset</code> specifying which physical CPUs to pin to. See + the <code>iothreadids</code> + <a href="#elementsIOThreadsAllocation"><code>description</code></a> + for valid <code>iothread</code> values.

This description is fair enough. It hints that the implicit ones are numbered too.

<span class="since">Since 1.2.9</span> </dd> <dt><code>shares</code></dt> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bd25d52..969e56f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2118,8 +2118,10 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int npin) void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def) { - if (def) + if (def) { + virBitmapFree(def->cpumask); VIR_FREE(def);

This fixes the syntax check failure from 1/9.

+ } }

@@ -2341,9 +2343,6 @@ void virDomainDefFree(virDomainDefPtr def)

virDomainPinDefFree(def->cputune.emulatorpin);

- virDomainPinDefArrayFree(def->cputune.iothreadspin, - def->cputune.niothreadspin); - for (i = 0; i < def->cputune.nvcpusched; i++) virBitmapFree(def->cputune.vcpusched[i].ids); VIR_FREE(def->cputune.vcpusched); @@ -13316,74 +13315,77 @@ virDomainVcpuPinDefParseXML(xmlNodePtr node, * and an iothreadspin has the form * <iothreadpin iothread='1' cpuset='2'/> */ -static virDomainPinDefPtr +static int virDomainIOThreadPinDefParseXML(xmlNodePtr node, xmlXPathContextPtr ctxt, - int iothreads) + virDomainDefPtr def) { - virDomainPinDefPtr def; + int ret = -1; + virDomainIOThreadIDDefPtr iothrid; + virBitmapPtr cpumask; xmlNodePtr oldnode = ctxt->node; unsigned int iothreadid; char *tmp = NULL;

- if (VIR_ALLOC(def) < 0) - return NULL; - ctxt->node = node;

if (!(tmp = virXPathString("string(./@iothread)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing iothread id in iothreadpin")); - goto error; + goto cleanup; }

if (virStrToLong_uip(tmp, NULL, 10, &iothreadid) < 0) { virReportError(VIR_ERR_XML_ERROR, _("invalid setting for iothread '%s'"), tmp); - goto error; + goto cleanup; } VIR_FREE(tmp);

if (iothreadid == 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("zero is an invalid iothread id value")); - goto error; - } - - /* IOThreads are numbered "iothread1...iothread<n>", where - * "n" is the iothreads value */ - if (iothreadid > iothreads) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("iothread id must not exceed iothreads"));

[1]

- goto error; + goto cleanup; }

- def->id = iothreadid; - if (!(tmp = virXMLPropString(node, "cpuset"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing cpuset for iothreadpin")); - goto error; + goto cleanup; }

- if (virBitmapParse(tmp, 0, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) - goto error; + if (virBitmapParse(tmp, 0, &cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) + goto cleanup;

- if (virBitmapIsAllClear(def->cpumask)) { + if (virBitmapIsAllClear(cpumask)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid value of 'cpuset': %s"), tmp); - goto error; + goto cleanup; + } + + if (!(iothrid = virDomainIOThreadIDFind(def, iothreadid))) { + if (!(iothrid = virDomainIOThreadIDAdd(def, iothreadid)))

Why are you adding a iothread definition here? This code is executed after the iothread info should already be parsed so adding a thread here doesn't make sense. The section here should ressemble the check [1] that you've removed.

+ goto cleanup; + iothrid->autofill = true; + } + + if (iothrid->cpumask) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("duplicate iothreadpin for same iothread '%u'"), + iothreadid); + goto cleanup; }

+ iothrid->cpumask = cpumask; + cpumask = NULL; + ret = 0; + cleanup: VIR_FREE(tmp); + virBitmapFree(cpumask); ctxt->node = oldnode; - return def; - - error: - VIR_FREE(def); - goto cleanup; + return ret; }

@@ -14250,28 +14252,11 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; }

- if (n && VIR_ALLOC_N(def->cputune.iothreadspin, n) < 0) - goto error; - for (i = 0; i < n; i++) { - virDomainPinDefPtr iothreadpin = NULL; - iothreadpin = virDomainIOThreadPinDefParseXML(nodes[i], ctxt, - def->iothreads); - if (!iothreadpin) + if (virDomainIOThreadPinDefParseXML(nodes[i], ctxt, def) < 0) goto error; - - if (virDomainPinIsDuplicate(def->cputune.iothreadspin, - def->cputune.niothreadspin, - iothreadpin->id)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("duplicate iothreadpin for same iothread")); - virDomainPinDefFree(iothreadpin); - goto error; - } - - def->cputune.iothreadspin[def->cputune.niothreadspin++] = - iothreadpin; } + def->cputune.niothreadspin = n;

This variable should be removed along with the iothread pinning structure, shouldn't it?

VIR_FREE(nodes);

if ((n = virXPathNodeSet("./cputune/vcpusched", ctxt, &nodes)) < 0) { @@ -14384,7 +14369,7 @@ virDomainDefParseXML(xmlDocPtr xml,

if (virDomainNumatuneHasPlacementAuto(def->numa) && !def->cpumask && !def->cputune.vcpupin && - !def->cputune.emulatorpin && !def->cputune.iothreadspin) + !def->cputune.emulatorpin && !def->cputune.niothreadspin)

This could be replaced by a function that checks whether any of the threads has pinning info present rather than counting the pinning info.

def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO;

if ((n = virXPathNodeSet("./resource", ctxt, &nodes)) < 0) { @@ -20837,20 +20822,25 @@ virDomainDefFormatInternal(virDomainDefPtr def, VIR_FREE(cpumask); }

- for (i = 0; i < def->cputune.niothreadspin; i++) { - char *cpumask; - /* Ignore the iothreadpin which inherit from "cpuset of "<vcpu>." */ - if (virBitmapEqual(def->cpumask, def->cputune.iothreadspin[i]->cpumask)) - continue; + if (def->cputune.niothreadspin) {

The check above is useless. If neither of the iothreads has pin info, it would be skipped ...

+ for (i = 0; i < def->niothreadids; i++) { + char *cpumask;

- virBufferAsprintf(buf, "<iothreadpin iothread='%u' ", - def->cputune.iothreadspin[i]->id); + /* Ignore iothreadids with no cpumask or a cpumask that + * inherits from "cpuset of "<vcpu>." */ + if (!def->iothreadids[i]->cpumask || + virBitmapEqual(def->cpumask, def->iothreadids[i]->cpumask)) + continue;

... here. Also why are you explicitly rejecting cpumask that is equal to the default one? If a user explicitly specifies it that way then the info would be lost. Additionally, def->cpumask here is used on the iothread automatically without filling it to the structure if it's used so there's no need to do that.

- if (!(cpumask = virBitmapFormat(def->cputune.iothreadspin[i]->cpumask))) - goto error; + virBufferAsprintf(buf, "<iothreadpin iothread='%u' ", + def->iothreadids[i]->iothread_id);

- virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask); - VIR_FREE(cpumask); + if (!(cpumask = virBitmapFormat(def->iothreadids[i]->cpumask))) + goto error; + + virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask); + VIR_FREE(cpumask); + } }

for (i = 0; i < def->cputune.nvcpusched; i++) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index cc98f3d..c71e1b8 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2057,6 +2057,7 @@ struct _virDomainIOThreadIDDef { bool autofill; unsigned int iothread_id; int thread_id; + virBitmapPtr cpumask; };

void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def); @@ -2075,7 +2076,6 @@ struct _virDomainCputune { virDomainPinDefPtr *vcpupin; virDomainPinDefPtr emulatorpin; size_t niothreadspin;

This one should be removed too.

- virDomainPinDefPtr *iothreadspin;

size_t nvcpusched; virDomainThreadSchedParamPtr vcpusched; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index cdf0aaf..5282449 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1149,7 +1149,7 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) virCgroupPtr cgroup_iothread = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr def = vm->def; - size_t i, j; + size_t i; unsigned long long period = vm->def->cputune.period; long long quota = vm->def->cputune.quota; char *mem_mask = NULL; @@ -1214,18 +1214,11 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) /* default cpu masks */ if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) cpumask = priv->autoCpuset; + else if (def->iothreadids[i]->cpumask) + cpumask = def->iothreadids[i]->cpumask;

This has to be the first case. Even if VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO is specified, it cannot override what the user configured pinning.

else cpumask = def->cpumask;

- /* specific cpu mask */ - for (j = 0; j < def->cputune.niothreadspin; j++) { - if (def->cputune.iothreadspin[j]->id == - def->iothreadids[i]->iothread_id) { - cpumask = def->cputune.iothreadspin[j]->cpumask; - break; - }

Finally, this can be removed!

- } - if (cpumask && qemuSetupCgroupCpusetCpus(cgroup_iothread, cpumask) < 0) goto cleanup; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0f95cc7..ee13d08 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5894,19 +5894,14 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef, goto cleanup;

for (i = 0; i < targetDef->niothreadids; i++) { - virDomainPinDefPtr pininfo; - if (VIR_ALLOC(info_ret[i]) < 0) goto cleanup;

/* IOThread ID's are taken from the iothreadids list */ info_ret[i]->iothread_id = targetDef->iothreadids[i]->iothread_id;

- /* Initialize the cpumap */ - pininfo = virDomainPinFind(targetDef->cputune.iothreadspin, - targetDef->cputune.niothreadspin, - targetDef->iothreadids[i]->iothread_id); - if (!pininfo) { + cpumask = targetDef->iothreadids[i]->cpumask; + if (!cpumask) { if (targetDef->cpumask) { cpumask = targetDef->cpumask; } else { @@ -5915,8 +5910,6 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef, virBitmapSetAll(bitmap); cpumask = bitmap; } - } else { - cpumask = pininfo->cpumask; } if (virBitmapToData(cpumask, &info_ret[i]->cpumap, &info_ret[i]->cpumaplen) < 0) @@ -5999,8 +5992,6 @@ qemuDomainPinIOThread(virDomainPtr dom, virDomainDefPtr persistentDef = NULL; virBitmapPtr pcpumap = NULL; qemuDomainObjPrivatePtr priv; - virDomainPinDefPtr *newIOThreadsPin = NULL; - size_t newIOThreadsPinNum = 0; virCgroupPtr cgroup_iothread = NULL; virObjectEventPtr event = NULL; char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = ""; @@ -6050,33 +6041,21 @@ qemuDomainPinIOThread(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_LIVE) {

virDomainIOThreadIDDefPtr iothrid; + virBitmapPtr cpumask;

if (!(iothrid = virDomainIOThreadIDFind(vm->def, iothread_id))) { virReportError(VIR_ERR_INVALID_ARG, - _("iothread value %d not found"), iothread_id); + _("iothreadid %d not found"), iothread_id);

I'd compromise between those two. "iothread %d not foud"

goto endjob; }

- if (vm->def->cputune.iothreadspin) { - newIOThreadsPin = - virDomainPinDefCopy(vm->def->cputune.iothreadspin, - vm->def->cputune.niothreadspin); - if (!newIOThreadsPin) - goto endjob; - - newIOThreadsPinNum = vm->def->cputune.niothreadspin; - } else { - if (VIR_ALLOC(newIOThreadsPin) < 0) - goto endjob; - newIOThreadsPinNum = 0; - } - - if (virDomainPinAdd(&newIOThreadsPin, &newIOThreadsPinNum, - cpumap, maplen, iothread_id) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update iothreadspin")); + if (!(cpumask = virBitmapNewData(cpumap, maplen))) goto endjob; - } + + if (!iothrid->cpumask) + vm->def->cputune.niothreadspin++;

This should go. If you add a function that checks whether pinning s specified it will not be needed.

+ virBitmapFree(iothrid->cpumask); + iothrid->cpumask = cpumask;

/* Configure the corresponding cpuset cgroup before set affinity. */ if (virCgroupHasController(priv->cgroup, @@ -6099,14 +6078,6 @@ qemuDomainPinIOThread(virDomainPtr dom, } }

- if (vm->def->cputune.iothreadspin) - virDomainPinDefArrayFree(vm->def->cputune.iothreadspin, - vm->def->cputune.niothreadspin); - - vm->def->cputune.iothreadspin = newIOThreadsPin; - vm->def->cputune.niothreadspin = newIOThreadsPinNum; - newIOThreadsPin = NULL; - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto endjob;

@@ -6124,31 +6095,25 @@ qemuDomainPinIOThread(virDomainPtr dom, }

if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + virDomainIOThreadIDDefPtr iothrid; + virBitmapPtr cpumask; + /* Coverity didn't realize that targetDef must be set if we got here. */ sa_assert(persistentDef);

- if (iothread_id > persistentDef->iothreads) { + if (!(iothrid = virDomainIOThreadIDFind(persistentDef, iothread_id))) { virReportError(VIR_ERR_INVALID_ARG, - _("iothread value out of range %d > %d"), - iothread_id, persistentDef->iothreads); + _("iothreadid %d not found"), iothread_id); goto endjob; }

- if (!persistentDef->cputune.iothreadspin) { - if (VIR_ALLOC(persistentDef->cputune.iothreadspin) < 0) - goto endjob; - persistentDef->cputune.niothreadspin = 0; - } - if (virDomainPinAdd(&persistentDef->cputune.iothreadspin, - &persistentDef->cputune.niothreadspin, - cpumap, - maplen, - iothread_id) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update or add iothreadspin xml " - "of a persistent domain")); + if (!(cpumask = virBitmapNewData(cpumap, maplen))) goto endjob; - } + + if (!iothrid->cpumask) + persistentDef->cputune.niothreadspin++;

Again, this should not be necessary.

+ virBitmapFree(iothrid->cpumask); + iothrid->cpumask = cpumask;

ret = virDomainSaveConfig(cfg->configDir, persistentDef); goto endjob;

The code looks much better now, but this patch has still some nits. Peter