Re: [PATCH v3 05/13] flow, tcp: Add flow-centric dispatch for deferred flow handling

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

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

On Tue, Jan 02, 2024 at 07:13:28PM +0100, Stefano Brivio wrote:
> On Sun, 31 Dec 2023 16:56:30 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Dec 28, 2023 at 07:24:46PM +0100, Stefano Brivio wrote:
> > > On Thu, 21 Dec 2023 17:15:41 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > tcp_defer_handler(), amongst other things, scans the flow table and does
> > > > some processing for each TCP connection.  When we add other protocols to
> > > > the flow table, they're likely to want some similar scanning.  It makes
> > > > more sense for cache friendliness to perform a single scan of the flow
> > > > table and dispatch to the protocol specific handlers, rather than having
> > > > each protocol separately scan the table.
> > > > 
> > > > To that end, add a new flow_defer_handler() handling all flow-linked
> > > > deferred operations.
> > > > 
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > ---
> > > >  flow.c     | 23 +++++++++++++++++++++++
> > > >  flow.h     |  1 +
> > > >  passt.c    |  1 +
> > > >  tcp.c      | 19 ++-----------------
> > > >  tcp_conn.h |  1 +
> > > >  5 files changed, 28 insertions(+), 17 deletions(-)
> > > > 
> > > > diff --git a/flow.c b/flow.c
> > > > index a1c0a34..0a0402d 100644
> > > > --- a/flow.c
> > > > +++ b/flow.c
> > > > @@ -83,3 +83,26 @@ void flow_log_(const struct flow_common *f, int pri, const char *fmt, ...)
> > > >  
> > > >  	logmsg(pri, "Flow %u (%s): %s", flow_idx(f), FLOW_TYPE(f), msg);
> > > >  }
> > > > +
> > > > +/**
> > > > + * flow_defer_handler() - Handler for per-flow deferred tasks
> > > > + * @c:		Execution context
> > > > + */
> > > > +void flow_defer_handler(struct ctx *c)
> > > > +{
> > > > +	union flow *flow;
> > > > +
> > > > +	for (flow = flowtab + c->flow_count - 1; flow >= flowtab; flow--) {
> > > > +		switch (flow->f.type) {
> > > > +		case FLOW_TCP:
> > > > +			tcp_flow_defer(c, flow);
> > > > +			break;
> > > > +		case FLOW_TCP_SPLICE:
> > > > +			tcp_splice_flow_defer(c, flow);
> > > > +			break;
> > > > +		default:
> > > > +			/* Assume other flow types don't need any handling */
> > > > +			;
> > > > +		}
> > > > +	}
> > > > +}
> > > > diff --git a/flow.h b/flow.h
> > > > index 959b461..6b17fa8 100644
> > > > --- a/flow.h
> > > > +++ b/flow.h
> > > > @@ -67,6 +67,7 @@ static inline bool flow_sidx_eq(flow_sidx_t a, flow_sidx_t b)
> > > >  union flow;
> > > >  
> > > >  void flow_table_compact(struct ctx *c, union flow *hole);
> > > > +void flow_defer_handler(struct ctx *c);
> > > >  
> > > >  void flow_log_(const struct flow_common *f, int pri, const char *fmt, ...)
> > > >  	__attribute__((format(printf, 3, 4)));
> > > > diff --git a/passt.c b/passt.c
> > > > index 0246b04..5f72a28 100644
> > > > --- a/passt.c
> > > > +++ b/passt.c
> > > > @@ -103,6 +103,7 @@ static void post_handler(struct ctx *c, const struct timespec *now)
> > > >  	/* NOLINTNEXTLINE(bugprone-branch-clone): intervals can be the same */
> > > >  	CALL_PROTO_HANDLER(c, now, icmp, ICMP);
> > > >  
> > > > +	flow_defer_handler(c);
> > > >  #undef CALL_PROTO_HANDLER
> > > >  }
> > > >  
> > > > diff --git a/tcp.c b/tcp.c
> > > > index ad1a70d..9230d80 100644
> > > > --- a/tcp.c
> > > > +++ b/tcp.c
> > > > @@ -1306,7 +1306,7 @@ static struct tcp_tap_conn *tcp_hash_lookup(const struct ctx *c,
> > > >   * @c:		Execution context
> > > >   * @flow:	Flow table entry for this connection
> > > >   */
> > > > -static void tcp_flow_defer(struct ctx *c, union flow *flow)
> > > > +void tcp_flow_defer(struct ctx *c, union flow *flow)
> > > >  {
> > > >  	const struct tcp_tap_conn *conn = &flow->tcp;
> > > >  
> > > > @@ -1364,26 +1364,11 @@ static void tcp_l2_data_buf_flush(const struct ctx *c)
> > > >   * tcp_defer_handler() - Handler for TCP deferred tasks
> > > >   * @c:		Execution context
> > > >   */
> > > > +/* cppcheck-suppress constParameterPointer */  
> > > 
> > > This needs to be:
> > > 
> > >   /* cppcheck-suppress [constParameterPointer, unmatchedSuppression] */
> > > 
> > > otherwise we get warnings with cppcheck 2.10,  
> > 
> > Drat, do we?  I was hoping this was a new warning type with the newer
> > cppcheck, and it would ignore the suppression if it was for a warning
> > type it didn't know about.
> 
> Yeah... go figure. On the other hand it's not really a new type in the
> sense that this _should_ have been covered by a "constParameter"
> warning, before 2.11.

Dang.  Oh well, updated.

> > > and we'll get warnings if
> > > cppcheck's behaviour ever changes again.  
> > 
> > That's actually a good thing.  This one isn't a workaround for a
> > cppcheck false positive or weird semantic that we hope will go away.
> > Rhe warning is real and correct as far as it goes.  The problem is
> > that the signature needs to match that of other deferred handlers
> > because of how we generate the calls from a macro.  Some of those
> > others need write access to the context.
> 
> Oops, I didn't realise. Well, in any case, then we don't expect
> cppcheck's behaviour to ever change in this regard, so I don't see any
> advantage omitting unmatchedSuppression here.
> 

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