[libvirt] [RFC PATCH] udev: Remove udev handle from main loop when udev thread stops

John Ferlan jferlan at redhat.com
Tue Feb 12 20:46:10 UTC 2019



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 at 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;
>          }
> 




More information about the libvir-list mailing list