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

[Christophe Fergeau]

On Fri, Apr 13, 2018 at 01:25:08PM +0100, Frediano Ziglio wrote:
> In order to use the new TCP_CORK feature, disable auto flush.
> 
> Depending on channel implementation and purpose of the channel enabling
> blindly for all channels could cause performance issues, specifically if
> flush is not done at the right time.
> CommonGraphicsChannel channels were tested to make sure is not that case.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/common-graphics-channel.c | 17 +++++++++--------
>  server/red-channel-client.c      |  5 +++++
>  2 files changed, 14 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..fe912802 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -1328,6 +1328,11 @@ 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);
> +        /* channel has no pending data to send so now we can flush data in
> +         * order to avoid data stall into buffers in case of manual
> +         * flushing
> +         */
> +        red_stream_flush(rcc->priv->stream);

I finally convinced myself that having it here makes sense, so
Acked-by: Christophe Fergeau <cfergeau at redhat.com> for the series.

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/20180417/47af6bca/attachment.sig>