[Spice-devel] [RFC PATCH 3/3] worker: use manual flushing on stream to decrease packet fragmentation

[Jonathon Jongsma]

On Mon, 2016-02-22 at 13:38 -0500, Frediano Ziglio wrote:
> > 
> > On Tue, 2016-02-16 at 15:36 +0000, Frediano Ziglio wrote:
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  server/red-channel.c | 12 ++++++++----
> > >  server/red-worker.c  | 24 +++++++++++++-----------
> > >  2 files changed, 21 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/server/red-channel.c b/server/red-channel.c
> > > index d7ff37e..c2a0c78 100644
> > > --- a/server/red-channel.c
> > > +++ b/server/red-channel.c
> > > @@ -1364,10 +1364,14 @@ 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);
> > > +    if (red_channel_client_no_item_being_sent(rcc) && ring_is_empty(&rcc
> > > ->pipe)) {
> > > +        if (rcc->stream->watch) {
> > > +            rcc->channel->core->watch_update_mask(rcc->stream->watch,
> > > +                                                 
> > >  SPICE_WATCH_EVENT_READ);
> > > +        }
> > > +        reds_stream_flush(rcc->stream);
> > > +    } else if (red_channel_client_waiting_for_ack(rcc)) {
> > > +        reds_stream_flush(rcc->stream);
> > 
> > Can you explain the reason for flushing when we're waiting for an ACK? Is
> > there
> > a reason we couldn't just move the flush completely outside of the if
> > statement
> > and flush in all circumstances in this function?
> > 
> 
> Mainly it's the optimal way.
> For Ack you need to flush as otherwise you risk to wait an pong for a not sent
> ping if the messages are really small.
> For the other if you want to flush only if there are nothing queued (neither
> data neither other messages).
> Note that you can have lot of messages in the queue but you are waiting for
> ack (pong) before sending other data.
> 
> OT: I don't like the idea of ACKs on bytes and as discussed with Pavel in Brno
> I don't see the point of using additional Acking on top of tcp (which already
> implements flow control) but one problem at a time.
> 
> > >      }
> > >      rcc->during_send = FALSE;
> > >      red_channel_client_unref(rcc);
> > > diff --git a/server/red-worker.c b/server/red-worker.c
> > > index bf12a22..b368a63 100644
> > > --- a/server/red-worker.c
> > > +++ b/server/red-worker.c
> > > @@ -416,17 +416,19 @@ static int
> > > common_channel_config_socket(RedChannelClient
> > > *rcc)
> > >  
> > >      // TODO - this should be dynamic, not one time at channel creation
> > >      ccc->is_low_bandwidth = main_channel_client_is_low_bandwidth(mcc);
> > > -    delay_val = ccc->is_low_bandwidth ? 0 : 1;
> > > -    /* 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/
> > > -     */
> > > -    if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY, &delay_val,
> > > -                   sizeof(delay_val)) == -1) {
> > > -        if (errno != ENOTSUP) {
> > > -            spice_warning("setsockopt failed, %s", strerror(errno));
> > > +    if (!reds_stream_set_auto_flush(rcc->stream, false)) {
> > > +        delay_val = ccc->is_low_bandwidth ? 0 : 1;
> > > +        /* 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/
> > > +         */
> > > +        if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY,
> > > &delay_val,
> > > +                       sizeof(delay_val)) == -1) {
> > > +            if (errno != ENOTSUP) {
> > > +                spice_warning("setsockopt failed, %s", strerror(errno));
> > > +            }
> > 
> > 
> > I don't quite understand why you're setting NODELAY if CORK fails here. Can
> > you
> > provide a bit more justification/explanation here?
> > 
> 
> It's a fallback. Cork and flush are a better way to achieve no delay sending
> data also avoiding also too much fragmentation (which was the reason for
> Nagle).
> It's actually a bit paranoid as the socket and system (Linux) we are using
> supports TCP_CORK so function won't fail but we are ready for failures or
> porting (I don't know... *BSD, Mac).


I apologize, somehow I completely missed the fact that the old code was already
setting NODELAY. For some reason I thought that your patch added that part. I'll
blame it on Monday.


> 
> Note that the flush APIs are independent on the implementation. They can be
> used with SSL and compression too.
> Assume we are sending (not impossible!) 20 messages of 50 bytes each. The
> standard framing for ethernet (wire or wireless) + tcp is about 80 bytes.
> Sending as separate network packets you use 20 * ( 50 + 80 ) = 2600 bytes
> while with cork you just spend 1080 ( 20 * 50 + 80 ).
> Using SSL assuming a cipher of 128 bit we can assume to encrypt N bytes
> we need 3 + ROUNDUP(N + 8, 16) (3 is the TLS framing, 8 is for the padding
> and 16 is the key size in bytes)that is for 50 bytes 67 bytes so we could
> have 20 * ( 67 + 80 ) = 2940 bytes instead of
> 3 + ROUNDUP(20 * 50 + 8, 16) = 1011 bytes .
> Similar padding+framing apply to compression algorithms.
> 
> > 
> > >          }
> > >      }
> > >      return TRUE;
> > 
> 
> Frediano