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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-03-13 at 14:57 +0000, Konrad Rzeszutek Wilk wrote:
> On Wed, Mar 13, 2013 at 10:41:32AM +0000, Ian Campbell wrote:
> > On Mon, 2013-03-11 at 14:20 +0000, Konrad Rzeszutek Wilk wrote:
> > > The XENMEM_claim_pages operates per domain and its usage is suppose to
> > > be done system wide. As such this patch introduces a global
> > > configuration option 'claim_mode' that by default is disabled.
> 
> 
> Thanks for taking a look. I will update the patch right away. The one
> particular question I have is in regards to the xl.c and using
> libxl_claim_mode_from_string - as it will require adding in
> #include <libxl_types.h>.

I don't think it should, libxl_types.h is included by libxl.h. (it's
hidden halfway through, due to the ordering constraints of the
autogenerated header.

> Which I am not sure if that is OK as it brings another layer of
> 'API' code in the xl code?

You should be good to go with just libxl.h, no need for an explicit
libxl_types include.

> > > The problem is that when a guest is launched the process of allocating
> > > memory from the hypervisor is not atomic and for large guests can take a 
> > > while.
> > > During which time other guests (especially self-ballooning) can take
> > > balloon up / down making the free memory information available to the
> > > toolstack stale.  With the "claim_mode" we can "stake a claim" for
> > > exact number of pages we need to allocate the guest and we are guaranteed 
> > > that
> > > they will be there when we start allocating the guest.
> > 
> > Is it strictly "guaranteed"? AIUI there are still cases where the claim
> > may succeed but domain building will not succeed (32 bit PV guests being
> > the first to spring to mind). It is more correct to say it increases the
> > chances of success or something like that.
> 
> Let me reword it. It will guarantee that the memory is available. However,
> you pointed out to some of the edge cases (that are on the list of things
> that need to be implemented, one of them being superpages)- that I should
> enumerate in the document as right now for those cases (say superpages)
> the claim call is not even done.

That might be a good idea.

> > 
> > > +If the claim fails the guest creation process will also fail.
> > > +
> > > +Default: C<off>
> > > +
> > > +=over 4
> > > +
> > > +=item B<"off">
> > > +
> > > +No action is taken and there is no stake put for the memory.
> > 
> > "no stake put" is a bit clumsy.
> > 
> > "No claim is made. Domain creation will be attempted as normal and may
> > fail due to out of memory errors"  ? Not great either.
> > 
> > > +=item B<"on">
> > > +
> > > +Normal memory and freeable pool of ephereal pages (tmem) is used when
> > 
> > Ephereal again?
> > 
> > > +calculating whether there is enough memory free to launch a guest.
> > > +
> > > +=back
> > > +
> > >  =back
> > >  
> > >  =head1 SEE ALSO
> > > diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf
> > > index 28ab796..2b64f7e 100644
> > > --- a/tools/examples/xl.conf
> > > +++ b/tools/examples/xl.conf
> > > @@ -20,3 +20,8 @@
> > >  # if disabled the old behaviour will be used, and hotplug scripts will be
> > >  # launched by udev.
> > >  #run_hotplug_scripts=1
> > > +#
> > > +# stake a claim of memory when launching a guest. This guarantees 
> > > immediate
> > > +# feedback whether the guest can be launched due to memory exhaustion
> > > +# (which can take a long time to find out if launching huge guests).
> > 
> > These mentions of long time and huge guests would be good in the docs
> > too I think.
> 
> As in other docs beside the man page for xl.conf?

I meant xl.conf, which isn't really explicit about this particular
aspect of the failure mode.

> > 
> > > @@ -102,6 +103,10 @@ static void parse_global_config(const char 
> > > *configfile,
> > >      }
> > >      if (!xlu_cfg_get_string (config, "blkdev_start", &buf, 0))
> > >          blkdev_start = strdup(buf);
> > > +
> > > +    if (!xlu_cfg_get_string(config, "claim_mode", &buf, 0))
> > > +        libxl_parse_claim_mode(buf, &global_claim_mode);
> > 
> > This goes away if you don't invent your own type but FYI the IDL
> > provides you with a libxl_claim_mode_from_string() function
> > automatically.
> 
> It means I have to add #include <libxl_types.h> in this file.

Nope ;-)

> And I was not sure whether that was OK or whether this code 'xl.c'
> should only have '#include <libxl.h>'

Ian.


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