Re: [PATCH 2/2] tools/libxl: Introduce LIBXL_CPUPOOL_POOLID_ANY

[George Dunlap <george.dunlap@citrix.com>]

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@citrix.com>
>>>>>>
>>>>> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
>>>>>
>>>>> 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.

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

 -George

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