Re: [PATCH v3 06/15] tcp, flow: Replace TCP specific hash function with general flow hash

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5117 bytes --]

On Wed, Jan 17, 2024 at 08:59:34PM +0100, Stefano Brivio wrote:
> On Thu, 21 Dec 2023 18:02:28 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > 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.
> > 
> > Replace the TCP specific hash function with one suitable for general flows,
> > or rather for one side of a general flow.  This includes all the
> > information from struct flowside, plus the L4 protocol number.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  flow.c | 21 +++++++++++++++++++++
> >  flow.h | 19 +++++++++++++++++++
> >  tcp.c  | 59 +++++++++++-----------------------------------------------
> >  3 files changed, 51 insertions(+), 48 deletions(-)
> > 
> > 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 = FLOW_IDX(flow);
> >  }
> >  
> > +/**
> > + * 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 = SIPHASH_INIT(c->hash_secret);
> > +
> > +	ASSERT(flowside_complete(fside));
> > +	inany_siphash_feed(&state, &fside->faddr);
> > +	inany_siphash_feed(&state, &fside->eaddr);
> 
> Customary newline here.

Done.

> > +	return siphash_final(&state, 38, (uint64_t)proto << 40 |
> > +			     (uint64_t)fside->pif << 32 |
> > +			     fside->fport << 16 | fside->eport);
> 
> 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 == 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 flowside *fside)
> >  
> >  #define SIDES			2
> >  
> > +/**
> > + * flowside_eq() - Check if two flowsides are equal
> 
> ...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).
> 
> > + * @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 == right->pif &&
> > +		inany_equals(&left->eaddr, &right->eaddr) &&
> > +		left->eport == right->eport &&
> > +		inany_equals(&left->faddr, &right->faddr) &&
> > +		left->fport == right->fport;
> 
> Preferably align under "return ".

Done.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]