[libvirt] [PATCH 1/4] qemu: During startup need more QEMU_CAPS_OBJECT_IOTHREAD checks

[John Ferlan]

On 10/13/2015 12:08 PM, Peter Krempa wrote:
> On Tue, Oct 13, 2015 at 11:47:07 -0400, John Ferlan wrote:
>> During qemu process startup (qemuProcessStart), the call to
>> qemuProcessDetectIOThreadPIDs will not attempt to fill in the
>> ->thread_id values if the binary doesn't support IOThreads.
>> However, subsequent calls to setup the IOThread cgroups, affinity,
>> and scheduler parameters had no such check, thus could attempt
>> to set thread_id = 0, which would not be a good idea.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/qemu/qemu_cgroup.c  |  3 +++
>>  src/qemu/qemu_process.c | 18 ++++++++++++------
>>  2 files changed, 15 insertions(+), 6 deletions(-)
> 
> Do we even allow to start a VM that has IOthreads enabled but qemu does
> not support them? In that case that's the point to fix so that we have a
> central point where it's checked.
> 

Upon further reflection...

During qemuBuildCommandLine there is a check for 'iothreads > 0 and have
the capability', then create 'niothreadids' objects.

Then during qemuCheckIOThreads called from qemuBuildDriveDevStr, we will
fail if there's a disk with an "iothread='#'" property.

Thus, if just defined (eg, <iothreads> and <iothreadids>), we ignore
them if the emulator doesn't support it. However, if used, then yes we
fail. Naturally this is one of those only one downstream platform really
cares - just happens to be one that does a lot of QA for Red Hat.

Although a fairly useless configuration, I think because it seems it's
possible to have 'niothreadids' with empty fields other than
'iothread_id' that checking for the capability is more correct in these
instances than checking for niothreadids.

Unless of course you feel we should fail in qemuBuildCommandLine if
iothreads are defined.

John
>>
>> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
>> index 570dab5..66318ea 100644
>> --- a/src/qemu/qemu_cgroup.c
>> +++ b/src/qemu/qemu_cgroup.c
>> @@ -1163,6 +1163,9 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm)
>>      char *mem_mask = NULL;
>>      virDomainNumatuneMemMode mem_mode;
>>  
>> +    if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD))
>> +        return 0;
> 
> Spreading checks like this through the code usually ends up to be
> fragile.
> 
> I think this should be if (def->niothreads == 0) return 0;
> 
>> +
>>      if ((period || quota) &&
>>          !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
>>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 8aa9efc..67e7cbc 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -2514,10 +2514,14 @@ qemuProcessSetEmulatorAffinity(virDomainObjPtr vm)
>>  static int
>>  qemuProcessSetIOThreadsAffinity(virDomainObjPtr vm)
>>  {
>> +    qemuDomainObjPrivatePtr priv = vm->privateData;
>>      virDomainDefPtr def = vm->def;
>>      size_t i;
>>      int ret = -1;
>>  
>> +    if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD))
>> +        return 0;
>> +
>>      for (i = 0; i < def->niothreadids; i++) {
> 
> Here this function is doing the right thing.
> 
>>          /* set affinity only for existing iothreads */
>>          if (!def->iothreadids[i]->cpumask)
>> @@ -2574,12 +2578,14 @@ qemuProcessSetSchedulers(virDomainObjPtr vm)
>>              return -1;
>>      }
>>  
>> -    for (i = 0; i < vm->def->niothreadids; i++) {
>> -        if (qemuProcessSetSchedParams(vm->def->iothreadids[i]->iothread_id,
>> -                                      vm->def->iothreadids[i]->thread_id,
>> -                                      vm->def->cputune.niothreadsched,
>> -                                      vm->def->cputune.iothreadsched) < 0)
>> -            return -1;
>> +    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) {
>> +        for (i = 0; i < vm->def->niothreadids; i++) {
> 
> As well as here.
> 
>> +            if (qemuProcessSetSchedParams(vm->def->iothreadids[i]->iothread_id,
>> +                                          vm->def->iothreadids[i]->thread_id,
>> +                                          vm->def->cputune.niothreadsched,
>> +                                          vm->def->cputune.iothreadsched) < 0)
>> +                return -1;
>> +        }
> 
> Peter
>