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

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
> 


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