On Tue, Feb 12, 2019 at 09:46 PM +0100, John Ferlan <jferlan@xxxxxxxxxx> wrote:
> 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@xxxxxxxxxxxxx>
>> ---
>>
>> 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(+)
>>
>
[…snip…]
> 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.
Hmm, I looked at the code again and it seems like your suggested change
could be a small performance improvement but it makes the code not more
readable. The current code is not wrong/problematic because changing
`priv->threadQuit` to true changes nothing. virCondSignal doesn’t block
and virThreadJoin/pthread_join returns immediately if the passed thread
has already terminated
(http://man7.org/linux/man-pages/man3/pthread_join.3.html).
The join would also still be needed with your proposed change since
otherwise there would be a (possible) race condition between
nodeStateCleanup() and setting `priv->threadQuit`.
>
> 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;
>> }
>>
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
