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>
---
[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