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 want 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. > > > > 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. > > > > So, prefer the incremental approach and remove the length-specific > > functions. > > > > 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 > > > > diff --git a/Makefile b/Makefile > > index 4435bd6..ec3c3fb 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -45,7 +45,7 @@ FLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS) > > > > PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c icmp.c igmp.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 = qrap.c > > SRCS = $(PASST_SRCS) $(QRAP_SRCS) > > > > 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]; > > 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. > > > }; > > +static_assert(sizeof(union inany_addr) == sizeof(struct in6_addr)); > > > > /** 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_addr *aa, in_port_t *port, > > } > > } > > > > +/** inany_siphash_feed- Fold IPv[46] address into an in-progress siphash > > + * @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 = 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 = SIPHASH_INIT(k); > > - uint32_t *in32 = (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 = SIPHASH_INIT(k); > > - uint32_t *in32 = (uint32_t *)in; > > - int i; > > - > > - for (i = 0; i < 2; i++, in32 += 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 = SIPHASH_INIT(k); > > - uint64_t *in64 = (uint64_t *)in; > > - int i; > > - > > - for (i = 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 = SIPHASH_INIT(k); > > - uint32_t *in32 = (uint32_t *)in; > > - int i; > > - > > - for (i = 0; i < 4; i++, in32 += 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_addr *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 = { > > - *faddr, eport, fport > > - }; > > - uint64_t b = 0; > > + struct siphash_state state = SIPHASH_INIT(c->tcp.hash_secret); > > + uint64_t hash; > > > > - b = siphash_20b((uint8_t *)&in, c->tcp.hash_secret); > > + inany_siphash_feed(&state, faddr); > > + hash = siphash_final(&state, 20, (uint64_t)eport << 16 | fport); > > > > - return (unsigned int)(b % TCP_HASH_TABLE_SIZE); > > + return (unsigned int)(hash % TCP_HASH_TABLE_SIZE); > > } > > > > /** > > @@ -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 *conn, > > const struct timespec *now) > > { > > + struct siphash_state state = 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 = { > > - .src = conn->faddr, > > - .srcport = conn->fport, > > - .dstport = conn->eport, > > - }; > > uint64_t hash; > > uint32_t ns; > > > > @@ -1833,9 +1819,11 @@ static void tcp_seq_init(const struct ctx *c, struct 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 = aany; > > > > - hash = siphash_36b((uint8_t *)&in, c->tcp.hash_secret); > > + inany_siphash_feed(&state, &conn->faddr); > > + inany_siphash_feed(&state, &aany); > > + hash = siphash_final(&state, 36, > > + (uint64_t)conn->fport << 16 | conn->eport); > > > > /* 32ns ticks, overflows 32 bits every 137s */ > > ns = (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" > > > > #include "tcp_conn.h" > -- 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