|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 3/6] xl: Implement XENMEM_claim_pages support via 'claim_mode' global config
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>.
Which means that we have now:
xl.c -> uses libxl.h
-> libxl -> uses libxl_types.
But will now be:
xl.c -> uses libxl.h and libxl_types
-> libxl > uses libxl_types.
Which I am not sure if that is OK as it brings another layer of
'API' code in the xl code?
> >
> > 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.
>
> > There are two options:
> > "on" - Use the default memory and also include the ephereal tmem pool in
> > this
>
> ephemeral ?
>
> Or maybe "ethereal" ?
It is "ephemeral/freeable"
>
> > calculation. This can affect temporarily negatively 'tmem' enabled
> > guests
> > but provides more freeable memory.
>
> The last sentence here doesn't parse for me. "This can have a temporary
> negative effect on 'tmem'..." ? What are those temporary negative
> effects?
Let me update that with a paragraph or two.
>
> > "off" - (default) No claim attempt is made.
>
> Is there a middle ground between off and on which stakes a claim but
> doesn't negatively effect tmem guests?
There was in the earlier version (v11), with a 'normal' case. But
Tim suggested tossing it out so out it went.
>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > ---
> > docs/man/xl.conf.pod.5 | 25 +++++++++++++++++++++++++
> > tools/examples/xl.conf | 5 +++++
> > tools/libxl/libxl.c | 5 +++++
> > tools/libxl/libxl.h | 2 ++
> > tools/libxl/libxl_dom.c | 3 ++-
> > tools/libxl/libxl_types.idl | 7 ++++++-
> > tools/libxl/xl.c | 5 +++++
> > tools/libxl/xl.h | 1 +
> > tools/libxl/xl_cmdimpl.c | 5 +++++
> > 9 files changed, 56 insertions(+), 2 deletions(-)
> >
> > diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5
> > index 82c6b20..fded0ab 100644
> > --- a/docs/man/xl.conf.pod.5
> > +++ b/docs/man/xl.conf.pod.5
> > @@ -98,6 +98,31 @@ Configures the name of the first block device to be used
> > for temporary
> > block device allocations by the toolstack.
> > The default choice is "xvda".
> >
> > +=item B<claim_mode="off|on">
>
> For consistency with other option this should be
> =item B<claim_mode=BOOLEAN>
> (which may require code changes)
>
> > +If enabled when a guest is created it will provide an guarantee whether the
> > +guest can be launched or not due to memory exhaustion.
>
> "guarantee" again. But more importantly this is missing the "when". Even
> without this option you get an eventual guarantee (i.e. after the guest
> actually starts). I think you want to say something about a quick
> upfront check or something.
OK.
>
> I would also start with "If this option is enabled then when a guest is
> created..." since it took me a couple of attempts to understand the
> current wording.
>
> > This is needed when
> > +using tmem type guests
>
> A reference to how to enable (or tell if it is enabled) this might be
> useful? Perhaps "(i.e. those with XXXX=FOO in their domain
> configuration, see xl.cfg(5))" ?
OK.
>
> > that can balloon up and down (self-balloon) extremely
> > +quickly and the list of free memory information is stale the moment it is
> > +displayed. When this mode is used there will be initially a claim set for
> > the
> > +amount of pages needed which will be fulfilled as the guest is created.
>
> "there will be initially a claim set..." -> "an initial claim will be
> set...".
>
> The last half (which will be fulfilled) doesn't read right to me either.
>
> When this mode is used an initial claim will be set covering the
> number of pages which will be required as the guest is created
>
> ?
Wow. I am impressed by how badly I wrote that.
>
> > +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?
>
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index 5b080ed..e417851 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -94,6 +94,11 @@ libxl_tsc_mode = Enumeration("tsc_mode", [
> > (3, "native_paravirt"),
> > ])
> >
> > +libxl_claim_mode = Enumeration("claim_mode", [
> > + (0, "off"),
> > + (1, "on"),
> > + ]);
>
> Eeew, we have a perfectly serviceable bool (and even defbool if you
> prefer) type. Please use one or the other instead of making up a third.
Sure.
>
> > @@ -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.
And I was not sure whether that was OK or whether this code 'xl.c'
should only have '#include <libxl.h>'
>
> > +
> > xlu_cfg_destroy(config);
> > }
> >
> > diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
> > index be6f38b..b1d434e 100644
> > --- a/tools/libxl/xl.h
> > +++ b/tools/libxl/xl.h
> > @@ -145,6 +145,7 @@ int xl_child_pid(xlchildnum); /* returns 0 if child
> > struct is not in use */
> > extern int autoballoon;
> > extern int run_hotplug_scripts;
> > extern int dryrun_only;
> > +extern unsigned int global_claim_mode;
> > extern char *lockfile;
> > extern char *default_vifscript;
> > extern char *default_bridge;
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index a98705e..27298ea 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -757,6 +757,11 @@ static void parse_config_data(const char
> > *config_source,
> > if (!xlu_cfg_get_long (config, "maxmem", &l, 0))
> > b_info->max_memkb = l * 1024;
> >
> > + if (global_claim_mode)
> > + b_info->claim_mode = (libxl_claim_mode)global_claim_mode;
> > + else
> > + b_info->claim_mode = 0;
>
> This is equivalent to
> b_info->claim_mode = (libxl_claim_mode)global_claim_mode;
> and AFAICT the cast is also redundant (and/or the type of the global is
> wrong)
Yup. Will collapse it in one.
>
> > if (xlu_cfg_get_string (config, "on_poweroff", &buf, 0))
> > buf = "destroy";
> > if (!parse_action_on_shutdown(buf, &d_config->on_poweroff)) {
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |