[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]