Re: [libvirt] [PATCH 4/5] qemu: Support setting NUMA distances

On 11/22/2017 01:04 AM, John Ferlan wrote: > > > On 11/14/2017 09:47 AM, Michal Privoznik wrote: >> Since we already have such support for libxl all we need is qemu >> driver adjustment. And a test case. >> >> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >> --- >> src/qemu/qemu_command.c | 36 +++++++++++- >> .../qemuxml2argv-numatune-distances.args | 63 +++++++++++++++++++++ >> .../qemuxml2argv-numatune-distances.xml | 65 ++++++++++++++++++++++ >> tests/qemuxml2argvtest.c | 2 + >> 4 files changed, 165 insertions(+), 1 deletion(-) >> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numatune-distances.args >> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numatune-distances.xml >> >> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c >> index eb72db33b..8b9daaea3 100644 >> --- a/src/qemu/qemu_command.c >> +++ b/src/qemu/qemu_command.c >> @@ -7675,7 +7675,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, >> virCommandPtr cmd, >> qemuDomainObjPrivatePtr priv) >> { >> - size_t i; >> + size_t i, j; >> virQEMUCapsPtr qemuCaps = priv->qemuCaps; >> virBuffer buf = VIR_BUFFER_INITIALIZER; >> char *cpumask = NULL, *tmpmask = NULL, *next = NULL; >> @@ -7685,6 +7685,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, >> int ret = -1; >> size_t ncells = virDomainNumaGetNodeCount(def->numa); >> const long system_page_size = virGetSystemPageSizeKB(); >> + bool numa_distances = false; >> >> if (virDomainNumatuneHasPerNodeBinding(def->numa) && >> !(virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM) || >> @@ -7793,6 +7794,39 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, >> >> virCommandAddArgBuffer(cmd, &buf); >> } >> + >> + /* If NUMA node distance is specified for at least one pair >> + * of nodes, we have to specify all the distances. Even >> + * though they might be the default ones. */ >> + for (i = 0; i < ncells; i++) { >> + for (j = 0; j < ncells; j++) { >> + if (!virDomainNumaNodeDistanceSpecified(def->numa, i, j)) >> + continue; >> + >> + numa_distances = true; >> + >> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NUMA_DIST)) { >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> + _("setting NUMA distances is not " >> + "supported with this qemu")); >> + goto cleanup; >> + } >> + } >> + } > > Not sure I understand the need for the above double loop.... Even with > your adjustment... > > It would seem that all that's necessary is > > for (i < 0; i < ncells; i++) { > if (numa->mem_nodes[i].ndistances > 0) It things only were so simple. Thing is, numa is a private struct. We need accessors for getting any value. I can't just dereference numa struct from qemu code.

That's why I'm introducing an accessor in 2/5. Moreover, in case all values are defaults I wanted to not put them onto the cmd line. The advantage would be that it would work even with older qemu which doesn't have the capability. I mean, qemu does follow the spec and if no distances are provided it uses the ones from the spec (10 and 20).