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

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > ---
> >  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

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list