| Message ID | 20191113134729.1121-1-pdurrant@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | domain_create: honour global grant/maptrack frame limits... | expand |
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
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
> -----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
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;
...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(-)