|SUCCESS| [v2] mbuf: fix mbuf operations history recording

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

Subject: Re: [PATCH v2] mbuf: fix mbuf operations history recording

git sha: 20260511133952.65539-1-mb at smartsharesystems.com
Author: Morten Brorup <mb at smartsharesystems.com>
Subject: [PATCH v2] mbuf: fix mbuf operations history recording

This patch addresses three bugs in mbuf operations history recording
related to allocation and bulk freeing operations.

> diff --git a/lib/mbuf/rte_mbuf.c b/lib/mbuf/rte_mbuf.c
> index c2476e7704..005bfaa573 100644
> --- a/lib/mbuf/rte_mbuf.c
> +++ b/lib/mbuf/rte_mbuf.c
> @@ -540,8 +540,8 @@ __rte_pktmbuf_free_seg_via_array(struct rte_mbuf *m,
>  	if (likely(m != NULL)) {
>  		if (*nb_pending == pending_sz ||
>  		    (*nb_pending > 0 && m->pool != pending[0]->pool)) {
> -			rte_mempool_put_bulk(pending[0]->pool,
> -					(void **)pending, *nb_pending);
> +			rte_mbuf_raw_free_bulk(pending[0]->pool,
> +					pending, *nb_pending);
>  			*nb_pending = 0;
>  		}
>  

[ ... ]

> @@ -581,7 +579,7 @@ void rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned int count)
>  	}
>  
>  	if (nb_pending > 0)
> -		rte_mempool_put_bulk(pending[0]->pool, (void **)pending, nb_pending);
> +		rte_mbuf_raw_free_bulk(pending[0]->pool, pending, nb_pending);
>  }

Does rte_mbuf_raw_free_bulk() properly reset refcnt values before
putting mbufs back to the pool? The original code using
rte_mempool_put_bulk() relied on the caller having already decremented
refcnt to zero. If rte_mbuf_raw_free_bulk() does not handle refcnt,
does this code now leak mbufs with non-zero refcnt values back into
the pool?

Also in rte_pktmbuf_free_bulk()->__rte_pktmbuf_free_seg_via_array(),
the pending[] array gets populated with mbufs via
rte_pktmbuf_prefree_seg() which already decrements refcnt. When the
pending array is flushed using rte_mbuf_raw_free_bulk(), does this
double-free the mbufs if rte_mbuf_raw_free_bulk() also decrements
refcnt?

> diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
> index e7c3bbadd4..60ec8158cd 100644
> --- a/lib/mbuf/rte_mbuf.h
> +++ b/lib/mbuf/rte_mbuf.h
> @@ -663,14 +663,14 @@ static __rte_always_inline int
>  rte_mbuf_raw_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf **mbufs, unsigned int count)
>  {
>  	int rc = rte_mempool_get_bulk(mp, (void **)mbufs, count);
> -	if (likely(rc == 0)) {
> -		for (unsigned int idx = 0; idx < count; idx++)
> -			__rte_mbuf_raw_sanity_check_mp(mbufs[idx], mp);
> -	}
> +	if (unlikely(rc))
> +		return rc;
> +	for (unsigned int idx = 0; idx < count; idx++)
> +		__rte_mbuf_raw_sanity_check_mp(mbufs[idx], mp);

The change to early-return style looks correct and fixes the original
issue where mbufs array would be dereferenced even when allocation
failed. This improves code safety.

However, does the coding style guide have a preference? The original
pattern with explicit (rc == 0) is more explicit, while the new
if(unlikely(rc)) relies on implicit zero comparison. According to the
DPDK coding style, comparisons to zero should be explicit for integer
types. Should this be if (unlikely(rc != 0))?

>  
>  	rte_mbuf_history_mark_bulk(mbufs, count, RTE_MBUF_HISTORY_OP_LIB_ALLOC);
>  
> -	return rc;
> +	return 0;
>  }

[ ... ]

> @@ -1068,8 +1068,6 @@ static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
>  	if (unlikely(rc))
>  		return rc;
>  
> -	rte_mbuf_history_mark_bulk(mbufs, count, RTE_MBUF_HISTORY_OP_LIB_ALLOC);
> -
>  	rte_mbuf_raw_reset_bulk(pool, mbufs, count);

Removing the duplicate history marking in rte_pktmbuf_alloc_bulk()
makes sense since rte_mbuf_raw_alloc_bulk() already records the
operation. This avoids double-recording for the same allocation.