[Spice-devel] [PATCH spice-server 3/3] common-graphics-channel: use manual flushing on stream to decrease packet fragmentation

[Christophe Fergeau]

On Tue, Jan 16, 2018 at 02:18:15PM +0000, Frediano Ziglio wrote:
> In order to use new TCP_CORK feature disable auto flush.

'the new TCP_CORK feature, disable auto flush'
Might be worth explaining in the commit log why you disable auto_flush
only for RedCommonGraphicsChannel.

> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/common-graphics-channel.c | 17 +++++++++--------
>  server/red-channel-client.c      |  1 +
>  2 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/server/common-graphics-channel.c b/server/common-graphics-channel.c
> index ce6b5e57..083ab3eb 100644
> --- a/server/common-graphics-channel.c
> +++ b/server/common-graphics-channel.c
> @@ -83,14 +83,15 @@ bool common_channel_client_config_socket(RedChannelClient *rcc)
>  
>      // TODO - this should be dynamic, not one time at channel creation
>      is_low_bandwidth = main_channel_client_is_low_bandwidth(mcc);
> -    /* FIXME: Using Nagle's Algorithm can lead to apparent delays, depending
> -     * on the delayed ack timeout on the other side.
> -     * Instead of using Nagle's, we need to implement message buffering on
> -     * the application level.
> -     * see: http://www.stuartcheshire.org/papers/NagleDelayedAck/
> -     */
> -    red_stream_set_no_delay(stream, !is_low_bandwidth);
> -
> +    if (!red_stream_set_auto_flush(stream, false)) {
> +        /* FIXME: Using Nagle's Algorithm can lead to apparent delays, depending
> +         * on the delayed ack timeout on the other side.
> +         * Instead of using Nagle's, we need to implement message buffering on
> +         * the application level.
> +         * see: http://www.stuartcheshire.org/papers/NagleDelayedAck/
> +         */
> +        red_stream_set_no_delay(stream, !is_low_bandwidth);
> +    }
>      // TODO: move wide/narrow ack setting to red_channel.
>      red_channel_client_ack_set_client_window(rcc,
>          is_low_bandwidth ?
> diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> index f154c5c6..32ac30d1 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -1328,6 +1328,7 @@ void red_channel_client_push(RedChannelClient *rcc)
>      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);
> +        red_stream_flush(rcc->priv->stream);

I would add a rationale in the commit log regarding why the
red_stream_flush() is in this specific place.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180412/9bf4982a/attachment.sig>