[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



> -----Original Message-----
> From: Paul Durrant <pdurrant@xxxxxxxxxx>
> Sent: 28 November 2019 12:51
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: George Dunlap <george.dunlap@xxxxxxxxxx>; Durrant, Paul
> <pdurrant@xxxxxxxxxx>; Ian Jackson <ian.jackson@xxxxxxxxxxxxx>; Wei Liu
> <wl@xxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Julien
> Grall <julien@xxxxxxx>; Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>;
> Stefano Stabellini <sstabellini@xxxxxxxxxx>; Anthony PERARD
> <anthony.perard@xxxxxxxxxx>; Marek Marczykowski-Górecki
> <marmarek@xxxxxxxxxxxxxxxxxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Subject: [PATCH-for-4.13 v3] Rationalize max_grant_frames and
> max_maptrack_frames handling
> 
> 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>
> 
> 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          | 24 ++++++++++++++--
>  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          | 46 ++++++++++++++++++++++++++++---
>  xen/include/public/domctl.h       | 10 ++++---
>  xen/include/xen/grant_table.h     |  8 +++---
>  13 files changed, 100 insertions(+), 38 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'}),
> 
>      ("device_model_version", libxl_device_model_version),
>      ("device_model_stubdomain", libxl_defbool),
> diff --git a/tools/libxl/libxlu_cfg.c b/tools/libxl/libxlu_cfg.c
> index 72815d25dd..09d5c78a46 100644
> --- a/tools/libxl/libxlu_cfg.c
> +++ b/tools/libxl/libxlu_cfg.c
> @@ -268,8 +268,9 @@ int xlu_cfg_replace_string(const XLU_Config *cfg,
> const char *n,
>      return 0;
>  }
> 
> -int xlu_cfg_get_long(const XLU_Config *cfg, const char *n,
> -                     long *value_r, int dont_warn) {
> +int xlu_cfg_get_bounded_long(const XLU_Config *cfg, const char *n,
> +                             long min, long max, long *value_r,
> +                             int dont_warn) {
>      long l;
>      XLU_ConfigSetting *set;
>      int e;
> @@ -303,10 +304,29 @@ int xlu_cfg_get_long(const XLU_Config *cfg, const
> char *n,
>                      cfg->config_source, set->lineno, n);
>          return EINVAL;
>      }
> +    if (l < min)

...and, of course, there's missing braces here and below. Don't know why the 
compiler didn't complain... it’s pretty new. Anyway I'll send v4 shortly.

  Paul

> +        if (!dont_warn)
> +            fprintf(cfg->report,
> +                    "%s:%d: warning: value `%ld' is smaller than minimum
> bound '%ld'\n",
> +                    cfg->config_source, set->lineno, l, min);
> +        return EINVAL;
> +    if (l > max)
> +        if (!dont_warn)
> +            fprintf(cfg->report,
> +                    "%s:%d: warning: value `%ld' is greater than maximum
> bound '%ld'\n",
> +                    cfg->config_source, set->lineno, l, max);
> +        return EINVAL;
> +
>      *value_r= l;
>      return 0;
>  }
> 
> +int xlu_cfg_get_long(const XLU_Config *cfg, const char *n,
> +                     long *value_r, int dont_warn) {
> +    return xlu_cfg_get_bounded_long(cfg, n, LONG_MIN, LONG_MAX, value_r,
> +                                    dont_warn);
> +}
> +
>  int xlu_cfg_get_defbool(const XLU_Config *cfg, const char *n,
> libxl_defbool *b,
>                       int dont_warn)
>  {
> diff --git a/tools/libxl/libxlutil.h b/tools/libxl/libxlutil.h
> index 057cc25cb2..92e35c5462 100644
> --- a/tools/libxl/libxlutil.h
> +++ b/tools/libxl/libxlutil.h
> @@ -63,6 +63,8 @@ int xlu_cfg_replace_string(const XLU_Config *cfg, const
> char *n,
>                             char **value_r, int dont_warn);
>  int xlu_cfg_get_long(const XLU_Config*, const char *n, long *value_r,
>                       int dont_warn);
> +int xlu_cfg_get_bounded_long(const XLU_Config*, const char *n, long min,
> +                             long max, long *value_r, int dont_warn);
>  int xlu_cfg_get_defbool(const XLU_Config*, const char *n, libxl_defbool
> *b,
>                       int dont_warn);
> 
> diff --git a/tools/python/xen/lowlevel/xc/xc.c
> b/tools/python/xen/lowlevel/xc/xc.c
> index 44d3606141..a751e85910 100644
> --- a/tools/python/xen/lowlevel/xc/xc.c
> +++ b/tools/python/xen/lowlevel/xc/xc.c
> @@ -127,8 +127,8 @@ static PyObject *pyxc_domain_create(XcObject *self,
>          },
>          .max_vcpus = 1,
>          .max_evtchn_port = -1, /* No limit. */
> -        .max_grant_frames = 32,
> -        .max_maptrack_frames = 1024,
> +        .max_grant_frames = -1,
> +        .max_maptrack_frames = -1,
>      };
> 
>      static char *kwd_list[] = { "domid", "ssidref", "handle", "flags",
> diff --git a/tools/xl/xl.c b/tools/xl/xl.c
> index ddd29b3f1b..921c64f5ed 100644
> --- a/tools/xl/xl.c
> +++ b/tools/xl/xl.c
> @@ -23,6 +23,7 @@
>  #include <ctype.h>
>  #include <inttypes.h>
>  #include <regex.h>
> +#include <limits.h>
> 
>  #include <libxl.h>
>  #include <libxl_utils.h>
> @@ -96,7 +97,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) {
> @@ -197,16 +197,11 @@ static void parse_global_config(const char
> *configfile,
>      xlu_cfg_replace_string (config, "colo.default.proxyscript",
>          &default_colo_proxy_script, 0);
> 
> -    if (!xlu_cfg_get_long (config, "max_grant_frames", &l, 0))
> +    if (!xlu_cfg_get_bounded_long (config, "max_grant_frames", 0,
> INT_MAX,
> +                                   &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))
> +    if (!xlu_cfg_get_bounded_long (config, "max_maptrack_frames", 0,
> +                                   INT_MAX, &l, 0))
>          max_maptrack_frames = l;
> 
>      libxl_cpu_bitmap_alloc(ctx, &global_vm_affinity_mask, 0);
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 112f8ee026..555991dae3 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1411,13 +1411,16 @@ void parse_config_data(const char *config_source,
>          !xlu_cfg_get_string (config, "cpus_soft", &buf, 0))
>          parse_vcpu_affinity(b_info, cpus, buf, num_cpus, false);
> 
> -    if (!xlu_cfg_get_long (config, "max_grant_frames", &l, 0))
> +    if (!xlu_cfg_get_bounded_long (config, "max_grant_frames", 0,
> INT_MAX,
> +                                   &l, 0))
>          b_info->max_grant_frames = l;
>      else
>          b_info->max_grant_frames = max_grant_frames;
> -    if (!xlu_cfg_get_long (config, "max_maptrack_frames", &l, 0))
> +
> +    if (!xlu_cfg_get_bounded_long (config, "max_maptrack_frames", 0,
> +                                   INT_MAX, &l, 0))
>          b_info->max_maptrack_frames = l;
> -    else if (max_maptrack_frames != -1)
> +    else
>          b_info->max_maptrack_frames = max_maptrack_frames;
> 
>      libxl_defbool_set(&b_info->claim_mode, claim_mode);
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 51d32106b7..3c899cd4a0 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -789,7 +789,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>          .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
>          .max_evtchn_port = -1,
>          .max_grant_frames = gnttab_dom0_frames(),
> -        .max_maptrack_frames = opt_max_maptrack_frames,
> +        .max_maptrack_frames = -1,
>      };
>      int rc;
> 
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 00ee87bde5..7d27f36053 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -697,8 +697,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>      struct xen_domctl_createdomain dom0_cfg = {
>          .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity :
> 0,
>          .max_evtchn_port = -1,
> -        .max_grant_frames = opt_max_grant_frames,
> -        .max_maptrack_frames = opt_max_maptrack_frames,
> +        .max_grant_frames = -1,
> +        .max_maptrack_frames = -1,
>      };
> 
>      /* Critical region without IDT or TSS.  Any fault is deadly! */
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index b34d520f6d..f5053a6ee8 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -84,11 +84,43 @@ struct grant_table {
>      struct grant_table_arch arch;
>  };
> 
> +static int __init parse_gnttab_limit(const char *param, const char *arg,
> +                                     unsigned int *valp)
> +{
> +    const char *e;
> +    unsigned long val;
> +
> +    val = simple_strtoul(arg, &e, 0);
> +    if ( *e )
> +        return -EINVAL;
> +
> +    if ( val >= 0 && val <= INT_MAX )
> +        *valp = val;
> +    else
> +        printk("%s: value '%s' is out of range; using value '%u'\n",
> +               param, arg, *valp);
> +
> +    return 0;
> +}
> +
>  unsigned int __read_mostly opt_max_grant_frames = 64;
> -integer_runtime_param("gnttab_max_frames", opt_max_grant_frames);
> +
> +static int __init parse_gnttab_max_frames(const char *arg)
> +{
> +    return parse_gnttab_limit("gnttab_max_frames", arg,
> +                              &opt_max_grant_frames);
> +}
> +custom_runtime_param("gnttab_max_frames", parse_gnttab_max_frames);
> 
>  unsigned int __read_mostly opt_max_maptrack_frames = 1024;
> -integer_runtime_param("gnttab_max_maptrack_frames",
> opt_max_maptrack_frames);
> +
> +static int __init parse_gnttab_max_maptrack_frames(const char *arg)
> +{
> +    return parse_gnttab_limit("gnttab_max_maptrack_frames", arg,
> +                              &opt_max_maptrack_frames);
> +}
> +custom_runtime_param("gnttab_max_maptrack_frames",
> +                     parse_gnttab_max_maptrack_frames);
> 
>  #ifndef GNTTAB_MAX_VERSION
>  #define GNTTAB_MAX_VERSION 2
> @@ -1837,12 +1869,18 @@ active_alloc_failed:
>      return -ENOMEM;
>  }
> 
> -int grant_table_init(struct domain *d, unsigned int max_grant_frames,
> -                     unsigned int max_maptrack_frames)
> +int grant_table_init(struct domain *d, int max_grant_frames,
> +                     int max_maptrack_frames)
>  {
>      struct grant_table *gt;
>      int ret = -ENOMEM;
> 
> +    /* Default to maximum value if no value was specified */
> +    if ( max_grant_frames < 0 )
> +        max_grant_frames = opt_max_grant_frames;
> +    if ( max_maptrack_frames < 0 )
> +        max_maptrack_frames = opt_max_maptrack_frames;
> +
>      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..e313da499f 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -82,13 +82,15 @@ 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 in the hypervisor".
>       */
>      uint32_t max_vcpus;
>      uint32_t max_evtchn_port;
> -    uint32_t max_grant_frames;
> -    uint32_t max_maptrack_frames;
> +    int32_t max_grant_frames;
> +    int32_t max_maptrack_frames;
> 
>      struct xen_arch_domainconfig arch;
>  };
> diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
> index 6f9345d9ef..34886bb6f8 100644
> --- a/xen/include/xen/grant_table.h
> +++ b/xen/include/xen/grant_table.h
> @@ -36,8 +36,8 @@ extern unsigned int opt_max_grant_frames;
>  extern unsigned int opt_max_maptrack_frames;
> 
>  /* Create/destroy per-domain grant table context. */
> -int grant_table_init(struct domain *d, unsigned int max_grant_frames,
> -                     unsigned int max_maptrack_frames);
> +int grant_table_init(struct domain *d, int max_grant_frames,
> +                     int max_maptrack_frames);
>  void grant_table_destroy(
>      struct domain *d);
>  void grant_table_init_vcpu(struct vcpu *v);
> @@ -68,8 +68,8 @@ int gnttab_get_status_frame(struct domain *d, unsigned
> long idx,
>  #define opt_max_maptrack_frames 0
> 
>  static inline int grant_table_init(struct domain *d,
> -                                   unsigned int max_grant_frames,
> -                                   unsigned int max_maptrack_frames)
> +                                   int max_grant_frames,
> +                                   int max_maptrack_frames)
>  {
>      return 0;
>  }
> --
> 2.20.1

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.