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

login
register
mail settings
Submitter Corentin Chary
Date March 10, 2011, 3:13 p.m.
Message ID <1299769982-18815-1-git-send-email-corentin.chary@gmail.com>
Download mbox | patch
Permalink /patch/86297/
State New
Headers show

Comments

Corentin Chary - March 10, 2011, 3:13 p.m.
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@gmail.com>
---
 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(-)
Corentin Chary - March 14, 2011, 9:19 a.m.
On Thu, Mar 10, 2011 at 3:13 PM, Corentin Chary
<corentin.chary@gmail.com> 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@gmail.com>
> ---
>  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 ?
Thanks
Peter Lieven - March 14, 2011, 9:55 a.m.
Am 14.03.2011 um 10:19 schrieb Corentin Chary:

> On Thu, Mar 10, 2011 at 3:13 PM, Corentin Chary
> <corentin.chary@gmail.com> 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@gmail.com>
>> ---
>>  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 ?

will do. but will take until likely take until wednesday.

peter


> Thanks
> 
> -- 
> Corentin Chary
> http://xf.iksaif.net
Peter Lieven - March 15, 2011, 4:55 p.m.
On 14.03.2011 10:19, Corentin Chary wrote:
> On Thu, Mar 10, 2011 at 3:13 PM, Corentin Chary
> <corentin.chary@gmail.com>  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@gmail.com>
>> ---
>>   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
>
Peter Lieven - March 15, 2011, 6:07 p.m.
On 15.03.2011 17:55, Peter Lieven wrote:
> On 14.03.2011 10:19, Corentin Chary wrote:
>> On Thu, Mar 10, 2011 at 3:13 PM, Corentin Chary
>> <corentin.chary@gmail.com>  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@gmail.com>
>>> ---
>>>   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....
commit c53af37f375ce9c4999ff451c51173bdc1167e67
seems to fix this. remember this is not in 0.14.0 which could cause
a lot of trouble to 0.14.0 users when they do a lot of vnc work...

peter



>
> peter
>
>
>> Thanks
>>
>

Patch

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