|
[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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |