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@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list