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

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?

> > Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; > --- [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?

>       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