Re: [libvirt PATCH v2 4/4] qemu: enable asynchronous teardown on s390x hosts

[Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>]

On 6/28/23 10:26 PM, Jonathon Jongsma wrote:
> On 6/27/23 10:51 AM, Boris Fiuczynski wrote:
> > Enablement of asynchronous teardown on S390 and add tests for
> > asynchronous teardown autogeneration support.
>
> I don't know all of the implications of enabling vs not enabling this 
> feature. It sounds like it speeds up shutdown significantly in some 
> situations. But at minimum I would expect that the commit log would have 
> a thorough justification for why the default behavior for s390 will 
> change to use this new approach. Why does s390 require this feature to 
> be the default and not any other architecture? The qemu commit log for 
> this feature indicates that it was intended to speed up shutdown and 
> cleanup of huge vms, and particularly protected vms. It mentioned that 
> this is often the case on s390x, but is CPU architecture really the best 
> deciding factor for enabling this feature? Any existing s390 domain that 
> was defined on a previous version of libvirt will automatically change 
> to async-teardown after upgrading libvirt. That seems potentially risky. 
> What are the potential downsides?

I will add an explanation into the commit message.

> > Signed-off-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>
> > ---
>
> [snip]
>
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 8f77e8fc58..8ed3283546 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -4081,6 +4081,7 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver 
> > *driver,
> >       bool addDefaultUSBMouse = false;
> >       bool addPanicDevice = false;
> >       bool addITCOWatchdog = false;
> > +    bool addAsyncTeardown = false;
> >       /* add implicit input devices */
> >       if (qemuDomainDefAddImplicitInputDevice(def) < 0)
> > @@ -4164,6 +4165,7 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver 
> > *driver,
> >           addDefaultUSB = false;
> >           addPanicDevice = true;
> >           addPCIRoot = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI);
> > +        addAsyncTeardown = virQEMUCapsGet(qemuCaps, 
> > QEMU_CAPS_RUN_WITH_ASYNC_TEARDOWN);
> >           break;
> >       case VIR_ARCH_SPARC:
> > @@ -4334,6 +4336,11 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver 
> > *driver,
> >           }
> >       }
> > +    if (addAsyncTeardown &&
> > +        def->features[VIR_DOMAIN_FEATURE_ASYNC_TEARDOWN] == 
> > VIR_TRISTATE_BOOL_ABSENT)
> > +        def->features[VIR_DOMAIN_FEATURE_ASYNC_TEARDOWN] = 
> > VIR_TRISTATE_BOOL_YES;
> > +
> > +
>
> Wouldn't qemuDomainDefEnableDefaultFeatures() be a better location to 
> put this?

You are correct. I will move the code into that method and include a 
comment with reasoning why this feature is enabled on S390 hosts by default.

> >       if (qemuDomainDefAddDefaultAudioBackend(driver, def) < 0)
> >           return -1;
> > @@ -6694,6 +6701,13 @@ qemuDomainDefFormatBufInternal(virQEMUDriver 
> > *driver,
> >                   }
> >               }
> >           }
> > +
> > +        /* Remove asynchronous teardown enablement for backwards 
> > compatibility
> > +         * on S390 as it gets autogenerated on S390 if supported anyway.
> > +         */
> > +        if (ARCH_IS_S390(def->os.arch) &&
> > +            def->features[VIR_DOMAIN_FEATURE_ASYNC_TEARDOWN] != 
> > VIR_TRISTATE_BOOL_YES)
> > +            def->features[VIR_DOMAIN_FEATURE_ASYNC_TEARDOWN] = 
> > VIR_TRISTATE_BOOL_ABSENT;
> >       }
>
>
> Jonathon


--
Mit freundlichen Grüßen/Kind regards
   Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294