From mboxrd@z Thu Jan 1 00:00:00 1970 From: konrad wilk Subject: Re: [PATCH 2/6] xl: Implement XENMEM_claim_pages support via 'claim_mode' global config Date: Mon, 15 Apr 2013 19:20:19 -0400 Message-ID: <516C8B33.1030404@oracle.com> References: <1365623998-13710-1-git-send-email-konrad.wilk@oracle.com> <1365623998-13710-3-git-send-email-konrad.wilk@oracle.com> <20840.16881.907625.772933@mariner.uk.xensource.com> <20840.19597.331764.59016@mariner.uk.xensource.com> <1366018494.4963.27.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1366018494.4963.27.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: George Dunlap , Dan Magenheimer , "xen-devel@lists.xensource.com" , Ian Jackson List-Id: xen-devel@lists.xenproject.org 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. >