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

[Jonathon Jongsma]

On Fri, 2016-01-29 at 10:53 +0000, Frediano Ziglio wrote:
> Data were pushed during the main red worker loop.
> However there was no garantiee that there was some activities that
> make the loop do another iteration.
> This could cause in some condition the loop to stop till a new event
> was sent.
> Events were:
> - data from dispatcher (including wake up from guest);
> - data from clients;
> - timeouts on stream;
> - other timeouts;
> - polling.
> So usually polling or wake ups from guest could wake up the loop.
> This patch wants to add event to trigger when we can write data to
> clients.
> This has some advantages:
> - could lower latency as we don't have to wait 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%);
> - help reducing the possibility of hanging loops;
> - avoid having to scan all clients to detect which one can accept
>   data.
> 

I had to re-read this commit log several times to understand it fully. How about
something like:

----------------------
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 | 25 ++++++++++++++++---------
>  server/red-worker.c  | 13 -------------
>  2 files changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/server/red-channel.c b/server/red-channel.c
> index fd41493..632007e 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -1345,6 +1345,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);
>  }
> @@ -1645,7 +1650,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 int prepare_pipe_add(RedChannelClient *rcc, PipeItem *item)

> @@ -1653,16 +1658,21 @@ 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++;
>      return TRUE;
>  }
>  
>  void red_channel_client_pipe_add(RedChannelClient *rcc, PipeItem *item)
>  {
>  
> -    if (!validate_pipe_add(rcc, item)) {
> +    if (!prepare_pipe_add(rcc, item)) {
>          return;
>      }
> -    rcc->pipe_size++;

I don't really like the fact that the pipe size is incremented separately from
the actual modification of the pipe ring. This makes it more likely to get out
-of-sync. On the other hand, I believe that a future patch will change the pipe
to a GList* type, so I don't care too much..

>      ring_add(&rcc->pipe, &item->link);
>  }
>  
> @@ -1676,10 +1686,9 @@ void red_channel_client_pipe_add_after(RedChannelClient
> *rcc,
>                                         PipeItem *item, PipeItem *pos)
>  {
>      spice_assert(pos);
> -    if (!validate_pipe_add(rcc, item)) {
> +    if (!prepare_pipe_add(rcc, item)) {
>          return;
>      }
> -    rcc->pipe_size++;
>      ring_add_after(&item->link, &pos->link);
>  }
>  
> @@ -1692,19 +1701,17 @@ 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)) {
> +    if (!prepare_pipe_add(rcc, item)) {
>          return;
>      }
> -    rcc->pipe_size++;
>      ring_add_before(&item->link, &rcc->pipe);
>  }
>  
>  void red_channel_client_pipe_add_tail(RedChannelClient *rcc, PipeItem *item)
>  {
> -    if (!validate_pipe_add(rcc, item)) {
> +    if (!prepare_pipe_add(rcc, item)) {
>          return;
>      }
> -    rcc->pipe_size++;
>      ring_add_before(&item->link, &rcc->pipe);
>      red_channel_client_push(rcc);
>  }
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 560d172..cf8e73d 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;
>  }
>  


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