From: Marc-André Lureau <[email protected]> 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. Reviewed-by: Daniel P. Berrangé <[email protected]> Signed-off-by: Marc-André Lureau <[email protected]> --- ui/vnc-jobs.h | 3 ++- ui/vnc.h | 2 ++ ui/vnc-jobs.c | 62 +++++++++++++++++++++++++++++++++------------------ ui/vnc.c | 3 ++- 4 files changed, 46 insertions(+), 24 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 472a55f7b5f..780fd39469f 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..90b68bf4cb9 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,12 +116,15 @@ 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); @@ -139,6 +133,7 @@ void vnc_job_push(VncJob *job) static bool vnc_has_job_locked(VncState *vs) { + VncJobQueue *queue = vs->vd->queue; VncJob *job; QTAILQ_FOREACH(job, &queue->jobs, next) { @@ -151,6 +146,8 @@ 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)) { qemu_cond_wait(&queue->cond, &queue->mutex); @@ -252,9 +249,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,7 +341,7 @@ disconnected: return 0; } -static VncJobQueue *vnc_queue_init(void) +static VncJobQueue *vnc_queue_new(void) { VncJobQueue *queue = g_new0(VncJobQueue, 1); @@ -350,29 +351,46 @@ static VncJobQueue *vnc_queue_init(void) return queue; } +static void vnc_queue_free(VncJobQueue *queue) +{ + qemu_cond_destroy(&queue->cond); + qemu_mutex_destroy(&queue->mutex); + g_free(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 */ + assert(vd->queue == NULL); + + vd->queue = vnc_queue_new(); + qemu_thread_create(&vd->queue->thread, "vnc_worker", vnc_worker_thread, vd->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); + g_clear_pointer(&vd->queue, vnc_queue_free); } diff --git a/ui/vnc.c b/ui/vnc.c index c87d1f61a0a..3a908670ab9 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); @@ -3513,6 +3513,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.54.0
