public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
* [PATCH v9 0/2] Add support for SO_PEEK_OFF
@ 2024-07-12 19:04 Jon Maloy
  2024-07-12 19:04 ` [PATCH v9 1/2] tcp: leverage support of SO_PEEK_OFF socket option when available Jon Maloy
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Jon Maloy @ 2024-07-12 19:04 UTC (permalink / raw)
  To: passt-dev, sbrivio, lvivier, dgibson, jmaloy

We add support for the SO_PEEK_OFF option, plus work around a kernel
bug discovered along the way.

Jon Maloy (2):
  tcp: leverage support of SO_PEEK_OFF socket option when available
  tcp: handle shrunk window advertisemenst from guest

 tcp.c     | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 tcp.h     |  3 +++
 tcp_buf.c | 25 ++++++++++++++++------
 3 files changed, 81 insertions(+), 9 deletions(-)

-- 
2.45.2


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

* [PATCH v9 1/2] tcp: leverage support of SO_PEEK_OFF socket option when available
  2024-07-12 19:04 [PATCH v9 0/2] Add support for SO_PEEK_OFF Jon Maloy
@ 2024-07-12 19:04 ` Jon Maloy
  2024-07-12 19:04 ` [PATCH v9 2/2] tcp: handle shrunk window advertisemenst from guest Jon Maloy
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Jon Maloy @ 2024-07-12 19:04 UTC (permalink / raw)
  To: passt-dev, sbrivio, lvivier, dgibson, jmaloy

From linux-6.9.0 the kernel will contain
commit 05ea491641d3 ("tcp: add support for SO_PEEK_OFF socket option").

This new feature makes is possible to call recv_msg(MSG_PEEK) and make
it start reading data from a given offset set by the SO_PEEK_OFF socket
option. This way, we can avoid repeated reading of already read bytes of
a received message, hence saving read cycles when forwarding TCP
messages in the host->name space direction.

In this commit, we add functionality to leverage this feature when
available, while we fall back to the previous behavior when not.

Measurements with iperf3 shows that throughput increases with 15-20
percent in the host->namespace direction when this feature is used.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Jon Maloy <jmaloy@redhat.com>

 ---
v2: - Some smaller changes as suggested by David Gibson and Stefano Brivio.
    - Moved initial set_peek_offset(0) to only the locations where the socket is set
      to ESTABLISHED.
    - Removed the per-packet synchronization between sk_peek_off and
      already_sent. Instead only doing it in retransmit situations.
    - The problem I found when trouble shooting the occasionally occurring
      out of synch values between 'already_sent' and 'sk_peek_offset' may
      have deeper implications that we may need to be investigate.

v3: - Rebased to most recent version of tcp.c, plus the previous
      patch in this series.
    - Some changes based on feedback from PASST team

v4: - Some small changes based on feedback from Stefan/David.

v5: - Re-added accidentally dropped set_peek_offset() line.
      Thank you, David.

v6: - Adapted to new file structure. No code changes.

v7: - Added a tcp_rst() when we tcp_set_peek_offset() fails.
      Suggested by David and Stefano.
---
 tcp.c     | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 tcp.h     |  3 +++
 tcp_buf.c | 25 +++++++++++++++++++------
 3 files changed, 73 insertions(+), 9 deletions(-)

diff --git a/tcp.c b/tcp.c
index 698e7ec..e7e0a98 100644
--- a/tcp.c
+++ b/tcp.c
@@ -373,6 +373,9 @@ static union inany_addr low_rtt_dst[LOW_RTT_TABLE_SIZE];
 
 char		tcp_buf_discard		[MAX_WINDOW];
 
+/* Does the kernel support TCP_PEEK_OFF? */
+bool peek_offset_cap;
+
 /* sendmsg() to socket */
 static struct iovec	tcp_iov			[UIO_MAXIOV];
 
@@ -388,6 +391,25 @@ static_assert(ARRAY_SIZE(tc_hash) >= FLOW_MAX,
 int init_sock_pool4		[TCP_SOCK_POOL_SIZE];
 int init_sock_pool6		[TCP_SOCK_POOL_SIZE];
 
+/**
+ * tcp_set_peek_offset() - Set SO_PEEK_OFF offset on a socket if supported
+ * @s:          Socket to update
+ * @offset:     Offset in bytes
+ *
+ * Return:      -1 when it fails, 0 otherwise.
+ */
+int tcp_set_peek_offset(int s, int offset)
+{
+	if (!peek_offset_cap)
+		return 0;
+
+	if (setsockopt(s, SOL_SOCKET, SO_PEEK_OFF, &offset, sizeof(offset))) {
+		err("Failed to set SO_PEEK_OFF to %i in socket %i", offset, s);
+		return -1;
+	}
+	return 0;
+}
+
 /**
  * tcp_conn_epoll_events() - epoll events mask for given connection state
  * @events:	Current connection events
@@ -1948,6 +1970,10 @@ static int tcp_data_from_tap(struct ctx *c, struct tcp_tap_conn *conn,
 			   "fast re-transmit, ACK: %u, previous sequence: %u",
 			   max_ack_seq, conn->seq_to_tap);
 		conn->seq_to_tap = max_ack_seq;
+		if (tcp_set_peek_offset(conn->sock, 0)) {
+			tcp_rst(c, conn);
+			return -1;
+		}
 		tcp_data_from_sock(c, conn);
 	}
 
@@ -2040,6 +2066,10 @@ static void tcp_conn_from_sock_finish(struct ctx *c, struct tcp_tap_conn *conn,
 	conn->seq_ack_to_tap = conn->seq_from_tap;
 
 	conn_event(c, conn, ESTABLISHED);
+	if (tcp_set_peek_offset(conn->sock, 0)) {
+		tcp_rst(c, conn);
+		return;
+	}
 
 	/* The client might have sent data already, which we didn't
 	 * dequeue waiting for SYN,ACK from tap -- check now.
@@ -2120,6 +2150,8 @@ int tcp_tap_handler(struct ctx *c, uint8_t pif, sa_family_t af,
 			goto reset;
 
 		conn_event(c, conn, ESTABLISHED);
+		if (tcp_set_peek_offset(conn->sock, 0))
+			goto reset;
 
 		if (th->fin) {
 			conn->seq_from_tap++;
@@ -2368,8 +2400,12 @@ void tcp_timer_handler(struct ctx *c, union epoll_ref ref)
 			flow_dbg(conn, "ACK timeout, retry");
 			conn->retrans++;
 			conn->seq_to_tap = conn->seq_ack_from_tap;
-			tcp_data_from_sock(c, conn);
-			tcp_timer_ctl(c, conn);
+			if (tcp_set_peek_offset(conn->sock, 0)) {
+				tcp_rst(c, conn);
+			} else {
+				tcp_data_from_sock(c, conn);
+				tcp_timer_ctl(c, conn);
+			}
 		}
 	} else {
 		struct itimerspec new = { { 0 }, { ACT_TIMEOUT, 0 } };
@@ -2660,7 +2696,8 @@ static void tcp_sock_refill_init(const struct ctx *c)
  */
 int tcp_init(struct ctx *c)
 {
-	unsigned b;
+	unsigned int b, optv = 0;
+	int s;
 
 	for (b = 0; b < TCP_HASH_TABLE_SIZE; b++)
 		tc_hash[b] = FLOW_SIDX_NONE;
@@ -2684,6 +2721,17 @@ int tcp_init(struct ctx *c)
 		NS_CALL(tcp_ns_socks_init, c);
 	}
 
+	/* Probe for SO_PEEK_OFF support */
+	s = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, IPPROTO_TCP);
+	if (s < 0) {
+		warn_perror("Temporary TCP socket creation failed");
+	} else {
+		if (!setsockopt(s, SOL_SOCKET, SO_PEEK_OFF, &optv, sizeof(int)))
+			peek_offset_cap = true;
+		close(s);
+	}
+	info("SO_PEEK_OFF%ssupported", peek_offset_cap ? " " : " not ");
+
 	return 0;
 }
 
diff --git a/tcp.h b/tcp.h
index a9b8bf8..e9ff019 100644
--- a/tcp.h
+++ b/tcp.h
@@ -24,6 +24,9 @@ void tcp_timer(struct ctx *c, const struct timespec *now);
 void tcp_defer_handler(struct ctx *c);
 
 void tcp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s);
+int tcp_set_peek_offset(int s, int offset);
+
+extern bool peek_offset_cap;
 
 /**
  * union tcp_epoll_ref - epoll reference portion for TCP connections
diff --git a/tcp_buf.c b/tcp_buf.c
index ff9f797..11dce02 100644
--- a/tcp_buf.c
+++ b/tcp_buf.c
@@ -408,6 +408,7 @@ int tcp_buf_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn)
 	uint32_t already_sent, seq;
 	struct iovec *iov;
 
+	/* How much have we read/sent since last received ack ? */
 	already_sent = conn->seq_to_tap - conn->seq_ack_from_tap;
 
 	if (SEQ_LT(already_sent, 0)) {
@@ -416,6 +417,10 @@ int tcp_buf_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn)
 			   conn->seq_ack_from_tap, conn->seq_to_tap);
 		conn->seq_to_tap = conn->seq_ack_from_tap;
 		already_sent = 0;
+		if (tcp_set_peek_offset(s, 0)) {
+			tcp_rst(c, conn);
+			return -1;
+		}
 	}
 
 	if (!wnd_scaled || already_sent >= wnd_scaled) {
@@ -433,11 +438,16 @@ int tcp_buf_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn)
 		iov_rem = (wnd_scaled - already_sent) % mss;
 	}
 
-	mh_sock.msg_iov = iov_sock;
-	mh_sock.msg_iovlen = fill_bufs + 1;
-
-	iov_sock[0].iov_base = tcp_buf_discard;
-	iov_sock[0].iov_len = already_sent;
+	/* Prepare iov according to kernel capability */
+	if (!peek_offset_cap) {
+		mh_sock.msg_iov = iov_sock;
+		iov_sock[0].iov_base = tcp_buf_discard;
+		iov_sock[0].iov_len = already_sent;
+		mh_sock.msg_iovlen = fill_bufs + 1;
+	} else {
+		mh_sock.msg_iov = &iov_sock[1];
+		mh_sock.msg_iovlen = fill_bufs;
+	}
 
 	if (( v4 && tcp4_payload_used + fill_bufs > TCP_FRAMES_MEM) ||
 	    (!v4 && tcp6_payload_used + fill_bufs > TCP_FRAMES_MEM)) {
@@ -478,7 +488,10 @@ int tcp_buf_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn)
 		return 0;
 	}
 
-	sendlen = len - already_sent;
+	sendlen = len;
+	if (!peek_offset_cap)
+		sendlen -= already_sent;
+
 	if (sendlen <= 0) {
 		conn_flag(c, conn, STALLED);
 		return 0;
-- 
@@ -408,6 +408,7 @@ int tcp_buf_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn)
 	uint32_t already_sent, seq;
 	struct iovec *iov;
 
+	/* How much have we read/sent since last received ack ? */
 	already_sent = conn->seq_to_tap - conn->seq_ack_from_tap;
 
 	if (SEQ_LT(already_sent, 0)) {
@@ -416,6 +417,10 @@ int tcp_buf_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn)
 			   conn->seq_ack_from_tap, conn->seq_to_tap);
 		conn->seq_to_tap = conn->seq_ack_from_tap;
 		already_sent = 0;
+		if (tcp_set_peek_offset(s, 0)) {
+			tcp_rst(c, conn);
+			return -1;
+		}
 	}
 
 	if (!wnd_scaled || already_sent >= wnd_scaled) {
@@ -433,11 +438,16 @@ int tcp_buf_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn)
 		iov_rem = (wnd_scaled - already_sent) % mss;
 	}
 
-	mh_sock.msg_iov = iov_sock;
-	mh_sock.msg_iovlen = fill_bufs + 1;
-
-	iov_sock[0].iov_base = tcp_buf_discard;
-	iov_sock[0].iov_len = already_sent;
+	/* Prepare iov according to kernel capability */
+	if (!peek_offset_cap) {
+		mh_sock.msg_iov = iov_sock;
+		iov_sock[0].iov_base = tcp_buf_discard;
+		iov_sock[0].iov_len = already_sent;
+		mh_sock.msg_iovlen = fill_bufs + 1;
+	} else {
+		mh_sock.msg_iov = &iov_sock[1];
+		mh_sock.msg_iovlen = fill_bufs;
+	}
 
 	if (( v4 && tcp4_payload_used + fill_bufs > TCP_FRAMES_MEM) ||
 	    (!v4 && tcp6_payload_used + fill_bufs > TCP_FRAMES_MEM)) {
@@ -478,7 +488,10 @@ int tcp_buf_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn)
 		return 0;
 	}
 
-	sendlen = len - already_sent;
+	sendlen = len;
+	if (!peek_offset_cap)
+		sendlen -= already_sent;
+
 	if (sendlen <= 0) {
 		conn_flag(c, conn, STALLED);
 		return 0;
-- 
2.45.2


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

* [PATCH v9 2/2] tcp: handle shrunk window advertisemenst from guest
  2024-07-12 19:04 [PATCH v9 0/2] Add support for SO_PEEK_OFF Jon Maloy
  2024-07-12 19:04 ` [PATCH v9 1/2] tcp: leverage support of SO_PEEK_OFF socket option when available Jon Maloy
@ 2024-07-12 19:04 ` Jon Maloy
  2024-07-15  0:34   ` David Gibson
  2024-07-12 19:43 ` [PATCH v9 0/2] Add support for SO_PEEK_OFF Stefano Brivio
  2024-07-15 16:58 ` Stefano Brivio
  3 siblings, 1 reply; 8+ messages in thread
From: Jon Maloy @ 2024-07-12 19:04 UTC (permalink / raw)
  To: passt-dev, sbrivio, lvivier, dgibson, jmaloy

A bug in kernel TCP may lead to a deadlock where a zero window is sent
from the guest peer, while it is unable to send out window updates even
after socket reads have freed up enough buffer space to permit a larger
window. In this situation, new window advertisements from the peer can
only be triggered by data packets arriving from this side.

However, currently such packets are never sent, because the zero-window
condition prevents this side from sending out any packets whatsoever
to the peer.

We notice that the above bug is triggered *only* after the peer has
dropped one or more arriving packets because of severe memory squeeze,
and that we hence always enter a retransmission situation when this
occurs. This also means that the implementation goes against the
RFC-9293 recommendation that a previously advertised window never
should shrink.

RFC-9293 seems to permit that we can continue sending up to the right
edge of the last advertised non-zero window in such situations, so that
is what we do to resolve this situation.

It turns out that this solution is extremely simple to implememt in the
code: We just omit to save the advertised zero-window when we see that
it has shrunk, i.e., if the acknowledged sequence number in the
advertisement message is lower than that of the last data byte sent
from our side.

When that is the case, the following happens:
- The 'retr' flag in tcp_data_from_tap() will be 'false', so no
  retransmission will occur at this occasion.
- The data stream will soon reach the right edge of the previously
  advertised window. In fact, in all observed cases we have seen that
  it is already there when the zero-advertisement arrives.
- At that moment, the flags STALLED and ACK_FROM_TAP_DUE will be set,
  unless they already have been, meaning that only the next timer
  expiration will open for data retransmission or transmission.
- When that happens, the memory squeeze at the guest will normally have
  abated, and the data flow can resume.

It should be noted that although this solves the problem we have at
hand, it is a work-around, and not a genuine solution to the described
kernel bug.

Suggested-by: Stefano Brivio <sbrivio@redhat.com>
Signed-off-by: Jon Maloy <jmaloy@redhat.com>

--
v2: Updated comment with reference to kernel commit where bug was
    introduced.
---
 tcp.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tcp.c b/tcp.c
index e7e0a98..bda6758 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1426,6 +1426,14 @@ static void tcp_get_tap_ws(struct tcp_tap_conn *conn,
 static void tcp_tap_window_update(struct tcp_tap_conn *conn, unsigned wnd)
 {
 	wnd = MIN(MAX_WINDOW, wnd << conn->ws_from_tap);
+
+	/* Work-around for bug introduced in peer kernel code, commit
+	 * d4317abf7160 ("net: tcp: send zero-window ACK when no memory").
+	 * We don't update if window shrank to zero.
+	 */
+	if (!wnd && SEQ_LT(conn->seq_ack_from_tap, conn->seq_to_tap))
+		return;
+
 	conn->wnd_from_tap = MIN(wnd >> conn->ws_from_tap, USHRT_MAX);
 
 	/* FIXME: reflect the tap-side receiver's window back to the sock-side
-- 
@@ -1426,6 +1426,14 @@ static void tcp_get_tap_ws(struct tcp_tap_conn *conn,
 static void tcp_tap_window_update(struct tcp_tap_conn *conn, unsigned wnd)
 {
 	wnd = MIN(MAX_WINDOW, wnd << conn->ws_from_tap);
+
+	/* Work-around for bug introduced in peer kernel code, commit
+	 * d4317abf7160 ("net: tcp: send zero-window ACK when no memory").
+	 * We don't update if window shrank to zero.
+	 */
+	if (!wnd && SEQ_LT(conn->seq_ack_from_tap, conn->seq_to_tap))
+		return;
+
 	conn->wnd_from_tap = MIN(wnd >> conn->ws_from_tap, USHRT_MAX);
 
 	/* FIXME: reflect the tap-side receiver's window back to the sock-side
-- 
2.45.2


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

* Re: [PATCH v9 0/2] Add support for SO_PEEK_OFF
  2024-07-12 19:04 [PATCH v9 0/2] Add support for SO_PEEK_OFF Jon Maloy
  2024-07-12 19:04 ` [PATCH v9 1/2] tcp: leverage support of SO_PEEK_OFF socket option when available Jon Maloy
  2024-07-12 19:04 ` [PATCH v9 2/2] tcp: handle shrunk window advertisemenst from guest Jon Maloy
@ 2024-07-12 19:43 ` Stefano Brivio
  2024-07-15 16:58 ` Stefano Brivio
  3 siblings, 0 replies; 8+ messages in thread
From: Stefano Brivio @ 2024-07-12 19:43 UTC (permalink / raw)
  To: Jon Maloy; +Cc: passt-dev, lvivier, dgibson

On Fri, 12 Jul 2024 15:04:48 -0400
Jon Maloy <jmaloy@redhat.com> wrote:

> We add support for the SO_PEEK_OFF option, plus work around a kernel
> bug discovered along the way.
> 
> Jon Maloy (2):
>   tcp: leverage support of SO_PEEK_OFF socket option when available
>   tcp: handle shrunk window advertisemenst from guest
> 
>  tcp.c     | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  tcp.h     |  3 +++
>  tcp_buf.c | 25 ++++++++++++++++------
>  3 files changed, 81 insertions(+), 9 deletions(-)

Thanks, it looks good to me now! I'll wait for David's review before
applying it.

-- 
Stefano


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

* Re: [PATCH v9 2/2] tcp: handle shrunk window advertisemenst from guest
  2024-07-12 19:04 ` [PATCH v9 2/2] tcp: handle shrunk window advertisemenst from guest Jon Maloy
@ 2024-07-15  0:34   ` David Gibson
  2024-07-15 17:08     ` Stefano Brivio
  0 siblings, 1 reply; 8+ messages in thread
From: David Gibson @ 2024-07-15  0:34 UTC (permalink / raw)
  To: Jon Maloy; +Cc: passt-dev, sbrivio, lvivier, dgibson

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

On Fri, Jul 12, 2024 at 03:04:50PM -0400, Jon Maloy wrote:
> A bug in kernel TCP may lead to a deadlock where a zero window is sent
> from the guest peer, while it is unable to send out window updates even
> after socket reads have freed up enough buffer space to permit a larger
> window. In this situation, new window advertisements from the peer can
> only be triggered by data packets arriving from this side.
> 
> However, currently such packets are never sent, because the zero-window
> condition prevents this side from sending out any packets whatsoever
> to the peer.
> 
> We notice that the above bug is triggered *only* after the peer has
> dropped one or more arriving packets because of severe memory squeeze,
> and that we hence always enter a retransmission situation when this
> occurs. This also means that the implementation goes against the
> RFC-9293 recommendation that a previously advertised window never
> should shrink.
> 
> RFC-9293 seems to permit that we can continue sending up to the right
> edge of the last advertised non-zero window in such situations, so that
> is what we do to resolve this situation.
> 
> It turns out that this solution is extremely simple to implememt in the
> code: We just omit to save the advertised zero-window when we see that
> it has shrunk, i.e., if the acknowledged sequence number in the
> advertisement message is lower than that of the last data byte sent
> from our side.
> 
> When that is the case, the following happens:
> - The 'retr' flag in tcp_data_from_tap() will be 'false', so no
>   retransmission will occur at this occasion.
> - The data stream will soon reach the right edge of the previously
>   advertised window. In fact, in all observed cases we have seen that
>   it is already there when the zero-advertisement arrives.
> - At that moment, the flags STALLED and ACK_FROM_TAP_DUE will be set,
>   unless they already have been, meaning that only the next timer
>   expiration will open for data retransmission or transmission.
> - When that happens, the memory squeeze at the guest will normally have
>   abated, and the data flow can resume.
> 
> It should be noted that although this solves the problem we have at
> hand, it is a work-around, and not a genuine solution to the described
> kernel bug.
> 
> Suggested-by: Stefano Brivio <sbrivio@redhat.com>
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>

I only half-understand the problem here, but the fix LGTM.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

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

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

* Re: [PATCH v9 0/2] Add support for SO_PEEK_OFF
  2024-07-12 19:04 [PATCH v9 0/2] Add support for SO_PEEK_OFF Jon Maloy
                   ` (2 preceding siblings ...)
  2024-07-12 19:43 ` [PATCH v9 0/2] Add support for SO_PEEK_OFF Stefano Brivio
@ 2024-07-15 16:58 ` Stefano Brivio
  2024-07-15 18:52   ` Jon Maloy
  3 siblings, 1 reply; 8+ messages in thread
From: Stefano Brivio @ 2024-07-15 16:58 UTC (permalink / raw)
  To: Jon Maloy; +Cc: passt-dev, lvivier, dgibson

On Fri, 12 Jul 2024 15:04:48 -0400
Jon Maloy <jmaloy@redhat.com> wrote:

> We add support for the SO_PEEK_OFF option, plus work around a kernel
> bug discovered along the way.
> 
> Jon Maloy (2):
>   tcp: leverage support of SO_PEEK_OFF socket option when available
>   tcp: handle shrunk window advertisemenst from guest
> 
>  tcp.c     | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  tcp.h     |  3 +++
>  tcp_buf.c | 25 ++++++++++++++++------
>  3 files changed, 81 insertions(+), 9 deletions(-)

Applied!

I just s/advertisemenst/advertisements/ in 2/2, and fixed up the SHA of
the buggy kernel commit... I guess d4317abf7160 is from your local tree?
Upstream has e2142825c120.

-- 
Stefano


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

* Re: [PATCH v9 2/2] tcp: handle shrunk window advertisemenst from guest
  2024-07-15  0:34   ` David Gibson
@ 2024-07-15 17:08     ` Stefano Brivio
  0 siblings, 0 replies; 8+ messages in thread
From: Stefano Brivio @ 2024-07-15 17:08 UTC (permalink / raw)
  To: David Gibson; +Cc: Jon Maloy, passt-dev, lvivier, dgibson

On Mon, 15 Jul 2024 10:34:23 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jul 12, 2024 at 03:04:50PM -0400, Jon Maloy wrote:
> > A bug in kernel TCP may lead to a deadlock where a zero window is sent
> > from the guest peer, while it is unable to send out window updates even
> > after socket reads have freed up enough buffer space to permit a larger
> > window. In this situation, new window advertisements from the peer can
> > only be triggered by data packets arriving from this side.
> > 
> > However, currently such packets are never sent, because the zero-window
> > condition prevents this side from sending out any packets whatsoever
> > to the peer.
> > 
> > We notice that the above bug is triggered *only* after the peer has
> > dropped one or more arriving packets because of severe memory squeeze,
> > and that we hence always enter a retransmission situation when this
> > occurs. This also means that the implementation goes against the
> > RFC-9293 recommendation that a previously advertised window never
> > should shrink.
> > 
> > RFC-9293 seems to permit that we can continue sending up to the right
> > edge of the last advertised non-zero window in such situations, so that
> > is what we do to resolve this situation.
> > 
> > It turns out that this solution is extremely simple to implememt in the
> > code: We just omit to save the advertised zero-window when we see that
> > it has shrunk, i.e., if the acknowledged sequence number in the
> > advertisement message is lower than that of the last data byte sent
> > from our side.
> > 
> > When that is the case, the following happens:
> > - The 'retr' flag in tcp_data_from_tap() will be 'false', so no
> >   retransmission will occur at this occasion.
> > - The data stream will soon reach the right edge of the previously
> >   advertised window. In fact, in all observed cases we have seen that
> >   it is already there when the zero-advertisement arrives.
> > - At that moment, the flags STALLED and ACK_FROM_TAP_DUE will be set,
> >   unless they already have been, meaning that only the next timer
> >   expiration will open for data retransmission or transmission.
> > - When that happens, the memory squeeze at the guest will normally have
> >   abated, and the data flow can resume.
> > 
> > It should be noted that although this solves the problem we have at
> > hand, it is a work-around, and not a genuine solution to the described
> > kernel bug.
> > 
> > Suggested-by: Stefano Brivio <sbrivio@redhat.com>
> > Signed-off-by: Jon Maloy <jmaloy@redhat.com>  
> 
> I only half-understand the problem here

Long story short(er): we fill up the socket receive buffer in a Linux
guest, completely, complying with the window.

At that point, since kernel commit e2142825c120, on memory pressure, we
get an ACK segment from the Linux guest *not* acknowledging all the
data we sent (a bit less), but reporting zero as window (as if we sent
"too much" data, which is not the case).

After that, we don't get any further segment at all (second issue
introduced by e2142825c120), and whatever pending transfer times out.

-- 
Stefano


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

* Re: [PATCH v9 0/2] Add support for SO_PEEK_OFF
  2024-07-15 16:58 ` Stefano Brivio
@ 2024-07-15 18:52   ` Jon Maloy
  0 siblings, 0 replies; 8+ messages in thread
From: Jon Maloy @ 2024-07-15 18:52 UTC (permalink / raw)
  To: Stefano Brivio; +Cc: passt-dev, lvivier, dgibson



On 2024-07-15 12:58, Stefano Brivio wrote:
> On Fri, 12 Jul 2024 15:04:48 -0400
> Jon Maloy <jmaloy@redhat.com> wrote:
>
>> We add support for the SO_PEEK_OFF option, plus work around a kernel
>> bug discovered along the way.
>>
>> Jon Maloy (2):
>>    tcp: leverage support of SO_PEEK_OFF socket option when available
>>    tcp: handle shrunk window advertisemenst from guest
>>
>>   tcp.c     | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
>>   tcp.h     |  3 +++
>>   tcp_buf.c | 25 ++++++++++++++++------
>>   3 files changed, 81 insertions(+), 9 deletions(-)
> Applied!
Great! Finally. Thanks!
>
> I just s/advertisemenst/advertisements/ in 2/2, and fixed up the SHA of
> the buggy kernel commit... I guess d4317abf7160 is from your local tree?
I use net-next by old habit, and don't update it often. Maybe I should 
change that.
> Upstream has e2142825c120.
>
///jon


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

end of thread, other threads:[~2024-07-15 18:52 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-07-12 19:04 [PATCH v9 0/2] Add support for SO_PEEK_OFF Jon Maloy
2024-07-12 19:04 ` [PATCH v9 1/2] tcp: leverage support of SO_PEEK_OFF socket option when available Jon Maloy
2024-07-12 19:04 ` [PATCH v9 2/2] tcp: handle shrunk window advertisemenst from guest Jon Maloy
2024-07-15  0:34   ` David Gibson
2024-07-15 17:08     ` Stefano Brivio
2024-07-12 19:43 ` [PATCH v9 0/2] Add support for SO_PEEK_OFF Stefano Brivio
2024-07-15 16:58 ` Stefano Brivio
2024-07-15 18:52   ` Jon Maloy

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

	https://passt.top/passt

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