Re: [PATCH 10/10] siphash: Use incremental rather than all-at-once siphash functions

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 26 Sep 2023 17:02:19 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> 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 <david@gibson.dropbear.id.au>
> > > ---
> > >  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.

Nothing else from my side.

-- 
Stefano