Re: [Xen-devel] [PATCH 2/2] tools/libxl: Introduce LIBXL_CPUPOOL_POOLID_ANY

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Feb 09, 2017 at 11:35:16AM +0000, George Dunlap wrote:
> On 09/02/17 11:24, Wei Liu wrote:
> > On Thu, Feb 09, 2017 at 11:17:46AM +0000, George Dunlap wrote:
> >> On 09/02/17 10:35, Wei Liu wrote:
> >>> On Wed, Feb 08, 2017 at 04:17:58PM +0000, George Dunlap wrote:
> >>>> On 08/02/17 16:11, Dario Faggioli wrote:
> >>>>> On Wed, 2017-02-08 at 14:51 +0000, George Dunlap wrote:
> >>>>>> Callers to libxl_cpupool_create() can either request a specific pool
> >>>>>> id, or request that Xen do it for them.  But at the moment, the
> >>>>>> "automatic" selection is indicated by using a magic value, 0.  This
> >>>>>> is
> >>>>>> undesirable both because it doesn't obviously have meaning, but also
> >>>>>> because '0' is a valid cpupool (albeit one which at the moment can't
> >>>>>> be changed).
> >>>>>>
> >>>>>> Introduce a constant, LIBXL_CPUPOOL_POOLID_ANY, to indicate this
> >>>>>> instead.  Still accept '0' as meaning "ANY" for backwards
> >>>>>> compatibility.
> >>>>>>
> >>>>>> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> >>>>>>
> >>>>> Reviewed-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> >>>>>
> >>>>> With one remark.
> >>>>>
> >>>>>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> >>>>>> --- a/tools/libxl/libxl.h
> >>>>>> +++ b/tools/libxl/libxl.h
> >>>>>> @@ -2086,6 +2086,12 @@ int libxl_tmem_shared_auth(libxl_ctx *ctx,
> >>>>>> uint32_t domid, char* uuid,
> >>>>>>  int libxl_tmem_freeable(libxl_ctx *ctx);
> >>>>>>  
> >>>>>>  int libxl_get_freecpus(libxl_ctx *ctx, libxl_bitmap *cpumap);
> >>>>>> +
> >>>>>> +/* 
> >>>>>> + * Set poolid to LIBXL_CPUOOL_POOLID_ANY to have Xen choose a
> >>>>>> + * free poolid for you.
> >>>>>> + */
> >>>>>> +#define LIBXL_CPUPOOL_POOLID_ANY 0xFFFFFFFF
> >>>>>>
> >>>>> Do we want this to be here, or in libxl_types.idl.
> >>>>>
> >>>>> Asking because, AFAICT, it's the only one LIBXL_FOO_BAR defined like
> >>>>> this. I appreciate that there's few point in making this an enum, as it
> >>>>> is only one value, and will most likely remain so, but still, I thought
> >>>>> I'd at least bring this up.
> >>>>>
> >>>>> FWIW, my Reviewed-by stands both if it is kept as is, and if it is
> >>>>> moved to IDL.
> >>>>
> >>>> Well there's things like:
> >>>>
> >>>> #define LIBXL_PCI_FUNC_ALL (~0U)
> >>>>
> >>>> #define LIBXL_TIMER_MODE_DEFAULT -1
> >>>> #define LIBXL_MEMKB_DEFAULT ~0ULL
> >>>>
> >>>> #define LIBXL_RDM_MEM_BOUNDARY_MEMKB_DEFAULT (2048 * 1024)
> >>>>
> >>>> #define LIBXL_MS_VM_GENID_LEN 16
> >>>>
> >>>> #define LIBXL_SUSPEND_DEBUG 1
> >>>> #define LIBXL_SUSPEND_LIVE 2
> >>>>
> >>>> Many of which seem similar in some ways.  Enums I think are meant to be
> >>>> exhaustive (as in, contain all possible options), not be special cases.
> >>>>
> >>>> But I'm happy to defer to the tools maintainers.
> >>>>
> >>>
> >>> I don't really care if it is an enum or a macro.
> >>>
> >>> There is an issue that is  more subtle than where it lives or what form
> >>> it is in.
> >>>
> >>> You need to modify all the poolid fields in various structure to make it
> >>> as default.  Otherwise the whole json infrastructure would still use 0
> >>> as the default value.
> >>
> >> Hmm, at the moment there are only two structs that include poolid:
> >> cpupoolinfo, which is OUT-only (so the field should always be
> >> initialized) and domain_create_info, for which 0 is a much better
> >> default logically than "ANY".
> >>
> > 
> > I don't follow here. Isn't 0 already "ANY"?
> 
> No.  This is why I'm bothering to paint this bikeshed: In every context
> *except* "cpupool create", 0 means cpupool0 -- the one that was created
> at boot, which always contains dom0, and which cannot be destroyed.
> (You can only remove all but one of the cpus.)  If you remove a cpupool
> from poolid 0, you remove it from cpupool0, not "any" pool.  If you
> create a domain and ask to put it in poolid 0, it will be put in
> cpupool0, not "any" pool.  The only context in which "0" currently means
> "any" is when you're creating a cpupool.
> 

OK. This makes sense.

> >>> And maybe a LIBXL_HAVE macro is needed, too.
> >>
> >> I thought about that, but figured people could just do #ifdef
> >> LIBXL_CPUPOOL_POOLID_ANY.  It seemed a bit strange to define a whole new
> >> macro just to see if another macro existed.
> >>
> > 
> > I want to reserve the possibility to change that into an enum in the
> > future.
> 
> Yes, I had thought of that -- but like I said, I thought enums were
> meant mostly for things for which there was an exhaustive list.  In this
> case it's a "magic" value for a parameter which normally has a plain
> numerical meaning.
> 
> But I can add the #define if you wish.
> 

You don't need to do that.

Wei.

>  -George

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