Libguestfs - Jun 2019 - Re: [PATCH libnbd 2/3] states: Add handle h->wflags field.

Appears to have a measurable benefit, see 3/3 for test results.

Rich.

Add an optional .send method flags parameter.  This parameter may be
ignored by the socket layer.  If used it is intended to add
optimization flags such as MSG_MORE for socket layers which can handle
this.
---
 generator/states.c | 2 +-
 lib/crypto.c       | 2 +-
 lib/internal.h     | 2 +-
 lib/socket.c       | 4 ++--
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/generator/states.c b/generator/states.c
index 145e8c1..e879a83 100644
--- a/generator/states.c
+++ b/generator/states.c
@@ -92,7 +92,7 @@ send_from_wbuf (struct nbd_handle *h)
 
   if (h->wlen == 0)
     return 0;                   /* move to next state */
-  r = h->sock->ops->send (h, h->sock, h->wbuf, h->wlen);
+  r = h->sock->ops->send (h, h->sock, h->wbuf, h->wlen, 0);
   if (r == -1) {
     if (errno == EAGAIN || errno == EWOULDBLOCK)
       return 1;                 /* more data */
diff --git a/lib/crypto.c b/lib/crypto.c
index e0f173f..703bc84 100644
--- a/lib/crypto.c
+++ b/lib/crypto.c
@@ -164,7 +164,7 @@ tls_recv (struct nbd_handle *h, struct socket *sock, void
*buf, size_t len)
 
 static ssize_t
 tls_send (struct nbd_handle *h,
-          struct socket *sock, const void *buf, size_t len)
+          struct socket *sock, const void *buf, size_t len, int flags)
 {
   ssize_t r;
 
diff --git a/lib/internal.h b/lib/internal.h
index 503bf34..1ffb5b7 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -201,7 +201,7 @@ struct socket_ops {
   ssize_t (*recv) (struct nbd_handle *h,
                    struct socket *sock, void *buf, size_t len);
   ssize_t (*send) (struct nbd_handle *h,
-                   struct socket *sock, const void *buf, size_t len);
+                   struct socket *sock, const void *buf, size_t len, int
flags);
   bool (*pending) (struct socket *sock);
   int (*get_fd) (struct socket *sock);
   int (*close) (struct socket *sock);
diff --git a/lib/socket.c b/lib/socket.c
index 084398b..8555855 100644
--- a/lib/socket.c
+++ b/lib/socket.c
@@ -42,11 +42,11 @@ socket_recv (struct nbd_handle *h, struct socket *sock, void
*buf, size_t len)
 
 static ssize_t
 socket_send (struct nbd_handle *h,
-             struct socket *sock, const void *buf, size_t len)
+             struct socket *sock, const void *buf, size_t len, int flags)
 {
   ssize_t r;
 
-  r = send (sock->u.fd, buf, len, 0);
+  r = send (sock->u.fd, buf, len, flags);
   if (r == -1 && errno != EAGAIN && errno != EWOULDBLOCK)
     set_error (errno, "send");
   return r;
-- 
2.21.0

This field contains optimization flags (ie. MSG_MORE) which are passed
through to the socket layer if it supports them.  The flags are reset
automatically when we move to another state.
---
 generator/states.c | 6 ++++--
 lib/internal.h     | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/generator/states.c b/generator/states.c
index e879a83..36cca37 100644
--- a/generator/states.c
+++ b/generator/states.c
@@ -92,7 +92,7 @@ send_from_wbuf (struct nbd_handle *h)
 
   if (h->wlen == 0)
     return 0;                   /* move to next state */
-  r = h->sock->ops->send (h, h->sock, h->wbuf, h->wlen, 0);
+  r = h->sock->ops->send (h, h->sock, h->wbuf, h->wlen,
h->wflags);
   if (r == -1) {
     if (errno == EAGAIN || errno == EWOULDBLOCK)
       return 1;                 /* more data */
@@ -101,8 +101,10 @@ send_from_wbuf (struct nbd_handle *h)
   }
   h->wbuf += r;
   h->wlen -= r;
-  if (h->wlen == 0)
+  if (h->wlen == 0) {
+    h->wflags = 0;              /* reset this when moving to next state */
     return 0;                   /* move to next state */
+  }
   else
     return 1;                   /* more data */
 }
diff --git a/lib/internal.h b/lib/internal.h
index 1ffb5b7..e7be05b 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -110,6 +110,7 @@ struct nbd_handle {
   /* As above, but for writing using send_from_wbuf. */
   const void *wbuf;
   size_t wlen;
+  int wflags;
 
   /* Static buffer used for short amounts of data, such as handshake
    * and commands.
-- 
2.21.0

Since we disabled Nagle's algorithm we may send very small packets
over the wire in some situations where we are calling send(2) from
states that are responsible for small parts of the protocol.  By
setting the MSG_MORE flag we can indicate to the kernel that more data
will follow (usually) immediately and so it can append the data to the
same outgoing packet.

Although there is some variability in the test there is a measurable
benefit.  Using this test:

  $ time nbdkit memory 100M --run 'examples/threaded-reads-and-writes
localhost 10809'

before applying this patch:

  real	0m54.151s
  real	0m54.950s
  real	0m55.927s

and after applying this patch:

  real	0m39.154s
  real	0m44.249s
  real	0m44.027s

Thanks: Eric Blake for suggesting this change.
---
 generator/states-issue-command.c                 | 2 ++
 generator/states-newstyle-opt-export-name.c      | 1 +
 generator/states-newstyle-opt-go.c               | 3 +++
 generator/states-newstyle-opt-set-meta-context.c | 5 +++++
 4 files changed, 11 insertions(+)

diff --git a/generator/states-issue-command.c b/generator/states-issue-command.c
index 627a54f..cce43d7 100644
--- a/generator/states-issue-command.c
+++ b/generator/states-issue-command.c
@@ -42,6 +42,8 @@
   h->request.count = htobe32 ((uint32_t) cmd->count);
   h->wbuf = &h->request;
   h->wlen = sizeof (h->request);
+  if (cmd->type == NBD_CMD_WRITE)
+    h->wflags = MSG_MORE;
   SET_NEXT_STATE (%SEND_REQUEST);
   return 0;
 
diff --git a/generator/states-newstyle-opt-export-name.c
b/generator/states-newstyle-opt-export-name.c
index 774c93c..968cea8 100644
--- a/generator/states-newstyle-opt-export-name.c
+++ b/generator/states-newstyle-opt-export-name.c
@@ -25,6 +25,7 @@
   h->sbuf.option.optlen = strlen (h->export_name);
   h->wbuf = &h->sbuf;
   h->wlen = sizeof h->sbuf.option;
+  h->wflags = MSG_MORE;
   SET_NEXT_STATE (%SEND);
   return 0;
 
diff --git a/generator/states-newstyle-opt-go.c
b/generator/states-newstyle-opt-go.c
index eea70cb..06bbaca 100644
--- a/generator/states-newstyle-opt-go.c
+++ b/generator/states-newstyle-opt-go.c
@@ -26,6 +26,7 @@
     htobe32 (/* exportnamelen */ 4 + strlen (h->export_name) + /* nrinfos */
2);
   h->wbuf = &h->sbuf;
   h->wlen = sizeof h->sbuf.option;
+  h->wflags = MSG_MORE;
   SET_NEXT_STATE (%SEND);
   return 0;
 
@@ -38,6 +39,7 @@
     h->sbuf.len = htobe32 (exportnamelen);
     h->wbuf = &h->sbuf;
     h->wlen = 4;
+    h->wflags = MSG_MORE;
     SET_NEXT_STATE (%SEND_EXPORTNAMELEN);
   }
   return 0;
@@ -48,6 +50,7 @@
   case 0:
     h->wbuf = h->export_name;
     h->wlen = strlen (h->export_name);
+    h->wflags = MSG_MORE;
     SET_NEXT_STATE (%SEND_EXPORT);
   }
   return 0;
diff --git a/generator/states-newstyle-opt-set-meta-context.c
b/generator/states-newstyle-opt-set-meta-context.c
index 1cd65c3..7148774 100644
--- a/generator/states-newstyle-opt-set-meta-context.c
+++ b/generator/states-newstyle-opt-set-meta-context.c
@@ -47,6 +47,7 @@
   h->sbuf.option.optlen = htobe32 (len);
   h->wbuf = &h->sbuf;
   h->wlen = sizeof (h->sbuf.option);
+  h->wflags = MSG_MORE;
   SET_NEXT_STATE (%SEND);
   return 0;
 
@@ -57,6 +58,7 @@
     h->sbuf.len = htobe32 (strlen (h->export_name));
     h->wbuf = &h->sbuf.len;
     h->wlen = sizeof h->sbuf.len;
+    h->wflags = MSG_MORE;
     SET_NEXT_STATE (%SEND_EXPORTNAMELEN);
   }
   return 0;
@@ -67,6 +69,7 @@
   case 0:
     h->wbuf = h->export_name;
     h->wlen = strlen (h->export_name);
+    h->wflags = MSG_MORE;
     SET_NEXT_STATE (%SEND_EXPORTNAME);
   }
   return 0;
@@ -79,6 +82,7 @@
       htobe32 (nbd_internal_string_list_length (h->request_meta_contexts));
     h->wbuf = &h->sbuf;
     h->wlen = sizeof h->sbuf.nrqueries;
+    h->wflags = MSG_MORE;
     SET_NEXT_STATE (%SEND_NRQUERIES);
   }
   return 0;
@@ -103,6 +107,7 @@
   h->sbuf.len = htobe32 (strlen (query));
   h->wbuf = &h->sbuf.len;
   h->wlen = sizeof h->sbuf.len;
+  h->wflags = MSG_MORE;
   SET_NEXT_STATE (%SEND_QUERYLEN);
   return 0;
 
-- 
2.21.0

There's an obvious bug in this patch in that it doesn't reset the
h->wflags field in all cases.  Updated patch below.

I rechecked the performance measurements and they are the same after
the updated patch.

Rich.
>From 15a687b50acecebcfd3dc6222d93e6df984b83c6 Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones@redhat.com>
Date: Sat, 8 Jun 2019 19:12:22 +0100
Subject: [PATCH] states: Add handle h->wflags field.

This field contains optimization flags (ie. MSG_MORE) which are passed
through to the socket layer if it supports them.  The flags are reset
automatically when we move to another state.
---
 generator/states.c | 10 +++++++---
 lib/internal.h     |  1 +
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/generator/states.c b/generator/states.c
index e879a83..b0dab83 100644
--- a/generator/states.c
+++ b/generator/states.c
@@ -91,8 +91,8 @@ send_from_wbuf (struct nbd_handle *h)
   ssize_t r;
 
   if (h->wlen == 0)
-    return 0;                   /* move to next state */
-  r = h->sock->ops->send (h, h->sock, h->wbuf, h->wlen, 0);
+    goto next_state;
+  r = h->sock->ops->send (h, h->sock, h->wbuf, h->wlen,
h->wflags);
   if (r == -1) {
     if (errno == EAGAIN || errno == EWOULDBLOCK)
       return 1;                 /* more data */
@@ -102,9 +102,13 @@ send_from_wbuf (struct nbd_handle *h)
   h->wbuf += r;
   h->wlen -= r;
   if (h->wlen == 0)
-    return 0;                   /* move to next state */
+    goto next_state;
   else
     return 1;                   /* more data */
+
+ next_state:
+  h->wflags = 0;                /* reset this when moving to next state */
+  return 0;                     /* move to next state */
 }
 
 /*----- End of prologue. -----*/
diff --git a/lib/internal.h b/lib/internal.h
index 1ffb5b7..e7be05b 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -110,6 +110,7 @@ struct nbd_handle {
   /* As above, but for writing using send_from_wbuf. */
   const void *wbuf;
   size_t wlen;
+  int wflags;
 
   /* Static buffer used for short amounts of data, such as handshake
    * and commands.
-- 
2.21.0



-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On 6/8/19 1:38 PM, Richard W.M. Jones wrote:
> Add an optional .send method flags parameter.  This parameter may be
> ignored by the socket layer.  If used it is intended to add
> optimization flags such as MSG_MORE for socket layers which can handle
> this.
> ---
>  generator/states.c | 2 +-
>  lib/crypto.c       | 2 +-
>  lib/internal.h     | 2 +-
>  lib/socket.c       | 4 ++--
>  4 files changed, 5 insertions(+), 5 deletions(-)
> 
ACK

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 6/8/19 1:38 PM, Richard W.M. Jones wrote:
> Since we disabled Nagle's algorithm we may send very small packets
> over the wire in some situations where we are calling send(2) from
> states that are responsible for small parts of the protocol.  By
> setting the MSG_MORE flag we can indicate to the kernel that more data
> will follow (usually) immediately and so it can append the data to the
> same outgoing packet.
> 
MSG_MORE is Linux-specific; this probably won't compile on other
platforms. I think you need to gate it behind an agnostic name, and then
use #ifdef MSG_MORE before actually using it.

Also, you tested plaintext modes, but there may also be benefits in
using gnutls_record_cork/uncork in response to a MORE flag (can be a
separate patch from this one).
> Although there is some variability in the test there is a measurable
> benefit.  Using this test:
> 
>   $ time nbdkit memory 100M --run 'examples/threaded-reads-and-writes
localhost 10809'
> 
> before applying this patch:
> 
>   real	0m54.151s
>   real	0m54.950s
>   real	0m55.927s
> 
> and after applying this patch:
> 
>   real	0m39.154s
>   real	0m44.249s
>   real	0m44.027s
> 
> Thanks: Eric Blake for suggesting this change.
> ---
The change itself looks reasonable, even if we need to fix things to be
a bit more portable to other platforms.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org