On Sun, 10 May 2026 14:31:57 +0200 Morten Brørup <[email protected]> wrote:
> > From: Stephen Hemminger [mailto:[email protected]] > > Sent: Saturday, 9 May 2026 17.47 > > > > On Sat, 9 May 2026 10:47:53 +0200 > > Morten Brørup <[email protected]> wrote: > > > > > > From: Stephen Hemminger [mailto:[email protected]] > > > > 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 <[email protected]> > > > > > > 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)
