Re: [Xen-devel] [PATCH v9 6/9] libxl/xl: deprecate the build_info->cpumap field

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-06-18 at 18:26 +0200, Dario Faggioli wrote:
> On mer, 2014-06-18 at 16:53 +0100, Ian Campbell wrote:
> > On Wed, 2014-06-18 at 16:28 +0200, Dario Faggioli wrote:
> 
> > > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> > > index c087cbc..af48622 100644
> > > --- a/docs/man/xl.cfg.pod.5
> > > +++ b/docs/man/xl.cfg.pod.5
> > > @@ -143,11 +143,11 @@ Combining this with "all" is also possible, meaning 
> > > "all,^nodes:1"
> > >  results in all the vcpus of the guest running on all the cpus on the
> > >  host, except for the cpus belonging to the host NUMA node 1.
> > >  
> > > -=item ["2", "3"] (or [2, 3])
> > > +=item ["2", "3-8,^5"]
> > >  
> > > -To ask for specific vcpu mapping. That means (in this example), vcpu #0
> > > -of the guest will run on cpu #2 of the host and vcpu #1 of the guest will
> > > -run on cpu #3 of the host.
> > > +To ask for specific vcpu mapping. That means (in this example), vcpu 0
> > > +of the guest will run on cpu 2 of the host and vcpu 1 of the guest will
> > > +run on cpus 3,4,6,7,8 of the host.
> > 
> > Why is deprecating a field in the libxl API changing the xl
> > configuration file syntax?
> > 
> Because what happens as a consequence of, in xl, filling the array
> instead of just setting b_info->cpumap, is that we get to use the same
> parsing function (vcpupin_parse()) for all the elements of the array
> itself, in both the following cases:
> 
>  cpus="1-4,7"
> 
> (which sort of becomes equivalent to ["1-4,7", "1-4,7"])
> 
> and:
> 
>  cpus=["3", "4"]
> 
> Up to the previous patch, the latter case was handled asking the
> elements of the list to be integers. Here we start using vcpupin_parse()
> on them, and that is more powerful, and allows the elements to be
> ranges.
> 
> Anyway, I'll see if I can split this patch in two, as you suggest below,
> to make things more evident.

Thanks.

> 
> > > @@ -261,6 +262,13 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
> > >              return rc;
> > >      }
> > >      libxl_domain_set_nodeaffinity(ctx, domid, &info->nodemap);
> > > +    /*
> > > +     * info->cpumap is DEPRECATED, but we still want old applications
> > > +     * that may be using it to continue working.
> > > +     */
> > > +    if (!libxl_bitmap_is_full(&info->cpumap))
> > 
> > The caller is expected to initialise this unused field to a non-default
> > state? That doesn't sound right. Did you mean !is_empty?
> > 
> Nope. The default for this is to be full, so what I'm checking is really
> that it stayed default. See libxl__domain_build_info_setdefault():
> 
>     ...
>     if (!b_info->cpumap.size) {
>         if (libxl_cpu_bitmap_alloc(CTX, &b_info->cpumap, 0))
>             return ERROR_FAIL;
>         libxl_bitmap_set_any(&b_info->cpumap);
>     }
>     ...
> 
> Can I change this? If I can, I think the best would be to remove the
> allocation from libxl__domain_build_info_setdefault(), so that all the
> checks could become something like `if(cpumap.size)'.

I agree.

> But does stop allocating the bitmap qualifies as an incompatible API
> change?

I don't think so, do you think it might for some reason?

>  If yes, I think we're stuck with checking whether or not it's
> full, to see if the user is doing something with it or not.
> 
> > TBH I think you'd be better off just silently ignoring cpumap if the new
> > thing is set.
> > 
> This, I can try.
> 
> > Or maybe converting the cpumap into the new array so the rest of the
> > libxl internals only needs to deal with one.
> > 
> Converting it to the new array where? AFAICT, this is the only place
> where cpumap is used, so I don't think it's worth converting it.

e.g. in setdefaults:
   if (cpumap.size && arraything == NULL)
       allocate an array and fill it from cpumap

but if it is only used once then I guess it doesn't matter.

Ian.


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