|SUCCESS| [v6] net/idpf: update for new mempool cache algorithm

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-27

Review of patch: net/idpf: update for new mempool cache algorithm

commit 1bd8bda001dcedcd (based on patch metadata)
Author: Morten Brorup <mb at smartsharesystems.com>
Subject: [PATCH v6] net/idpf: update for new mempool cache algorithm

This patch updates the idpf driver's AVX512 receive queue rearm functions
to work with a new mempool cache algorithm. The changes modify how the
mempool cache is backfilled when it runs low on objects.

> diff --git a/drivers/net/intel/idpf/idpf_common_rxtx_avx512.c b/drivers/net/intel/idpf/idpf_common_rxtx_avx512.c
> index 9af275cd9d..dd2263b8d7 100644
> --- a/drivers/net/intel/idpf/idpf_common_rxtx_avx512.c
> +++ b/drivers/net/intel/idpf/idpf_common_rxtx_avx512.c
> @@ -148,15 +148,31 @@ idpf_singleq_rearm(struct idpf_rx_queue *rxq)
>  	/* Can this be satisfied from the cache? */
>  	if (cache->len < IDPF_RXQ_REARM_THRESH) {
>  		/* No. Backfill the cache first, and then fill from it */
> -		uint32_t req = IDPF_RXQ_REARM_THRESH + (cache->size -
> -							cache->len);
>  
> -		/* How many do we require i.e. number to fill the cache + the request */
> +		/* Backfill would exceed the cache bounce buffer limit? */
> +		__rte_assume(cache->size / 2 <= RTE_MEMPOOL_CACHE_MAX_SIZE / 2);
> +		if (unlikely(cache->size / 2 < IDPF_RXQ_REARM_THRESH)) {
> +			idpf_singleq_rearm_common(rxq);
> +			return;
> +		}
> +
> +		/*
> +		 * Backfill the cache from the backend;
> +		 * move up the hot objects in the cache to the top half of the cache,
> +		 * and fetch (size / 2) objects to the bottom of the cache.
> +		 */
> +		__rte_assume(cache->len < cache->size / 2);
> +		rte_memcpy(&cache->objs[cache->size / 2], &cache->objs[0],
> +				sizeof(void *) * cache->len);

Does this code correctly handle potential integer overflow in the
calculation sizeof(void *) * cache->len? The cache->len value comes from
runtime state and might not be bounds-checked prior to this multiplication.

> 		int ret = rte_mempool_ops_dequeue_bulk
> -				(rxq->mp, &cache->objs[cache->len], req);
> +				(rxq->mp, &cache->objs[0], cache->size / 2);
>  		if (ret == 0) {
> -			cache->len += req;
> +			cache->len += cache->size / 2;

Does this addition correctly maintain cache->len <= cache->size? Given the
prior rte_memcpy() moved objects to &cache->objs[cache->size / 2], and now
cache->size / 2 objects are added at position 0, can cache->len exceed
cache->size in edge cases?

[ ... ]

> @@ -565,15 +581,31 @@ idpf_splitq_rearm(struct idpf_rx_queue *rx_bufq)
>  	/* Can this be satisfied from the cache? */
>  	if (cache->len < IDPF_RXQ_REARM_THRESH) {
>  		/* No. Backfill the cache first, and then fill from it */
> -		uint32_t req = IDPF_RXQ_REARM_THRESH + (cache->size -
> -							cache->len);
>  
> -		/* How many do we require i.e. number to fill the cache + the request */
> +		/* Backfill would exceed the cache bounce buffer limit? */
> +		__rte_assume(cache->size / 2 <= RTE_MEMPOOL_CACHE_MAX_SIZE / 2);
> +		if (unlikely(cache->size / 2 < IDPF_RXQ_REARM_THRESH)) {
> +			idpf_splitq_rearm_common(rx_bufq);
> +			return;
> +		}
> +
> +		/*
> +		 * Backfill the cache from the backend;
> +		 * move up the hot objects in the cache to the top half of the cache,
> +		 * and fetch (size / 2) objects to the bottom of the cache.
> +		 */
> +		__rte_assume(cache->len < cache->size / 2);
> +		rte_memcpy(&cache->objs[cache->size / 2], &cache->objs[0],
> +				sizeof(void *) * cache->len);
>  		int ret = rte_mempool_ops_dequeue_bulk
> -				(rx_bufq->mp, &cache->objs[cache->len], req);
> +				(rx_bufq->mp, &cache->objs[0], cache->size / 2);
>  		if (ret == 0) {
> -			cache->len += req;
> +			cache->len += cache->size / 2;
>  		} else {
> +			/*
> +			 * No further action is required for roll back, as the objects moved
> +			 * in the cache were actually copied, and the cache remains intact.
> +			 */

In both idpf_singleq_rearm() and idpf_splitq_rearm(), when
rte_mempool_ops_dequeue_bulk() fails after the rte_memcpy() has already
relocated objects, does the code correctly handle the inconsistent cache
state? The comment claims the cache remains intact, but cache->objs[0]
through cache->objs[cache->len - 1] are now stale after being copied to
cache->objs[cache->size / 2] and onward. Any subsequent cache access would
see duplicate pointers in both regions.

Can cache->len correctly represent the cache state when some objects exist
in two locations within cache->objs[]? Would accessing cache via normal
dequeue operations access stale pointers from the lower half?