Re: [PATCH v2 05/10] flow: Make flow timer per-caller for thread safety

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

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

On Fri, Jul 31, 2026 at 06:23:24PM +0200, Laurent Vivier wrote:
> Move the static flow_timer_run variable out of flow.c and pass it as a
> parameter to flow_defer_handler().  This allows each caller to maintain
> its own timer state: each vhost-user queue pair worker uses the per-qpair
> context.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

One small misgiving about this, although I doubt it can cause a real
problem.  Becayse this timer processing now doesn't happen all at
once, at each queue is on a separate timer cycle, it's theoretically
possible for a flow to change qpair every FLOW_TIMER_INTERVAL and
thereby indefinitely avoid having the timer running on it.

It's pretty tricky to do (depending on how close in real time each
queue's timers end up running), and I don't *think* that can do
anything terribly bad (maybe delay cleanup).  But since the guest
could in principle control the qpair placements to specifically
trigger this, it just makes me think it warrants a good close look to
make sure that something bad can't be triggered that way.

> ---
>  flow.c  | 10 ++++------
>  flow.h  |  2 +-
>  passt.c |  8 +++++---
>  3 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/flow.c b/flow.c
> index 1efa9b966991..59963ea5b1c2 100644
> --- a/flow.c
> +++ b/flow.c
> @@ -142,9 +142,6 @@ static flow_sidx_t flow_hashtab[FLOW_HASH_SIZE];
>  static_assert(ARRAY_SIZE(flow_hashtab) >= 2 * FLOW_MAX,
>  "Safe linear probing requires hash table with more entries than the number of sides in the flow table");
>  
> -/* Last time the flow timers ran */
> -static struct timespec flow_timer_run;
> -
>  /** flowside_from_af() - Initialise flowside from addresses
>   * @side:	flowside to initialise
>   * @af:		Address family (AF_INET or AF_INET6)
> @@ -875,10 +872,11 @@ flow_sidx_t flow_lookup_sa(const struct ctx *c, uint8_t proto, uint8_t pif,
>   * flow_defer_handler() - Handler for per-flow deferred and timed tasks
>   * @c:		Execution context
>   * @now:	Current timestamp
> + * @timer_run:	Last time the flow timers ran
>   * @qpair:	Queue pair to process
>   */
>  void flow_defer_handler(const struct ctx *c, const struct timespec *now,
> -			unsigned int qpair)
> +			struct timespec *timer_run, unsigned int qpair)
>  {
>  	struct flow_free_cluster *free_head = NULL;
>  	unsigned *last_next = &flow_first_free;
> @@ -886,9 +884,9 @@ void flow_defer_handler(const struct ctx *c, const struct timespec *now,
>  	bool timer = false;
>  	union flow *flow;
>  
> -	if (timespec_diff_ms(now, &flow_timer_run) >= FLOW_TIMER_INTERVAL) {
> +	if (timespec_diff_ms(now, timer_run) >= FLOW_TIMER_INTERVAL) {
>  		timer = true;
> -		flow_timer_run = *now;
> +		*timer_run = *now;
>  	}
>  
>  	assert(!flow_new_entry); /* Incomplete flow at end of cycle */
> diff --git a/flow.h b/flow.h
> index 028d42bccc0c..ac1d3897ca98 100644
> --- a/flow.h
> +++ b/flow.h
> @@ -272,7 +272,7 @@ int flow_epoll_set(const struct flow_common *f, int command, uint32_t events,
>  		   int fd, unsigned int sidei);
>  
>  void flow_defer_handler(const struct ctx *c, const struct timespec *now,
> -			unsigned int qpair);
> +			struct timespec *timer_run, unsigned int qpair);
>  int flow_migrate_source_early(struct ctx *c, const struct migrate_stage *stage,
>  			      int fd);
>  int flow_migrate_source_pre(struct ctx *c, const struct migrate_stage *stage,
> diff --git a/passt.c b/passt.c
> index bc9f1575ef8c..69fd7b12450a 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -108,15 +108,16 @@ struct passt_stats {
>   * post_handler() - Run periodic and deferred tasks for L4 protocol handlers
>   * @c:		Execution context
>   * @now:	Current timestamp
> + * @timer_run:	Last time the flow timers ran
>   * @qpair:	Queue pair to process
>   */
>  static void post_handler(struct ctx *c, const struct timespec *now,
> -			 unsigned int qpair)
> +			 struct timespec *timer_run, unsigned int qpair)
>  {
>  	if (!c->no_tcp)
>  		tcp_defer_handler(c, now, qpair);
>  
> -	flow_defer_handler(c, now, qpair);
> +	flow_defer_handler(c, now, timer_run, qpair);
>  	fwd_scan_ports_timer(c, now);
>  
>  	if (!c->no_ndp)
> @@ -231,6 +232,7 @@ static void print_stats(const struct ctx *c, const struct passt_stats *stats,
>  static void passt_worker(void *opaque, int nfds, struct epoll_event *events)
>  {
>  	static struct passt_stats stats = { 0 };
> +	static struct timespec flow_timer_run;
>  	struct ctx *c = opaque;
>  	struct timespec now;
>  	int i;
> @@ -314,7 +316,7 @@ static void passt_worker(void *opaque, int nfds, struct epoll_event *events)
>  		print_stats(c, &stats, &now);
>  	}
>  
> -	post_handler(c, &now, QPAIR_DEFAULT);
> +	post_handler(c, &now, &flow_timer_run, QPAIR_DEFAULT);
>  
>  	migrate_handler(c, &now);
>  }
> -- 
> 2.54.0
> 

-- 
David Gibson (he or they)	| 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 --]