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

Re: [libvirt] [PATCH v5 02/10] qemu: Use domain iothreadids to IOThread's 'thread_id'



On Mon, Apr 27, 2015 at 10:49:32 -0400, John Ferlan wrote:
> 
> 
> On 04/27/2015 10:08 AM, Peter Krempa wrote:
> > On Fri, Apr 24, 2015 at 12:05:54 -0400, John Ferlan wrote:
> >> 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/qemu/qemu_command.c b/src/qemu/qemu_command.c
> >> index 29b876e..cc96d5b 100644
> >> --- a/src/qemu/qemu_command.c
> >> +++ b/src/qemu/qemu_command.c
> >> @@ -678,6 +678,24 @@ qemuOpenVhostNet(virDomainDefPtr def,
> >>  }
> >>  
> >>  int
> >> + 	(char *alias,
> >> +                             unsigned int *iothread_id)
> > 
> > I still think that the monitor should parse the aliases rather than
> > having to use the code in two places .. (see below).
> > 
> 
> Fair enough, but that's yet another design change being requested upon
> this set of changes. Changing the qemuMonitorIOThreadInfoPtr to return
> an iothread_id rather than the alias character string.  Regardless of
> where it's transformed, I would think/believe a single function rather
> than cut-n-paste in two places is "preferable".
> 
> I understand your point, but I think for the purposes of "this" set of
> changes - I'd ask that this be something for a followup patch.
> 
> >> +{
> >> +    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;
> > 
> > ...
> > 
> >> @@ -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)) {
> > 
> > Spurious newline addition.
> > 
> > ...
> > 
> >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> >> index 989c20c..330ffdf 100644
> >> --- a/src/qemu/qemu_driver.c
> >> +++ b/src/qemu/qemu_driver.c
> >> @@ -5850,14 +5850,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;
> > 
> > ... one place that parses the alias ...
> > 
> >>  
> >> -        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;
> > 
> > ...
> > 
> >> @@ -10087,8 +10083,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 6707170..0d15432 100644
> >> --- a/src/qemu/qemu_process.c
> >> +++ b/src/qemu/qemu_process.c
> >> @@ -2267,12 +2267,16 @@ qemuProcessDetectIOThreadPIDs(virQEMUDriverPtr driver,
> >>          goto cleanup;
> >>      }
> > 
> > A few lines prior here is the check that the expected thread count
> > equals to the actual thread count. For some reason a few lines before
> > returns success if 0 threads are returned by the monitor. The two checks
> > should be inverted so that it makes sense.
> > 
> 
> If there are no threads, then it's not a failure, thus change ret to be
> 0. Again, this is something that's not within the scope of this set of
> changes and I believe if necessary could be a followup patch.
> 
> I'm not clear on the value of changing the order of the checks.

The problem is that if there are no iothreads reported by qemu, but we
did request some then it IS failure.

> 
> Check failure first - return failure
> 
> Check possible successes next.
> 
> >>  
> >> -    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;
> > 
> > This construct is wrong since it expects that the order the devices are
> > stored in libvirt is the same as in the qemu monitor reply. You need to
> > iterate the list of threads from the monitor and lookup the
> > corresponding info for it.
> 
> Ahh... Right, but we are getting the iothread_id's here from the monitor
> and filling in an array - the call to virDomainIOThreadIDFind had better
> not fail ;-) - if does though the domain disappears, so which is worse?
> 
> So ...
> 
>     virDomainIOThreadIDDefPtr iothrid;
> ...
> 
>     if (!(iothrid = virDomainIOThreadIDFind(vm->def, iothread_id))) {
>         virReportError(VIR_ERR_INVALID_ARG,
>                        _("iothread %d not found"), iothread_id);
>     }
>     iothrid->thread_id = iothreads[i]->thread_id;

Yep.

> 
> > 
> > Btw, it would be much easier if the monitor code would parse the IDs
> > since you wouldn't need to parse them here (I've already suggested this
> > once).
> > 
> 
> Again, design/structure change - can we let this be a followup patch?

Yes this can be a separate change. I only find it less wasteful since
every single caller needs to parse the IDs anyways.

> 
> >> +    }
> >>  
> >>      ret = 0;
> >>  
> > 
> > ...
> > 

Peter

Attachment: signature.asc
Description: Digital signature


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