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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, May 15, 2014 at 02:59:13AM +0200, Dario Faggioli wrote:
> 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>
> 

Thanks.

> 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?
> 

Correct. I found out it's not necessary to touch them.

> 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?
> 

That's more or less a personal taste...

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

The paradigm of using libxl_type is to always call dispose. 

Wei.

> 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)
> 



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