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 B2C025A027F for ; Fri, 1 Dec 2023 01:08:52 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1701389330; bh=dCbmuQPrWp85TmTIuye+1pDP1+TxSkGH9ZR1EtlShhA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=O4NuZvMoh53g2ECXBk/Ev/bXDkNWfx9Az9LbZFO7dOEjAoT2LTlwWpsB7lfsXeG19 Vyr0BV1G2RFapmQJeyvTxxfIghniz6E2Kg/yllazYVMwhP/wQqYhAY0A4zzzx6zkpj 6ZvEgdf9t0Bkgm6FbheuJRrMKLq/Uh0+nTf3jkVY= Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4ShD1Z1TSyz4xDB; Fri, 1 Dec 2023 11:08:50 +1100 (AEDT) Date: Fri, 1 Dec 2023 11:07:50 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v3 16/16] tcp: Don't defer hash table removal Message-ID: References: <20231130020222.4056647-1-david@gibson.dropbear.id.au> <20231130020222.4056647-17-david@gibson.dropbear.id.au> <20231130134532.7c5b959e@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="xx01jZwfq4fjW7W9" Content-Disposition: inline In-Reply-To: <20231130134532.7c5b959e@elisabeth> Message-ID-Hash: WIION7TZDBWT54A53TNSFQBUEOLRRXCN X-Message-ID-Hash: WIION7TZDBWT54A53TNSFQBUEOLRRXCN 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.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: --xx01jZwfq4fjW7W9 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Nov 30, 2023 at 01:45:32PM +0100, Stefano Brivio wrote: > On Thu, 30 Nov 2023 13:02:22 +1100 > David Gibson wrote: >=20 > > When a TCP connection is closed, we mark it by setting events to CLOSED, > > then some time later we do final cleanups: closing sockets, removing fr= om > > the hash table and so forth. > >=20 > > This does mean that when making a hash lookup we need to exclude any > > apparent matches that are CLOSED, since they represent a stale connecti= on. > > This can happen in practice if one connection closes and a new one with= the > > same endpoints is started shortly afterward. > >=20 > > Checking for CLOSED is quite specific to TCP however, and won't work wh= en > > we extend the hash table to more general flows. So, alter the code to > > immediately remove the connection from the hash table when CLOSED, alth= ough > > we still defer closing sockets and other cleanup. > >=20 > > Signed-off-by: David Gibson > > --- > > tcp.c | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > >=20 > > diff --git a/tcp.c b/tcp.c > > index 74d06bf..17c7cba 100644 > > --- a/tcp.c > > +++ b/tcp.c > > @@ -781,6 +781,9 @@ static void conn_flag_do(const struct ctx *c, struc= t tcp_tap_conn *conn, > > tcp_timer_ctl(c, conn); > > } > > =20 > > +static void tcp_hash_remove(const struct ctx *c, > > + const struct tcp_tap_conn *conn); > > + > > /** > > * conn_event_do() - Set and log connection events, update epoll state > > * @c: Execution context > > @@ -825,7 +828,9 @@ static void conn_event_do(const struct ctx *c, stru= ct tcp_tap_conn *conn, > > flow_dbg(conn, "%s", > > num =3D=3D -1 ? "CLOSED" : tcp_event_str[num]); > > =20 > > - if ((event =3D=3D TAP_FIN_RCVD) && !(conn->events & SOCK_FIN_RCVD)) > > + if (event =3D=3D CLOSED) > > + tcp_hash_remove(c, conn); > > + else if ((event =3D=3D TAP_FIN_RCVD) && !(conn->events & SOCK_FIN_RCV= D)) > > conn_flag(c, conn, ACTIVE_CLOSE); > > else > > tcp_epoll_ctl(c, conn); > > @@ -1150,7 +1155,7 @@ static int tcp_hash_match(const struct tcp_tap_co= nn *conn, > > const union inany_addr *faddr, > > in_port_t eport, in_port_t fport) > > { > > - if (conn->events !=3D CLOSED && inany_equals(&conn->faddr, faddr) && > > + if (inany_equals(&conn->faddr, faddr) && > > conn->eport =3D=3D eport && conn->fport =3D=3D fport) > > return 1; > > =20 > > @@ -1308,7 +1313,6 @@ static void tcp_conn_destroy(struct ctx *c, union= flow *flow) > > if (conn->timer !=3D -1) > > close(conn->timer); > > =20 > > - tcp_hash_remove(c, conn); > > flow_table_compact(c, flow); >=20 > I was pretty sure, due to the way I originally implemented this, that > removing an entry from the hash table without compacting the table > afterwards, with an event possibly coming between the two, would > present some inconsistency while we're handling that event. >=20 > But looking at it now, I don't see any issue with this. I just wanted > to raise it in case you're aware of (but didn't think about) some > concern in this sense. I think it's ok. The thing is that compacting the connection table itself is largely independent of the hash table, whose buckets are separately indexed. A hash remove shuffles things around in the hash buckets, but doesn't change where connections sit in the connection table. A connection table compaction changes the indices in the connection table, which requires updating things in the hash buckets, but not moving things around in the buckets - exactly the same entries are in every hash bucket, it's just that one of them has a new "name" now. > By the way, the reason why I deferred tcp_hash_remove() back then was > to save cycles between epoll events and get higher CRR rates, but I > think the effect is negligible anyway. Right.. to process a FIN and the next SYN at once, I guess? I figured this might make a difference, but probably not much. There's no syscall here, and batching doesn't reduce the total amount of work in this case. --=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 --xx01jZwfq4fjW7W9 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmVpI9UACgkQzQJF27ox 2GeX/A//QRClbtSZtqtMlj4o9AHiPWi/5wAE/J67dZ48lXK4/MYu90IhDvkZu8YK /iXZC0Dqec0ARen7uKWaP4on4cfCusNEXkT+n7yq5dt15bMICjTZ9zjK/2WHlN58 UJGjQauEVO2AZodAXo/Oz8eFabI9fsB/bKfu9IsWPQrSiYqF7BG+VDtb8NflpE71 W0/BaIjHMeKTdLZN/JI7kKILnuMLDNrhKRIbBmT1bxh6ZsU1BV2AYb+1i9PhQdlI UCeDMNBGL9NIa2BJz2TS/VmkGjtMfK+On3ztqkaMEBI4BjpEuQrPxWqKcvETbNzz lOjDDNpMq4KQuOpYyfw6AKZhO4AQubRECfVYwKQXRM5E8n957XuVCzDCqaJJ7R7b kJbe4LpNhsqXY32VRvlC3vyXjNO1mE2d3sVFBmnUsa6TDA6GXVJn5isuSEWJ8KAA xg9t+4Djm+6ZoHDNvTnQ2CXU2sAQU6Br46PFynVkuLDnZ42/+jgaJTIK7Jzar1Ag uo8pDhvvDOxy4RlSddpGJtbuIPBHk7To4pH24z7DNwy3ewXGLut9ceyR7S4sPFMg ys0nXa8qh7lD0YBZKKODHm1c7VKXqCT6Hag0I6NK1mzAfyn5+iMo0eOh7UzywduU dAVcU/dWnpUaqabsci7kyPIBJJKm528CwSrfZl6MTmG9jWal4j8= =ptlg -----END PGP SIGNATURE----- --xx01jZwfq4fjW7W9--