Re: [PATCH 07/10] qemu: exit thread synchronously in qemuDomainObjStopWorker

[Nikolay Shirokovskiy]

On 21.07.2020 19:09, Daniel P. Berrangé wrote:
> On Tue, Jul 14, 2020 at 12:32:58PM +0300, Nikolay Shirokovskiy wrote:
>> The change won't hurt much current callers performance I guess and now we can
>> use the function when we need to be sure of synchronous thread exit as well.
> 
> I can't remember the exact scenario, but I'm reasonably sure i tried
> this approach when adding the event thread support and hit scenario
> where this would deadlock.
> 
>>
>> Signed-off-by: Nikolay Shirokovskiy 
>> ---
>>  src/qemu/qemu_domain.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 2d9d822..18651d0 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -1571,6 +1571,7 @@ qemuDomainObjStopWorker(virDomainObjPtr dom)
>>  qemuDomainObjPrivatePtr priv = dom->privateData;
>>  
>>  if (priv->eventThread) {
>> +virEventThreadClose(priv->eventThread);
>>  g_object_unref(priv->eventThread);
> 
> IIRC, it was something like this unref does not always release the
> last reference. 
> 
>>  priv->eventThread = NULL;
> 
> IOW, despite setting evnetThread to NULL, the thread may linger for
> a short while longer in the background until any operations have
> completed.
> 

Yeah, we can not stop event loop thread synchronously when call 
qemuDomainObjStopWorker from qemuProcessHandleMonitorEOF as
the latter is called from event loop.

Hmm, pthread_join will fail in this case:

   EDEADLK
  A deadlock was detected (e.g., two threads tried to  join  with  
each
  other); or thread specifies the calling thread.

Anyway, make sense calling virEventThreadClose optional.


Nikolay

Re: [PATCH 07/10] qemu: exit thread synchronously in qemuDomainObjStopWorker

[Daniel P . Berrangé]

On Tue, Jul 14, 2020 at 12:32:58PM +0300, Nikolay Shirokovskiy wrote:
> The change won't hurt much current callers performance I guess and now we can
> use the function when we need to be sure of synchronous thread exit as well.

I can't remember the exact scenario, but I'm reasonably sure i tried
this approach when adding the event thread support and hit scenario
where this would deadlock.

> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/qemu/qemu_domain.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 2d9d822..18651d0 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1571,6 +1571,7 @@ qemuDomainObjStopWorker(virDomainObjPtr dom)
>  qemuDomainObjPrivatePtr priv = dom->privateData;
>  
>  if (priv->eventThread) {
> +virEventThreadClose(priv->eventThread);
>  g_object_unref(priv->eventThread);

IIRC, it was something like this unref does not always release the
last reference. 

>  priv->eventThread = NULL;

IOW, despite setting evnetThread to NULL, the thread may linger for
a short while longer in the background until any operations have
completed.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

[PATCH 07/10] qemu: exit thread synchronously in qemuDomainObjStopWorker

[Nikolay Shirokovskiy]

The change won't hurt much current callers performance I guess and now we can
use the function when we need to be sure of synchronous thread exit as well.

Signed-off-by: Nikolay Shirokovskiy 
---
 src/qemu/qemu_domain.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2d9d822..18651d0 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1571,6 +1571,7 @@ qemuDomainObjStopWorker(virDomainObjPtr dom)
 qemuDomainObjPrivatePtr priv = dom->privateData;
 
 if (priv->eventThread) {
+virEventThreadClose(priv->eventThread);
 g_object_unref(priv->eventThread);
 priv->eventThread = NULL;
 }
-- 
1.8.3.1