Re: [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added + unittest

[Olivier MATZ <olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>]

Hi Neil,

On 03/18/2015 09:58 PM, Neil Horman wrote:
>> +/**
>> + * Free a bulk of mbufs into its original mempool.
>> + * This function assumes:
>> + * - refcnt equals 1
>> + * - mbufs are direct
>> + * - all mbufs must belong to the same mempool
>> + *
>> + * @param mbufs
>> + *    Array of pointers to mbuf
>> + * @param count
>> + *    Array size
>> + */
>> +static inline void rte_pktmbuf_bulk_free(struct rte_mbuf **mbufs,
>> +					 unsigned count)
>> +{
>> +	unsigned idx;
>> +
>> +	RTE_MBUF_ASSERT(count > 0);
>> +
>> +	for (idx = 0; idx < count; idx++) {
>> +		RTE_MBUF_ASSERT(mbufs[idx]->pool == mbufs[0]->pool);
>> +		RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 1);
>> +		rte_mbuf_refcnt_set(mbufs[idx], 0);
> This is really a misuse of the API.  The entire point of reference counting is
> to know when an mbuf has no more references and can be freed.  By forcing all
> the reference counts to zero here, you allow the refcnt infrastructure to be
> circumvented, causing memory leaks.
>
> I think what you need to do here is enhance the underlying pktmbuf interface
> such that an rte_mbuf structure has a destructor method association with it
> which is called when its refcnt reaches zero.  That way the
> rte_pktmbuf_bulk_free function can just decrement the refcnt on each
> mbuf_structure, and the pool as a whole can be returned when the destructor
> function discovers that all mbufs in that bulk pool are freed.

I don't really understand what's the problem here. The API explicitly
describes the conditions for calling this functions: the segments are
directs, they belong to the same mempool, and their refcnt is 1.

This function could be useful in a driver which knows that the mbuf
it allocated matches this conditions. I think an application that
only uses direct mbufs and one mempool could also use this function.

Regards,
Olivier