[PATCH v1 0/4] libxl: fix affinity assignment
Olaf Hering (4): libxl: remove obsolete VIR_LIBXL_EVENT_CONST libxl: remove obsolete check for result of xc_get_max_cpus libxl: remove obsolete libxlDomainSetVcpuAffinities function libxl: set vcpu affinity during domain creation src/libxl/libxl_conf.c | 53 ++++++++++++++++++++++++++++++++++++++++ src/libxl/libxl_domain.c | 48 +----------------------------------- src/libxl/libxl_domain.h | 18 +------------- src/libxl/libxl_driver.c | 6 +---- 4 files changed, 56 insertions(+), 69 deletions(-)
In Xen 4.2 struct libxl_event_hooks had a member which was erroneously declared const. Since libvirt requires at least Xen 4.6, remove the dead code. Signed-off-by: Olaf Hering <olaf@aepfle.de> --- src/libxl/libxl_domain.c | 2 +- src/libxl/libxl_domain.h | 14 +------------- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 917f6f1d81..d78765ad84 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -641,7 +641,7 @@ libxlDomainHandleDeath(libxlDriverPrivate *driver, virDomainObj *vm) * Handle previously registered domain event notification from libxenlight. */ void -libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event) +libxlDomainEventHandler(void *data, libxl_event *event) { libxlDriverPrivate *driver = data; libxl_shutdown_reason xl_reason = event->u.domain_shutdown.shutdown_reason; diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h index 8223b1d255..cbe7ba19ba 100644 --- a/src/libxl/libxl_domain.h +++ b/src/libxl/libxl_domain.h @@ -117,20 +117,8 @@ void libxlDomainCleanup(libxlDriverPrivate *driver, virDomainObj *vm); -/* - * Note: Xen 4.3 removed the const from the event handler signature. - * Detect which signature to use based on - * LIBXL_HAVE_NONCONST_EVENT_OCCURS_EVENT_ARG. - */ -#ifdef LIBXL_HAVE_NONCONST_EVENT_OCCURS_EVENT_ARG -# define VIR_LIBXL_EVENT_CONST /* empty */ -#else -# define VIR_LIBXL_EVENT_CONST const -#endif - void -libxlDomainEventHandler(void *data, - VIR_LIBXL_EVENT_CONST libxl_event *event); +libxlDomainEventHandler(void *data, libxl_event *event); int libxlDomainAutoCoreDump(libxlDriverPrivate *driver,
On 5/3/21 4:56 AM, Olaf Hering wrote:
In Xen 4.2 struct libxl_event_hooks had a member which was erroneously declared const. Since libvirt requires at least Xen 4.6, remove the dead code.
Signed-off-by: Olaf Hering <olaf@aepfle.de> --- src/libxl/libxl_domain.c | 2 +- src/libxl/libxl_domain.h | 14 +------------- 2 files changed, 2 insertions(+), 14 deletions(-)
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 917f6f1d81..d78765ad84 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -641,7 +641,7 @@ libxlDomainHandleDeath(libxlDriverPrivate *driver, virDomainObj *vm) * Handle previously registered domain event notification from libxenlight. */ void -libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event) +libxlDomainEventHandler(void *data, libxl_event *event) { libxlDriverPrivate *driver = data; libxl_shutdown_reason xl_reason = event->u.domain_shutdown.shutdown_reason; diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h index 8223b1d255..cbe7ba19ba 100644 --- a/src/libxl/libxl_domain.h +++ b/src/libxl/libxl_domain.h @@ -117,20 +117,8 @@ void libxlDomainCleanup(libxlDriverPrivate *driver, virDomainObj *vm);
-/* - * Note: Xen 4.3 removed the const from the event handler signature. - * Detect which signature to use based on - * LIBXL_HAVE_NONCONST_EVENT_OCCURS_EVENT_ARG. - */ -#ifdef LIBXL_HAVE_NONCONST_EVENT_OCCURS_EVENT_ARG -# define VIR_LIBXL_EVENT_CONST /* empty */ -#else -# define VIR_LIBXL_EVENT_CONST const -#endif - void -libxlDomainEventHandler(void *data, - VIR_LIBXL_EVENT_CONST libxl_event *event); +libxlDomainEventHandler(void *data, libxl_event *event);
int libxlDomainAutoCoreDump(libxlDriverPrivate *driver,
xc_get_max_cpus from Xen version 4.3 may return 0 in case xc_physinfo fails. This has been fixed in Xen 4.4. Remove the obsolete result check from libvirt. Just convert libxl error codes to plain -1. Signed-off-by: Olaf Hering <olaf@aepfle.de> --- src/libxl/libxl_driver.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index cf3ee4db3d..99a170ff2a 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -972,11 +972,7 @@ libxlConnectGetMaxVcpus(virConnectPtr conn, const char *type G_GNUC_UNUSED) cfg = libxlDriverConfigGet(driver); ret = libxl_get_max_cpus(cfg->ctx); - /* On failure, libxl_get_max_cpus() will return ERROR_FAIL from Xen 4.4 - * onward, but it ever returning 0 is obviously wrong too (and it is - * what happens, on failure, on Xen 4.3 and earlier). Therefore, a 'less - * or equal' is the catchall we want. */ - if (ret <= 0) + if (ret < 0) ret = -1; virObjectUnref(cfg);
On 5/3/21 4:56 AM, Olaf Hering wrote:
xc_get_max_cpus from Xen version 4.3 may return 0 in case xc_physinfo fails. This has been fixed in Xen 4.4. Remove the obsolete result check from libvirt. Just convert libxl error codes to plain -1.
Signed-off-by: Olaf Hering <olaf@aepfle.de> --- src/libxl/libxl_driver.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
Reviewed-by: Jim Fehlig <jfehlig@suse.com> I'll push 1 and 2 independent of the others. Jim
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index cf3ee4db3d..99a170ff2a 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -972,11 +972,7 @@ libxlConnectGetMaxVcpus(virConnectPtr conn, const char *type G_GNUC_UNUSED)
cfg = libxlDriverConfigGet(driver); ret = libxl_get_max_cpus(cfg->ctx); - /* On failure, libxl_get_max_cpus() will return ERROR_FAIL from Xen 4.4 - * onward, but it ever returning 0 is obviously wrong too (and it is - * what happens, on failure, on Xen 4.3 and earlier). Therefore, a 'less - * or equal' is the catchall we want. */ - if (ret <= 0) + if (ret < 0) ret = -1;
virObjectUnref(cfg);
Since Xen 4.5 the libxl allows to set affinities during domain creation. This enables Xen to allocate the domain memory on NUMA nodes close to the specified pcpus. Remove the old approach, which moved vcpus after Xen had already allocated domain memory. Signed-off-by: Olaf Hering <olaf@aepfle.de> --- src/libxl/libxl_domain.c | 46 ---------------------------------------- src/libxl/libxl_domain.h | 4 ---- 2 files changed, 50 deletions(-) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index d78765ad84..625e04a9b0 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -967,49 +967,6 @@ libxlDomainAutoCoreDump(libxlDriverPrivate *driver, return 0; } -int -libxlDomainSetVcpuAffinities(libxlDriverPrivate *driver, virDomainObj *vm) -{ - g_autoptr(libxlDriverConfig) cfg = libxlDriverConfigGet(driver); - virDomainVcpuDef *vcpu; - libxl_bitmap map; - virBitmap *cpumask = NULL; - size_t i; - int ret = -1; - - libxl_bitmap_init(&map); - - for (i = 0; i < virDomainDefGetVcpus(vm->def); ++i) { - vcpu = virDomainDefGetVcpu(vm->def, i); - - if (!vcpu->online) - continue; - - if (!(cpumask = vcpu->cpumask)) - cpumask = vm->def->cpumask; - - if (!cpumask) - continue; - - if (virBitmapToData(cpumask, &map.map, (int *)&map.size) < 0) - goto cleanup; - - if (libxl_set_vcpuaffinity(cfg->ctx, vm->def->id, i, &map, NULL) != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to pin vcpu '%zu' with libxenlight"), i); - goto cleanup; - } - - libxl_bitmap_dispose(&map); /* Also returns to freshly-init'd state */ - } - - ret = 0; - - cleanup: - libxl_bitmap_dispose(&map); - return ret; -} - static int libxlDomainFreeMem(libxl_ctx *ctx, libxl_domain_config *d_config) { @@ -1460,9 +1417,6 @@ libxlDomainStart(libxlDriverPrivate *driver, goto destroy_dom; } - if (libxlDomainSetVcpuAffinities(driver, vm) < 0) - goto destroy_dom; - if (!start_paused) { libxlDomainUnpauseWrapper(cfg->ctx, domid); virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_BOOTED); diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h index cbe7ba19ba..928ee8f5cd 100644 --- a/src/libxl/libxl_domain.h +++ b/src/libxl/libxl_domain.h @@ -124,10 +124,6 @@ int libxlDomainAutoCoreDump(libxlDriverPrivate *driver, virDomainObj *vm); -int -libxlDomainSetVcpuAffinities(libxlDriverPrivate *driver, - virDomainObj *vm); - int libxlDomainStartNew(libxlDriverPrivate *driver, virDomainObj *vm,
Since Xen 4.5 libxl allows to set affinities during domain creation. This enables Xen to allocate the domain memory on NUMA systems close to the specified pcpus. Libvirt can now handle <domain/cputune/vcpupin> in domU.xml correctly. Without this change, Xen will create the domU and assign NUMA memory and vcpu affinities on its own. Later libvirt will adjust the affinity, which may move the vcpus away from the assigned NUMA node. Signed-off-by: Olaf Hering <olaf@aepfle.de> --- src/libxl/libxl_conf.c | 53 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 3ccb00ec35..3a969c38cd 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -283,6 +283,56 @@ libxlMakeChrdevStr(virDomainChrDef *def, char **buf) return 0; } +static int +libxlDomainSetVcpuAffinities(virDomainDef *def, + libxl_ctx *ctx, + libxl_domain_build_info *b_info) +{ + libxl_bitmap *vcpu_affinity_array; + unsigned int vcpuid; + unsigned int vcpu_idx = 0; + virDomainVcpuDef *vcpu; + bool has_vcpu_pin = false; + + /* Get highest vcpuid with cpumask */ + for (vcpuid = 0; vcpuid < b_info->max_vcpus; vcpuid++) { + vcpu = virDomainDefGetVcpu(def, vcpuid); + if (!vcpu) + continue; + if (!vcpu->cpumask) + continue; + vcpu_idx = vcpuid; + has_vcpu_pin = true; + } + /* Nothing to do */ + if (!has_vcpu_pin) + return 0; + + /* Adjust index */ + vcpu_idx++; + + b_info->num_vcpu_hard_affinity = vcpu_idx; + /* Will be released by libxl_domain_config_dispose */ + b_info->vcpu_hard_affinity = calloc(vcpu_idx, sizeof(libxl_bitmap)); + vcpu_affinity_array = b_info->vcpu_hard_affinity; + + for (vcpuid = 0; vcpuid < vcpu_idx; vcpuid++) { + libxl_bitmap *map = &vcpu_affinity_array[vcpuid]; + libxl_bitmap_init(map); + /* libxl owns the bitmap */ + if (libxl_cpu_bitmap_alloc(ctx, map, 0)) + return -1; + vcpu = virDomainDefGetVcpu(def, vcpuid); + /* Apply the given mask, or allow unhandled vcpus to run anywhere */ + if (vcpu && vcpu->cpumask) + virBitmapToDataBuf(vcpu->cpumask, map->map, map->size); + else + libxl_bitmap_set_any(map); + } + libxl_defbool_set(&b_info->numa_placement, false); + return 0; +} + static int libxlMakeDomBuildInfo(virDomainDef *def, libxlDriverConfig *cfg, @@ -320,6 +370,9 @@ libxlMakeDomBuildInfo(virDomainDef *def, for (i = 0; i < virDomainDefGetVcpus(def); i++) libxl_bitmap_set((&b_info->avail_vcpus), i); + if (libxlDomainSetVcpuAffinities(def, ctx, b_info)) + return -1; + switch ((virDomainClockOffsetType) clock.offset) { case VIR_DOMAIN_CLOCK_OFFSET_VARIABLE: if (clock.data.variable.basis == VIR_DOMAIN_CLOCK_BASIS_LOCALTIME)
On 5/3/21 4:56 AM, Olaf Hering wrote:
Since Xen 4.5 libxl allows to set affinities during domain creation. This enables Xen to allocate the domain memory on NUMA systems close to the specified pcpus.
Libvirt can now handle <domain/cputune/vcpupin> in domU.xml correctly.
Without this change, Xen will create the domU and assign NUMA memory and vcpu affinities on its own. Later libvirt will adjust the affinity, which may move the vcpus away from the assigned NUMA node.
That's not nice. Thanks for looking into it and providing a fix!
Signed-off-by: Olaf Hering <olaf@aepfle.de> --- src/libxl/libxl_conf.c | 53 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 3ccb00ec35..3a969c38cd 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -283,6 +283,56 @@ libxlMakeChrdevStr(virDomainChrDef *def, char **buf) return 0; }
+static int +libxlDomainSetVcpuAffinities(virDomainDef *def, + libxl_ctx *ctx, + libxl_domain_build_info *b_info)
We should tweak the name of this function after moving it from libxl_domain.c. Dropping 'Domain' is probably enough, e.g. libxlSetVcpuAffinities.
+{ + libxl_bitmap *vcpu_affinity_array; + unsigned int vcpuid; + unsigned int vcpu_idx = 0;
libvirt typically uses size_t.
+ virDomainVcpuDef *vcpu; + bool has_vcpu_pin = false; + + /* Get highest vcpuid with cpumask */ + for (vcpuid = 0; vcpuid < b_info->max_vcpus; vcpuid++) { + vcpu = virDomainDefGetVcpu(def, vcpuid); + if (!vcpu) + continue; + if (!vcpu->cpumask) + continue; + vcpu_idx = vcpuid; + has_vcpu_pin = true; + } + /* Nothing to do */ + if (!has_vcpu_pin) + return 0; + + /* Adjust index */ + vcpu_idx++; + + b_info->num_vcpu_hard_affinity = vcpu_idx; + /* Will be released by libxl_domain_config_dispose */ + b_info->vcpu_hard_affinity = calloc(vcpu_idx, sizeof(libxl_bitmap));
Fails syntax-check. You'll need to use g_new0. Patch 3 looks good, but I'll wait to push it until this one is ready. BTW, any reason for splitting 3 and 4? It's nice for review, but unless I'm missing something I think they should be squashed together. Jim
+ vcpu_affinity_array = b_info->vcpu_hard_affinity; + + for (vcpuid = 0; vcpuid < vcpu_idx; vcpuid++) { + libxl_bitmap *map = &vcpu_affinity_array[vcpuid]; + libxl_bitmap_init(map); + /* libxl owns the bitmap */ + if (libxl_cpu_bitmap_alloc(ctx, map, 0)) + return -1; + vcpu = virDomainDefGetVcpu(def, vcpuid); + /* Apply the given mask, or allow unhandled vcpus to run anywhere */ + if (vcpu && vcpu->cpumask) + virBitmapToDataBuf(vcpu->cpumask, map->map, map->size); + else + libxl_bitmap_set_any(map); + } + libxl_defbool_set(&b_info->numa_placement, false); + return 0; +} + static int libxlMakeDomBuildInfo(virDomainDef *def, libxlDriverConfig *cfg, @@ -320,6 +370,9 @@ libxlMakeDomBuildInfo(virDomainDef *def, for (i = 0; i < virDomainDefGetVcpus(def); i++) libxl_bitmap_set((&b_info->avail_vcpus), i);
+ if (libxlDomainSetVcpuAffinities(def, ctx, b_info)) + return -1; + switch ((virDomainClockOffsetType) clock.offset) { case VIR_DOMAIN_CLOCK_OFFSET_VARIABLE: if (clock.data.variable.basis == VIR_DOMAIN_CLOCK_BASIS_LOCALTIME)
Am Tue, 4 May 2021 16:34:05 -0600 schrieb Jim Fehlig <jfehlig@suse.com>:
+static int +libxlDomainSetVcpuAffinities(virDomainDef *def, + libxl_ctx *ctx, + libxl_domain_build_info *b_info)
We should tweak the name of this function after moving it from libxl_domain.c. Dropping 'Domain' is probably enough, e.g. libxlSetVcpuAffinities.
Ok, can do that change.
+ unsigned int vcpu_idx = 0; libvirt typically uses size_t.
Other code uses unsigned int when dealing with vcpus AFAICS, but sure I can change it.
+ /* Will be released by libxl_domain_config_dispose */ + b_info->vcpu_hard_affinity = calloc(vcpu_idx, sizeof(libxl_bitmap)); Fails syntax-check. You'll need to use g_new0.
Then syntax-check must be fixed. I was expecting the worst when double checking with the glib docs, and was not disappointed. "uses its own allocator", "pair g_free with g_new*" and the like. Since libxl does not use glib, and since it does not provide its own allocation function for that memory area, I decided to pair libxl's plain free() with this plain malloc(). Hence the comment. Olaf
On 5/4/21 7:10 PM, Olaf Hering wrote:
Am Tue, 4 May 2021 16:34:05 -0600 schrieb Jim Fehlig <jfehlig@suse.com>:
+static int +libxlDomainSetVcpuAffinities(virDomainDef *def, + libxl_ctx *ctx, + libxl_domain_build_info *b_info)
We should tweak the name of this function after moving it from libxl_domain.c. Dropping 'Domain' is probably enough, e.g. libxlSetVcpuAffinities.
Ok, can do that change.
+ unsigned int vcpu_idx = 0; libvirt typically uses size_t.
Other code uses unsigned int when dealing with vcpus AFAICS, but sure I can change it.
+ /* Will be released by libxl_domain_config_dispose */ + b_info->vcpu_hard_affinity = calloc(vcpu_idx, sizeof(libxl_bitmap)); Fails syntax-check. You'll need to use g_new0.
Then syntax-check must be fixed.
You can easily fix it by adding the file to 'exclude_file_name_regexp--sc_prohibit_raw_allocation' in build-aux/syntax-check.mk, although it's a pretty big hammer :-).
I was expecting the worst when double checking with the glib docs, and was not disappointed. "uses its own allocator", "pair g_free with g_new*" and the like.
Since libxl does not use glib, and since it does not provide its own allocation function for that memory area, I decided to pair libxl's plain free() with this plain malloc(). Hence the comment.
Makes sense. As you can see in the exclude list, other areas of libvirt have encountered the issue. Jim
Am Tue, 4 May 2021 16:34:05 -0600 schrieb Jim Fehlig <jfehlig@suse.com>:
+ /* Will be released by libxl_domain_config_dispose */ + b_info->vcpu_hard_affinity = calloc(vcpu_idx, sizeof(libxl_bitmap)); Fails syntax-check. You'll need to use g_new0.
There are other areas where libxl expects the caller to provide a malloc buffer, which it free's later in its *_dispose functions. libxlMakeDiskList for example. This looks like an inconsistency in the API. Perhaps we assume glib folks will make no mistakes in the future to use a private allocator again. Or we provide a simple calloc/free wrapper in libxl_api_wrapper.h, which can be easily blacklisted in exclude_file_name_regexp--sc_prohibit_raw_allocation. Olaf
participants (2)
-
Jim Fehlig -
Olaf Hering