[Xen-devel] [PATCH] domain_create: honour global grant/maptrack frame limits...

[Xen-devel] [PATCH] domain_create: honour global grant/maptrack frame limits...

[Paul Durrant]

...when their values are larger than the per-domain configured limits.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Wei Liu <wl@xen.org>

After mining through commits it is still unclear to me exactly when Xen
stopped honouring the global values, but I really think this commit should
be back-ported to stable trees as it was a behavioural change that can
cause domUs to fail in non-obvious ways.
---
 xen/common/domain.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 611116c7fc..aad6d55b82 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -335,6 +335,7 @@ struct domain *domain_create(domid_t domid,
     enum { INIT_watchdog = 1u<<1,
            INIT_evtchn = 1u<<3, INIT_gnttab = 1u<<4, INIT_arch = 1u<<5 };
     int err, init_status = 0;
+    unsigned int max_grant_frames, max_maptrack_frames;
 
     if ( config && (err = sanitise_domain_config(config)) )
         return ERR_PTR(err);
@@ -456,8 +457,17 @@ struct domain *domain_create(domid_t domid,
             goto fail;
         init_status |= INIT_evtchn;
 
-        if ( (err = grant_table_init(d, config->max_grant_frames,
-                                     config->max_maptrack_frames)) != 0 )
+        /*
+         * Make sure that the configured values don't reduce any
+         * global command line override.
+         */
+        max_grant_frames = max(config->max_grant_frames,
+                               opt_max_grant_frames);
+        max_maptrack_frames = max(config->max_maptrack_frames,
+                                  opt_max_maptrack_frames);
+
+        if ( (err = grant_table_init(d, max_grant_frames,
+                                     max_maptrack_frames)) != 0 )
             goto fail;
         init_status |= INIT_gnttab;
 
-- 
2.17.1


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

Re: [Xen-devel] [PATCH] domain_create: honour global grant/maptrack frame limits...

[Paul Durrant]

On Wed, 13 Nov 2019 at 13:55, Paul Durrant <pdurrant@amazon.com> wrote:
>
> ...when their values are larger than the per-domain configured limits.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Julien Grall <julien@xen.org>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Wei Liu <wl@xen.org>
>
> After mining through commits it is still unclear to me exactly when Xen
> stopped honouring the global values, but I really think this commit should
> be back-ported to stable trees as it was a behavioural change that can
> cause domUs to fail in non-obvious ways.

Any other opinions on this? AFAICT questions is still open:

- Do we consider not honouring the command line values to be a
regression (since domUs that would have worked before will no longer
work after a basic upgrade of Xen)?

  Paul

> ---
>  xen/common/domain.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 611116c7fc..aad6d55b82 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -335,6 +335,7 @@ struct domain *domain_create(domid_t domid,
>      enum { INIT_watchdog = 1u<<1,
>             INIT_evtchn = 1u<<3, INIT_gnttab = 1u<<4, INIT_arch = 1u<<5 };
>      int err, init_status = 0;
> +    unsigned int max_grant_frames, max_maptrack_frames;
>
>      if ( config && (err = sanitise_domain_config(config)) )
>          return ERR_PTR(err);
> @@ -456,8 +457,17 @@ struct domain *domain_create(domid_t domid,
>              goto fail;
>          init_status |= INIT_evtchn;
>
> -        if ( (err = grant_table_init(d, config->max_grant_frames,
> -                                     config->max_maptrack_frames)) != 0 )
> +        /*
> +         * Make sure that the configured values don't reduce any
> +         * global command line override.
> +         */
> +        max_grant_frames = max(config->max_grant_frames,
> +                               opt_max_grant_frames);
> +        max_maptrack_frames = max(config->max_maptrack_frames,
> +                                  opt_max_maptrack_frames);
> +
> +        if ( (err = grant_table_init(d, max_grant_frames,
> +                                     max_maptrack_frames)) != 0 )
>              goto fail;
>          init_status |= INIT_gnttab;
>
> --
> 2.17.1
>
>
> _______________________________________________
> 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

Re: [Xen-devel] [PATCH] domain_create: honour global grant/maptrack frame limits...

[Jürgen Groß]

On 26.11.19 12:30, Paul Durrant wrote:
> On Wed, 13 Nov 2019 at 13:55, Paul Durrant <pdurrant@amazon.com> wrote:
>>
>> ...when their values are larger than the per-domain configured limits.
>>
>> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
>> ---
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Julien Grall <julien@xen.org>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Wei Liu <wl@xen.org>
>>
>> After mining through commits it is still unclear to me exactly when Xen
>> stopped honouring the global values, but I really think this commit should
>> be back-ported to stable trees as it was a behavioural change that can
>> cause domUs to fail in non-obvious ways.
> 
> Any other opinions on this? AFAICT questions is still open:
> 
> - Do we consider not honouring the command line values to be a
> regression (since domUs that would have worked before will no longer
> work after a basic upgrade of Xen)?
> 
>    Paul
> 
>> ---
>>   xen/common/domain.c | 14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>> index 611116c7fc..aad6d55b82 100644
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -335,6 +335,7 @@ struct domain *domain_create(domid_t domid,
>>       enum { INIT_watchdog = 1u<<1,
>>              INIT_evtchn = 1u<<3, INIT_gnttab = 1u<<4, INIT_arch = 1u<<5 };
>>       int err, init_status = 0;
>> +    unsigned int max_grant_frames, max_maptrack_frames;
>>
>>       if ( config && (err = sanitise_domain_config(config)) )
>>           return ERR_PTR(err);
>> @@ -456,8 +457,17 @@ struct domain *domain_create(domid_t domid,
>>               goto fail;
>>           init_status |= INIT_evtchn;
>>
>> -        if ( (err = grant_table_init(d, config->max_grant_frames,
>> -                                     config->max_maptrack_frames)) != 0 )
>> +        /*
>> +         * Make sure that the configured values don't reduce any
>> +         * global command line override.
>> +         */
>> +        max_grant_frames = max(config->max_grant_frames,
>> +                               opt_max_grant_frames);
>> +        max_maptrack_frames = max(config->max_maptrack_frames,
>> +                                  opt_max_maptrack_frames);
>> +
>> +        if ( (err = grant_table_init(d, max_grant_frames,
>> +                                     max_maptrack_frames)) != 0 )

So basically the per-domain settings are ignored.

They are not allowed to be smaller than the global limits (due to
using max()).

They are not allowed to be larger than the global limits (due to the
test in grant_table_init().

That is _not_ the purpose of being able to control the settings per
domain.


Juergen

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

Re: [Xen-devel] [PATCH] domain_create: honour global grant/maptrack frame limits...

[Andrew Cooper]

On 26/11/2019 11:30, Paul Durrant wrote:
> On Wed, 13 Nov 2019 at 13:55, Paul Durrant <pdurrant@amazon.com> wrote:
>> ...when their values are larger than the per-domain configured limits.
>>
>> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
>> ---
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Julien Grall <julien@xen.org>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Wei Liu <wl@xen.org>
>>
>> After mining through commits it is still unclear to me exactly when Xen
>> stopped honouring the global values, but I really think this commit should
>> be back-ported to stable trees as it was a behavioural change that can
>> cause domUs to fail in non-obvious ways.
> Any other opinions on this? AFAICT questions is still open:
>
> - Do we consider not honouring the command line values to be a
> regression (since domUs that would have worked before will no longer
> work after a basic upgrade of Xen)?

I think I've been very clear on my opinion of this patch, and what I
would consider an acceptable way forward.

This patch breaks things in exactly the (opposite) way you are
complaining about having happened when the Xen command line options were
replaced with xl.conf options for domU.

Yes - it wasn't great to have done things like this.  No - its not
acceptable to do the same again and break people now relying on the per
domain settings to take effect.

~Andrew

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

Re: [Xen-devel] [PATCH] domain_create: honour global grant/maptrack frame limits...

[Durrant, Paul]

> -----Original Message-----
> From: Jürgen Groß <jgross@suse.com>
> Sent: 26 November 2019 11:37
> To: Paul Durrant <pdurrant@gmail.com>; Durrant, Paul <pdurrant@amazon.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall
> <julien@xen.org>; Wei Liu <wl@xen.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; George Dunlap <George.Dunlap@eu.citrix.com>;
> Andrew Cooper <andrew.cooper3@citrix.com>; Ian Jackson
> <ian.jackson@eu.citrix.com>; Jan Beulich <jbeulich@suse.com>; xen-devel
> <xen-devel@lists.xenproject.org>
> Subject: Re: [Xen-devel] [PATCH] domain_create: honour global
> grant/maptrack frame limits...
> 
> On 26.11.19 12:30, Paul Durrant wrote:
> > On Wed, 13 Nov 2019 at 13:55, Paul Durrant <pdurrant@amazon.com> wrote:
> >>
> >> ...when their values are larger than the per-domain configured limits.
> >>
> >> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> >> ---
> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> >> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> >> Cc: Jan Beulich <jbeulich@suse.com>
> >> Cc: Julien Grall <julien@xen.org>
> >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >> Cc: Stefano Stabellini <sstabellini@kernel.org>
> >> Cc: Wei Liu <wl@xen.org>
> >>
> >> After mining through commits it is still unclear to me exactly when Xen
> >> stopped honouring the global values, but I really think this commit
> should
> >> be back-ported to stable trees as it was a behavioural change that can
> >> cause domUs to fail in non-obvious ways.
> >
> > Any other opinions on this? AFAICT questions is still open:
> >
> > - Do we consider not honouring the command line values to be a
> > regression (since domUs that would have worked before will no longer
> > work after a basic upgrade of Xen)?
> >
> >    Paul
> >
> >> ---
> >>   xen/common/domain.c | 14 ++++++++++++--
> >>   1 file changed, 12 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/xen/common/domain.c b/xen/common/domain.c
> >> index 611116c7fc..aad6d55b82 100644
> >> --- a/xen/common/domain.c
> >> +++ b/xen/common/domain.c
> >> @@ -335,6 +335,7 @@ struct domain *domain_create(domid_t domid,
> >>       enum { INIT_watchdog = 1u<<1,
> >>              INIT_evtchn = 1u<<3, INIT_gnttab = 1u<<4, INIT_arch =
> 1u<<5 };
> >>       int err, init_status = 0;
> >> +    unsigned int max_grant_frames, max_maptrack_frames;
> >>
> >>       if ( config && (err = sanitise_domain_config(config)) )
> >>           return ERR_PTR(err);
> >> @@ -456,8 +457,17 @@ struct domain *domain_create(domid_t domid,
> >>               goto fail;
> >>           init_status |= INIT_evtchn;
> >>
> >> -        if ( (err = grant_table_init(d, config->max_grant_frames,
> >> -                                     config->max_maptrack_frames)) !=
> 0 )
> >> +        /*
> >> +         * Make sure that the configured values don't reduce any
> >> +         * global command line override.
> >> +         */
> >> +        max_grant_frames = max(config->max_grant_frames,
> >> +                               opt_max_grant_frames);
> >> +        max_maptrack_frames = max(config->max_maptrack_frames,
> >> +                                  opt_max_maptrack_frames);
> >> +
> >> +        if ( (err = grant_table_init(d, max_grant_frames,
> >> +                                     max_maptrack_frames)) != 0 )
> 
> So basically the per-domain settings are ignored.
> 

Basically, yes.

> They are not allowed to be smaller than the global limits (due to
> using max()).
> 
> They are not allowed to be larger than the global limits (due to the
> test in grant_table_init().
> 
> That is _not_ the purpose of being able to control the settings per
> domain.
> 

Ok, if a straight-up return to old behaviour is out then I guess 4.13 will carry the regression.

  Paul

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

Re: [Xen-devel] [PATCH] domain_create: honour global grant/maptrack frame limits...

[George Dunlap]

On 11/26/19 11:30 AM, Paul Durrant wrote:
> On Wed, 13 Nov 2019 at 13:55, Paul Durrant <pdurrant@amazon.com> wrote:
>>
>> ...when their values are larger than the per-domain configured limits.
>>
>> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
>> ---
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Julien Grall <julien@xen.org>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Wei Liu <wl@xen.org>
>>
>> After mining through commits it is still unclear to me exactly when Xen
>> stopped honouring the global values, but I really think this commit should
>> be back-ported to stable trees as it was a behavioural change that can
>> cause domUs to fail in non-obvious ways.
> 
> Any other opinions on this? AFAICT questions is still open:
> 
> - Do we consider not honouring the command line values to be a
> regression (since domUs that would have worked before will no longer
> work after a basic upgrade of Xen)?

This would be a bit easier to form a "policy" opinion on (or perhaps
alternate solutions to) if more of the situation were outlined here.

Is the problem that the per-domain config is always set, and doesn't
take the hypervisor-set config into account?  Wouldn't it be better to
modify the toolstack to use the hypervisor value if it's not set?

In fact, it looks kind of like things are screwed up anyway -- the
"default" value of max_grant_frames, if no value is specified, is set in
xl.c.  If that were the behavior we wanted, it should be set in libxl.c.

But it doesn't seem like it should be terribly difficult to get a "use
the default" sentinel value passed in to Xen, such that:

1. People who don't do anything will get the default currently specified
in xl.c

2. People who set the value on the Xen command-line and don't set
anything in the guest config file will get the Xen command-line value

3. People who set the value in the config file will get the value they
specified (regardless of the global setting).

Is that the behaviour you'd like to see, Paul?

 -George

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

Re: [Xen-devel] [PATCH] domain_create: honour global grant/maptrack frame limits...

[Durrant, Paul]

> -----Original Message-----
> From: George Dunlap <george.dunlap@citrix.com>
> Sent: 26 November 2019 12:32
> To: Paul Durrant <pdurrant@gmail.com>; Durrant, Paul <pdurrant@amazon.com>
> Cc: xen-devel <xen-devel@lists.xenproject.org>; Stefano Stabellini
> <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Wei Liu
> <wl@xen.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; George
> Dunlap <George.Dunlap@eu.citrix.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Jan
> Beulich <jbeulich@suse.com>
> Subject: Re: [Xen-devel] [PATCH] domain_create: honour global
> grant/maptrack frame limits...
> 
> On 11/26/19 11:30 AM, Paul Durrant wrote:
> > On Wed, 13 Nov 2019 at 13:55, Paul Durrant <pdurrant@amazon.com> wrote:
> >>
> >> ...when their values are larger than the per-domain configured limits.
> >>
> >> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> >> ---
> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> >> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> >> Cc: Jan Beulich <jbeulich@suse.com>
> >> Cc: Julien Grall <julien@xen.org>
> >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >> Cc: Stefano Stabellini <sstabellini@kernel.org>
> >> Cc: Wei Liu <wl@xen.org>
> >>
> >> After mining through commits it is still unclear to me exactly when Xen
> >> stopped honouring the global values, but I really think this commit
> should
> >> be back-ported to stable trees as it was a behavioural change that can
> >> cause domUs to fail in non-obvious ways.
> >
> > Any other opinions on this? AFAICT questions is still open:
> >
> > - Do we consider not honouring the command line values to be a
> > regression (since domUs that would have worked before will no longer
> > work after a basic upgrade of Xen)?
> 
> This would be a bit easier to form a "policy" opinion on (or perhaps
> alternate solutions to) if more of the situation were outlined here.
> 
> Is the problem that the per-domain config is always set, and doesn't
> take the hypervisor-set config into account?  Wouldn't it be better to
> modify the toolstack to use the hypervisor value if it's not set?
> 
> In fact, it looks kind of like things are screwed up anyway -- the
> "default" value of max_grant_frames, if no value is specified, is set in
> xl.c.  If that were the behavior we wanted, it should be set in libxl.c.
> 
> But it doesn't seem like it should be terribly difficult to get a "use
> the default" sentinel value passed in to Xen, such that:
> 
> 1. People who don't do anything will get the default currently specified
> in xl.c
> 
> 2. People who set the value on the Xen command-line and don't set
> anything in the guest config file will get the Xen command-line value
> 
> 3. People who set the value in the config file will get the value they
> specified (regardless of the global setting).
> 
> Is that the behaviour you'd like to see, Paul?

I think the order should be:

If set in xl.cfg => use that, else
If set in xl.conf => use that, else
Use the command line/default value

I.e. the ultimate value should be set in Xen (and possibly overridden by the command line) and not hardcoded at any other layer.

There is also the issue of limits but I guess the rationale there should be: If a value *is* specified then it should not exceed the value set in Xen.

Does that sound right?

  Paul


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

Re: [Xen-devel] [PATCH] domain_create: honour global grant/maptrack frame limits...

[George Dunlap]

On 11/26/19 1:26 PM, Durrant, Paul wrote:
>> -----Original Message-----
>> From: George Dunlap <george.dunlap@citrix.com>
>> Sent: 26 November 2019 12:32
>> To: Paul Durrant <pdurrant@gmail.com>; Durrant, Paul <pdurrant@amazon.com>
>> Cc: xen-devel <xen-devel@lists.xenproject.org>; Stefano Stabellini
>> <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Wei Liu
>> <wl@xen.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; George
>> Dunlap <George.Dunlap@eu.citrix.com>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Jan
>> Beulich <jbeulich@suse.com>
>> Subject: Re: [Xen-devel] [PATCH] domain_create: honour global
>> grant/maptrack frame limits...
>>
>> On 11/26/19 11:30 AM, Paul Durrant wrote:
>>> On Wed, 13 Nov 2019 at 13:55, Paul Durrant <pdurrant@amazon.com> wrote:
>>>>
>>>> ...when their values are larger than the per-domain configured limits.
>>>>
>>>> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
>>>> ---
>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
>>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>>>> Cc: Jan Beulich <jbeulich@suse.com>
>>>> Cc: Julien Grall <julien@xen.org>
>>>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>>> Cc: Wei Liu <wl@xen.org>
>>>>
>>>> After mining through commits it is still unclear to me exactly when Xen
>>>> stopped honouring the global values, but I really think this commit
>> should
>>>> be back-ported to stable trees as it was a behavioural change that can
>>>> cause domUs to fail in non-obvious ways.
>>>
>>> Any other opinions on this? AFAICT questions is still open:
>>>
>>> - Do we consider not honouring the command line values to be a
>>> regression (since domUs that would have worked before will no longer
>>> work after a basic upgrade of Xen)?
>>
>> This would be a bit easier to form a "policy" opinion on (or perhaps
>> alternate solutions to) if more of the situation were outlined here.
>>
>> Is the problem that the per-domain config is always set, and doesn't
>> take the hypervisor-set config into account?  Wouldn't it be better to
>> modify the toolstack to use the hypervisor value if it's not set?
>>
>> In fact, it looks kind of like things are screwed up anyway -- the
>> "default" value of max_grant_frames, if no value is specified, is set in
>> xl.c.  If that were the behavior we wanted, it should be set in libxl.c.
>>
>> But it doesn't seem like it should be terribly difficult to get a "use
>> the default" sentinel value passed in to Xen, such that:
>>
>> 1. People who don't do anything will get the default currently specified
>> in xl.c
>>
>> 2. People who set the value on the Xen command-line and don't set
>> anything in the guest config file will get the Xen command-line value
>>
>> 3. People who set the value in the config file will get the value they
>> specified (regardless of the global setting).
>>
>> Is that the behaviour you'd like to see, Paul?
> 
> I think the order should be:
> 
> If set in xl.cfg => use that, else
> If set in xl.conf => use that, else
> Use the command line/default value
> 
> I.e. the ultimate value should be set in Xen (and possibly overridden by the command line) and not hardcoded at any other layer.
> 
> There is also the issue of limits but I guess the rationale there should be: If a value *is* specified then it should not exceed the value set in Xen.
> 
> Does that sound right?

So part of the issue here sounds like a terminology issue.  Is it the
case that there's a default "max", and you want to raise the default
"max"; is that right?

But the documentation actually says:

"Specify the maximum number of frames which any domain may use as part
of its grant table."

Which makes it sound a lot more like a "maximum max" -- i.e., that any
domain which is created with a value higher than this should fail.

 -George

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

Re: [Xen-devel] [PATCH] domain_create: honour global grant/maptrack frame limits...

[Durrant, Paul]

> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: 13 November 2019 14:05
> To: Durrant, Paul <pdurrant@amazon.com>; xen-devel@lists.xenproject.org
> Subject: Re: [Xen-devel] [PATCH] domain_create: honour global
> grant/maptrack frame limits...
> 
> On 13/11/2019 13:47, Paul Durrant wrote:
> > ...when their values are larger than the per-domain configured limits.
> >
> > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> > ---
> > After mining through commits it is still unclear to me exactly when Xen
> > stopped honouring the global values, but I really think this commit
> should
> > be back-ported to stable trees as it was a behavioural change that can
> > cause domUs to fail in non-obvious ways.
> 
> -1.  Overriding toolstack settings like this is the same kind of "bad"
> as silently converting HAP => Shadow.
> 
> In particular, this breaks one of points of the original per-domain work
> to deliberately allow stub xenstored to be configured with tiny
> grant/maptrack tables.

Ok, but IMO subtly breaking domUs in the process is not really acceptable behaviour.

> 
> You also break the setting of these parameters in xl.conf.

No I don't. xl.conf can still increase values over the command line.

> 
> I'm not defending how the interface changed subtly/unexpected; that was
> bad and we should have done better, but this change is just as bad in
> the opposite direction.
> 
> The way to fix this is to plumb the Xen default parameters though so
> that, in the absence of any explicit configuration (vm.cfg or xl.conf),
> libxl can then use "xen cmdline" as a source of configuration, before
> falling back to hardcoded numbers.
> 

I agree that is the best way to fix it, but not so easy to backport. So my proposal (via email a few days ago) was that regressions are fixed immediately in this way and then a *proper* fix is done moving forward, which I shall base upon Juergen's patches which should allow easy retrieval of the command line values.

  Paul

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

Re: [Xen-devel] [PATCH] domain_create: honour global grant/maptrack frame limits...

[Andrew Cooper]

On 13/11/2019 13:47, Paul Durrant wrote:
> ...when their values are larger than the per-domain configured limits.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> ---
> After mining through commits it is still unclear to me exactly when Xen
> stopped honouring the global values, but I really think this commit should
> be back-ported to stable trees as it was a behavioural change that can
> cause domUs to fail in non-obvious ways.

-1.  Overriding toolstack settings like this is the same kind of "bad"
as silently converting HAP => Shadow.

In particular, this breaks one of points of the original per-domain work
to deliberately allow stub xenstored to be configured with tiny
grant/maptrack tables.

You also break the setting of these parameters in xl.conf.

I'm not defending how the interface changed subtly/unexpected; that was
bad and we should have done better, but this change is just as bad in
the opposite direction.

The way to fix this is to plumb the Xen default parameters though so
that, in the absence of any explicit configuration (vm.cfg or xl.conf),
libxl can then use "xen cmdline" as a source of configuration, before
falling back to hardcoded numbers.

~Andrew

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

Re: [Xen-devel] [PATCH] domain_create: honour global grant/maptrack frame limits...

[Durrant, Paul]

Sorry, the Cc list got dropped... I'll re-send.

  Paul

> -----Original Message-----
> From: Paul Durrant <pdurrant@amazon.com>
> Sent: 13 November 2019 13:47
> To: xen-devel@lists.xenproject.org
> Cc: Durrant, Paul <pdurrant@amazon.com>
> Subject: [PATCH] domain_create: honour global grant/maptrack frame
> limits...
> 
> ...when their values are larger than the per-domain configured limits.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> ---
> After mining through commits it is still unclear to me exactly when Xen
> stopped honouring the global values, but I really think this commit should
> be back-ported to stable trees as it was a behavioural change that can
> cause domUs to fail in non-obvious ways.
> ---
>  xen/common/domain.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 611116c7fc..aad6d55b82 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -335,6 +335,7 @@ struct domain *domain_create(domid_t domid,
>      enum { INIT_watchdog = 1u<<1,
>             INIT_evtchn = 1u<<3, INIT_gnttab = 1u<<4, INIT_arch = 1u<<5 };
>      int err, init_status = 0;
> +    unsigned int max_grant_frames, max_maptrack_frames;
> 
>      if ( config && (err = sanitise_domain_config(config)) )
>          return ERR_PTR(err);
> @@ -456,8 +457,17 @@ struct domain *domain_create(domid_t domid,
>              goto fail;
>          init_status |= INIT_evtchn;
> 
> -        if ( (err = grant_table_init(d, config->max_grant_frames,
> -                                     config->max_maptrack_frames)) != 0 )
> +        /*
> +         * Make sure that the configured values don't reduce any
> +         * global command line override.
> +         */
> +        max_grant_frames = max(config->max_grant_frames,
> +                               opt_max_grant_frames);
> +        max_maptrack_frames = max(config->max_maptrack_frames,
> +                                  opt_max_maptrack_frames);
> +
> +        if ( (err = grant_table_init(d, max_grant_frames,
> +                                     max_maptrack_frames)) != 0 )
>              goto fail;
>          init_status |= INIT_gnttab;
> 
> --
> 2.17.1


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

[Xen-devel] [PATCH] domain_create: honour global grant/maptrack frame limits...

[Paul Durrant]

...when their values are larger than the per-domain configured limits.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
After mining through commits it is still unclear to me exactly when Xen
stopped honouring the global values, but I really think this commit should
be back-ported to stable trees as it was a behavioural change that can
cause domUs to fail in non-obvious ways.
---
 xen/common/domain.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 611116c7fc..aad6d55b82 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -335,6 +335,7 @@ struct domain *domain_create(domid_t domid,
     enum { INIT_watchdog = 1u<<1,
            INIT_evtchn = 1u<<3, INIT_gnttab = 1u<<4, INIT_arch = 1u<<5 };
     int err, init_status = 0;
+    unsigned int max_grant_frames, max_maptrack_frames;
 
     if ( config && (err = sanitise_domain_config(config)) )
         return ERR_PTR(err);
@@ -456,8 +457,17 @@ struct domain *domain_create(domid_t domid,
             goto fail;
         init_status |= INIT_evtchn;
 
-        if ( (err = grant_table_init(d, config->max_grant_frames,
-                                     config->max_maptrack_frames)) != 0 )
+        /*
+         * Make sure that the configured values don't reduce any
+         * global command line override.
+         */
+        max_grant_frames = max(config->max_grant_frames,
+                               opt_max_grant_frames);
+        max_maptrack_frames = max(config->max_maptrack_frames,
+                                  opt_max_maptrack_frames);
+
+        if ( (err = grant_table_init(d, max_grant_frames,
+                                     max_maptrack_frames)) != 0 )
             goto fail;
         init_status |= INIT_gnttab;
 
-- 
2.17.1


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