[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 Thu, Apr 23, 2015 at 08:58:26 -0400, John Ferlan wrote:
> 
> 
> On 04/23/2015 08:02 AM, Peter Krempa wrote:
> > On Tue, Apr 21, 2015 at 19:31:25 -0400, John Ferlan wrote:
> >> Remove the iothreadspin array from cputune and replace with a cpumask
> >> to be stored in the iothreadids list
> >>
> >> Signed-off-by: John Ferlan <jferlan redhat com>
> >> ---
> >>  docs/formatdomain.html.in |  10 ++--
> >>  src/conf/domain_conf.c    | 118 +++++++++++++++++++++-------------------------
> >>  src/conf/domain_conf.h    |   2 +-
> >>  src/qemu/qemu_cgroup.c    |  13 ++---
> >>  src/qemu/qemu_driver.c    |  79 +++++++++----------------------
> >>  src/qemu/qemu_process.c   |   7 +--
> >>  6 files changed, 86 insertions(+), 143 deletions(-)
> >>
> >> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> >> index 518f7c5..7af6bd7 100644
> >> --- a/docs/formatdomain.html.in
> >> +++ b/docs/formatdomain.html.in
> >> @@ -624,11 +624,11 @@
> >>           and attribute <code>cpuset</code> of element <code>vcpu</code> is
> >>           not specified, the IOThreads are pinned to all the physical CPUs
> >>           by default. There are two required attributes, the attribute
> >> -         <code>iothread</code> specifies the IOThread id and the attribute
> >> -         <code>cpuset</code> specifying which physical CPUs to pin to. The
> >> -         <code>iothread</code> value begins at "1" through the number of
> >> -          <a href="#elementsIOThreadsAllocation"><code>iothreads</code></a>
> >> -         allocated to the domain. A value of "0" is not permitted.
> >> +         <code>iothread</code> specifies the IOThread ID and the attribute
> >> +         <code>cpuset</code> specifying which physical CPUs to pin to. See
> >> +         the <code>iothreadids</code>
> >> +         <a href="#elementsIOThreadsAllocation"><code>description</code></a>
> >> +         for valid <code>iothread</code> values.
> > 
> > This description is fair enough. It hints that the implicit ones are
> > numbered too.
> > 
> >>          <span class="since">Since 1.2.9</span>
> >>         </dd>
> >>        <dt><code>shares</code></dt>
> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> >> index bd25d52..969e56f 100644
> >> --- a/src/conf/domain_conf.c
> >> +++ b/src/conf/domain_conf.c
> >> @@ -2118,8 +2118,10 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int npin)
> >>  void
> >>  virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def)
> >>  {
> >> -    if (def)
> >> +    if (def) {
> >> +        virBitmapFree(def->cpumask);
> >>          VIR_FREE(def);
> > 
> > This fixes the syntax check failure from 1/9.
> > 
> 
> Byproduct of rewrites...
> 
> I've change 1/9 to be (so that each patch passes check/syntax-check)
> 
> +    if (!def)
> +        return;
> +    VIR_FREE(def);
> 
> 
> Which will change this to be:
>      if (!def)
>          return;
> +    virBitmapFree(def->cpumask);
>      VIR_FREE(def);
> 
> 
> 
> >> +    }
> >>  }
> >>  
> >>  
> >> @@ -2341,9 +2343,6 @@ void virDomainDefFree(virDomainDefPtr def)
> >>  
> >>      virDomainPinDefFree(def->cputune.emulatorpin);
> >>  
> >> -    virDomainPinDefArrayFree(def->cputune.iothreadspin,
> >> -                                 def->cputune.niothreadspin);
> >> -
> >>      for (i = 0; i < def->cputune.nvcpusched; i++)
> >>          virBitmapFree(def->cputune.vcpusched[i].ids);
> >>      VIR_FREE(def->cputune.vcpusched);
> >> @@ -13316,74 +13315,77 @@ virDomainVcpuPinDefParseXML(xmlNodePtr node,
> >>   * and an iothreadspin has the form
> >>   *   <iothreadpin iothread='1' cpuset='2'/>
> >>   */
> >> -static virDomainPinDefPtr
> >> +static int
> >>  virDomainIOThreadPinDefParseXML(xmlNodePtr node,
> >>                                  xmlXPathContextPtr ctxt,
> >> -                                int iothreads)
> >> +                                virDomainDefPtr def)
> >>  {
> >> -    virDomainPinDefPtr def;
> >> +    int ret = -1;
> >> +    virDomainIOThreadIDDefPtr iothrid;
> >> +    virBitmapPtr cpumask;
> >>      xmlNodePtr oldnode = ctxt->node;
> >>      unsigned int iothreadid;
> >>      char *tmp = NULL;
> >>  
> >> -    if (VIR_ALLOC(def) < 0)
> >> -        return NULL;
> >> -
> >>      ctxt->node = node;
> >>  
> >>      if (!(tmp = virXPathString("string(./@iothread)", ctxt))) {
> >>          virReportError(VIR_ERR_XML_ERROR, "%s",
> >>                         _("missing iothread id in iothreadpin"));
> >> -        goto error;
> >> +        goto cleanup;
> >>      }
> >>  
> >>      if (virStrToLong_uip(tmp, NULL, 10, &iothreadid) < 0) {
> >>          virReportError(VIR_ERR_XML_ERROR,
> >>                         _("invalid setting for iothread '%s'"), tmp);
> >> -        goto error;
> >> +        goto cleanup;
> >>      }
> >>      VIR_FREE(tmp);
> >>  
> >>      if (iothreadid == 0) {
> >>          virReportError(VIR_ERR_XML_ERROR, "%s",
> >>                         _("zero is an invalid iothread id value"));
> >> -        goto error;
> >> -    }
> >> -
> >> -    /* IOThreads are numbered "iothread1...iothread<n>", where
> >> -     * "n" is the iothreads value */
> >> -    if (iothreadid > iothreads) {
> >> -        virReportError(VIR_ERR_XML_ERROR, "%s",
> >> -                       _("iothread id must not exceed iothreads"));
> > 
> > [1]
> > 
> >> -        goto error;
> >> +        goto cleanup;
> >>      }
> >>  
> >> -    def->id = iothreadid;
> >> -
> >>      if (!(tmp = virXMLPropString(node, "cpuset"))) {
> >>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >>                         _("missing cpuset for iothreadpin"));
> >> -        goto error;
> >> +        goto cleanup;
> >>      }
> >>  
> >> -    if (virBitmapParse(tmp, 0, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0)
> >> -        goto error;
> >> +    if (virBitmapParse(tmp, 0, &cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0)
> >> +        goto cleanup;
> >>  
> >> -    if (virBitmapIsAllClear(def->cpumask)) {
> >> +    if (virBitmapIsAllClear(cpumask)) {
> >>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> >>                         _("Invalid value of 'cpuset': %s"),
> >>                         tmp);
> >> -        goto error;
> >> +        goto cleanup;
> >> +    }
> >> +
> >> +    if (!(iothrid = virDomainIOThreadIDFind(def, iothreadid))) {
> >> +        if (!(iothrid = virDomainIOThreadIDAdd(def, iothreadid)))
> > 
> > Why are you adding a iothread definition here? This code is executed
> > after the iothread info should already be parsed so adding a thread here
> > doesn't make sense. The section here should ressemble the check [1] that
> > you've removed.
> > 
> 
> Because you required that the iothreadpin information to be included in
> the iothreadid; however, if someone hasn't defined <iothreadids>'s, then
> there won't be any until the PostParse is called - thus we have to add
> one here so that we can store the pin information for an iothread

Okay, I didn't see that because I've remembered a different version of
patch 1 where you allocated the full list upfront.

By the way, the approach in this series now has the following loophole:

1) Specify <iothreads> > nelemes(<iothreadids>)
2) Specify iothreadpin for thread 9999.

Now one of the threads is added with id 9999 due to the code above.

Thus I think that the PostParse callback code should probably be removed
and the missing threads should be numbered right after <iohtreadids> is
parsed so that this patch doesn't allow that.

> 
> This means someone could have:
> 
> <iothreads>4</iothreads>
> <cputune>
> ...
>     <iothreadpin iothread="1" cpuset="5,6"/>
> ...
> </cputune>
> 
> NOTE: No <iothreadids>
> 
> So I believe it stays as is
> 
> >> +            goto cleanup;
> >> +        iothrid->autofill = true;
> >> +    }
> >> +
> >> +    if (iothrid->cpumask) {
> >> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> >> +                       _("duplicate iothreadpin for same iothread '%u'"),
> >> +                       iothreadid);
> >> +        goto cleanup;
> >>      }
> >>  
> >> +    iothrid->cpumask = cpumask;
> >> +    cpumask = NULL;
> >> +    ret = 0;
> >> +
> >>   cleanup:
> >>      VIR_FREE(tmp);
> >> +    virBitmapFree(cpumask);
> >>      ctxt->node = oldnode;
> >> -    return def;
> >> -
> >> - error:
> >> -    VIR_FREE(def);
> >> -    goto cleanup;
> >> +    return ret;
> >>  }
> >>  
> >>  
> >> @@ -14250,28 +14252,11 @@ virDomainDefParseXML(xmlDocPtr xml,
> >>          goto error;
> >>      }
> >>  
> >> -    if (n && VIR_ALLOC_N(def->cputune.iothreadspin, n) < 0)
> >> -        goto error;
> >> -
> >>      for (i = 0; i < n; i++) {
> >> -        virDomainPinDefPtr iothreadpin = NULL;
> >> -        iothreadpin = virDomainIOThreadPinDefParseXML(nodes[i], ctxt,
> >> -                                                      def->iothreads);
> >> -        if (!iothreadpin)
> >> +        if (virDomainIOThreadPinDefParseXML(nodes[i], ctxt, def) < 0)
> >>              goto error;
> >> -
> >> -        if (virDomainPinIsDuplicate(def->cputune.iothreadspin,
> >> -                                    def->cputune.niothreadspin,
> >> -                                    iothreadpin->id)) {
> >> -            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >> -                           _("duplicate iothreadpin for same iothread"));
> >> -            virDomainPinDefFree(iothreadpin);
> >> -            goto error;
> >> -        }
> >> -
> >> -        def->cputune.iothreadspin[def->cputune.niothreadspin++] =
> >> -            iothreadpin;
> >>      }
> >> +    def->cputune.niothreadspin = n;
> > 
> > This variable should be removed along with the iothread pinning
> > structure, shouldn't it?
> > 
> 
> OK
> 
> >>      VIR_FREE(nodes);
> >>  
> >>      if ((n = virXPathNodeSet("./cputune/vcpusched", ctxt, &nodes)) < 0) {
> >> @@ -14384,7 +14369,7 @@ virDomainDefParseXML(xmlDocPtr xml,
> >>  
> >>      if (virDomainNumatuneHasPlacementAuto(def->numa) &&
> >>          !def->cpumask && !def->cputune.vcpupin &&
> >> -        !def->cputune.emulatorpin && !def->cputune.iothreadspin)
> >> +        !def->cputune.emulatorpin && !def->cputune.niothreadspin)
> > 
> > This could be replaced by a function that checks whether any of the
> > threads has pinning info present rather than counting the pinning info.
> > 
> 
> OK
> 
> >>          def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO;
> >>  
> >>      if ((n = virXPathNodeSet("./resource", ctxt, &nodes)) < 0) {
> >> @@ -20837,20 +20822,25 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> >>          VIR_FREE(cpumask);
> >>      }
> >>  
> >> -    for (i = 0; i < def->cputune.niothreadspin; i++) {
> >> -        char *cpumask;
> >> -        /* Ignore the iothreadpin which inherit from "cpuset of "<vcpu>." */
> >> -        if (virBitmapEqual(def->cpumask, def->cputune.iothreadspin[i]->cpumask))
> >> -            continue;
> >> +    if (def->cputune.niothreadspin) {
> > 
> > The check above is useless. If neither of the iothreads has pin info, it
> > would be skipped ...
> > 
> 
> yep
> 
> >> +        for (i = 0; i < def->niothreadids; i++) {
> >> +            char *cpumask;
> >>  
> >> -        virBufferAsprintf(buf, "<iothreadpin iothread='%u' ",
> >> -                          def->cputune.iothreadspin[i]->id);
> >> +            /* Ignore iothreadids with no cpumask or a cpumask that
> >> +             * inherits from "cpuset of "<vcpu>." */
> >> +            if (!def->iothreadids[i]->cpumask ||
> >> +                virBitmapEqual(def->cpumask, def->iothreadids[i]->cpumask))
> >> +                continue;
> > 
> > ... here. Also why are you explicitly rejecting cpumask that is equal to
> > the default one? If a user explicitly specifies it that way then the
> > info would be lost. Additionally, def->cpumask here is used on the
> > iothread automatically without filling it to the structure if it's used
> > so there's no need to do that.
> > 
> 
> Prior review - it's already there - see commit id '938fb12f'....  I'm
> fairly certain I was specifically asked to do that...

It doesn't make much sense though. The default pinning is used from
def->cpumap if the specific is not present. If the specific pinning is
removed it should be freed.

> 
> If it needs to change that should be a separate patch IMO... I think if
> you look a few lines above you'll see the same/similar check for vcpupin
> 
> >>  
> >> -        if (!(cpumask = virBitmapFormat(def->cputune.iothreadspin[i]->cpumask)))
> >> -            goto error;
> >> +            virBufferAsprintf(buf, "<iothreadpin iothread='%u' ",
> >> +                              def->iothreadids[i]->iothread_id);
> >>  
> >> -        virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask);
> >> -        VIR_FREE(cpumask);
> >> +            if (!(cpumask = virBitmapFormat(def->iothreadids[i]->cpumask)))
> >> +                goto error;
> >> +
> >> +            virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask);
> >> +            VIR_FREE(cpumask);
> >> +        }
> >>      }
> >>  
> >>      for (i = 0; i < def->cputune.nvcpusched; i++) {
> >> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> >> index cc98f3d..c71e1b8 100644
> >> --- a/src/conf/domain_conf.h
> >> +++ b/src/conf/domain_conf.h
> >> @@ -2057,6 +2057,7 @@ struct _virDomainIOThreadIDDef {
> >>      bool autofill;
> >>      unsigned int iothread_id;
> >>      int thread_id;
> >> +    virBitmapPtr cpumask;
> >>  };
> >>  
> >>  void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def);
> >> @@ -2075,7 +2076,6 @@ struct _virDomainCputune {
> >>      virDomainPinDefPtr *vcpupin;
> >>      virDomainPinDefPtr emulatorpin;
> >>      size_t niothreadspin;
> > 
> > This one should be removed too.
> > 
> 
> yep
> 
> >> -    virDomainPinDefPtr *iothreadspin;
> >>  
> >>      size_t nvcpusched;
> >>      virDomainThreadSchedParamPtr vcpusched;
> >> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> >> index cdf0aaf..5282449 100644
> >> --- a/src/qemu/qemu_cgroup.c
> >> +++ b/src/qemu/qemu_cgroup.c
> >> @@ -1149,7 +1149,7 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm)
> >>      virCgroupPtr cgroup_iothread = NULL;
> >>      qemuDomainObjPrivatePtr priv = vm->privateData;
> >>      virDomainDefPtr def = vm->def;
> >> -    size_t i, j;
> >> +    size_t i;
> >>      unsigned long long period = vm->def->cputune.period;
> >>      long long quota = vm->def->cputune.quota;
> >>      char *mem_mask = NULL;
> >> @@ -1214,18 +1214,11 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm)
> >>              /* default cpu masks */
> >>              if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO)
> >>                  cpumask = priv->autoCpuset;
> >> +            else if (def->iothreadids[i]->cpumask)
> >> +                cpumask = def->iothreadids[i]->cpumask;
> > 
> > This has to be the first case. Even if
> > VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO is specified, it cannot override what
> > the user configured pinning.
> > 
> 
> OK, but see qemuSetupCgroupForVcpu and qemuSetupCgroupForEmulator...
> Vcpu has one way of doing it and Emulator slightly different, but both
> check AUTO first.
> 
> VCPU checks AUTO first, then sets to default. Then it checks the vcpupin
> and resets to whatever is there.
> 
> Emulator checks AUTO first, then it checks if emulatorpin is set and
> uses that, and finally falls back to default if defined.

Um, I made a mistake when I refactored qemuSetupCgroupForEmulator.
qemuSetupCgroupForVcpu is the right approach.

> 
> I followed emulatorpin
> 
> If I'm wrong, then emulator and vcpu are wrong, but I think fixing that
> is a separate patch.

yes.

> 
> >>              else
> >>                  cpumask = def->cpumask;
> >>  
> >> -            /* specific cpu mask */
> >> -            for (j = 0; j < def->cputune.niothreadspin; j++) {
> >> -                if (def->cputune.iothreadspin[j]->id ==
> >> -                    def->iothreadids[i]->iothread_id) {
> >> -                    cpumask = def->cputune.iothreadspin[j]->cpumask;
> >> -                    break;
> >> -                }
> > 
> > Finally, this can be removed!
> > 
> >> -            }
> >> -
> >>              if (cpumask &&
> >>                  qemuSetupCgroupCpusetCpus(cgroup_iothread, cpumask) < 0)
> >>                  goto cleanup;
> >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> >> index 0f95cc7..ee13d08 100644
> >> --- a/src/qemu/qemu_driver.c
> >> +++ b/src/qemu/qemu_driver.c
> >> @@ -5894,19 +5894,14 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef,
> >>          goto cleanup;
> >>  
> >>      for (i = 0; i < targetDef->niothreadids; i++) {
> >> -        virDomainPinDefPtr pininfo;
> >> -
> >>          if (VIR_ALLOC(info_ret[i]) < 0)
> >>              goto cleanup;
> >>  
> >>          /* IOThread ID's are taken from the iothreadids list */
> >>          info_ret[i]->iothread_id = targetDef->iothreadids[i]->iothread_id;
> >>  
> >> -        /* Initialize the cpumap */
> >> -        pininfo = virDomainPinFind(targetDef->cputune.iothreadspin,
> >> -                                   targetDef->cputune.niothreadspin,
> >> -                                   targetDef->iothreadids[i]->iothread_id);
> >> -        if (!pininfo) {
> >> +        cpumask = targetDef->iothreadids[i]->cpumask;
> >> +        if (!cpumask) {
> >>              if (targetDef->cpumask) {
> >>                  cpumask = targetDef->cpumask;
> >>              } else {
> >> @@ -5915,8 +5910,6 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef,
> >>                  virBitmapSetAll(bitmap);
> >>                  cpumask = bitmap;
> >>              }
> >> -        } else {
> >> -            cpumask = pininfo->cpumask;
> >>          }
> >>          if (virBitmapToData(cpumask, &info_ret[i]->cpumap,
> >>                              &info_ret[i]->cpumaplen) < 0)
> >> @@ -5999,8 +5992,6 @@ qemuDomainPinIOThread(virDomainPtr dom,
> >>      virDomainDefPtr persistentDef = NULL;
> >>      virBitmapPtr pcpumap = NULL;
> >>      qemuDomainObjPrivatePtr priv;
> >> -    virDomainPinDefPtr *newIOThreadsPin = NULL;
> >> -    size_t newIOThreadsPinNum = 0;
> >>      virCgroupPtr cgroup_iothread = NULL;
> >>      virObjectEventPtr event = NULL;
> >>      char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = "";
> >> @@ -6050,33 +6041,21 @@ qemuDomainPinIOThread(virDomainPtr dom,
> >>      if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> >>  
> >>          virDomainIOThreadIDDefPtr iothrid;
> >> +        virBitmapPtr cpumask;
> >>  
> >>          if (!(iothrid = virDomainIOThreadIDFind(vm->def, iothread_id))) {
> >>              virReportError(VIR_ERR_INVALID_ARG,
> >> -                           _("iothread value %d not found"), iothread_id);
> >> +                           _("iothreadid %d not found"), iothread_id);
> > 
> > I'd compromise between those two.
> > 
> > "iothread %d not foud"
> > 
> 
> OK
> 
> >>              goto endjob;
> >>          }
> >>  
> >> -        if (vm->def->cputune.iothreadspin) {
> >> -            newIOThreadsPin =
> >> -                virDomainPinDefCopy(vm->def->cputune.iothreadspin,
> >> -                                    vm->def->cputune.niothreadspin);
> >> -            if (!newIOThreadsPin)
> >> -                goto endjob;
> >> -
> >> -            newIOThreadsPinNum = vm->def->cputune.niothreadspin;
> >> -        } else {
> >> -            if (VIR_ALLOC(newIOThreadsPin) < 0)
> >> -                goto endjob;
> >> -            newIOThreadsPinNum = 0;
> >> -        }
> >> -
> >> -        if (virDomainPinAdd(&newIOThreadsPin, &newIOThreadsPinNum,
> >> -                            cpumap, maplen, iothread_id) < 0) {
> >> -            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >> -                           _("failed to update iothreadspin"));
> >> +        if (!(cpumask = virBitmapNewData(cpumap, maplen)))
> >>              goto endjob;
> >> -        }
> >> +
> >> +        if (!iothrid->cpumask)
> >> +            vm->def->cputune.niothreadspin++;
> > 
> > This should go. If you add a function that checks whether pinning s
> > specified it will not be needed.
> > 
> 
> Yep
> 
> >> +        virBitmapFree(iothrid->cpumask);
> >> +        iothrid->cpumask = cpumask;
> >>  
> >>          /* Configure the corresponding cpuset cgroup before set affinity. */
> >>          if (virCgroupHasController(priv->cgroup,
> >> @@ -6099,14 +6078,6 @@ qemuDomainPinIOThread(virDomainPtr dom,
> >>              }
> >>          }
> >>  
> >> -        if (vm->def->cputune.iothreadspin)
> >> -            virDomainPinDefArrayFree(vm->def->cputune.iothreadspin,
> >> -                                     vm->def->cputune.niothreadspin);
> >> -
> >> -        vm->def->cputune.iothreadspin = newIOThreadsPin;
> >> -        vm->def->cputune.niothreadspin = newIOThreadsPinNum;
> >> -        newIOThreadsPin = NULL;
> >> -
> >>          if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
> >>              goto endjob;
> >>  
> >> @@ -6124,31 +6095,25 @@ qemuDomainPinIOThread(virDomainPtr dom,
> >>      }
> >>  
> >>      if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> >> +        virDomainIOThreadIDDefPtr iothrid;
> >> +        virBitmapPtr cpumask;
> >> +
> >>          /* Coverity didn't realize that targetDef must be set if we got here. */
> >>          sa_assert(persistentDef);
> >>  
> >> -        if (iothread_id > persistentDef->iothreads) {
> >> +        if (!(iothrid = virDomainIOThreadIDFind(persistentDef, iothread_id))) {
> >>              virReportError(VIR_ERR_INVALID_ARG,
> >> -                           _("iothread value out of range %d > %d"),
> >> -                           iothread_id, persistentDef->iothreads);
> >> +                           _("iothreadid %d not found"), iothread_id);
> >>              goto endjob;
> >>          }
> >>  
> >> -        if (!persistentDef->cputune.iothreadspin) {
> >> -            if (VIR_ALLOC(persistentDef->cputune.iothreadspin) < 0)
> >> -                goto endjob;
> >> -            persistentDef->cputune.niothreadspin = 0;
> >> -        }
> >> -        if (virDomainPinAdd(&persistentDef->cputune.iothreadspin,
> >> -                            &persistentDef->cputune.niothreadspin,
> >> -                            cpumap,
> >> -                            maplen,
> >> -                            iothread_id) < 0) {
> >> -            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >> -                           _("failed to update or add iothreadspin xml "
> >> -                             "of a persistent domain"));
> >> +        if (!(cpumask = virBitmapNewData(cpumap, maplen)))
> >>              goto endjob;
> >> -        }
> >> +
> >> +        if (!iothrid->cpumask)
> >> +            persistentDef->cputune.niothreadspin++;
> > 
> > Again, this should not be necessary.
> > 
> 
> yep
> 
> >> +        virBitmapFree(iothrid->cpumask);
> >> +        iothrid->cpumask = cpumask;
> >>  
> >>          ret = virDomainSaveConfig(cfg->configDir, persistentDef);
> >>          goto endjob;
> > 
> > The code looks much better now, but this patch has still some nits.
> > 
> > Peter
> > 
> 
> 
> All the following changes to be squashed in:
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index a191b02..9876557 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2115,6 +2115,19 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int
> npin)

Adding patches in plaintex in thunderbird or other client that breaks
lines makes them unusable so I don't usually bother checking it.

>  }

Peter

Attachment: signature.asc
Description: Digital signature


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