RE: [PATCH 13/20] mbuf: allow NULL array in rte_pktmbuf_free_bulk

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Saturday, 9 May 2026 17.47
> 
> On Sat, 9 May 2026 10:47:53 +0200
> Morten Brørup <mb@smartsharesystems.com> wrote:
> 
> > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > Sent: Friday, 8 May 2026 22.34
> > >
> > > This allows callers to avoid NULL checks and just call
> > > rte_pktmbuf_free_bulk, similar to rte_pktmbuf_free.
> > >
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> >
> > I disagree with this patch.
> >
> > The parameter is an array of (pointers to) mbufs.
> > We already accept that the array can contain NULL pointers (no mbuf
> present).
> > This is extremely forgiving, considering that other fast path
> functions don't allow NULL pointers in arrays;
> > e.g. rte_eth_tx_burst(), rte_mempool_put_bulk().
> > But since it's a "free()" class of function, I don't object to it.
> >
> > However, this patch changes the parameter type from "array" to "array
> or NULL (no array present)".
> > And I don't think we should change the parameter type; it should
> remain "array" only.
> >
> > If there are any scenarios where a non-present array (NULL) is passed
> to the function, the count should be zero too.
> > And when the count is zero, the function does not dereference the
> array, so explicitly checking for NULL is superfluous.
> >
> > We have a convention of not checking parameter validity in fast path
> functions.
> > And I consider it invalid parameters passing NULL with a non-zero
> count.
> >
> > You might argue that this is a "free()" class of function, which
> warrants checking for NULL; but since it already accepts NULL with zero
> count, it is already covered.
> >
> > We could change the function declaration for clarity:
> >
> > void rte_pktmbuf_free_bulk(
> > 		unsigned int count;
> > 		struct rte_mbuf *mbufs[count], unsigned int count);
> >
> > Or add a debug assertion at the start of the function:
> > RTE_ASSERT(mbufs != NULL || count == 0);
> 
> Ok, it was more motivated by common pattern in driver cleanup paths
> like:
> 
> --- a/app/test-compress-perf/comp_perf_test_common.c
> +++ b/app/test-compress-perf/comp_perf_test_common.c
> @@ -83,11 +83,9 @@ comp_perf_free_memory(struct comp_test_data
> *test_data,
>  {
>  	uint32_t i;
> 
> -	if (mem->decomp_bufs != NULL)
> -		rte_pktmbuf_free_bulk(mem->decomp_bufs, mem->total_bufs);
> +	rte_pktmbuf_free_bulk(mem->decomp_bufs, mem->total_bufs);
> 
> -	if (mem->comp_bufs != NULL)
> -		rte_pktmbuf_free_bulk(mem->comp_bufs, mem->total_bufs);
> +	rte_pktmbuf_free_bulk(mem->comp_bufs, mem->total_bufs);
> 

Skimming comp_perf_test_common.c, it looks like mem->total_bufs is initialized to the number of wanted buffers, and then mem->decomp_bufs is set up afterwards. In other words, total_bufs can be non-zero while comp_bufs is NULL.

IMO, removing the NULL comparison here would pass invalid parameters to rte_pktmbuf_free_bulk().

Train of thoughts...

On the other hand, it does provide a good example where considering rte_pktmbuf_free_bulk() a "free()" class function accepting a NULL pointer would be helpful.
And the added performance cost of checking for a NULL pointer is per burst, not per packet.
I'm not as strongly opposed as I was initially.

However, looking at it in a broader scope gets me be back to being opposed:
This patch is for freeing mbufs.
If we consider freeing mempool objects, the cleanup function would call rte_mempool_put_bulk() to free the objects, which is the function for freeing previously allocated mempool objects. It just happens to not have "free" as part of its name.

The mempool single object "free()" function, rte_mempool_put(), doesn't accept a NULL pointer.
Similarly, the mempool bulk free function, rte_mempool_put_bulk(), doesn't accept holes (NULL pointers) in the array.
I certainly do not want to introduce holes into mempool object arrays.

Maybe it was a bad decision to allow holes in mbuf arrays being passed to rte_pktmbuf_free_bulk(). Such holes are not accepted in any other DPDK APIs.

At this point, I'm still not in favor of this patch.
It's defensive coding (with a performance cost, however small) in a fast path function.