Re: [PATCH v5] vnc: don't mess up with iohandlers in the vnc thread

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 14.03.2011 10:19, Corentin Chary wrote:
On Thu, Mar 10, 2011 at 3:13 PM, Corentin Chary
<corentin.chary@xxxxxxxxx>  wrote:
The threaded VNC servers messed up with QEMU fd handlers without
any kind of locking, and that can cause some nasty race conditions.

Using qemu_mutex_lock_iothread() won't work because vnc_dpy_cpy(),
which will wait for the current job queue to finish, can be called with
the iothread lock held.

Instead, we now store the data in a temporary buffer, and use a bottom
half to notify the main thread that new data is available.

vnc_[un]lock_ouput() is still needed to access VncState members like
abort, csock or jobs_buffer.

Signed-off-by: Corentin Chary<corentin.chary@xxxxxxxxx>
---
  ui/vnc-jobs-async.c |   48 +++++++++++++++++++++++++++++-------------------
  ui/vnc-jobs.h       |    1 +
  ui/vnc.c            |   12 ++++++++++++
  ui/vnc.h            |    2 ++
  4 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c
index f596247..4438776 100644
--- a/ui/vnc-jobs-async.c
+++ b/ui/vnc-jobs-async.c
@@ -28,6 +28,7 @@

  #include "vnc.h"
  #include "vnc-jobs.h"
+#include "qemu_socket.h"

  /*
  * Locking:
@@ -155,6 +156,24 @@ void vnc_jobs_join(VncState *vs)
         qemu_cond_wait(&queue->cond,&queue->mutex);
     }
     vnc_unlock_queue(queue);
+    vnc_jobs_consume_buffer(vs);
+}
+
+void vnc_jobs_consume_buffer(VncState *vs)
+{
+    bool flush;
+
+    vnc_lock_output(vs);
+    if (vs->jobs_buffer.offset) {
+        vnc_write(vs, vs->jobs_buffer.buffer, vs->jobs_buffer.offset);
+        buffer_reset(&vs->jobs_buffer);
+    }
+    flush = vs->csock != -1&&  vs->abort != true;
+    vnc_unlock_output(vs);
+
+    if (flush) {
+      vnc_flush(vs);
+    }
  }

  /*
@@ -197,7 +216,6 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
     VncState vs;
     int n_rectangles;
     int saved_offset;
-    bool flush;

     vnc_lock_queue(queue);
     while (QTAILQ_EMPTY(&queue->jobs)&&  !queue->exit) {
@@ -213,6 +231,7 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)

     vnc_lock_output(job->vs);
     if (job->vs->csock == -1 || job->vs->abort == true) {
+        vnc_unlock_output(job->vs);
         goto disconnected;
     }
     vnc_unlock_output(job->vs);
@@ -233,10 +252,6 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)

         if (job->vs->csock == -1) {
             vnc_unlock_display(job->vs->vd);
-            /* output mutex must be locked before going to
-             * disconnected:
-             */
-            vnc_lock_output(job->vs);
             goto disconnected;
         }

@@ -254,24 +269,19 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
     vs.output.buffer[saved_offset] = (n_rectangles>>  8)&  0xFF;
     vs.output.buffer[saved_offset + 1] = n_rectangles&  0xFF;

-    /* Switch back buffers */
     vnc_lock_output(job->vs);
-    if (job->vs->csock == -1) {
-        goto disconnected;
+    if (job->vs->csock != -1) {
+        buffer_reserve(&job->vs->jobs_buffer, vs.output.offset);
+        buffer_append(&job->vs->jobs_buffer, vs.output.buffer,
+                      vs.output.offset);
+        /* Copy persistent encoding data */
+        vnc_async_encoding_end(job->vs,&vs);
+
+       qemu_bh_schedule(job->vs->bh);
     }
-
-    vnc_write(job->vs, vs.output.buffer, vs.output.offset);
-
-disconnected:
-    /* Copy persistent encoding data */
-    vnc_async_encoding_end(job->vs,&vs);
-    flush = (job->vs->csock != -1&&  job->vs->abort != true);
     vnc_unlock_output(job->vs);

-    if (flush) {
-        vnc_flush(job->vs);
-    }
-
+disconnected:
     vnc_lock_queue(queue);
     QTAILQ_REMOVE(&queue->jobs, job, next);
     vnc_unlock_queue(queue);
diff --git a/ui/vnc-jobs.h b/ui/vnc-jobs.h
index b8dab81..4c661f9 100644
--- a/ui/vnc-jobs.h
+++ b/ui/vnc-jobs.h
@@ -40,6 +40,7 @@ void vnc_jobs_join(VncState *vs);

  #ifdef CONFIG_VNC_THREAD

+void vnc_jobs_consume_buffer(VncState *vs);
  void vnc_start_worker_thread(void);
  bool vnc_worker_thread_running(void);
  void vnc_stop_worker_thread(void);
diff --git a/ui/vnc.c b/ui/vnc.c
index 610f884..f28873b 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1011,7 +1011,10 @@ static void vnc_disconnect_finish(VncState *vs)

  #ifdef CONFIG_VNC_THREAD
     qemu_mutex_destroy(&vs->output_mutex);
+    qemu_bh_delete(vs->bh);
+    buffer_free(&vs->jobs_buffer);
  #endif
+
     for (i = 0; i<  VNC_STAT_ROWS; ++i) {
         qemu_free(vs->lossy_rect[i]);
     }
@@ -1226,6 +1229,14 @@ static long vnc_client_read_plain(VncState *vs)
     return ret;
  }

+#ifdef CONFIG_VNC_THREAD
+static void vnc_jobs_bh(void *opaque)
+{
+    VncState *vs = opaque;
+
+    vnc_jobs_consume_buffer(vs);
+}
+#endif

  /*
  * First function called whenever there is more data to be read from
@@ -2525,6 +2536,7 @@ static void vnc_connect(VncDisplay *vd, int csock)

  #ifdef CONFIG_VNC_THREAD
     qemu_mutex_init(&vs->output_mutex);
+    vs->bh = qemu_bh_new(vnc_jobs_bh, vs);
  #endif

     QTAILQ_INSERT_HEAD(&vd->clients, vs, next);
diff --git a/ui/vnc.h b/ui/vnc.h
index 8a1e7b9..bca5f87 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -283,6 +283,8 @@ struct VncState
     VncJob job;
  #else
     QemuMutex output_mutex;
+    QEMUBH *bh;
+    Buffer jobs_buffer;
  #endif

     /* Encoding specific, if you add something here, don't forget to
--
1.7.3.4


Paolo, Anthony, Jan, are you ok with that one ?
Peter Lieve, could you test that patch ?
i have seen one segfault. unfortunatly, i had no debugger attached.

but whats equally worse during stress test, i managed to trigger oom-killer.
it seems we have a memory leak somewhere....

peter


Thanks


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux