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

[Stefano Brivio <sbrivio@redhat.com>]

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, and we'll get warnings if
cppcheck's behaviour ever changes again.

-- 
Stefano