Re: [Xen-devel] [PATCH-for-4.13 v5] Rationalize max_grant_frames and max_maptrack_frames handling

[Wei Liu <wl@xxxxxxx>]

On Thu, Nov 28, 2019 at 04:52:24PM +0000, Paul Durrant wrote:
> From: George Dunlap <george.dunlap@xxxxxxxxxx>
> 
> Xen used to have single, system-wide limits for the number of grant
> frames and maptrack frames a guest was allowed to create. Increasing
> or decreasing this single limit on the Xen command-line would change
> the limit for all guests on the system.
> 
> Later, per-domain limits for these values was created. The system-wide
> limits became strict limits: domains could not be created with higher
> limits, but could be created with lower limits. However, that change
> also introduced a range of different "default" values into various
> places in the toolstack:
> 
> - The python libxc bindings hard-coded these values to 32 and 1024,
>   respectively
> - The libxl default values are 32 and 1024 respectively.
> - xl will use the libxl default for maptrack, but does its own default
>   calculation for grant frames: either 32 or 64, based on the max
>   possible mfn.
> 
> These defaults interact poorly with the hypervisor command-line limit:
> 
> - The hypervisor command-line limit cannot be used to raise the limit
>   for all guests anymore, as the default in the toolstack will
>   effectively override this.
> - If you use the hypervisor command-line limit to *reduce* the limit,
>   then the "default" values generated by the toolstack are too high,
>   and all guest creations will fail.
> 
> In other words, the toolstack defaults require any change to be
> effected by having the admin explicitly specify a new value in every
> guest.
> 
> In order to address this, have grant_table_init treat negative values
> for max_grant_frames and max_maptrack_frames as instructions to use the
> system-wide default, and have all the above toolstacks default to passing
> -1 unless a different value is explicitly configured.
> 
> This restores the old behavior in that changing the hypervisor command-line
> option can change the behavior for all guests, while retaining the ability
> to set per-guest values.  It also removes the bug that reducing the
> system-wide max will cause all domains without explicit limits to fail.
> 
> NOTE: - The Ocaml bindings require the caller to always specify a value,
>         and the code to start a xenstored stubdomain hard-codes these to 4
>       and 128 respectively; this behavour will not be modified.
> 
> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>
> ---
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Wei Liu <wl@xxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Julien Grall <julien@xxxxxxx>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> Cc: "Marek Marczykowski-Górecki" <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> Cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> 
> v5:
>  - Remove erroneous __init annotations
>  - Fail out of range command line values with ERANGE
>  - Make opt_max_maptrack_frames static
> 
> v4:
>  - Add missing braces in xlu_cfg_get_bounded_long()
> 
> v3:
>  - Make sure that specified values cannot be negative or overflow a
>    signed int
> 
> v2:
>  - re-worked George's original commit massage a little
>  - fixed the text in xl.conf.5.pod
>  - use -1 as the sentinel value for 'default' and < 0 for checking it
> ---
>  docs/man/xl.conf.5.pod            |  6 +++--
>  tools/libxl/libxl.h               |  4 +--
>  tools/libxl/libxl_types.idl       |  4 +--
>  tools/libxl/libxlu_cfg.c          | 26 ++++++++++++++++--
>  tools/libxl/libxlutil.h           |  2 ++
>  tools/python/xen/lowlevel/xc/xc.c |  4 +--
>  tools/xl/xl.c                     | 15 ++++-------
>  tools/xl/xl_parse.c               |  9 ++++---
>  xen/arch/arm/setup.c              |  2 +-
>  xen/arch/x86/setup.c              |  4 +--
>  xen/common/grant_table.c          | 45 +++++++++++++++++++++++++++----
>  xen/include/public/domctl.h       | 10 ++++---
>  xen/include/xen/grant_table.h     | 10 +++----
>  13 files changed, 100 insertions(+), 41 deletions(-)
> 
> diff --git a/docs/man/xl.conf.5.pod b/docs/man/xl.conf.5.pod
> index 962144e38e..207ab3e77a 100644
> --- a/docs/man/xl.conf.5.pod
> +++ b/docs/man/xl.conf.5.pod
> @@ -81,13 +81,15 @@ Default: C</var/lock/xl>
>  
>  Sets the default value for the C<max_grant_frames> domain config value.
>  
> -Default: C<32> on hosts up to 16TB of memory, C<64> on hosts larger than 16TB
> +Default: value of Xen command line B<gnttab_max_frames> parameter (or its
> +default value if unspecified).
>  
>  =item B<max_maptrack_frames=NUMBER>
>  
>  Sets the default value for the C<max_maptrack_frames> domain config value.
>  
> -Default: C<1024>
> +Default: value of Xen command line B<gnttab_max_maptrack_frames>
> +parameter (or its default value if unspecified).
>  
>  =item B<vif.default.script="PATH">
>  
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 49b56fa1a3..a2a5d321c5 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -364,8 +364,8 @@
>   */
>  #define LIBXL_HAVE_BUILDINFO_GRANT_LIMITS 1
>  
> -#define LIBXL_MAX_GRANT_FRAMES_DEFAULT 32
> -#define LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT 1024
> +#define LIBXL_MAX_GRANT_FRAMES_DEFAULT -1
> +#define LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT -1
>  
>  /*
>   * LIBXL_HAVE_BUILDINFO_* indicates that libxl_domain_build_info has
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 0546d7865a..63e29bb2fb 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -511,8 +511,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
>  
>      ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
>  
> -    ("max_grant_frames",    uint32, {'init_val': 
> 'LIBXL_MAX_GRANT_FRAMES_DEFAULT'}),
> -    ("max_maptrack_frames", uint32, {'init_val': 
> 'LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT'}),
> +    ("max_grant_frames",    integer, {'init_val': 
> 'LIBXL_MAX_GRANT_FRAMES_DEFAULT'}),
> +    ("max_maptrack_frames", integer, {'init_val': 
> 'LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT'}),

What if we use 0xffffffff to denote default instead? That wouldn't
require changing the type here.

The type change here makes me feel a bit uncomfortable, though in
practice it may not matter. I don't see anyone would specify a value
that would become negative when cast from uint32 to integer.

If the decision is to change the type, please provide a #define in
libxl.h.

Ian and Anthony, your opinion?

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel