Re: [libvirt] [PATCH 03/11] qemu: Pass QEMUCaps to virDomainDefPostParse
On Mon, Oct 01, 2018 at 14:58:11 +0200, Marc Hartmayer wrote: > On Mon, Oct 01, 2018 at 12:36 PM +0200, Peter Krempa > wrote: [...] > >> Yep, that’s why I said in the cover letter “With this patch series the > >> behavior is still not (completely) fixed, but the performance has been > >> significantly improved.”. > > > > I don't see what should be fixed here, but mostly as I did not read the > > series. > > The problem is that we currently don’t take into account that the QEMU > binary can change during a task (e.g. define a domain). Therefore, it’s > possible that two calls of virQEMUCapsCacheLookup return different QEMU > capabilities. I don't think that is too big of a problem in the light that the binary can change between the final call to virQEMUCapsCacheLookup which is used by the code generating the commandline and actual exec of the qemu binary once the commandline is generated. Solving that won't be easy at all. Since virDomainDefValidate is called with the final capabilities instance when starting the qemu process the possibility that it would change and the problems it might create are negligible when compared to actually execing something else. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/11] qemu: Pass QEMUCaps to virDomainDefPostParse
On Mon, Oct 01, 2018 at 12:36 PM +0200, Peter Krempa wrote: […snip…] >> I would do all the precondition stuff (e.g. the priv->qemuCaps isn’t >> valid anymore) as early as possible in the overall “task/job” process. > > Well, technically priv->qemuCaps is invalid and unneeded when the VM is > not running and valid and immutable if it is running. > > If the VM is not running, there is no qemu process associated to it so > it does not make sense to store random qemuCaps along with it, since the > qemu binary might change at the point when we attempt to start it. Yep, that makes sense. > > I was not a fan of needing qemuCaps in the post parse infrastructure of > qemu, but at this point we can't do much about it especially as we want > to keep the config stable after defined. > >> > This patch does not seem to make sense with the justification provided >> > here as the post-parse callbacks fill in the proper caps here and this >> > part clearly uses a new domain configuration, so the default behaviour >> > should be used. >> > >> > Changing this would also need that we change the normal parser path to >> > achieve the same results which is impossible as you don't have access to >> > priv->qemuCaps prior to creating the virDomainObj object. >> >> Yep, that’s why I said in the cover letter “With this patch series the >> behavior is still not (completely) fixed, but the performance has been >> significantly improved.”. > > I don't see what should be fixed here, but mostly as I did not read the > series. The problem is that we currently don’t take into account that the QEMU binary can change during a task (e.g. define a domain). Therefore, it’s possible that two calls of virQEMUCapsCacheLookup return different QEMU capabilities. […snip] -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/11] qemu: Pass QEMUCaps to virDomainDefPostParse
On Mon, Oct 01, 2018 at 10:31:36 +0200, Marc Hartmayer wrote:
> On Mon, Oct 01, 2018 at 09:33 AM +0200, Peter Krempa
> wrote:
> > On Fri, Sep 28, 2018 at 22:02:59 -0400, John Ferlan wrote:
> >>
> >>
> >> On 9/20/18 1:44 PM, Marc Hartmayer wrote:
> >> > ...although priv->qemuCaps will be NULL in almost every case when the
> >> > post parse callback has failed. That may change in the future.
> >> >
> >> > Signed-off-by: Marc Hartmayer
> >> > Reviewed-by: Boris Fiuczynski
> >> > ---
> >> > src/qemu/qemu_process.c | 2 +-
> >> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> >> > index 44c63c42d618..6c5a6472d8cd 100644
> >> > --- a/src/qemu/qemu_process.c
> >> > +++ b/src/qemu/qemu_process.c
> >> > @@ -5282,7 +5282,7 @@ qemuProcessInit(virQEMUDriverPtr driver,
> >>
> >> The comment just above here could use a tweak for grammar ;-):
> >>
> >> /* in case when the post parse callback failed we need to re-run it
> >> on the
> >> * old config prior we start the VM */
> >>
> >> > if (vm->def->postParseFailed) {
> >> > VIR_DEBUG("re-running the post parse callback");
> >> >
> >> > -if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt,
> >> > NULL) < 0)
> >> > +if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt,
> >> > priv->qemuCaps) < 0)
> >>
> >> Searching through history of this line finds Peter's original commit in
> >> this area - 7726d158, which seems to indicate a very specific reason for
> >> providing a NULL capabilities value here.
> >>
> >> I think from this patch on is something Peter has worked on a lot, so I
> >> would prefer to defer to Peter on them since I'm sure he understands all
> >> the various PostParse and Validate special conditions and various flags
> >> usage better than I do.
> >
> > Thanks for notifying me.
>
> First, thanks for your answer!
>
> >
> > In general, when parsing a "new" domain config as it's the case here,
> > NULL should be passed in.
>
> Wouldn’t it be better to invalidate the QEMU caps (priv->qemuCaps)
> explicitly beforehand? (for example if the 'postParseFailed' flag is set
> and we’re starting the domain).
As stated below, in the normal code-flow at the point when the XML is
parsed there is no virDomainObj and thus no priv.
Even when you create a virDomainObj prior to that, the priv->qemuCaps is
based on the binary which in turn is based on defaults added by the post
parse callback as the binary is a qemu-driver-specific.
> > The qemu driver registers the 'domainPostParseDataAlloc' callback to
> > qemuDomainPostParseDataAlloc. This callback is executed after the
> > 'basic' post parse callback which fills in the emulator binary.
> >
> > Passing an explicit qemuCaps is meant only for special cases e.g.
> > migration where we have a specific set of capabilities which need to be
> > used rather than a new one.
>
> I would do all the precondition stuff (e.g. the priv->qemuCaps isn’t
> valid anymore) as early as possible in the overall “task/job” process.
Well, technically priv->qemuCaps is invalid and unneeded when the VM is
not running and valid and immutable if it is running.
If the VM is not running, there is no qemu process associated to it so
it does not make sense to store random qemuCaps along with it, since the
qemu binary might change at the point when we attempt to start it.
I was not a fan of needing qemuCaps in the post parse infrastructure of
qemu, but at this point we can't do much about it especially as we want
to keep the config stable after defined.
> > This patch does not seem to make sense with the justification provided
> > here as the post-parse callbacks fill in the proper caps here and this
> > part clearly uses a new domain configuration, so the default behaviour
> > should be used.
> >
> > Changing this would also need that we change the normal parser path to
> > achieve the same results which is impossible as you don't have access to
> > priv->qemuCaps prior to creating the virDomainObj object.
>
> Yep, that’s why I said in the cover letter “With this patch series the
> behavior is still not (completely) fixed, but the performance has been
> significantly improved.”.
I don't see what should be fixed here, but mostly as I did not read the
series.
> IMHO, it’s a design flaw that the virDomainObjList class is responsible
> for the creation of the virDomainObj… The responsibilities of the
> classes are mixed and this enforces the lock order virDomainObjList ->
> virDomainObj (what actually is a performance bottleneck). IMHO, we
> should create the virDomainObj earlier and add it to the
> virDomainObjList when it’s useful.
I think that is orthogonal from the post parse infrastructure. Post
parse applies on virDomainDef object, while the domain list is of
virDomainObj type. That is thre reason to have private copy of the caps
in the post parse code flow. You don't have to associate a config with a
domai
Re: [libvirt] [PATCH 03/11] qemu: Pass QEMUCaps to virDomainDefPostParse
On Mon, Oct 01, 2018 at 09:33 AM +0200, Peter Krempa wrote:
> On Fri, Sep 28, 2018 at 22:02:59 -0400, John Ferlan wrote:
>>
>>
>> On 9/20/18 1:44 PM, Marc Hartmayer wrote:
>> > ...although priv->qemuCaps will be NULL in almost every case when the
>> > post parse callback has failed. That may change in the future.
>> >
>> > Signed-off-by: Marc Hartmayer
>> > Reviewed-by: Boris Fiuczynski
>> > ---
>> > src/qemu/qemu_process.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> > index 44c63c42d618..6c5a6472d8cd 100644
>> > --- a/src/qemu/qemu_process.c
>> > +++ b/src/qemu/qemu_process.c
>> > @@ -5282,7 +5282,7 @@ qemuProcessInit(virQEMUDriverPtr driver,
>>
>> The comment just above here could use a tweak for grammar ;-):
>>
>> /* in case when the post parse callback failed we need to re-run it
>> on the
>> * old config prior we start the VM */
>>
>> > if (vm->def->postParseFailed) {
>> > VIR_DEBUG("re-running the post parse callback");
>> >
>> > -if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, NULL)
>> > < 0)
>> > +if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt,
>> > priv->qemuCaps) < 0)
>>
>> Searching through history of this line finds Peter's original commit in
>> this area - 7726d158, which seems to indicate a very specific reason for
>> providing a NULL capabilities value here.
>>
>> I think from this patch on is something Peter has worked on a lot, so I
>> would prefer to defer to Peter on them since I'm sure he understands all
>> the various PostParse and Validate special conditions and various flags
>> usage better than I do.
>
> Thanks for notifying me.
First, thanks for your answer!
>
> In general, when parsing a "new" domain config as it's the case here,
> NULL should be passed in.
Wouldn’t it be better to invalidate the QEMU caps (priv->qemuCaps)
explicitly beforehand? (for example if the 'postParseFailed' flag is set
and we’re starting the domain).
>
> The qemu driver registers the 'domainPostParseDataAlloc' callback to
> qemuDomainPostParseDataAlloc. This callback is executed after the
> 'basic' post parse callback which fills in the emulator binary.
>
> Passing an explicit qemuCaps is meant only for special cases e.g.
> migration where we have a specific set of capabilities which need to be
> used rather than a new one.
I would do all the precondition stuff (e.g. the priv->qemuCaps isn’t
valid anymore) as early as possible in the overall “task/job” process.
>
> This patch does not seem to make sense with the justification provided
> here as the post-parse callbacks fill in the proper caps here and this
> part clearly uses a new domain configuration, so the default behaviour
> should be used.
>
> Changing this would also need that we change the normal parser path to
> achieve the same results which is impossible as you don't have access to
> priv->qemuCaps prior to creating the virDomainObj object.
Yep, that’s why I said in the cover letter “With this patch series the
behavior is still not (completely) fixed, but the performance has been
significantly improved.”.
IMHO, it’s a design flaw that the virDomainObjList class is responsible
for the creation of the virDomainObj… The responsibilities of the
classes are mixed and this enforces the lock order virDomainObjList ->
virDomainObj (what actually is a performance bottleneck). IMHO, we
should create the virDomainObj earlier and add it to the
virDomainObjList when it’s useful.
What’s your opinion about that?
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/11] qemu: Pass QEMUCaps to virDomainDefPostParse
On Fri, Sep 28, 2018 at 22:02:59 -0400, John Ferlan wrote:
>
>
> On 9/20/18 1:44 PM, Marc Hartmayer wrote:
> > ...although priv->qemuCaps will be NULL in almost every case when the
> > post parse callback has failed. That may change in the future.
> >
> > Signed-off-by: Marc Hartmayer
> > Reviewed-by: Boris Fiuczynski
> > ---
> > src/qemu/qemu_process.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index 44c63c42d618..6c5a6472d8cd 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -5282,7 +5282,7 @@ qemuProcessInit(virQEMUDriverPtr driver,
>
> The comment just above here could use a tweak for grammar ;-):
>
> /* in case when the post parse callback failed we need to re-run it
> on the
> * old config prior we start the VM */
>
> > if (vm->def->postParseFailed) {
> > VIR_DEBUG("re-running the post parse callback");
> >
> > -if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, NULL)
> > < 0)
> > +if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt,
> > priv->qemuCaps) < 0)
>
> Searching through history of this line finds Peter's original commit in
> this area - 7726d158, which seems to indicate a very specific reason for
> providing a NULL capabilities value here.
>
> I think from this patch on is something Peter has worked on a lot, so I
> would prefer to defer to Peter on them since I'm sure he understands all
> the various PostParse and Validate special conditions and various flags
> usage better than I do.
Thanks for notifying me.
In general, when parsing a "new" domain config as it's the case here,
NULL should be passed in.
The qemu driver registers the 'domainPostParseDataAlloc' callback to
qemuDomainPostParseDataAlloc. This callback is executed after the
'basic' post parse callback which fills in the emulator binary.
Passing an explicit qemuCaps is meant only for special cases e.g.
migration where we have a specific set of capabilities which need to be
used rather than a new one.
This patch does not seem to make sense with the justification provided
here as the post-parse callbacks fill in the proper caps here and this
part clearly uses a new domain configuration, so the default behaviour
should be used.
Changing this would also need that we change the normal parser path to
achieve the same results which is impossible as you don't have access to
priv->qemuCaps prior to creating the virDomainObj object.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/11] qemu: Pass QEMUCaps to virDomainDefPostParse
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
> ...although priv->qemuCaps will be NULL in almost every case when the
> post parse callback has failed. That may change in the future.
>
> Signed-off-by: Marc Hartmayer
> Reviewed-by: Boris Fiuczynski
> ---
> src/qemu/qemu_process.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 44c63c42d618..6c5a6472d8cd 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -5282,7 +5282,7 @@ qemuProcessInit(virQEMUDriverPtr driver,
The comment just above here could use a tweak for grammar ;-):
/* in case when the post parse callback failed we need to re-run it
on the
* old config prior we start the VM */
> if (vm->def->postParseFailed) {
> VIR_DEBUG("re-running the post parse callback");
>
> -if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, NULL) <
> 0)
> +if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt,
> priv->qemuCaps) < 0)
Searching through history of this line finds Peter's original commit in
this area - 7726d158, which seems to indicate a very specific reason for
providing a NULL capabilities value here.
I think from this patch on is something Peter has worked on a lot, so I
would prefer to defer to Peter on them since I'm sure he understands all
the various PostParse and Validate special conditions and various flags
usage better than I do.
I'll still provide a few comments along the way.
John
I did CC Peter just to bring his attention to the series...
> goto cleanup;
> }
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 03/11] qemu: Pass QEMUCaps to virDomainDefPostParse
...although priv->qemuCaps will be NULL in almost every case when the
post parse callback has failed. That may change in the future.
Signed-off-by: Marc Hartmayer
Reviewed-by: Boris Fiuczynski
---
src/qemu/qemu_process.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 44c63c42d618..6c5a6472d8cd 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5282,7 +5282,7 @@ qemuProcessInit(virQEMUDriverPtr driver,
if (vm->def->postParseFailed) {
VIR_DEBUG("re-running the post parse callback");
-if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, NULL) < 0)
+if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt,
priv->qemuCaps) < 0)
goto cleanup;
}
--
2.17.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
