5:36 a.m.
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
<pkrempa(a)redhat.com> 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 <mhartmay(a)linux.ibm.com>
>> > Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
>> > ---
>> > 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
domain all the time.
What’s your opinion about that?
I did not touch the domain obj list for quite some time so I don't have
any guidance on whether its possible/feasible to untangle it.