|SUCCESS| net/mlx5: fix counter TAILQ race between free and query callback

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/164965

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-04

Commit: (git sha not provided in patch file)
Author: Laaahu <lilinhu618 at gmail.com>
Subject: [PATCH] net/mlx5: fix counter TAILQ race between free and query callback

Summary:
This patch fixes a race condition in the mlx5 flow counter management code
where concurrent operations on TAILQ lists could lead to list corruption
and counter leaks. The race occurs between flow_dv_counter_free() and
mlx5_flow_async_pool_query_handle() when they operate on the same counter
list under different locks.

> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 915ea29a5a..20aad87f5d 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -9904,6 +9904,20 @@ mlx5_flow_async_pool_query_handle(struct mlx5_dev_ctx_shared *sh,
>  
>  	if (unlikely(status)) {
>  		raw_to_free = pool->raw_hw;
> +		/*
> +		 * The query failed, so the freed counters accumulated in the
> +		 * old-gen list during this round would otherwise be stranded.
> +		 * Move them back to the global free list to avoid a leak when
> +		 * queries fail persistently.
> +		 */
> +		if (!TAILQ_EMPTY(&pool->counters[query_gen])) {
> +			rte_spinlock_lock(&pool->csl);
> +			rte_spinlock_lock(&cmng->csl[cnt_type]);
> +			TAILQ_CONCAT(&cmng->counters[cnt_type],
> +				     &pool->counters[query_gen], next);
> +			rte_spinlock_unlock(&cmng->csl[cnt_type]);
> +			rte_spinlock_unlock(&pool->csl);
> +		}

In the error path above, does the code properly maintain query_gen state?
If the query failed but counters were moved back to the global list, does
any subsequent code path assume those counters are still in
pool->counters[query_gen]? After this TAILQ_CONCAT, is
pool->counters[query_gen] properly reinitialized?

>  	} else {
>  		raw_to_free = pool->raw;
>  		if (pool->is_aged)
> @@ -9913,11 +9927,20 @@ mlx5_flow_async_pool_query_handle(struct mlx5_dev_ctx_shared *sh,
>  		rte_spinlock_unlock(&pool->sl);
>  		/* Be sure the new raw counters data is updated in memory. */
>  		rte_io_wmb();
> +		/*
> +		 * A counter free thread may have read a stale query_gen
> +		 * before the generation was flipped and could still be
> +		 * inserting into this same old-gen list. Hold pool->csl to
> +		 * serialize TAILQ_CONCAT with that TAILQ_INSERT_TAIL and
> +		 * avoid corrupting the list.
> +		 */
>  		if (!TAILQ_EMPTY(&pool->counters[query_gen])) {
> +			rte_spinlock_lock(&pool->csl);
>  			rte_spinlock_lock(&cmng->csl[cnt_type]);
>  			TAILQ_CONCAT(&cmng->counters[cnt_type],
>  				     &pool->counters[query_gen], next);
>  			rte_spinlock_unlock(&cmng->csl[cnt_type]);
> +			rte_spinlock_unlock(&pool->csl);

The locking order here is pool->csl then cmng->csl[cnt_type]. In the
comment from the commit message, it states "Lock order is pool->csl then
cmng->csl, matching all other sites." Can you verify flow_dv_counter_free()
also follows this lock ordering? If flow_dv_counter_free() takes pool->csl
while another thread holds cmng->csl and then tries to acquire pool->csl
here, does this create a potential deadlock?

The memory barrier rte_io_wmb() is placed before acquiring the locks. Does
the barrier provide the intended synchronization guarantees when the
protected data is accessed after the locks are released by other threads?
Should the barrier be inside the lock-protected section?