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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Jun 18, 2014 at 06:40:30PM +0200, Dario Faggioli wrote:
> On mer, 2014-06-18 at 16:09 +0100, Wei Liu wrote:
> > On Wed, Jun 18, 2014 at 04:28:25PM +0200, Dario Faggioli wrote:
> > [...]
> > > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > > index 484ad84..b22b41e 100644
> > > --- a/tools/libxl/libxl_dom.c
> > > +++ b/tools/libxl/libxl_dom.c
> > > @@ -250,7 +250,8 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
> > >       * whatever that turns out to be.
> > >       */
> > >      if (libxl_defbool_val(info->numa_placement)) {
> > > -        if (!libxl_bitmap_is_full(&info->cpumap)) {
> > > +        if (!libxl_bitmap_is_full(&info->cpumap) ||
> > > +            info->num_vcpu_hard_affinity) {
> > 
> > Not related to this patch, I'm wondering why you used
> > libxl_bitmap_is_full in the first place. (And I admit I missed this spot
> > when I wrote my patch, sorry)
> > 
> Why I used _is_full() in the first place instead of doing what? As said
> to Ian in another email of this thread, the default set in
> libxl__build_info_set_defaults() for info->cpumap is "mask full", so the
> idea there is to check whether a cpumap is being specified by the
> caller, or if the default value is still there.
> 
> I remember this being discussed quite thoroughly at the time, and it
> seemed the best option. Re-thinking about it right now, it probably
> would have been better to just leave it alone (I mean, not even allocate
> the bitmap!) in _set_defaults(), and check for it having been allocated
> by the caller here (with `if(cpumap.size)').
> 
> I'm not sure we can change it like that, without it being considered an
> API compatibility breakage.... If we can, I'm all for it (if you
> remember, was trying to do right that in v8).
> 

I think this is library internal details. As long as the application
sees the same behavior then we are fine.

In V8 setting the default value and honoring cpumap are both removed so
that's wrong. If you choose to not set the full map then check the size
later I think it's also OK. That implies making the default value from a
full bitmap to an empty bitmap, but it's all library internal
implementation. The application still sees the same behavior.

> > >              LOG(ERROR, "Can run NUMA placement only if no vcpu "
> > >                         "affinity is specified");
> > >              return ERROR_INVAL;
> > > @@ -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))
> > > +        LOG(WARN, "cpumap field of libxl_domain_build_info is 
> > > DEPRECATED. "
> > > +                  "Please, use the vcpu_hard_affinity array instead");
> > 
> > Shouldn't it be !libxl_bitmap_is_empty?
> > 
> Nope (see above).
> 

OK. If you don't use empty bitmap as default value. It is probably a bit
hard to WARN at this point because you cannot distinguish whether a
"full" bitmap is the "default value" or "user specified value". Probably
you can do it in libxl__domain_build_info_setdefault if you stick with
the full bitmap approach?

Wei.


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



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