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@xxxxxxxxxx>
>> ---
>> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list