[Qemu-devel] [PATCH] server: don't call reds_stream_free from worker thread context

[Qemu-devel] [PATCH] server: don't call reds_stream_free from worker thread context

[Gerd Hoffmann]

reds_stream_free() may call the channel_event callback which is not
supposed to be callsed from worker thread context.  This patch moves
the reds_stream_free call for the display channel from the worker to
the dispatcher to fix this issue.

[ Note: not tested yet, against 0.8 branch, sending out for review &
        comments nevertheless ]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 server/red_dispatcher.c |    5 +++++
 server/red_worker.c     |    3 +--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
index f74b13e..801a575 100644
--- a/server/red_dispatcher.c
+++ b/server/red_dispatcher.c
@@ -51,6 +51,7 @@ struct RedDispatcher {
     int y_res;
     int use_hardware_cursor;
     RedDispatcher *next;
+    RedsStream *stream;
     RedWorkerMessage async_message;
     pthread_mutex_t  async_lock;
     QXLDevSurfaceCreate surface_create;
@@ -81,6 +82,7 @@ static void red_dispatcher_set_peer(Channel *channel, RedsStream *stream, int mi
 
     red_printf("");
     dispatcher = (RedDispatcher *)channel->data;
+    dispatcher->stream = stream;
     RedWorkerMessage message = RED_WORKER_MESSAGE_DISPLAY_CONNECT;
     write_message(dispatcher->channel, &message);
     send_data(dispatcher->channel, &stream, sizeof(RedsStream *));
@@ -93,6 +95,9 @@ static void red_dispatcher_shutdown_peer(Channel *channel)
     red_printf("");
     RedWorkerMessage message = RED_WORKER_MESSAGE_DISPLAY_DISCONNECT;
     write_message(dispatcher->channel, &message);
+    read_message(dispatcher->channel, &message);
+    ASSERT(message == RED_WORKER_MESSAGE_READY);
+    reds_stream_free(dispatcher->stream);
 }
 
 static void red_dispatcher_migrate(Channel *channel)
diff --git a/server/red_worker.c b/server/red_worker.c
index 5f07803..f77b0f2 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -8486,8 +8486,6 @@ static void red_disconnect_channel(RedChannel *channel)
 {
     channel_release_res(channel);
     red_pipe_clear(channel);
-    reds_stream_free(channel->stream);
-    channel->stream = NULL;
     channel->send_data.blocked = FALSE;
     channel->send_data.size = channel->send_data.pos = 0;
     spice_marshaller_reset(channel->send_data.marshaller);
@@ -10060,6 +10058,7 @@ static void handle_dev_input(EventListener *listener, uint32_t events)
     case RED_WORKER_MESSAGE_CURSOR_DISCONNECT:
         red_printf("cursor disconnect");
         red_disconnect_cursor((RedChannel *)worker->cursor_channel);
+        write_ready = 1;
         break;
     case RED_WORKER_MESSAGE_CURSOR_MIGRATE:
         red_printf("cursor migrate");
-- 
1.7.1

Re: [Qemu-devel] [Spice-devel] [PATCH] server: don't call reds_stream_free from worker thread context

[Alon Levy]

On Fri, Sep 02, 2011 at 05:19:54PM +0200, Gerd Hoffmann wrote:
> reds_stream_free() may call the channel_event callback which is not
> supposed to be callsed from worker thread context.  This patch moves
> the reds_stream_free call for the display channel from the worker to
> the dispatcher to fix this issue.
> 

Looks good. I told Luiz that this would be harder to fix in spice-server,
and so I take it back - seems to be easier to fix in spice-server actually.

But still the possible problem of other users in qemu of monitor that don't
make sure to use the main thread remains.

> [ Note: not tested yet, against 0.8 branch, sending out for review &
>         comments nevertheless ]
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  server/red_dispatcher.c |    5 +++++
>  server/red_worker.c     |    3 +--
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
> index f74b13e..801a575 100644
> --- a/server/red_dispatcher.c
> +++ b/server/red_dispatcher.c
> @@ -51,6 +51,7 @@ struct RedDispatcher {
>      int y_res;
>      int use_hardware_cursor;
>      RedDispatcher *next;
> +    RedsStream *stream;
>      RedWorkerMessage async_message;
>      pthread_mutex_t  async_lock;
>      QXLDevSurfaceCreate surface_create;
> @@ -81,6 +82,7 @@ static void red_dispatcher_set_peer(Channel *channel, RedsStream *stream, int mi
>  
>      red_printf("");
>      dispatcher = (RedDispatcher *)channel->data;
> +    dispatcher->stream = stream;
>      RedWorkerMessage message = RED_WORKER_MESSAGE_DISPLAY_CONNECT;
>      write_message(dispatcher->channel, &message);
>      send_data(dispatcher->channel, &stream, sizeof(RedsStream *));
> @@ -93,6 +95,9 @@ static void red_dispatcher_shutdown_peer(Channel *channel)
>      red_printf("");
>      RedWorkerMessage message = RED_WORKER_MESSAGE_DISPLAY_DISCONNECT;
>      write_message(dispatcher->channel, &message);
> +    read_message(dispatcher->channel, &message);
> +    ASSERT(message == RED_WORKER_MESSAGE_READY);
> +    reds_stream_free(dispatcher->stream);
>  }
>  
>  static void red_dispatcher_migrate(Channel *channel)
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 5f07803..f77b0f2 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -8486,8 +8486,6 @@ static void red_disconnect_channel(RedChannel *channel)
>  {
>      channel_release_res(channel);
>      red_pipe_clear(channel);
> -    reds_stream_free(channel->stream);
> -    channel->stream = NULL;
>      channel->send_data.blocked = FALSE;
>      channel->send_data.size = channel->send_data.pos = 0;
>      spice_marshaller_reset(channel->send_data.marshaller);
> @@ -10060,6 +10058,7 @@ static void handle_dev_input(EventListener *listener, uint32_t events)
>      case RED_WORKER_MESSAGE_CURSOR_DISCONNECT:
>          red_printf("cursor disconnect");
>          red_disconnect_cursor((RedChannel *)worker->cursor_channel);
> +        write_ready = 1;
>          break;
>      case RED_WORKER_MESSAGE_CURSOR_MIGRATE:
>          red_printf("cursor migrate");
> -- 
> 1.7.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Qemu-devel] [Spice-devel] [PATCH] server: don't call reds_stream_free from worker thread context

[Yonit Halperin]

On 09/02/2011 06:19 PM, Gerd Hoffmann wrote:
> reds_stream_free() may call the channel_event callback which is not
> supposed to be callsed from worker thread context.  This patch moves
> the reds_stream_free call for the display channel from the worker to
> the dispatcher to fix this issue.
>
> [ Note: not tested yet, against 0.8 branch, sending out for review&
>          comments nevertheless ]
>
> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
> ---
>   server/red_dispatcher.c |    5 +++++
>   server/red_worker.c     |    3 +--
>   2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
> index f74b13e..801a575 100644
> --- a/server/red_dispatcher.c
> +++ b/server/red_dispatcher.c
> @@ -51,6 +51,7 @@ struct RedDispatcher {
>       int y_res;
>       int use_hardware_cursor;
>       RedDispatcher *next;
> +    RedsStream *stream;
>       RedWorkerMessage async_message;
>       pthread_mutex_t  async_lock;
>       QXLDevSurfaceCreate surface_create;
> @@ -81,6 +82,7 @@ static void red_dispatcher_set_peer(Channel *channel, RedsStream *stream, int mi
>
>       red_printf("");
>       dispatcher = (RedDispatcher *)channel->data;
> +    dispatcher->stream = stream;
>       RedWorkerMessage message = RED_WORKER_MESSAGE_DISPLAY_CONNECT;
>       write_message(dispatcher->channel,&message);
>       send_data(dispatcher->channel,&stream, sizeof(RedsStream *));
> @@ -93,6 +95,9 @@ static void red_dispatcher_shutdown_peer(Channel *channel)
>       red_printf("");
>       RedWorkerMessage message = RED_WORKER_MESSAGE_DISPLAY_DISCONNECT;
>       write_message(dispatcher->channel,&message);
> +    read_message(dispatcher->channel,&message);
> +    ASSERT(message == RED_WORKER_MESSAGE_READY);
> +    reds_stream_free(dispatcher->stream);

Hi,
RED_WORKER_MESSAGE_DISPLAY_DISCONNECT is not the only place that 
triggers red_disconnect_channel (and as a result, 
reds_stream_free(dispatcher->stream)). red_disconnect_channel is called 
also when there is an error upon receive/send and also when timeouts 
related to the client occur (e.g., in flush_display_commands).

We probably better make the dispatcher bi-directional, i.e., not only 
push messages to the worker, but also listen.

Cheers,
Yonit.

>   }
>
>   static void red_dispatcher_migrate(Channel *channel)
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 5f07803..f77b0f2 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -8486,8 +8486,6 @@ static void red_disconnect_channel(RedChannel *channel)
>   {
>       channel_release_res(channel);
>       red_pipe_clear(channel);
> -    reds_stream_free(channel->stream);
> -    channel->stream = NULL;
>       channel->send_data.blocked = FALSE;
>       channel->send_data.size = channel->send_data.pos = 0;
>       spice_marshaller_reset(channel->send_data.marshaller);
> @@ -10060,6 +10058,7 @@ static void handle_dev_input(EventListener *listener, uint32_t events)
>       case RED_WORKER_MESSAGE_CURSOR_DISCONNECT:
>           red_printf("cursor disconnect");
>           red_disconnect_cursor((RedChannel *)worker->cursor_channel);
> +        write_ready = 1;
>           break;
>       case RED_WORKER_MESSAGE_CURSOR_MIGRATE:
>           red_printf("cursor migrate");

Re: [Qemu-devel] [Spice-devel] [PATCH] server: don't call reds_stream_free from worker thread context

[Gerd Hoffmann]

   Hi,

> Hi,
> RED_WORKER_MESSAGE_DISPLAY_DISCONNECT is not the only place that
> triggers red_disconnect_channel (and as a result,
> reds_stream_free(dispatcher->stream)). red_disconnect_channel is called
> also when there is an error upon receive/send and also when timeouts
> related to the client occur (e.g., in flush_display_commands).

Ok.

> We probably better make the dispatcher bi-directional, i.e., not only
> push messages to the worker, but also listen.

That sounds like a non-trivial thing.

What does the master branch here btw?  I had a brief look and saw that 
the code looks quite different here (probably due to the multiclient work).

cheers,
   Gerd

Re: [Qemu-devel] [Spice-devel] [PATCH] server: don't call reds_stream_free from worker thread context

[Alon Levy]

On Mon, Sep 05, 2011 at 11:02:43AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> >Hi,
> >RED_WORKER_MESSAGE_DISPLAY_DISCONNECT is not the only place that
> >triggers red_disconnect_channel (and as a result,
> >reds_stream_free(dispatcher->stream)). red_disconnect_channel is called
> >also when there is an error upon receive/send and also when timeouts
> >related to the client occur (e.g., in flush_display_commands).
> 
> Ok.
> 
> >We probably better make the dispatcher bi-directional, i.e., not only
> >push messages to the worker, but also listen.
> 
> That sounds like a non-trivial thing.
> 
> What does the master branch here btw?  I had a brief look and saw
> that the code looks quite different here (probably due to the
> multiclient work).
> 

I verified it still calls reds_stream_free from the worker thread, only
now the call itself is done in red_channel.c (via red_channel_disconnect
or something like that), which is called from red_worker.c

> cheers,
>   Gerd
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Qemu-devel] [Spice-devel] [PATCH] server: don't call reds_stream_free from worker thread context

[Gerd Hoffmann]

   Hi,

> I verified it still calls reds_stream_free from the worker thread, only
> now the call itself is done in red_channel.c (via red_channel_disconnect
> or something like that), which is called from red_worker.c

Where the code in red_channel.c is now shared for all channel types? 
Hmm.  That makes it a bit harder to change the workflow I guess ...

cheers,
   Gerd

Re: [Qemu-devel] [Spice-devel] [PATCH] server: don't call reds_stream_free from worker thread context

[Alon Levy]

On Mon, Sep 05, 2011 at 03:29:39PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> >I verified it still calls reds_stream_free from the worker thread, only
> >now the call itself is done in red_channel.c (via red_channel_disconnect
> >or something like that), which is called from red_worker.c
> 
> Where the code in red_channel.c is now shared for all channel types?
> Hmm.  That makes it a bit harder to change the workflow I guess ...

can do the usual (well, done once in hw/qxl.c) trick of

 if (pthread_id() == stored_thread_id_from_main_channel_creation) {
   write_to_pipe_read_in_main_thread
 } else {
   real_reds_stream_free();
 }

> 
> cheers,
>   Gerd
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Qemu-devel] [Spice-devel] [PATCH] server: don't call reds_stream_free from worker thread context

[Alon Levy]

On Mon, Sep 05, 2011 at 03:29:39PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> >I verified it still calls reds_stream_free from the worker thread, only
> >now the call itself is done in red_channel.c (via red_channel_disconnect
> >or something like that), which is called from red_worker.c
> 
> Where the code in red_channel.c is now shared for all channel types?
> Hmm.  That makes it a bit harder to change the workflow I guess ...
> 

But just to be clear, I think it's better to fix this in monitor, like Daniel
suggested. Still waiting for Anthony to reply to his last email.

> cheers,
>   Gerd
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel