Re: [libvirt] [PATCH] fix numa-related (and kernel-dependent) test failures

"Daniel P. Berrange" <berrange(a)redhat.com&gt; wrote: This took longer than I expected. First, I just fixed the portability problem, but identically in two places. Can't have that. So factored out that duplication. That required many Makefile.am changes so as not to induce link failure for every program. It also required addition to exported sym table so libvirtd still links. Finally it all built and the daemon-conf test was failing. Phew. That was just because for other testing I'd left a non-root libvirtd running. [re-reviewing the diff, I see I left some white-space changes (removing space-before-TAB) in some Makefile.am. I can move those into a separate patch on request. ] From: Jim Meyering <meyering(a)redhat.com&gt; Date: Thu, 18 Dec 2008 13:53:47 +0100 Subject: [PATCH] make NUMA-initialization code more portable and more robust qemudCapsInitNUMA and umlCapsInitNUMA were identical, so this change factors them into a new function, virCapsInitNUMA, and puts it in nodeinfo.c. Putting it there means several programs must now link against -lnuma, hence all the Makefile.am adjustments. In addition to factoring out the duplicates, this change also adjusts that function definition (along with its macros) so that it works with Fedora 9's numactl version 1, and makes it so the code will work even if someone builds the kernel with CONFIG_NR_CPUS > 4096. Finally, also perform this NUMA initialization for the lxc and openvz drivers. * src/nodeinfo.c: Include <stdint.h>, <numa.h> and "memory.h". (virCapsInitNUMA): Rename from qemudCapsInitNUMA and umlCapsInitNUMA. (NUMA_MAX_N_CPUS): Define depending on NUMA API version. (n_bits, MASK_CPU_ISSET): Define, adjust, use uint64 rather than long. * src/nodeinfo.h: Include "capabilities.h". (virCapsInitNUMA): Declare it. * docs/examples/index.py (dump_Makefile): Add $(NUMACTL_LIBS) to LDADDS. * docs/examples/Makefile.am: Regenerate. * examples/domain-events/events-c/Makefile.am: * src/Makefile.am: Add $(NUMACTL_CFLAGS) and $(NUMACTL_LIBS) to various compile/link-related variables. * src/qemu_conf.c (qemudCapsInitNUMA): Remove duplicate code. Adjust caller. * src/uml_conf.c (umlCapsInitNUMA): Likewise. * tests/Makefile.am: Link with $(NUMACTL_LIBS). * src/lxc_conf.c (lxcCapsInit): Perform NUMA initialization here, too. * src/openvz_conf.c (openvzCapsInit): And here. * qemud/Makefile.am (libvirtd_LDADD): Add $(NUMACTL_LIBS) (libvirtd_CFLAGS): Add $(NUMACTL_CFLAGS) and align. * src/libvirt_sym.version.in: Add virCapsInitNUMA so that libvirtd can link to this function. --- docs/examples/Makefile.am | 2 +- docs/examples/index.py | 2 +- examples/domain-events/events-c/Makefile.am | 4 +- qemud/Makefile.am | 27 ++++++---- src/Makefile.am | 21 ++++---- src/libvirt_sym.version.in | 1 + src/lxc_conf.c | 3 + src/nodeinfo.c | 75 ++++++++++++++++++++++++++- src/nodeinfo.h | 4 +- src/openvz_conf.c | 3 + src/qemu_conf.c | 67 +----------------------- src/uml_conf.c | 69 +------------------------ tests/Makefile.am | 1 + 13 files changed, 119 insertions(+), 160 deletions(-) diff --git a/docs/examples/Makefile.am b/docs/examples/Makefile.am index d8e4868..f3a3e29 100644 --- a/docs/examples/Makefile.am +++ b/docs/examples/Makefile.am @@ -3,7 +3,7 @@ SUBDIRS=python INCLUDES = -I$(top_builddir)/include -I$(top_srcdir)/include -I@srcdir@/include DEPS = $(top_builddir)/src/libvirt.la -LDADDS = @STATIC_BINARIES@ $(WARN_CFLAGS) $(top_builddir)/src/libvirt.la $(COVERAGE_LDFLAGS) +LDADDS = @STATIC_BINARIES@ $(WARN_CFLAGS) $(top_builddir)/src/libvirt.la $(NUMACTL_LIBS) $(COVERAGE_LDFLAGS) rebuild: examples.xml index.html diff --git a/docs/examples/index.py b/docs/examples/index.py index 6be80c5..ad22073 100755 --- a/docs/examples/index.py +++ b/docs/examples/index.py @@ -226,7 +226,7 @@ SUBDIRS=python INCLUDES = -I$(top_builddir)/include -I$(top_srcdir)/include -I@srcdir@/include DEPS = $(top_builddir)/src/libvirt.la LDADDS = @STATIC_BINARIES@ $(WARN_CFLAGS) $(top_builddir)/src/libvirt.la \ - $(COVERAGE_LDFLAGS) + $(NUMACTL_LIBS) $(COVERAGE_LDFLAGS) rebuild: examples.xml index.html diff --git a/examples/domain-events/events-c/Makefile.am b/examples/domain-events/events-c/Makefile.am index 60b1589..4e829cb 100644 --- a/examples/domain-events/events-c/Makefile.am +++ b/examples/domain-events/events-c/Makefile.am @@ -1,5 +1,5 @@ -INCLUDES = -I@top_srcdir@/include +INCLUDES = -I$(top_srcdir)/include noinst_PROGRAMS = event-test event_test_CFLAGS = $(WARN_CFLAGS) event_test_SOURCES = event-test.c -event_test_LDADD = @top_builddir(a)/src/libvirt.la +event_test_LDADD = $(top_builddir)/src/libvirt.la $(NUMACTL_LIBS) diff --git a/qemud/Makefile.am b/qemud/Makefile.am index 8983416..e8278f3 100644 --- a/qemud/Makefile.am +++ b/qemud/Makefile.am @@ -77,16 +77,22 @@ libvirtd_SOURCES = $(DAEMON_SOURCES) #-D_XOPEN_SOURCE=600 -D_XOPEN_SOURCE_EXTENDED=1 -D_POSIX_C_SOURCE=199506L libvirtd_CFLAGS = \ - -I$(top_srcdir)/gnulib/lib -I../gnulib/lib \ - -I$(top_srcdir)/include -I$(top_builddir)/include \ - -I$(top_srcdir)/src \ - $(LIBXML_CFLAGS) $(GNUTLS_CFLAGS) $(SASL_CFLAGS) \ - $(POLKIT_CFLAGS) \ - $(WARN_CFLAGS) -DLOCAL_STATE_DIR="\"$(localstatedir)\"" \ - $(COVERAGE_CFLAGS) \ - -DSYSCONF_DIR="\"$(sysconfdir)\"" \ - -DQEMUD_PID_FILE="\"$(QEMUD_PID_FILE)\"" \ - -DREMOTE_PID_FILE="\"$(REMOTE_PID_FILE)\"" \ + -I$(top_srcdir)/gnulib/lib \ + -I../gnulib/lib \ + -I$(top_srcdir)/include \ + -I$(top_builddir)/include \ + -I$(top_srcdir)/src \ + $(LIBXML_CFLAGS) \ + $(GNUTLS_CFLAGS) \ + $(SASL_CFLAGS) \ + $(NUMACTL_CFLAGS) \ + $(POLKIT_CFLAGS) \ + $(WARN_CFLAGS) \ + $(COVERAGE_CFLAGS) \ + -DLOCAL_STATE_DIR="\"$(localstatedir)\"" \ + -DSYSCONF_DIR="\"$(sysconfdir)\"" \ + -DQEMUD_PID_FILE="\"$(QEMUD_PID_FILE)\"" \ + -DREMOTE_PID_FILE="\"$(REMOTE_PID_FILE)\"" \ -DGETTEXT_PACKAGE=\"$(PACKAGE)\" libvirtd_LDFLAGS = \ @@ -96,6 +102,7 @@ libvirtd_LDFLAGS = \ libvirtd_LDADD = \ $(LIBXML_LIBS) \ $(GNUTLS_LIBS) \ + $(NUMACTL_LIBS) \ $(SASL_LIBS) \ $(POLKIT_LIBS) diff --git a/src/Makefile.am b/src/Makefile.am index 68f0c01..6c65f91 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -55,7 +55,7 @@ UTIL_SOURCES = \ xml.c xml.h # Internal generic driver infrastructure -DRIVER_SOURCES = \ +DRIVER_SOURCES = \ driver.c driver.h \ internal.h \ datatypes.c datatypes.h \ @@ -147,7 +147,7 @@ STORAGE_DRIVER_FS_SOURCES = \ storage_backend_fs.h storage_backend_fs.c STORAGE_DRIVER_LVM_SOURCES = \ - storage_backend_logical.h \ + storage_backend_logical.h \ storage_backend_logical.c STORAGE_DRIVER_ISCSI_SOURCES = \ @@ -189,8 +189,8 @@ libvirt_driver_la_SOURCES = \ $(STORAGE_CONF_SOURCES) \ $(NODE_DEVICE_CONF_SOURCES) -libvirt_driver_la_CFLAGS = $(XEN_CFLAGS) -libvirt_driver_la_LDFLAGS = $(XEN_LIBS) +libvirt_driver_la_CFLAGS = $(XEN_CFLAGS) $(NUMACTL_CFLAGS) +libvirt_driver_la_LDFLAGS = $(XEN_LIBS) $(NUMACTL_LIBS) if WITH_TEST if WITH_DRIVER_MODULES @@ -430,13 +430,14 @@ virsh_SOURCES = \ virsh.c virsh_LDFLAGS = $(WARN_CFLAGS) $(COVERAGE_LDFLAGS) -virsh_LDADD = \ - $(STATIC_BINARIES) \ +virsh_LDADD = \ + $(STATIC_BINARIES) \ $(WARN_CFLAGS) \ + $(NUMACTL_LIBS) \ libvirt.la \ ../gnulib/lib/libgnu.la \ $(VIRSH_LIBS) -virsh_CFLAGS = $(COVERAGE_CFLAGS) $(READLINE_CFLAGS) +virsh_CFLAGS = $(COVERAGE_CFLAGS) $(READLINE_CFLAGS) $(NUMACTL_CFLAGS) BUILT_SOURCES = virsh-net-edit.c virsh-pool-edit.c virsh-net-edit.c: virsh.c Makefile.am @@ -518,11 +519,11 @@ libexec_PROGRAMS += libvirt_lxc libvirt_lxc_SOURCES = \ $(LXC_CONTROLLER_SOURCES) \ - $(UTIL_SOURCES) \ + $(UTIL_SOURCES) \ $(DOMAIN_CONF_SOURCES) libvirt_lxc_LDFLAGS = $(WARN_CFLAGS) $(COVERAGE_LDCFLAGS) -libvirt_lxc_LDADD = $(LIBXML_LIBS) ../gnulib/lib/libgnu.la -libvirt_lxc_CFLAGS = $(LIBPARTED_CFLAGS) +libvirt_lxc_LDADD = $(LIBXML_LIBS) $(NUMACTL_LIBS) ../gnulib/lib/libgnu.la +libvirt_lxc_CFLAGS = $(LIBPARTED_CFLAGS) $(NUMACTL_CFLAGS) endif endif EXTRA_DIST += $(LXC_CONTROLLER_SOURCES) diff --git a/src/libvirt_sym.version.in b/src/libvirt_sym.version.in index fa9bc5a..afe8659 100644 --- a/src/libvirt_sym.version.in +++ b/src/libvirt_sym.version.in @@ -489,6 +489,7 @@ LIBVIRT_PRIVATE_@VERSION@ { # nodeinfo.h virNodeInfoPopulate; + virCapsInitNUMA; # node_device_conf.h diff --git a/src/lxc_conf.c b/src/lxc_conf.c index 0db9bb5..4fb57d3 100644 --- a/src/lxc_conf.c +++ b/src/lxc_conf.c @@ -43,6 +43,9 @@ virCapsPtr lxcCapsInit(void) 0, 0)) == NULL) goto no_memory; + if (virCapsInitNUMA(caps) < 0) + goto no_memory; + /* XXX shouldn't 'borrow' KVM's prefix */ virCapabilitiesSetMacPrefix(caps, (unsigned char []){ 0x52, 0x54, 0x00 }); diff --git a/src/nodeinfo.c b/src/nodeinfo.c index fd58c2b..aac7d3e 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -26,13 +26,22 @@ #include <stdio.h> #include <string.h> #include <stdlib.h> +#include <stdint.h> #include <errno.h> -#include "c-ctype.h" + +#if HAVE_NUMACTL +# define NUMA_VERSION1_COMPATIBILITY 1 +# include <numa.h> +#endif #ifdef HAVE_SYS_UTSNAME_H #include <sys/utsname.h> #endif +#include "memory.h" + +#include "c-ctype.h" + #include "virterror_internal.h" #include "nodeinfo.h" #include "physmem.h" @@ -171,3 +180,67 @@ int virNodeInfoPopulate(virConnectPtr conn, return -1; #endif } + +#if HAVE_NUMACTL +# if LIBNUMA_API_VERSION <= 1 +# define NUMA_MAX_N_CPUS 4096 +# else +# define NUMA_MAX_N_CPUS (numa_all_cpus_ptr->size) +# endif + +# define n_bits(var) (8 * sizeof(var)) +# define MASK_CPU_ISSET(mask, cpu) \ + (((mask)[((cpu) / n_bits(*(mask)))] >> ((cpu) % n_bits(*(mask)))) & 1) + +int +virCapsInitNUMA(virCapsPtr caps) +{ + int n; + uint64_t *mask = NULL; + int *cpus = NULL; + int ret = -1; + int max_n_cpus = NUMA_MAX_N_CPUS; + + if (numa_available() < 0) + return 0; + + int mask_n_bytes = max_n_cpus / 8; + if (VIR_ALLOC_N(mask, mask_n_bytes / sizeof *mask) < 0) + goto cleanup; + + for (n = 0 ; n <= numa_max_node() ; n++) { + int i; + int ncpus; + if (numa_node_to_cpus(n, mask, mask_n_bytes) < 0) + goto cleanup; + + for (ncpus = 0, i = 0 ; i < max_n_cpus ; i++) + if (MASK_CPU_ISSET(mask, i)) + ncpus++; + + if (VIR_ALLOC_N(cpus, ncpus) < 0) + goto cleanup; + + for (ncpus = 0, i = 0 ; i < max_n_cpus ; i++) + if (MASK_CPU_ISSET(mask, i)) + cpus[ncpus++] = i; + + if (virCapabilitiesAddHostNUMACell(caps, + n, + ncpus, + cpus) < 0) + goto cleanup; + + VIR_FREE(cpus); + } + + ret = 0; + +cleanup: + VIR_FREE(cpus); + VIR_FREE(mask); + return ret; +} +#else +int virCapsInitNUMA(virCapsPtr caps ATTRIBUTE_UNUSED) { return 0; } +#endif diff --git a/src/nodeinfo.h b/src/nodeinfo.h index e7c78eb..030f0ee 100644 --- a/src/nodeinfo.h +++ b/src/nodeinfo.h @@ -1,7 +1,7 @@ /* * nodeinfo.c: Helper routines for OS specific node information * - * Copyright (C) 2006, 2007 Red Hat, Inc. + * Copyright (C) 2006-2008 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -25,7 +25,9 @@ #define __VIR_NODEINFO_H__ #include "libvirt/libvirt.h" +#include "capabilities.h" int virNodeInfoPopulate(virConnectPtr conn, virNodeInfoPtr nodeinfo); +int virCapsInitNUMA(virCapsPtr caps); #endif /* __VIR_NODEINFO_H__*/ diff --git a/src/openvz_conf.c b/src/openvz_conf.c index 44a243b..49cc183 100644 --- a/src/openvz_conf.c +++ b/src/openvz_conf.c @@ -146,6 +146,9 @@ virCapsPtr openvzCapsInit(void) 0, 0)) == NULL) goto no_memory; + if (virCapsInitNUMA(caps) < 0) + goto no_memory; + virCapabilitiesSetMacPrefix(caps, (unsigned char[]){ 0x52, 0x54, 0x00 }); if ((guest = virCapabilitiesAddGuest(caps, diff --git a/src/qemu_conf.c b/src/qemu_conf.c index c973adb..ed5bcc9 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -36,11 +36,6 @@ #include <arpa/inet.h> #include <sys/utsname.h> -#if HAVE_NUMACTL -#define NUMA_VERSION1_COMPATIBILITY 1 -#include <numa.h> -#endif - #include "virterror_internal.h" #include "qemu_conf.h" #include "uuid.h" @@ -298,66 +293,6 @@ qemudCapsInitGuest(virCapsPtr caps, return 0; } -#if HAVE_NUMACTL -#define MAX_CPUS 4096 -#define MAX_CPUS_MASK_SIZE (sizeof(unsigned long)) -#define MAX_CPUS_MASK_BITS (MAX_CPUS_MASK_SIZE * 8) -#define MAX_CPUS_MASK_LEN (MAX_CPUS / (MAX_CPUS_MASK_BITS)) - -#define MASK_CPU_ISSET(mask, cpu) \ - (((mask)[((cpu) / MAX_CPUS_MASK_BITS)] >> ((cpu) % MAX_CPUS_MASK_BITS)) & 1) - -static int -qemudCapsInitNUMA(virCapsPtr caps) -{ - int n, i; - unsigned long *mask = NULL; - int ncpus; - int *cpus = NULL; - int ret = -1; - - if (numa_available() < 0) - return 0; - - if (VIR_ALLOC_N(mask, MAX_CPUS_MASK_LEN) < 0) - goto cleanup; - - for (n = 0 ; n <= numa_max_node() ; n++) { - int mask_n_bytes = numa_all_cpus_ptr->size / 8; - if (numa_node_to_cpus(n, mask, mask_n_bytes) < 0) - goto cleanup; - - for (ncpus = 0, i = 0 ; i < MAX_CPUS ; i++) - if (MASK_CPU_ISSET(mask, i)) - ncpus++; - - if (VIR_ALLOC_N(cpus, ncpus) < 0) - goto cleanup; - - for (ncpus = 0, i = 0 ; i < MAX_CPUS ; i++) - if (MASK_CPU_ISSET(mask, i)) - cpus[ncpus++] = i; - - if (virCapabilitiesAddHostNUMACell(caps, - n, - ncpus, - cpus) < 0) - goto cleanup; - - VIR_FREE(cpus); - } - - ret = 0; - -cleanup: - VIR_FREE(cpus); - VIR_FREE(mask); - return ret; -} -#else -static int qemudCapsInitNUMA(virCapsPtr caps ATTRIBUTE_UNUSED) { return 0; } -#endif - virCapsPtr qemudCapsInit(void) { struct utsname utsname; virCapsPtr caps; @@ -373,7 +308,7 @@ virCapsPtr qemudCapsInit(void) { /* Using KVM's mac prefix for QEMU too */ virCapabilitiesSetMacPrefix(caps, (unsigned char[]){ 0x52, 0x54, 0x00 }); - if (qemudCapsInitNUMA(caps) < 0) + if (virCapsInitNUMA(caps) < 0) goto no_memory; for (i = 0 ; i < ARRAY_CARDINALITY(arch_info_hvm) ; i++) diff --git a/src/uml_conf.c b/src/uml_conf.c index 00adf27..67c646a 100644 --- a/src/uml_conf.c +++ b/src/uml_conf.c @@ -36,11 +36,6 @@ #include <arpa/inet.h> #include <sys/utsname.h> -#if HAVE_NUMACTL -#define NUMA_VERSION1_COMPATIBILITY 1 -#include <numa.h> -#endif - #include "uml_conf.h" #include "uuid.h" #include "buf.h" @@ -52,68 +47,6 @@ #define umlLog(level, msg...) fprintf(stderr, msg) - - -#if HAVE_NUMACTL -#define MAX_CPUS 4096 -#define MAX_CPUS_MASK_SIZE (sizeof(unsigned long)) -#define MAX_CPUS_MASK_BITS (MAX_CPUS_MASK_SIZE * 8) -#define MAX_CPUS_MASK_LEN (MAX_CPUS / (MAX_CPUS_MASK_BITS)) - -#define MASK_CPU_ISSET(mask, cpu) \ - (((mask)[((cpu) / MAX_CPUS_MASK_BITS)] >> ((cpu) % MAX_CPUS_MASK_BITS)) & 1) - -static int -umlCapsInitNUMA(virCapsPtr caps) -{ - int n, i; - unsigned long *mask = NULL; - int ncpus; - int *cpus = NULL; - int ret = -1; - - if (numa_available() < 0) - return 0; - - if (VIR_ALLOC_N(mask, MAX_CPUS_MASK_LEN) < 0) - goto cleanup; - - for (n = 0 ; n <= numa_max_node() ; n++) { - int mask_n_bytes = numa_all_cpus_ptr->size / 8; - if (numa_node_to_cpus(n, mask, mask_n_bytes) < 0) - goto cleanup; - - for (ncpus = 0, i = 0 ; i < MAX_CPUS ; i++) - if (MASK_CPU_ISSET(mask, i)) - ncpus++; - - if (VIR_ALLOC_N(cpus, ncpus) < 0) - goto cleanup; - - for (ncpus = 0, i = 0 ; i < MAX_CPUS ; i++) - if (MASK_CPU_ISSET(mask, i)) - cpus[ncpus++] = i; - - if (virCapabilitiesAddHostNUMACell(caps, - n, - ncpus, - cpus) < 0) - goto cleanup; - - VIR_FREE(cpus); - } - - ret = 0; - -cleanup: - VIR_FREE(cpus); - VIR_FREE(mask); - return ret; -} -#else -static int umlCapsInitNUMA(virCapsPtr caps ATTRIBUTE_UNUSED) { return 0; } -#endif - virCapsPtr umlCapsInit(void) { struct utsname utsname; virCapsPtr caps; @@ -126,7 +59,7 @@ virCapsPtr umlCapsInit(void) { 0, 0)) == NULL) goto no_memory; - if (umlCapsInitNUMA(caps) < 0) + if (virCapsInitNUMA(caps) < 0) goto no_memory; if ((guest = virCapabilitiesAddGuest(caps, diff --git a/tests/Makefile.am b/tests/Makefile.am index 87e4235..88f44a6 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -30,6 +30,7 @@ LDADDS = \ $(GNUTLS_LIBS) \ $(SASL_LIBS) \ $(SELINUX_LIBS) \ + $(NUMACTL_LIBS) \ $(WARN_CFLAGS) \ ../src/libvirt_test.la \ ../gnulib/lib/libgnu.la \ -- 1.6.1.rc2.316.geb2f0