[PATCH 0/2] Definitive fix for NUMA nodes detection
See 2/2 for explanation. Michal Prívozník (2): virnuma: Assume numa_bitmask_isbitset() exists virnuma: Use numa_nodes_ptr when checking available NUMA nodes meson.build | 5 +---- src/util/virnuma.c | 8 ++++---- 2 files changed, 5 insertions(+), 8 deletions(-) -- 2.26.2
This function was introduced in the 2.0.6 release which happened in December 2010. I think it is safe to assume that all libnuma we deal with have the function. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- meson.build | 5 +---- src/util/virnuma.c | 6 +++--- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/meson.build b/meson.build index bcb978292b..195d7cd784 100644 --- a/meson.build +++ b/meson.build @@ -1239,13 +1239,10 @@ else intl_dep = dependency('', required: false) endif +numactl_version = '2.0.6' numactl_dep = cc.find_library('numa', required: get_option('numactl')) if numactl_dep.found() conf.set('WITH_NUMACTL', 1) - - if cc.has_function('numa_bitmask_isbitset', dependencies: [ numactl_dep ]) - conf.set('WITH_NUMA_BITMASK_ISBITSET', 1) - endif endif openwsman_version = '2.2.3' diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 2872ce3c5e..b8cf8b4510 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -423,7 +423,7 @@ virNumaGetMaxCPUs(void) } -#if WITH_NUMACTL && WITH_NUMA_BITMASK_ISBITSET +#if WITH_NUMACTL /** * virNumaNodeIsAvailable: * @node: node to check @@ -493,7 +493,7 @@ virNumaGetDistances(int node, return 0; } -#else /* !(WITH_NUMACTL && WITH_NUMA_BITMASK_ISBITSET) */ +#else /* !WITH_NUMACTL */ bool virNumaNodeIsAvailable(int node) @@ -518,7 +518,7 @@ virNumaGetDistances(int node G_GNUC_UNUSED, VIR_DEBUG("NUMA distance information isn't available on this host"); return 0; } -#endif /* !(WITH_NUMACTL && WITH_NUMA_BITMASK_ISBITSET) */ +#endif /* !WITH_NUMACTL */ /* currently all the huge page stuff below is linux only */ -- 2.26.2
On Fri, Sep 11, 2020 at 01:45:11PM +0200, Michal Privoznik wrote:
This function was introduced in the 2.0.6 release which happened in December 2010. I think it is safe to assume that all libnuma we deal with have the function.
Yeah, there's a fair bit of historical baggage we could clean up. I mean we're still asking for # define NUMA_VERSION1_COMPATIBILITY 1 which is probably not relevant since RHEL6
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- meson.build | 5 +---- src/util/virnuma.c | 6 +++--- 2 files changed, 4 insertions(+), 7 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
In v6.7.0-rc1~86 I've tried to fix a problem where we were not detecting NUMA nodes properly because we misused behaviour of a libnuma API and as it turned out the behaviour was correct for hosts with 64 CPUs in one NUMA node. So I changed the code to use nodemask_isset(&numa_all_nodes, ..) instead and it fixed the problem on such hosts. However, what I did not realize is that numa_all_nodes does not reflect all NUMA nodes visible to userspace, it contains only those nodes that the process (libvirtd) an allocate memory from, which can be only a subset of all NUMA nodes. The bitmask that contains all NUMA nodes visible to userspace and which one I should have used is: numa_nodes_ptr. For curious ones: https://github.com/numactl/numactl/commit/4a22f2238234155e11e3e2717c01186472... And as I was fixing virNumaGetNodeCPUs() I came to realize that we already have a function that wraps the correct bitmask: virNumaNodeIsAvailable(). Fixes: 24d7d85208f812a45686b32a0561cc9c5c9a49c9 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1876956 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnuma.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virnuma.c b/src/util/virnuma.c index b8cf8b4510..39f0f30917 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -260,7 +260,7 @@ virNumaGetNodeCPUs(int node, *cpus = NULL; - if (!nodemask_isset(&numa_all_nodes, node)) { + if (!virNumaNodeIsAvailable(node)) { VIR_DEBUG("NUMA topology for cell %d is not available, ignoring", node); return -2; } -- 2.26.2
On Fri, Sep 11, 2020 at 01:45:12PM +0200, Michal Privoznik wrote:
In v6.7.0-rc1~86 I've tried to fix a problem where we were not detecting NUMA nodes properly because we misused behaviour of a libnuma API and as it turned out the behaviour was correct for hosts with 64 CPUs in one NUMA node. So I changed the code to use nodemask_isset(&numa_all_nodes, ..) instead and it fixed the problem on such hosts. However, what I did not realize is that numa_all_nodes does not reflect all NUMA nodes visible to userspace, it contains only those nodes that the process (libvirtd) an allocate memory from, which can be only a subset of all NUMA nodes. The bitmask that contains all NUMA nodes visible to userspace and which one I should have used is: numa_nodes_ptr. For curious ones:
https://github.com/numactl/numactl/commit/4a22f2238234155e11e3e2717c01186472...
And as I was fixing virNumaGetNodeCPUs() I came to realize that we already have a function that wraps the correct bitmask: virNumaNodeIsAvailable().
Fixes: 24d7d85208f812a45686b32a0561cc9c5c9a49c9 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1876956 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnuma.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (2)
-
Daniel P. Berrangé -
Michal Privoznik