udp_sock_handler() has a surprising difference in flow between pasta and
passt mode: For pasta we send each frame to the tap interface as we prepare
it.  For passt, though, we prepare all the frames, then send them with a
single sendmmsg().

Alter the pasta path to also prepare all the frames, then send them at the
end.  We already have a suitable data structure for the passt case.  This
will make it easier to abstract out the tap backend difference in future.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 udp.c | 61 ++++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 42 insertions(+), 19 deletions(-)

diff --git a/udp.c b/udp.c
index 4610a02..a27fe88 100644
--- a/udp.c
+++ b/udp.c
@@ -783,6 +783,35 @@ static size_t udp_update_hdr6(const struct ctx *c, int n, in_port_t
dstport,
 	return buf_len;
 }
 
+/**
+ * udp_tap_send_pasta() - Send datagrams to the pasta tap interface
+ * @c:		Execution context
+ * @mmh:	Array of message headers to send
+ * @n:		Number of message headers to send
+ *
+ * #syscalls:pasta write
+ */
+static void udp_tap_send_pasta(const struct ctx *c, struct mmsghdr *mmh,
+			       unsigned int n)
+{
+	unsigned int i, j;
+
+	for (i = 0; i < n; i++) {
+		for (j = 0; j < mmh[i].msg_hdr.msg_iovlen; j++) {
+			struct iovec *iov = &mmh[i].msg_hdr.msg_iov[j];
+
+			/* We can't use writev() because the tap
+			 * character device relies on the write()
+			 * boundaries to discern frame boundaries
+			 */
+			if (write(c->fd_tap, iov->iov_base, iov->iov_len) < 0)
+				debug("tap write: %s", strerror(errno));
+			else
+				pcap(iov->iov_base, iov->iov_len);
+		}
+	}
+}
+
 /**
  * udp_sock_handler() - Handle new data from socket
  * @c:		Execution context
@@ -835,31 +864,25 @@ void udp_sock_handler(const struct ctx *c, union epoll_ref ref,
uint32_t events,
 		else
 			buf_len = udp_update_hdr4(c, i, dstport, now);
 
-		if (c->mode == MODE_PASTA) {
-			void *frame = tap_iov[i].iov_base;
+		tap_iov[i].iov_len = buf_len;
 
-			if (write(c->fd_tap, frame, buf_len) < 0)
-				debug("tap write: %s", strerror(errno));
-			pcap(frame, buf_len);
-		} else {
-			tap_iov[i].iov_len = buf_len;
-
-			/* With bigger messages, qemu closes the connection. */
-			if (msg_bufs && msg_len + buf_len > SHRT_MAX) {
-				tap_mmh[msg_i].msg_hdr.msg_iovlen = msg_bufs;
-				msg_i++;
-				tap_mmh[msg_i].msg_hdr.msg_iov = &tap_iov[i];
-				msg_len = msg_bufs = 0;
-			}
-
-			msg_len += buf_len;
-			msg_bufs++;
+		/* With bigger messages, qemu closes the connection. */
+		if (c->mode == MODE_PASST && msg_bufs &&
+		    msg_len + buf_len > SHRT_MAX) {
+			tap_mmh[msg_i].msg_hdr.msg_iovlen = msg_bufs;
+			msg_i++;
+			tap_mmh[msg_i].msg_hdr.msg_iov = &tap_iov[i];
+			msg_len = msg_bufs = 0;
 		}
+		msg_len += buf_len;
+		msg_bufs++;
 	}
 	tap_mmh[msg_i].msg_hdr.msg_iovlen = msg_bufs;
 
-	if (c->mode == MODE_PASTA)
+	if (c->mode == MODE_PASTA) {
+		udp_tap_send_pasta(c, tap_mmh, msg_i + 1);
 		return;
+	}
 
 	ret = sendmmsg(c->fd_tap, tap_mmh, msg_i + 1,
 		       MSG_NOSIGNAL | MSG_DONTWAIT);
-- 
2.38.1

The receive part of udp_sock_handler() and udp_sock_handler_splice() is now
almost identical.  In preparation for merging that, split the receive part
of udp_sock_handler() from the part preparing and sending the frames for
sending on the tap interface.  The latter goes into a new udp_tap_send()
function.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 udp.c | 79 +++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 52 insertions(+), 27 deletions(-)

diff --git a/udp.c b/udp.c
index 7281bc3..64c9219 100644
--- a/udp.c
+++ b/udp.c
@@ -880,51 +880,39 @@ static void udp_tap_send_passt(const struct ctx *c, struct mmsghdr
*mmh, int n)
 }
 
 /**
- * udp_sock_handler() - Handle new data from socket
+ * udp_tap_send() - Prepare UDP datagrams and send to tap interface
  * @c:		Execution context
- * @ref:	epoll reference
- * @events:	epoll events bitmap
+ * @start:	Index of first datagram in udp[46]_l2_buf pool
+ * @n:		Number of datagrams to send
+ * @dstport:	Destination port number
+ * @v6:		True if using IPv6
  * @now:	Current timestamp
  *
- * #syscalls recvmmsg
+ * Return: size of tap frame with headers
  */
-void udp_sock_handler(const struct ctx *c, union epoll_ref ref, uint32_t events,
-		      const struct timespec *now)
+static void udp_tap_send(const struct ctx *c,
+			 unsigned int start, unsigned int n,
+			 in_port_t dstport, bool v6, const struct timespec *now)
 {
-	in_port_t dstport = ref.r.p.udp.udp.port;
-	struct mmsghdr *tap_mmh, *sock_mmh;
 	int msg_bufs = 0, msg_i = 0;
-	ssize_t n, msg_len = 0;
+	struct mmsghdr *tap_mmh;
 	struct iovec *tap_iov;
+	ssize_t msg_len = 0;
 	unsigned int i;
 
-	if (events == EPOLLERR)
-		return;
-
-	if (ref.r.p.udp.udp.splice) {
-		udp_sock_handler_splice(c, ref, events, now);
-		return;
-	}
-
-	if (ref.r.p.udp.udp.v6) {
+	if (v6) {
 		tap_mmh = udp6_l2_mh_tap;
-		sock_mmh = udp6_l2_mh_sock;
 		tap_iov = udp6_l2_iov_tap;
 	} else {
 		tap_mmh = udp4_l2_mh_tap;
-		sock_mmh = udp4_l2_mh_sock;
 		tap_iov = udp4_l2_iov_tap;
 	}
 
-	n = recvmmsg(ref.r.s, sock_mmh, UDP_TAP_FRAMES, 0, NULL);
-	if (n <= 0)
-		return;
-
-	tap_mmh[0].msg_hdr.msg_iov = &tap_iov[0];
-	for (i = 0; i < (unsigned)n; i++) {
+	tap_mmh[0].msg_hdr.msg_iov = &tap_iov[start];
+	for (i = start; i < start + n; i++) {
 		size_t buf_len;
 
-		if (ref.r.p.udp.udp.v6)
+		if (v6)
 			buf_len = udp_update_hdr6(c, i, dstport, now);
 		else
 			buf_len = udp_update_hdr4(c, i, dstport, now);
@@ -950,6 +938,43 @@ void udp_sock_handler(const struct ctx *c, union epoll_ref ref,
uint32_t events,
 		udp_tap_send_passt(c, tap_mmh, msg_i + 1);
 }
 
+/**
+ * udp_sock_handler() - Handle new data from socket
+ * @c:		Execution context
+ * @ref:	epoll reference
+ * @events:	epoll events bitmap
+ * @now:	Current timestamp
+ *
+ * #syscalls recvmmsg
+ */
+void udp_sock_handler(const struct ctx *c, union epoll_ref ref, uint32_t events,
+		      const struct timespec *now)
+{
+	in_port_t dstport = ref.r.p.udp.udp.port;
+	bool v6 = ref.r.p.udp.udp.v6;
+	struct mmsghdr *sock_mmh;
+	ssize_t n;
+
+	if (events == EPOLLERR)
+		return;
+
+	if (ref.r.p.udp.udp.splice) {
+		udp_sock_handler_splice(c, ref, events, now);
+		return;
+	}
+
+	if (ref.r.p.udp.udp.v6)
+		sock_mmh = udp6_l2_mh_sock;
+	else
+		sock_mmh = udp4_l2_mh_sock;
+
+	n = recvmmsg(ref.r.s, sock_mmh, UDP_TAP_FRAMES, 0, NULL);
+	if (n <= 0)
+		return;
+
+	udp_tap_send(c, 0, n, dstport, v6, now);
+}
+
 /**
  * udp_tap_handler() - Handle packets from tap
  * @c:		Execution context
-- 
2.38.1

udp_splice_namebuf is now used only for spliced sending, and so it only
ever populated with the localhost address, either IPv4 or IPv6.  So,
replace the awkward initialization in udp_sock_handler_splice() with
statically initialized versions for IPv4 and IPv6.  We then just need to
update the port number in udp_sock_handler_splice().

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 udp.c  | 40 ++++++++++++++++++----------------------
 util.h |  7 +++++++
 2 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/udp.c b/udp.c
index 24fa984..7c601cc 100644
--- a/udp.c
+++ b/udp.c
@@ -232,11 +232,18 @@ static struct mmsghdr	udp4_l2_mh_tap		[UDP_MAX_FRAMES];
 static struct mmsghdr	udp6_l2_mh_tap		[UDP_MAX_FRAMES];
 
 /* recvmmsg()/sendmmsg() data for "spliced" connections */
-static struct sockaddr_storage udp_splice_namebuf;
-
 static struct iovec	udp4_iov_splice		[UDP_MAX_FRAMES];
 static struct iovec	udp6_iov_splice		[UDP_MAX_FRAMES];
 
+static struct sockaddr_in udp_localname4 = {
+	.sin_family = AF_INET,
+	.sin_addr = IN4ADDR_LOOPBACK_INIT,
+};
+static struct sockaddr_in6 udp_localname6 = {
+	.sin6_family = AF_INET6,
+	.sin6_addr = IN6ADDR_LOOPBACK_INIT,
+};
+
 static struct mmsghdr	udp4_mh_splice		[UDP_MAX_FRAMES];
 static struct mmsghdr	udp6_mh_splice		[UDP_MAX_FRAMES];
 
@@ -610,23 +617,10 @@ static void udp_sock_handler_splice(const struct ctx *c, union
epoll_ref ref,
 	if (n <= 0)
 		return;
 
-	if (v6) {
-		*((struct sockaddr_in6 *)&udp_splice_namebuf) =
-		 ((struct sockaddr_in6) {
-			.sin6_family = AF_INET6,
-			.sin6_addr = IN6ADDR_LOOPBACK_INIT,
-			.sin6_port = htons(dst),
-			.sin6_scope_id = 0,
-		});
-	} else {
-		*((struct sockaddr_in *)&udp_splice_namebuf) =
-		 ((struct sockaddr_in) {
-			.sin_family = AF_INET,
-			.sin_addr = { .s_addr = htonl(INADDR_LOOPBACK) },
-			.sin_port = htons(dst),
-			.sin_zero = { 0 },
-		});
-	}
+	if (v6)
+		udp_localname6.sin6_port = htons(dst);
+	else
+		udp_localname4.sin_port = htons(dst);
 
 	for (i = 0; i < n; i += m) {
 		in_port_t src = sa_port(v6, mmh_recv[i].msg_hdr.msg_name);
@@ -1252,9 +1246,11 @@ static void udp_splice_iov_init(void)
 		struct msghdr *mh4 = &udp4_mh_splice[i].msg_hdr;
 		struct msghdr *mh6 = &udp6_mh_splice[i].msg_hdr;
 
-		mh4->msg_name = mh6->msg_name = &udp_splice_namebuf;
-		mh4->msg_namelen = sizeof(udp_splice_namebuf);
-		mh6->msg_namelen = sizeof(udp_splice_namebuf);
+		mh4->msg_name = &udp_localname4;
+		mh4->msg_namelen = sizeof(udp_localname4);
+
+		mh6->msg_name = &udp_localname6;
+		mh6->msg_namelen = sizeof(udp_localname6);
 
 		udp4_iov_splice[i].iov_base = udp4_l2_buf[i].data;
 		udp6_iov_splice[i].iov_base = udp6_l2_buf[i].data;
diff --git a/util.h b/util.h
index 02b55a9..3baec91 100644
--- a/util.h
+++ b/util.h
@@ -79,6 +79,13 @@
 	(IN_MULTICAST(ntohl((a)->s_addr)))
 #define IN4_ARE_ADDR_EQUAL(a, b) \
 	(((struct in_addr *)(a))->s_addr == ((struct in_addr *)b)->s_addr)
+#if __BYTE_ORDER == __BIG_ENDIAN
+#define IN4ADDR_LOOPBACK_INIT \
+	{ .s_addr	= INADDR_LOOPBACK }
+#else
+#define IN4ADDR_LOOPBACK_INIT \
+	{ .s_addr	= __bswap_constant_32(INADDR_LOOPBACK) }
+#endif
 
 #define NS_FN_STACK_SIZE	(RLIMIT_STACK_VAL * 1024 / 8)
 int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
-- 
2.38.1

Currently we have special sockets for receiving datagrams from locahost
which can use the optimized "splice" path rather than going across the tap
interface.

We want to loosen this so that sockets can receive sockets that will be
forwarded by both the spliced and non-spliced paths.  To do this, we alter
the meaning of the @splice bit in the reference to mean that packets
receieved on this socket *can* be spliced, not that they *will* be spliced.
They'll only actually be spliced if they come from 127.0.0.1 or ::1.

We can't (for now) remove the splice bit entirely, unlike with TCP.  Our
gateway mapping means that if the ns initiates communication to the gw
address, we'll translate that to target 127.0.0.1 on the host side.  Reply
packets will therefore have source address 127.0.0.1 when received on the
host, but these need to go via the tap path where that will be translated
back to the gateway address.  We need the @splice bit to distinguish that
case from packets going from localhost to a port mapped explicitly with
-u which should be spliced.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 udp.c | 54 +++++++++++++++++++++++++++++++++++-------------------
 udp.h |  2 +-
 2 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/udp.c b/udp.c
index 6ccfe8c..011a157 100644
--- a/udp.c
+++ b/udp.c
@@ -513,16 +513,27 @@ static int udp_splice_new_ns(void *arg)
 }
 
 /**
- * sa_port() - Determine port from a sockaddr_in or sockaddr_in6
+ * udp_mmh_splice_port() - Is source address of message suitable for splicing?
  * @v6:		Is @sa a sockaddr_in6 (otherwise sockaddr_in)?
- * @sa:		Pointer to either sockaddr_in or sockaddr_in6
+ * @mmh:	mmsghdr of incoming message
+ *
+ * Return: if @sa refers to localhost (127.0.0.1 or ::1) the port from
+ *         @sa, otherwise 0.
+ *
+ * NOTE: this relies on the fact that it's not valid to use UDP port 0
  */
-static in_port_t sa_port(bool v6, const void *sa)
+static in_port_t udp_mmh_splice_port(bool v6, const struct mmsghdr *mmh)
 {
-	const struct sockaddr_in6 *sa6 = sa;
-	const struct sockaddr_in *sa4 = sa;
+	const struct sockaddr_in6 *sa6 = mmh->msg_hdr.msg_name;
+	const struct sockaddr_in *sa4 = mmh->msg_hdr.msg_name;;
+
+	if (v6 && IN6_IS_ADDR_LOOPBACK(&sa6->sin6_addr))
+		return ntohs(sa6->sin6_port);
 
-	return v6 ? ntohs(sa6->sin6_port) : ntohs(sa4->sin_port);
+	if (ntohl(sa4->sin_addr.s_addr) == INADDR_LOOPBACK)
+		return ntohs(sa4->sin_port);
+
+	return 0;
 }
 
 /**
@@ -918,23 +929,28 @@ void udp_sock_handler(const struct ctx *c, union epoll_ref ref,
uint32_t events,
 	if (n <= 0)
 		return;
 
-	if (!ref.r.p.udp.udp.splice) {
-		udp_tap_send(c, 0, n, dstport, v6, now);
-		return;
-	}
-
 	for (i = 0; i < n; i += m) {
-		in_port_t src = sa_port(v6, mmh_recv[i].msg_hdr.msg_name);
+		in_port_t splicefrom = 0;
+		m = n;
+
+		if (ref.r.p.udp.udp.splice) {
+			splicefrom = udp_mmh_splice_port(v6, mmh_recv + i);
 
-		for (m = 1; i + m < n; m++) {
-			void *mname = mmh_recv[i + m].msg_hdr.msg_name;
-			if (sa_port(v6, mname) != src)
-				break;
+			for (m = 1; i + m < n; m++) {
+				in_port_t p;
+
+				p = udp_mmh_splice_port(v6, mmh_recv + i + m);
+				if (p != splicefrom)
+					break;
+			}
 		}
 
-		udp_splice_sendfrom(c, i, m, src, dstport, v6,
-				    ref.r.p.udp.udp.ns, ref.r.p.udp.udp.orig,
-				    now);
+		if (splicefrom)
+			udp_splice_sendfrom(c, i, m, splicefrom, dstport,
+					    v6, ref.r.p.udp.udp.ns,
+					    ref.r.p.udp.udp.orig, now);
+		else
+			udp_tap_send(c, i, m, dstport, v6, now);
 	}
 }
 
diff --git a/udp.h b/udp.h
index 053991e..2a03335 100644
--- a/udp.h
+++ b/udp.h
@@ -22,7 +22,7 @@ void udp_update_l2_buf(const unsigned char *eth_d, const unsigned char
*eth_s,
 /**
  * union udp_epoll_ref - epoll reference portion for TCP connections
  * @bound:		Set if this file descriptor is a bound socket
- * @splice:		Set if descriptor is associated to "spliced" connection
+ * @splice:		Set if descriptor packets to be "spliced"
  * @orig:		Set if a spliced socket which can originate "connections"
  * @ns:			Set if this is a socket in the pasta network namespace
  * @v6:			Set for IPv6 sockets or connections
-- 
2.38.1

On Mon,  5 Dec 2022 19:14:17 +1100
David Gibson <david(a)gibson.dropbear.id.au> wrote:

...
  At present, the UDP "splice" and
"tap" paths are quite separate.  We
 have separate sockets to receive packets bound for the tap and splice
 paths.  This leads to some code duplication, and extra open sockets.
 
 This series partially unifies the two paths, allowing us to use a
 single (host side) socket, bound to 0.0.0.0 or :: to receive packets
 for both cases.
 
 This is based on my earlier series with some fixes for the tap path. 
Applied and pushed too.

-- 
Stefano

Sorry for the long delay here,

On Mon,  5 Dec 2022 19:14:21 +1100
David Gibson <david(a)gibson.dropbear.id.au> wrote:

...
  Usually udp_sock_handler() will receive and forward
multiple (up to 32)
 datagrams in udp_sock_handler(), then forward them all to the tap
 interface.  For unclear reasons, though, when in pasta mode we will only
 receive and forward a single datagram at a time.  Change it to receive
 multiple datagrams at once, like the other paths. 
This is explained in the commit message of 6c931118643c ("tcp, udp:
Receive batching doesn't pay off when writing single frames to tap").

I think it's worth re-checking the throughput now as this path is a bit
different, but unfortunately I didn't include this in the "perf" tests :(
because at the time I introduced those I wasn't sure it even made sense to
have traffic from the same host being directed to the tap device.

The iperf3 runs were I observed this are actually the ones from the Podman
demo. Ideally that case should be also checked in the perf/pasta_udp tests.

How fundamental is this for the rest of the series? I couldn't find any
actual dependency on this but I might be missing something.

-- 
Stefano

On Mon,  5 Dec 2022 19:14:23 +1100
David Gibson <david(a)gibson.dropbear.id.au> wrote:

...
  These two functions now have a very similar structure,
and their first
 part (calling recvmmsg()) is functionally identical.  So, merge the two
 functions into one.
 
 Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
 ---
  udp.c | 86 +++++++++++++++++++----------------------------------------
  1 file changed, 28 insertions(+), 58 deletions(-)
 
 diff --git a/udp.c b/udp.c
 index 7c601cc..6ccfe8c 100644
 --- a/udp.c
 +++ b/udp.c
 @@ -590,52 +590,6 @@ static void udp_splice_sendfrom(const struct ctx *c, unsigned start,
unsigned n,
  	sendmmsg(s, mmh_send + start, n, MSG_NOSIGNAL);
  }
  
 -/**
 - * udp_sock_handler_splice() - Handler for socket mapped to "spliced"
connection
 - * @c:		Execution context
 - * @ref:	epoll reference
 - * @events:	epoll events bitmap
 - * @now:	Current timestamp
 - */
 -static void udp_sock_handler_splice(const struct ctx *c, union epoll_ref ref,
 -				    uint32_t events, const struct timespec *now)
 -{
 -	in_port_t dst = ref.r.p.udp.udp.port;
 -	int v6 = ref.r.p.udp.udp.v6, n, i, m;
 -	struct mmsghdr *mmh_recv;
 -
 -	if (!(events & EPOLLIN))
 -		return;
 -
 -	if (v6)
 -		mmh_recv = udp6_l2_mh_sock;
 -	else
 -		mmh_recv = udp4_l2_mh_sock;
 -
 -	n = recvmmsg(ref.r.s, mmh_recv, UDP_MAX_FRAMES, 0, NULL);
 -
 -	if (n <= 0)
 -		return;
 -
 -	if (v6)
 -		udp_localname6.sin6_port = htons(dst);
 -	else
 -		udp_localname4.sin_port = htons(dst);
 -
 -	for (i = 0; i < n; i += m) {
 -		in_port_t src = sa_port(v6, mmh_recv[i].msg_hdr.msg_name);
 -
 -		for (m = 1; i + m < n; m++) {
 -			void *mname = mmh_recv[i + m].msg_hdr.msg_name;
 -			if (sa_port(v6, mname) != src)
 -				break;
 -		}
 -
 -		udp_splice_sendfrom(c, i, m, src, dst, v6, ref.r.p.udp.udp.ns,
 -				    ref.r.p.udp.udp.orig, now);
 -	}
 -}
 -
  /**
   * udp_update_hdr4() - Update headers for one IPv4 datagram
   * @c:		Execution context
 @@ -945,27 +899,43 @@ void udp_sock_handler(const struct ctx *c, union epoll_ref ref,
uint32_t events,
  {
  	in_port_t dstport = ref.r.p.udp.udp.port;
  	bool v6 = ref.r.p.udp.udp.v6;
 -	struct mmsghdr *sock_mmh;
 +	struct mmsghdr *mmh_recv;
 +	unsigned int i, m;
  	ssize_t n;
  
 -	if (events == EPOLLERR)
 +	if (!(events & EPOLLIN)) 
Pre-existing, unrelated issue, but this reminds me: we don't handle
socket errors here, and while udp_timer_one() will drop any sockets we
created, eventually, it would probably be better to act right away.

Not that I have in mind a valid example of an error on UDP sockets,
except perhaps if the interface goes down (but we'll handle that
separately).

-- 
Stefano

On Tue, Dec 13, 2022 at 11:48:58PM +0100, Stefano Brivio wrote:
...
  On Mon,  5 Dec 2022 19:14:23 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
 
  These two functions now have a very similar
structure, and their first
 part (calling recvmmsg()) is functionally identical.  So, merge the two
 functions into one.
 
 Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
 ---
  udp.c | 86 +++++++++++++++++++----------------------------------------
  1 file changed, 28 insertions(+), 58 deletions(-)
 
 diff --git a/udp.c b/udp.c
 index 7c601cc..6ccfe8c 100644
 --- a/udp.c
 +++ b/udp.c
 @@ -590,52 +590,6 @@ static void udp_splice_sendfrom(const struct ctx *c, unsigned start,
unsigned n,
  	sendmmsg(s, mmh_send + start, n, MSG_NOSIGNAL);
  }
  
 -/**
 - * udp_sock_handler_splice() - Handler for socket mapped to "spliced"
connection
 - * @c:		Execution context
 - * @ref:	epoll reference
 - * @events:	epoll events bitmap
 - * @now:	Current timestamp
 - */
 -static void udp_sock_handler_splice(const struct ctx *c, union epoll_ref ref,
 -				    uint32_t events, const struct timespec *now)
 -{
 -	in_port_t dst = ref.r.p.udp.udp.port;
 -	int v6 = ref.r.p.udp.udp.v6, n, i, m;
 -	struct mmsghdr *mmh_recv;
 -
 -	if (!(events & EPOLLIN))
 -		return;
 -
 -	if (v6)
 -		mmh_recv = udp6_l2_mh_sock;
 -	else
 -		mmh_recv = udp4_l2_mh_sock;
 -
 -	n = recvmmsg(ref.r.s, mmh_recv, UDP_MAX_FRAMES, 0, NULL);
 -
 -	if (n <= 0)
 -		return;
 -
 -	if (v6)
 -		udp_localname6.sin6_port = htons(dst);
 -	else
 -		udp_localname4.sin_port = htons(dst);
 -
 -	for (i = 0; i < n; i += m) {
 -		in_port_t src = sa_port(v6, mmh_recv[i].msg_hdr.msg_name);
 -
 -		for (m = 1; i + m < n; m++) {
 -			void *mname = mmh_recv[i + m].msg_hdr.msg_name;
 -			if (sa_port(v6, mname) != src)
 -				break;
 -		}
 -
 -		udp_splice_sendfrom(c, i, m, src, dst, v6, ref.r.p.udp.udp.ns,
 -				    ref.r.p.udp.udp.orig, now);
 -	}
 -}
 -
  /**
   * udp_update_hdr4() - Update headers for one IPv4 datagram
   * @c:		Execution context
 @@ -945,27 +899,43 @@ void udp_sock_handler(const struct ctx *c, union epoll_ref ref,
uint32_t events,
  {
  	in_port_t dstport = ref.r.p.udp.udp.port;
  	bool v6 = ref.r.p.udp.udp.v6;
 -	struct mmsghdr *sock_mmh;
 +	struct mmsghdr *mmh_recv;
 +	unsigned int i, m;
  	ssize_t n;
  
 -	if (events == EPOLLERR)
 +	if (!(events & EPOLLIN)) 
 
 Pre-existing, unrelated issue, but this reminds me: we don't handle
 socket errors here, and while udp_timer_one() will drop any sockets we
 created, eventually, it would probably be better to act right away. 
Ok... I'm not sure what, if anything, you would like me to do about
it, however.

...
  Not that I have in mind a valid example of an error on
UDP sockets,
 except perhaps if the interface goes down (but we'll handle that
 separately). 
-- 
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

On Tue, Dec 13, 2022 at 11:48:47PM +0100, Stefano Brivio wrote:
...
  Sorry for the long delay here,
 
 On Mon,  5 Dec 2022 19:14:21 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
 
  Usually udp_sock_handler() will receive and
forward multiple (up to 32)
 datagrams in udp_sock_handler(), then forward them all to the tap
 interface.  For unclear reasons, though, when in pasta mode we will only
 receive and forward a single datagram at a time.  Change it to receive
 multiple datagrams at once, like the other paths. 
 
 This is explained in the commit message of 6c931118643c ("tcp, udp:
 Receive batching doesn't pay off when writing single frames to tap").
 
 I think it's worth re-checking the throughput now as this path is a bit
 different, but unfortunately I didn't include this in the "perf" tests :(
 because at the time I introduced those I wasn't sure it even made sense to
 have traffic from the same host being directed to the tap device.
 
 The iperf3 runs were I observed this are actually the ones from the Podman
 demo. Ideally that case should be also checked in the perf/pasta_udp tests. 
Hm, ok.

...
  How fundamental is this for the rest of the series? I
couldn't find any
 actual dependency on this but I might be missing something. 
So the issue is that prior to this change in pasta we receive multiple
frames at once on the splice path, but one frame at a time on the tap
path.  By the end of this series we can't do that any more, because we
don't know before the recvmmsg() which one we'll be doing.

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

On Wed, 14 Dec 2022 12:19:14 +1100
David Gibson <david(a)gibson.dropbear.id.au> wrote:

...
  On Tue, Dec 13, 2022 at 11:48:58PM +0100, Stefano
Brivio wrote:
  On Mon,  5 Dec 2022 19:14:23 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
   
  These two functions now have a very similar
structure, and their first
 part (calling recvmmsg()) is functionally identical.  So, merge the two
 functions into one.
 
 Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
 ---
  udp.c | 86 +++++++++++++++++++----------------------------------------
  1 file changed, 28 insertions(+), 58 deletions(-)
 
 diff --git a/udp.c b/udp.c
 index 7c601cc..6ccfe8c 100644
 --- a/udp.c
 +++ b/udp.c
 @@ -590,52 +590,6 @@ static void udp_splice_sendfrom(const struct ctx *c, unsigned start,
unsigned n,
  	sendmmsg(s, mmh_send + start, n, MSG_NOSIGNAL);
  }
  
 -/**
 - * udp_sock_handler_splice() - Handler for socket mapped to "spliced"
connection
 - * @c:		Execution context
 - * @ref:	epoll reference
 - * @events:	epoll events bitmap
 - * @now:	Current timestamp
 - */
 -static void udp_sock_handler_splice(const struct ctx *c, union epoll_ref ref,
 -				    uint32_t events, const struct timespec *now)
 -{
 -	in_port_t dst = ref.r.p.udp.udp.port;
 -	int v6 = ref.r.p.udp.udp.v6, n, i, m;
 -	struct mmsghdr *mmh_recv;
 -
 -	if (!(events & EPOLLIN))
 -		return;
 -
 -	if (v6)
 -		mmh_recv = udp6_l2_mh_sock;
 -	else
 -		mmh_recv = udp4_l2_mh_sock;
 -
 -	n = recvmmsg(ref.r.s, mmh_recv, UDP_MAX_FRAMES, 0, NULL);
 -
 -	if (n <= 0)
 -		return;
 -
 -	if (v6)
 -		udp_localname6.sin6_port = htons(dst);
 -	else
 -		udp_localname4.sin_port = htons(dst);
 -
 -	for (i = 0; i < n; i += m) {
 -		in_port_t src = sa_port(v6, mmh_recv[i].msg_hdr.msg_name);
 -
 -		for (m = 1; i + m < n; m++) {
 -			void *mname = mmh_recv[i + m].msg_hdr.msg_name;
 -			if (sa_port(v6, mname) != src)
 -				break;
 -		}
 -
 -		udp_splice_sendfrom(c, i, m, src, dst, v6, ref.r.p.udp.udp.ns,
 -				    ref.r.p.udp.udp.orig, now);
 -	}
 -}
 -
  /**
   * udp_update_hdr4() - Update headers for one IPv4 datagram
   * @c:		Execution context
 @@ -945,27 +899,43 @@ void udp_sock_handler(const struct ctx *c, union epoll_ref ref,
uint32_t events,
  {
  	in_port_t dstport = ref.r.p.udp.udp.port;
  	bool v6 = ref.r.p.udp.udp.v6;
 -	struct mmsghdr *sock_mmh;
 +	struct mmsghdr *mmh_recv;
 +	unsigned int i, m;
  	ssize_t n;
  
 -	if (events == EPOLLERR)
 +	if (!(events & EPOLLIN))   
 
 Pre-existing, unrelated issue, but this reminds me: we don't handle
 socket errors here, and while udp_timer_one() will drop any sockets we
 created, eventually, it would probably be better to act right away.   
 
 Ok... I'm not sure what, if anything, you would like me to do about
 it, however. 
No no, sorry, nothing, I wanted to share in case you happen to touch
this again soon, or if you had thoughts about it. I can fix this
separately once you're done with changes for UDP.

-- 
Stefano

On Wed, 14 Dec 2022 12:47:25 +1100
David Gibson <david(a)gibson.dropbear.id.au> wrote:

...
  On Tue, Dec 13, 2022 at 11:49:18PM +0100, Stefano
Brivio wrote:
  On Mon,  5 Dec 2022 19:14:24 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
   
  Currently we have special sockets for receiving
datagrams from locahost
 which can use the optimized "splice" path rather than going across the tap
 interface.
 
 We want to loosen this so that sockets can receive sockets that will be
 forwarded by both the spliced and non-spliced paths.  To do this, we alter
 the meaning of the @splice bit in the reference to mean that packets
 receieved on this socket *can* be spliced, not that they *will* be spliced.
 They'll only actually be spliced if they come from 127.0.0.1 or ::1.
 
 We can't (for now) remove the splice bit entirely, unlike with TCP.  Our
 gateway mapping means that if the ns initiates communication to the gw
 address, we'll translate that to target 127.0.0.1 on the host side.  Reply
 packets will therefore have source address 127.0.0.1 when received on the
 host, but these need to go via the tap path where that will be translated
 back to the gateway address.  We need the @splice bit to distinguish that
 case from packets going from localhost to a port mapped explicitly with
 -u which should be spliced.
 
 Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
 ---
  udp.c | 54 +++++++++++++++++++++++++++++++++++-------------------
  udp.h |  2 +-
  2 files changed, 36 insertions(+), 20 deletions(-)
 
 diff --git a/udp.c b/udp.c
 index 6ccfe8c..011a157 100644
 --- a/udp.c
 +++ b/udp.c
 @@ -513,16 +513,27 @@ static int udp_splice_new_ns(void *arg)
  }
  
  /**
 - * sa_port() - Determine port from a sockaddr_in or sockaddr_in6
 + * udp_mmh_splice_port() - Is source address of message suitable for splicing?
   * @v6:		Is @sa a sockaddr_in6 (otherwise sockaddr_in)?
 - * @sa:		Pointer to either sockaddr_in or sockaddr_in6
 + * @mmh:	mmsghdr of incoming message
 + *
 + * Return: if @sa refers to localhost (127.0.0.1 or ::1) the port from
 + *         @sa, otherwise 0.
 + *
 + * NOTE: this relies on the fact that it's not valid to use UDP port 0   
 
 The port is reserved by IANA indeed, but... it can actually be used. On
 Linux, you can bind() it and you can connect() to it. As far as I can
 tell from the new version of udp_sock_handler() we would actually
 misdirect packets in that case.   
 
 Hm, ok.  Given the IANA reservation, I think it would be acceptable to
 simply drop such packets - but if we were to make that choice we
 should do so explicitly, rather than misdirecting them. 
Acceptable, sure, but... I don't know, it somehow doesn't look
desirable to me. The kernel doesn't enforce this, so I guess we
shouldn't either.

...
 
  How bad would
it be to use an int here?   
 
 Pretty straightforward.  Just means we have to use the somewhat
 abtruse "if (port <= USHRT_MAX)" or "if (port >= 0)" or
something
 instead of just "if (port)".  Should I go ahead and make that change? 
Eh, I don't like it either, but... I guess it's better than the
alternative, so yes, thanks. Or pass port as a pointer, set on return.
I'm fine with both.

...
 
  By the way, I
think the comment should also mention that the port is
 returned in host order.   
 
 Ok, easily done.  Generally I try to keep the endianness associated
 with the type, rather than attempting to document it for each variable
 (or even worse, each point in time for each variable). 
Yes, I see, and it's a more valid approach than mine, but still mine
comes almost for free.

By the way, I got distracted by this and I forgot about two things:

...
  +static in_port_t udp_mmh_splice_port(bool v6, const
struct mmsghdr *mmh)
  {
 -	const struct sockaddr_in6 *sa6 = sa;
 -	const struct sockaddr_in *sa4 = sa;
 +	const struct sockaddr_in6 *sa6 = mmh->msg_hdr.msg_name;
 +	const struct sockaddr_in *sa4 = mmh->msg_hdr.msg_name;; 
Stray semicolon here.

...
  +
 +	if (v6 && IN6_IS_ADDR_LOOPBACK(&sa6->sin6_addr))
 +		return ntohs(sa6->sin6_port);
  
 -	return v6 ? ntohs(sa6->sin6_port) : ntohs(sa4->sin_port);
 +	if (ntohl(sa4->sin_addr.s_addr) == INADDR_LOOPBACK)
 +		return ntohs(sa4->sin_port); 
If it's IPv6, but not a loopback address, we'll check if
sa4->sin_addr.s_addr == INADDR_LOOPBACK -- which might actually be true for
an IPv6, non-loopback address.

Also, I think we can happily "splice" for any loopback address, not
just 127.0.0.1. What about something like:

	if (v6 && IN6_IS_ADDR_LOOPBACK(&sa6->sin6_addr))
		return ntohs(sa6->sin6_port);

	if (!v4 && IN4_IS_ADDR_LOOPBACK(&sa4->sin_addr))
		return ntohs(sa4->sin_port);

	return -1;

?

-- 
Stefano

Sorry for the further delay,

On Wed, 14 Dec 2022 11:35:46 +0100
Stefano Brivio <sbrivio(a)redhat.com> wrote:

...
  On Wed, 14 Dec 2022 12:42:14 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
 
  On Tue, Dec 13, 2022 at 11:48:47PM +0100, Stefano
Brivio wrote:  
  Sorry for the long delay here,
 
 On Mon,  5 Dec 2022 19:14:21 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
     
  Usually udp_sock_handler() will receive and
forward multiple (up to 32)
 datagrams in udp_sock_handler(), then forward them all to the tap
 interface.  For unclear reasons, though, when in pasta mode we will only
 receive and forward a single datagram at a time.  Change it to receive
 multiple datagrams at once, like the other paths.     
 
 This is explained in the commit message of 6c931118643c ("tcp, udp:
 Receive batching doesn't pay off when writing single frames to tap").
 
 I think it's worth re-checking the throughput now as this path is a bit
 different, but unfortunately I didn't include this in the "perf" tests :(
 because at the time I introduced those I wasn't sure it even made sense to
 have traffic from the same host being directed to the tap device.
 
 The iperf3 runs were I observed this are actually the ones from the Podman
 demo. Ideally that case should be also checked in the perf/pasta_udp tests.     
 
 Hm, ok.
   
  How fundamental is this for the rest of the
series? I couldn't find any
 actual dependency on this but I might be missing something.     
 
 So the issue is that prior to this change in pasta we receive multiple
 frames at once on the splice path, but one frame at a time on the tap
 path.  By the end of this series we can't do that any more, because we
 don't know before the recvmmsg() which one we'll be doing.   
 
 Oh, right, I see. Then let me add this path to the perf/pasta_udp test
 and check how relevant this is now, I'll get back to you in a bit. 
I was checking the wrong path. With this:

--
diff --git a/test/perf/pasta_udp b/test/perf/pasta_udp
index 27ea724..973c2f4 100644
--- a/test/perf/pasta_udp
+++ b/test/perf/pasta_udp
@@ -31,6 +31,14 @@ report	pasta lo_udp 1 __FREQ__
 
 th	MTU 1500B 4000B 16384B 65535B
 
+tr	UDP throughput over IPv6: host to ns
+nsout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type ==
"ether").ifname'
+nsout	ADDR6 ip -j -6 addr show|jq -rM '.[] | select(.ifname ==
"__IFNAME__").addr_info[] | select(.scope == "global" and .prefixlen
== 64).local'
+bw	-
+bw	-
+bw	-
+iperf3	BW host ns __ADDR6__ 100${i}2 __THREADS__ __TIME__ __OPTS__ -b 15G
+bw	__BW__ 7.0 9.0
 
 tr	UDP throughput over IPv6: ns to host
 ns	ip link set dev lo mtu 1500
diff --git a/test/run b/test/run
index e07513f..b53182b 100755
--- a/test/run
+++ b/test/run
@@ -67,6 +67,14 @@ run() {
 	test build/clang_tidy
 	teardown build
 
+	VALGRIND=0
+	setup passt_in_ns
+	test passt/ndp
+	test passt/dhcp
+	test perf/pasta_udp
+	test passt_in_ns/shutdown
+	teardown passt_in_ns
+
 	setup pasta
 	test pasta/ndp
 	test pasta/dhcp
--

I get 21.6 gbps after this series, and 29.7 gbps before -- it's quite
significant.

And there's nothing strange in perf's output, really, the distribution
of overhead per functions is pretty much the same, but writing multiple
messages to the tap device just takes more cycles per message compared
to a single message.

I'm a bit ashamed to propose this, but do you think about something
like:

	if (c->mode == MODE_PASTA) {
		if (recvmmsg(ref.r.s, mmh_recv, 1, 0, NULL) <= 0)
			return;

		if (udp_mmh_splice_port(v6, mmh_recv)) {
			n = recvmmsg(ref.r.s, mmh_recv + 1,
				     UDP_MAX_FRAMES - 1, 0, NULL);
		}

		if (n > 0)
			n++;
		else
			n = 1;
	} else {
		n = recvmmsg(ref.r.s, mmh_recv, UDP_MAX_FRAMES, 0, NULL);
		if (n <= 0)
			return;
	}

? Other than the inherent ugliness, it looks like a good approximation
to me.

-- 
Stefano

On Wed, 21 Dec 2022 17:00:24 +1100
David Gibson <david(a)gibson.dropbear.id.au> wrote:

...
  On Tue, Dec 20, 2022 at 11:42:46AM +0100, Stefano
Brivio wrote:
  Sorry for the further delay,
 
 On Wed, 14 Dec 2022 11:35:46 +0100
 Stefano Brivio <sbrivio(a)redhat.com> wrote:
   
  On Wed, 14 Dec 2022 12:42:14 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
   
  On Tue, Dec 13, 2022 at 11:48:47PM +0100, Stefano
Brivio wrote:    
 > Sorry for the long delay here,
 > 
 > On Mon,  5 Dec 2022 19:14:21 +1100
 > David Gibson <david(a)gibson.dropbear.id.au> wrote:
 >       
 > > Usually udp_sock_handler() will receive and forward multiple (up to 32)
 > > datagrams in udp_sock_handler(), then forward them all to the tap
 > > interface.  For unclear reasons, though, when in pasta mode we will only
 > > receive and forward a single datagram at a time.  Change it to receive
 > > multiple datagrams at once, like the other paths.      
 > 
 > This is explained in the commit message of 6c931118643c ("tcp, udp:
 > Receive batching doesn't pay off when writing single frames to tap").
 > 
 > I think it's worth re-checking the throughput now as this path is a bit
 > different, but unfortunately I didn't include this in the "perf"
tests :(
 > because at the time I introduced those I wasn't sure it even made sense to
 > have traffic from the same host being directed to the tap device.
 > 
 > The iperf3 runs were I observed this are actually the ones from the Podman
 > demo. Ideally that case should be also checked in the perf/pasta_udp tests.      
 
 Hm, ok.
     
 > How fundamental is this for the rest of the series? I couldn't find any
 > actual dependency on this but I might be missing something.      
 
 So the issue is that prior to this change in pasta we receive multiple
 frames at once on the splice path, but one frame at a time on the tap
 path.  By the end of this series we can't do that any more, because we
 don't know before the recvmmsg() which one we'll be doing.     
 
 Oh, right, I see. Then let me add this path to the perf/pasta_udp test
 and check how relevant this is now, I'll get back to you in a bit.   
 
 I was checking the wrong path. With this:
 
 diff --git a/test/perf/pasta_udp b/test/perf/pasta_udp
 index 27ea724..973c2f4 100644
 --- a/test/perf/pasta_udp
 +++ b/test/perf/pasta_udp
 @@ -31,6 +31,14 @@ report	pasta lo_udp 1 __FREQ__
  
  th	MTU 1500B 4000B 16384B 65535B
  
 +tr	UDP throughput over IPv6: host to ns
 +nsout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type ==
"ether").ifname'
 +nsout	ADDR6 ip -j -6 addr show|jq -rM '.[] | select(.ifname ==
"__IFNAME__").addr_info[] | select(.scope == "global" and .prefixlen
== 64).local'
 +bw	-
 +bw	-
 +bw	-
 +iperf3	BW host ns __ADDR6__ 100${i}2 __THREADS__ __TIME__ __OPTS__ -b 15G
 +bw	__BW__ 7.0 9.0
  
  tr	UDP throughput over IPv6: ns to host
  ns	ip link set dev lo mtu 1500
 diff --git a/test/run b/test/run
 index e07513f..b53182b 100755
 --- a/test/run
 +++ b/test/run
 @@ -67,6 +67,14 @@ run() {
  	test build/clang_tidy
  	teardown build
  
 +	VALGRIND=0
 +	setup passt_in_ns
 +	test passt/ndp
 +	test passt/dhcp
 +	test perf/pasta_udp
 +	test passt_in_ns/shutdown
 +	teardown passt_in_ns
 +
  	setup pasta
  	test pasta/ndp
  	test pasta/dhcp   
 
 Ah, ok.  Can we add that to the standard set of tests ASAP, please. 
Yes -- that part itself was easy, but now I'm fighting against my own
finest write-only code that generates the JavaScript snippet for the
performance report (perf_fill_lines() in test/lib/perf_report -- and
this is not a suggestion to have a look at it ;)).

I'm trying to rework it a bit together with the "new" test.

...
 
  I get 21.6
gbps after this series, and 29.7 gbps before -- it's quite
 significant.   
 
 Drat.
 
  And there's nothing strange in perf's
output, really, the distribution
 of overhead per functions is pretty much the same, but writing multiple
 messages to the tap device just takes more cycles per message compared
 to a single message.   
 
 That's so weird.  It should be basically an identical set of write()s,
 except that they happen in a batch, rather than a bit spread out.  I
 guess it has to be some kind of cache locality thing.  I wonder if the
 difference would go away or reverse if we had a way to submit multiple
 frames with a single syscall. 
I haven't tried, but to test this, I think we could actually just write
multiple frames in a single call, with subsequent headers and
everything, and the iperf3 server will simply report how many bytes it
received.

...
 
  I'm a bit
ashamed to propose this, but do you think about something
 like:   
 
  	if (c->mode == MODE_PASTA) { if
(recvmmsg(ref.r.s, mmh_recv,
 		1, 0, NULL) <= 0) return;   
 
  		if (udp_mmh_splice_port(v6, mmh_recv)) { n =
 			recvmmsg(ref.r.s, mmh_recv + 1, UDP_MAX_FRAMES
 			- 1, 0, NULL); }   
 
  		if (n > 0) n++; else n = 1; } else { n =
 			recvmmsg(ref.r.s, mmh_recv, UDP_MAX_FRAMES, 0,
 			NULL); if (n <= 0) return; }   
 
  ? Other than the inherent ugliness, it looks like
a good
 approximation to me.   
 
 Hmm.  Well, the first question is how much impact does going 1 message
 at a time have on the spliced throughput.  If it's not too bad, then
 we could just always go one at a time for pasta, regardless of
 splicing.  And we could even abstract that difference into the tap
 backend with a callback like tap_batch_size(c). 
Right... it used to be significantly worse in the "spliced" case, I
checked that when I did that commit to use 1 instead of UDP_MAX_FRAME
in the other case, but I don't have data. I'll test this again.

-- 
Stefano

On Wed, Jan 04, 2023 at 01:08:52AM +0100, Stefano Brivio wrote:
...
  On Wed, 21 Dec 2022 17:00:24 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
 
  On Tue, Dec 20, 2022 at 11:42:46AM +0100, Stefano
Brivio wrote:
  Sorry for the further delay,
 
 On Wed, 14 Dec 2022 11:35:46 +0100
 Stefano Brivio <sbrivio(a)redhat.com> wrote:
   
  On Wed, 14 Dec 2022 12:42:14 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
   
 > On Tue, Dec 13, 2022 at 11:48:47PM +0100, Stefano Brivio wrote:    
 > > Sorry for the long delay here,
 > > 
 > > On Mon,  5 Dec 2022 19:14:21 +1100
 > > David Gibson <david(a)gibson.dropbear.id.au> wrote:
 > >       
 > > > Usually udp_sock_handler() will receive and forward multiple (up to 32)
 > > > datagrams in udp_sock_handler(), then forward them all to the tap
 > > > interface.  For unclear reasons, though, when in pasta mode we will only
 > > > receive and forward a single datagram at a time.  Change it to receive
 > > > multiple datagrams at once, like the other paths.      
 > > 
 > > This is explained in the commit message of 6c931118643c ("tcp, udp:
 > > Receive batching doesn't pay off when writing single frames to
tap").
 > > 
 > > I think it's worth re-checking the throughput now as this path is a bit
 > > different, but unfortunately I didn't include this in the "perf"
tests :(
 > > because at the time I introduced those I wasn't sure it even made sense
to
 > > have traffic from the same host being directed to the tap device.
 > > 
 > > The iperf3 runs were I observed this are actually the ones from the Podman
 > > demo. Ideally that case should be also checked in the perf/pasta_udp tests.    
 
 > 
 > Hm, ok.
 >     
 > > How fundamental is this for the rest of the series? I couldn't find any
 > > actual dependency on this but I might be missing something.      
 > 
 > So the issue is that prior to this change in pasta we receive multiple
 > frames at once on the splice path, but one frame at a time on the tap
 > path.  By the end of this series we can't do that any more, because we
 > don't know before the recvmmsg() which one we'll be doing.    
 
 Oh, right, I see. Then let me add this path to the perf/pasta_udp test
 and check how relevant this is now, I'll get back to you in a bit.   
 
 I was checking the wrong path. With this:
 
 diff --git a/test/perf/pasta_udp b/test/perf/pasta_udp
 index 27ea724..973c2f4 100644
 --- a/test/perf/pasta_udp
 +++ b/test/perf/pasta_udp
 @@ -31,6 +31,14 @@ report	pasta lo_udp 1 __FREQ__
  
  th	MTU 1500B 4000B 16384B 65535B
  
 +tr	UDP throughput over IPv6: host to ns
 +nsout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type ==
"ether").ifname'
 +nsout	ADDR6 ip -j -6 addr show|jq -rM '.[] | select(.ifname ==
"__IFNAME__").addr_info[] | select(.scope == "global" and .prefixlen
== 64).local'
 +bw	-
 +bw	-
 +bw	-
 +iperf3	BW host ns __ADDR6__ 100${i}2 __THREADS__ __TIME__ __OPTS__ -b 15G
 +bw	__BW__ 7.0 9.0
  
  tr	UDP throughput over IPv6: ns to host
  ns	ip link set dev lo mtu 1500
 diff --git a/test/run b/test/run
 index e07513f..b53182b 100755
 --- a/test/run
 +++ b/test/run
 @@ -67,6 +67,14 @@ run() {
  	test build/clang_tidy
  	teardown build
  
 +	VALGRIND=0
 +	setup passt_in_ns
 +	test passt/ndp
 +	test passt/dhcp
 +	test perf/pasta_udp
 +	test passt_in_ns/shutdown
 +	teardown passt_in_ns
 +
  	setup pasta
  	test pasta/ndp
  	test pasta/dhcp   
 
 Ah, ok.  Can we add that to the standard set of tests ASAP, please.
 
  I get 21.6 gbps after this series, and 29.7 gbps
before -- it's quite
 significant.   
 
 Drat.
 
  And there's nothing strange in perf's
output, really, the distribution
 of overhead per functions is pretty much the same, but writing multiple
 messages to the tap device just takes more cycles per message compared
 to a single message.   
 
 That's so weird.  It should be basically an identical set of write()s,
 except that they happen in a batch, rather than a bit spread out.  I
 guess it has to be some kind of cache locality thing.  I wonder if the
 difference would go away or reverse if we had a way to submit multiple
 frames with a single syscall.
 
  I'm a bit ashamed to propose this, but do
you think about something
 like:   
 
  	if (c->mode == MODE_PASTA) { if
(recvmmsg(ref.r.s, mmh_recv,
 		1, 0, NULL) <= 0) return;   
 
  		if (udp_mmh_splice_port(v6, mmh_recv)) { n =
 			recvmmsg(ref.r.s, mmh_recv + 1, UDP_MAX_FRAMES
 			- 1, 0, NULL); }   
 
  		if (n > 0) n++; else n = 1; } else { n =
 			recvmmsg(ref.r.s, mmh_recv, UDP_MAX_FRAMES, 0,
 			NULL); if (n <= 0) return; }   
 
  ? Other than the inherent ugliness, it looks like
a good
 approximation to me.   
 
 Hmm.  Well, the first question is how much impact does going 1 message
 at a time have on the spliced throughput.  If it's not too bad, then
 we could just always go one at a time for pasta, regardless of
 splicing.  And we could even abstract that difference into the tap
 backend with a callback like tap_batch_size(c). 
 
 So, finally I had the chance to try this out.
 
 First off, baseline with the patch adding the new tests I just sent,
 and the series you posted:
 
 === perf/pasta_udp
  pasta: throughput and latency (local traffic)
 Throughput in Gbps, latency in µs, one thread at 3.6 GHz, 4 streams
                                                          MTU: |  1500B |  4000B | 16384B
| 65535B |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv6: ns to host             |    4.4 |    8.5 |   19.5
|   23.0 |
              UDP RR latency over IPv6: ns to host             |      - |      - |      -
|     27 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv4: ns to host             |    4.3 |    8.8 |   18.5
|   24.4 |
              UDP RR latency over IPv4: ns to host             |      - |      - |      -
|     26 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv6: host to ns             |      - |      - |      -
|   22.5 |
              UDP RR latency over IPv6: host to ns             |      - |      - |      -
|     30 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv4: host to ns             |      - |      - |      -
|   24.5 |
              UDP RR latency over IPv4: host to ns             |      - |      - |      -
|     25 |
                                                              
'--------'--------'--------'--------'
 ...passed.
 
  pasta: throughput and latency (traffic via tap)
 Throughput in Gbps, latency in µs, one thread at 3.6 GHz, 4 streams
                                                          MTU: |  1500B |  4000B | 16384B
| 65520B |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv6: ns to host             |    4.4 |   10.4 |   16.0
|   23.4 |
              UDP RR latency over IPv6: ns to host             |      - |      - |      -
|     27 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv4: ns to host             |    5.2 |   10.8 |   16.0
|   24.0 |
              UDP RR latency over IPv4: ns to host             |      - |      - |      -
|     28 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv6: host to ns             |      - |      - |      -
|   21.5 |
              UDP RR latency over IPv6: host to ns             |      - |      - |      -
|     29 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv4: host to ns             |      - |      - |      -
|   26.3 |
              UDP RR latency over IPv4: host to ns             |      - |      - |      -
|     26 |
                                                              
'--------'--------'--------'--------'
 
 which seems to indicate the whole "splicing" thing is pretty much
 useless, for UDP (except for that 16 KiB MTU case, but I wonder how
 relevant that is).
 
 If I set UDP_MAX_FRAMES to 1, with a quick workaround for the resulting
 warning in udp_tap_send() (single frame to send, hence single message),
 it gets somewhat weird:
 
 === perf/pasta_udp
  pasta: throughput and latency (local traffic)
 Throughput in Gbps, latency in µs, one thread at 3.6 GHz, 4 streams
                                                          MTU: |  1500B |  4000B | 16384B
| 65535B |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv6: ns to host             |    3.4 |    7.0 |   21.6
|   31.6 |
              UDP RR latency over IPv6: ns to host             |      - |      - |      -
|     30 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv4: ns to host             |    3.8 |    7.0 |   22.0
|   32.4 |
              UDP RR latency over IPv4: ns to host             |      - |      - |      -
|     26 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv6: host to ns             |      - |      - |      -
|   29.3 |
              UDP RR latency over IPv6: host to ns             |      - |      - |      -
|     31 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv4: host to ns             |      - |      - |      -
|   33.8 |
              UDP RR latency over IPv4: host to ns             |      - |      - |      -
|     25 |
                                                              
'--------'--------'--------'--------'
 ...passed.
 
  pasta: throughput and latency (traffic via tap)
 Throughput in Gbps, latency in µs, one thread at 3.6 GHz, 4 streams
                                                          MTU: |  1500B |  4000B | 16384B
| 65520B |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv6: ns to host             |    4.7 |   10.3 |   16.0
|   24.0 |
              UDP RR latency over IPv6: ns to host             |      - |      - |      -
|     27 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv4: ns to host             |    5.6 |   11.4 |   16.0
|   24.0 |
              UDP RR latency over IPv4: ns to host             |      - |      - |      -
|     26 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv6: host to ns             |      - |      - |      -
|   21.5 |
              UDP RR latency over IPv6: host to ns             |      - |      - |      -
|     29 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv4: host to ns             |      - |      - |      -
|   28.7 |
              UDP RR latency over IPv4: host to ns             |      - |      - |      -
|     29 |
                                                              
'--------'--------'--------'--------'
 
 ...except for the cases with low MTUs, throughput is significantly
 higher if we read and send one message at a time on the "spliced" path.
 
 Next, I would like to:
 
 - bisect between 32 and 1 for UDP_MAX_FRAMES: maybe 32 affects data
   locality too much, but some lower value would still be beneficial by
   lowering syscall overhead 
Ok.

...
  - try with sendmsg() instead of sendmmsg(), at this
point. Looking at
   the kernel, that doesn't seem to make a real difference. 
Which sendmmsg() specifically are you looking at changing?

...
  About this series: should we just go ahead and apply
it with
 UDP_MAX_FRAMES set to 1 for the moment being? It's anyway better than
 the existing situation. 
I think that's a good idea - or rather, not setting UDP_MAX_FRAMES to
1, but clamping the batch size to 1 for pasta - I'm pretty sure we
still want the batching for passt.  We lose a little bit on
small-packet spliced, but we gain on both tap and large-packet
spliced.  This will unblock the dual stack udp stuff and we can
further tune it later.

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