[pseudo] [RFC 1/2] pseudo_ipc.c: Use MSG_NOSIGNAL if available

[pseudo] [RFC 1/2] pseudo_ipc.c: Use MSG_NOSIGNAL if available

[Rasmus Villemoes]

MSG_NOSIGNAL has been in Linux since 2.2, and has been standardized in
POSIX 2008. Using that when available avoids the overhead of the two
syscalls to set and restore the SIGPIPE handler. Moreover, we can
eliminate one write() call by making use of sendmsg() to do
scatter-gather I/O.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 pseudo_ipc.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 50 insertions(+), 13 deletions(-)

diff --git a/pseudo_ipc.c b/pseudo_ipc.c
index 82c6f70..7786880 100644
--- a/pseudo_ipc.c
+++ b/pseudo_ipc.c
@@ -26,13 +26,17 @@
 #include <ctype.h>
 #include <errno.h>
 #include <sys/stat.h>
+#include <sys/socket.h>
 
 #include "pseudo.h"
 #include "pseudo_ipc.h"
 
-/* Short reads or writes can cause a sigpipe, killing the program, so we
- * trap it and report that something happened.
+/* Short reads or writes can cause a sigpipe, killing the program, so
+ * we trap it and report that something happened. We can avoid the
+ * overhead of setting/restoring the SIGPIPE handler if MSG_NOSIGNAL
+ * is available.
  */
+#ifndef MSG_NOSIGNAL
 static sig_atomic_t pipe_error = 0;
 static void (*old_handler)(int) = SIG_DFL;
 
@@ -51,6 +55,9 @@ static void
 allow_sigpipe(void) {
 	signal(SIGPIPE, old_handler);
 }
+#else
+#define pipe_error 0
+#endif
 
 #if 0
 /* useful only when debugging crazy stuff */
@@ -66,6 +73,36 @@ display_msg_header(pseudo_msg_t *msg) {
 }
 #endif
 
+#ifdef MSG_NOSIGNAL
+static int
+do_send(int fd, struct iovec *iov, int iovlen)
+{
+	struct msghdr hdr;
+
+	memset(&hdr, 0, sizeof(hdr));
+	hdr.msg_iov = iov;
+	hdr.msg_iovlen = iovlen;
+
+	return sendmsg(fd, &hdr, MSG_NOSIGNAL);
+}
+#else
+static int
+do_send(int fd, struct iovec *iov, int iovlen)
+{
+	int r, s = 0, i;
+
+	ignore_sigpipe();
+	for (i = 0; i < iovlen; ++i) {
+		r = write(fd, iov[i].iov_base, iov[i].iov_len);
+		s += r;
+		if (r != (int)iov[i].iov_len)
+			break;
+	}
+	allow_sigpipe();
+	return s;
+}
+#endif
+
 /*
  * send message on fd
  * return:
@@ -75,6 +112,7 @@ display_msg_header(pseudo_msg_t *msg) {
  */
 int
 pseudo_msg_send(int fd, pseudo_msg_t *msg, size_t len, const char *path) {
+	struct iovec iov[2];
 	int r;
 
 	if (!msg)
@@ -89,14 +127,13 @@ pseudo_msg_send(int fd, pseudo_msg_t *msg, size_t len, const char *path) {
 		if (len == (size_t) -1)
 			len = strlen(path) + 1;
 		msg->pathlen = len;
-		ignore_sigpipe();
-		r = write(fd, msg, PSEUDO_HEADER_SIZE);
-		if (r == PSEUDO_HEADER_SIZE) {
-			r += write(fd, path, len);
-		}
-		allow_sigpipe();
+		iov[0].iov_base = msg;
+		iov[0].iov_len = PSEUDO_HEADER_SIZE;
+		iov[1].iov_base = (void*)path;
+		iov[1].iov_len = len;
+		r = do_send(fd, iov, 2);
 		pseudo_debug(PDBGF_IPC | PDBGF_VERBOSE, "wrote %d bytes\n", r);
-		if (pipe_error || (r == -1 && errno == EBADF))
+		if (pipe_error || (r == -1 && (errno == EBADF || errno == EPIPE)))
 			return -1;
 		return ((size_t) r != PSEUDO_HEADER_SIZE + len);
 	} else {
@@ -105,11 +142,11 @@ pseudo_msg_send(int fd, pseudo_msg_t *msg, size_t len, const char *path) {
 			msg->result, pseudo_res_name(msg->result),
 			msg->pathlen, msg->path, (int) msg->mode);
 		// display_msg_header(msg);
-		ignore_sigpipe();
-		r = write(fd, msg, PSEUDO_HEADER_SIZE + msg->pathlen);
-		allow_sigpipe();
+		iov[0].iov_base = msg;
+		iov[0].iov_len = PSEUDO_HEADER_SIZE + msg->pathlen;
+		r = do_send(fd, iov, 1);
 		pseudo_debug(PDBGF_IPC | PDBGF_VERBOSE, "wrote %d bytes\n", r);
-		if (pipe_error || (r == -1 && errno == EBADF))
+		if (pipe_error || (r == -1 && (errno == EBADF || errno == EPIPE)))
 			return -1;
 		return ((size_t) r != PSEUDO_HEADER_SIZE + msg->pathlen);
 	}
-- 
2.16.4

[pseudo] [RFC 2/2] pseudo_ipc.c: eliminate some code duplication

[Rasmus Villemoes]

Since msg->pathlen is set to len in the first branch, we can share the
final submit-and-check-for-success part of the two branches.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 pseudo_ipc.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/pseudo_ipc.c b/pseudo_ipc.c
index 7786880..3571dad 100644
--- a/pseudo_ipc.c
+++ b/pseudo_ipc.c
@@ -113,7 +113,7 @@ do_send(int fd, struct iovec *iov, int iovlen)
 int
 pseudo_msg_send(int fd, pseudo_msg_t *msg, size_t len, const char *path) {
 	struct iovec iov[2];
-	int r;
+	int r, iovc;
 
 	if (!msg)
 		return 1;
@@ -131,11 +131,7 @@ pseudo_msg_send(int fd, pseudo_msg_t *msg, size_t len, const char *path) {
 		iov[0].iov_len = PSEUDO_HEADER_SIZE;
 		iov[1].iov_base = (void*)path;
 		iov[1].iov_len = len;
-		r = do_send(fd, iov, 2);
-		pseudo_debug(PDBGF_IPC | PDBGF_VERBOSE, "wrote %d bytes\n", r);
-		if (pipe_error || (r == -1 && (errno == EBADF || errno == EPIPE)))
-			return -1;
-		return ((size_t) r != PSEUDO_HEADER_SIZE + len);
+		iovc = 2;
 	} else {
 		pseudo_debug(PDBGF_IPC, "msg type %d (%s), result %d (%s), path %.*s, mode 0%o\n",
 			msg->type, pseudo_op_name(msg->op),
@@ -144,12 +140,13 @@ pseudo_msg_send(int fd, pseudo_msg_t *msg, size_t len, const char *path) {
 		// display_msg_header(msg);
 		iov[0].iov_base = msg;
 		iov[0].iov_len = PSEUDO_HEADER_SIZE + msg->pathlen;
-		r = do_send(fd, iov, 1);
-		pseudo_debug(PDBGF_IPC | PDBGF_VERBOSE, "wrote %d bytes\n", r);
-		if (pipe_error || (r == -1 && (errno == EBADF || errno == EPIPE)))
-			return -1;
-		return ((size_t) r != PSEUDO_HEADER_SIZE + msg->pathlen);
+		iovc = 1;
 	}
+	r = do_send(fd, iov, iovc);
+	pseudo_debug(PDBGF_IPC | PDBGF_VERBOSE, "wrote %d bytes\n", r);
+	if (pipe_error || (r == -1 && (errno == EBADF || errno == EPIPE)))
+		return -1;
+	return ((size_t) r != PSEUDO_HEADER_SIZE + msg->pathlen);
 }
 
 /* attempts to receive a message from fd
-- 
2.16.4

Re: [pseudo] [RFC 1/2] pseudo_ipc.c: Use MSG_NOSIGNAL if available

[Rasmus Villemoes]

On 2018-07-11 16:30, Rasmus Villemoes wrote:
> MSG_NOSIGNAL has been in Linux since 2.2, and has been standardized in
> POSIX 2008. Using that when available avoids the overhead of the two
> syscalls to set and restore the SIGPIPE handler. Moreover, we can
> eliminate one write() call by making use of sendmsg() to do
> scatter-gather I/O.

Ping. The README in the pseudo repo says to use the openembedded-core
list for pseudo patches, so I assume this is the right place. I have
other ideas for improving pseudo's performance on both client and server
side, but there's no point sending patches to /dev/null.

Rasmus

Re: [pseudo] [RFC 1/2] pseudo_ipc.c: Use MSG_NOSIGNAL if available

[Richard Purdie]

On Wed, 2018-08-15 at 08:59 +0200, Rasmus Villemoes wrote:
> On 2018-07-11 16:30, Rasmus Villemoes wrote:
> > MSG_NOSIGNAL has been in Linux since 2.2, and has been standardized
> > in
> > POSIX 2008. Using that when available avoids the overhead of the
> > two
> > syscalls to set and restore the SIGPIPE handler. Moreover, we can
> > eliminate one write() call by making use of sendmsg() to do
> > scatter-gather I/O.
> 
> Ping. The README in the pseudo repo says to use the openembedded-core
> list for pseudo patches, so I assume this is the right place. I have
> other ideas for improving pseudo's performance on both client and
> server
> side, but there's no point sending patches to /dev/null.

Pseudo maintenance is a little tricky right now and I'd guess Peter is
unavailable. Would you be able to format these as additional patches to
the pseudo recipe? That way we could test and include them in builds
and then Peter will review them when he has time.

Cheers,

Richard

Re: [pseudo] [RFC 1/2] pseudo_ipc.c: Use MSG_NOSIGNAL if available

[Seebs]

On Wed, 15 Aug 2018 10:27:29 +0100
Richard Purdie <richard.purdie@linuxfoundation.org> wrote:

> On Wed, 2018-08-15 at 08:59 +0200, Rasmus Villemoes wrote:
> > On 2018-07-11 16:30, Rasmus Villemoes wrote:
> > > MSG_NOSIGNAL has been in Linux since 2.2, and has been
> > > standardized in
> > > POSIX 2008. Using that when available avoids the overhead of the
> > > two
> > > syscalls to set and restore the SIGPIPE handler. Moreover, we can
> > > eliminate one write() call by making use of sendmsg() to do
> > > scatter-gather I/O.
> > 
> > Ping. The README in the pseudo repo says to use the
> > openembedded-core list for pseudo patches, so I assume this is the
> > right place. I have other ideas for improving pseudo's performance
> > on both client and server
> > side, but there's no point sending patches to /dev/null.
> 
> Pseudo maintenance is a little tricky right now and I'd guess Peter is
> unavailable. Would you be able to format these as additional patches
> to the pseudo recipe? That way we could test and include them in
> builds and then Peter will review them when he has time.

Hi! Sorry about the delay, I wanted to review these more carefully.
They looked reasonable to me, I just haven't had a chance to look more
closely. They're still in my "I need to respond to this" mailbox.

-s