Re: [Xen-devel] [PATCH V5 03/32] xl / libxl: push VCPU affinity pinning down to libxl

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On mar, 2014-05-13 at 22:53 +0100, Wei Liu wrote:
> This patch introduces a key value list called "vcpu_affinity" in libxl
> IDL to preserve VCPU to PCPU mapping. This is necessary for libxl to
> preserve all information to construct a domain.
> 
> Also define LIBXL_HAVE_AFFINITY_KEY_VALUE_LIST in libxl.h to mark the
> change in API.
> 
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
>
Reviewed-and-Tested-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>

One question: I've skimmed through the rest of the series, and it looks
like you are not doing anything particular with the other maps, already
existing in b_info (cpumap and nodemap) yet, is that right?

Also, a very minor comment on the code...

> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 661999c..b818815 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -263,6 +263,54 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>      libxl_domain_set_nodeaffinity(ctx, domid, &info->nodemap);
>      libxl_set_vcpuaffinity_all(ctx, domid, info->max_vcpus, &info->cpumap);
>  
> +    /* If we have vcpu affinity list, pin vcpu to pcpu. */
> +    if (d_config->b_info.vcpu_affinity) {
> +        int i;
> +        libxl_bitmap vcpu_cpumap;
> +        int *vcpu_to_pcpu, sz = sizeof(int) * d_config->b_info.max_vcpus;
> +
> +        vcpu_to_pcpu = libxl__zalloc(gc, sz);
> +        memset(vcpu_to_pcpu, -1, sz);
> +
> +        for (i = 0; i < d_config->b_info.max_vcpus; i++) {
> +            libxl_key_value_list kvl = d_config->b_info.vcpu_affinity;
> +            const char *key, *val;
> +            int k, v;
> +
> +            key = kvl[i * 2];
> +            if (!key)
> +                break;
> +            val = kvl[i * 2 + 1];
> +
> +            k = atoi(key);
> +            v = atoi(val);
> +            vcpu_to_pcpu[k] = v;
> +        }
> +
> +        rc = libxl_cpu_bitmap_alloc(ctx, &vcpu_cpumap, 0);
> +        if (rc) {
> +            libxl_bitmap_dispose(&vcpu_cpumap);
> +            return ERROR_FAIL;
> +        }
> +
Perhaps you can move this up, i.e., try the allocation first so that, if
it fails, you can bail before getting into decoding the key/value list?

Also, if libxl_cpu_bitmap_alloc() fails, do you really have to call
libxl_bitmap_dispose()? I thought not.

Anyway, as said, these both are minor issues. Just give them a thought
if you respin the series but, if not, the patch is fine as it is for me.

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel