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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux