Re: [PATCH 2/6] xl: Implement XENMEM_claim_pages support via 'claim_mode' global config

[konrad wilk <konrad.wilk@oracle.com>]

On 4/15/2013 5:34 AM, Ian Campbell wrote:
> On Fri, 2013-04-12 at 19:03 +0100, Ian Jackson wrote:
>> Ian Jackson writes ("Re: [PATCH 2/6] xl: Implement XENMEM_claim_pages support via 'claim_mode' global config"):
>>> Sorry, I just spotted this.  I think the libxl_defbool_setdefault
>>> shouldn't be there.  The defbool should be initialised to "default",
>>> which can be done by setting it to 0, as per:
>>>
>>>    * To allow users of the library to naively select all defaults this
>>>    * state is represented as 0. False is < 0 and True is > 0.
>>>
>>> in libxl.h.  And since it's a variable of static duration the C
>>> implementation will initialise it to 0.  So just deleting the
>>> setdefault is right.
>>>
>>> The result is that the default is set in libxl, only.
>> Konrad points out that without this, xl can't easily find out whether
>> the claim mode is enabled or not.
> Does it need to know? Is the presence of any non-zero value for a claim
> enough indication for each function which might care to make a local
> decision? At least nothing in this particular patch appears to care what
> libxl's default is.

The issue was that if you try to do libxl_get_defbool and the bool is a 
default - it will
blow up with an assert.
> Is this setting supposed to be global (at either the host or specific
> toolstack level) or is it supposed to be per-domain?
Global
>
> If it is at the toolstack level then shouldn't it be set in the
> libxl_ctx instead of libxl_domain_build_info? If it is per-domain then
> shouldn't there be the possibility of an override in the domain config?
>
> If its supposed to be host wide then that seems to argue for a
> requirement for a libxl specific configuration file, so that all
> toolstacks (at least those which use libxl) can be configured. The xapi
> guys were asking me about the possibility of such settings last week in
> the context of host wide driver domain policy...
>
> Anyway, back to the original point of this mail, assuming my questions
> above haven't made that moot:
>
>> Options are:
>>
>> 1. Leave it as it is, default set in libxl but overridden by
>>     separate setting in xl (perhaps we should add a comment).
>>     Tolerable.
>>
>> 2. Move the default setting out of libxl entirely, so callers
>>     must pass 0 or 1.  (I don't approve of this; we might want
>>     to change the behaviour of naive toolstacks in the future.)
> Agree that this is best avoided.
>
>> 3. Provide a new interface to libxl which allows the claim
>>     default to be retrieved.  Palaver.
> Could incorporate it into whichever of the libxl_*info interfaces seems
> most appropriate. libxl_physinfo contains a lot of the associated free
> memory values, so although claim mode isn't really "phys" perhaps that's
> the best place for it?
>
>> 4. Have xl operations which need to know the default claim mode
>>     set up a dummy domain creation config, ask libxl to set the
>>     defaults in it, and then read out the value.  Very ugly.
> Very.
>
>> Of these I prefer 1.  Opinions ?  Whatever we do needs to be in 4.3.
> 1 is probably the best of the bunch but I note that in that case the
> implementation in xl should be just:
>
>     xl.c:
> 	int claim_mode; /* = 0 */
>
>     xl.c:parse_global_config():
> 	if (!xlu_cfg_get_long (config, "claim_mode", &l, 0))
>              claim_mode = l;
>
>     xl_cmdimpl.c:parse_config_data():
>          libxl_defbool_set_default(&b_info->claim_mode, claim_mode)
>
> i.e. xl's glboal claim mode setting is just a bool, not a defbool.

Yes. That will work too. This was how the earlier versions had it.

I will post a new version when I back in office tomorrow.

Thanks!
>
> Ian.
>