| Message ID | 1481306143-19534-1-git-send-email-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/12/16 17:55, Paul Durrant wrote: > ...if the domain is not under construction. > > If upstream QEMU is in use then it will explicitly create an ioreq server > rather than implicitly creating the default ioreq server, which is a > side-effect of reading HVM_PARAM_IOREQ_PFN, HVM_PARAM_BUFIOREQ_PFN, > or HVM_PARAM_BUFIOREQ_EVTCHN (as is done by legacy QEMUs). > > However, if the domain is subsequently saved/migrated then those parameters > are read and hence the default server will be unnecessarily instantiated. > > This patch adds an extra check of the 'creation_finished' flag when those > HVM params are read and will only instantiate the server if the domain is > under construction, which will always be the case when QEMU is invoked. > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> CC'ing the COLO guys. Please can you test with this patch? > --- > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > --- > xen/arch/x86/hvm/hvm.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index e0f936b..c531f37 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -5337,7 +5337,16 @@ static int hvmop_get_param( > { > domid_t domid; > > - /* May need to create server. */ > + /* > + * It may be necessary to create a default ioreq server here, > + * because legacy versions of QEMU are not aware of the new API > + * for explicit ioreq server creation. However, if the domain > + * is not under construction then it will not be QEMU querying > + * the parameters and thus the query should have that side-effect. > + */ > + if ( d->creation_finished ) > + break; > + > domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN]; > rc = hvm_create_ioreq_server(d, domid, 1, > HVM_IOREQSRV_BUFIOREQ_LEGACY, NULL);
On 12/10/2016 02:21 AM, Andrew Cooper wrote: > On 09/12/16 17:55, Paul Durrant wrote: >> ...if the domain is not under construction. >> >> If upstream QEMU is in use then it will explicitly create an ioreq server >> rather than implicitly creating the default ioreq server, which is a >> side-effect of reading HVM_PARAM_IOREQ_PFN, HVM_PARAM_BUFIOREQ_PFN, >> or HVM_PARAM_BUFIOREQ_EVTCHN (as is done by legacy QEMUs). >> >> However, if the domain is subsequently saved/migrated then those parameters >> are read and hence the default server will be unnecessarily instantiated. >> >> This patch adds an extra check of the 'creation_finished' flag when those >> HVM params are read and will only instantiate the server if the domain is >> under construction, which will always be the case when QEMU is invoked. >> >> Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > > CC'ing the COLO guys. Please can you test with this patch? Tested-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> I have tested this patch, it works well on COLO! Thanks Zhang Chen > >> --- >> Cc: Jan Beulich <jbeulich@suse.com> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> xen/arch/x86/hvm/hvm.c | 11 ++++++++++- >> 1 file changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c >> index e0f936b..c531f37 100644 >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -5337,7 +5337,16 @@ static int hvmop_get_param( >> { >> domid_t domid; >> >> - /* May need to create server. */ >> + /* >> + * It may be necessary to create a default ioreq server here, >> + * because legacy versions of QEMU are not aware of the new API >> + * for explicit ioreq server creation. However, if the domain >> + * is not under construction then it will not be QEMU querying >> + * the parameters and thus the query should have that side-effect. >> + */ >> + if ( d->creation_finished ) >> + break; >> + >> domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN]; >> rc = hvm_create_ioreq_server(d, domid, 1, >> HVM_IOREQSRV_BUFIOREQ_LEGACY, NULL); > > > . >
>>> On 09.12.16 at 18:55, <paul.durrant@citrix.com> wrote: > ...if the domain is not under construction. I think the title will end up misleading this way. How about "x86/hvm: don't unconditionally create a default ioreq server" with "Avoid doing so if the domain is not under construction" as the 1st sentence? As the patch is otherwise ready to go in, that adjustment could certainly be done while committing (i.e. no re-send necessary). Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 12 December 2016 07:54 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen- > devel@lists.xenproject.org > Subject: Re: [PATCH] x86/hvm: don't create a default ioreq server... > > >>> On 09.12.16 at 18:55, <paul.durrant@citrix.com> wrote: > > ...if the domain is not under construction. > > I think the title will end up misleading this way. How about "x86/hvm: > don't unconditionally create a default ioreq server" with "Avoid doing > so if the domain is not under construction" as the 1st sentence? > Yes, that would avoid the shortlog being misleading. > As the patch is otherwise ready to go in, that adjustment could > certainly be done while committing (i.e. no re-send necessary). > Thanks. I also realised there is typo in the code comment: " the query should have that side-effect" should be " the query should *not* have that side-effect" Please can you fix this too. Cheers, Paul > Jan
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index e0f936b..c531f37 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -5337,7 +5337,16 @@ static int hvmop_get_param( { domid_t domid; - /* May need to create server. */ + /* + * It may be necessary to create a default ioreq server here, + * because legacy versions of QEMU are not aware of the new API + * for explicit ioreq server creation. However, if the domain + * is not under construction then it will not be QEMU querying + * the parameters and thus the query should have that side-effect. + */ + if ( d->creation_finished ) + break; + domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN]; rc = hvm_create_ioreq_server(d, domid, 1, HVM_IOREQSRV_BUFIOREQ_LEGACY, NULL);
...if the domain is not under construction. If upstream QEMU is in use then it will explicitly create an ioreq server rather than implicitly creating the default ioreq server, which is a side-effect of reading HVM_PARAM_IOREQ_PFN, HVM_PARAM_BUFIOREQ_PFN, or HVM_PARAM_BUFIOREQ_EVTCHN (as is done by legacy QEMUs). However, if the domain is subsequently saved/migrated then those parameters are read and hence the default server will be unnecessarily instantiated. This patch adds an extra check of the 'creation_finished' flag when those HVM params are read and will only instantiate the server if the domain is under construction, which will always be the case when QEMU is invoked. Signed-off-by: Paul Durrant <paul.durrant@citrix.com> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- xen/arch/x86/hvm/hvm.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)