Re: [PATCH dpdk v2] fib6: implement RCU rule reclamation

["Medvedkin, Vladimir" <vladimir.medvedkin@intel.com>]

Hi Robin,

LGTM, with a few minor notes:

- could you please update release notes?

- see the rest inline

On 10/15/2025 10:53 AM, Robin Jarry wrote:
> Currently, for the TRIE algorithm, the tbl8 group is freed even though
> the readers might be using the tbl8 group entries. The freed tbl8 group
> can be reallocated quickly. As a result, lookup may be performed
> incorrectly.
>
> To address that, RCU QSBR is integrated for safe tbl8 group reclamation.
>
> Cc: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
> Signed-off-by: Robin Jarry <rjarry@redhat.com>
> ---
>
> Notes:
>      v2:
>      
>      - Fixed typos in docstrings.
>      - Tests depend on: http://patches.dpdk.org/project/dpdk/list/?series=36356
>
>   app/test/test_fib6.c | 214 +++++++++++++++++++++++++++++++++++++++++++
>   lib/fib/rte_fib6.c   |  15 +++
>   lib/fib/rte_fib6.h   |  52 +++++++++++
>   lib/fib/trie.c       |  89 ++++++++++++++++--
>   lib/fib/trie.h       |   8 ++
>   5 files changed, 371 insertions(+), 7 deletions(-)
<snip>
> diff --git a/lib/fib/rte_fib6.c b/lib/fib/rte_fib6.c
> index 93a1c7197b26..dffcbdf657e8 100644
> --- a/lib/fib/rte_fib6.c
> +++ b/lib/fib/rte_fib6.c
> @@ -346,3 +346,18 @@ rte_fib6_select_lookup(struct rte_fib6 *fib,
>   		return -EINVAL;
>   	}
>   }
> +
> +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_fib6_rcu_qsbr_add, 25.07)
25.11?
> +int
> +rte_fib6_rcu_qsbr_add(struct rte_fib6 *fib, struct rte_fib6_rcu_config *cfg)
> +{
> +	if (fib == NULL || cfg == NULL)
> +		return -EINVAL;
> +
> +	switch (fib->type) {
> +	case RTE_FIB6_TRIE:
> +		return trie_rcu_qsbr_add(fib->dp, cfg, fib->name);
> +	default:
> +		return -ENOTSUP;
> +	}
> +}
<snip>
> +int
> +trie_rcu_qsbr_add(struct rte_trie_tbl *dp, struct rte_fib6_rcu_config *cfg,
> +	const char *name)
> +{
> +	struct rte_rcu_qsbr_dq_parameters params = {0};
> +	char rcu_dq_name[RTE_RCU_QSBR_DQ_NAMESIZE];
> +
> +	if (dp == NULL || cfg == NULL)
> +		return -EINVAL;
> +
> +	if (dp->v != NULL)
> +		return -EEXIST;
> +
> +	switch (cfg->mode) {
> +	case RTE_FIB6_QSBR_MODE_DQ:
> +		/* Init QSBR defer queue. */
> +		snprintf(rcu_dq_name, sizeof(rcu_dq_name),
> +			"FIB_RCU_%s", name);
> +		params.name = rcu_dq_name;
> +		params.size = cfg->dq_size;
> +		if (params.size == 0)
> +			params.size = RTE_FIB6_RCU_DQ_RECLAIM_SZ;
> +		params.trigger_reclaim_limit = cfg->reclaim_thd;
> +		params.max_reclaim_size = cfg->reclaim_max;
> +		if (params.max_reclaim_size == 0)
> +			params.max_reclaim_size = RTE_FIB6_RCU_DQ_RECLAIM_MAX;
> +		params.esize = sizeof(uint64_t);
> +		params.free_fn = __rcu_qsbr_free_resource;
> +		params.p = dp;
> +		params.v = cfg->v;
> +		dp->dq = rte_rcu_qsbr_dq_create(&params);
> +		if (dp->dq == NULL) {
> +			FIB_LOG(ERR, "FIB6 defer queue creation failed");
> +			return -ENOMEM;
> +		}
I think it would be better to add here an explicit break instead of 
falling through
> +	case RTE_FIB6_QSBR_MODE_SYNC:
> +		/* No other things to do. */
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	dp->rcu_mode = cfg->mode;
> +	dp->v = cfg->v;
> +
> +	return 0;
> +}
> diff --git a/lib/fib/trie.h b/lib/fib/trie.h
> index bcb161702b49..c34cc2c05731 100644
> --- a/lib/fib/trie.h
> +++ b/lib/fib/trie.h
> @@ -38,6 +38,10 @@ struct rte_trie_tbl {
>   	uint64_t	*tbl8;		/**< tbl8 table. */
>   	uint32_t	*tbl8_pool;	/**< bitmap containing free tbl8 idxes*/
>   	uint32_t	tbl8_pool_pos;
> +	/* RCU config. */
> +	enum rte_fib6_qsbr_mode rcu_mode; /**< Blocking, defer queue. */
> +	struct rte_rcu_qsbr *v; /**< RCU QSBR variable. */
> +	struct rte_rcu_qsbr_dq *dq; /**< RCU QSBR defer queue. */
>   	/* tbl24 table. */
>   	alignas(RTE_CACHE_LINE_SIZE) uint64_t	tbl24[];
>   };
> @@ -143,4 +147,8 @@ int
>   trie_modify(struct rte_fib6 *fib, const struct rte_ipv6_addr *ip,
>   	uint8_t depth, uint64_t next_hop, int op);
>   
> +int
> +trie_rcu_qsbr_add(struct rte_trie_tbl *dp, struct rte_fib6_rcu_config *cfg,
> +	const char *name);
> +
>   #endif /* _TRIE_H_ */

-- 
Regards,
Vladimir