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

[Frediano Ziglio]

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

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