[libvirt] [PATCH 0/5] qemu: Support setting NUMA distances
Now that XML parsing & formatting is merged this is fairly trivial. Michal Privoznik (5): virDomainNumaGetNodeDistance: Fix input arguments validation numa: Introduce virDomainNumaNodeDistanceSpecified qemu_capabilities: Introcude QEMU_CAPS_NUMA_DIST qemu: Support setting NUMA distances news: Document which drivers support NUMA distances docs/news.xml | 2 +- src/conf/numa_conf.c | 15 ++++- src/conf/numa_conf.h | 4 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_capabilities.c | 5 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 36 +++++++++++- .../caps_2.10.0-gicv2.aarch64.xml | 1 + .../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 + .../qemuxml2argv-numatune-distances.args | 63 +++++++++++++++++++++ .../qemuxml2argv-numatune-distances.xml | 65 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 + 15 files changed, 196 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numatune-distances.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numatune-distances.xml -- 2.13.6
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@redhat.com> --- 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/14/2017 09:47 AM, Michal Privoznik wrote:
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@redhat.com> --- 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)
If @distances can only be set if "node < numa->nmem_nodes", then how could "node >= numa->nmem_nodes" ever be true and @distances be non NULL? IOW: I see no need for the check... This former condition also trips across my "favorite" condition check of "if !intValue" substituting for "if intValue == 0" <sigh>. BTW: I do think there is a memory leak @distances entries are not VIR_FREE'd in virDomainNumaFree. I was looking for instances where ndistances maybe have been forgotten to be set to 0 even though distances was cleared. I can send a patch or you can for that if you want - IDC... There's a couple of other cleanups I'd like to see w/r/t using (!*ndistances) and how the @*distance are set to ldist/rdist outside of the if condition that allocated, but those are type A type things ;-) John
return (node == cellid) ? LOCAL_DISTANCE : REMOTE_DISTANCE;
return distances[cellid].value;
On 11/22/2017 12:22 AM, John Ferlan wrote:
On 11/14/2017 09:47 AM, Michal Privoznik wrote:
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@redhat.com> --- 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)
If @distances can only be set if "node < numa->nmem_nodes", then how could "node >= numa->nmem_nodes" ever be true and @distances be non NULL? IOW: I see no need for the check... This former condition also trips across my "favorite" condition check of "if !intValue" substituting for "if intValue == 0" <sigh>.
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
On 11/22/2017 04:45 AM, Michal Privoznik wrote:
On 11/22/2017 12:22 AM, John Ferlan wrote:
On 11/14/2017 09:47 AM, Michal Privoznik wrote:
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@redhat.com> --- 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)
If @distances can only be set if "node < numa->nmem_nodes", then how could "node >= numa->nmem_nodes" ever be true and @distances be non NULL? IOW: I see no need for the check... This former condition also trips across my "favorite" condition check of "if !intValue" substituting for "if intValue == 0" <sigh>.
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.
True, probably before we use it in distances too! So flip/flop the check and s/node/cellid/ John
The function returns true/false depending on distance configuration being present in the domain XML. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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/14/2017 09:47 AM, Michal Privoznik wrote:
The function returns true/false depending on distance configuration being present in the domain XML.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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; }
Two blank lines here too.
+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; +} + +
According to how I read commit message '74119a03' it *is* possible to set the @value to the same value as LOCAL_DISTANCE(10) or REMOTE_DISTANCE(20) - so using that as a comparison for whether it was specified would seem to be wrong. Still if a distance *is* provided, then it seems that 'id' and 'value' are also required to be provided or defaulted. That means what seems to matter regarding whether something was provided is if the *.value and/or "*.cellid are zero At least that's how I'm reading virDomainNumaDefNodeDistanceParseXML John
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;
On 11/22/2017 12:38 AM, John Ferlan wrote:
On 11/14/2017 09:47 AM, Michal Privoznik wrote:
The function returns true/false depending on distance configuration being present in the domain XML.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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; }
Two blank lines here too.
+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; +} + +
According to how I read commit message '74119a03' it *is* possible to set the @value to the same value as LOCAL_DISTANCE(10) or REMOTE_DISTANCE(20) - so using that as a comparison for whether it was specified would seem to be wrong.
Sure it is possible. But see my reply to 4/5 why I need this function.
Still if a distance *is* provided, then it seems that 'id' and 'value' are also required to be provided or defaulted. That means what seems to matter regarding whether something was provided is if the *.value and/or "*.cellid are zero
At least that's how I'm reading virDomainNumaDefNodeDistanceParseXML
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
On 11/22/2017 04:45 AM, Michal Privoznik wrote:
On 11/22/2017 12:38 AM, John Ferlan wrote:
On 11/14/2017 09:47 AM, Michal Privoznik wrote:
The function returns true/false depending on distance configuration being present in the domain XML.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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; }
Two blank lines here too.
+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; +} + +
According to how I read commit message '74119a03' it *is* possible to set the @value to the same value as LOCAL_DISTANCE(10) or REMOTE_DISTANCE(20) - so using that as a comparison for whether it was specified would seem to be wrong.
Sure it is possible. But see my reply to 4/5 why I need this function.
Still if a distance *is* provided, then it seems that 'id' and 'value' are also required to be provided or defaulted. That means what seems to matter regarding whether something was provided is if the *.value and/or "*.cellid are zero
At least that's how I'm reading virDomainNumaDefNodeDistanceParseXML
Okay, so the name is confusing. Would you be okay if I rename this to virDomainNumaNodeDistanceSpecifiedAndNotDefault()? Ugrh, long name. Any suggestion is welcome.
Sometimes comments to the function that state what it's returning help - especially the long single boolean condition. Just remember, naming is hard! How about virDomainNumaNodeDistanceIsUsingDefaults? The prefix virDomainNumaNodeDistance makes names longer too... Not sure I was as hung up over the name as I was the functionality that at this point in time didn't make as much sense. Although paired with your comments in patch 4, perhaps make more sense. I'm also not a fan of the large single condition for the bool function - especially where some conditions are positive and some are negative, but that probably just me... If I try to decompose it... /* Out of range, not created, or not supplied */ if (node >= numa->nmem_nodes || sibling >= numa->nmem_nodes || !numa->mem_nodes[node].distances || numa->mem_nodes[node].distances[sibling].value == 0) return false; /* Supplied using default distances */ if (numa->mem_nodes[node].distances[sibling].value == LOCAL_DISTANCE || numa->mem_nodes[node].distances[sibling].value == REMOTE_DISTANCE) return true; return false;
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.
Hiding comparisons behind some other function that requires me to jump elsewhere which can be painful too. John
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@redhat.com> --- 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
On 11/14/2017 09:47 AM, Michal Privoznik wrote:
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@redhat.com> --- 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(+)
With the obvious merge to the ever changing current capabilities at the top of the tree... Reviewed-by: John Ferlan <jferlan@redhat.com> John
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@redhat.com> --- 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 Tue, Nov 14, 2017 at 15:47:39 +0100, 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@redhat.com> --- 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;
This capability does not need to be checked in the loop. Also the loop does not make sense to be finished once you deduct that 'numa_distances' is true.
+ } + } + } +
On 11/14/2017 04:13 PM, Peter Krempa wrote:
On Tue, Nov 14, 2017 at 15:47:39 +0100, 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@redhat.com> --- 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;
This capability does not need to be checked in the loop. Also the loop does not make sense to be finished once you deduct that 'numa_distances' is true.
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/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@redhat.com> --- 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) break; } if (i < ncells) { CapsCheck The next double for loop now would seem to apply without the need for numa_distances boolean. } Or am I off in the weeds? John
+ + 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);
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@redhat.com> --- 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). Michal
On 11/22/2017 04:45 AM, Michal Privoznik wrote:
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@redhat.com> --- 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.
Oh right... Could have another accessor too, but your need is a bit more specific especially since as you point out next qemu will default to 10 and 20... Still I think with the different name this will make more sense in v2.
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).
Nothing is ever so simple, but I did ask if I was in the weeds too basically because I've learned over time that simplicity and numa are not synonymous! John
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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><distances></code> element within the NUMA cell - configuration. + configuration. Drivers which support this include Xen and QEMU. </description> </change> <change> -- 2.13.6
On 11/14/2017 09:47 AM, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/news.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Drat - should have noted this in review of 4/5... I think you'll also need a change to docs/formatdomain.html.in - see the last tidbit in commit id '74119a03f' Describing distances between NUMA cells is currently only supported by Xen. If no <code>distances</code> are given to describe the SLIT data between different cells, it will default to a scheme using 10 for local and 20 for remote distances. For this though... Reviewed-by: John Ferlan <jferlan@redhat.com> John
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><distances></code> element within the NUMA cell - configuration. + configuration. Drivers which support this include Xen and QEMU. </description> </change> <change>
participants (3)
-
John Ferlan -
Michal Privoznik -
Peter Krempa