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

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2764 bytes --]

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

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

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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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