[PATCH v2 0/4] Some cleanups to tap and tcp

public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed

* [PATCH v2 0/4] Some cleanups to tap and tcp
@ 2023-02-16  5:43 David Gibson
  2023-02-16  5:43 ` [PATCH v2 1/4] tap: Don't pcap frames that didn't get sent David Gibson
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: David Gibson @ 2023-02-16  5:43 UTC (permalink / raw)
  To: Stefano Brivio, passt-dev; +Cc: David Gibson

This is a handful of simple cleanups which I made while investigating
https://bugs.passt.top/show_bug.cgi?id=41.  Note that these don't
themselves actually address that bug, they're just unrelated cleanups
that happen to be in adjacent code.

I've run basic tests on these, but not quite the complete test suite:
I haven't yet had a chance to debug an unrelated problem with the IPv4
performance tests failing.

Changes since v1:
 * Minor comment and style fixes

David Gibson (4):
  tap: Don't pcap frames that didn't get sent
  tap: Eliminate goto from tap_handler()
  tcp: Remove 'recvmsg' goto from tcp_data_from_sock
  tcp: Remove 'zero_len' goto from tcp_data_from_sock

 tap.c | 49 +++++++++++++++++++++++++++----------------------
 tcp.c | 37 +++++++++++++++++--------------------
 2 files changed, 44 insertions(+), 42 deletions(-)

-- 
2.39.1

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/4] tap: Don't pcap frames that didn't get sent
  2023-02-16  5:43 [PATCH v2 0/4] Some cleanups to tap and tcp David Gibson
@ 2023-02-16  5:43 ` David Gibson
  2023-02-16  5:43 ` [PATCH v2 2/4] tap: Eliminate goto from tap_handler() David Gibson
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2023-02-16  5:43 UTC (permalink / raw)
  To: Stefano Brivio, passt-dev; +Cc: David Gibson

In tap_send_frames() we send a number of frames to the tap device, then
also write them to the pcap capture file (if configured).  However the tap
send can partially fail (short write()s or similar), meaning that some
of the requested frames weren't actually sent, but we still write those
frames to the capture file.

We do give a debug message in this case, but it's misleading to add frames
that we know weren't sent to the capture file.  Rework to avoid this.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tap.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/tap.c b/tap.c
index 716d887..ae60fd4 100644
--- a/tap.c
+++ b/tap.c
@@ -309,10 +309,12 @@ void tap_icmp6_send(const struct ctx *c,
  * @iov:	Array of buffers, each containing one frame
  * @n:		Number of buffers/frames in @iov
  *
+ * Return: number of frames successfully sent
+ *
  * #syscalls:pasta write
  */
-static void tap_send_frames_pasta(struct ctx *c,
-				  const struct iovec *iov, size_t n)
+static size_t tap_send_frames_pasta(struct ctx *c,
+				    const struct iovec *iov, size_t n)
 {
 	size_t i;
 
@@ -325,6 +327,8 @@ static void tap_send_frames_pasta(struct ctx *c,
 			iov--;
 		}
 	}
+
+	return n;
 }
 
 /**
@@ -357,10 +361,12 @@ static void tap_send_remainder(const struct ctx *c, const struct iovec *iov,
  * @iov:	Array of buffers, each containing one frame
  * @n:		Number of buffers/frames in @iov
  *
+ * Return: number of frames successfully sent
+ *
  * #syscalls:passt sendmsg
  */
-static void tap_send_frames_passt(const struct ctx *c,
-				  const struct iovec *iov, size_t n)
+static size_t tap_send_frames_passt(const struct ctx *c,
+				    const struct iovec *iov, size_t n)
 {
 	struct msghdr mh = {
 		.msg_iov = (void *)iov,
@@ -371,7 +377,7 @@ static void tap_send_frames_passt(const struct ctx *c,
 
 	sent = sendmsg(c->fd_tap, &mh, MSG_NOSIGNAL | MSG_DONTWAIT);
 	if (sent < 0)
-		return;
+		return 0;
 
 	/* Check for any partial frames due to short send */
 	for (i = 0; i < n; i++) {
@@ -386,8 +392,7 @@ static void tap_send_frames_passt(const struct ctx *c,
 		i++;
 	}
 
-	if (i < n)
-		debug("tap: dropped %lu frames due to short send", n - i);
+	return i;
 }
 
 /**
@@ -398,15 +403,20 @@ static void tap_send_frames_passt(const struct ctx *c,
  */
 void tap_send_frames(struct ctx *c, const struct iovec *iov, size_t n)
 {
+	size_t m;
+
 	if (!n)
 		return;
 
 	if (c->mode == MODE_PASST)
-		tap_send_frames_passt(c, iov, n);
+		m = tap_send_frames_passt(c, iov, n);
 	else
-		tap_send_frames_pasta(c, iov, n);
+		m = tap_send_frames_pasta(c, iov, n);
+
+	if (m < n)
+		debug("tap: dropped %lu frames of %lu due to short send", n - m, n);
 
-	pcap_multiple(iov, n, c->mode == MODE_PASST ? sizeof(uint32_t) : 0);
+	pcap_multiple(iov, m, c->mode == MODE_PASST ? sizeof(uint32_t) : 0);
 }
 
 /**
-- 
@@ -309,10 +309,12 @@ void tap_icmp6_send(const struct ctx *c,
  * @iov:	Array of buffers, each containing one frame
  * @n:		Number of buffers/frames in @iov
  *
+ * Return: number of frames successfully sent
+ *
  * #syscalls:pasta write
  */
-static void tap_send_frames_pasta(struct ctx *c,
-				  const struct iovec *iov, size_t n)
+static size_t tap_send_frames_pasta(struct ctx *c,
+				    const struct iovec *iov, size_t n)
 {
 	size_t i;
 
@@ -325,6 +327,8 @@ static void tap_send_frames_pasta(struct ctx *c,
 			iov--;
 		}
 	}
+
+	return n;
 }
 
 /**
@@ -357,10 +361,12 @@ static void tap_send_remainder(const struct ctx *c, const struct iovec *iov,
  * @iov:	Array of buffers, each containing one frame
  * @n:		Number of buffers/frames in @iov
  *
+ * Return: number of frames successfully sent
+ *
  * #syscalls:passt sendmsg
  */
-static void tap_send_frames_passt(const struct ctx *c,
-				  const struct iovec *iov, size_t n)
+static size_t tap_send_frames_passt(const struct ctx *c,
+				    const struct iovec *iov, size_t n)
 {
 	struct msghdr mh = {
 		.msg_iov = (void *)iov,
@@ -371,7 +377,7 @@ static void tap_send_frames_passt(const struct ctx *c,
 
 	sent = sendmsg(c->fd_tap, &mh, MSG_NOSIGNAL | MSG_DONTWAIT);
 	if (sent < 0)
-		return;
+		return 0;
 
 	/* Check for any partial frames due to short send */
 	for (i = 0; i < n; i++) {
@@ -386,8 +392,7 @@ static void tap_send_frames_passt(const struct ctx *c,
 		i++;
 	}
 
-	if (i < n)
-		debug("tap: dropped %lu frames due to short send", n - i);
+	return i;
 }
 
 /**
@@ -398,15 +403,20 @@ static void tap_send_frames_passt(const struct ctx *c,
  */
 void tap_send_frames(struct ctx *c, const struct iovec *iov, size_t n)
 {
+	size_t m;
+
 	if (!n)
 		return;
 
 	if (c->mode == MODE_PASST)
-		tap_send_frames_passt(c, iov, n);
+		m = tap_send_frames_passt(c, iov, n);
 	else
-		tap_send_frames_pasta(c, iov, n);
+		m = tap_send_frames_pasta(c, iov, n);
+
+	if (m < n)
+		debug("tap: dropped %lu frames of %lu due to short send", n - m, n);
 
-	pcap_multiple(iov, n, c->mode == MODE_PASST ? sizeof(uint32_t) : 0);
+	pcap_multiple(iov, m, c->mode == MODE_PASST ? sizeof(uint32_t) : 0);
 }
 
 /**
-- 
2.39.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/4] tap: Eliminate goto from tap_handler()
  2023-02-16  5:43 [PATCH v2 0/4] Some cleanups to tap and tcp David Gibson
  2023-02-16  5:43 ` [PATCH v2 1/4] tap: Don't pcap frames that didn't get sent David Gibson
@ 2023-02-16  5:43 ` David Gibson
  2023-02-16  5:43 ` [PATCH v2 3/4] tcp: Remove 'recvmsg' goto from tcp_data_from_sock David Gibson
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2023-02-16  5:43 UTC (permalink / raw)
  To: Stefano Brivio, passt-dev; +Cc: David Gibson

The goto here really doesn't improve clarity or brevity at all.  Use a
clearer construct.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tap.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/tap.c b/tap.c
index ae60fd4..eb00135 100644
--- a/tap.c
+++ b/tap.c
@@ -1239,18 +1239,13 @@ void tap_handler(struct ctx *c, int fd, uint32_t events,
 	}
 
 	if ((c->mode == MODE_PASST && tap_handler_passt(c, now)) ||
-	    (c->mode == MODE_PASTA && tap_handler_pasta(c, now)))
-		goto reinit;
-
-	if (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR))
-		goto reinit;
+	    (c->mode == MODE_PASTA && tap_handler_pasta(c, now)) ||
+	    (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR))) {
+		if (c->one_off) {
+			info("Client closed connection, exiting");
+			exit(EXIT_SUCCESS);
+		}
 
-	return;
-reinit:
-	if (c->one_off) {
-		info("Client closed connection, exiting");
-		exit(EXIT_SUCCESS);
+		tap_sock_init(c);
 	}
-
-	tap_sock_init(c);
 }
-- 
@@ -1239,18 +1239,13 @@ void tap_handler(struct ctx *c, int fd, uint32_t events,
 	}
 
 	if ((c->mode == MODE_PASST && tap_handler_passt(c, now)) ||
-	    (c->mode == MODE_PASTA && tap_handler_pasta(c, now)))
-		goto reinit;
-
-	if (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR))
-		goto reinit;
+	    (c->mode == MODE_PASTA && tap_handler_pasta(c, now)) ||
+	    (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR))) {
+		if (c->one_off) {
+			info("Client closed connection, exiting");
+			exit(EXIT_SUCCESS);
+		}
 
-	return;
-reinit:
-	if (c->one_off) {
-		info("Client closed connection, exiting");
-		exit(EXIT_SUCCESS);
+		tap_sock_init(c);
 	}
-
-	tap_sock_init(c);
 }
-- 
2.39.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 3/4] tcp: Remove 'recvmsg' goto from tcp_data_from_sock
  2023-02-16  5:43 [PATCH v2 0/4] Some cleanups to tap and tcp David Gibson
  2023-02-16  5:43 ` [PATCH v2 1/4] tap: Don't pcap frames that didn't get sent David Gibson
  2023-02-16  5:43 ` [PATCH v2 2/4] tap: Eliminate goto from tap_handler() David Gibson
@ 2023-02-16  5:43 ` David Gibson
  2023-02-16  5:43 ` [PATCH v2 4/4] tcp: Remove 'zero_len' " David Gibson
  2023-02-16 22:22 ` [PATCH v2 0/4] Some cleanups to tap and tcp Stefano Brivio
  4 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2023-02-16  5:43 UTC (permalink / raw)
  To: Stefano Brivio, passt-dev; +Cc: David Gibson

This goto can be handled just as simply and more clearly with a do while.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tcp.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/tcp.c b/tcp.c
index f028e01..b101441 100644
--- a/tcp.c
+++ b/tcp.c
@@ -2186,13 +2186,12 @@ static int tcp_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn)
 		iov_sock[fill_bufs].iov_len = iov_rem;
 
 	/* Receive into buffers, don't dequeue until acknowledged by guest. */
-recvmsg:
-	len = recvmsg(s, &mh_sock, MSG_PEEK);
-	if (len < 0) {
-		if (errno == EINTR)
-			goto recvmsg;
+	do
+		len = recvmsg(s, &mh_sock, MSG_PEEK);
+	while (len < 0 && errno == EINTR);
+
+	if (len < 0)
 		goto err;
-	}
 
 	if (!len)
 		goto zero_len;
-- 
@@ -2186,13 +2186,12 @@ static int tcp_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn)
 		iov_sock[fill_bufs].iov_len = iov_rem;
 
 	/* Receive into buffers, don't dequeue until acknowledged by guest. */
-recvmsg:
-	len = recvmsg(s, &mh_sock, MSG_PEEK);
-	if (len < 0) {
-		if (errno == EINTR)
-			goto recvmsg;
+	do
+		len = recvmsg(s, &mh_sock, MSG_PEEK);
+	while (len < 0 && errno == EINTR);
+
+	if (len < 0)
 		goto err;
-	}
 
 	if (!len)
 		goto zero_len;
-- 
2.39.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 4/4] tcp: Remove 'zero_len' goto from tcp_data_from_sock
  2023-02-16  5:43 [PATCH v2 0/4] Some cleanups to tap and tcp David Gibson
                   ` (2 preceding siblings ...)
  2023-02-16  5:43 ` [PATCH v2 3/4] tcp: Remove 'recvmsg' goto from tcp_data_from_sock David Gibson
@ 2023-02-16  5:43 ` David Gibson
  2023-02-16 22:22 ` [PATCH v2 0/4] Some cleanups to tap and tcp Stefano Brivio
  4 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2023-02-16  5:43 UTC (permalink / raw)
  To: Stefano Brivio, passt-dev; +Cc: David Gibson

This goto exists purely to move this exception case out of line.  Although
that does make the "normal" path a little clearer, it comes at the cost of
not knowing how where control will flow after jumping to the zero_len
label.  The exceptional case isn't that long, so just put it inline.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tcp.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/tcp.c b/tcp.c
index b101441..10dbdb8 100644
--- a/tcp.c
+++ b/tcp.c
@@ -2193,8 +2193,18 @@ static int tcp_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn)
 	if (len < 0)
 		goto err;
 
-	if (!len)
-		goto zero_len;
+	if (!len) {
+		if ((conn->events & (SOCK_FIN_RCVD | TAP_FIN_SENT)) == SOCK_FIN_RCVD) {
+			if ((ret = tcp_send_flag(c, conn, FIN | ACK))) {
+				tcp_rst(c, conn);
+				return ret;
+			}
+
+			conn_event(c, conn, TAP_FIN_SENT);
+		}
+
+		return 0;
+	}
 
 	sendlen = len - already_sent;
 	if (sendlen <= 0) {
@@ -2233,18 +2243,6 @@ err:
 	}
 
 	return ret;
-
-zero_len:
-	if ((conn->events & (SOCK_FIN_RCVD | TAP_FIN_SENT)) == SOCK_FIN_RCVD) {
-		if ((ret = tcp_send_flag(c, conn, FIN | ACK))) {
-			tcp_rst(c, conn);
-			return ret;
-		}
-
-		conn_event(c, conn, TAP_FIN_SENT);
-	}
-
-	return 0;
 }
 
 /**
-- 
@@ -2193,8 +2193,18 @@ static int tcp_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn)
 	if (len < 0)
 		goto err;
 
-	if (!len)
-		goto zero_len;
+	if (!len) {
+		if ((conn->events & (SOCK_FIN_RCVD | TAP_FIN_SENT)) == SOCK_FIN_RCVD) {
+			if ((ret = tcp_send_flag(c, conn, FIN | ACK))) {
+				tcp_rst(c, conn);
+				return ret;
+			}
+
+			conn_event(c, conn, TAP_FIN_SENT);
+		}
+
+		return 0;
+	}
 
 	sendlen = len - already_sent;
 	if (sendlen <= 0) {
@@ -2233,18 +2243,6 @@ err:
 	}
 
 	return ret;
-
-zero_len:
-	if ((conn->events & (SOCK_FIN_RCVD | TAP_FIN_SENT)) == SOCK_FIN_RCVD) {
-		if ((ret = tcp_send_flag(c, conn, FIN | ACK))) {
-			tcp_rst(c, conn);
-			return ret;
-		}
-
-		conn_event(c, conn, TAP_FIN_SENT);
-	}
-
-	return 0;
 }
 
 /**
-- 
2.39.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/4] Some cleanups to tap and tcp
  2023-02-16  5:43 [PATCH v2 0/4] Some cleanups to tap and tcp David Gibson
                   ` (3 preceding siblings ...)
  2023-02-16  5:43 ` [PATCH v2 4/4] tcp: Remove 'zero_len' " David Gibson
@ 2023-02-16 22:22 ` Stefano Brivio
  2023-02-16 22:55   ` David Gibson
  4 siblings, 1 reply; 8+ messages in thread
From: Stefano Brivio @ 2023-02-16 22:22 UTC (permalink / raw)
  To: David Gibson; +Cc: passt-dev

On Thu, 16 Feb 2023 16:43:07 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> This is a handful of simple cleanups which I made while investigating
> https://bugs.passt.top/show_bug.cgi?id=41.  Note that these don't
> themselves actually address that bug, they're just unrelated cleanups
> that happen to be in adjacent code.
> 
> I've run basic tests on these, but not quite the complete test suite:
> I haven't yet had a chance to debug an unrelated problem with the IPv4
> performance tests failing.
> 
> Changes since v1:
>  * Minor comment and style fixes
> 
> David Gibson (4):
>   tap: Don't pcap frames that didn't get sent
>   tap: Eliminate goto from tap_handler()
>   tcp: Remove 'recvmsg' goto from tcp_data_from_sock
>   tcp: Remove 'zero_len' goto from tcp_data_from_sock

Applied.

-- 
Stefano


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/4] Some cleanups to tap and tcp
  2023-02-16 22:22 ` [PATCH v2 0/4] Some cleanups to tap and tcp Stefano Brivio
@ 2023-02-16 22:55   ` David Gibson
  2023-02-17  7:44     ` Stefano Brivio
  0 siblings, 1 reply; 8+ messages in thread
From: David Gibson @ 2023-02-16 22:55 UTC (permalink / raw)
  To: Stefano Brivio; +Cc: passt-dev

[-- Attachment #1: Type: text/plain, Size: 1180 bytes --]

On Thu, Feb 16, 2023 at 11:22:07PM +0100, Stefano Brivio wrote:
> On Thu, 16 Feb 2023 16:43:07 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > This is a handful of simple cleanups which I made while investigating
> > https://bugs.passt.top/show_bug.cgi?id=41.  Note that these don't
> > themselves actually address that bug, they're just unrelated cleanups
> > that happen to be in adjacent code.
> > 
> > I've run basic tests on these, but not quite the complete test suite:
> > I haven't yet had a chance to debug an unrelated problem with the IPv4
> > performance tests failing.
> > 
> > Changes since v1:
> >  * Minor comment and style fixes
> > 
> > David Gibson (4):
> >   tap: Don't pcap frames that didn't get sent
> >   tap: Eliminate goto from tap_handler()
> >   tcp: Remove 'recvmsg' goto from tcp_data_from_sock
> >   tcp: Remove 'zero_len' goto from tcp_data_from_sock
> 
> Applied.

Hmm.. I'm not seeing them in the tree yet..

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/4] Some cleanups to tap and tcp
  2023-02-16 22:55   ` David Gibson
@ 2023-02-17  7:44     ` Stefano Brivio
  0 siblings, 0 replies; 8+ messages in thread
From: Stefano Brivio @ 2023-02-17  7:44 UTC (permalink / raw)
  To: David Gibson; +Cc: passt-dev

On Fri, 17 Feb 2023 09:55:59 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Feb 16, 2023 at 11:22:07PM +0100, Stefano Brivio wrote:
> > On Thu, 16 Feb 2023 16:43:07 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > This is a handful of simple cleanups which I made while investigating
> > > https://bugs.passt.top/show_bug.cgi?id=41.  Note that these don't
> > > themselves actually address that bug, they're just unrelated cleanups
> > > that happen to be in adjacent code.
> > > 
> > > I've run basic tests on these, but not quite the complete test suite:
> > > I haven't yet had a chance to debug an unrelated problem with the IPv4
> > > performance tests failing.
> > > 
> > > Changes since v1:
> > >  * Minor comment and style fixes
> > > 
> > > David Gibson (4):
> > >   tap: Don't pcap frames that didn't get sent
> > >   tap: Eliminate goto from tap_handler()
> > >   tcp: Remove 'recvmsg' goto from tcp_data_from_sock
> > >   tcp: Remove 'zero_len' goto from tcp_data_from_sock  
> > 
> > Applied.  
> 
> Hmm.. I'm not seeing them in the tree yet..

Oops, yes, I actually pushed it a few minutes after you sent this
email...

-- 
Stefano


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-02-17  7:44 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-16  5:43 [PATCH v2 0/4] Some cleanups to tap and tcp David Gibson
2023-02-16  5:43 ` [PATCH v2 1/4] tap: Don't pcap frames that didn't get sent David Gibson
2023-02-16  5:43 ` [PATCH v2 2/4] tap: Eliminate goto from tap_handler() David Gibson
2023-02-16  5:43 ` [PATCH v2 3/4] tcp: Remove 'recvmsg' goto from tcp_data_from_sock David Gibson
2023-02-16  5:43 ` [PATCH v2 4/4] tcp: Remove 'zero_len' " David Gibson
2023-02-16 22:22 ` [PATCH v2 0/4] Some cleanups to tap and tcp Stefano Brivio
2023-02-16 22:55   ` David Gibson
2023-02-17  7:44     ` Stefano Brivio

Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).