4:05 p.m.
This is a redesign of the previous patch series [1].
After the reviews of the first version, I ended up discarding
the idea of a qemu user configuration for the unplug timeout
value. Instead, this patch series now handles the problematic
case of PPC64 guests with an exclusive PPC64 only timeout
value of 10 seconds.
Patch 3 now changes how the 'unplug timeout' message is
reported, using VIR_ERR_OPERATION_TIMEOUT instead of
VIR_ERR_OPERATION_FAILED and making it clearer that
the timeout does not equal to unplug failed.
[1] https://www.redhat.com/archives/libvir-list/2019-August/msg00698.html
Daniel Henrique Barboza (3):
qemu: use a bigger unplug timeout for PPC64 guests
qemu: Remove qemu_hotplugpriv.h and qemuDomainRemoveDeviceWaitTime
qemu_hotplug: make setvcpus timeout error message user-friendlier
src/qemu/Makefile.inc.am | 1 -
src/qemu/qemu_conf.h | 3 +++
src/qemu/qemu_driver.c | 11 +++++++++++
src/qemu/qemu_hotplug.c | 21 +++++++++------------
src/qemu/qemu_hotplugpriv.h | 32 --------------------------------
tests/qemuhotplugtest.c | 3 +--
6 files changed, 24 insertions(+), 47 deletions(-)
delete mode 100644 src/qemu/qemu_hotplugpriv.h
--
2.21.0
4:05 p.m.
New subject: [libvirt] [PATCH v2 1/3] qemu: use a bigger unplug timeout for PPC64 guests
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 QEMU driver attribute
'unplugTimeout' was added. The timeout value is set during
qemuStateInitialize only once. All qemu_hotplug.c functions
that uses the timeout have easy access to a qemu_driver object,
thus the change to use unplugTimeout is straightforward.
The now unused 'qemuDomainRemoveDeviceWaitTime' global can't
be simply erased from qemu_hotplug.c though. Next patch will
remove it properly.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/qemu/qemu_conf.h | 3 +++
src/qemu/qemu_driver.c | 11 +++++++++++
src/qemu/qemu_hotplug.c | 11 ++++++-----
tests/qemuhotplugtest.c | 2 +-
4 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 8473d6d4ca..edda1421d0 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -296,6 +296,9 @@ struct _virQEMUDriver {
/* Immutable pointer, self-locking APIs */
virHashAtomicPtr migrationErrors;
+
+ /* Immutable value */
+ unsigned int unplugTimeout;
};
virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1e041a8bac..2ebe22ee79 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -134,6 +134,13 @@ VIR_LOG_INIT("qemu.qemu_driver");
#define QEMU_NB_BANDWIDTH_PARAM 7
+/* 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 5000
+#define QEMU_UNPLUG_TIMEOUT_PPC64 10000
+
static void qemuProcessEventHandler(void *data, void *opaque);
static int qemuStateCleanup(void);
@@ -1095,6 +1102,10 @@ qemuStateInitialize(bool privileged,
if (!qemu_driver->workerPool)
goto error;
+ qemu_driver->unplugTimeout = QEMU_UNPLUG_TIMEOUT;
+ if (ARCH_IS_PPC64(qemu_driver->caps->host.arch))
+ qemu_driver->unplugTimeout = QEMU_UNPLUG_TIMEOUT_PPC64;
+
qemuProcessReconnectAll(qemu_driver);
qemuAutostartDomains(qemu_driver);
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index bd8868b0f7..4088cc5e8e 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5237,7 +5237,8 @@ qemuDomainResetDeviceRemoval(virDomainObjPtr vm)
* - we failed to reliably wait for the event and thus use fallback behavior
*/
static int
-qemuDomainWaitForDeviceRemoval(virDomainObjPtr vm)
+qemuDomainWaitForDeviceRemoval(virQEMUDriverPtr driver,
+ virDomainObjPtr vm)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
unsigned long long until;
@@ -5245,7 +5246,7 @@ qemuDomainWaitForDeviceRemoval(virDomainObjPtr vm)
if (virTimeMillisNow(&until) < 0)
return 1;
- until += qemuDomainRemoveDeviceWaitTime;
+ until += driver->unplugTimeout;
while (priv->unplug.alias) {
if ((rc = virDomainObjWaitUntil(vm, until)) == 1)
@@ -5701,7 +5702,7 @@ qemuDomainDetachDeviceChr(virQEMUDriverPtr driver,
} else if (async) {
ret = 0;
} else {
- if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
+ if ((ret = qemuDomainWaitForDeviceRemoval(driver, vm)) == 1)
ret = qemuDomainRemoveChrDevice(driver, vm, tmpChr, true);
}
@@ -6001,7 +6002,7 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
if (async) {
ret = 0;
} else {
- if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
+ if ((ret = qemuDomainWaitForDeviceRemoval(driver, vm)) == 1)
ret = qemuDomainRemoveDevice(driver, vm, &detach);
}
@@ -6107,7 +6108,7 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver,
goto cleanup;
}
- if ((rc = qemuDomainWaitForDeviceRemoval(vm)) <= 0) {
+ if ((rc = qemuDomainWaitForDeviceRemoval(driver, vm)) <= 0) {
if (rc == 0)
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("vcpu unplug request timed out"));
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index 3c177c6622..da6258991d 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -645,7 +645,7 @@ mymain(void)
driver.hostdevMgr = virHostdevManagerGetDefault();
/* wait only 100ms for DEVICE_DELETED event */
- qemuDomainRemoveDeviceWaitTime = 100;
+ driver.unplugTimeout = 100;
#define DO_TEST(file, ACTION, dev, fial, kep, ...) \
do { \
--
2.21.0
3:15 p.m.
New subject: [libvirt] [PATCH v2 1/3] qemu: use a bigger unplug timeout for PPC64 guests
On 9/11/19 5:05 PM, Daniel Henrique Barboza wrote:
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 QEMU driver attribute
'unplugTimeout' was added. The timeout value is set during
qemuStateInitialize only once. All qemu_hotplug.c functions
that uses the timeout have easy access to a qemu_driver object,
thus the change to use unplugTimeout is straightforward.
The now unused 'qemuDomainRemoveDeviceWaitTime' global can't
be simply erased from qemu_hotplug.c though. Next patch will
remove it properly.
Sorry for the wrong review delay. I see this implements danpb's
suggestion from the previous thread. The implementation seems a little
odd to me though because it is differentiating on host arch, but this is
about guest arch right? And probably an arbitrary number of options,
like I imagine TCG would want a longer timeout too (though that's not
anything you need to deal with)
So I think this should be a function that lives in qemu_hotplug.c and
acts on a DomainDef at least. The test suite will have to mock that in a
qemuhotplugmock.c file, tests/qemucpumock.c is a good example to follow.
If you do that as an upfront patch, it can go in first. Then add the ppc
changes in an add on patch.
You can CC me on the next version and I will review it
Thanks,
Cole
2:54 p.m.
New subject: [libvirt] [PATCH v2 1/3] qemu: use a bigger unplug timeout for PPC64 guests
On 10/9/19 5:15 PM, Cole Robinson wrote:
On 9/11/19 5:05 PM, Daniel Henrique Barboza wrote:
> 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 QEMU driver attribute
> 'unplugTimeout' was added. The timeout value is set during
> qemuStateInitialize only once. All qemu_hotplug.c functions
> that uses the timeout have easy access to a qemu_driver object,
> thus the change to use unplugTimeout is straightforward.
>
> The now unused 'qemuDomainRemoveDeviceWaitTime' global can't
> be simply erased from qemu_hotplug.c though. Next patch will
> remove it properly.
>
Sorry for the wrong review delay. I see this implements danpb's
suggestion from the previous thread. The implementation seems a little
odd to me though because it is differentiating on host arch, but this
is about guest arch right? And probably an arbitrary number of
options, like I imagine TCG would want a longer timeout too (though
that's not anything you need to deal with)
I think it's sensible to say that a TCG guest would always required a
greater unplug timeout than a hardware accelerated one. However, never
thought about considering TCG guests in this patch series though. A shame.
So, considering that TCG guest exists, we can parametrize this unplug
timeout
calculation by considering guest (not host) architecture and if it's a
TCG or
a KVM guest. Speaking about the problem I'm trying to fix with ppc64 and
what we already have, we can roll out this unplug timeout logic like:
- pseries guest on KVM: 10 seconds
- everyone else on KVM: 5 seconds (untouched, like it is today)
For TCG guests, perhaps double the KVM timeout? This would need
experimentation, but double the timeout seems ok at first glance. Then we
can do:
- TCG pseries guest: 10 * 2 = 20 seconds
- TCG with every other arch guest = 5 * 2 = 10 seconds
This TCG calculation can be left alone for now as well - I can create
the API
considering that TCG guest exists, but do not infer the timeout for it.
Either
way works for me.
So I think this should be a function that lives in qemu_hotplug.c and
acts on a DomainDef at least. The test suite will have to mock that in
a qemuhotplugmock.c file, tests/qemucpumock.c is a good example to
follow.
Just to check if we're on the same page, by 'I think this should be a
function'
you mean the calculation of qemuDomainRemoveDeviceWaitTime that I just
mentioned above, right?
If you do that as an upfront patch, it can go in first. Then add the
ppc changes in an add on patch.
Works for me.
Thanks,
DHB
You can CC me on the next version and I will review it
Thanks,
Cole
11:48 a.m.
New subject: [libvirt] [PATCH v2 1/3] qemu: use a bigger unplug timeout for PPC64 guests
On 10/11/19 3:54 PM, Daniel Henrique Barboza wrote:
On 10/9/19 5:15 PM, Cole Robinson wrote:
> On 9/11/19 5:05 PM, Daniel Henrique Barboza wrote:
>> 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 QEMU driver attribute
>> 'unplugTimeout' was added. The timeout value is set during
>> qemuStateInitialize only once. All qemu_hotplug.c functions
>> that uses the timeout have easy access to a qemu_driver object,
>> thus the change to use unplugTimeout is straightforward.
>>
>> The now unused 'qemuDomainRemoveDeviceWaitTime' global can't
>> be simply erased from qemu_hotplug.c though. Next patch will
>> remove it properly.
>>
>
> Sorry for the wrong review delay. I see this implements danpb's
> suggestion from the previous thread. The implementation seems a little
> odd to me though because it is differentiating on host arch, but this
> is about guest arch right? And probably an arbitrary number of
> options, like I imagine TCG would want a longer timeout too (though
> that's not anything you need to deal with)
I think it's sensible to say that a TCG guest would always required a
greater unplug timeout than a hardware accelerated one. However, never
thought about considering TCG guests in this patch series though. A shame.
So, considering that TCG guest exists, we can parametrize this unplug
timeout
calculation by considering guest (not host) architecture and if it's a
TCG or
a KVM guest. Speaking about the problem I'm trying to fix with ppc64 and
what we already have, we can roll out this unplug timeout logic like:
- pseries guest on KVM: 10 seconds
- everyone else on KVM: 5 seconds (untouched, like it is today)
> For TCG guests, perhaps double the KVM timeout? This would need
experimentation, but double the timeout seems ok at first glance. Then we
can do:
- TCG pseries guest: 10 * 2 = 20 seconds
- TCG with every other arch guest = 5 * 2 = 10 seconds
This TCG calculation can be left alone for now as well - I can create
the API
considering that TCG guest exists, but do not infer the timeout for it.
Either
way works for me.
I suggest just fixing the case you care about, leave TCG as it is (the
default 5 seconds), otherwise we may be trying to fix something that no
one cares about in real life.
But I think conceptually the function is a better fit incase the logic
every becomes more complicated than a single host check.
>
> So I think this should be a function that lives in qemu_hotplug.c and
> acts on a DomainDef at least. The test suite will have to mock that in
> a qemuhotplugmock.c file, tests/qemucpumock.c is a good example to
> follow.
Just to check if we're on the same page, by 'I think this should be a
function'
you mean the calculation of qemuDomainRemoveDeviceWaitTime that I just
mentioned above, right?
Yup that's what I meant
Thanks,
Cole
4:05 p.m.
New subject: [libvirt] [PATCH v2 2/3] qemu: Remove qemu_hotplugpriv.h and qemuDomainRemoveDeviceWaitTime
qemu_hotplugpriv.h is a header file created to share a global variable
called 'qemuDomainRemoveDeviceWaitTime', declared in qemu_hotplug.c,
to other files that would want to change the timeout value
(currently, only tests/qemuhotplugtest.c).
Previous patch deprecated the variable, using qemu_driver->unplugTimeout
to set the timeout instead. This means that the header file is now
unused, and can be safely discarded.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/qemu/Makefile.inc.am | 1 -
src/qemu/qemu_hotplug.c | 5 -----
src/qemu/qemu_hotplugpriv.h | 32 --------------------------------
tests/qemuhotplugtest.c | 1 -
4 files changed, 39 deletions(-)
delete mode 100644 src/qemu/qemu_hotplugpriv.h
diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am
index 48fd0332ec..5f2198f6a5 100644
--- a/src/qemu/Makefile.inc.am
+++ b/src/qemu/Makefile.inc.am
@@ -29,7 +29,6 @@ QEMU_DRIVER_SOURCES = \
qemu/qemu_hostdev.h \
qemu/qemu_hotplug.c \
qemu/qemu_hotplug.h \
- qemu/qemu_hotplugpriv.h \
qemu/qemu_conf.c \
qemu/qemu_conf.h \
qemu/qemu_process.c \
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 4088cc5e8e..d3a3e25f08 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -23,8 +23,6 @@
#include <config.h>
#include "qemu_hotplug.h"
-#define LIBVIRT_QEMU_HOTPLUGPRIV_H_ALLOW
-#include "qemu_hotplugpriv.h"
#include "qemu_alias.h"
#include "qemu_capabilities.h"
#include "qemu_domain.h"
@@ -63,9 +61,6 @@ VIR_LOG_INIT("qemu.qemu_hotplug");
#define CHANGE_MEDIA_TIMEOUT 5000
-/* Wait up to 5 seconds for device removal to finish. */
-unsigned long long qemuDomainRemoveDeviceWaitTime = 1000ull * 5;
-
static void
qemuDomainResetDeviceRemoval(virDomainObjPtr vm);
diff --git a/src/qemu/qemu_hotplugpriv.h b/src/qemu/qemu_hotplugpriv.h
deleted file mode 100644
index a5c443ba85..0000000000
--- a/src/qemu/qemu_hotplugpriv.h
+++ /dev/null
@@ -1,32 +0,0 @@
-/*
- * qemu_hotplugpriv.h: private declarations for QEMU device hotplug management
- *
- * Copyright (C) 2013 Red Hat, Inc.
- *
- * 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/>;.
- *
- */
-
-#ifndef LIBVIRT_QEMU_HOTPLUGPRIV_H_ALLOW
-# error "qemu_hotplugpriv.h may only be included by qemu_hotplug.c or test
suites"
-#endif /* LIBVIRT_QEMU_HOTPLUGPRIV_H_ALLOW */
-
-#pragma once
-
-/*
- * This header file should never be used outside unit tests.
- */
-
-extern unsigned long long qemuDomainRemoveDeviceWaitTime;
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index da6258991d..03ff32e37e 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -23,7 +23,6 @@
#include "qemu/qemu_conf.h"
#include "qemu/qemu_hotplug.h"
#define LIBVIRT_QEMU_HOTPLUGPRIV_H_ALLOW
-#include "qemu/qemu_hotplugpriv.h"
#include "qemumonitortestutils.h"
#include "testutils.h"
#include "testutilsqemu.h"
--
2.21.0
4:05 p.m.
New subject: [libvirt] [PATCH v2 3/3] qemu_hotplug: make setvcpus timeout error message user-friendlier
The current 'setvcpus' timeout message requires a deeper
understanding of QEMU/Libvirt internals to proper react to it.
One who knows how setvcpus unplug work (it is an asynchronous
operation between QEMU and guest that Libvirt can't know for
sure if it failed, unless an explicit error happened during the
timeout period) will read the message and not assume a failed
operation. But the regular user, most often than not, will read
it and believe that the unplug operation failed.
This leads to situations where the user isn't exactly relieved
when accessing the guest and seeing that the unplug operation
worked. Instead, the user feel mislead by the timeout message
setvcpus threw.
Changing the timeout message to let the user know that the
unplug status is not known, and manual inspection in the guest
is required, is not a silver bullet. But it gives a more
realistic expectation of what happened, as best as we can tell
from Libvirt side anyways.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/qemu/qemu_hotplug.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index d3a3e25f08..53051807d7 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -6105,8 +6105,9 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver,
if ((rc = qemuDomainWaitForDeviceRemoval(driver, vm)) <= 0) {
if (rc == 0)
- virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("vcpu unplug request timed out"));
+ virReportError(VIR_ERR_OPERATION_TIMEOUT, "%s",
+ _("vcpu unplug request timed out. Unplug result "
+ "must be manually inspected in the domain"));
goto cleanup;
}
--
2.21.0
9:54 a.m.
Ping
On 9/11/19 6:05 PM, Daniel Henrique Barboza wrote:
This is a redesign of the previous patch series [1].
After the reviews of the first version, I ended up discarding
the idea of a qemu user configuration for the unplug timeout
value. Instead, this patch series now handles the problematic
case of PPC64 guests with an exclusive PPC64 only timeout
value of 10 seconds.
Patch 3 now changes how the 'unplug timeout' message is
reported, using VIR_ERR_OPERATION_TIMEOUT instead of
VIR_ERR_OPERATION_FAILED and making it clearer that
the timeout does not equal to unplug failed.
[1] https://www.redhat.com/archives/libvir-list/2019-August/msg00698.html
Daniel Henrique Barboza (3):
qemu: use a bigger unplug timeout for PPC64 guests
qemu: Remove qemu_hotplugpriv.h and qemuDomainRemoveDeviceWaitTime
qemu_hotplug: make setvcpus timeout error message user-friendlier
src/qemu/Makefile.inc.am | 1 -
src/qemu/qemu_conf.h | 3 +++
src/qemu/qemu_driver.c | 11 +++++++++++
src/qemu/qemu_hotplug.c | 21 +++++++++------------
src/qemu/qemu_hotplugpriv.h | 32 --------------------------------
tests/qemuhotplugtest.c | 3 +--
6 files changed, 24 insertions(+), 47 deletions(-)
delete mode 100644 src/qemu/qemu_hotplugpriv.h
1918
days inactive
1951
days old
7 comments
2 participants
participants (2)
-
Cole Robinson -
Daniel Henrique Barboza