[PATCH v2 46/67] ui/vnc: make the worker thread per-VncDisplay

Marc-André Lureau posted 67 patches 17 hours ago
Maintainers: John Snow <jsnow@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, Mauro Carvalho Chehab <mchehab+huawei@kernel.org>, Pierrick Bouvier <pierrick.bouvier@linaro.org>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Jan Kiszka <jan.kiszka@web.de>, Phil Dennis-Jordan <phil@philjordan.eu>, Richard Henderson <richard.henderson@linaro.org>, Helge Deller <deller@gmx.de>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Gerd Hoffmann <kraxel@redhat.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Samuel Tardieu <sam@rfc1149.net>, "Hervé Poussineau" <hpoussin@reactos.org>, Aleksandar Rikalo <arikalo@gmail.com>, Laurent Vivier <laurent@vivier.eu>, Thomas Huth <th.huth+qemu@posteo.eu>, BALATON Zoltan <balaton@eik.bme.hu>, "Michael S. Tsirkin" <mst@redhat.com>, Stefano Garzarella <sgarzare@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>, Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>, Dmitry Osipenko <dmitry.osipenko@collabora.com>, Dmitry Fleytman <dmitry.fleytman@gmail.com>, Stefano Stabellini <sstabellini@kernel.org>, Anthony PERARD <anthony@xenproject.org>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Alistair Francis <alistair@alistair23.me>, Alex Williamson <alex@shazbot.org>, "Cédric Le Goater" <clg@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Fabiano Rosas <farosas@suse.de>
[PATCH v2 46/67] ui/vnc: make the worker thread per-VncDisplay
Posted by Marc-André Lureau 17 hours ago
The VNC encoding worker thread was using a single global queue shared
across all VNC displays, with no way to stop it. This made it impossible
to properly clean up resources when a VncDisplay is freed.

Move the VncJobQueue from a file-scoped global to a per-VncDisplay
member, so each display owns its worker thread and queue. Add
vnc_stop_worker_thread() to perform an orderly shutdown: signal the
thread to exit, join it, and destroy the queue. The thread is now
created as QEMU_THREAD_JOINABLE instead of QEMU_THREAD_DETACHED.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/vnc-jobs.h |  3 ++-
 ui/vnc.h      |  2 ++
 ui/vnc-jobs.c | 76 +++++++++++++++++++++++++++++++++--------------------------
 ui/vnc.c      |  3 ++-
 4 files changed, 49 insertions(+), 35 deletions(-)

diff --git a/ui/vnc-jobs.h b/ui/vnc-jobs.h
index 59f66bcc353..e5ab55c1da6 100644
--- a/ui/vnc-jobs.h
+++ b/ui/vnc-jobs.h
@@ -37,7 +37,8 @@ void vnc_job_push(VncJob *job);
 void vnc_jobs_join(VncState *vs);
 
 void vnc_jobs_consume_buffer(VncState *vs);
-void vnc_start_worker_thread(void);
+void vnc_start_worker_thread(VncDisplay *vd);
+void vnc_stop_worker_thread(VncDisplay *vd);
 
 /* Locks */
 static inline int vnc_trylock_display(VncDisplay *vd)
diff --git a/ui/vnc.h b/ui/vnc.h
index 110c2bd4600..9a09fcdad8b 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -62,6 +62,7 @@
 
 typedef struct VncState VncState;
 typedef struct VncJob VncJob;
+typedef struct VncJobQueue VncJobQueue;
 typedef struct VncRect VncRect;
 typedef struct VncRectEntry VncRectEntry;
 
@@ -158,6 +159,7 @@ struct VncDisplay
     int ledstate;
     QKbdState *kbd;
     QemuMutex mutex;
+    VncJobQueue *queue;
 
     int cursor_msize;
     uint8_t *cursor_mask;
diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
index 5b17ef54091..c809287dd3a 100644
--- a/ui/vnc-jobs.c
+++ b/ui/vnc-jobs.c
@@ -29,8 +29,6 @@
 #include "qemu/osdep.h"
 #include "vnc.h"
 #include "vnc-jobs.h"
-#include "qemu/sockets.h"
-#include "qemu/main-loop.h"
 #include "trace.h"
 
 /*
@@ -56,17 +54,10 @@ struct VncJobQueue {
     QemuCond cond;
     QemuMutex mutex;
     QemuThread thread;
+    bool exit;
     QTAILQ_HEAD(, VncJob) jobs;
 };
 
-typedef struct VncJobQueue VncJobQueue;
-
-/*
- * We use a single global queue, but most of the functions are
- * already reentrant, so we can easily add more than one encoding thread
- */
-static VncJobQueue *queue;
-
 static void vnc_lock_queue(VncJobQueue *queue)
 {
     qemu_mutex_lock(&queue->mutex);
@@ -125,19 +116,22 @@ static void vnc_job_free(VncJob *job)
  */
 void vnc_job_push(VncJob *job)
 {
+    VncJobQueue *queue = job->vs->vd->queue;
+
     assert(!QTAILQ_IN_USE(job, next));
 
     if (QLIST_EMPTY(&job->rectangles)) {
         vnc_job_free(job);
     } else {
         vnc_lock_queue(queue);
+        assert(!queue->exit);
         QTAILQ_INSERT_TAIL(&queue->jobs, job, next);
         qemu_cond_broadcast(&queue->cond);
         vnc_unlock_queue(queue);
     }
 }
 
-static bool vnc_has_job_locked(VncState *vs)
+static bool vnc_has_job_locked(VncJobQueue *queue, VncState *vs)
 {
     VncJob *job;
 
@@ -151,8 +145,10 @@ static bool vnc_has_job_locked(VncState *vs)
 
 void vnc_jobs_join(VncState *vs)
 {
+    VncJobQueue *queue = vs->vd->queue;
+
     vnc_lock_queue(queue);
-    while (vnc_has_job_locked(vs)) {
+    while (vnc_has_job_locked(queue, vs)) {
         qemu_cond_wait(&queue->cond, &queue->mutex);
     }
     vnc_unlock_queue(queue);
@@ -252,9 +248,13 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
     int saved_offset;
 
     vnc_lock_queue(queue);
-    while (QTAILQ_EMPTY(&queue->jobs)) {
+    while (QTAILQ_EMPTY(&queue->jobs) && !queue->exit) {
         qemu_cond_wait(&queue->cond, &queue->mutex);
     }
+    if (queue->exit) {
+        vnc_unlock_queue(queue);
+        return 1;
+    }
     job = QTAILQ_FIRST(&queue->jobs);
     vnc_unlock_queue(queue);
 
@@ -340,39 +340,49 @@ disconnected:
     return 0;
 }
 
-static VncJobQueue *vnc_queue_init(void)
-{
-    VncJobQueue *queue = g_new0(VncJobQueue, 1);
-
-    qemu_cond_init(&queue->cond);
-    qemu_mutex_init(&queue->mutex);
-    QTAILQ_INIT(&queue->jobs);
-    return queue;
-}
-
 static void *vnc_worker_thread(void *arg)
 {
     VncJobQueue *queue = arg;
 
     while (!vnc_worker_thread_loop(queue)) ;
-    g_assert_not_reached();
+
     return NULL;
 }
 
-static bool vnc_worker_thread_running(void)
+void vnc_start_worker_thread(VncDisplay *vd)
 {
-    return queue; /* Check global queue */
+    VncJobQueue *queue;
+
+    assert(vd->queue == NULL);
+
+    queue = g_new0(VncJobQueue, 1);
+    qemu_cond_init(&queue->cond);
+    qemu_mutex_init(&queue->mutex);
+    QTAILQ_INIT(&queue->jobs);
+    vd->queue = queue;
+
+    qemu_thread_create(&queue->thread, "vnc_worker", vnc_worker_thread, queue,
+                       QEMU_THREAD_JOINABLE);
 }
 
-void vnc_start_worker_thread(void)
+void vnc_stop_worker_thread(VncDisplay *vd)
 {
-    VncJobQueue *q;
+    VncJobQueue *queue = vd->queue;
 
-    if (vnc_worker_thread_running())
+    if (!queue) {
         return;
+    }
+
+    /* all VNC clients must have finished before we can stop the worker thread */
+    vnc_lock_queue(queue);
+    assert(QTAILQ_EMPTY(&queue->jobs));
+    queue->exit = true;
+    qemu_cond_broadcast(&queue->cond);
+    vnc_unlock_queue(queue);
 
-    q = vnc_queue_init();
-    qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q,
-                       QEMU_THREAD_DETACHED);
-    queue = q; /* Set global queue */
+    qemu_thread_join(&queue->thread);
+    qemu_cond_destroy(&queue->cond);
+    qemu_mutex_destroy(&queue->mutex);
+    g_free(queue);
+    vd->queue = NULL;
 }
diff --git a/ui/vnc.c b/ui/vnc.c
index ba7376360e6..4e5a9ee0341 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3457,7 +3457,7 @@ void vnc_display_init(const char *id, Error **errp)
     vd->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
     vd->connections_limit = 32;
 
-    vnc_start_worker_thread();
+    vnc_start_worker_thread(vd);
 
     register_displaychangelistener(&vd->dcl);
     vd->kbd = qkbd_state_init(vd->dcl.con);
@@ -3517,6 +3517,7 @@ static void vnc_display_free(VncDisplay *vd)
 
     assert(QTAILQ_EMPTY(&vd->clients));
 
+    vnc_stop_worker_thread(vd);
     vnc_display_close(vd);
     unregister_displaychangelistener(&vd->dcl);
     qkbd_state_free(vd->kbd);

-- 
2.53.0