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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

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.

> > @@ -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)'.

But does stop allocating the bitmap qualifies as an incompatible API
change? 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.
Ignoring if the array is specified is a better solution, I think

> > +        LOG(WARN, "cpumap field of libxl_domain_build_info is DEPRECATED. "
> > +                  "Please, use the vcpu_hard_affinity array instead");
> >      libxl_set_vcpuaffinity_all(ctx, domid, info->max_vcpus,
> >                                 &info->cpumap, NULL);
> >  
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index 05978d7..0b3e4e9 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -297,7 +297,12 @@ libxl_domain_sched_params = 
> > Struct("domain_sched_params",[
> >  libxl_domain_build_info = Struct("domain_build_info",[
> >      ("max_vcpus",       integer),
> >      ("avail_vcpus",     libxl_bitmap),
> > -    ("cpumap",          libxl_bitmap),
> > +    ("cpumap",          libxl_bitmap), # DEPRECATED!
> > +    # The cpumap field above has been deprecated by the introduction of the
> > +    # vcpu_hard_affinity array. It is not removed and it is still 
> > honoured, for
> > +    # API stability and backward compatibility reasons, but should not be 
> > used
> > +    # any longer. The vcpu_hard_affinity array is what should be used 
> > instead,
> > +    # to set the hard affinity of the various vCPUs.
> 
> This comment needs to talk about the precedence between the two fields
> in the event that both are present.
> 
Right. I'll add a word about it.

> WRT the structure of the series: All of the libxl deprecation stuff here
> could be squashed into the previous patch which added the new field.
> That would make more sense since otherwise you have a middle state where
> both fields are present and valid and it is ill defined what is what.
> 
> All the xl stuff could then come next as a "move away from deprecated
> interface" patch.
> 
> As it is each patch seems to do half of each thing. I'm not entirely
> sure what the intermediate state is supposed to be.
> 
> > @@ -840,42 +839,30 @@ static void parse_config_data(const char 
> > *config_source,
> >                  fprintf(stderr, "Unable to allocate cpumap for vcpu %d\n", 
> > i);
> >                  exit(1);
> >              }
> > -            libxl_bitmap_set_any(&b_info->vcpu_hard_affinity[i]);
> > +            libxl_bitmap_set_none(&b_info->vcpu_hard_affinity[i]);
> 
> What do these sorts of changes have to do with the deprecation of
> another field?
> 
> It looks to me like the previous patch has just done something wrong and
> you are fixing it here for some reason.
> 
It's not like that, but I think I see what you mean. I'll try to
restructure the series so it does not gives this impression.

Thanks and 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