On Sat, Sep 23, 2023 at 12:06:06AM +0200, Stefano Brivio wrote:

Note that tcp_sock_consume() doesn't update ACK sequence counters anymore.

Fixes: cc6d8286d104 ("tcp: Reset ACK_FROM_TAP_DUE flag only as needed, update timer") Signed-off-by: Stefano Brivio

Reviewed-by: David Gibson

--- tcp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcp.c b/tcp.c index dd3142d..aa1c8c9 100644 --- a/tcp.c +++ b/tcp.c @@ -2103,7 +2103,7 @@ static void tcp_conn_from_tap(struct ctx *c, }

/** - * tcp_sock_consume() - Consume (discard) data from buffer, update ACK sequence + * tcp_sock_consume() - Consume (discard) data from buffer * @conn: Connection pointer * @ack_seq: ACK sequence, host order *

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

I think the change itself here is sound, but I have some nits to pick with the description and reasoning. On Sat, Sep 23, 2023 at 12:06:07AM +0200, Stefano Brivio wrote:

In tcp_tap_handler(), we shouldn't reset the STALLED flag (indicating that we ran out of tap-side window space, or that all available socket data is already in flight -- better names welcome!

Hmm.. when you put it like that it makes me wonder if those two quite different conditions really need the same handling. Hrm.. I guess both conditions mean that we can't accept data from the socket, even if it's availble.

) on any event: do that only if the first packet in a batch has the ACK flag set.

"First packet in a batch" may not be accurate here - we're looking at whichever packet we were up to before calling data_from_tap(). There could have been earlier packets in the receive batch that were already processed. This also raises the question of why the first data packet should be particularly privileged here. I'm wondering if what we really want to check is whether data_from_tap() advanced the ack pointer at all. I'm not clear on when the th->ack check would ever fail in practice: aren't the only normal packets in a TCP connection without ACK the initial SYN or an RST? We've handled the SYN case earlier, so should we just have a blanket case above this that if we get a packet with !ACK, we reset the connection?

Make sure we check for pending socket data when we reset it: reverting back to level-triggered epoll events, as tcp_epoll_ctl() does, isn't guaranteed to actually trigger a socket event.

Which sure seems like a kernel bug. Some weird edge conditions for edge-triggered seems expected, but this doesn't seem like valid level-triggered semantics. Hmmm... is toggling EPOLLET even what we want. IIUC, the heart of what's going on here is that we can't take more data from the socket until something happens on the tap side (either the window expands, or it acks some data). In which case should we be toggling EPOLLIN on the socket instead? That seems more explicitly to be saying to the socket side "we don't currently care if you have data available".

Further, note that the flag only makes sense once a connection is established, so move all this to the right place, which is convenient for the next patch, as we want to check if the STALLED flag was set before processing any new information about the window size advertised by the tap.

Signed-off-by: Stefano Brivio --- tcp.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tcp.c b/tcp.c index aa1c8c9..5528e05 100644 --- a/tcp.c +++ b/tcp.c @@ -2572,8 +2572,6 @@ int tcp_tap_handler(struct ctx *c, int af, const void *saddr, const void *daddr, if (th->ack && !(conn->events & ESTABLISHED)) tcp_update_seqack_from_tap(c, conn, ntohl(th->ack_seq));

- conn_flag(c, conn, ~STALLED); - /* Establishing connection from socket */ if (conn->events & SOCK_ACCEPTED) { if (th->syn && th->ack && !th->fin) { @@ -2631,6 +2629,11 @@ int tcp_tap_handler(struct ctx *c, int af, const void *saddr, const void *daddr, if (conn->seq_ack_to_tap != conn->seq_from_tap) ack_due = 1;

+ if ((conn->flags & STALLED) && th->ack) { + conn_flag(c, conn, ~STALLED); + tcp_data_from_sock(c, conn); + } + if ((conn->events & TAP_FIN_RCVD) && !(conn->events & SOCK_FIN_SENT)) { shutdown(conn->sock, SHUT_WR); conn_event(c, conn, SOCK_FIN_SENT);

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

On Sat, Sep 23, 2023 at 12:06:08AM +0200, Stefano Brivio wrote:

It looks like we need it as workaround for this situation, readily reproducible at least with a 6.5 Linux kernel, with default rmem_max and wmem_max values:

- an iperf3 client on the host sends about 160 KiB, typically segmented into five frames by passt. We read this data using MSG_PEEK

- the iperf3 server on the guest starts receiving

- meanwhile, the host kernel advertised a zero-sized window to the receiver, as expected

- eventually, the guest acknowledges all the data sent so far, and we drop it from the buffer, courtesy of tcp_sock_consume(), using recv() with MSG_TRUNC

- the client, however, doesn't get an updated window value, and even keepalive packets are answered with zero-window segments, until the connection is closed

It looks like dropping data from a socket using MSG_TRUNC doesn't cause a recalculation of the window, which would be expected as a result of any receiving operation that invalidates data on a buffer (that is, not with MSG_PEEK).

Strangely enough, setting TCP_WINDOW_CLAMP via setsockopt(), even to the previous value we clamped to, forces a recalculation of the window which is advertised to the guest.

I couldn't quite confirm this issue by following all the possible code paths in the kernel, yet. If confirmed, this should be fixed in the kernel, but meanwhile this workaround looks robust to me (and it will be needed for backward compatibility anyway).

So, I tested this, and things got a bit complicated. First, I reproduced the "read side" problem by setting net.core.rmem_max to 256kiB while setting net.core.wmem_max to 16MiB. The "160kiB" stall happened almost every time. Applying this patch appears to fix it completely, getting GiB/s throughput consistently. So, yah. Then I tried reproducing it differently: by setting both net.core.rmem_max and net.core.wmem_max to 16MiB, but setting SO_RCVBUF to 128kiB explicitly in tcp_sock_set_bufsize() (which actually results in a 256kiB buffer, because of the kernel's weird interpretation). With the SO_RCVBUF clamp and without this patch, I don't get the 160kiB stall consistently any more. What I *do* get is nearly every time - but not *every* time - is slow transfers, ~40Mbps vs. ~12Gbps. Sometimes it stalls after several seconds. The stall is slightly different from the 160kiB stall though: the 160kiB stall seems 0 bytes transferred on both sides. With the RCVBUF stall I get a trickle of bytes (620 bytes/s) on the receiver/guest side, with mostly 0 bytes per interval on the sender but occasionally an interval with several hundred KB. That is it seems like there's a buffer somewhere that's very slowly draining into the receiver, then getting topped up in an instant once it gets low enough. When I have both this patch and the RCVBUF clamp, I don't seem to be able to reproduce the trickle-stall anymore, but I still get the slow transfer speeds most, but not every time. Sometimes, but only rarely, I do seem to still get a complete stall (0 bytes on both sides). So it looks like there are two different things going on here. It looks like this patch fixes at least something, but there might be some more things to investigate afterwards. On that basis: Tested-by: David Gibson Reviewed-by: David Gibson

Reported-by: Matej Hrica Link: https://bugs.passt.top/show_bug.cgi?id=74 Analysed-by: David Gibson Signed-off-by: Stefano Brivio --- tcp.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/tcp.c b/tcp.c index 5528e05..4606f17 100644 --- a/tcp.c +++ b/tcp.c @@ -1780,7 +1780,23 @@ static void tcp_clamp_window(const struct ctx *c, struct tcp_tap_conn *conn, wnd <<= conn->ws_from_tap; wnd = MIN(MAX_WINDOW, wnd);

- if (conn->flags & WND_CLAMPED) { + /* TODO: With (at least) Linux kernel versions 6.1 to 6.5, if we end up + * with a zero-sized window on a TCP socket, dropping data (once + * acknowledged by the guest) with recv() and MSG_TRUNC doesn't appear + * to be enough to make the kernel advertise a non-zero window to the + * receiver. Forcing a TCP_WINDOW_CLAMP setting, even with the existing + * value, fixes this. + * + * The STALLED flag on a connection is a sufficient indication that we + * might have a zero-sized window on the socket, because it's set if we + * exhausted the tap-side window, or if everything we receive from a + * socket is already in flight to the guest. + * + * So, if STALLED is set, and we received a window value from the tap, + * force a TCP_WINDOW_CLAMP setsockopt(). This should be investigated + * further and fixed in the kernel instead, if confirmed. + */ + if (!(conn->flags & STALLED) && conn->flags & WND_CLAMPED) { if (prev_scaled == wnd) return;

@@ -2409,12 +2425,12 @@ static int tcp_data_from_tap(struct ctx *c, struct tcp_tap_conn *conn, i = keep - 1; }

- tcp_clamp_window(c, conn, max_ack_seq_wnd); - /* On socket flush failure, pretend there was no ACK, try again later */ if (ack && !tcp_sock_consume(conn, max_ack_seq)) tcp_update_seqack_from_tap(c, conn, max_ack_seq);

+ tcp_clamp_window(c, conn, max_ack_seq_wnd); + if (retr) { trace("TCP: fast re-transmit, ACK: %u, previous sequence: %u", max_ack_seq, conn->seq_to_tap);

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

On Sat, Sep 23, 2023 at 12:06:09AM +0200, Stefano Brivio wrote:

...so that we'll retry sending them, instead of more-or-less silently dropping them. This happens quite frequently if our sending buffer on the UNIX domain socket is heavily constrained (for instance, by the 208 KiB default memory limit).

It might be argued that dropping frames is part of the expected TCP flow: we don't dequeue those from the socket anyway, so we'll eventually retransmit them.

But we don't need the receiver to tell us (by the way of duplicate or missing ACKs) that we couldn't send them: we already know as sendmsg() reports that. This seems to considerably increase throughput stability and throughput itself for TCP connections with default wmem_max values.

Unfortunately, the 16 bits left as padding in the frame descriptors

I assume you're referring to the 'pad' fields in tcp[46]_l2_buf_t, yes? For AVX2 we have substantially more space here. Couldn't we put a conn (or seq) pointer in here at the cost of a few bytes MSS for non-AVX2 and zero cost for AVX2 (which is probably the majority case)?

we use internally aren't enough to uniquely identify for which connection we should update sequence numbers: create a parallel array of pointers to sequence numbers and L4 lengths, of TCP_FRAMES_MEM size, and go through it after calling sendmsg().

Signed-off-by: Stefano Brivio --- tap.c | 10 +++++++--- tap.h | 2 +- tcp.c | 43 ++++++++++++++++++++++++++++++++++++------- 3 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/tap.c b/tap.c index 93db989..b30ff81 100644 --- a/tap.c +++ b/tap.c @@ -413,13 +413,15 @@ static size_t tap_send_frames_passt(const struct ctx *c, * @c: Execution context * @iov: Array of buffers, each containing one frame (with L2 headers) * @n: Number of buffers/frames in @iov + * + * Return: number of frames actually sent */ -void tap_send_frames(struct ctx *c, const struct iovec *iov, size_t n) +size_t tap_send_frames(struct ctx *c, const struct iovec *iov, size_t n) { size_t m;

if (!n) - return; + return 0;

if (c->mode == MODE_PASST) m = tap_send_frames_passt(c, iov, n); @@ -427,9 +429,11 @@ void tap_send_frames(struct ctx *c, const struct iovec *iov, size_t n) m = tap_send_frames_pasta(c, iov, n);

if (m < n) - debug("tap: dropped %lu frames of %lu due to short send", n - m, n); + debug("tap: failed to send %lu frames of %lu", n - m, n);

pcap_multiple(iov, m, c->mode == MODE_PASST ? sizeof(uint32_t) : 0); + + return m; }

/** diff --git a/tap.h b/tap.h index 021fb7c..952fafc 100644 --- a/tap.h +++ b/tap.h @@ -73,7 +73,7 @@ void tap_icmp6_send(const struct ctx *c, const struct in6_addr *src, const struct in6_addr *dst, void *in, size_t len); int tap_send(const struct ctx *c, const void *data, size_t len); -void tap_send_frames(struct ctx *c, const struct iovec *iov, size_t n); +size_t tap_send_frames(struct ctx *c, const struct iovec *iov, size_t n); void tap_update_mac(struct tap_hdr *taph, const unsigned char *eth_d, const unsigned char *eth_s); void tap_listen_handler(struct ctx *c, uint32_t events); diff --git a/tcp.c b/tcp.c index 4606f17..76b7b8d 100644 --- a/tcp.c +++ b/tcp.c @@ -434,6 +434,16 @@ static int tcp_sock_ns [NUM_PORTS][IP_VERSIONS]; */ static union inany_addr low_rtt_dst[LOW_RTT_TABLE_SIZE];

+/** + * tcp_buf_seq_update - Sequences to update with length of frames once sent + * @seq: Pointer to sequence number sent to tap-side, to be updated + * @len: TCP payload length + */ +struct tcp_buf_seq_update { + uint32_t *seq; + uint16_t len; +}; + /* Static buffers */

/** @@ -462,6 +472,8 @@ static struct tcp4_l2_buf_t { #endif tcp4_l2_buf[TCP_FRAMES_MEM];

+static struct tcp_buf_seq_update tcp4_l2_buf_seq_update[TCP_FRAMES_MEM]; + static unsigned int tcp4_l2_buf_used;

/** @@ -490,6 +502,8 @@ struct tcp6_l2_buf_t { #endif tcp6_l2_buf[TCP_FRAMES_MEM];

+static struct tcp_buf_seq_update tcp6_l2_buf_seq_update[TCP_FRAMES_MEM]; + static unsigned int tcp6_l2_buf_used;

/* recvmsg()/sendmsg() data for tap */ @@ -1369,10 +1383,17 @@ static void tcp_l2_flags_buf_flush(struct ctx *c) */ static void tcp_l2_data_buf_flush(struct ctx *c) { - tap_send_frames(c, tcp6_l2_iov, tcp6_l2_buf_used); + unsigned i; + size_t m; + + m = tap_send_frames(c, tcp6_l2_iov, tcp6_l2_buf_used); + for (i = 0; i < m; i++) + *tcp6_l2_buf_seq_update[i].seq += tcp6_l2_buf_seq_update[i].len; tcp6_l2_buf_used = 0;

- tap_send_frames(c, tcp4_l2_iov, tcp4_l2_buf_used); + m = tap_send_frames(c, tcp4_l2_iov, tcp4_l2_buf_used); + for (i = 0; i < m; i++) + *tcp4_l2_buf_seq_update[i].seq += tcp4_l2_buf_seq_update[i].len; tcp4_l2_buf_used = 0; }

@@ -2149,10 +2170,11 @@ static int tcp_sock_consume(struct tcp_tap_conn *conn, uint32_t ack_seq) * @plen: Payload length at L4 * @no_csum: Don't compute IPv4 checksum, use the one from previous buffer * @seq: Sequence number to be sent - * @now: Current timestamp + * @seq_update: Pointer to sequence number to update on successful send */ static void tcp_data_to_tap(struct ctx *c, struct tcp_tap_conn *conn, - ssize_t plen, int no_csum, uint32_t seq) + ssize_t plen, int no_csum, uint32_t seq, + uint32_t *seq_update)

seq_update is always &conn->seq_to_tap, so there's no need for an additional parameter.

{ struct iovec *iov;

@@ -2160,6 +2182,9 @@ static void tcp_data_to_tap(struct ctx *c, struct tcp_tap_conn *conn, struct tcp4_l2_buf_t *b = &tcp4_l2_buf[tcp4_l2_buf_used]; uint16_t *check = no_csum ? &(b - 1)->iph.check : NULL;

+ tcp4_l2_buf_seq_update[tcp4_l2_buf_used].seq = seq_update; + tcp4_l2_buf_seq_update[tcp4_l2_buf_used].len = plen; + iov = tcp4_l2_iov + tcp4_l2_buf_used++; iov->iov_len = tcp_l2_buf_fill_headers(c, conn, b, plen, check, seq); @@ -2168,6 +2193,9 @@ static void tcp_data_to_tap(struct ctx *c, struct tcp_tap_conn *conn, } else if (CONN_V6(conn)) { struct tcp6_l2_buf_t *b = &tcp6_l2_buf[tcp6_l2_buf_used];

+ tcp6_l2_buf_seq_update[tcp6_l2_buf_used].seq = seq_update; + tcp6_l2_buf_seq_update[tcp6_l2_buf_used].len = plen; + iov = tcp6_l2_iov + tcp6_l2_buf_used++; iov->iov_len = tcp_l2_buf_fill_headers(c, conn, b, plen, NULL, seq); @@ -2193,7 +2221,7 @@ static int tcp_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn) int s = conn->sock, i, ret = 0; struct msghdr mh_sock = { 0 }; uint16_t mss = MSS_GET(conn); - uint32_t already_sent; + uint32_t already_sent, seq; struct iovec *iov;

already_sent = conn->seq_to_tap - conn->seq_ack_from_tap; @@ -2282,14 +2310,15 @@ static int tcp_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn)

/* Finally, queue to tap */ plen = mss; + seq = conn->seq_to_tap;

This will only be correct if tcp_l2_data_buf_flush() is *always* called between tcp_data_from_sock() calls for the same socket. That should be true for the normal course of things. However, couldn't it happen that we get a normal socket EPOLLIN event for a particular connection - calling tcp_data_from_sock() - but in the same epoll() round we also get a tap ack for the same connection which causes another call to tcp_data_from_sock() (with the change from patch 2/5). IIRC those would both happen before the deferred handling and therefore the data_buf_flush(). Not sure how to deal with that short of separate 'seq_queued' and 'seq_sent' counters in the connection structure, which is a bit unfortunate.

for (i = 0; i < send_bufs; i++) { int no_csum = i && i != send_bufs - 1 && tcp4_l2_buf_used;

if (i == send_bufs - 1) plen = last_len;

- tcp_data_to_tap(c, conn, plen, no_csum, conn->seq_to_tap); - conn->seq_to_tap += plen; + tcp_data_to_tap(c, conn, plen, no_csum, seq, &conn->seq_to_tap); + seq += plen; }

conn_flag(c, conn, ACK_FROM_TAP_DUE);

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

On Wed, Sep 27, 2023 at 07:06:03PM +0200, Stefano Brivio wrote:

On Mon, 25 Sep 2023 14:47:52 +1000 David Gibson wrote:

...

On Sat, Sep 23, 2023 at 12:06:09AM +0200, Stefano Brivio wrote:

...

...so that we'll retry sending them, instead of more-or-less silently dropping them. This happens quite frequently if our sending buffer on the UNIX domain socket is heavily constrained (for instance, by the 208 KiB default memory limit).

It might be argued that dropping frames is part of the expected TCP flow: we don't dequeue those from the socket anyway, so we'll eventually retransmit them.

But we don't need the receiver to tell us (by the way of duplicate or missing ACKs) that we couldn't send them: we already know as sendmsg() reports that. This seems to considerably increase throughput stability and throughput itself for TCP connections with default wmem_max values.

Unfortunately, the 16 bits left as padding in the frame descriptors

I assume you're referring to the 'pad' fields in tcp[46]_l2_buf_t, yes?

Right, that.

...

For AVX2 we have substantially more space here. Couldn't we put a conn (or seq) pointer in here at the cost of a few bytes MSS for non-AVX2 and zero cost for AVX2 (which is probably the majority case)?

Yes, true. On the other hand, having this parallel array only affects readability I guess, whereas inserting pointers and lengths in tcp[46]_l2_buf_t actually decreases the usable MSS (not just on non-AVX2 x86, but also on other architectures). So I'd rather stick to this.

Yeah, I guess so. Actually.. I did just think of one other option. It avoids both any extra padding and a parallel array, but at the cost of additional work when frames are dropped. We could use that 16-bits of padding to store the TCP payload length. Then when we don't manage to send all our frames, we do another loop through and add up how many stream bytes we actually sent to update the seq pointer.

...

...

we use internally aren't enough to uniquely identify for which connection we should update sequence numbers: create a parallel array of pointers to sequence numbers and L4 lengths, of TCP_FRAMES_MEM size, and go through it after calling sendmsg().

Signed-off-by: Stefano Brivio --- tap.c | 10 +++++++--- tap.h | 2 +- tcp.c | 43 ++++++++++++++++++++++++++++++++++++------- 3 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/tap.c b/tap.c index 93db989..b30ff81 100644 --- a/tap.c +++ b/tap.c @@ -413,13 +413,15 @@ static size_t tap_send_frames_passt(const struct ctx *c, * @c: Execution context * @iov: Array of buffers, each containing one frame (with L2 headers) * @n: Number of buffers/frames in @iov + * + * Return: number of frames actually sent */ -void tap_send_frames(struct ctx *c, const struct iovec *iov, size_t n) +size_t tap_send_frames(struct ctx *c, const struct iovec *iov, size_t n) { size_t m;

if (!n) - return; + return 0;

if (c->mode == MODE_PASST) m = tap_send_frames_passt(c, iov, n); @@ -427,9 +429,11 @@ void tap_send_frames(struct ctx *c, const struct iovec *iov, size_t n) m = tap_send_frames_pasta(c, iov, n);

if (m < n) - debug("tap: dropped %lu frames of %lu due to short send", n - m, n); + debug("tap: failed to send %lu frames of %lu", n - m, n);

pcap_multiple(iov, m, c->mode == MODE_PASST ? sizeof(uint32_t) : 0); + + return m; }

/** diff --git a/tap.h b/tap.h index 021fb7c..952fafc 100644 --- a/tap.h +++ b/tap.h @@ -73,7 +73,7 @@ void tap_icmp6_send(const struct ctx *c, const struct in6_addr *src, const struct in6_addr *dst, void *in, size_t len); int tap_send(const struct ctx *c, const void *data, size_t len); -void tap_send_frames(struct ctx *c, const struct iovec *iov, size_t n); +size_t tap_send_frames(struct ctx *c, const struct iovec *iov, size_t n); void tap_update_mac(struct tap_hdr *taph, const unsigned char *eth_d, const unsigned char *eth_s); void tap_listen_handler(struct ctx *c, uint32_t events); diff --git a/tcp.c b/tcp.c index 4606f17..76b7b8d 100644 --- a/tcp.c +++ b/tcp.c @@ -434,6 +434,16 @@ static int tcp_sock_ns [NUM_PORTS][IP_VERSIONS]; */ static union inany_addr low_rtt_dst[LOW_RTT_TABLE_SIZE];

+/** + * tcp_buf_seq_update - Sequences to update with length of frames once sent + * @seq: Pointer to sequence number sent to tap-side, to be updated + * @len: TCP payload length + */ +struct tcp_buf_seq_update { + uint32_t *seq; + uint16_t len; +}; + /* Static buffers */

/** @@ -462,6 +472,8 @@ static struct tcp4_l2_buf_t { #endif tcp4_l2_buf[TCP_FRAMES_MEM];

+static struct tcp_buf_seq_update tcp4_l2_buf_seq_update[TCP_FRAMES_MEM]; + static unsigned int tcp4_l2_buf_used;

/** @@ -490,6 +502,8 @@ struct tcp6_l2_buf_t { #endif tcp6_l2_buf[TCP_FRAMES_MEM];

+static struct tcp_buf_seq_update tcp6_l2_buf_seq_update[TCP_FRAMES_MEM]; + static unsigned int tcp6_l2_buf_used;

/* recvmsg()/sendmsg() data for tap */ @@ -1369,10 +1383,17 @@ static void tcp_l2_flags_buf_flush(struct ctx *c) */ static void tcp_l2_data_buf_flush(struct ctx *c) { - tap_send_frames(c, tcp6_l2_iov, tcp6_l2_buf_used); + unsigned i; + size_t m; + + m = tap_send_frames(c, tcp6_l2_iov, tcp6_l2_buf_used); + for (i = 0; i < m; i++) + *tcp6_l2_buf_seq_update[i].seq += tcp6_l2_buf_seq_update[i].len; tcp6_l2_buf_used = 0;

- tap_send_frames(c, tcp4_l2_iov, tcp4_l2_buf_used); + m = tap_send_frames(c, tcp4_l2_iov, tcp4_l2_buf_used); + for (i = 0; i < m; i++) + *tcp4_l2_buf_seq_update[i].seq += tcp4_l2_buf_seq_update[i].len; tcp4_l2_buf_used = 0; }

@@ -2149,10 +2170,11 @@ static int tcp_sock_consume(struct tcp_tap_conn *conn, uint32_t ack_seq) * @plen: Payload length at L4 * @no_csum: Don't compute IPv4 checksum, use the one from previous buffer * @seq: Sequence number to be sent - * @now: Current timestamp + * @seq_update: Pointer to sequence number to update on successful send */ static void tcp_data_to_tap(struct ctx *c, struct tcp_tap_conn *conn, - ssize_t plen, int no_csum, uint32_t seq) + ssize_t plen, int no_csum, uint32_t seq, + uint32_t *seq_update)

seq_update is always &conn->seq_to_tap, so there's no need for an additional parameter.

Oh, right, I'll drop that.

...

...

{ struct iovec *iov;

@@ -2160,6 +2182,9 @@ static void tcp_data_to_tap(struct ctx *c, struct tcp_tap_conn *conn, struct tcp4_l2_buf_t *b = &tcp4_l2_buf[tcp4_l2_buf_used]; uint16_t *check = no_csum ? &(b - 1)->iph.check : NULL;

+ tcp4_l2_buf_seq_update[tcp4_l2_buf_used].seq = seq_update; + tcp4_l2_buf_seq_update[tcp4_l2_buf_used].len = plen; + iov = tcp4_l2_iov + tcp4_l2_buf_used++; iov->iov_len = tcp_l2_buf_fill_headers(c, conn, b, plen, check, seq); @@ -2168,6 +2193,9 @@ static void tcp_data_to_tap(struct ctx *c, struct tcp_tap_conn *conn, } else if (CONN_V6(conn)) { struct tcp6_l2_buf_t *b = &tcp6_l2_buf[tcp6_l2_buf_used];

+ tcp6_l2_buf_seq_update[tcp6_l2_buf_used].seq = seq_update; + tcp6_l2_buf_seq_update[tcp6_l2_buf_used].len = plen; + iov = tcp6_l2_iov + tcp6_l2_buf_used++; iov->iov_len = tcp_l2_buf_fill_headers(c, conn, b, plen, NULL, seq); @@ -2193,7 +2221,7 @@ static int tcp_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn) int s = conn->sock, i, ret = 0; struct msghdr mh_sock = { 0 }; uint16_t mss = MSS_GET(conn); - uint32_t already_sent; + uint32_t already_sent, seq; struct iovec *iov;

already_sent = conn->seq_to_tap - conn->seq_ack_from_tap; @@ -2282,14 +2310,15 @@ static int tcp_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn)

/* Finally, queue to tap */ plen = mss; + seq = conn->seq_to_tap;

This will only be correct if tcp_l2_data_buf_flush() is *always* called between tcp_data_from_sock() calls for the same socket. That should be true for the normal course of things. However, couldn't it happen that we get a normal socket EPOLLIN event for a particular connection - calling tcp_data_from_sock() - but in the same epoll() round we also get a tap ack for the same connection which causes another call to tcp_data_from_sock() (with the change from patch 2/5). IIRC those would both happen before the deferred handling and therefore the data_buf_flush().

Ah, yes, I actually wrote this before 2/5 and concluded it was okay :/ but with that change, it's not. Unless we drop that change from 2/5.

Even if we drop the change, it's a worryingly subtle constraint.

...

Not sure how to deal with that short of separate 'seq_queued' and 'seq_sent' counters in the connection structure, which is a bit unfortunate.

I wonder how bad it is if we call tcp_l2_data_buf_flush() unconditionally before calling tcp_data_from_sock() from tcp_tap_handler(). But again, maybe this is not needed at all, we should check that epoll detail from 2/5 first...

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

On Fri, Sep 29, 2023 at 05:19:50PM +0200, Stefano Brivio wrote:

On Thu, 28 Sep 2023 11:58:45 +1000 David Gibson wrote:

...

On Wed, Sep 27, 2023 at 07:06:03PM +0200, Stefano Brivio wrote:

...

On Mon, 25 Sep 2023 14:47:52 +1000 David Gibson wrote:

...

On Sat, Sep 23, 2023 at 12:06:09AM +0200, Stefano Brivio wrote:

...

...so that we'll retry sending them, instead of more-or-less silently dropping them. This happens quite frequently if our sending buffer on the UNIX domain socket is heavily constrained (for instance, by the 208 KiB default memory limit).

It might be argued that dropping frames is part of the expected TCP flow: we don't dequeue those from the socket anyway, so we'll eventually retransmit them.

But we don't need the receiver to tell us (by the way of duplicate or missing ACKs) that we couldn't send them: we already know as sendmsg() reports that. This seems to considerably increase throughput stability and throughput itself for TCP connections with default wmem_max values.

Unfortunately, the 16 bits left as padding in the frame descriptors

I assume you're referring to the 'pad' fields in tcp[46]_l2_buf_t, yes?

Right, that.

...

For AVX2 we have substantially more space here. Couldn't we put a conn (or seq) pointer in here at the cost of a few bytes MSS for non-AVX2 and zero cost for AVX2 (which is probably the majority case)?

Yes, true. On the other hand, having this parallel array only affects readability I guess, whereas inserting pointers and lengths in tcp[46]_l2_buf_t actually decreases the usable MSS (not just on non-AVX2 x86, but also on other architectures). So I'd rather stick to this.

Yeah, I guess so.

Actually.. I did just think of one other option. It avoids both any extra padding and a parallel array, but at the cost of additional work when frames are dropped. We could use that 16-bits of padding to store the TCP payload length. Then when we don't manage to send all our frames, we do another loop through and add up how many stream bytes we actually sent to update the seq pointer.

Hmm, yes. It's slightly more memory efficient, but the complexity seems a bit overkill to me.

More importantly, I forgot the fact that by the time we're sending the frames, we don't know what connection they're associated with any more. [snip]

...

...

...

...

@@ -2282,14 +2310,15 @@ static int tcp_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn)

/* Finally, queue to tap */ plen = mss; + seq = conn->seq_to_tap;

This will only be correct if tcp_l2_data_buf_flush() is *always* called between tcp_data_from_sock() calls for the same socket. That should be true for the normal course of things. However, couldn't it happen that we get a normal socket EPOLLIN event for a particular connection - calling tcp_data_from_sock() - but in the same epoll() round we also get a tap ack for the same connection which causes another call to tcp_data_from_sock() (with the change from patch 2/5). IIRC those would both happen before the deferred handling and therefore the data_buf_flush().

Ah, yes, I actually wrote this before 2/5 and concluded it was okay :/ but with that change, it's not. Unless we drop that change from 2/5.

Even if we drop the change, it's a worryingly subtle constraint.

Another option to avoid this...

...

...

...

Not sure how to deal with that short of separate 'seq_queued' and 'seq_sent' counters in the connection structure, which is a bit unfortunate.

I wonder how bad it is if we call tcp_l2_data_buf_flush() unconditionally before calling tcp_data_from_sock() from tcp_tap_handler(). But again, maybe this is not needed at all, we should check that epoll detail from 2/5 first...

other than this one, would be to use that external table to update sequence numbers *in the frames* as we send stuff out.

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

On Thu, Oct 05, 2023 at 08:19:00AM +0200, Stefano Brivio wrote:

On Tue, 3 Oct 2023 14:22:59 +1100 David Gibson wrote:

...

On Fri, Sep 29, 2023 at 05:19:50PM +0200, Stefano Brivio wrote:

...

On Thu, 28 Sep 2023 11:58:45 +1000 David Gibson wrote:

...

On Wed, Sep 27, 2023 at 07:06:03PM +0200, Stefano Brivio wrote:

...

On Mon, 25 Sep 2023 14:47:52 +1000 David Gibson wrote:

...

On Sat, Sep 23, 2023 at 12:06:09AM +0200, Stefano Brivio wrote: > ...so that we'll retry sending them, instead of more-or-less silently > dropping them. This happens quite frequently if our sending buffer on > the UNIX domain socket is heavily constrained (for instance, by the > 208 KiB default memory limit). > > It might be argued that dropping frames is part of the expected TCP > flow: we don't dequeue those from the socket anyway, so we'll > eventually retransmit them. > > But we don't need the receiver to tell us (by the way of duplicate or > missing ACKs) that we couldn't send them: we already know as > sendmsg() reports that. This seems to considerably increase > throughput stability and throughput itself for TCP connections with > default wmem_max values. > > Unfortunately, the 16 bits left as padding in the frame descriptors

I assume you're referring to the 'pad' fields in tcp[46]_l2_buf_t, yes?

Right, that.

...

For AVX2 we have substantially more space here. Couldn't we put a conn (or seq) pointer in here at the cost of a few bytes MSS for non-AVX2 and zero cost for AVX2 (which is probably the majority case)?

Yes, true. On the other hand, having this parallel array only affects readability I guess, whereas inserting pointers and lengths in tcp[46]_l2_buf_t actually decreases the usable MSS (not just on non-AVX2 x86, but also on other architectures). So I'd rather stick to this.

Yeah, I guess so.

Actually.. I did just think of one other option. It avoids both any extra padding and a parallel array, but at the cost of additional work when frames are dropped. We could use that 16-bits of padding to store the TCP payload length. Then when we don't manage to send all our frames, we do another loop through and add up how many stream bytes we actually sent to update the seq pointer.

Hmm, yes. It's slightly more memory efficient, but the complexity seems a bit overkill to me.

More importantly, I forgot the fact that by the time we're sending the frames, we don't know what connection they're associated with any more.

Oh, I thought you wanted to rebuild the information about the connection by looking into the hash table or something like that.

No, I guess we could do that, but I think it would be substantially messier than the parallel array approach.

...

[snip]

...

...

...

...

> @@ -2282,14 +2310,15 @@ static int tcp_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn) > > /* Finally, queue to tap */ > plen = mss; > + seq = conn->seq_to_tap;

This will only be correct if tcp_l2_data_buf_flush() is *always* called between tcp_data_from_sock() calls for the same socket. That should be true for the normal course of things. However, couldn't it happen that we get a normal socket EPOLLIN event for a particular connection - calling tcp_data_from_sock() - but in the same epoll() round we also get a tap ack for the same connection which causes another call to tcp_data_from_sock() (with the change from patch 2/5). IIRC those would both happen before the deferred handling and therefore the data_buf_flush().

Ah, yes, I actually wrote this before 2/5 and concluded it was okay :/ but with that change, it's not. Unless we drop that change from 2/5.

Even if we drop the change, it's a worryingly subtle constraint.

Another option to avoid this...

...

...

...

Not sure how to deal with that short of separate 'seq_queued' and 'seq_sent' counters in the connection structure, which is a bit unfortunate.

I wonder how bad it is if we call tcp_l2_data_buf_flush() unconditionally before calling tcp_data_from_sock() from tcp_tap_handler(). But again, maybe this is not needed at all, we should check that epoll detail from 2/5 first...

other than this one, would be to use that external table to update sequence numbers *in the frames* as we send stuff out.

Not really sure what you're proposing there.

That tcp_l2_buf_fill_headers() calculates the sequence from conn->seq_to_tap plus a cumulative count from that table, instead of passing it from the caller.

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

On Sat, Sep 23, 2023 at 12:06:10AM +0200, Stefano Brivio wrote:

Signed-off-by: Stefano Brivio --- passt.1 | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)

diff --git a/passt.1 b/passt.1 index 1ad4276..bcbe6fd 100644 --- a/passt.1 +++ b/passt.1 @@ -926,6 +926,39 @@ If the sending window cannot be queried, it will always be announced as the current sending buffer size to guest or target namespace. This might affect throughput of TCP connections.

+.SS Tuning for high throughput + +On Linux, by default, the maximum memory that can be set for receive and send +socket buffers is 208 KiB. Those limits are set by the +\fI/proc/sys/net/core/rmem_max\fR and \fI/proc/sys/net/core/wmem_max\fR files, +see \fBsocket\fR(7). + +As of Linux 6.5, while the TCP implementation can dynamically shrink buffers +depending on utilisation even above those limits, such a small limit will

"shrink buffers" and "even above those limits" don't seem to quite work together.

+reflect on the advertised TCP window at the beginning of a connection, and the

Hmmm.... while [rw]mem_max might limit that initial window size, I wouldn't expect increasing the limits alone to increase that initial window size: wouldn't that instead be affected by the TCP default buffer size i.e. the middle value in net.ipv4.tcp_rmem?

+buffer size of the UNIX domain socket buffer used by \fBpasst\fR cannot exceed +these limits anyway. + +Further, as of Linux 6.5, using socket options \fBSO_RCVBUF\fR and +\fBSO_SNDBUF\fR will prevent TCP buffers to expand above the \fIrmem_max\fR and +\fIwmem_max\fR limits because the automatic adjustment provided by the TCP +implementation is then disabled. + +As a consequence, \fBpasst\fR and \fBpasta\fR probe these limits at start-up and +will not set TCP socket buffer sizes if they are lower than 2 MiB, because this +would affect the maximum size of TCP buffers for the whole duration of a +connection. + +Note that 208 KiB is, accounting for kernel overhead, enough to fit less than +three TCP packets at the default MSS. In applications where high throughput is +expected, it is therefore advisable to increase those limits to at least 2 MiB, +or even 16 MiB: + +.nf + sysctl -w net.core.rmem_max=$((16 << 20) + sysctl -w net.core.wmem_max=$((16 << 20) +.fi

As noted in a previous mail, empirically, this doesn't necessarily seem to work better for me. I'm wondering if we'd be better off never touching RCFBUF and SNDBUF for TCP sockets, and letting the kernel do its adaptive thing. We probably still want to expand the buffers as much as we can for the Unix socket, though. And we likely still want expanded limits for the tests so that iperf3 can use large buffers

+ .SH LIMITATIONS

Currently, IGMP/MLD proxying (RFC 4605) and support for SCTP (RFC 4960) are not

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

On Wed, Sep 27, 2023 at 07:06:16PM +0200, Stefano Brivio wrote:

On Mon, 25 Sep 2023 14:57:40 +1000 David Gibson wrote:

...

On Sat, Sep 23, 2023 at 12:06:10AM +0200, Stefano Brivio wrote:

...

Signed-off-by: Stefano Brivio --- passt.1 | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)

diff --git a/passt.1 b/passt.1 index 1ad4276..bcbe6fd 100644 --- a/passt.1 +++ b/passt.1 @@ -926,6 +926,39 @@ If the sending window cannot be queried, it will always be announced as the current sending buffer size to guest or target namespace. This might affect throughput of TCP connections.

+.SS Tuning for high throughput + +On Linux, by default, the maximum memory that can be set for receive and send +socket buffers is 208 KiB. Those limits are set by the +\fI/proc/sys/net/core/rmem_max\fR and \fI/proc/sys/net/core/wmem_max\fR files, +see \fBsocket\fR(7). + +As of Linux 6.5, while the TCP implementation can dynamically shrink buffers +depending on utilisation even above those limits, such a small limit will

"shrink buffers" and "even above those limits" don't seem to quite work together.

Oops. I guess I should simply s/shrink/grow/ here.

Or "resize" would work too.

...

...

+reflect on the advertised TCP window at the beginning of a connection, and the

Hmmm.... while [rw]mem_max might limit that initial window size, I wouldn't expect increasing the limits alone to increase that initial window size: wouldn't that instead be affected by the TCP default buffer size i.e. the middle value in net.ipv4.tcp_rmem?

If we don't use SO_RCVBUF, yes... but we currently do, and with that, we can get a much larger initial window (as we do now).

Good point.

On the other hand, maybe, as mentioned in my follow-up about 3/5, we should drop SO_RCVBUF for TCP sockets.

Ok.

...

...

+buffer size of the UNIX domain socket buffer used by \fBpasst\fR cannot exceed +these limits anyway. + +Further, as of Linux 6.5, using socket options \fBSO_RCVBUF\fR and +\fBSO_SNDBUF\fR will prevent TCP buffers to expand above the \fIrmem_max\fR and +\fIwmem_max\fR limits because the automatic adjustment provided by the TCP +implementation is then disabled. + +As a consequence, \fBpasst\fR and \fBpasta\fR probe these limits at start-up and +will not set TCP socket buffer sizes if they are lower than 2 MiB, because this +would affect the maximum size of TCP buffers for the whole duration of a +connection. + +Note that 208 KiB is, accounting for kernel overhead, enough to fit less than +three TCP packets at the default MSS. In applications where high throughput is +expected, it is therefore advisable to increase those limits to at least 2 MiB, +or even 16 MiB: + +.nf + sysctl -w net.core.rmem_max=$((16 << 20) + sysctl -w net.core.wmem_max=$((16 << 20) +.fi

As noted in a previous mail, empirically, this doesn't necessarily seem to work better for me. I'm wondering if we'd be better off never touching RCFBUF and SNDBUF for TCP sockets, and letting the kernel do its adaptive thing. We probably still want to expand the buffers as much as we can for the Unix socket, though. And we likely still want expanded limits for the tests so that iperf3 can use large buffers

Right. Let's keep this patch for a later time then, and meanwhile check if we should drop SO_RCVBUF, SO_SNDBUF, or both, for TCP sockets.

Makes sense to me. -- 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