[PATCH 0/8] Don't use additional sockets for receiving "spliced" UDP communications

newer
[PATCH] test/perf/pasta_tcp: Add...

older
[PATCH] test/perf/pasta_udp: Add...

David Gibson

5 Dec 2022 5 Dec '22

9:14 a.m.

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. David Gibson (8): udp: Move sending pasta tap frames to the end of udp_sock_handler() udp: Split sending to passt tap interface into separate function udp: Split receive from preparation and send in udp_sock_handler() udp: Receive multiple datagrams at once on the pasta sock->tap path udp: Pre-populate msg_names with local address udp: Unify udp_sock_handler_splice() with udp_sock_handler() udp: Decide whether to "splice" per datagram rather than per socket udp: Don't use separate sockets to listen for spliced packets udp.c | 382 ++++++++++++++++++++++++++++++--------------------------- udp.h | 2 +- util.h | 7 ++ 3 files changed, 207 insertions(+), 184 deletions(-) -- 2.38.1

Show replies by date

David Gibson

5 Dec 5 Dec

9:14 a.m.

New subject: [PATCH 1/8] udp: Move sending pasta tap frames to the end of udp_sock_handler()

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

David Gibson

9:14 a.m.

New subject: [PATCH 2/8] udp: Split sending to passt tap interface into separate function

The last part of udp_sock_handler() does the actual on-sending of frames to the tap interface. For pasta that's just a call to udp_tap_send_pasta() but for passt, it's a moderately complex and open coded. For symmetry, move the passt send path into its own function, udp_tap_send_passt(). This will make it easier to abstract the tap interface in future (e.g. when we want to add vhost-user). Signed-off-by: David Gibson --- udp.c | 130 ++++++++++++++++++++++++++++++++-------------------------- 1 file changed, 72 insertions(+), 58 deletions(-) diff --git a/udp.c b/udp.c index a27fe88..7281bc3 100644 --- a/udp.c +++ b/udp.c @@ -812,6 +812,73 @@ static void udp_tap_send_pasta(const struct ctx *c, struct mmsghdr *mmh, } } +/** + * udp_tap_send_passt() - Send datagrams to the passt tap interface + * @c: Execution context + * @mmh: Array of message headers to send + * @n: Number of message headers to send + * + * #syscalls:passt sendmmsg sendmsg + */ +static void udp_tap_send_passt(const struct ctx *c, struct mmsghdr *mmh, int n) +{ + struct msghdr *last_mh; + ssize_t missing = 0; + size_t msg_len = 0; + unsigned int i; + int ret; + + ret = sendmmsg(c->fd_tap, mmh, n, MSG_NOSIGNAL | MSG_DONTWAIT); + if (ret <= 0) + return; + + /* If we lose some messages to sendmmsg() here, fine, it's UDP. However, + * the last message needs to be delivered completely, otherwise qemu + * will fail to reassemble the next message and close the connection. Go + * through headers from the last sent message, counting bytes, and, if + * and as soon as we see more bytes than sendmmsg() sent, re-send the + * rest with a blocking call. + * + * In pictures, given this example: + * + * iov #0 iov #1 iov #2 iov #3 + * tap_mmh[ret - 1].msg_hdr: .... ...... ..... ...... + * tap_mmh[ret - 1].msg_len: 7 .... ... + * + * when 'msglen' reaches: 10 ^ + * and 'missing' below is: 3 --- + * + * re-send everything from here: ^-- ----- ------ + */ + last_mh = &mmh[ret - 1].msg_hdr; + for (i = 0; i < last_mh->msg_iovlen; i++) { + if (missing <= 0) { + msg_len += last_mh->msg_iov[i].iov_len; + missing = msg_len - mmh[ret - 1].msg_len; + } + + if (missing > 0) { + uint8_t **iov_base; + int first_offset; + + iov_base = (uint8_t **)&last_mh->msg_iov[i].iov_base; + first_offset = last_mh->msg_iov[i].iov_len - missing; + *iov_base += first_offset; + last_mh->msg_iov[i].iov_len = missing; + + last_mh->msg_iov = &last_mh->msg_iov[i]; + + if (sendmsg(c->fd_tap, last_mh, MSG_NOSIGNAL) < 0) + debug("UDP: %li bytes to tap missing", missing); + + *iov_base -= first_offset; + break; + } + } + + pcapmm(mmh, ret); +} + /** * udp_sock_handler() - Handle new data from socket * @c: Execution context @@ -820,16 +887,14 @@ static void udp_tap_send_pasta(const struct ctx *c, struct mmsghdr *mmh, * @now: Current timestamp * * #syscalls recvmmsg - * #syscalls:passt sendmmsg sendmsg */ 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; - ssize_t n, msg_len = 0, missing = 0; struct mmsghdr *tap_mmh, *sock_mmh; - int msg_bufs = 0, msg_i = 0, ret; - struct msghdr *last_mh; + int msg_bufs = 0, msg_i = 0; + ssize_t n, msg_len = 0; struct iovec *tap_iov; unsigned int i; @@ -879,61 +944,10 @@ void udp_sock_handler(const struct ctx *c, union epoll_ref ref, uint32_t events, } 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); - if (ret <= 0) - return; - - /* If we lose some messages to sendmmsg() here, fine, it's UDP. However, - * the last message needs to be delivered completely, otherwise qemu - * will fail to reassemble the next message and close the connection. Go - * through headers from the last sent message, counting bytes, and, if - * and as soon as we see more bytes than sendmmsg() sent, re-send the - * rest with a blocking call. - * - * In pictures, given this example: - * - * iov #0 iov #1 iov #2 iov #3 - * tap_mmh[ret - 1].msg_hdr: .... ...... ..... ...... - * tap_mmh[ret - 1].msg_len: 7 .... ... - * - * when 'msglen' reaches: 10 ^ - * and 'missing' below is: 3 --- - * - * re-send everything from here: ^-- ----- ------ - */ - last_mh = &tap_mmh[ret - 1].msg_hdr; - for (i = 0, msg_len = 0; i < last_mh->msg_iovlen; i++) { - if (missing <= 0) { - msg_len += last_mh->msg_iov[i].iov_len; - missing = msg_len - tap_mmh[ret - 1].msg_len; - } - - if (missing > 0) { - uint8_t **iov_base; - int first_offset; - - iov_base = (uint8_t **)&last_mh->msg_iov[i].iov_base; - first_offset = last_mh->msg_iov[i].iov_len - missing; - *iov_base += first_offset; - last_mh->msg_iov[i].iov_len = missing; - - last_mh->msg_iov = &last_mh->msg_iov[i]; - - if (sendmsg(c->fd_tap, last_mh, MSG_NOSIGNAL) < 0) - debug("UDP: %li bytes to tap missing", missing); - - *iov_base -= first_offset; - break; - } - } - - pcapmm(tap_mmh, ret); + else + udp_tap_send_passt(c, tap_mmh, msg_i + 1); } /** -- 2.38.1

David Gibson

9:14 a.m.

New subject: [PATCH 3/8] udp: Split receive from preparation and send in udp_sock_handler()

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

David Gibson

9:14 a.m.

New subject: [PATCH 4/8] udp: Receive multiple datagrams at once on the pasta sock->tap path

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. Signed-off-by: David Gibson --- udp.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/udp.c b/udp.c index 64c9219..24fa984 100644 --- a/udp.c +++ b/udp.c @@ -119,7 +119,6 @@ #define UDP_CONN_TIMEOUT 180 /* s, timeout for ephemeral or local bind */ #define UDP_MAX_FRAMES 32 /* max # of frames to receive at once */ -#define UDP_TAP_FRAMES (c->mode == MODE_PASST ? UDP_MAX_FRAMES : 1) /** * struct udp_tap_port - Port tracking based on tap-facing source port @@ -968,7 +967,7 @@ void udp_sock_handler(const struct ctx *c, union epoll_ref ref, uint32_t events, else sock_mmh = udp4_l2_mh_sock; - n = recvmmsg(ref.r.s, sock_mmh, UDP_TAP_FRAMES, 0, NULL); + n = recvmmsg(ref.r.s, sock_mmh, UDP_MAX_FRAMES, 0, NULL); if (n <= 0) return; -- 2.38.1

Stefano Brivio

13 Dec 13 Dec

11:48 p.m.

New subject: [PATCH 4/8] udp: Receive multiple datagrams at once on the pasta sock->tap path

Sorry for the long delay here, On Mon, 5 Dec 2022 19:14:21 +1100 David Gibson 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

David Gibson

14 Dec 14 Dec

2:42 a.m.

New subject: [PATCH 4/8] udp: Receive multiple datagrams at once on the pasta sock->tap path

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

Stefano Brivio

11:35 a.m.

New subject: [PATCH 4/8] udp: Receive multiple datagrams at once on the pasta sock->tap path

On Wed, 14 Dec 2022 12:42:14 +1100 David Gibson 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 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. -- Stefano

Stefano Brivio

20 Dec 20 Dec

11:42 a.m.

New subject: [PATCH 4/8] udp: Receive multiple datagrams at once on the pasta sock->tap path

Sorry for the further delay, On Wed, 14 Dec 2022 11:35:46 +0100 Stefano Brivio wrote:

...

On Wed, 14 Dec 2022 12:42:14 +1100 David Gibson 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 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

David Gibson

21 Dec 21 Dec

7 a.m.

New subject: [PATCH 4/8] udp: Receive multiple datagrams at once on the pasta sock->tap path

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

...
On Wed, 14 Dec 2022 12:42:14 +1100 David Gibson 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 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). -- 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

Stefano Brivio

22 Dec 22 Dec

3:37 a.m.

New subject: [PATCH 4/8] udp: Receive multiple datagrams at once on the pasta sock->tap path

On Wed, 21 Dec 2022 17:00:24 +1100 David Gibson 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 wrote:

...
On Wed, 14 Dec 2022 12:42:14 +1100 David Gibson 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 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

Stefano Brivio

4 Jan 4 Jan

1:08 a.m.

New subject: [PATCH 4/8] udp: Receive multiple datagrams at once on the pasta sock->tap path

On Wed, 21 Dec 2022 17:00:24 +1100 David Gibson 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 wrote:

...
On Wed, 14 Dec 2022 12:42:14 +1100 David Gibson 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 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 - try with sendmsg() instead of sendmmsg(), at this point. Looking at the kernel, that doesn't seem to make a real difference. 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. -- Stefano

David Gibson

5:53 a.m.

New subject: [PATCH 4/8] udp: Receive multiple datagrams at once on the pasta sock->tap path

On Wed, Jan 04, 2023 at 01:08:52AM +0100, Stefano Brivio wrote:

...

On Wed, 21 Dec 2022 17:00:24 +1100 David Gibson 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 wrote:

...
On Wed, 14 Dec 2022 12:42:14 +1100 David Gibson 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 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

David Gibson

5 Dec 5 Dec

9:14 a.m.

New subject: [PATCH 5/8] udp: Pre-populate msg_names with local address

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

Stefano Brivio

13 Dec 13 Dec

11:48 p.m.

New subject: [PATCH 5/8] udp: Pre-populate msg_names with local address

On Mon, 5 Dec 2022 19:14:22 +1100 David Gibson wrote:

...

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 --- 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, +};

Nit, not a strong preference and not worth re-spinning just for this: I think udp4_localname and udp6_localname would be more consistent with everything else here, hence easier to type without double checking. -- Stefano

David Gibson

14 Dec 14 Dec

2:22 a.m.

New subject: [PATCH 5/8] udp: Pre-populate msg_names with local address

On Tue, Dec 13, 2022 at 11:48:52PM +0100, Stefano Brivio wrote:

...

On Mon, 5 Dec 2022 19:14:22 +1100 David Gibson wrote:

...
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 --- 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, +};

Nit, not a strong preference and not worth re-spinning just for this: I think udp4_localname and udp6_localname would be more consistent with everything else here, hence easier to type without double checking.

Good point. I'll change it only if I need a respin for other reasons. -- 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

David Gibson

5 Dec 5 Dec

9:14 a.m.

New subject: [PATCH 6/8] udp: Unify udp_sock_handler_splice() with udp_sock_handler()

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 --- 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)) return; - if (ref.r.p.udp.udp.splice) { - udp_sock_handler_splice(c, ref, events, now); - return; + if (v6) { + mmh_recv = udp6_l2_mh_sock; + udp_localname6.sin6_port = htons(dstport); + } else { + mmh_recv = udp4_l2_mh_sock; + udp_localname4.sin_port = htons(dstport); } - 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_MAX_FRAMES, 0, NULL); + n = recvmmsg(ref.r.s, mmh_recv, UDP_MAX_FRAMES, 0, NULL); if (n <= 0) return; - udp_tap_send(c, 0, n, dstport, v6, now); + 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); + + 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, dstport, v6, + ref.r.p.udp.udp.ns, ref.r.p.udp.udp.orig, + now); + } } /** -- 2.38.1

Stefano Brivio

13 Dec 13 Dec

11:48 p.m.

New subject: [PATCH 6/8] udp: Unify udp_sock_handler_splice() with udp_sock_handler()

On Mon, 5 Dec 2022 19:14:23 +1100 David Gibson 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 --- 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

David Gibson

14 Dec 14 Dec

2:19 a.m.

New subject: [PATCH 6/8] udp: Unify udp_sock_handler_splice() with udp_sock_handler()

On Tue, Dec 13, 2022 at 11:48:58PM +0100, Stefano Brivio wrote:

...

On Mon, 5 Dec 2022 19:14:23 +1100 David Gibson 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 --- 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

Stefano Brivio

11:35 a.m.

New subject: [PATCH 6/8] udp: Unify udp_sock_handler_splice() with udp_sock_handler()

On Wed, 14 Dec 2022 12:19:14 +1100 David Gibson 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 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 --- 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

David Gibson

5 Dec 5 Dec

9:14 a.m.

New subject: [PATCH 7/8] udp: Decide whether to "splice" per datagram rather than per socket

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

Stefano Brivio

13 Dec 13 Dec

11:49 p.m.

New subject: [PATCH 7/8] udp: Decide whether to "splice" per datagram rather than per socket

On Mon, 5 Dec 2022 19:14:24 +1100 David Gibson 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 --- 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. How bad would it be to use an int here? By the way, I think the comment should also mention that the port is returned in host order. -- Stefano

David Gibson

14 Dec 14 Dec

2:47 a.m.

New subject: [PATCH 7/8] udp: Decide whether to "splice" per datagram rather than per socket

On Tue, Dec 13, 2022 at 11:49:18PM +0100, Stefano Brivio wrote:

...

On Mon, 5 Dec 2022 19:14:24 +1100 David Gibson 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 --- 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.

...

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?

...

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

Stefano Brivio

11:35 a.m.

New subject: [PATCH 7/8] udp: Decide whether to "splice" per datagram rather than per socket

On Wed, 14 Dec 2022 12:47:25 +1100 David Gibson 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 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 --- 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

David Gibson

15 Dec 15 Dec

1:33 a.m.

New subject: [PATCH 7/8] udp: Decide whether to "splice" per datagram rather than per socket

On Wed, Dec 14, 2022 at 11:35:54AM +0100, Stefano Brivio wrote:

...

On Wed, 14 Dec 2022 12:47:25 +1100 David Gibson 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 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 --- 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.

I think the int is less ugly than the pointer. Ok, I'll make that change.

...

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

Right. Actually rereading the function in question, it specifically says "port from @sa" and in the sockaddr, of course, it's network endian, so it could particularly do with clarification in this case.

...

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.

Fixed.

...

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

Oops, yes, that's definitely wrong, and wrong enough to respin.

...

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

Well.. yes and no. Yes, we can deliver packets in this way, but we'll lost track of the original from address, so reply packets will be misdirected. Hrm.. ISTR you mentioned some cases that worked despite that, so I'll make the change, and hope to fix it up better when I get to the NAT / splice semantics rework.

...

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;

?

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

David Gibson

5 Dec 5 Dec

9:14 a.m.

New subject: [PATCH 8/8] udp: Don't use separate sockets to listen for spliced packets

Currently, when ports are forwarded inbound in pasta mode, we open two sockets for incoming traffic: one listens on the public IP address and will forward packets to the tuntap interface. The other listens on localhost and forwards via "splicing" (resending directly via sockets in the ns). Now that we've improved the logic about whether we "splice" any individual packet, we don't need this. Instead we can have a single socket bound to 0.0.0.0 or ::, marked as able to splice and udp_sock_handler() will deal with each packet as appropriate. Signed-off-by: David Gibson --- udp.c | 53 +++++++++++++---------------------------------------- 1 file changed, 13 insertions(+), 40 deletions(-) diff --git a/udp.c b/udp.c index 011a157..f7b9bdc 100644 --- a/udp.c +++ b/udp.c @@ -1118,7 +1118,6 @@ void udp_sock_init(const struct ctx *c, int ns, sa_family_t af, const void *addr, const char *ifname, in_port_t port) { union udp_epoll_ref uref = { .u32 = 0 }; - const void *bind_addr; int s; if (ns) { @@ -1130,67 +1129,41 @@ void udp_sock_init(const struct ctx *c, int ns, sa_family_t af, } if ((af == AF_INET || af == AF_UNSPEC) && c->ifi4) { - if (!addr && c->mode == MODE_PASTA) - bind_addr = &c->ip4.addr; - else - bind_addr = addr; - uref.udp.v6 = 0; + uref.udp.splice = (c->mode == MODE_PASTA); + uref.udp.orig = true; if (!ns) { - uref.udp.splice = 0; - s = sock_l4(c, AF_INET, IPPROTO_UDP, bind_addr, ifname, + s = sock_l4(c, AF_INET, IPPROTO_UDP, addr, ifname, port, uref.u32); udp_tap_map[V4][uref.udp.port].sock = s; - - if (c->mode == MODE_PASTA) { - bind_addr = &(uint32_t){ htonl(INADDR_LOOPBACK) }; - uref.udp.splice = uref.udp.orig = true; - - s = sock_l4(c, AF_INET, IPPROTO_UDP, bind_addr, - ifname, port, uref.u32); - udp_splice_init[V4][port].sock = s; - } + udp_splice_init[V4][port].sock = s; } else { - uref.udp.splice = uref.udp.orig = uref.udp.ns = true; - - bind_addr = &(uint32_t){ htonl(INADDR_LOOPBACK) }; + struct in_addr loopback = { htonl(INADDR_LOOPBACK) }; + uref.udp.ns = true; - s = sock_l4(c, AF_INET, IPPROTO_UDP, bind_addr, + s = sock_l4(c, AF_INET, IPPROTO_UDP, &loopback, ifname, port, uref.u32); udp_splice_ns[V4][port].sock = s; } } if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6) { - if (!addr && c->mode == MODE_PASTA) - bind_addr = &c->ip6.addr; - else - bind_addr = addr; - uref.udp.v6 = 1; + uref.udp.splice = (c->mode == MODE_PASTA); + uref.udp.orig = true; if (!ns) { - uref.udp.splice = 0; - s = sock_l4(c, AF_INET6, IPPROTO_UDP, bind_addr, ifname, + s = sock_l4(c, AF_INET6, IPPROTO_UDP, addr, ifname, port, uref.u32); udp_tap_map[V6][uref.udp.port].sock = s; - - if (c->mode == MODE_PASTA) { - bind_addr = &in6addr_loopback; - uref.udp.splice = uref.udp.orig = true; - - s = sock_l4(c, AF_INET6, IPPROTO_UDP, bind_addr, - ifname, port, uref.u32); - udp_splice_init[V6][port].sock = s; - } + udp_splice_init[V6][port].sock = s; } else { - bind_addr = &in6addr_loopback; - uref.udp.splice = uref.udp.orig = uref.udp.ns = true; + uref.udp.ns = true; - s = sock_l4(c, AF_INET6, IPPROTO_UDP, bind_addr, + s = sock_l4(c, AF_INET6, IPPROTO_UDP, &in6addr_loopback, ifname, port, uref.u32); udp_splice_ns[V6][port].sock = s; } -- 2.38.1

Stefano Brivio

6 Dec 6 Dec

7:45 a.m.

On Mon, 5 Dec 2022 19:14:17 +1100 David Gibson 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

Stefano Brivio

7:46 a.m.

On Tue, 6 Dec 2022 07:45:42 +0100 Stefano Brivio wrote:

...

On Mon, 5 Dec 2022 19:14:17 +1100 David Gibson 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.

Oops, sorry, not this one -- I'm still reviewing it. -- Stefano

864

Age (days ago)

894

Last active (days ago)

List overview

Download

27 comments

2 participants

participants (2)

David Gibson
Stefano Brivio

[PATCH 0/8] Don't use additional sockets for receiving "spliced" UDP communications

tags

participants (2)