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