[for-4.13,2/2] Rationalize max_grant_frames and max_maptrack_frames handling

Commit Message

George Dunlap Nov. 26, 2019, 5:17 p.m. UTC

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 <[e-mail]>
---
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 <[e-mail]>
CC: Wei Liu <[e-mail]>
CC: Andrew Cooper <[e-mail]>
CC: Jan Beulich <[e-mail]>
CC: Paul Durrant <[e-mail]>
CC: Julien Grall <[e-mail]>
CC: Konrad Rzeszutek Wilk <[e-mail]>
CC: Stefano Stabellini <[e-mail]>
CC: Juergen Gross <[e-mail]>
CC: Marek Marczykowski-Górecki <[e-mail]>
---
 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(-)

Comments

George Dunlap Nov. 26, 2019, 5:30 p.m. UTC | #1

On 11/26/19 5:17 PM, George Dunlap wrote:
> - 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.

[snip]

> @@ -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);
> -    }

Sorry, meant to add a patch to add this functionality back into the
hypervisor -- i.e., so that opt_max_grant_frames would be 32 on systems
with 32-bit mfns.

But this seems like a fairly strange calculation anyway; it's not clear
to me where it would have come from.

 -George

Paul Durrant Nov. 26, 2019, 5:32 p.m. UTC | #2

> -----Original Message-----
> From: Xen-devel <[e-mail]> On Behalf Of
> George Dunlap
> Sent: 26 November 2019 17:18
> To: [e-mail]
> Cc: Juergen Gross <[e-mail]>; Stefano Stabellini
> <[e-mail]>; Julien Grall <[e-mail]>; Wei Liu
> <[e-mail]>; Paul Durrant <[e-mail]>; Andrew Cooper
> <[e-mail]>; Konrad Rzeszutek Wilk
> <[e-mail]>; George Dunlap <[e-mail]>; Marek
> Marczykowski-Górecki <[e-mail]>; Jan Beulich
> <[e-mail]>; Ian Jackson <[e-mail]>
> 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 <[e-mail]>
> ---
> 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 <[e-mail]>
> CC: Wei Liu <[e-mail]>
> CC: Andrew Cooper <[e-mail]>
> CC: Jan Beulich <[e-mail]>
> CC: Paul Durrant <[e-mail]>
> CC: Julien Grall <[e-mail]>
> CC: Konrad Rzeszutek Wilk <[e-mail]>
> CC: Stefano Stabellini <[e-mail]>
> CC: Juergen Gross <[e-mail]>
> CC: Marek Marczykowski-Górecki <[e-mail]>
> ---
>  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
> [e-mail]
> https://lists.xenproject.org/mailman/listinfo/xen-devel

Ian Jackson Nov. 26, 2019, 5:36 p.m. UTC | #3

George Dunlap writes ("[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.

If I am not mistaken, this is an important change to have.

I have seen reports of users who ran out of grant/maptrack frames
because of updates to use multiring protocols etc.  The error messages
are not very good and the recommended workaround has been to increase
the default limit on the hypervisor command line.

It is important that we don't break that workaround!

Thanks,
Ian.

Jürgen Groß Nov. 27, 2019, 4:32 a.m. UTC | #4

On 26.11.19 18:17, George Dunlap wrote:
> 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 <[e-mail]>
> ---
> 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 <[e-mail]>
> CC: Wei Liu <[e-mail]>
> CC: Andrew Cooper <[e-mail]>
> CC: Jan Beulich <[e-mail]>
> CC: Paul Durrant <[e-mail]>
> CC: Julien Grall <[e-mail]>
> CC: Konrad Rzeszutek Wilk <[e-mail]>
> CC: Stefano Stabellini <[e-mail]>
> CC: Juergen Gross <[e-mail]>
> CC: Marek Marczykowski-Górecki <[e-mail]>
> ---
>   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

I'd rather use -1 for the "not specified" value. This allows to set e.g.
the maptrack frames to 0 for non-driver domains.


Juergen

Jürgen Groß Nov. 27, 2019, 4:34 a.m. UTC | #5

On 26.11.19 18:30, George Dunlap wrote:
> On 11/26/19 5:17 PM, George Dunlap wrote:
>> - 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.
> 
> [snip]
> 
>> @@ -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);
>> -    }
> 
> Sorry, meant to add a patch to add this functionality back into the
> hypervisor -- i.e., so that opt_max_grant_frames would be 32 on systems
> with 32-bit mfns.
> 
> But this seems like a fairly strange calculation anyway; it's not clear
> to me where it would have come from.
mfns above the 32-bit limit require to use grant v2. This in turn
doubles the grant frames needed for the same number of grants.


Juergen

Paul Durrant Nov. 27, 2019, 8:42 a.m. UTC | #6

> -----Original Message-----
> From: Xen-devel <[e-mail]> On Behalf Of Ian
> Jackson
> Sent: 26 November 2019 17:36
> To: George Dunlap <[e-mail]>
> Cc: Juergen Gross <[e-mail]>; Stefano Stabellini
> <[e-mail]>; Julien Grall <[e-mail]>; Wei Liu
> <[e-mail]>; Paul Durrant <[e-mail]>; Andrew Cooper
> <[e-mail]>; Konrad Rzeszutek Wilk
> <[e-mail]>; Marek Marczykowski-Górecki
> <[e-mail]>; Hans van Kranenburg <[e-mail]>;
> Jan Beulich <[e-mail]>; [e-mail]
> Subject: Re: [Xen-devel] [PATCH for-4.13 2/2] Rationalize max_grant_frames
> and max_maptrack_frames handling
> 
> George Dunlap writes ("[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.
> 
> If I am not mistaken, this is an important change to have.
> 

It is, and many thanks to George for picking this up.

> I have seen reports of users who ran out of grant/maptrack frames
> because of updates to use multiring protocols etc.  The error messages
> are not very good and the recommended workaround has been to increase
> the default limit on the hypervisor command line.
> 
> It is important that we don't break that workaround!

Alas it has apparently been broken for several releases now :-(

  Paul

> 
> Thanks,
> Ian.
> 
> _______________________________________________
> Xen-devel mailing list
> [e-mail]
> https://lists.xenproject.org/mailman/listinfo/xen-devel

Jan Beulich Nov. 27, 2019, 9:25 a.m. UTC | #7

On 27.11.2019 05:32, Jürgen Groß wrote:
> On 26.11.19 18:17, George Dunlap wrote:
>> --- 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
> 
> I'd rather use -1 for the "not specified" value. This allows to set e.g.
> the maptrack frames to 0 for non-driver domains.

Yes. But it in turn wouldn't allow taking 0 to mean "default" in the
hypervisor.

Jan

Jürgen Groß Nov. 27, 2019, 9:38 a.m. UTC | #8

On 27.11.19 10:25, Jan Beulich wrote:
> On 27.11.2019 05:32, Jürgen Groß wrote:
>> On 26.11.19 18:17, George Dunlap wrote:
>>> --- 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
>>
>> I'd rather use -1 for the "not specified" value. This allows to set e.g.
>> the maptrack frames to 0 for non-driver domains.
> 
> Yes. But it in turn wouldn't allow taking 0 to mean "default" in the
> hypervisor.

That is a logical consequence, yes.


Juergen

George Dunlap Nov. 27, 2019, 10:40 a.m. UTC | #9

On Wed, Nov 27, 2019 at 4:33 AM Jürgen Groß <[e-mail]> wrote:
>
> On 26.11.19 18:17, George Dunlap wrote:
> > 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 <[e-mail]>
> > ---
> > 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 <[e-mail]>
> > CC: Wei Liu <[e-mail]>
> > CC: Andrew Cooper <[e-mail]>
> > CC: Jan Beulich <[e-mail]>
> > CC: Paul Durrant <[e-mail]>
> > CC: Julien Grall <[e-mail]>
> > CC: Konrad Rzeszutek Wilk <[e-mail]>
> > CC: Stefano Stabellini <[e-mail]>
> > CC: Juergen Gross <[e-mail]>
> > CC: Marek Marczykowski-Górecki <[e-mail]>
> > ---
> >   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
>
> I'd rather use -1 for the "not specified" value. This allows to set e.g.
> the maptrack frames to 0 for non-driver domains.

I did wonder whether having 0 frames was something we might want.  I
can certainly change that.

 -George

Ian Jackson Nov. 27, 2019, 11:10 a.m. UTC | #10

Durrant, Paul writes ("RE: [Xen-devel] [PATCH for-4.13 2/2] Rationalize max_grant_frames and max_maptrack_frames handling"):
> > -----Original Message-----
> > From: Xen-devel <[e-mail]> On Behalf Of Ian
> > Jackson
> > I have seen reports of users who ran out of grant/maptrack frames
> > because of updates to use multiring protocols etc.  The error messages
> > are not very good and the recommended workaround has been to increase
> > the default limit on the hypervisor command line.
> > 
> > It is important that we don't break that workaround!
> 
> Alas it has apparently been broken for several releases now :-(

I guess at least in Debian (where I have seen this) we haven't
released with any affected versions yet...

Ian.

Paul Durrant Nov. 27, 2019, 11:13 a.m. UTC | #11

> -----Original Message-----
> From: Ian Jackson <[e-mail]>
> Sent: 27 November 2019 11:10
> To: Durrant, Paul <[e-mail]>
> Cc: Ian Jackson <[e-mail]>; George Dunlap
> <[e-mail]>; Juergen Gross <[e-mail]>; Stefano
> Stabellini <[e-mail]>; Julien Grall <[e-mail]>; Wei
> Liu <[e-mail]>; Paul Durrant <[e-mail]>; Andrew Cooper
> <[e-mail]>; Konrad Rzeszutek Wilk
> <[e-mail]>; Marek Marczykowski-Górecki
> <[e-mail]>; Hans van Kranenburg <[e-mail]>;
> Jan Beulich <[e-mail]>; [e-mail]
> Subject: RE: [Xen-devel] [PATCH for-4.13 2/2] Rationalize max_grant_frames
> and max_maptrack_frames handling
> 
> Durrant, Paul writes ("RE: [Xen-devel] [PATCH for-4.13 2/2] Rationalize
> max_grant_frames and max_maptrack_frames handling"):
> > > -----Original Message-----
> > > From: Xen-devel <[e-mail]> On Behalf Of
> Ian
> > > Jackson
> > > I have seen reports of users who ran out of grant/maptrack frames
> > > because of updates to use multiring protocols etc.  The error messages
> > > are not very good and the recommended workaround has been to increase
> > > the default limit on the hypervisor command line.
> > >
> > > It is important that we don't break that workaround!
> >
> > Alas it has apparently been broken for several releases now :-(
> 
> I guess at least in Debian (where I have seen this) we haven't
> released with any affected versions yet...

I believe the problem was introduce in 4.10, so I think it would be prudent to also back-port the final fix to stable trees from then on.

  Paul

> 
> Ian.

George Dunlap Nov. 27, 2019, 12:07 p.m. UTC | #12

On 11/27/19 4:34 AM, Jürgen Groß wrote:
> On 26.11.19 18:30, George Dunlap wrote:
>> On 11/26/19 5:17 PM, George Dunlap wrote:
>>> - 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.
>>
>> [snip]
>>
>>> @@ -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);
>>> -    }
>>
>> Sorry, meant to add a patch to add this functionality back into the
>> hypervisor -- i.e., so that opt_max_grant_frames would be 32 on systems
>> with 32-bit mfns.
>>
>> But this seems like a fairly strange calculation anyway; it's not clear
>> to me where it would have come from.
> mfns above the 32-bit limit require to use grant v2. This in turn
> doubles the grant frames needed for the same number of grants.

But is large mfns the *only* reason to use grant v2?  Aren't modern
guests going to use grant v2 regardless of the max mfn size?

 -George

Jürgen Groß Nov. 27, 2019, 12:15 p.m. UTC | #13

On 27.11.19 13:07, George Dunlap wrote:
> On 11/27/19 4:34 AM, Jürgen Groß wrote:
>> On 26.11.19 18:30, George Dunlap wrote:
>>> On 11/26/19 5:17 PM, George Dunlap wrote:
>>>> - 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.
>>>
>>> [snip]
>>>
>>>> @@ -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);
>>>> -    }
>>>
>>> Sorry, meant to add a patch to add this functionality back into the
>>> hypervisor -- i.e., so that opt_max_grant_frames would be 32 on systems
>>> with 32-bit mfns.
>>>
>>> But this seems like a fairly strange calculation anyway; it's not clear
>>> to me where it would have come from.
>> mfns above the 32-bit limit require to use grant v2. This in turn
>> doubles the grant frames needed for the same number of grants.
> 
> But is large mfns the *only* reason to use grant v2?  Aren't modern
> guests going to use grant v2 regardless of the max mfn size?

Large mfns leave the guest no choice. Linux kernel V2 support was
removed and I reintroduced it for being able to support large mfns in
guests.

Current Linux kernel will use V1 if the max mfn fits in 32 bits and V2
only if there can be memory above that boundary.


Juergen

Hans van Kranenburg Nov. 27, 2019, 10:32 p.m. UTC | #14

Hi all,

On 11/27/19 12:13 PM, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Ian Jackson <[e-mail]>
>> Sent: 27 November 2019 11:10
>> [...]
>> Subject: RE: [Xen-devel] [PATCH for-4.13 2/2] Rationalize max_grant_frames
>> and max_maptrack_frames handling
>>
>> Durrant, Paul writes ("RE: [Xen-devel] [PATCH for-4.13 2/2] Rationalize
>> max_grant_frames and max_maptrack_frames handling"):
>>>> -----Original Message-----
>>>> From: Xen-devel <[e-mail]> On Behalf Of
>> Ian
>>>> Jackson
>>>> I have seen reports of users who ran out of grant/maptrack frames
>>>> because of updates to use multiring protocols etc.  The error messages
>>>> are not very good and the recommended workaround has been to increase
>>>> the default limit on the hypervisor command line.
>>>>
>>>> It is important that we don't break that workaround!
>>>
>>> Alas it has apparently been broken for several releases now :-(
>>
>> I guess at least in Debian (where I have seen this) we haven't
>> released with any affected versions yet...
> 
> I believe the problem was introduce in 4.10, so I think it would be prudent to also back-port the final fix to stable trees from then on.

Yes, the max grant frame issue has historically always been a painful
experience for end users, and Xen 4.11 which we now have in the current
Debian stable has made it worse compared to previous versions indeed.

Changing the hypervisor command line worked in the past, and now that
value is overwritten again by a lower value in the toolstack, which
requires setting per-domU settings, or, what I did, just additionally
also setting max_grant_frames in /etc/xen/xl.conf to the same value as
the hypervisor command line.

This change is very welcome, even to 4.11-stable if possible, since it
will not break existing configuration of users.

If changing only the value of the hypervisor command line works again,
then old information that shows up when the users searches the web will
be useful again, which is good.

Hans

P.S. Now I'm curious to figure out what a maptrack frame is, didn't hear
about that one before.

Jürgen Groß Nov. 28, 2019, 5:57 a.m. UTC | #15

On 27.11.19 23:32, Hans van Kranenburg wrote:
> Hi all,
> 
> On 11/27/19 12:13 PM, Durrant, Paul wrote:
>>> -----Original Message-----
>>> From: Ian Jackson <[e-mail]>
>>> Sent: 27 November 2019 11:10
>>> [...]
>>> Subject: RE: [Xen-devel] [PATCH for-4.13 2/2] Rationalize max_grant_frames
>>> and max_maptrack_frames handling
>>>
>>> Durrant, Paul writes ("RE: [Xen-devel] [PATCH for-4.13 2/2] Rationalize
>>> max_grant_frames and max_maptrack_frames handling"):
>>>>> -----Original Message-----
>>>>> From: Xen-devel <[e-mail]> On Behalf Of
>>> Ian
>>>>> Jackson
>>>>> I have seen reports of users who ran out of grant/maptrack frames
>>>>> because of updates to use multiring protocols etc.  The error messages
>>>>> are not very good and the recommended workaround has been to increase
>>>>> the default limit on the hypervisor command line.
>>>>>
>>>>> It is important that we don't break that workaround!
>>>>
>>>> Alas it has apparently been broken for several releases now :-(
>>>
>>> I guess at least in Debian (where I have seen this) we haven't
>>> released with any affected versions yet...
>>
>> I believe the problem was introduce in 4.10, so I think it would be prudent to also back-port the final fix to stable trees from then on.
> 
> Yes, the max grant frame issue has historically always been a painful
> experience for end users, and Xen 4.11 which we now have in the current
> Debian stable has made it worse compared to previous versions indeed.
> 
> Changing the hypervisor command line worked in the past, and now that
> value is overwritten again by a lower value in the toolstack, which
> requires setting per-domU settings, or, what I did, just additionally
> also setting max_grant_frames in /etc/xen/xl.conf to the same value as
> the hypervisor command line.
> 
> This change is very welcome, even to 4.11-stable if possible, since it
> will not break existing configuration of users.
> 
> If changing only the value of the hypervisor command line works again,
> then old information that shows up when the users searches the web will
> be useful again, which is good.
> 
> Hans
> 
> P.S. Now I'm curious to figure out what a maptrack frame is, didn't hear
> about that one before.

The maptrack frames are used by the hypervisor for keeping track which
grants are mapped by a specific domain. So they are necessary for driver
domains (including dom0), and max_maptrack_frames limits how many
mappings of other domain's pages can be active simultaneously in a
domain.


Juergen

George Dunlap Nov. 28, 2019, 2:54 p.m. UTC | #16

On 11/27/19 10:32 PM, Hans van Kranenburg wrote:
> Hi all,
> 
> On 11/27/19 12:13 PM, Durrant, Paul wrote:
>>> -----Original Message-----
>>> From: Ian Jackson <[e-mail]>
>>> Sent: 27 November 2019 11:10
>>> [...]
>>> Subject: RE: [Xen-devel] [PATCH for-4.13 2/2] Rationalize max_grant_frames
>>> and max_maptrack_frames handling
>>>
>>> Durrant, Paul writes ("RE: [Xen-devel] [PATCH for-4.13 2/2] Rationalize
>>> max_grant_frames and max_maptrack_frames handling"):
>>>>> -----Original Message-----
>>>>> From: Xen-devel <[e-mail]> On Behalf Of
>>> Ian
>>>>> Jackson
>>>>> I have seen reports of users who ran out of grant/maptrack frames
>>>>> because of updates to use multiring protocols etc.  The error messages
>>>>> are not very good and the recommended workaround has been to increase
>>>>> the default limit on the hypervisor command line.
>>>>>
>>>>> It is important that we don't break that workaround!
>>>>
>>>> Alas it has apparently been broken for several releases now :-(
>>>
>>> I guess at least in Debian (where I have seen this) we haven't
>>> released with any affected versions yet...
>>
>> I believe the problem was introduce in 4.10, so I think it would be prudent to also back-port the final fix to stable trees from then on.
> 
> Yes, the max grant frame issue has historically always been a painful
> experience for end users, and Xen 4.11 which we now have in the current
> Debian stable has made it worse compared to previous versions indeed.

This rather suggests that the default value isn't very well chosen.
Ideally some investigation would be done to improve the default sizing;
end-users shouldn't have to know anything about grant table frames.

 -George

Hans van Kranenburg Nov. 28, 2019, 3:33 p.m. UTC | #17

On 11/28/19 3:54 PM, George Dunlap wrote:
> On 11/27/19 10:32 PM, Hans van Kranenburg wrote:
>> Hi all,
>>
>> On 11/27/19 12:13 PM, Durrant, Paul wrote:
>>>> -----Original Message-----
>>>> From: Ian Jackson <[e-mail]>
>>>> Sent: 27 November 2019 11:10
>>>> [...]
>>>> Subject: RE: [Xen-devel] [PATCH for-4.13 2/2] Rationalize max_grant_frames
>>>> and max_maptrack_frames handling
>>>>
>>>> Durrant, Paul writes ("RE: [Xen-devel] [PATCH for-4.13 2/2] Rationalize
>>>> max_grant_frames and max_maptrack_frames handling"):
>>>>>> -----Original Message-----
>>>>>> From: Xen-devel <[e-mail]> On Behalf Of
>>>> Ian
>>>>>> Jackson
>>>>>> I have seen reports of users who ran out of grant/maptrack frames
>>>>>> because of updates to use multiring protocols etc.  The error messages
>>>>>> are not very good and the recommended workaround has been to increase
>>>>>> the default limit on the hypervisor command line.
>>>>>>
>>>>>> It is important that we don't break that workaround!
>>>>>
>>>>> Alas it has apparently been broken for several releases now :-(
>>>>
>>>> I guess at least in Debian (where I have seen this) we haven't
>>>> released with any affected versions yet...
>>>
>>> I believe the problem was introduce in 4.10, so I think it would be prudent to also back-port the final fix to stable trees from then on.
>>
>> Yes, the max grant frame issue has historically always been a painful
>> experience for end users, and Xen 4.11 which we now have in the current
>> Debian stable has made it worse compared to previous versions indeed.
> 
> This rather suggests that the default value isn't very well chosen.
> Ideally some investigation would be done to improve the default sizing;
> end-users shouldn't have to know anything about grant table frames.

Most of the problems started happening a few years ago when using a
newer Linux that got all kinds of multiqueue block stuff for disk and
network enabled on top of an older Xen. (e.g. in Debian using the Linux
4.9 backports kernel on top of Xen 4.4 in Jessie).

The default for the hypervisor option has already been doubled from 32
to 64, which I think is sufficient. However, having the toolstack revert
it back to 32 again is not very helpful, but that's what this thread is
about to solve. :)

A while ago I did some testing:
   https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=880554#119

I haven't been able to cause nr_frames to go over 64 in any test myself,
and also have never seen values that high in production use. The above
debian bug also does not contain any other report from anyone with a
number above 64. There are reports of users setting it to 256 and then
not caring about it any more, but they didn't report the xen_diag output
back after that, so there's no real data.

Hans

Jürgen Groß Nov. 28, 2019, 3:43 p.m. UTC | #18

On 28.11.19 16:33, Hans van Kranenburg wrote:
> On 11/28/19 3:54 PM, George Dunlap wrote:
>> On 11/27/19 10:32 PM, Hans van Kranenburg wrote:
>>> Hi all,
>>>
>>> On 11/27/19 12:13 PM, Durrant, Paul wrote:
>>>>> -----Original Message-----
>>>>> From: Ian Jackson <[e-mail]>
>>>>> Sent: 27 November 2019 11:10
>>>>> [...]
>>>>> Subject: RE: [Xen-devel] [PATCH for-4.13 2/2] Rationalize max_grant_frames
>>>>> and max_maptrack_frames handling
>>>>>
>>>>> Durrant, Paul writes ("RE: [Xen-devel] [PATCH for-4.13 2/2] Rationalize
>>>>> max_grant_frames and max_maptrack_frames handling"):
>>>>>>> -----Original Message-----
>>>>>>> From: Xen-devel <[e-mail]> On Behalf Of
>>>>> Ian
>>>>>>> Jackson
>>>>>>> I have seen reports of users who ran out of grant/maptrack frames
>>>>>>> because of updates to use multiring protocols etc.  The error messages
>>>>>>> are not very good and the recommended workaround has been to increase
>>>>>>> the default limit on the hypervisor command line.
>>>>>>>
>>>>>>> It is important that we don't break that workaround!
>>>>>>
>>>>>> Alas it has apparently been broken for several releases now :-(
>>>>>
>>>>> I guess at least in Debian (where I have seen this) we haven't
>>>>> released with any affected versions yet...
>>>>
>>>> I believe the problem was introduce in 4.10, so I think it would be prudent to also back-port the final fix to stable trees from then on.
>>>
>>> Yes, the max grant frame issue has historically always been a painful
>>> experience for end users, and Xen 4.11 which we now have in the current
>>> Debian stable has made it worse compared to previous versions indeed.
>>
>> This rather suggests that the default value isn't very well chosen.
>> Ideally some investigation would be done to improve the default sizing;
>> end-users shouldn't have to know anything about grant table frames.
> 
> Most of the problems started happening a few years ago when using a
> newer Linux that got all kinds of multiqueue block stuff for disk and
> network enabled on top of an older Xen. (e.g. in Debian using the Linux
> 4.9 backports kernel on top of Xen 4.4 in Jessie).
> 
> The default for the hypervisor option has already been doubled from 32
> to 64, which I think is sufficient. However, having the toolstack revert
> it back to 32 again is not very helpful, but that's what this thread is
> about to solve. :)
> 
> A while ago I did some testing:
>     https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=880554#119
> 
> I haven't been able to cause nr_frames to go over 64 in any test myself,
> and also have never seen values that high in production use. The above
> debian bug also does not contain any other report from anyone with a
> number above 64. There are reports of users setting it to 256 and then
> not caring about it any more, but they didn't report the xen_diag output
> back after that, so there's no real data.

I have seen guests needing 256.

My Linux kernel patches reducing the default max. number of queues in
netfront/netback to 8 made things much better (on a large host running
a guest with 64 vcpus using 8 network interfaces was blowing up rather
fast).


Juergen

Patch

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