Re: [PATCH v2 03/10] flow, tcp: Consolidate flow pointer<->index helpers

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

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

On Thu, Sep 07, 2023 at 03:01:21AM +0200, Stefano Brivio wrote:
> On Mon, 28 Aug 2023 15:41:39 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Both tcp.c and tcp_splice.c define CONN_IDX() variants to find the index
> > of their connection structures in the connection table, now become the
> > unified flow table.  We can easily combine these into a common helper.
> > While we're there, add some trickery for some additional type safety.
> > 
> > They also define their own CONN() versions, which aren't so easily combined
> > since they need to return different types, but we can have them use a
> > common helper.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  flow_table.h | 20 ++++++++++++++++++++
> >  tcp.c        | 49 ++++++++++++++++++++++++-------------------------
> >  tcp_conn.h   |  2 +-
> >  tcp_splice.c | 17 ++++++++---------
> >  4 files changed, 53 insertions(+), 35 deletions(-)
> > 
> > diff --git a/flow_table.h b/flow_table.h
> > index c4c646b..dd4875e 100644
> > --- a/flow_table.h
> > +++ b/flow_table.h
> > @@ -22,4 +22,24 @@ union flow {
> >  /* Global Flow Table */
> >  extern union flow flowtab[];
> >  
> > +
> > +/** flowk_idx - Index of flow from common structure
> 
> "flowk"

Oops, fixed.

> > + * @f:	Common flow fields pointer
> > + *
> > + * Return: index of @f in the flow table
> > + */
> > +static inline unsigned flow_idx(const struct flow_common *f)
> > +{
> > +	return (union flow *)f - flowtab;
> > +}
> > +
> > +/** FLOW_IDX - Find the index of a flow
> > + * @f_:	Flow pointer, either union flow * or protocol specific
> > + *
> > + * Return: index of @f in the flow table
> > + */
> > +#define FLOW_IDX(f_)		(flow_idx(&(f_)->f))
> > +
> > +#define FLOW(index)		(&flowtab[(index)])
> > +
> >  #endif /* FLOW_TABLE_H */
> > diff --git a/tcp.c b/tcp.c
> > index 7994197..7d2e89d 100644
> > --- a/tcp.c
> > +++ b/tcp.c
> > @@ -561,8 +561,7 @@ tcp6_l2_flags_buf[TCP_FRAMES_MEM];
> >  
> >  static unsigned int tcp6_l2_flags_buf_used;
> >  
> > -#define CONN(index)		(&flowtab[(index)].tcp)
> > -#define CONN_IDX(conn)		((union flow *)(conn) - flowtab)
> > +#define CONN(index)		(&FLOW(index)->tcp)
> 
> Extra parentheses are not needed, but I've been wondering for a bit why
> you would use "&FLOW" here, as FLOW(x) already resolves to &flowtab[x]...
> perhaps (&(FLOW(index)->tcp)) is actually easier to read?

Makes sense to me, 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 --]