From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 338185A0337 for ; Thu, 2 May 2024 03:44:11 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1714614247; bh=fd8W9BRpbW1OMGK5bO38VcD/SFhO/RYiJc1rfG/DcQM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=hpGGN3x5CLuI5TJXit1OjqDRL3usCJF84/xRCw9KW9qBRmEBtP+3Y8qs3QE2jo4wU GfiX4Gv0cq4WUQdIgCm3Fi3j0uvJLa3HZXkPNVcWcTFi4aAXW7iW7jiA6Ff5kxZQkQ Fp8sdAgmBGCrIzNwIgOtDjSoFDYf/BhLNzjjEhtcsYeTora5oc8kU4m04k1JvfHG+G UoB9mGZnAb/WGEAy/xZVkgkHQFc3bHZcekcyLJmfZGomei4tHSTOeHZQqqN8QQKTiA quBzZXZv9UqXKKEKNxlufVO73HIGzpioXk0EKIujd7QPOZFVdqIfkvvqslikcVDYon cnT1svmW1V9fQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4VVGtv1J6gz4x1R; Thu, 2 May 2024 11:44:07 +1000 (AEST) Date: Thu, 2 May 2024 11:31:52 +1000 From: David Gibson To: Jon Maloy Subject: Re: [PATCH v2 1/2] tcp: leverage support of SO_PEEK_OFF socket option when available Message-ID: References: <20240501202839.3491885-1-jmaloy@redhat.com> <20240501202839.3491885-2-jmaloy@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="R8li7eIlJJvFRWEm" Content-Disposition: inline In-Reply-To: <20240501202839.3491885-2-jmaloy@redhat.com> Message-ID-Hash: GEHOWBQAJ7VTYLUTNZWAQNXP3WWZXECT X-Message-ID-Hash: GEHOWBQAJ7VTYLUTNZWAQNXP3WWZXECT X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, sbrivio@redhat.com, lvivier@redhat.com, dgibson@redhat.com X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --R8li7eIlJJvFRWEm Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, May 01, 2024 at 04:28:38PM -0400, Jon Maloy wrote: > >From linux-6.9.0 the kernel will contain > commit 05ea491641d3 ("tcp: add support for SO_PEEK_OFF socket option"). >=20 > 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. >=20 > In this commit, we add functionality to leverage this feature when > available, while we fall back to the previous behavior when not. >=20 > Measurements with iperf3 shows that throughput increases with 15-20 perce= nt > in the host->namespace direction when this feature is used. >=20 > Signed-off-by: Jon Maloy >=20 > --- > v2: - Some smaller changes as suggested by David Gibson and Stefano Brivi= o. > - Moved set_peek_offset() to only the locations where the socket is s= et > 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. > --- > tcp.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 55 insertions(+), 3 deletions(-) >=20 > diff --git a/tcp.c b/tcp.c > index 905d26f..535f1a2 100644 > --- a/tcp.c > +++ b/tcp.c > @@ -513,6 +513,9 @@ static struct iovec tcp6_l2_iov [TCP_FRAMES_MEM]; > static struct iovec tcp4_l2_flags_iov [TCP_FRAMES_MEM]; > static struct iovec tcp6_l2_flags_iov [TCP_FRAMES_MEM]; > =20 > +/* Does the kernel support TCP_PEEK_OFF? */ > +static bool peek_offset_cap; > + > /* sendmsg() to socket */ > static struct iovec tcp_iov [UIO_MAXIOV]; > =20 > @@ -582,6 +585,22 @@ static_assert(ARRAY_SIZE(tc_hash) >=3D FLOW_MAX, > int init_sock_pool4 [TCP_SOCK_POOL_SIZE]; > int init_sock_pool6 [TCP_SOCK_POOL_SIZE]; > =20 > +/** > + * set_peek_offset() - Set SO_PEEK_OFF offset on a socket if supported > + * @conn Connection struct with reference to socket and flags > + * @offset Offset in bytes > + */ > +static void set_peek_offset(struct tcp_tap_conn *conn, int offset) > +{ > + int s; > + > + if (!peek_offset_cap) > + return; > + s =3D conn->sock; > + if (setsockopt(s, SOL_SOCKET, SO_PEEK_OFF, &offset, sizeof(offset))) > + perror("Failed to set SO_PEEK_OFF\n"); > +} > + > /** > * tcp_conn_epoll_events() - epoll events mask for given connection state > * @events: Current connection events > @@ -2174,6 +2193,12 @@ static int tcp_data_from_sock(struct ctx *c, struc= t tcp_tap_conn *conn) > if (iov_rem) > iov_sock[fill_bufs].iov_len =3D iov_rem; > =20 > + if (peek_offset_cap) { > + /* Don't use discard buffer */ > + mh_sock.msg_iov =3D &iov_sock[1]; > + mh_sock.msg_iovlen -=3D 1; > + } > + I think this would be a little clearer if moved up to where we currently initialise mh_sock.msg_iov and iov_sock[0], and make that an else clause to this if. That would make it more obvious that we have two different - and mutually exclusive - mechanisms for dealing with un-acked data in the socket buffer. Not worth a respin on its own, though. > /* Receive into buffers, don't dequeue until acknowledged by guest. */ > do > len =3D recvmsg(s, &mh_sock, MSG_PEEK); > @@ -2195,7 +2220,10 @@ static int tcp_data_from_sock(struct ctx *c, struc= t tcp_tap_conn *conn) > return 0; > } > =20 > - sendlen =3D len - already_sent; > + sendlen =3D len; > + if (!peek_offset_cap) > + sendlen -=3D already_sent; > + > if (sendlen <=3D 0) { > conn_flag(c, conn, STALLED); > return 0; > @@ -2365,9 +2393,17 @@ static int tcp_data_from_tap(struct ctx *c, struct= tcp_tap_conn *conn, > flow_trace(conn, > "fast re-transmit, ACK: %u, previous sequence: %u", > max_ack_seq, conn->seq_to_tap); > + > + /* Ensure seq_from_tap isn't updated twice after call */ > + tcp_l2_data_buf_flush(c); tcp_l2_data_buf_flush() was replaced by tcp_payload_flush() in a recently merged change from Laurent. IIUC, this is necessary because otherwise our update to seq_to_tap can be clobbered from tcp_payload_flush() when we process the queued-but-not-sent frames. This seems like a correct fix, but not an optimal one: we're flushing out data we've already determined we're going to retransmit. Instead, I think we want a different helper that simply discards the queued frames - I'm thinking maybe we actually want a helper that's called from both the fast and slow retransmit paths and handles that. Ah, wait, we only want to discard queued frames that belong to this connection, that's trickier. It seems to me this is a pre-existing bug, we just managed to get away with it previously. I think this is at least one cause of the weirdly jumping forwarding sequence numbers you observed. So I think we want to make a patch fixing this that goes before the SO_PEEK_OFF changes. > + > conn->seq_ack_from_tap =3D max_ack_seq; > conn->seq_to_tap =3D max_ack_seq; > + set_peek_offset(conn, 0); > tcp_data_from_sock(c, conn); > + > + /* Empty queue before any POLL event tries to send it again */ > + tcp_l2_data_buf_flush(c); I'm not clear on what the second flush call is for. The only frames queued should be those added by the tcp_data_from_sock() just above, and those should be flushed when we get to tcp_defer_handler() before we return to the epoll loop. > } > =20 > if (!iov_i) > @@ -2459,6 +2495,7 @@ static void tcp_conn_from_sock_finish(struct ctx *c= , struct tcp_tap_conn *conn, > conn->seq_ack_to_tap =3D conn->seq_from_tap; > =20 > conn_event(c, conn, ESTABLISHED); > + set_peek_offset(conn, 0); > =20 > /* The client might have sent data already, which we didn't > * dequeue waiting for SYN,ACK from tap -- check now. > @@ -2539,6 +2576,7 @@ int tcp_tap_handler(struct ctx *c, uint8_t pif, int= af, > goto reset; > =20 > conn_event(c, conn, ESTABLISHED); > + set_peek_offset(conn, 0); The set_peek_offset() could go into conn_event() to avoid the duplication. Not sure if it's worth it or not. > if (th->fin) { > conn->seq_from_tap++; > @@ -2705,7 +2743,7 @@ void tcp_listen_handler(struct ctx *c, union epoll_= ref ref, > struct sockaddr_storage sa; > socklen_t sl =3D sizeof(sa); > union flow *flow; > - int s; > + int s =3D 0; > =20 > if (c->no_tcp || !(flow =3D flow_alloc())) > return; > @@ -2767,7 +2805,10 @@ void tcp_timer_handler(struct ctx *c, union epoll_= ref ref) > flow_dbg(conn, "ACK timeout, retry"); > conn->retrans++; > conn->seq_to_tap =3D conn->seq_ack_from_tap; > + set_peek_offset(conn, 0); > + tcp_l2_data_buf_flush(c); Uh.. doesn't this flush have to go *before* the seq_to_tap update, for the reasons discussed above? > tcp_data_from_sock(c, conn); > + tcp_l2_data_buf_flush(c); > tcp_timer_ctl(c, conn); > } > } else { > @@ -3041,7 +3082,8 @@ static void tcp_sock_refill_init(const struct ctx *= c) > */ > int tcp_init(struct ctx *c) > { > - unsigned b; > + unsigned int b, optv =3D 0; > + int s; > =20 > for (b =3D 0; b < TCP_HASH_TABLE_SIZE; b++) > tc_hash[b] =3D FLOW_SIDX_NONE; > @@ -3065,6 +3107,16 @@ int tcp_init(struct ctx *c) > NS_CALL(tcp_ns_socks_init, c); > } > =20 > + /* Probe for SO_PEEK_OFF support */ > + s =3D socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, IPPROTO_TCP); > + if (s < 0) { > + warn("Temporary TCP socket creation failed"); > + } else { > + if (!setsockopt(s, SOL_SOCKET, SO_PEEK_OFF, &optv, sizeof(int))) > + peek_offset_cap =3D true; > + close(s); > + } > + debug("SO_PEEK_OFF%ssupported", peek_offset_cap ? " " : " not "); > return 0; > } > =20 --=20 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 --R8li7eIlJJvFRWEm Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmYy7PQACgkQzQJF27ox 2GdrZxAAgzTVq3kv4DYQlejOzzGIOhnmlfZFlWP2ky6PNRgQNtPSm4Cg0s9BoAIF HKwPzN8VWuRzXkKxJQJ1stDN5hzond0PEJOJRKYZThpjNvDG9FSgB8rM8s9vhAAj Ow6B70LJQUalNxZY/DrNJxXYrm6QedPA08AgQ8LkJSj2G9hX2V+jWQXSfYdnOx8I JabA//CRHM/DPSmuqyEGCH35FIvqwJMHuB500X1gqpKPOIV9gHypJn3ATfdzrZK8 /+pOljD1mE0rvpF9pT1mbiUhEBNeHEVXse9l0PODRNkrFUSUoTU8KA9DTh/vv6hj G5zsHeTqfonf5jLP4RlHAA5URwZ03OXg5iCacWAu5ya8P2n6B5p/YNA0aqzjZjZd QeXsl9vymDoqVsukOPoqg2Z6nlQTLMAX1d9PqjW4icCfbr6JLbIQI//bHVLbUdU6 SfK1QQq8WkYm9x/UAqb8TWsp40Wrv0xLrEUqEXYGQ0vDpAvR3+pwT6IrVNjYrayS 0NWmQlNrHHVlPWL7z+CyM99CWRCNzeBAiXJeOak9RwfLWhi95sEUt3lSnoxL0ThG wdr9FjuuAJJKMY5LllXGN68rI/KnN5MZ68uy3QQgvb97qtvLpoYaz0O5DC6oy6Ig AtXNoaEBYM7RqY9bnB2CwxOzbnJHFqr3o99tolXrtEM478b1ToA= =M8ms -----END PGP SIGNATURE----- --R8li7eIlJJvFRWEm--