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 D73FB5A0274 for ; Fri, 19 Jan 2024 01:26:39 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1705623993; bh=esRghxYVU2jjszFh4zhrADjHg/O9xXWHjDphAVAfA4A=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=kZIBooDNGeG8uCHC9YHoS383gtEUu3Iv+V5hqWSzSWsghLqOUq7HDGi6vZaOEM0yd Znf4eShFXsJQVzBZZYBUot52WNlvwNAZcW5X0AECkF1aA44JquItLGuIR5sFlrHRcf 1Xv5JwvoldclpNizqv9+Am7w6Aa0VZxQJDXZd9dj5Rutpo1PP4Jg32X4gh/jFxh4DU LW0peHPiPq6AFcL6EV+OlM/lgiYc+h3eu81UOnpzS2ooiO3B6F/gjifcG+MaaYLsq+ RqrUeLDKUPQcltELNubTrVW04M7MyeWfyXPbu4xvbhK+fsRyVlqQTpIVc5uNv/xzdn Zr56M1vP7xsgw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4TGL5P63lRz4xPN; Fri, 19 Jan 2024 11:26:33 +1100 (AEDT) Date: Fri, 19 Jan 2024 10:55:51 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v3 06/15] tcp, flow: Replace TCP specific hash function with general flow hash Message-ID: References: <20231221070237.1422557-1-david@gibson.dropbear.id.au> <20231221070237.1422557-7-david@gibson.dropbear.id.au> <20240117205934.4b7d7043@elisabeth> <20240118164234.1db14655@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="/HzgRVX5DKzcCCFE" Content-Disposition: inline In-Reply-To: <20240118164234.1db14655@elisabeth> Message-ID-Hash: 3WT6LX4LKM5AGETR2MPCT3LETGHQEILA X-Message-ID-Hash: 3WT6LX4LKM5AGETR2MPCT3LETGHQEILA 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: --/HzgRVX5DKzcCCFE Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jan 18, 2024 at 04:42:34PM +0100, Stefano Brivio wrote: > On Thu, 18 Jan 2024 12:15:12 +1100 > David Gibson wrote: >=20 > > On Wed, Jan 17, 2024 at 08:59:34PM +0100, Stefano Brivio wrote: > > > On Thu, 21 Dec 2023 18:02:28 +1100 > > > David Gibson wrote: > > > =20 > > > > Currently we match TCP packets received on the tap connection to a = TCP > > > > connection via a hash table based on the forwarding address and both > > > > ports. We hope in future to allow for multiple guest side addresse= s, or > > > > for multiple interfaces which means we may need to distinguish base= d on > > > > the endpoint address and pif as well. We also want a unified hash = table > > > > to cover multiple protocols, not just TCP. > > > >=20 > > > > Replace the TCP specific hash function with one suitable for genera= l flows, > > > > or rather for one side of a general flow. This includes all the > > > > information from struct flowside, plus the L4 protocol number. > > > >=20 > > > > Signed-off-by: David Gibson > > > > --- > > > > flow.c | 21 +++++++++++++++++++++ > > > > flow.h | 19 +++++++++++++++++++ > > > > tcp.c | 59 +++++++++++-------------------------------------------= ---- > > > > 3 files changed, 51 insertions(+), 48 deletions(-) > > > >=20 > > > > diff --git a/flow.c b/flow.c > > > > index bc8cfc6..263633e 100644 > > > > --- a/flow.c > > > > +++ b/flow.c > > > > @@ -229,6 +229,27 @@ void flow_alloc_cancel(union flow *flow) > > > > flow_first_free =3D FLOW_IDX(flow); > > > > } > > > > =20 > > > > +/** > > > > + * flow_hash() - Calculate hash value for one side of a flow > > > > + * @c: Execution context > > > > + * @proto: Protocol of this flow (IP L4 protocol number) > > > > + * @fside: Flowside > > > > + * > > > > + * Return: hash value > > > > + */ > > > > +uint64_t flow_hash(const struct ctx *c, uint8_t proto, > > > > + const struct flowside *fside) > > > > +{ > > > > + struct siphash_state state =3D SIPHASH_INIT(c->hash_secret); > > > > + > > > > + ASSERT(flowside_complete(fside)); > > > > + inany_siphash_feed(&state, &fside->faddr); > > > > + inany_siphash_feed(&state, &fside->eaddr); =20 > > >=20 > > > Customary newline here. =20 > >=20 > > Done. > >=20 > > > > + return siphash_final(&state, 38, (uint64_t)proto << 40 | > > > > + (uint64_t)fside->pif << 32 | > > > > + fside->fport << 16 | fside->eport); =20 > > >=20 > > > If we add the fields from the 'tail' part (not the whole fside) to an > > > anonymous struct in a similar way to what we had before fc8f0f8c48ef > > > ("siphash: Use incremental rather than all-at-once siphash functions"= ), > > > then we could drop those shift and masks, and use sizeof(that) + > > > sizeof(fside->faddr) + sizeof(fside->eaddr) instead of '38'. =20 > >=20 > > Hrm, I guess so. There are some wrinkles: > > * we'd need a union so we can get the actual u64 value we need to > > pass >=20 > I haven't tried, but I guess you can just cast a (packed) struct. No... I don't think you can: $ cat pack64.c #include #include #include int main(int argc, char *argv[]) { struct foo { uint16_t a, b; uint8_t c; } bar =3D {1, 2, 3}; printf("bar is %zd bytes and has 64-bit value %08lx\n", sizeof(bar), (uint64_t)bar); exit(0); } $ gcc pack64.c pack64.c: In function =E2=80=98main=E2=80=99: pack64.c:13:16: error: aggregate value used where an integer was expected 13 | sizeof(bar), (uint64_t)bar); | ^~~~~~ > > * we'd need to make sure that the the remaining (64-38 =3D=3D 26) byt= es > > of that union are consistently initialised >=20 > Where do the 64 _bytes_ come from? Duh, sorry. I meant the remaining (8 - 6 =3D=3D 2) bytes of the union. > > * the struct part would need to be packed, or padding will mess with > > us >=20 > Right, yes, just see tcp_seq_init() before fc8f0f8c48ef. >=20 > > * the exact value would now depend on the host endianness, which is > > probably fine, but worth noting >=20 > Oh, I didn't even think of that when I wrote the old tcp_seq_init(). > Anyway, yes, it doesn't matter. >=20 > > > > +} > > > > + > > > > /** > > > > * flow_defer_handler() - Handler for per-flow deferred and timed = tasks > > > > * @c: Execution context > > > > diff --git a/flow.h b/flow.h > > > > index e7c4484..72ded54 100644 > > > > --- a/flow.h > > > > +++ b/flow.h > > > > @@ -81,6 +81,22 @@ static inline bool flowside_complete(const struc= t flowside *fside) > > > > =20 > > > > #define SIDES 2 > > > > =20 > > > > +/** > > > > + * flowside_eq() - Check if two flowsides are equal =20 > > >=20 > > > ...this raises the question: if the protocol number is now used in the > > > hash, shouldn't it eventually become part of struct flowside -- and > > > compared here, too? =20 > >=20 > > > I guess it's useful iff we allow flowsides for the same flow to have > > > different protocol numbers. =20 > >=20 > > Right, which is not something I'm planning on doing, or at least not > > very soon. > >=20 > > > Now, forwarding between TCP and UDP endpoints might not make a lot of > > > sense, because we would have to make so many arbitrary assumptions as > > > to make it probably not generic enough to be useful. =20 > >=20 > > Exactly. > >=20 > > > But TCP to stream-oriented UNIX domain socket makes sense, and we also > > > had user requests in that sense. Oh and... what would be the second > > > protocol number in that case? =20 > >=20 > > Right, that's a possible future extension. But if we're going outside > > the realm of IP, a number of things need to be changed. I think > > that's something for another day. >=20 > I'm not suggesting to support that right away, I was just wondering if > it actually makes sense, right from the beginning, to keep the hash > information "consistent" with the flow table information, including > having the protocol number in the flow table. >=20 > But I didn't really think it through, you know better. Yeah, I think this would at most make things very slightly easier down the track when we want to add that, at the cost of making things significantly less natural now. --=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 --/HzgRVX5DKzcCCFE Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmWpunsACgkQzQJF27ox 2Gcw6BAAhdvdRn8W7uqrf0VpBeca6OcKSMG3EL0ty6HLmo7hTB1HwulaCJYzwi5x tzVEta7QFwk8yIJr97YBj3md73rb7oNukdBsG2riZMRPjAx76SUVf8+anQzGcMA4 tAT2elxgQBzVKbqknLUCRUhrJronHOa6fK6MXg+EgBDLyCMLPI/Re6ac/TbPu7tt zXc0CaZdtohyTCIsFzl0krYnqEBAEEbGuRTGVJXDO1nrfDRLRLi0UmcuiFaSdzs8 UjWL++NPXRAtmBVh8VMBWUVQGHtcx7yG7/Xf5KrIqkV9J+HzYPfMbD+kKUEzTQT2 AfGrtIRNpXeccU16F9DZ2CVjkv1Uj14bfqjnIeq6tzua7Fra4myR+YI7dMGQIu7E B6HEzvFesrsNExvBWsURr0bHmLvhmv9ZucTYu0TkYJyG+PpVYxi9+saVQLRiOojg 1slNZaQaXFtkcolFnmyBWV18Xf92CDHO77cCkm8+eyi9lT+nPOTIi85fqXPxk+ij v55PlUvvEtf4EuvJEJ9oYMhO5pdduOnl2bJkbZnTu+4Tk+BbbTio0UWDmC+ZDle+ 2c5f/QqCDRb9DR03iDoGmMJLCPx1s2RDTqo/cHwD8e3Kce8FJzxzkacXYMtf5akG x6yjge8u2A06zI1WXyrboG5fSr09zuA5dTz38pdaoZDL39h8E2k= =ODTr -----END PGP SIGNATURE----- --/HzgRVX5DKzcCCFE--