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

[Peter Krempa]

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 at 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
> +qemuDomainParseIOThreadAlias(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).

> +{
> +    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 (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.

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).

> +    }
>  
>      ret = 0;
>  

...

> @@ -5314,8 +5313,6 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>      virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason);
>      VIR_FREE(priv->vcpupids);
>      priv->nvcpupids = 0;
> -    VIR_FREE(priv->iothreadpids);

Shouldn't we clear the thread IDs once the VM is stopped? It shouldn't
be necessary to do so though.

> -    priv->niothreadpids = 0;
>      virObjectUnref(priv->qemuCaps);
>      priv->qemuCaps = NULL;
>      VIR_FREE(priv->pidfile);

The rest looks good. I specially like killing the status
formatter/parser code.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150427/01890da2/attachment-0001.sig>