[Spice-devel] [PATCH spice-server] red_channel: replace an assert upon threads mismatch with a warning

[Alon Levy]

> The assert:
> spice_assert(pthread_equal(pthread_self(), client->thread_id))
> and the assert:
> spice_assert(pthread_equal(pthread_self(), rcc->channel->thread_id))
> were coded in order to protect data that is accessed from the main
> context (red_client and most of the channels), from
> access by threads of other channels (namely, the display and cursor
> channels), and vice versa.
> However, some of the calls to the sound channel interface,
> and also the char_device interface, can be done from the vcpu thread.
> It doesn't endanger these channels internal data, since qemu use global
> mutex for the vcpu and io threads.
> Thus, pthread_self() can be !=  channel->thread_id, if one of them is
> the vcpu thread and the other is the io-thread, and we shouldn't assert.
> 
> Future plans: A more complete and complicated solution would be to manage our
> own thread for
> spice-channels, and push input from qemu to this thread, instead of
> counting on the global mutex of qemu

ACK

> 
> rhbz#823472
> ---
>  server/red_channel.c | 28 +++++++++++++++++++++++++---
>  1 file changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/server/red_channel.c b/server/red_channel.c
> index 119e5e5..5662041 100644
> --- a/server/red_channel.c
> +++ b/server/red_channel.c
> @@ -938,6 +938,8 @@ RedChannel *red_channel_create(int size,
>  
>      channel->out_bytes_counter = 0;
>  
> +    spice_debug("channel type %d id %d thread_id 0x%lx",
> +                channel->type, channel->id, channel->thread_id);
>      return channel;
>  }
>  
> @@ -982,6 +984,8 @@ RedChannel *red_channel_create_dummy(int size, uint32_t
> type, uint32_t id)
>      red_channel_set_common_cap(channel, SPICE_COMMON_CAP_MINI_HEADER);
>  
>      channel->thread_id = pthread_self();
> +    spice_debug("channel type %d id %d thread_id 0x%lx",
> +                channel->type, channel->id, channel->thread_id);
>  
>      channel->out_bytes_counter = 0;
>  
> @@ -1657,7 +1661,14 @@ void
> red_channel_client_ack_set_client_window(RedChannelClient *rcc, int client_
>  
>  static void red_channel_remove_client(RedChannelClient *rcc)
>  {
> -    spice_assert(pthread_equal(pthread_self(), rcc->channel->thread_id));
> +    if (!pthread_equal(pthread_self(), rcc->channel->thread_id)) {
> +        spice_warning("channel type %d id %d - "
> +                      "channel->thread_id (0x%lx) != pthread_self (0x%lx)."
> +                      "If one of the threads is != io-thread && !=
> vcpu-thread, "
> +                      "this might be a BUG",
> +                      rcc->channel->type, rcc->channel->id,
> +                      rcc->channel->thread_id, pthread_self());
> +    }
>      ring_remove(&rcc->channel_link);
>      spice_assert(rcc->channel->clients_num > 0);
>      rcc->channel->clients_num--;
> @@ -1937,7 +1948,12 @@ void red_client_migrate(RedClient *client)
>      RedChannelClient *rcc;
>  
>      spice_printerr("migrate client with #channels %d",
>      client->channels_num);
> -    spice_assert(pthread_equal(pthread_self(), client->thread_id));
> +    if (!pthread_equal(pthread_self(), client->thread_id)) {
> +        spice_warning("client->thread_id (0x%lx) != pthread_self (0x%lx)."
> +                      "If one of the threads is != io-thread && !=
> vcpu-thread,"
> +                      " this might be a BUG",
> +                      client->thread_id, pthread_self());
> +    }
>      RING_FOREACH_SAFE(link, next, &client->channels) {
>          rcc = SPICE_CONTAINEROF(link, RedChannelClient, client_link);
>          if (red_channel_client_is_connected(rcc)) {
> @@ -1952,7 +1968,13 @@ void red_client_destroy(RedClient *client)
>      RedChannelClient *rcc;
>  
>      spice_printerr("destroy client with #channels %d",
>      client->channels_num);
> -    spice_assert(pthread_equal(pthread_self(), client->thread_id));
> +    if (!pthread_equal(pthread_self(), client->thread_id)) {
> +        spice_warning("client->thread_id (0x%lx) != pthread_self (0x%lx)."
> +                      "If one of the threads is != io-thread && !=
> vcpu-thread,"
> +                      " this might be a BUG",
> +                      client->thread_id,
> +                      pthread_self());
> +    }
>      RING_FOREACH_SAFE(link, next, &client->channels) {
>          // some channels may be in other threads, so disconnection
>          // is not synchronous.
> --
> 1.8.1.4
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>