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

[Christophe Fergeau]

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.

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

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

> +}
> +
>  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'


Acked-by: Christophe Fergeau <cfergeau at redhat.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180412/726ea68f/attachment.sig>