| Message ID | 1314976794-31931-1-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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");
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
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
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
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
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
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");
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(-)