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&gt; > --- > > 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; > } >