[libvirt] [RFC PATCH v1 2/4] libxl: vnuma support

[Jim Fehlig]

On 06/12/2017 12:54 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 21 for
> remote nodes/sockets."
>
> Signed-off-by: Wim ten Have <wim.ten.have at oracle.com>
> ---
>  src/libxl/libxl_conf.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 128 insertions(+)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 04d9dd1..3e1a759 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -617,6 +617,129 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>      return 0;
>  }
>
> +#ifdef LIBXL_HAVE_VNUMA
> +static int
> +libxlMakeVnumaList(
> +    virDomainDefPtr def,
> +    libxl_ctx *ctx,
> +    libxl_domain_config *d_config)

The usual pattern is

static int
libxlMakeVnumaList(virDomainDefPtr def,
                    libxl_ctx *ctx,
                    libxl_domain_config *d_config)

> +{
> +    int ret = -1;
> +    char *vcpus = NULL;
> +    size_t i, j;
> +    size_t nr_nodes;
> +    size_t num_vnuma;
> +    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);
> +        VIR_WARN("libxl_get_physinfo failed");

The function, and hence domain startup, is going to fail, so we'll need proper 
error reporting here.

> +        goto cleanup;
> +    }
> +    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"),
> +                       num_vnuma, nr_nodes);
> +        goto cleanup;
> +    }
> +
> +    /*
> +     * allocate the vnuma_nodes for assignment under b_info.
> +     */
> +    if (VIR_ALLOC_N(vnuma_nodes, num_vnuma) < 0)
> +        goto cleanup;
> +
> +    /*
> +     * parse the vnuma vnodes data.
> +     */
> +    for (i = 0; i < num_vnuma; i++) {
> +        int cpu;
> +        virBitmapPtr bitmap = NULL;
> +        libxl_bitmap vcpu_bitmap;
> +        libxl_vnode_info *p = &vnuma_nodes[i];
> +
> +        libxl_vnode_info_init(p);
> +
> +        /* pnode */
> +        p->pnode = i;
> +
> +        /* memory size */
> +        p->memkb = virDomainNumaGetNodeMemorySize(numa, i);
> +
> +        /* vcpus */
> +        vcpus = virBitmapFormat(virDomainNumaGetNodeCpumask(numa, i));
> +        if (vcpus == NULL) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("vnuma sibling %zu missing vcpus set"), i);
> +            goto cleanup;
> +        }
> +
> +        if (virBitmapParse(vcpus, &bitmap, VIR_DOMAIN_CPUMASK_LEN) < 0)
> +            goto cleanup;

Why the need to format and parse the bitmap returned by 
virtDomainNumaGetNodeCpumask? Can it be used directly where 'bitmap' is used below?

> +
> +        if ((cpu = virBitmapNextSetBit(bitmap, -1)) < 0) {
> +            VIR_FREE(bitmap);
> +            goto cleanup;
> +        }
> +
> +        libxl_bitmap_init(&vcpu_bitmap);
> +        if (libxl_cpu_bitmap_alloc(ctx, &vcpu_bitmap, b_info->max_vcpus)) {
> +            virReportOOMError();

It looks like 'bitmap' will leak on this error path.

> +            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);
> +
> +        VIR_FREE(bitmap);
> +        VIR_FREE(vcpus);

'vcpus' is freed in cleanup.

> +
> +        /* vdistances */
> +        if (VIR_ALLOC_N(p->distances, num_vnuma) < 0)
> +            goto cleanup;
> +        p->num_distances = num_vnuma;
> +
> +        /*
> +         * Apply the configured distance value. If not
> +         * available set 10 for local or 21 for remote nodes.
> +         */
> +        for (j = 0; j < num_vnuma; j++)
> +            p->distances[j] = virDomainNumaGetNodeDistance(numa, i, j) ?:
> +                                (i == j ? 10 : 21);

Is the '?:' shorthand a GNU extension? I could only find one other use of it in 
the codebase, in src/vz/vz_sdk.c. I like the concise form, but also fear it is 
difficult to read.

Regards,
Jim

> +    }
> +
> +    b_info->vnuma_nodes = vnuma_nodes;
> +    b_info->num_vnuma_nodes = num_vnuma;
> +
> +    ret = 0;
> +
> + cleanup:
> +    if (ret)
> +        VIR_FREE(vnuma_nodes);
> +    VIR_FREE(vcpus);
> +
> +    return ret;
> +}
> +#endif
> +
>  static int
>  libxlDiskSetDiscard(libxl_device_disk *x_disk, int discard)
>  {
> @@ -2207,6 +2330,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;
>
>