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 256375A026F for ; Thu, 18 Jan 2024 02:15:23 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1705540519; bh=mT0e9ObGFZ0wa+pGq/Cwxf8Xh2XofwIAhQUSZLWDJBA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=LHhX6+N16FCTGKnQ/k3tpZ1AJ9GfG6u59iD8k/m7AiIkBO1Hy+gzw4CtBbwyoRuAU ZA+AuyvFBKXrwlRz9T5Ziv1+ZkFwAfm8MlldjxKwqgvO9z49CDJfFo9nllROm20QWO 53dNBRi+stwUpDCIpUn4aFBg/ek65wvCdMBMPUhEgFxjp6yWSwgGTkNHJqEXqojYzy xiiGTofuE+za96wlFjV7+xXG0ZN3DK5eQioMNDzFjtX0CHdxNRdwDmfBeBeBTUlzIz D13qI7CtnsrzquszBaIGNA+p45FQyo4gb0Rki+IBXKL0Mj4lcKcvHdPnBIOFL10fzs cwsRec13w6uzA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4TFlD7387hz4xNH; Thu, 18 Jan 2024 12:15:19 +1100 (AEDT) Date: Thu, 18 Jan 2024 12:15:12 +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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="GwGucQT3k5SHz1MR" Content-Disposition: inline In-Reply-To: <20240117205934.4b7d7043@elisabeth> Message-ID-Hash: 43LIOX4R7ZMGX66CD5SA2WY4A7SSIP5T X-Message-ID-Hash: 43LIOX4R7ZMGX66CD5SA2WY4A7SSIP5T 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: --GwGucQT3k5SHz1MR Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 addresses, or > > for multiple interfaces which means we may need to distinguish based 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 general fl= ows, > > 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 > Customary newline here. Done. > > + return siphash_final(&state, 38, (uint64_t)proto << 40 | > > + (uint64_t)fside->pif << 32 | > > + fside->fport << 16 | fside->eport); >=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'. 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 * we'd need to make sure that the the remaining (64-38 =3D=3D 26) bytes of that union are consistently initialised * the struct part would need to be packed, or padding will mess with us * the exact value would now depend on the host endianness, which is probably fine, but worth noting > > +} > > + > > /** > > * 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 struct fl= owside *fside) > > =20 > > #define SIDES 2 > > =20 > > +/** > > + * flowside_eq() - Check if two flowsides are equal >=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? > I guess it's useful iff we allow flowsides for the same flow to have > different protocol numbers. Right, which is not something I'm planning on doing, or at least not very soon. > 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. Exactly. > 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? 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. > Same here, I'm not proposing a concrete change here, I'm rather raising > this if you didn't think about it (assuming it makes any sense). >=20 > > + * @left, @right: Flowsides to compare > > + * > > + * Return: true if equal, false otherwise > > + */ > > +static inline bool flowside_eq(const struct flowside *left, > > + const struct flowside *right) > > +{ > > + return left->pif =3D=3D right->pif && > > + inany_equals(&left->eaddr, &right->eaddr) && > > + left->eport =3D=3D right->eport && > > + inany_equals(&left->faddr, &right->faddr) && > > + left->fport =3D=3D right->fport; >=20 > Preferably align under "return ". Done. --=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 --GwGucQT3k5SHz1MR Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmWoe58ACgkQzQJF27ox 2Gdc9hAAlo1Ik6fDf91wiMfXfGP7fowclqBz+aSqbK74EMvtWqpTWoPK5bctz4Zm kMHhiISDD/9IL8hBwYiHEl31jQ34fc9FXRuf+TNBMB+NyCe9UCGGpyBFGz9Be0r+ emRpAbo2aVjiLS0Wo9S+1T92JrbslbXbtE1x/Ozm0nmwv42fEyLja3K1TsShpg1+ UN+UdIyI+oWqcywgezGWYetmr5l6Ug2MW7Z8eAARGQF5uuKW6NAebe0LPU1PRpJL DiXgMpL2wW1/IbCkxATmoeaGmr/tZBYfcgIL5DHRn6ksZ2esbpHkqRzPz1to3A+i ELpFm0CAY4qjqWwz+xVuU9FU8lYN1Y73GokDPxUqOtYqCiA4rtTRykg2amnGb45l aG2HQJGP9Eko09Yqe5SAhyuNloYolbgKNUlpmRTwZIGVgQZ8nCZnmvscEOVfzFwV u2cmJx7fS7/pcxToX0SPLoPG84jl482CWmS/rM1OD4ylar8gGcbYABvYBwd5nol8 kpOkPmN1egJukXwtgMwPQV/BwHeTwuoUQTIefRMLn1AW3qYJ+A4w2ulPHVSQbt2c zbJFYNPjCj+LGhrS7dpXkXaBbxedvY4wIjd31x329RKAUPX8u2JxfLlJOoDE571r vUlhAYUk5uZ7lU+kknrykGxRuUYw1kQ/rG7TD3PpgqsKJLNL8ZM= =5ghl -----END PGP SIGNATURE----- --GwGucQT3k5SHz1MR--