qemu_hotplug.c: adding qemuDomainGetUnplugTimeout

For some architectures and setups, device removal can take
longer than the default 5 seconds. This results in commands
such as 'virsh setvcpus' to fire timeout messages even if
the operation were successful in the guest, confusing the
user.

This patch sets a new 10 seconds unplug timeout for PPC64
guests. All other archs will keep the default 5 seconds
timeout.

Instead of putting 'if PPC64' conditionals inside qemu_hotplug.c
to set the new timeout value, a new function called
qemuDomainGetUnplugTimeout was added. The timeout value is then
retrieved when needed, by passing the correspondent DomainDef
object. This approach allows for different guest architectures
to have distint unplug timeout intervals, regardless of the
host architecture. This design also makes it easier to
modify/enhance the unplug timeout logic in the future
(allow for special timeouts for TCG domains, for example).

A new mock file was created to work with qemuhotplugtest.c,
given that the test timeout is significantly shorter than
the actual timeout value in qemu_hotplug.c.

The now unused 'qemuDomainRemoveDeviceWaitTime' global can't
be simply erased from qemu_hotplug.c though. Next patch will
remove it properly.

Reviewed-by: Cole Robinson <crobinso@redhat.com>
Suggested-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 811ad952c537f36b036fdbc4d5ddaa31e7884580..4e2921d113eca903e76b6e339768b958699470b1 100644 (file)
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -63,6 +63,13 @@ VIR_LOG_INIT("qemu.qemu_hotplug");
 
 #define CHANGE_MEDIA_TIMEOUT 5000
 
+/* Timeout in miliseconds for device removal. PPC64 domains
+ * can experience a bigger delay in unplug operations during
+ * heavy guest activity (vcpu being the most notable case), thus
+ * the timeout for PPC64 is also bigger. */
+#define QEMU_UNPLUG_TIMEOUT 1000ull * 5
+#define QEMU_UNPLUG_TIMEOUT_PPC64 1000ull * 10
+
 /* Wait up to 5 seconds for device removal to finish. */
 unsigned long long qemuDomainRemoveDeviceWaitTime = 1000ull * 5;
 
@@ -5094,6 +5101,17 @@ qemuDomainResetDeviceRemoval(virDomainObjPtr vm)
     priv->unplug.eventSeen = false;
 }
 
+
+unsigned long long
+qemuDomainGetUnplugTimeout(virDomainObjPtr vm)
+{
+    if (qemuDomainIsPSeries(vm->def))
+        return QEMU_UNPLUG_TIMEOUT_PPC64;
+
+    return QEMU_UNPLUG_TIMEOUT;
+}
+
+
 /* Returns:
  *  -1 Unplug of the device failed
  *
@@ -5112,7 +5130,7 @@ qemuDomainWaitForDeviceRemoval(virDomainObjPtr vm)
 
     if (virTimeMillisNow(&until) < 0)
         return 1;
-    until += qemuDomainRemoveDeviceWaitTime;
+    until += qemuDomainGetUnplugTimeout(vm);
 
     while (priv->unplug.alias) {
         if ((rc = virDomainObjWaitUntil(vm, until)) == 1)

diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
index 6d2cd34dbc984083d4252b2eb7091163c53dcba0..1dfc601110d3729f04f0a07683535612b9f0168d 100644 (file)
--- a/src/qemu/qemu_hotplug.h
+++ b/src/qemu/qemu_hotplug.h
@@ -161,3 +161,5 @@ int qemuDomainDetachDBusVMState(virQEMUDriverPtr driver,
                                 virDomainObjPtr vm,
                                 const char *id,
                                 qemuDomainAsyncJob asyncJob);
+
+unsigned long long qemuDomainGetUnplugTimeout(virDomainObjPtr vm);

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 9d9c582e428daf18b1b10b3fd2c918250dd83eb3..e009de830c2b1bb39004bb1fe5fc6aacf99800b0 100644 (file)
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -224,6 +224,7 @@ test_libraries = libshunload.la \
        libvirhostcpumock.la \
        libdomaincapsmock.la \
        libvirfilecachemock.la \
+       libqemuhotplugmock.la \
        $(NULL)
 
 if WITH_REMOTE
@@ -566,6 +567,11 @@ libqemucpumock_la_SOURCES = \
 libqemucpumock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
 libqemucpumock_la_LIBADD = $(MOCKLIBS_LIBS)
 
+libqemuhotplugmock_la_SOURCES = \
+       qemuhotplugmock.c
+libqemuhotplugmock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
+libqemuhotplugmock_la_LIBADD = $(MOCKLIBS_LIBS)
+
 qemuxml2argvtest_SOURCES = \
        qemuxml2argvtest.c testutilsqemu.c testutilsqemu.h \
        testutils.c testutils.h \
@@ -643,7 +649,11 @@ qemuhotplugtest_SOURCES = \
        testutils.c testutils.h \
        testutilsqemu.c testutilsqemu.h \
        $(NULL)
-qemuhotplugtest_LDADD = libqemumonitortestutils.la $(qemu_LDADDS)
+qemuhotplugtest_LDADD = \
+       libqemutestdriver.la \
+       libqemumonitortestutils.la \
+       $(qemu_LDADDS) \
+       $(NULL)
 
 qemublocktest_SOURCES = \
        qemublocktest.c \
@@ -717,6 +727,7 @@ EXTRA_DIST += qemuxml2argvtest.c qemuxml2xmltest.c \
        qemusecuritymock.c \
        qemufirmwaretest.c \
        qemuvhostusertest.c \
+       qemuhotplugmock.c \
        $(QEMUMONITORTESTUTILS_SOURCES)
 endif ! WITH_QEMU
 

diff --git a/tests/qemuhotplugmock.c b/tests/qemuhotplugmock.c
new file mode 100644 (file)
index 0000000..43a9d79
--- /dev/null
+++ b/tests/qemuhotplugmock.c
@@ -0,0 +1,33 @@
+/*
+ * Copyright (C) 2019 IBM Corporation
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * <http://www.gnu.org/licenses/>.
+ */
+
+#include <config.h>
+
+#include "qemu/qemu_hotplug.h"
+#include "conf/domain_conf.h"
+
+unsigned long long
+qemuDomainGetUnplugTimeout(virDomainObjPtr vm G_GNUC_UNUSED)
+{
+    /* Wait only 100ms for DEVICE_DELETED event. Give a greater
+     * timeout in case of PSeries guest to be consistent with the
+     * original logic. */
+    if (qemuDomainIsPSeries(vm->def))
+        return 200;
+    return 100;
+}

diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index a5e6c0135c31a0306c47e544268dc72fd50b1be3..a23fa3e37d1aa6c24d255de90b1304c35f32c197 100644 (file)
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -879,4 +879,5 @@ mymain(void)
 
 VIR_TEST_MAIN_PRELOAD(mymain,
                       VIR_TEST_MOCK("virpci"),
-                      VIR_TEST_MOCK("virprocess"));
+                      VIR_TEST_MOCK("virprocess"),
+                      VIR_TEST_MOCK("qemuhotplug"));