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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.
> 
> 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.

> There are two options:
>  "on" - Use the default memory and also include the ephereal tmem pool in this

                                                     ephemeral ?

Or maybe "ethereal" ? 

>       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?

>  "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?

> 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.

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))" ?

>  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
        
?

> +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.

> 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.

> @@ -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.

> +
>      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)

>      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