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 15B625A0268 for ; Thu, 17 Nov 2022 03:08:53 +0100 (CET) Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4NCNcx2QLqz4xZY; Thu, 17 Nov 2022 13:08:49 +1100 (AEDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1668650929; bh=1TUVEL+GaQGwDqmR6U1zmN6PfGCD0kAIdgsUL0G8tYk=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=JOkUeRo3SAyioON+5er1vryVS9169nFJ1h1W1+o8fKvwtLwGHWdjFfjKZwDpwLa4/ DDZ9/9/9y2iCF8xdMfHWfX9TWS3tL5puRRThyvbcyfvFZQXpAn2rvskd0Ohic6R8D/ VNhQXC4ENsbdkNgC3eHQqCNsaBMLSfvwtTO850c8= Date: Thu, 17 Nov 2022 12:37:04 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 15/32] tcp: Unify part of spliced and non-spliced conn_from_sock path Message-ID: References: <20221116044212.3876516-1-david@gibson.dropbear.id.au> <20221116044212.3876516-16-david@gibson.dropbear.id.au> <20221117005358.324ca3a0@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="W4GF/24XfTyyvzsV" Content-Disposition: inline In-Reply-To: <20221117005358.324ca3a0@elisabeth> Message-ID-Hash: RJ67DQF45JSHNIVHDSLSYOXVUC6NYQKX X-Message-ID-Hash: RJ67DQF45JSHNIVHDSLSYOXVUC6NYQKX 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 X-Mailman-Version: 3.3.3 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: --W4GF/24XfTyyvzsV Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Nov 17, 2022 at 12:53:58AM +0100, Stefano Brivio wrote: > On Wed, 16 Nov 2022 15:41:55 +1100 > David Gibson wrote: >=20 > > In tcp_sock_handler() we split off to handle spliced sockets before > > checking anything else. However the first steps of the "new connection" > > path for each case are the same: allocate a connection entry and accept= () > > the connection. > >=20 > > Remove this duplication by making tcp_conn_from_sock() handle both spli= ced > > and non-spliced cases, with help from more specific tcp_tap_conn_from_s= ock > > and tcp_splice_conn_from_sock functions for the later stages which diff= er. > >=20 > > Signed-off-by: David Gibson > > --- > > tcp.c | 68 ++++++++++++++++++++++++++++++++++------------------ > > tcp_splice.c | 58 +++++++++++++++++++++++--------------------- > > tcp_splice.h | 4 ++++ > > 3 files changed, 80 insertions(+), 50 deletions(-) > >=20 > > diff --git a/tcp.c b/tcp.c > > index 72d3b49..e66a82a 100644 > > --- a/tcp.c > > +++ b/tcp.c > > @@ -2753,28 +2753,19 @@ static void tcp_connect_finish(struct ctx *c, s= truct tcp_tap_conn *conn) > > } > > =20 > > /** > > - * tcp_conn_from_sock() - Handle new connection request from listening= socket > > + * tcp_tap_conn_from_sock() - Initialize state for non-spliced connect= ion > > * @c: Execution context > > * @ref: epoll reference of listening socket > > + * @conn: connection structure to initialize > > + * @s: Accepted socket > > + * @sa: Peer socket address (from accept()) > > * @now: Current timestamp > > */ > > -static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref, > > - const struct timespec *now) > > +static void tcp_tap_conn_from_sock(struct ctx *c, union epoll_ref ref, > > + struct tcp_tap_conn *conn, int s, > > + struct sockaddr *sa, > > + const struct timespec *now) > > { > > - struct sockaddr_storage sa; > > - struct tcp_tap_conn *conn; > > - socklen_t sl; > > - int s; > > - > > - if (c->tcp.conn_count >=3D TCP_MAX_CONNS) > > - return; > > - > > - sl =3D sizeof(sa); > > - s =3D accept4(ref.r.s, (struct sockaddr *)&sa, &sl, SOCK_NONBLOCK); > > - if (s < 0) > > - return; > > - > > - conn =3D CONN(c->tcp.conn_count++); > > conn->c.spliced =3D false; > > conn->sock =3D s; > > conn->timer =3D -1; > > @@ -2784,7 +2775,7 @@ static void tcp_conn_from_sock(struct ctx *c, uni= on epoll_ref ref, > > if (ref.r.p.tcp.tcp.v6) { > > struct sockaddr_in6 sa6; > > =20 > > - memcpy(&sa6, &sa, sizeof(sa6)); > > + memcpy(&sa6, sa, sizeof(sa6)); > > =20 > > if (IN6_IS_ADDR_LOOPBACK(&sa6.sin6_addr) || > > IN6_ARE_ADDR_EQUAL(&sa6.sin6_addr, &c->ip6.addr_seen) || > > @@ -2813,7 +2804,7 @@ static void tcp_conn_from_sock(struct ctx *c, uni= on epoll_ref ref, > > } else { > > struct sockaddr_in sa4; > > =20 > > - memcpy(&sa4, &sa, sizeof(sa4)); > > + memcpy(&sa4, sa, sizeof(sa4)); > > =20 > > memset(&conn->a.a4.zero, 0, sizeof(conn->a.a4.zero)); > > memset(&conn->a.a4.one, 0xff, sizeof(conn->a.a4.one)); > > @@ -2846,6 +2837,37 @@ static void tcp_conn_from_sock(struct ctx *c, un= ion epoll_ref ref, > > tcp_get_sndbuf(conn); > > } > > =20 > > +/** > > + * tcp_conn_from_sock() - Handle new connection request from listening= socket > > + * @c: Execution context > > + * @ref: epoll reference of listening socket > > + * @now: Current timestamp > > + */ > > +static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref, > > + const struct timespec *now) > > +{ > > + struct sockaddr_storage sa; > > + union tcp_conn *conn; > > + socklen_t sl; > > + int s; > > + > > + if (c->tcp.conn_count >=3D TCP_MAX_CONNS) > > + return; > > + > > + sl =3D sizeof(sa); > > + s =3D accept4(ref.r.s, (struct sockaddr *)&sa, &sl, SOCK_NONBLOCK); >=20 > Combined with 16/32 I'm not sure this is simplifying much -- it looks a > bit unnatural there to get the peer address not "directly" from > accept4(). On the other hand you drop a few lines -- I'm fine with > it either way. Um.. I'm not really sure what you're getting at here. > > + if (s < 0) > > + return; > > + > > + conn =3D tc + c->tcp.conn_count++; > > + > > + if (ref.r.p.tcp.tcp.splice) > > + tcp_splice_conn_from_sock(c, ref, &conn->splice, s); > > + else > > + tcp_tap_conn_from_sock(c, ref, &conn->tap, s, > > + (struct sockaddr *)&sa, now); > > +} > > + > > /** > > * tcp_timer_handler() - timerfd events: close, send ACK, retransmit, = or reset > > * @c: Execution context > > @@ -2925,13 +2947,13 @@ void tcp_sock_handler(struct ctx *c, union epol= l_ref ref, uint32_t events, > > return; > > } > > =20 > > - if (ref.r.p.tcp.tcp.splice) { > > - tcp_sock_handler_splice(c, ref, events); > > + if (ref.r.p.tcp.tcp.listen) { > > + tcp_conn_from_sock(c, ref, now); > > return; > > } > > =20 > > - if (ref.r.p.tcp.tcp.listen) { > > - tcp_conn_from_sock(c, ref, now); > > + if (ref.r.p.tcp.tcp.splice) { > > + tcp_sock_handler_splice(c, ref, events); > > return; > > } > > =20 > > diff --git a/tcp_splice.c b/tcp_splice.c > > index 7a06252..7007501 100644 > > --- a/tcp_splice.c > > +++ b/tcp_splice.c > > @@ -501,6 +501,36 @@ static void tcp_splice_dir(struct tcp_splice_conn = *conn, int ref_sock, > > *pipes =3D *from =3D=3D conn->a ? conn->pipe_a_b : conn->pipe_b_a; > > } > > =20 > > +/** > > + * tcp_splice_conn_from_sock() - Initialize state for spliced connecti= on > > + * @c: Execution context > > + * @ref: epoll reference of listening socket > > + * @conn: connection structure to initialize > > + * @s: Accepted socket > > + * > > + * #syscalls:pasta setsockopt > > + */ > > +void tcp_splice_conn_from_sock(struct ctx *c, union epoll_ref ref, > > + struct tcp_splice_conn *conn, int s) > > +{ > > + assert(c->mode =3D=3D MODE_PASTA); > > + > > + if (setsockopt(s, SOL_TCP, TCP_QUICKACK, &((int){ 1 }), > > + sizeof(int))) { > > + trace("TCP (spliced): failed to set TCP_QUICKACK on %i", > > + s); >=20 > This could be indented sanely, now. Fixed. > > + } > > + > > + conn->c.spliced =3D true; > > + c->tcp.splice_conn_count++; > > + conn->a =3D s; > > + conn->flags =3D ref.r.p.tcp.tcp.v6 ? SPLICE_V6 : 0; > > + > > + if (tcp_splice_new(c, conn, ref.r.p.tcp.tcp.index, > > + ref.r.p.tcp.tcp.outbound)) > > + conn_flag(c, conn, CLOSING); > > +} > > + > > /** > > * tcp_sock_handler_splice() - Handler for socket mapped to spliced co= nnection > > * @c: Execution context > > @@ -517,33 +547,7 @@ void tcp_sock_handler_splice(struct ctx *c, union = epoll_ref ref, > > uint32_t *seq_read, *seq_write; > > struct tcp_splice_conn *conn; > > =20 > > - if (ref.r.p.tcp.tcp.listen) { > > - int s; > > - > > - if (c->tcp.conn_count >=3D TCP_MAX_CONNS) > > - return; > > - > > - if ((s =3D accept4(ref.r.s, NULL, NULL, SOCK_NONBLOCK)) < 0) > > - return; > > - > > - if (setsockopt(s, SOL_TCP, TCP_QUICKACK, &((int){ 1 }), > > - sizeof(int))) { > > - trace("TCP (spliced): failed to set TCP_QUICKACK on %i", > > - s); > > - } > > - > > - conn =3D CONN(c->tcp.conn_count++); > > - conn->c.spliced =3D true; > > - c->tcp.splice_conn_count++; > > - conn->a =3D s; > > - conn->flags =3D ref.r.p.tcp.tcp.v6 ? SPLICE_V6 : 0; > > - > > - if (tcp_splice_new(c, conn, ref.r.p.tcp.tcp.index, > > - ref.r.p.tcp.tcp.outbound)) > > - conn_flag(c, conn, CLOSING); > > - > > - return; > > - } > > + assert(!ref.r.p.tcp.tcp.listen); > > =20 > > conn =3D CONN(ref.r.p.tcp.tcp.index); > > =20 > > diff --git a/tcp_splice.h b/tcp_splice.h > > index 22024d6..f9462ae 100644 > > --- a/tcp_splice.h > > +++ b/tcp_splice.h > > @@ -6,8 +6,12 @@ > > #ifndef TCP_SPLICE_H > > #define TCP_SPLICE_H > > =20 > > +struct tcp_splice_conn; > > + > > void tcp_sock_handler_splice(struct ctx *c, union epoll_ref ref, > > uint32_t events); > > +void tcp_splice_conn_from_sock(struct ctx *c, union epoll_ref ref, > > + struct tcp_splice_conn *conn, int s); > > void tcp_splice_init(struct ctx *c); > > =20 > > #endif /* TCP_SPLICE_H */ >=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 --W4GF/24XfTyyvzsV Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEoULxWu4/Ws0dB+XtgypY4gEwYSIFAmN1kDoACgkQgypY4gEw YSJNbQ//fhCwwmsw71QqzDxBiXq7kcuedHtv5AJd2pa3dpuotU8EbmM1pw9l424F D8EoYhxPsBQ5dCPzDrZYpdM5Zlp53PZnfwmKFdxmzkpP6OHTKGnX8M8SUIB4UDdQ FerWBYe/PgmlDNzeIA38lZoo/g/9lO8VGYFMMmyCwDRh26rkiqTCoCmNgXqCKZj1 B4ryY4IUDXabtDvOr+mFmBfFJW82tdImsof6RWe0vTsjUverUzYu5i63foozSls6 pPDy8pKK0o8c9Jc02Tr90iG1/yYCQgYn0ZYCgODnEorsWe88tmLRxUIYdTu8DRAR AgOMqw+cnjTSt6Mh5IwFkicw0C5r2vsqPOyOfwbWcmAF5A3u11rDzo0YZtMlQe+p 1CGcPbcAA1m0JJFQOQuONFIS+7/mRN7Br/mdwXqgFWN3xKwttP441I7+3jxLoqv9 +3Xf4lSvP3DPitfbsij3grOHMWtwzQJgs8qMIcvNZB8lhWFNun0C0ClIzFYv2FSp byvMBKIt5Qqvj2nTAxqn6g14w9ZtWZN2m8VYkPdCxHCgavX00NH/XKZNtU6DvP6K 7KcLZHseRmTMegEbM3zrcMzn3gGN8GHfS7dZ4bVJH6SEze1mpSZYHIJ/s04Acbm7 SPXOcQBwErfgPpj81XfnDzVBkcwuC7ZSeePM2uAFRGBYi9BIdz8= =BBdx -----END PGP SIGNATURE----- --W4GF/24XfTyyvzsV--