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

[Frediano Ziglio]

> 
> 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.
> 

Agreed. Mainly it could be that the channel push an item at a time in the
network having the opposite result. Quite unlikely to be honest but tested
that these channels works as expected.

> > 
> > 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.
> 

Maybe better to add a comment to code.
Maybe this hunk should be moved to 1/3 ?

> Christophe
> 

Frediano