[Spice-devel] [PATCH v2 1/4] worker: push data when clients can receive them

[Jonathon Jongsma]

Acked-by: Jonathon Jongsma <jjongsma at redhat.com>

On Wed, 2016-02-03 at 10:48 +0000, Frediano Ziglio wrote:
> During every iteration of the main worker loop, outgoing data was pushed to
> the client. However, there was no guarantee that the loop would be woken up
> in every situation. So there were some conditions where the loop would stop
> iterating until a new event was sent.
> 
> Currently, the events that can wake up the main worker loop include:
>  - data from dispatcher (including wakeups from the guest)
>  - data from clients
>  - timeouts on a stream
>  - other timeouts
>  - polling
> 
> This patch adds a new wakeup event: when we have items that are queued to
> be sent to a client, we set up a watch event for writing data to the
> client. If no items are waiting to be sent, this watch will be disabled.
> This allows us to remove the explicit push from the main worker loop.
> 
> This has some advantages:
>  - it could lower latency as we don't have to wait for a polling timeout.
>    From my experiments using a tight loop (so not really the ideal
>    condition to see the improvements) the total time was reduced by 2-3%)
>  - helps reduce the possibility of hanging loops
>  - avoids having to scan all clients to detect which one can accept data.
> 
> Signed-by: Frediano Ziglio <figlio at redhat.com>
> ---
>  server/red-channel.c | 39 ++++++++++++++++++---------------------
>  server/red-worker.c  | 13 -------------
>  2 files changed, 18 insertions(+), 34 deletions(-)
> 
> diff --git a/server/red-channel.c b/server/red-channel.c
> index 33d4158..fdd85b9 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -1359,6 +1359,11 @@ void red_channel_client_push(RedChannelClient *rcc)
>      while ((pipe_item = red_channel_client_pipe_item_get(rcc))) {
>          red_channel_client_send_item(rcc, pipe_item);
>      }
> +    if (red_channel_client_no_item_being_sent(rcc) && ring_is_empty(&rcc
> ->pipe)
> +        && rcc->stream->watch) {
> +        rcc->channel->core->watch_update_mask(rcc->stream->watch,
> +                                              SPICE_WATCH_EVENT_READ);
> +    }
>      rcc->during_send = FALSE;
>      red_channel_client_unref(rcc);
>  }
> @@ -1659,7 +1664,7 @@ void red_channel_pipe_item_init(RedChannel *channel,
> PipeItem *item, int type)
>      item->type = type;
>  }
>  
> -static inline int validate_pipe_add(RedChannelClient *rcc, PipeItem *item)
> +static inline gboolean client_pipe_add(RedChannelClient *rcc, PipeItem *item,
> RingItem *pos)
>  {
>      spice_assert(rcc && item);
>      if (SPICE_UNLIKELY(!red_channel_client_is_connected(rcc))) {
> @@ -1667,17 +1672,20 @@ static inline int validate_pipe_add(RedChannelClient
> *rcc, PipeItem *item)
>          red_channel_client_release_item(rcc, item, FALSE);
>          return FALSE;
>      }
> +    if (ring_is_empty(&rcc->pipe) && rcc->stream->watch) {
> +        rcc->channel->core->watch_update_mask(rcc->stream->watch,
> +                                         SPICE_WATCH_EVENT_READ |
> +                                         SPICE_WATCH_EVENT_WRITE);
> +    }
> +    rcc->pipe_size++;
> +    ring_add(pos, &item->link);
>      return TRUE;
>  }
>  
>  void red_channel_client_pipe_add(RedChannelClient *rcc, PipeItem *item)
>  {
>  
> -    if (!validate_pipe_add(rcc, item)) {
> -        return;
> -    }
> -    rcc->pipe_size++;
> -    ring_add(&rcc->pipe, &item->link);
> +    client_pipe_add(rcc, item, &rcc->pipe);
>  }
>  
>  void red_channel_client_pipe_add_push(RedChannelClient *rcc, PipeItem *item)
> @@ -1690,11 +1698,7 @@ void red_channel_client_pipe_add_after(RedChannelClient
> *rcc,
>                                         PipeItem *item, PipeItem *pos)
>  {
>      spice_assert(pos);
> -    if (!validate_pipe_add(rcc, item)) {
> -        return;
> -    }
> -    rcc->pipe_size++;
> -    ring_add_after(&item->link, &pos->link);
> +    client_pipe_add(rcc, item, &pos->link);
>  }
>  
>  int red_channel_client_pipe_item_is_linked(RedChannelClient *rcc,
> @@ -1706,21 +1710,14 @@ int
> red_channel_client_pipe_item_is_linked(RedChannelClient *rcc,
>  void red_channel_client_pipe_add_tail_no_push(RedChannelClient *rcc,
>                                                PipeItem *item)
>  {
> -    if (!validate_pipe_add(rcc, item)) {
> -        return;
> -    }
> -    rcc->pipe_size++;
> -    ring_add_before(&item->link, &rcc->pipe);
> +    client_pipe_add(rcc, item, rcc->pipe.prev);
>  }
>  
>  void red_channel_client_pipe_add_tail(RedChannelClient *rcc, PipeItem *item)
>  {
> -    if (!validate_pipe_add(rcc, item)) {
> -        return;
> +    if (client_pipe_add(rcc, item, rcc->pipe.prev)) {
> +        red_channel_client_push(rcc);
>      }
> -    rcc->pipe_size++;
> -    ring_add_before(&item->link, &rcc->pipe);
> -    red_channel_client_push(rcc);
>  }
>  
>  void red_channel_client_pipe_add_type(RedChannelClient *rcc, int
> pipe_item_type)
> diff --git a/server/red-worker.c b/server/red-worker.c
> index f6c4f45..031350a 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -321,16 +321,6 @@ static int red_process_display(RedWorker *worker, int
> *ring_is_empty)
>      return n;
>  }
>  
> -static void red_push(RedWorker *worker)
> -{
> -    if (worker->cursor_channel) {
> -        red_channel_push(RED_CHANNEL(worker->cursor_channel));
> -    }
> -    if (worker->display_channel) {
> -        red_channel_push(RED_CHANNEL(worker->display_channel));
> -    }
> -}
> -
>  static void red_disconnect_display(RedWorker *worker)
>  {
>      spice_warning("update timeout");
> @@ -1458,9 +1448,6 @@ static gboolean worker_source_dispatch(GSource *source,
> GSourceFunc callback,
>      red_process_cursor(worker, &ring_is_empty);
>      red_process_display(worker, &ring_is_empty);
>  
> -    /* TODO: remove me? that should be handled by watch out condition */
> -    red_push(worker);
> -
>      return TRUE;
>  }
>