9:49 a.m.
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(a)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.
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;
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?
> + }
>
> 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.
I suppose we could...
for (i = 0; i < vm->def->niothreadids; i++)
vm->def->iothreadids[i]->thread_id = 0;
John
> - 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