Re: [PATCH spice-server 2/4] red-channel-client: Prevent too tight loop waiting for ACKs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Sep 18, 2017 at 11:28:28AM +0100, Frediano Ziglio wrote:
> RedChannelClient has a "handle-acks" feature.
> If this feature is enabled after the configured number of messages
> it waits for an ACK.
> If is waiting for an ACK it stops sending messages.
> However the write notification was not disabled causing the
> loop event to always trigger as the socket in this case is
> ready to accept data.
> This is noticeable using slow network environments and having
> some additional loop instrumentation.

I'd add a bunch more commas to this log to make it easier to read :)
You are talking about a tight loop in the commit log, it would be worth
being explicit about exactly which code is being run in that tight loop
("the loop event to always trigger" means needing to look at the code
just to guess which code it is..)

> 
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
>  server/red-channel-client.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> index 764e6cd7e..98efdfd0b 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -45,6 +45,8 @@
>  #define IOV_MAX 1024
>  #endif
>  
> +#define SPICE_WATCH_EVENTS_READ_WRITE (SPICE_WATCH_EVENT_READ|SPICE_WATCH_EVENT_WRITE)

I don't think this really belongs in this commit, it litterally doubles
the amount of changes done in here :)

Christophe

> +
>  typedef struct SpiceDataHeaderOpaque SpiceDataHeaderOpaque;
>  
>  typedef uint16_t (*get_msg_type_proc)(SpiceDataHeaderOpaque *header);
> @@ -1327,7 +1329,8 @@ 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) && g_queue_is_empty(&rcc->priv->pipe)) {
> +    if ((red_channel_client_no_item_being_sent(rcc) && g_queue_is_empty(&rcc->priv->pipe)) ||
> +        red_channel_client_waiting_for_ack(rcc)) {
>          red_channel_client_watch_update_mask(rcc, SPICE_WATCH_EVENT_READ);
>      }
>      rcc->priv->during_send = FALSE;
> @@ -1452,6 +1455,7 @@ bool red_channel_client_handle_message(RedChannelClient *rcc, uint16_t type,
>      case SPICE_MSGC_ACK:
>          if (rcc->priv->ack_data.client_generation == rcc->priv->ack_data.generation) {
>              rcc->priv->ack_data.messages_window -= rcc->priv->ack_data.client_window;
> +            red_channel_client_watch_update_mask(rcc, SPICE_WATCH_EVENTS_READ_WRITE);
>              red_channel_client_push(rcc);
>          }
>          break;
> @@ -1542,8 +1546,7 @@ static inline gboolean prepare_pipe_add(RedChannelClient *rcc, RedPipeItem *item
>          return FALSE;
>      }
>      if (g_queue_is_empty(&rcc->priv->pipe)) {
> -        red_channel_client_watch_update_mask(rcc,
> -                                             SPICE_WATCH_EVENT_READ | SPICE_WATCH_EVENT_WRITE);
> +        red_channel_client_watch_update_mask(rcc, SPICE_WATCH_EVENTS_READ_WRITE);
>      }
>      return TRUE;
>  }
> -- 
> 2.13.5
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]