[Spice-devel] [PATCH spice-server 2/3] stream: implements flush using TCP_CORK

[Frediano Ziglio]

> 
> On Tue, Jan 16, 2018 at 02:18:14PM +0000, Frediano Ziglio wrote:
> > Cork is a system interface implemented by Linux and some *BSD systems to
> > tell the system that other data are expected to be written to a socket.
> > This allows the system to reduce network fragmentation waiting a network
> 
> 'waiting for network packets to be complete' I think.
> 
> > packet to be complete.
> > 
> > Using some replay capture and some instrumentation resulted in a
> > bandwith reduction of 11% and a packet reduction of 56%.
> 
> I would add a link to
> https://lists.freedesktop.org/archives/spice-devel/2017-February/035577.html
> and maybe even copy the data you put in there.
> 

Added part of it, the TLS part was not strictly related (and surely the
small TLS optimization tests does not belong here).

Specifically I added:

    The tests was done using replay utility so results could be a bit different
    from real cases as:
    - replay goes as fast as it can, for instance packets could
      be merged by the kernel decreasing packet numbers and a bit
      byte spent (this actually make the following improves worse);
    - there are fewer channels (no much cursor, sound, etc).
    The following tests shows count packet and total bytes from server to
    client using a real network. I used a direct cable connection using 1gb
    connection and 2 laptops.

    cork: 537 1582240
    cork: 681 1823754
    cork: 524 1583287
    cork: 538 1582350
    no cork: 1329 1834630
    no cork: 1290 1829094
    no cork: 1289 1830164
    no cork: 1317 1833589
    no cork: 1320 1835705

> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/red-stream.c | 34 +++++++++++++++++++++++++++++++++-
> >  1 file changed, 33 insertions(+), 1 deletion(-)
> > 
> > diff --git a/server/red-stream.c b/server/red-stream.c
> > index 4812d8e4..4833077c 100644
> > --- a/server/red-stream.c
> > +++ b/server/red-stream.c
> > @@ -24,6 +24,7 @@
> >  #include <unistd.h>
> >  #include <sys/socket.h>
> >  #include <fcntl.h>
> > +#include <netinet/tcp.h>
> >  
> >  #include <glib.h>
> >  
> > @@ -83,6 +84,8 @@ struct RedStreamPrivate {
> >       * deallocated when main_dispatcher handles the
> >       SPICE_CHANNEL_EVENT_DISCONNECTED
> >       * event, either from same thread or by call back from main thread. */
> >      SpiceChannelEventInfo* info;
> > +    bool use_cork;
> > +    bool corked;
> >  
> >      ssize_t (*read)(RedStream *s, void *buf, size_t nbyte);
> >      ssize_t (*write)(RedStream *s, const void *buf, size_t nbyte);
> > @@ -92,6 +95,15 @@ struct RedStreamPrivate {
> >      SpiceCoreInterfaceInternal *core;
> >  };
> >  
> > +/**
> > + * Set TCP_CORK on socket
> > + */
> > +/* NOTE: enabled must be int */
> 
> Maybe verify(sizeof(socket) == sizeof(int))?
> 

added a SPICE_VERIFY inside the function, specifically

    SPICE_VERIFY(sizeof(enabled) == sizeof(int));

> > +static inline int socket_set_cork(int socket, int enabled)
> 
> I'd drop the 'inline'
> 
> > +{
> > +    return setsockopt(socket, IPPROTO_TCP, TCP_CORK, &enabled,
> > sizeof(enabled));
> 
> I suspect we'll need to add a configure check for this? It seems to be
> called TCP_NOPUSH in OpenBSD? https://man.openbsd.org/tcp
> 

I'll add a

#ifndef TCP_CORK
#define TCP_CORK TCP_NOPUSH
#endif

at the beginning of the file

> > +}
> > +
> >  static ssize_t stream_write_cb(RedStream *s, const void *buf, size_t size)
> >  {
> >      return write(s->socket, buf, size);
> > @@ -205,11 +217,31 @@ bool red_stream_write_all(RedStream *stream, const
> > void *in_buf, size_t n)
> >  
> >  bool red_stream_set_auto_flush(RedStream *s, bool enable)
> >  {
> > -    return enable;
> > +    if (s->priv->use_cork == !enable) {
> 
> Might be slightly more readable if 'enable' is renamed to 'auto_flush'
> 

Agreed, done

> 
> Acked-by: Christophe Fergeau <cfergeau at redhat.com>
> 

Frediano