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

[konrad wilk <konrad.wilk@xxxxxxxxxx>]

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.

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