[libvirt] [PATCH] qemuxml2argvtest: uncoditionally mock NUMA routines
Currently, qemuxml2argvmock.c only mocks virNumaNodeIsAvailable(), and only if libvirt was built with NUMA support. This causes some test failures where NUMA is involved. For example, memory-hotplug-dimm fails because it goes all they way down to qemuBuildMemoryBackendStr() that calls virNumaNodesetIsAvailable() for a user specified nodeset and fails, hence the test (unexpectedly) fails. To make qemuxml2argtest successfully run on non-NUMA platforms, do the following: - mock virNumaNodeIsAvailable() unconditionally - add a mock for virNumaNodesetIsAvailable() --- tests/qemuxml2argvmock.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index 1616eed..57e56ab 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -56,17 +56,28 @@ virNumaGetMaxNode(void) return maxnodesNum; } -#if WITH_NUMACTL && HAVE_NUMA_BITMASK_ISBITSET -/* - * In case libvirt is compiled with full NUMA support, we need to mock - * this function in order to fake what numa nodes are available. - */ bool virNumaNodeIsAvailable(int node) { return node >= 0 && node <= virNumaGetMaxNode(); } -#endif /* WITH_NUMACTL && HAVE_NUMA_BITMASK_ISBITSET */ + +bool +virNumaNodesetIsAvailable(virBitmapPtr nodeset ATTRIBUTE_UNUSED) +{ + ssize_t bit = -1; + + if (!nodeset) + return true; + + while ((bit = virBitmapNextSetBit(nodeset, bit)) >= 0) { + if (virNumaNodeIsAvailable(bit)) + continue; + + return false; + } + return true; +} char * virTPMCreateCancelPath(const char *devpath) -- 2.7.4
On Thu, May 12, 2016 at 10:47:04AM +0300, Roman Bogorodskiy wrote:
Currently, qemuxml2argvmock.c only mocks virNumaNodeIsAvailable(), and only if libvirt was built with NUMA support. This causes some test failures where NUMA is involved.
This is misleading, you are probably compiling with clang, which inlines virNumaGetMaxNode, so the mocked version of it is never called. With no NUMA support, even libvirtd uses the implementation of virNumaNodeIsAvailable based on virNumaGetMaxNode. And the mocked virNumaNodesetIsAvailable is functionally identical to the implementation in virnuma.c. There were two attempts to get around it: __attribute__((__noinline__)) https://www.redhat.com/archives/libvir-list/2015-March/msg00203.html and mocking virNumaNodesetIsAvailable by its identical copy: https://www.redhat.com/archives/libvir-list/2016-March/msg00515.html (mocking virNumaNodeIsAvailable should not be needed). All they need is a decision. Jan
For example, memory-hotplug-dimm fails because it goes all they way down to qemuBuildMemoryBackendStr() that calls virNumaNodesetIsAvailable() for a user specified nodeset and fails, hence the test (unexpectedly) fails.
To make qemuxml2argtest successfully run on non-NUMA platforms, do the following:
- mock virNumaNodeIsAvailable() unconditionally - add a mock for virNumaNodesetIsAvailable() --- tests/qemuxml2argvmock.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)
On Fri, 2016-05-13 at 14:07 +0200, Ján Tomko wrote:
On Thu, May 12, 2016 at 10:47:04AM +0300, Roman Bogorodskiy wrote:
Currently, qemuxml2argvmock.c only mocks virNumaNodeIsAvailable(), and only if libvirt was built with NUMA support. This causes some test failures where NUMA is involved.
This is misleading, you are probably compiling with clang, which inlines virNumaGetMaxNode, so the mocked version of it is never called.
With no NUMA support, even libvirtd uses the implementation of virNumaNodeIsAvailable based on virNumaGetMaxNode. And the mocked virNumaNodesetIsAvailable is functionally identical to the implementation in virnuma.c.
There were two attempts to get around it: __attribute__((__noinline__)) https://www.redhat.com/archives/libvir-list/2015-March/msg00203.html and mocking virNumaNodesetIsAvailable by its identical copy: https://www.redhat.com/archives/libvir-list/2016-March/msg00515.html (mocking virNumaNodeIsAvailable should not be needed).
All they need is a decision.
+1 for the __attribute((__noinline__)) approach. -- Andrea Bolognani Software Engineer - Virtualization Team
Ján Tomko wrote:
On Thu, May 12, 2016 at 10:47:04AM +0300, Roman Bogorodskiy wrote:
Currently, qemuxml2argvmock.c only mocks virNumaNodeIsAvailable(), and only if libvirt was built with NUMA support. This causes some test failures where NUMA is involved.
This is misleading, you are probably compiling with clang, which inlines virNumaGetMaxNode, so the mocked version of it is never called.
With no NUMA support, even libvirtd uses the implementation of virNumaNodeIsAvailable based on virNumaGetMaxNode. And the mocked virNumaNodesetIsAvailable is functionally identical to the implementation in virnuma.c.
There were two attempts to get around it: __attribute__((__noinline__)) https://www.redhat.com/archives/libvir-list/2015-March/msg00203.html and mocking virNumaNodesetIsAvailable by its identical copy: https://www.redhat.com/archives/libvir-list/2016-March/msg00515.html (mocking virNumaNodeIsAvailable should not be needed).
All they need is a decision.
It's strange, but neither of these two patches fixes the issue for me. That's with 3.8.0. It starts working only when I pull in both virNumaNodeIsAvailable() and virNumaNodesetIsAvailable() into qemuxml2argvmock.c without the #if WITH_NUMACTL conditional. Roman Bogorodskiy
On Fri, May 13, 2016 at 08:53:08PM +0300, Roman Bogorodskiy wrote:
Ján Tomko wrote:
On Thu, May 12, 2016 at 10:47:04AM +0300, Roman Bogorodskiy wrote:
Currently, qemuxml2argvmock.c only mocks virNumaNodeIsAvailable(), and only if libvirt was built with NUMA support. This causes some test failures where NUMA is involved.
This is misleading, you are probably compiling with clang, which inlines virNumaGetMaxNode, so the mocked version of it is never called.
With no NUMA support, even libvirtd uses the implementation of virNumaNodeIsAvailable based on virNumaGetMaxNode. And the mocked virNumaNodesetIsAvailable is functionally identical to the implementation in virnuma.c.
There were two attempts to get around it: __attribute__((__noinline__)) https://www.redhat.com/archives/libvir-list/2015-March/msg00203.html and mocking virNumaNodesetIsAvailable by its identical copy: https://www.redhat.com/archives/libvir-list/2016-March/msg00515.html (mocking virNumaNodeIsAvailable should not be needed).
All they need is a decision.
It's strange, but neither of these two patches fixes the issue for me. That's with 3.8.0. It starts working only when I pull in both virNumaNodeIsAvailable() and virNumaNodesetIsAvailable() into qemuxml2argvmock.c without the #if WITH_NUMACTL conditional.
Right, libvirt's virNumaGetMaxNode gets inlined in libvirt's virNumaNodeIsAvailable, and the virNumaGetMaxNode defined in tests would not get used. ACK to the original patch then. Please mention that this is caused by different handling of inlining by clang in a comment and/or the commit message. Jan
Ján Tomko wrote:
On Fri, May 13, 2016 at 08:53:08PM +0300, Roman Bogorodskiy wrote:
Ján Tomko wrote:
On Thu, May 12, 2016 at 10:47:04AM +0300, Roman Bogorodskiy wrote:
Currently, qemuxml2argvmock.c only mocks virNumaNodeIsAvailable(), and only if libvirt was built with NUMA support. This causes some test failures where NUMA is involved.
This is misleading, you are probably compiling with clang, which inlines virNumaGetMaxNode, so the mocked version of it is never called.
With no NUMA support, even libvirtd uses the implementation of virNumaNodeIsAvailable based on virNumaGetMaxNode. And the mocked virNumaNodesetIsAvailable is functionally identical to the implementation in virnuma.c.
There were two attempts to get around it: __attribute__((__noinline__)) https://www.redhat.com/archives/libvir-list/2015-March/msg00203.html and mocking virNumaNodesetIsAvailable by its identical copy: https://www.redhat.com/archives/libvir-list/2016-March/msg00515.html (mocking virNumaNodeIsAvailable should not be needed).
All they need is a decision.
It's strange, but neither of these two patches fixes the issue for me. That's with 3.8.0. It starts working only when I pull in both virNumaNodeIsAvailable() and virNumaNodesetIsAvailable() into qemuxml2argvmock.c without the #if WITH_NUMACTL conditional.
Right, libvirt's virNumaGetMaxNode gets inlined in libvirt's virNumaNodeIsAvailable, and the virNumaGetMaxNode defined in tests would not get used.
ACK to the original patch then. Please mention that this is caused by different handling of inlining by clang in a comment and/or the commit message.
Thanks, I'll re-word the commit message then. Meanwhile, I was playing around with this over the weekend and it behaves really weird. I decided not to use debugger to make sure nothing's caused any additional side effects and added some debug printf's. Specifically, I've added logging to all the virNumaGetMaxNode() implementations, and I've also added some logging to virNumaNodeIsAvailable(). I fail to understand the result. Specifically, it produces the following output: virNumaGetMaxNode: mocked, maxnodesNum = 7 virNumaGetMaxNode() = -1 virNumaGetMaxNode: mocked, maxnodesNum = 7 virNumaGetMaxNode() = -1 virNumaGetMaxNode: mocked, maxnodesNum = 7 virNumaGetMaxNode() = -1 virNumaGetMaxNode: mocked, maxnodesNum = 7 virNumaGetMaxNode() = -1 virNumaGetMaxNode: mocked, maxnodesNum = 7 virNumaGetMaxNode() = -1 virNumaGetMaxNode: mocked, maxnodesNum = 7 virNumaNodeIsAvailable: max_node = -1 (I've added a look like for (int i = 0; i < 5; i++) printf("virNumaGetMaxNode() = %d\n", virNumaGetMaxNode()); So... it is calling the mocked version, but uses -1 as a result. When I change the real implementation to return e.g. 42 instead of -1, I get 42, but I don't see its logging. It doesn't change when I add ATTRIBUTE_NOINLINE for virNumaGetMaxNode. I'm wondering I miss something obvious or maybe my system is screwed? Debug ideas appreciated. Meanwhile, I'll try to better understand how LD_PRELOAD works as well as objdump's asm format, but that might take a while... Roman Bogorodskiy
participants (3)
-
Andrea Bolognani -
Ján Tomko -
Roman Bogorodskiy