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

[Stefano Brivio <sbrivio@redhat.com>]

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.

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

-- 
Stefano