Re: [PATCH 07/10] qemu: exit thread synchronously in qemuDomainObjStopWorker
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
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
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