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

[Jonathon Jongsma <jjongsma@xxxxxxxxxx>]

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@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?

>       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