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

["Durrant, Paul" <pdurrant@amazon.com>]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
> George Dunlap
> Sent: 26 November 2019 17:18
> To: xen-devel@lists.xenproject.org
> Cc: Juergen Gross <jgross@suse.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Wei Liu
> <wl@xen.org>; Paul Durrant <paul@xen.org>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; George Dunlap <george.dunlap@citrix.com>; Marek
> Marczykowski-Górecki <marmarek@invisiblethingslab.com>; Jan Beulich
> <jbeulich@suse.com>; Ian Jackson <ian.jackson@citrix.com>
> Subject: [Xen-devel] [PATCH for-4.13 2/2] Rationalize max_grant_frames and
> max_maptrack_frames handling
> 
> 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, the 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 '0' values for
> max_grant_frames and max_maptrack_frames as instructions to use the
> system-wide default.  Have all the above toolstacks default to passing
> 0 unless a different value is explicitly given.
> 
> This restores the old behavior, 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.
> 
> (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; these will not be addressed here.)
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> Release justification: This is an observed regression (albeit one that
> has spanned several releases now).
> 
> Compile-tested only.
> 
> NB this patch could be applied without the whitespace fixes (perhaps
> with some fix-ups); it's just easier since my editor strips trailing
> whitespace out automatically.
> 
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Paul Durrant <paul@xen.org>
> CC: Julien Grall <julien@xen.org>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Juergen Gross <jgross@suse.com>
> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
>  tools/libxl/libxl.h               |  4 ++--
>  tools/python/xen/lowlevel/xc/xc.c |  2 --
>  tools/xl/xl.c                     | 12 ++----------
>  xen/common/grant_table.c          |  7 +++++++
>  xen/include/public/domctl.h       |  6 ++++--
>  5 files changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 49b56fa1a3..1648d337e7 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 0
> +#define LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT 0
> 
>  /*
>   * LIBXL_HAVE_BUILDINFO_* indicates that libxl_domain_build_info has
> diff --git a/tools/python/xen/lowlevel/xc/xc.c
> b/tools/python/xen/lowlevel/xc/xc.c
> index 6d2afd5695..0f861872ce 100644
> --- a/tools/python/xen/lowlevel/xc/xc.c
> +++ b/tools/python/xen/lowlevel/xc/xc.c
> @@ -127,8 +127,6 @@ static PyObject *pyxc_domain_create(XcObject *self,
>          },
>          .max_vcpus = 1,
>          .max_evtchn_port = -1, /* No limit. */
> -        .max_grant_frames = 32,
> -        .max_maptrack_frames = 1024,
>      };
> 
>      static char *kwd_list[] = { "domid", "ssidref", "handle", "flags",
> diff --git a/tools/xl/xl.c b/tools/xl/xl.c
> index ddd29b3f1b..b6e220184d 100644
> --- a/tools/xl/xl.c
> +++ b/tools/xl/xl.c
> @@ -51,8 +51,8 @@ libxl_bitmap global_pv_affinity_mask;
>  enum output_format default_output_format = OUTPUT_FORMAT_JSON;
>  int claim_mode = 1;
>  bool progress_use_cr = 0;
> -int max_grant_frames = -1;
> -int max_maptrack_frames = -1;
> +int max_grant_frames = 0;
> +int max_maptrack_frames = 0;
> 
>  xentoollog_level minmsglevel = minmsglevel_default;
> 
> @@ -96,7 +96,6 @@ static void parse_global_config(const char *configfile,
>      XLU_Config *config;
>      int e;
>      const char *buf;
> -    libxl_physinfo physinfo;
> 
>      config = xlu_cfg_init(stderr, configfile);
>      if (!config) {
> @@ -199,13 +198,6 @@ static void parse_global_config(const char
> *configfile,
> 
>      if (!xlu_cfg_get_long (config, "max_grant_frames", &l, 0))
>          max_grant_frames = l;
> -    else {
> -        libxl_physinfo_init(&physinfo);
> -        max_grant_frames = (libxl_get_physinfo(ctx, &physinfo) != 0 ||
> -                            !(physinfo.max_possible_mfn >> 32))
> -                           ? 32 : 64;
> -        libxl_physinfo_dispose(&physinfo);
> -    }
>      if (!xlu_cfg_get_long (config, "max_maptrack_frames", &l, 0))
>          max_maptrack_frames = l;
> 
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index b34d520f6d..cd24029e33 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1843,6 +1843,13 @@ int grant_table_init(struct domain *d, unsigned int
> max_grant_frames,
>      struct grant_table *gt;
>      int ret = -ENOMEM;
> 
> +    /* Default to maximum values if no lower ones are specified */
> +    if ( !max_grant_frames )
> +        max_grant_frames = opt_max_grant_frames;
> +
> +    if ( !max_maptrack_frames )
> +        max_maptrack_frames = opt_max_maptrack_frames;
> +

This means should also be able to drop the field setting in dom0_cfg in __start_xen() too :-)

  Paul

>      if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES ||
>           max_grant_frames > opt_max_grant_frames ||
>           max_maptrack_frames > opt_max_maptrack_frames )
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 9f2cfd602c..27d04f67aa 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -82,8 +82,10 @@ struct xen_domctl_createdomain {
>      uint32_t iommu_opts;
> 
>      /*
> -     * Various domain limits, which impact the quantity of resources
> (global
> -     * mapping space, xenheap, etc) a guest may consume.
> +     * Various domain limits, which impact the quantity of resources
> +     * (global mapping space, xenheap, etc) a guest may consume.  For
> +     * max_grant_frames and max_maptrack_frames, "0" means "use the
> +     * default maximum value".
>       */
>      uint32_t max_vcpus;
>      uint32_t max_evtchn_port;
> --
> 2.24.0
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel