From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 976325A026D for ; Tue, 26 Sep 2023 09:06:50 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1695712007; bh=x68hLFH3IRU39NGSA1j57KRAu22P5jLmAmDb1MuyvTE=; h=Date:From:To:Subject:References:In-Reply-To:From; b=dm6e+yRGKMZHBxsl5gqn+LKH+Aj5osetYaVmOC+xX+zcqwzdeMdDyYCEfFwAA/8/g gBlgeN8wG0oJCWxwxSO0pxZHS8uerYg4iD5OweaVL4niM+KQe7LMdCd5FaWIu9Wxnm ceCC1PB7TMXkMdg9RJPzBasPjPsVUznWDDBV4x/4= Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4RvrQH0hMwz4xNh; Tue, 26 Sep 2023 17:06:47 +1000 (AEST) Date: Tue, 26 Sep 2023 17:02:19 +1000 From: David Gibson To: Stefano Brivio , passt-dev@passt.top Subject: Re: [PATCH 10/10] siphash: Use incremental rather than all-at-once siphash functions Message-ID: References: <20230922140630.3184256-1-david@gibson.dropbear.id.au> <20230922140630.3184256-11-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="bTpOZ8avh+yDS22n" Content-Disposition: inline In-Reply-To: Message-ID-Hash: UD2FJVG7YOOYEKYGGRPRG3POVXKFJ76G X-Message-ID-Hash: UD2FJVG7YOOYEKYGGRPRG3POVXKFJ76G 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 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: --bTpOZ8avh+yDS22n Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Sep 26, 2023 at 04:23:45PM +1000, David Gibson wrote: > On Sat, Sep 23, 2023 at 12:06:30AM +1000, David Gibson wrote: > > We have a bunch of variants of the siphash functions for different data > > sizes. The callers, in tcp.c, need to pack the various values they wan= t to > > hash into a temporary structure, then call the appropriate version. We= can > > avoid the copy into the temporary by directly using the incremental > > siphash functions. > >=20 > > The length specific hash functions also have an undocumented constraint > > that the data pointer they take must, in fact, be aligned to avoid > > unaligned accesses, which may cause crashes on some architectures. > >=20 > > So, prefer the incremental approach and remove the length-specific > > functions. > >=20 > > Signed-off-by: David Gibson > > --- > > Makefile | 2 +- > > inany.h | 16 ++++++- > > siphash.c | 121 --------------------------------------------------- > > tcp.c | 32 +++++--------- > > tcp_splice.c | 1 + > > 5 files changed, 27 insertions(+), 145 deletions(-) > > delete mode 100644 siphash.c > >=20 > > diff --git a/Makefile b/Makefile > > index 4435bd6..ec3c3fb 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -45,7 +45,7 @@ FLAGS +=3D -DDUAL_STACK_SOCKETS=3D$(DUAL_STACK_SOCKET= S) > > =20 > > PASST_SRCS =3D arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c icmp.c i= gmp.c \ > > isolation.c lineread.c log.c mld.c ndp.c netlink.c packet.c passt.c \ > > - pasta.c pcap.c siphash.c tap.c tcp.c tcp_splice.c udp.c util.c > > + pasta.c pcap.c tap.c tcp.c tcp_splice.c udp.c util.c > > QRAP_SRCS =3D qrap.c > > SRCS =3D $(PASST_SRCS) $(QRAP_SRCS) > > =20 > > diff --git a/inany.h b/inany.h > > index aadb20b..266d101 100644 > > --- a/inany.h > > +++ b/inany.h > > @@ -14,8 +14,9 @@ > > * @v4mapped.zero: All zero-bits for an IPv4 address > > * @v4mapped.one: All one-bits for an IPv4 address > > * @v4mapped.a4: If @a6 is an IPv4 mapped address, the IPv4 address > > + * @u64: As an array of u64s (solely for hashing) > > * > > - * @v4mapped shouldn't be accessed except via helpers. > > + * @v4mapped and @u64 shouldn't be accessed except via helpers. > > */ > > union inany_addr { > > struct in6_addr a6; > > @@ -24,7 +25,9 @@ union inany_addr { > > uint8_t one[2]; > > struct in_addr a4; > > } v4mapped; > > + uint64_t u64[2]; >=20 > I realised this change alters the alignment of inany from 4 bytes to 8 > bytes, which causes problems for things I have in the works. Revised > version coming. Actually, I might as well wait for any comments you have on v1, before folding that into v2. >=20 > > }; > > +static_assert(sizeof(union inany_addr) =3D=3D sizeof(struct in6_addr)); > > =20 > > /** inany_v4 - Extract IPv4 address, if present, from IPv[46] address > > * @addr: IPv4 or IPv6 address > > @@ -94,4 +97,15 @@ static inline void inany_from_sockaddr(union inany_a= ddr *aa, in_port_t *port, > > } > > } > > =20 > > +/** inany_siphash_feed- Fold IPv[46] address into an in-progress sipha= sh > > + * @state: siphash state > > + * @aa: inany to hash > > + */ > > +static inline void inany_siphash_feed(struct siphash_state *state, > > + const union inany_addr *aa) > > +{ > > + siphash_feed(state, aa->u64[0]); > > + siphash_feed(state, aa->u64[1]); > > +} > > + > > #endif /* INANY_H */ > > diff --git a/siphash.c b/siphash.c > > deleted file mode 100644 > > index d2b068c..0000000 > > --- a/siphash.c > > +++ /dev/null > > @@ -1,121 +0,0 @@ > > -// SPDX-License-Identifier: GPL-2.0-or-later > > - > > -/* PASST - Plug A Simple Socket Transport > > - * for qemu/UNIX domain socket mode > > - * > > - * PASTA - Pack A Subtle Tap Abstraction > > - * for network namespace/tap device mode > > - * > > - * siphash.c - SipHash routines > > - * > > - * Copyright (c) 2020-2021 Red Hat GmbH > > - * Author: Stefano Brivio > > - */ > > - > > -#include > > -#include > > - > > -#include "siphash.h" > > - > > -/** > > - * siphash_8b() - Table index or timestamp offset for TCP over IPv4 (8= bytes in) > > - * @in: Input data (remote address and two ports, or two addresses) > > - * @k: Hash function key, 128 bits > > - * > > - * Return: the 64-bit hash output > > - */ > > -/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */ > > -__attribute__((optimize("-fno-strict-aliasing"))) /* See csum_16b() */ > > -/* cppcheck-suppress unusedFunction */ > > -uint64_t siphash_8b(const uint8_t *in, const uint64_t *k) > > -{ > > - struct siphash_state state =3D SIPHASH_INIT(k); > > - > > - siphash_feed(&state, *(uint64_t *)in); > > - > > - return siphash_final(&state, 8, 0); > > -} > > - > > -/** > > - * siphash_12b() - Initial sequence number for TCP over IPv4 (12 bytes= in) > > - * @in: Input data (two addresses, two ports) > > - * @k: Hash function key, 128 bits > > - * > > - * Return: the 64-bit hash output > > - */ > > -/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */ > > -__attribute__((optimize("-fno-strict-aliasing"))) /* See csum_16b() */ > > -/* cppcheck-suppress unusedFunction */ > > -uint64_t siphash_12b(const uint8_t *in, const uint64_t *k) > > -{ > > - struct siphash_state state =3D SIPHASH_INIT(k); > > - uint32_t *in32 =3D (uint32_t *)in; > > - > > - siphash_feed(&state, (uint64_t)(*(in32 + 1)) << 32 | *in32); > > - > > - return siphash_final(&state, 12, *(in32 + 2)); > > -} > > - > > -/** > > - * siphash_20b() - Table index for TCP over IPv6 (20 bytes in) > > - * @in: Input data (remote address, two ports) > > - * @k: Hash function key, 128 bits > > - * > > - * Return: the 64-bit hash output > > - */ > > -/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */ > > -__attribute__((optimize("-fno-strict-aliasing"))) /* See csum_16b() */ > > -uint64_t siphash_20b(const uint8_t *in, const uint64_t *k) > > -{ > > - struct siphash_state state =3D SIPHASH_INIT(k); > > - uint32_t *in32 =3D (uint32_t *)in; > > - int i; > > - > > - for (i =3D 0; i < 2; i++, in32 +=3D 2) > > - siphash_feed(&state, (uint64_t)(*(in32 + 1)) << 32 | *in32); > > - > > - return siphash_final(&state, 20, *in32); > > -} > > - > > -/** > > - * siphash_32b() - Timestamp offset for TCP over IPv6 (32 bytes in) > > - * @in: Input data (two addresses) > > - * @k: Hash function key, 128 bits > > - * > > - * Return: the 64-bit hash output > > - */ > > -/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */ > > -__attribute__((optimize("-fno-strict-aliasing"))) /* See csum_16b() */ > > -/* cppcheck-suppress unusedFunction */ > > -uint64_t siphash_32b(const uint8_t *in, const uint64_t *k) > > -{ > > - struct siphash_state state =3D SIPHASH_INIT(k); > > - uint64_t *in64 =3D (uint64_t *)in; > > - int i; > > - > > - for (i =3D 0; i < 4; i++, in64++) > > - siphash_feed(&state, *in64); > > - > > - return siphash_final(&state, 32, 0); > > -} > > - > > -/** > > - * siphash_36b() - Initial sequence number for TCP over IPv6 (36 bytes= in) > > - * @in: Input data (two addresses, two ports) > > - * @k: Hash function key, 128 bits > > - * > > - * Return: the 64-bit hash output > > - */ > > -/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */ > > -__attribute__((optimize("-fno-strict-aliasing"))) /* See csum_16b() */ > > -uint64_t siphash_36b(const uint8_t *in, const uint64_t *k) > > -{ > > - struct siphash_state state =3D SIPHASH_INIT(k); > > - uint32_t *in32 =3D (uint32_t *)in; > > - int i; > > - > > - for (i =3D 0; i < 4; i++, in32 +=3D 2) > > - siphash_feed(&state, (uint64_t)(*(in32 + 1)) << 32 | *in32); > > - > > - return siphash_final(&state, 36, *in32); > > -} > > diff --git a/tcp.c b/tcp.c > > index 9f28020..18ceed1 100644 > > --- a/tcp.c > > +++ b/tcp.c > > @@ -1165,18 +1165,13 @@ static int tcp_hash_match(const struct tcp_tap_= conn *conn, > > static unsigned int tcp_hash(const struct ctx *c, const union inany_ad= dr *faddr, > > in_port_t eport, in_port_t fport) > > { > > - struct { > > - union inany_addr faddr; > > - in_port_t eport; > > - in_port_t fport; > > - } __attribute__((__packed__)) in =3D { > > - *faddr, eport, fport > > - }; > > - uint64_t b =3D 0; > > + struct siphash_state state =3D SIPHASH_INIT(c->tcp.hash_secret); > > + uint64_t hash; > > =20 > > - b =3D siphash_20b((uint8_t *)&in, c->tcp.hash_secret); > > + inany_siphash_feed(&state, faddr); > > + hash =3D siphash_final(&state, 20, (uint64_t)eport << 16 | fport); > > =20 > > - return (unsigned int)(b % TCP_HASH_TABLE_SIZE); > > + return (unsigned int)(hash % TCP_HASH_TABLE_SIZE); > > } > > =20 > > /** > > @@ -1815,17 +1810,8 @@ static void tcp_clamp_window(const struct ctx *c= , struct tcp_tap_conn *conn, > > static void tcp_seq_init(const struct ctx *c, struct tcp_tap_conn *con= n, > > const struct timespec *now) > > { > > + struct siphash_state state =3D SIPHASH_INIT(c->tcp.hash_secret); > > union inany_addr aany; > > - struct { > > - union inany_addr src; > > - in_port_t srcport; > > - union inany_addr dst; > > - in_port_t dstport; > > - } __attribute__((__packed__)) in =3D { > > - .src =3D conn->faddr, > > - .srcport =3D conn->fport, > > - .dstport =3D conn->eport, > > - }; > > uint64_t hash; > > uint32_t ns; > > =20 > > @@ -1833,9 +1819,11 @@ static void tcp_seq_init(const struct ctx *c, st= ruct tcp_tap_conn *conn, > > inany_from_af(&aany, AF_INET, &c->ip4.addr); > > else > > inany_from_af(&aany, AF_INET6, &c->ip6.addr); > > - in.dst =3D aany; > > =20 > > - hash =3D siphash_36b((uint8_t *)&in, c->tcp.hash_secret); > > + inany_siphash_feed(&state, &conn->faddr); > > + inany_siphash_feed(&state, &aany); > > + hash =3D siphash_final(&state, 36, > > + (uint64_t)conn->fport << 16 | conn->eport); > > =20 > > /* 32ns ticks, overflows 32 bits every 137s */ > > ns =3D (now->tv_sec * 1000000000 + now->tv_nsec) >> 5; > > diff --git a/tcp_splice.c b/tcp_splice.c > > index 5b36975..3b98260 100644 > > --- a/tcp_splice.c > > +++ b/tcp_splice.c > > @@ -52,6 +52,7 @@ > > #include "passt.h" > > #include "log.h" > > #include "tcp_splice.h" > > +#include "siphash.h" > > #include "inany.h" > > =20 > > #include "tcp_conn.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 --bTpOZ8avh+yDS22n Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmUSgeEACgkQzQJF27ox 2GfQew//VFvMiut+bv6194yADw47fNUO7pIUPn+D3UOrzA4hxtxf95B5lIJa/5Tw C45HtuOkVks32AUB2zYYKGiper30ITHgWgAX0L8kygtu8TWdT5X5O/2G/twRWBmc h6zstSBkxMm4nvJ+BJ/shPl7jVwCU5M5GE8aBml1A2K3oaarXQvoc8IQo+ttq0VR QDWG1/zyk2tFjEXDIcT+LsBZhen55mwU/tyAZU6PxbTe+ZyBq1Md5sizkq9FnvrV i+HXFrw8UILS+yTziq+4/n015lQJBTnJbuX+zn8G010JvVKDojFl5mNBJmGmQYE4 iYgXwGLoRYJU0VuXtnfbSTbIqpYnz2PC7PX2HNWFzR/U6H3Zu/x1or1A4IsN74Gg p06qdoJ4XXjLEVSMBzGF1TpgdHBnWngPBKRJKX3JEOXmLfeeKWBcvbtCp1XI2daj UltlzXQ41CL8jOlw8JdueaRBHAgHAL83yemu0/jpj3KMDPm2cCeA4Pn9jyJqnAWo 6B3nxR+i0BDEZ1Mu1sO3XHcbInQFxKC+4XbS16WJc+JyJSGOWteJxKMFio9N/LSp NRTGIIMvTDE405QqfFN+ARvPbF4T7m6M1ueTXPF1Ick6odKx0oDAn9Afk2VqncCI VZ5MMJhWkIT4Kb2ACC+bE/+4Eo49GyAYhVeHzzBJSjE+48sO9h4= =UrC0 -----END PGP SIGNATURE----- --bTpOZ8avh+yDS22n--