[libvirt] [PATCH v5 3/4] libxl: vnuma support
Jim Fehlig
jfehlig at suse.com
Thu Oct 26 23:40:37 UTC 2017
On 10/12/2017 01:31 PM, Wim Ten Have wrote:
> From: Wim ten Have <wim.ten.have at oracle.com>
>
> This patch generates a NUMA distance-aware libxl description from the
> information extracted from a NUMA distance-aware libvirt XML file.
>
> By default, if no NUMA node distance information is supplied in the
> libvirt XML file, this patch uses the distances 10 for local and 20
> for remote nodes/sockets."
Spurious " at the end of above sentence.
> Signed-off-by: Wim ten Have <wim.ten.have at oracle.com>
> ---
> Changes on v1:
> - Fix function calling (coding) standards.
> - Use virReportError(...) under all failing circumstances.
> - Reduce redundant (format->parse) code sorting bitmaps.
> - Avoid non GNU shorthand notations, difficult code practice.
> Changes on v2:
> - Have autonomous defaults applied from virDomainNumaGetNodeDistance.
> - Automatically make Xen driver simulate vnuma pnodes on non NUMA h/w.
> - Compute 'memory unit=' making it the sum of all node memory.
> Changes on v4:
> - Escape virDomainNumaGetNodeCount() if effective.
> ---
> src/libxl/libxl_conf.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++
> src/libxl/libxl_driver.c | 3 +-
> 2 files changed, 123 insertions(+), 1 deletion(-)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 34233a955..adac6c19c 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -374,6 +374,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
> def->features[VIR_DOMAIN_FEATURE_APIC] ==
> VIR_TRISTATE_SWITCH_ON);
> libxl_defbool_set(&b_info->u.hvm.acpi,
> + virDomainNumaGetNodeCount(def->numa) > 0 || > def->features[VIR_DOMAIN_FEATURE_ACPI] ==
> VIR_TRISTATE_SWITCH_ON);
This doesn't seem right. At a minimum we should ensure
def->features[VIR_DOMAIN_FEATURE_ACPI] is set to ON when a vnuma configuration
is defined. I think libxlDomainDefPostParse is a better place to do that.
>
> @@ -618,6 +619,121 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
> return 0;
> }
>
> +#ifdef LIBXL_HAVE_VNUMA
> +static int
> +libxlMakeVnumaList(virDomainDefPtr def,
> + libxl_ctx *ctx,
> + libxl_domain_config *d_config)
> +{
> + int ret = -1;
> + size_t i, j;
> + size_t nr_nodes;
> + size_t num_vnuma;
> + bool simulate = false;
> + virBitmapPtr bitmap = NULL;
> + virDomainNumaPtr numa = def->numa;
> + libxl_domain_build_info *b_info = &d_config->b_info;
> + libxl_physinfo physinfo;
> + libxl_vnode_info *vnuma_nodes = NULL;
> +
> + if (!numa)
> + return 0;
> +
> + num_vnuma = virDomainNumaGetNodeCount(numa);
> + if (!num_vnuma)
> + return 0;
> +
> + libxl_physinfo_init(&physinfo);
> + if (libxl_get_physinfo(ctx, &physinfo) < 0) {
> + libxl_physinfo_dispose(&physinfo);
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("libxl_get_physinfo_info failed"));
> + return -1;
> + }
> + nr_nodes = physinfo.nr_nodes;
> + libxl_physinfo_dispose(&physinfo);
> +
> + if (num_vnuma > nr_nodes) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Number of configured numa cells %zu exceeds the physical available nodes %zu, guest simulates numa"),
> + num_vnuma, nr_nodes);
It seems this should be VIR_WARN instead of virReportError, afterall an error is
not returned.
> + simulate = true;
> + }
> +
> + /*
> + * allocate the vnuma_nodes for assignment under b_info.
> + */
> + if (VIR_ALLOC_N(vnuma_nodes, num_vnuma) < 0)
> + return -1;
> +
> + /*
> + * parse the vnuma vnodes data.
> + */
> + for (i = 0; i < num_vnuma; i++) {
> + int cpu;
> + libxl_bitmap vcpu_bitmap;
> + libxl_vnode_info *p = &vnuma_nodes[i];
> +
> + libxl_vnode_info_init(p);
> +
> + /* pnode */
> + p->pnode = simulate ? 0 : i;
So any time the number of vnuma nodes exceeds the number of physical nodes, all
vnuma nodes are confined to physical node 0? Does xl behave this way too?
> +
> + /* memory size */
> + p->memkb = virDomainNumaGetNodeMemorySize(numa, i);
> +
> + /* vcpus */
> + bitmap = virDomainNumaGetNodeCpumask(numa, i);
> + if (bitmap == NULL) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("vnuma sibling %zu missing vcpus set"), i);
> + goto cleanup;
> + }
> +
> + if ((cpu = virBitmapNextSetBit(bitmap, -1)) < 0)
> + goto cleanup;
> +
> + libxl_bitmap_init(&vcpu_bitmap);
> + if (libxl_cpu_bitmap_alloc(ctx, &vcpu_bitmap, b_info->max_vcpus)) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + do {
> + libxl_bitmap_set(&vcpu_bitmap, cpu);
> + } while ((cpu = virBitmapNextSetBit(bitmap, cpu)) >= 0);
> +
> + libxl_bitmap_copy_alloc(ctx, &p->vcpus, &vcpu_bitmap);
> + libxl_bitmap_dispose(&vcpu_bitmap);
> +
> + /* vdistances */
> + if (VIR_ALLOC_N(p->distances, num_vnuma) < 0)
> + goto cleanup;
> + p->num_distances = num_vnuma;
> +
> + for (j = 0; j < num_vnuma; j++)
> + p->distances[j] = virDomainNumaGetNodeDistance(numa, i, j);
> + }
> +
> + b_info->vnuma_nodes = vnuma_nodes;
> + b_info->num_vnuma_nodes = num_vnuma;
> +
> + ret = 0;
> +
> + cleanup:
> + if (ret) {
> + for (i = 0; i < num_vnuma; i++) {
> + libxl_vnode_info *p = &vnuma_nodes[i];
> +
> + VIR_FREE(p->distances);
> + }
> + VIR_FREE(vnuma_nodes);
> + }
> +
> + return ret;
> +}
> +#endif
> +
> static int
> libxlDiskSetDiscard(libxl_device_disk *x_disk, int discard)
> {
> @@ -2209,6 +2325,11 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
> if (libxlMakeDomBuildInfo(def, ctx, caps, d_config) < 0)
> return -1;
>
> +#ifdef LIBXL_HAVE_VNUMA
> + if (libxlMakeVnumaList(def, ctx, d_config) < 0)
> + return -1;
> +#endif
> +
> if (libxlMakeDiskList(def, d_config) < 0)
> return -1;
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 8483d6ecf..656f4a82d 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -2788,7 +2788,8 @@ libxlDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag
> virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
>
> if (flags & VIR_DOMAIN_DEFINE_VALIDATE)
> - parse_flags |= VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA;
> + parse_flags |= (VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA |
> + VIR_DOMAIN_DEF_PARSE_ABI_UPDATE);
Why is this change needed? In its current form, the patch doesn't touch any code
in the domain def post parse callback.
Regards,
Jim
More information about the libvir-list
mailing list