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 <passt-dev@passt.top>; 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 <david@gibson.dropbear.id.au>
To: Stefano Brivio <sbrivio@redhat.com>
Subject: Re: [PATCH 15/32] tcp: Unify part of spliced and non-spliced
conn_from_sock path
Message-ID: <Y3WQQL3wXk4Go1F/@yekko>
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 <passt-dev.passt.top>
Archived-At: <https://archives.passt.top/passt-dev/Y3WQQL3wXk4Go1F/@yekko/>
Archived-At: <https://passt.top/hyperkitty/list/passt-dev@passt.top/message/RJ67DQF45JSHNIVHDSLSYOXVUC6NYQKX/>
List-Archive: <https://archives.passt.top/passt-dev/>
List-Archive: <https://passt.top/hyperkitty/list/passt-dev@passt.top/>
List-Help: <mailto:passt-dev-request@passt.top?subject=help>
List-Owner: <mailto:passt-dev-owner@passt.top>
List-Post: <mailto:passt-dev@passt.top>
List-Subscribe: <mailto:passt-dev-join@passt.top>
List-Unsubscribe: <mailto:passt-dev-leave@passt.top>
--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 <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> > ---
> > 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--