Re: [libvirt] [RFC PATCH] udev: Remove udev handle from main loop when udev thread stops
On 2/7/19 11:08 AM, Marc Hartmayer wrote:
Commit "nodedev: Move device enumumeration out of
nodeStateInitialize"
(9f0ae0b18e3e620) has moved the heavy task of device enumeration into
a separate thread. The problem with this commit is that there is a
functionality change in the cleanup when udevEnumerateDevices
fails. Before commit 9f0ae0b18e3e620, the entire driver was cleaned up
by calling nodeStateCleanup (defined in node_device_udev.c) which
resulted in libvirtd stopping with the error message
'daemonRunStateInit:800 : Driver state initialization failed'. With
the commit 9f0ae0b18e3e620 there is only a signal to the udev thread
that it must stop. This means that for example the watch handle isn't
removed from the main loop and this can result in the main loop
consuming 100% CPU time as soon as a new udev event occurs.
This patch proposes a simple solution to the described problem. In
case the udev thread stops the watch handle is removed from the main
loop.
Fixes: 9f0ae0b18e3e620 ("nodedev: Move device enumumeration out of
nodeStateInitialize")
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
---
Note: I'm not sure whether we should stop libvirtd (as it would have
been done before) or if this patch is already sufficient.
---
src/node_device/node_device_udev.c | 7 +++++++
1 file changed, 7 insertions(+)
What you have seems reasonable - although I wonder if it would be better
to remove the event handle in the error of nodeStateInitializeEnumerate.
I think the assumption made was that by setting threadQuit that the next
event have some sort of failure through udevEventMonitorSanityCheck
which removes the EventHandle too. Although I cannot be sure it's been
too long to remember for sure ;-)
Furthermore, should nodeStateCleanup be altered to check for priv->watch
== -1 and thus not worry about the code to set threadQuit, signal, and
joining the thread.
John
BTW: I'm subscribed to the list, no need to CC...
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index b1e5f00067e8..299f55260129 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1628,6 +1628,13 @@ udevEventHandleThread(void *opaque ATTRIBUTE_UNUSED)
}
if (priv->threadQuit) {
+ if (priv->watch != -1) {
+ /* Since the udev thread getting stopped remove the
+ * watch handle from the main loop */
+ virEventRemoveHandle(priv->watch);
+ priv->watch = -1;
+ }
+
virObjectUnlock(priv);
return;
}