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

Uri Lublin <uril@xxxxxxxxxx> · Wed, 18 Apr 2018 13:00:36 +0300

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


_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel







