[spice-server,2/3] red-stream: Implements flush using TCP_CORK

Submitted by Frediano Ziglio on April 17, 2018, 5:50 p.m.

Details

Message ID 1884041115.22719824.1523987408675.JavaMail.zimbra@redhat.com
State New
Headers show
Series "Series without cover letter" ( rev: 2 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio April 17, 2018, 5:50 p.m.
> 
> On 04/13/2018 03:25 PM, 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 for network
> > packets to be complete.
> > 
> > Using some replay capture and some instrumentation resulted in a
> > bandwith reduction of 11% and a packet reduction of 56%.
> > 
> > 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@redhat.com>
> > ---
> >   server/red-stream.c | 40 +++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 39 insertions(+), 1 deletion(-)
> > 
> > diff --git a/server/red-stream.c b/server/red-stream.c
> > index 9a9c1a0f..18c4a935 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>
> >   
> > @@ -37,6 +38,11 @@
> >   #include "red-stream.h"
> >   #include "reds.h"
> >   
> > +// compatibility for *BSD systems
> > +#ifndef TCP_CORK
> > +#define TCP_CORK TCP_NOPUSH
> > +#endif
> > +
> >   struct AsyncRead {
> >       void *opaque;
> >       uint8_t *now;
> > @@ -83,6 +89,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 +100,16 @@ struct RedStreamPrivate {
> >       SpiceCoreInterfaceInternal *core;
> >   };
> >   
> > +/**
> > + * Set TCP_CORK on socket
> > + */
> > +/* NOTE: enabled must be int */
> > +static int socket_set_cork(int socket, int enabled)
> > +{
> > +    SPICE_VERIFY(sizeof(enabled) == sizeof(int));
> > +    return setsockopt(socket, IPPROTO_TCP, TCP_CORK, &enabled,
> > sizeof(enabled));
> > +}
> > +
> >   static ssize_t stream_write_cb(RedStream *s, const void *buf, size_t
> >   size)
> >   {
> >       return write(s->socket, buf, size);
> > @@ -205,11 +223,31 @@ bool red_stream_write_all(RedStream *stream, const
> > void *in_buf, size_t n)
> >   
> >   bool red_stream_set_auto_flush(RedStream *s, bool auto_flush)
> >   {
> > -    return auto_flush;
> > +    if (s->priv->use_cork == !auto_flush) {
> > +        return true;
> > +    }
> > +
> > +    s->priv->use_cork = !auto_flush;
> > +    if (s->priv->use_cork) {
> > +        if (socket_set_cork(s->socket, 1)) {
> > +            s->priv->use_cork = false;
> > +            return false;
> > +        } else {
> > +            s->priv->corked = true;
> > +        }
> > +    } else if (s->priv->corked) {
> > +        socket_set_cork(s->socket, 0);
> > +        s->priv->corked = false;
> > +    }
> Hi Frediano,
> 
> It would be simpler to use !auto_flash or s->priv->use_cork
> and also only keep one of use_cork and corked.
> Possibly the logic is a bit different and not exactly what you want.
> 
>        if (socket_set_cork(s->socket, !auto_flash)) {
>            return false;
>        }
> 
>        s->priv->use_cork = !auto_flash;
> 
> 
> > +    return true;
> >   }
> 
> Uri.
> 

Not proper... the exact equivalent is this:


bool red_stream_set_auto_flush_new(RedStream *s, bool auto_flush)
{
    if (s->priv->use_cork == !auto_flush) {
        return true;
    }

    if (!auto_flush || s->priv->corked) {
        if (socket_set_cork(s->socket, !auto_flush) && !auto_flush) {
            return false;
        }
        s->priv->corked = !auto_flush;
    }

    s->priv->use_cork = !auto_flush;
    return true;
}


but is confusing, maybe easier to remove the "} else {" after a return
and move use_cork change after the ifs as suggested to avoid assigning
to false, like:



A bit longer than the previous but IMO more readable.

> >   
> >   void red_stream_flush(RedStream *s)
> >   {
> > +    if (s->priv->corked) {
> > +        socket_set_cork(s->socket, 0);
> > +        socket_set_cork(s->socket, 1);
> > +    }
> >   }
> >   
> >   #if HAVE_SASL
> > 
> 
> 

Frediano

Patch hide | download patch | download mbox

--- a/server/red-stream.c
+++ b/server/red-stream.c
@@ -227,18 +227,16 @@  bool red_stream_set_auto_flush(RedStream *s, bool auto_flush)
         return true;
     }

-    s->priv->use_cork = !auto_flush;
-    if (s->priv->use_cork) {
+    if (!auto_flush) {
         if (socket_set_cork(s->socket, 1)) {
-            s->priv->use_cork = false;
             return false;
-        } else {
-            s->priv->corked = true;
         }
+        s->priv->corked = true;
     } else if (s->priv->corked) {
         socket_set_cork(s->socket, 0);
         s->priv->corked = false;
     }
+    s->priv->use_cork = !auto_flush;
     return true;
 }


Comments

On 04/17/2018 08:50 PM, Frediano Ziglio wrote:
>>
>> On 04/13/2018 03:25 PM, 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 for network
>>> packets to be complete.
>>>
>>> Using some replay capture and some instrumentation resulted in a
>>> bandwith reduction of 11% and a packet reduction of 56%.
>>>
>>> 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@redhat.com>
>>> ---
>>>    server/red-stream.c | 40 +++++++++++++++++++++++++++++++++++++++-
>>>    1 file changed, 39 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/server/red-stream.c b/server/red-stream.c
>>> index 9a9c1a0f..18c4a935 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>
>>>    
>>> @@ -37,6 +38,11 @@
>>>    #include "red-stream.h"
>>>    #include "reds.h"
>>>    
>>> +// compatibility for *BSD systems
>>> +#ifndef TCP_CORK
>>> +#define TCP_CORK TCP_NOPUSH
>>> +#endif
>>> +
>>>    struct AsyncRead {
>>>        void *opaque;
>>>        uint8_t *now;
>>> @@ -83,6 +89,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 +100,16 @@ struct RedStreamPrivate {
>>>        SpiceCoreInterfaceInternal *core;
>>>    };
>>>    
>>> +/**
>>> + * Set TCP_CORK on socket
>>> + */
>>> +/* NOTE: enabled must be int */
>>> +static int socket_set_cork(int socket, int enabled)
>>> +{
>>> +    SPICE_VERIFY(sizeof(enabled) == sizeof(int));
>>> +    return setsockopt(socket, IPPROTO_TCP, TCP_CORK, &enabled,
>>> sizeof(enabled));
>>> +}
>>> +
>>>    static ssize_t stream_write_cb(RedStream *s, const void *buf, size_t
>>>    size)
>>>    {
>>>        return write(s->socket, buf, size);
>>> @@ -205,11 +223,31 @@ bool red_stream_write_all(RedStream *stream, const
>>> void *in_buf, size_t n)
>>>    
>>>    bool red_stream_set_auto_flush(RedStream *s, bool auto_flush)
>>>    {
>>> -    return auto_flush;
>>> +    if (s->priv->use_cork == !auto_flush) {
>>> +        return true;
>>> +    }
>>> +
>>> +    s->priv->use_cork = !auto_flush;
>>> +    if (s->priv->use_cork) {
>>> +        if (socket_set_cork(s->socket, 1)) {
>>> +            s->priv->use_cork = false;
>>> +            return false;
>>> +        } else {
>>> +            s->priv->corked = true;
>>> +        }
>>> +    } else if (s->priv->corked) {
>>> +        socket_set_cork(s->socket, 0);
>>> +        s->priv->corked = false;
>>> +    }
>> Hi Frediano,
>>
>> It would be simpler to use !auto_flash or s->priv->use_cork
>> and also only keep one of use_cork and corked.
>> Possibly the logic is a bit different and not exactly what you want.
>>
>>         if (socket_set_cork(s->socket, !auto_flash)) {
>>             return false;
>>         }
>>
>>         s->priv->use_cork = !auto_flash;
>>
>>
>>> +    return true;
>>>    }
>>
>> Uri.
>>
> 
> Not proper... the exact equivalent is this:

I know it's not equivalent, but simpler.

Do we really need both use_cork and corked ?

Even if corked==FALSE , still socket_set_cork(s->socket, 0)
would succeed.

Uri.

> 
> 
> bool red_stream_set_auto_flush_new(RedStream *s, bool auto_flush)
> {
>      if (s->priv->use_cork == !auto_flush) {
>          return true;
>      }
> 
>      if (!auto_flush || s->priv->corked) {
>          if (socket_set_cork(s->socket, !auto_flush) && !auto_flush) {
>              return false;
>          }
>          s->priv->corked = !auto_flush;
>      }
> 
>      s->priv->use_cork = !auto_flush;
>      return true;
> }
> 
> 
> but is confusing, maybe easier to remove the "} else {" after a return
> and move use_cork change after the ifs as suggested to avoid assigning
> to false, like:
> 
> 
> --- a/server/red-stream.c
> +++ b/server/red-stream.c
> @@ -227,18 +227,16 @@ bool red_stream_set_auto_flush(RedStream *s, bool auto_flush)
>           return true;
>       }
> 
> -    s->priv->use_cork = !auto_flush;
> -    if (s->priv->use_cork) {
> +    if (!auto_flush) {
>           if (socket_set_cork(s->socket, 1)) {
> -            s->priv->use_cork = false;
>               return false;
> -        } else {
> -            s->priv->corked = true;
>           }
> +        s->priv->corked = true;
>       } else if (s->priv->corked) {
>           socket_set_cork(s->socket, 0);
>           s->priv->corked = false;
>       }
> +    s->priv->use_cork = !auto_flush;
>       return true;
>   }
> 
> 
> A bit longer than the previous but IMO more readable.
> 
>>>    
>>>    void red_stream_flush(RedStream *s)
>>>    {
>>> +    if (s->priv->corked) {
>>> +        socket_set_cork(s->socket, 0);
>>> +        socket_set_cork(s->socket, 1);
>>> +    }
>>>    }
>>>    
>>>    #if HAVE_SASL
>>>
>>
>>
> 
> Frediano
>
> 
> On 04/17/2018 08:50 PM, Frediano Ziglio wrote:
> >>
> >> On 04/13/2018 03:25 PM, 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 for
> >>> network
> >>> packets to be complete.
> >>>
> >>> Using some replay capture and some instrumentation resulted in a
> >>> bandwith reduction of 11% and a packet reduction of 56%.
> >>>
> >>> 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@redhat.com>
> >>> ---
> >>>    server/red-stream.c | 40 +++++++++++++++++++++++++++++++++++++++-
> >>>    1 file changed, 39 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/server/red-stream.c b/server/red-stream.c
> >>> index 9a9c1a0f..18c4a935 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>
> >>>    
> >>> @@ -37,6 +38,11 @@
> >>>    #include "red-stream.h"
> >>>    #include "reds.h"
> >>>    
> >>> +// compatibility for *BSD systems
> >>> +#ifndef TCP_CORK
> >>> +#define TCP_CORK TCP_NOPUSH
> >>> +#endif
> >>> +
> >>>    struct AsyncRead {
> >>>        void *opaque;
> >>>        uint8_t *now;
> >>> @@ -83,6 +89,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 +100,16 @@ struct RedStreamPrivate {
> >>>        SpiceCoreInterfaceInternal *core;
> >>>    };
> >>>    
> >>> +/**
> >>> + * Set TCP_CORK on socket
> >>> + */
> >>> +/* NOTE: enabled must be int */
> >>> +static int socket_set_cork(int socket, int enabled)
> >>> +{
> >>> +    SPICE_VERIFY(sizeof(enabled) == sizeof(int));
> >>> +    return setsockopt(socket, IPPROTO_TCP, TCP_CORK, &enabled,
> >>> sizeof(enabled));
> >>> +}
> >>> +
> >>>    static ssize_t stream_write_cb(RedStream *s, const void *buf, size_t
> >>>    size)
> >>>    {
> >>>        return write(s->socket, buf, size);
> >>> @@ -205,11 +223,31 @@ bool red_stream_write_all(RedStream *stream, const
> >>> void *in_buf, size_t n)
> >>>    
> >>>    bool red_stream_set_auto_flush(RedStream *s, bool auto_flush)
> >>>    {
> >>> -    return auto_flush;
> >>> +    if (s->priv->use_cork == !auto_flush) {
> >>> +        return true;
> >>> +    }
> >>> +
> >>> +    s->priv->use_cork = !auto_flush;
> >>> +    if (s->priv->use_cork) {
> >>> +        if (socket_set_cork(s->socket, 1)) {
> >>> +            s->priv->use_cork = false;
> >>> +            return false;
> >>> +        } else {
> >>> +            s->priv->corked = true;
> >>> +        }
> >>> +    } else if (s->priv->corked) {
> >>> +        socket_set_cork(s->socket, 0);
> >>> +        s->priv->corked = false;
> >>> +    }
> >> Hi Frediano,
> >>
> >> It would be simpler to use !auto_flash or s->priv->use_cork
> >> and also only keep one of use_cork and corked.
> >> Possibly the logic is a bit different and not exactly what you want.
> >>
> >>         if (socket_set_cork(s->socket, !auto_flash)) {
> >>             return false;
> >>         }
> >>
> >>         s->priv->use_cork = !auto_flash;
> >>
> >>
> >>> +    return true;
> >>>    }
> >>
> >> Uri.
> >>
> > 
> > Not proper... the exact equivalent is this:
> 
> I know it's not equivalent, but simpler.
> 
> Do we really need both use_cork and corked ?
> 
> Even if corked==FALSE , still socket_set_cork(s->socket, 0)
> would succeed.
> 
> Uri.
> 

now I understood, taken in consideration the initial state and
possible changes this is equivalent:


bool red_stream_set_auto_flush(RedStream *s, bool auto_flush)
{
    if (s->priv->use_cork == !auto_flush) {
        return true;
    }

    if (socket_set_cork(s->socket, !auto_flush) && !auto_flush) {
        return false;
    }
    s->priv->use_cork = !auto_flush;
    return true;
}


The reason is that use_cork is (beside temporary states) always
equal to corked.

Frediano

> > 
> > 
> > bool red_stream_set_auto_flush_new(RedStream *s, bool auto_flush)
> > {
> >      if (s->priv->use_cork == !auto_flush) {
> >          return true;
> >      }
> > 
> >      if (!auto_flush || s->priv->corked) {
> >          if (socket_set_cork(s->socket, !auto_flush) && !auto_flush) {
> >              return false;
> >          }
> >          s->priv->corked = !auto_flush;
> >      }
> > 
> >      s->priv->use_cork = !auto_flush;
> >      return true;
> > }
> > 
> > 
> > but is confusing, maybe easier to remove the "} else {" after a return
> > and move use_cork change after the ifs as suggested to avoid assigning
> > to false, like:
> > 
> > 
> > --- a/server/red-stream.c
> > +++ b/server/red-stream.c
> > @@ -227,18 +227,16 @@ bool red_stream_set_auto_flush(RedStream *s, bool
> > auto_flush)
> >           return true;
> >       }
> > 
> > -    s->priv->use_cork = !auto_flush;
> > -    if (s->priv->use_cork) {
> > +    if (!auto_flush) {
> >           if (socket_set_cork(s->socket, 1)) {
> > -            s->priv->use_cork = false;
> >               return false;
> > -        } else {
> > -            s->priv->corked = true;
> >           }
> > +        s->priv->corked = true;
> >       } else if (s->priv->corked) {
> >           socket_set_cork(s->socket, 0);
> >           s->priv->corked = false;
> >       }
> > +    s->priv->use_cork = !auto_flush;
> >       return true;
> >   }
> > 
> > 
> > A bit longer than the previous but IMO more readable.
> > 
> >>>    
> >>>    void red_stream_flush(RedStream *s)
> >>>    {
> >>> +    if (s->priv->corked) {
> >>> +        socket_set_cork(s->socket, 0);
> >>> +        socket_set_cork(s->socket, 1);
> >>> +    }
> >>>    }
> >>>    
> >>>    #if HAVE_SASL
> >>>
> >>
> >>
> >