1:38 p.m.
New subject: [PATCH libnbd 1/3] lib: socket: Add .send flags parameter.
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
5:14 p.m.
New subject: [PATCH libnbd 1/3] lib: socket: Add .send flags parameter.
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
1:38 p.m.
New subject: [PATCH libnbd 2/3] 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 | 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
1:14 p.m.
New subject: [PATCH libnbd 2/3] states: Add handle h->wflags field.
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(a)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
5:22 p.m.
New subject: [PATCH libnbd 2/3] states: Add handle h->wflags field.
On 6/9/19 1:14 PM, Richard W.M. Jones wrote:
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(a)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 */
Hmm. If we blocked, do we still want to use MSG_MORE on the next
resumption of send()? Or should we blindly clear h->wflags as soon as
we block as well as when we move to the next state?
In practice, will we ever block when MSG_MORE is set? Ideally, we won't
overflow a TCP segment except on the final payload portion of
NBD_CMD_WRITE. Or are you hoping to set MSG_MORE even on other commands,
if there is a request queue, so that we can bundle multiple commands
into a single TCP segment?
Okay, I think that it's okay to always replay MSG_MORE when resuming a
write() that blocked, even if we'll never hit that situation unless we
rework a caller to pass MSG_MORE even for the last byte of one request
when we know the request queue is non-empty so another request is
immediately ready to also transmit.
+
+ 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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
1:38 p.m.
New subject: [PATCH libnbd 3/3] states: Use MSG_MORE to coalesce messages into single packets.
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
5:26 p.m.
New subject: [PATCH libnbd 3/3] states: Use MSG_MORE to coalesce messages into single packets.
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
1993
days inactive
1995
days old
7 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones