5:56 a.m.
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(-)
5:56 a.m.
New subject: [PATCH v1 1/4] libxl: remove obsolete VIR_LIBXL_EVENT_CONST
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(a)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,
5:26 p.m.
New subject: [PATCH v1 1/4] libxl: remove obsolete VIR_LIBXL_EVENT_CONST
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(a)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(a)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,
5:56 a.m.
New subject: [PATCH v1 2/4] libxl: remove obsolete check for result of xc_get_max_cpus
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(a)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);
5:27 p.m.
New subject: [PATCH v1 2/4] libxl: remove obsolete check for result of xc_get_max_cpus
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(a)aepfle.de>
---
src/libxl/libxl_driver.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
Reviewed-by: Jim Fehlig <jfehlig(a)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);
5:56 a.m.
New subject: [PATCH v1 3/4] libxl: remove obsolete libxlDomainSetVcpuAffinities function
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(a)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,
5:56 a.m.
New subject: [PATCH v1 4/4] libxl: set vcpu affinity during domain creation
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(a)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)
5:34 p.m.
New subject: [PATCH v1 4/4] libxl: set vcpu affinity during domain creation
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(a)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)
8:10 p.m.
New subject: [PATCH v1 4/4] libxl: set vcpu affinity during domain creation
Am Tue, 4 May 2021 16:34:05 -0600
schrieb Jim Fehlig <jfehlig(a)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
8:32 p.m.
New subject: [PATCH v1 4/4] libxl: set vcpu affinity during domain creation
On 5/4/21 7:10 PM, Olaf Hering wrote:
Am Tue, 4 May 2021 16:34:05 -0600
schrieb Jim Fehlig <jfehlig(a)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
10:23 p.m.
New subject: [PATCH v1 4/4] libxl: set vcpu affinity during domain creation
Am Tue, 4 May 2021 16:34:05 -0600
schrieb Jim Fehlig <jfehlig(a)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
1400
days inactive
1402
days old
10 comments
2 participants
participants (2)
-
Jim Fehlig -
Olaf Hering