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

[Stephen Hemminger <stephen@networkplumber.org>]

On Sun, 10 May 2026 14:31:57 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> > 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.
> 

More thoughts.
- allowing NULL array is not worth it; not a big win, and only one place mattered

- allowing holes in array seems odd, and not that useful. Probably can't change now.
  Not sure why callers would need it.

- Probably should add RTE_ASSERT() into free bulk. Should allow rte_pktmbuf_free_bulk(NULL, 0)
  for consistency with some other API's that take array and count. But assert should trigger
  on rte_pktmbuf_free_bulk(NULL, 1)