Re: [libvirt] [PATCH v2 5/5] qemu: Check for thread <=> memory alignment

On Thu, Jul 07, 2016 at 01:43:32PM +0100, Daniel P. Berrange wrote: > On Thu, Jul 07, 2016 at 01:42:33PM +0100, Daniel P. Berrange wrote: > > On Thu, Jul 07, 2016 at 02:12:32PM +0200, 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(a)redhat.com&gt; > > > --- > > > src/qemu/qemu_process.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 74 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > > > index 0aab01fd4d50..c012b6efcab6 100644 > > > --- a/src/qemu/qemu_process.c > > > +++ b/src/qemu/qemu_process.c > > > @@ -2304,6 +2304,76 @@ 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); > > > > We've had a general principle that we don't use VIR_WARN for this kind of > > thing, because libvirtd logs are genrally invisible to the person who is > > making the mistake. Meanwhile if this is intentional, we're spamming the > > logs for a situation the user explicitly chose. > > > > So NACK to the entire patch, as it doesn't do anything useful IMHO. > > BTW, I'd suggest NOTABUG on this > OK, I posted the first three patches as a clean-up separately so at least we shave off some duplicated code ;)