[libvirt] [PATCH 2/2] qemu: Check for thread <=> memory alignment

[Martin Kletzander]

On Thu, Jun 23, 2016 at 03:21:04PM -0400, John Ferlan wrote:
>
>
>On 06/22/2016 12:37 PM, Martin Kletzander wrote:
>> Some settings may be confusing and in case users use numad placement in
>> combination with static placement we could warn them as it might not be
>> wanted (but it's not forbidden).
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1254402
>>
>> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
>> ---
>>  src/qemu/qemu_driver.c  |   4 +-
>>  src/qemu/qemu_process.c | 107 +++++++++++++++++++++++++++++++++++++++++-------
>>  src/qemu/qemu_process.h |   6 ++-
>>  3 files changed, 99 insertions(+), 18 deletions(-)
>>
>
>Perhaps could have been two patches ... one to add the driver argument
>and the second to use it...
>

I wanted to do that actually, but I had to add ATTRIBUTE_UNUSED
everywhere and then remove it in the next patch which would not shorten
the patch at all, and in my opinion not even made it more readable.

>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 931efae27dee..4cf9f0560092 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -4702,7 +4702,7 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
>>          goto cleanup;
>>      }
>>
>> -    if (qemuProcessSetupVcpu(vm, vcpu) < 0)
>> +    if (qemuProcessSetupVcpu(driver, vm, vcpu) < 0)
>>          goto cleanup;
>>
>>      ret = 0;
>> @@ -5828,7 +5828,7 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver,
>>
>>      iothrid->thread_id = new_iothreads[idx]->thread_id;
>>
>> -    if (qemuProcessSetupIOThread(vm, iothrid) < 0)
>> +    if (qemuProcessSetupIOThread(driver, vm, iothrid) < 0)
>>          goto cleanup;
>>
>>      ret = 0;
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index d1247d2fd0f9..51709f8c9d58 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -2306,6 +2306,75 @@ qemuProcessSetLinkStates(virQEMUDriverPtr driver,
>>  }
>>
>>
>> +static int
>> +qemuProcessCheckCpusMemsAlignment(virQEMUDriverPtr driver,
>> +                                  virDomainObjPtr vm,
>> +                                  virBitmapPtr cpumask,
>> +                                  const char *mem_mask)
>> +{
>> +    int ret = -1;
>> +    int hostnodes = 0;
>> +    char *cpumask_str = NULL;
>> +    char *tmpmask_str = NULL;
>> +    char *mem_cpus_str = NULL;
>> +    virCapsPtr caps = NULL;
>> +    virBitmapPtr tmpmask = NULL;
>> +    virBitmapPtr mem_cpus = NULL;
>> +    virBitmapPtr mem_nodes = NULL;
>> +    virDomainNumatuneMemMode mem_mode;
>> +
>> +    if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) != 0)
>> +        return 0;
>> +
>> +    if (mem_mode != VIR_DOMAIN_NUMATUNE_MEM_STRICT)
>> +        return 0;
>> +
>> +    if (!mem_mask || !cpumask)
>> +        return 0;
>> +
>> +    if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
>> +        goto cleanup;
>> +
>> +    if (!(tmpmask = virBitmapNewCopy(cpumask)))
>> +        goto cleanup;
>> +
>> +    if ((hostnodes = virNumaGetMaxNode()) < 0)
>> +        goto cleanup;
>> +
>> +    if (virBitmapParse(mem_mask, &mem_nodes, hostnodes) < 0)
>> +        goto cleanup;
>> +
>> +    if (!(mem_cpus = virCapabilitiesGetCpusForNodemask(caps, mem_nodes)))
>> +        goto cleanup;
>> +
>> +    virBitmapSubtract(tmpmask, mem_cpus);
>> +    if (!virBitmapIsAllClear(tmpmask)) {
>> +        if (!(cpumask_str = virBitmapFormat(cpumask)))
>> +            goto cleanup;
>> +
>> +        if (!(tmpmask_str = virBitmapFormat(tmpmask)))
>> +            goto cleanup;
>> +
>> +        if (!(mem_cpus_str = virBitmapFormat(mem_cpus)))
>> +            goto cleanup;
>> +
>> +        VIR_WARN("CPUs '%s' in cpumask '%s' might not have access to any NUMA "
>> +                 "node in memory's nodeset '%s' which consists of CPUs: '%s'.",
>> +                 tmpmask_str, cpumask_str, mem_mask, mem_cpus_str);
>
>Hopefully enough details ;-)
>
>> +    }
>> +
>> +    ret = 0;
>> + cleanup:
>> +    VIR_FREE(mem_cpus_str);
>> +    VIR_FREE(tmpmask_str);
>> +    VIR_FREE(cpumask_str);
>> +    virBitmapFree(mem_cpus);
>
>Coverity complains that mem_nodes is leaked.
>

Oh, good catch, I renamed them so many times (due to keeping part of the
bad naming and trying to be descriptive at the same time).

[...]

>> @@ -2390,6 +2460,10 @@ qemuProcessSetupPid(virDomainObjPtr vm,
>>          if ((period || quota) &&
>>              qemuSetupCgroupVcpuBW(cgroup, period, quota) < 0)
>>              goto cleanup;
>> +
>> +        if (qemuProcessCheckCpusMemsAlignment(driver, vm,
>> +                                              use_cpumask, mem_mask) < 0)
>> +            goto cleanup;
>
>It doesn't seem to matter that for an emulator virCgroupSetCpusetMems is
>not called yet... But I figured I'd ask to double check!
>

Yes, it will be set a bit later on, so the check still makes sense.

>ACK with the coverity error fixed as this seems reasonable to me.
>

Thanks, I'll wait for the consensus on the first one.

Martin

>John
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160624/13ab4e0d/attachment-0001.sig>