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

[Uri Lublin]

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

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