There's no point in checking if numa->mem_nodes[node].ndistances is set if we check for numa->mem_nodes[node].distances. However, it makes sense to check if the sibling node caller passed falls within boundaries. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/conf/numa_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 7bba4120b..5f0b3f9ed 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -1154,7 +1154,7 @@ virDomainNumaGetNodeDistance(virDomainNumaPtr numa, */ if (!distances || !distances[cellid].value || - !numa->mem_nodes[node].ndistances) + node >= numa->nmem_nodes) return (node == cellid) ? LOCAL_DISTANCE : REMOTE_DISTANCE; return distances[cellid].value; -- 2.13.6

On 11/22/2017 12:22 AM, John Ferlan wrote: Ah right. This patch makes no sense. I don't even know what was I thinking :-) But now as I'm looking at the code, it might be worth to check if @cellid < numa->nmem_nodes; We check @node but not @cellid. Michal

The function returns true/false depending on distance configuration being present in the domain XML. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/conf/numa_conf.c | 13 +++++++++++++ src/conf/numa_conf.h | 4 ++++ src/libvirt_private.syms | 1 + 3 files changed, 18 insertions(+) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 5f0b3f9ed..6a42777e2 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -1137,6 +1137,19 @@ virDomainNumaSetNodeCount(virDomainNumaPtr numa, size_t nmem_nodes) return numa->nmem_nodes; } +bool +virDomainNumaNodeDistanceSpecified(virDomainNumaPtr numa, + size_t node, + size_t sibling) +{ + return node < numa->nmem_nodes && + sibling < numa->nmem_nodes && + numa->mem_nodes[node].distances && + numa->mem_nodes[node].distances[sibling].value != LOCAL_DISTANCE && + numa->mem_nodes[node].distances[sibling].value != REMOTE_DISTANCE; +} + + size_t virDomainNumaGetNodeDistance(virDomainNumaPtr numa, size_t node, diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index 4655de3aa..1d2e605b6 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -87,6 +87,10 @@ int virDomainNumatuneMaybeGetNodeset(virDomainNumaPtr numatune, size_t virDomainNumaGetNodeCount(virDomainNumaPtr numa); +bool virDomainNumaNodeDistanceSpecified(virDomainNumaPtr numa, + size_t node, + size_t sibling) + ATTRIBUTE_NONNULL(1); size_t virDomainNumaGetNodeDistance(virDomainNumaPtr numa, size_t node, size_t sibling) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5a4d50471..779bab7a3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -725,6 +725,7 @@ virDomainNumaGetNodeDistance; virDomainNumaGetNodeMemoryAccessMode; virDomainNumaGetNodeMemorySize; virDomainNumaNew; +virDomainNumaNodeDistanceSpecified; virDomainNumaSetNodeCount; virDomainNumaSetNodeCpumask; virDomainNumaSetNodeDistance; -- 2.13.6

On 11/22/2017 12:38 AM, John Ferlan wrote: Sure it is possible. But see my reply to 4/5 why I need this function. Okay, so the name is confusing. Would you be okay if I rename this to virDomainNumaNodeDistanceSpecifiedAndNotDefault()? Ugrh, long name. Any suggestion is welcome. Alternatively, I can move LOCAL/REMOTE_DISTANCE macros to numa_conf.h, give them proper prefix and call virDomainNumaGetNodeDistance() from qemu and check if returned value is LOCAL/REMOTE. Michal

This capability says if qemu is capable of specifying distances between NUMA nodes on the command line. Unfortunately, there's no real way to check this and thus we have to go with version check. QEMU introduced this in 0f203430dd8 (and friend) which was released in 2.10.0. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_capabilities.c | 5 +++++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.10.0-gicv2.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.10.0-gicv3.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml | 1 + 7 files changed, 11 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 7cb091056..fea526432 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -443,6 +443,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, /* 270 */ "vxhs", "virtio-blk.num-queues", + "numa.dist", ); @@ -4776,6 +4777,10 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, if (qemuCaps->version >= 2006000) virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACHINE_KERNEL_IRQCHIP_SPLIT); + /* no way to query for -numa dist */ + if (qemuCaps->version >= 2010000) + virQEMUCapsSet(qemuCaps, QEMU_CAPS_NUMA_DIST); + if (virQEMUCapsProbeQMPCommands(qemuCaps, mon) < 0) goto cleanup; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index cacc2b77e..e3aaf0fcc 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -429,6 +429,7 @@ typedef enum { /* 270 */ QEMU_CAPS_VXHS, /* -drive file.driver=vxhs via query-qmp-schema */ QEMU_CAPS_VIRTIO_BLK_NUM_QUEUES, /* virtio-blk-*.num-queues */ + QEMU_CAPS_NUMA_DIST, /* -numa dist */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.10.0-gicv2.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.10.0-gicv2.aarch64.xml index 9f9dceb68..06b90875d 100644 --- a/tests/qemucapabilitiesdata/caps_2.10.0-gicv2.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.10.0-gicv2.aarch64.xml @@ -180,6 +180,7 @@ <flag name='virtio-gpu.max_outputs'/> <flag name='vxhs'/> <flag name='virtio-blk.num-queues'/> + <flag name='numa.dist'/> <version>2010000</version> <kvmVersion>0</kvmVersion> <package> (v2.10.0)</package> diff --git a/tests/qemucapabilitiesdata/caps_2.10.0-gicv3.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.10.0-gicv3.aarch64.xml index 3c2d2eed6..389390fe4 100644 --- a/tests/qemucapabilitiesdata/caps_2.10.0-gicv3.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.10.0-gicv3.aarch64.xml @@ -180,6 +180,7 @@ <flag name='virtio-gpu.max_outputs'/> <flag name='vxhs'/> <flag name='virtio-blk.num-queues'/> + <flag name='numa.dist'/> <version>2010000</version> <kvmVersion>0</kvmVersion> <package> (v2.10.0)</package> diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml index 0dfa20726..d40767801 100644 --- a/tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml @@ -177,6 +177,7 @@ <flag name='virtio-gpu.max_outputs'/> <flag name='vxhs'/> <flag name='virtio-blk.num-queues'/> + <flag name='numa.dist'/> <version>2010000</version> <kvmVersion>0</kvmVersion> <package> (v2.10.0)</package> diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml index 7e44652fe..ed59925d8 100644 --- a/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml @@ -141,6 +141,7 @@ <flag name='virtio-gpu.max_outputs'/> <flag name='vxhs'/> <flag name='virtio-blk.num-queues'/> + <flag name='numa.dist'/> <version>2010000</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml index ddbd8c32f..50251edc0 100644 --- a/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml @@ -224,6 +224,7 @@ <flag name='virtio-gpu.max_outputs'/> <flag name='vxhs'/> <flag name='virtio-blk.num-queues'/> + <flag name='numa.dist'/> <version>2010000</version> <kvmVersion>0</kvmVersion> <package> (v2.10.0)</package> -- 2.13.6

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; + } + } + } + + if (numa_distances) { + for (i = 0; i < ncells; i++) { + for (j = 0; j < ncells; j++) { + size_t distance = virDomainNumaGetNodeDistance(def->numa, i, j); + + virCommandAddArg(cmd, "-numa"); + virBufferAsprintf(&buf, "dist,src=%zu,dst=%zu,val=%zu", i, j, distance); + + virCommandAddArgBuffer(cmd, &buf); + } + } + } + ret = 0; cleanup: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-numatune-distances.args b/tests/qemuxml2argvdata/qemuxml2argv-numatune-distances.args new file mode 100644 index 000000000..23b66246c --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-numatune-distances.args @@ -0,0 +1,63 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name QEMUGuest \ +-S \ +-M xenfv \ +-m 12288 \ +-smp 12,sockets=12,cores=1,threads=1 \ +-numa node,nodeid=0,cpus=0,cpus=11,mem=2048 \ +-numa node,nodeid=1,cpus=1,cpus=10,mem=2048 \ +-numa node,nodeid=2,cpus=2,cpus=9,mem=2048 \ +-numa node,nodeid=3,cpus=3,cpus=8,mem=2048 \ +-numa node,nodeid=4,cpus=4,cpus=7,mem=2048 \ +-numa node,nodeid=5,cpus=5-6,mem=2048 \ +-numa dist,src=0,dst=0,val=10 \ +-numa dist,src=0,dst=1,val=21 \ +-numa dist,src=0,dst=2,val=31 \ +-numa dist,src=0,dst=3,val=41 \ +-numa dist,src=0,dst=4,val=51 \ +-numa dist,src=0,dst=5,val=61 \ +-numa dist,src=1,dst=0,val=21 \ +-numa dist,src=1,dst=1,val=10 \ +-numa dist,src=1,dst=2,val=21 \ +-numa dist,src=1,dst=3,val=31 \ +-numa dist,src=1,dst=4,val=41 \ +-numa dist,src=1,dst=5,val=51 \ +-numa dist,src=2,dst=0,val=31 \ +-numa dist,src=2,dst=1,val=21 \ +-numa dist,src=2,dst=2,val=10 \ +-numa dist,src=2,dst=3,val=21 \ +-numa dist,src=2,dst=4,val=31 \ +-numa dist,src=2,dst=5,val=41 \ +-numa dist,src=3,dst=0,val=41 \ +-numa dist,src=3,dst=1,val=31 \ +-numa dist,src=3,dst=2,val=21 \ +-numa dist,src=3,dst=3,val=10 \ +-numa dist,src=3,dst=4,val=21 \ +-numa dist,src=3,dst=5,val=31 \ +-numa dist,src=4,dst=0,val=51 \ +-numa dist,src=4,dst=1,val=41 \ +-numa dist,src=4,dst=2,val=31 \ +-numa dist,src=4,dst=3,val=21 \ +-numa dist,src=4,dst=4,val=10 \ +-numa dist,src=4,dst=5,val=21 \ +-numa dist,src=5,dst=0,val=61 \ +-numa dist,src=5,dst=1,val=51 \ +-numa dist,src=5,dst=2,val=41 \ +-numa dist,src=5,dst=3,val=31 \ +-numa dist,src=5,dst=4,val=21 \ +-numa dist,src=5,dst=5,val=10 \ +-uuid c7a5fdb2-cdaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-boot c \ +-usb \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-numatune-distances.xml b/tests/qemuxml2argvdata/qemuxml2argv-numatune-distances.xml new file mode 100644 index 000000000..0f33526b4 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-numatune-distances.xml @@ -0,0 +1,65 @@ +<domain type='qemu'> + <name>QEMUGuest</name> + <uuid>c7a5fdb2-cdaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>8388608</memory> + <currentMemory unit='KiB'>8388608</currentMemory> + <vcpu placement='static'>12</vcpu> + <os> + <type arch='x86_64' machine='xenfv'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <cpu> + <numa> + <cell id='0' cpus='0,11' memory='2097152' unit='KiB'> + <distances> + <sibling id='1' value='21'/> + <sibling id='2' value='31'/> + <sibling id='3' value='41'/> + <sibling id='4' value='51'/> + <sibling id='5' value='61'/> + </distances> + </cell> + <cell id='1' cpus='1,10' memory='2097152' unit='KiB'> + <distances> + <sibling id='2' value='21'/> + <sibling id='3' value='31'/> + <sibling id='4' value='41'/> + <sibling id='5' value='51'/> + </distances> + </cell> + <cell id='2' cpus='2,9' memory='2097152' unit='KiB'> + <distances> + <sibling id='3' value='21'/> + <sibling id='4' value='31'/> + <sibling id='5' value='41'/> + </distances> + </cell> + <cell id='3' cpus='3,8' memory='2097152' unit='KiB'> + <distances> + <sibling id='4' value='21'/> + <sibling id='5' value='31'/> + </distances> + </cell> + <cell id='4' cpus='4,7' memory='2097152' unit='KiB'> + <distances> + <sibling id='5' value='21'/> + </distances> + </cell> + <cell id='5' cpus='5-6' memory='2097152' unit='KiB'/> + </numa> + </cpu> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 2e07b85aa..160374793 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1700,6 +1700,8 @@ mymain(void) QEMU_CAPS_OBJECT_MEMORY_RAM); DO_TEST_FAILURE("numatune-memnode-no-memory", NONE); + DO_TEST("numatune-distances", QEMU_CAPS_NUMA, QEMU_CAPS_NUMA_DIST); + DO_TEST("numatune-auto-nodeset-invalid", NONE); DO_TEST("numatune-auto-prefer", QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE); -- 2.13.6

On 11/14/2017 04:13 PM, Peter Krempa wrote: Ah. Correct. Consider this squashed in then: diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c index 8b9daaea3..bceafb084 100644 --- i/src/qemu/qemu_command.c +++ w/src/qemu/qemu_command.c @@ -7804,17 +7804,18 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, 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; - } + break; } } if (numa_distances) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NUMA_DIST)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("setting NUMA distances is not " + "supported with this qemu")); + goto cleanup; + } + for (i = 0; i < ncells; i++) { for (j = 0; j < ncells; j++) { size_t distance = virDomainNumaGetNodeDistance(def->numa, i, j); Michal

On 11/22/2017 01:04 AM, John Ferlan wrote: 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). Michal

Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- docs/news.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/news.xml b/docs/news.xml index 3966710ee..502679917 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -43,7 +43,7 @@ A NUMA hardware architecture supports the notion of distances between NUMA cells. This can now be specified using the <code>&lt;distances&gt;</code> element within the NUMA cell - configuration. + configuration. Drivers which support this include Xen and QEMU. </description> </change> <change> -- 2.13.6