[dpdk-dev] [PATCH] rte_mbuf: bulk allocation and freeing functions + unittest

Olivier MATZ olivier.matz at 6wind.com
Mon Mar 16 10:50:27 CET 2015


Hi Vadim,

Please see some comments below.

On 03/13/2015 11:14 AM, vadim.suraev at gmail.com wrote:
> From: "vadim.suraev at gmail.com" <vadim.suraev at gmail.com>
> 
> - an API function to allocate a bulk of rte_mbufs
> - an API function to free a bulk of rte_mbufs
> - an API function to free a chained rte_mbuf
> - unittest for aboce
> 
> Signed-off-by: vadim.suraev at gmail.com <vadim.suraev at gmail.com>
> ---
>  app/test/test_mbuf.c       |   73 ++++++++++++++++++++++++++++++++
>  lib/librte_mbuf/rte_mbuf.h |  101 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 174 insertions(+)
> 
> diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
> index 1ff66cb..a557705 100644
> --- a/app/test/test_mbuf.c
> +++ b/app/test/test_mbuf.c
> @@ -405,6 +405,67 @@ test_pktmbuf_pool(void)
>  	return ret;
>  }
>  
> +/* test pktmbuf bulk allocation and freeing
> +*/
> +static int
> +test_pktmbuf_pool_bulk(void)
> +{
> +	unsigned i;
> +	unsigned mbufs_to_allocate = NB_MBUF - 32; /* size of mempool - size of local cache, otherwise may fail */

Can you add a constant for local cache size?


> +	struct rte_mbuf *m[mbufs_to_allocate];
> +	int ret = 0;
> +	unsigned mbuf_count_before_allocation = rte_mempool_count(pktmbuf_pool);
> +
> +	for (i=0; i<mbufs_to_allocate; i++)
> +		m[i] = NULL;
> +	/* alloc NB_MBUF-32 mbufs */
> +	if ((ret = rte_pktmbuf_alloc_bulk(pktmbuf_pool, m, mbufs_to_allocate))) {
> +		printf("cannot allocate %d mbufs bulk mempool_count=%d ret=%d\n", mbufs_to_allocate, rte_mempool_count(pktmbuf_pool), ret);
> +		return -1;
> +	}
> +	if ((rte_mempool_count(pktmbuf_pool) + mbufs_to_allocate) != mbuf_count_before_allocation) {
> +		printf("mempool count %d + allocated %d != initial %d\n",
> +			rte_mempool_count(pktmbuf_pool), mbufs_to_allocate, mbuf_count_before_allocation);
> +		return -1;
> +	}

Could you verify your modifications with checkpatch? It will triggers
warnings for lines exceeding 80 columns or missing spaces around
operators (even though it's like this in the rest of the file).


> +	/* free them */
> +	rte_pktmbuf_bulk_free(m, mbufs_to_allocate);
> +	
> +	if (rte_mempool_count(pktmbuf_pool)  != mbuf_count_before_allocation) {
> +		printf("mempool count %d != initial %d\n",
> +			rte_mempool_count(pktmbuf_pool), mbuf_count_before_allocation);
> +		return -1;
> +	}
> +	for (i=0; i<mbufs_to_allocate; i++)
> +		m[i] = NULL;
> +
> +	/* alloc NB_MBUF-32 mbufs */
> +	if ((ret = rte_pktmbuf_alloc_bulk(pktmbuf_pool, m, mbufs_to_allocate))) {
> +		printf("cannot allocate %d mbufs bulk mempool_count=%d ret=%d\n", mbufs_to_allocate, rte_mempool_count(pktmbuf_pool), ret);
> +		return -1;
> +	}
> +	if ((rte_mempool_count(pktmbuf_pool) + mbufs_to_allocate) != mbuf_count_before_allocation) {
> +		printf("mempool count %d + allocated %d != initial %d\n",
> +			rte_mempool_count(pktmbuf_pool), mbufs_to_allocate, mbuf_count_before_allocation);
> +		return -1;
> +	}
> +
> +	/* chain it */
> +	for (i=0; i< mbufs_to_allocate - 1; i++) {
> +		m[i]->next = m[i + 1];
> +		m[0]->nb_segs++;
> +	}
> +	/* free them */
> +	rte_pktmbuf_free_chain(m[0]);
> +	
> +	if (rte_mempool_count(pktmbuf_pool)  != mbuf_count_before_allocation) {
> +		printf("mempool count %d != initial %d\n",
> +			rte_mempool_count(pktmbuf_pool), mbuf_count_before_allocation);
> +		return -1;
> +	}
> +	return ret;
> +}
> +
>  /*
>   * test that the pointer to the data on a packet mbuf is set properly
>   */
> @@ -790,6 +851,18 @@ test_mbuf(void)
>  		return -1;
>  	}
>  
> +	/* test bulk allocation and freeing */
> +	if (test_pktmbuf_pool_bulk() < 0) {
> +		printf("test_pktmbuf_pool_bulk() failed\n");
> +		return -1;
> +	}
> +
> +	/* once again to ensure all mbufs were freed */
> +	if (test_pktmbuf_pool_bulk() < 0) {
> +		printf("test_pktmbuf_pool_bulk() failed\n");
> +		return -1;
> +	}
> +
>  	/* test that the pointer to the data on a packet mbuf is set properly */
>  	if (test_pktmbuf_pool_ptr() < 0) {
>  		printf("test_pktmbuf_pool_ptr() failed\n");
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 17ba791..482920e 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -825,6 +825,107 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
>  }
>  
>  /**
> + * Allocates a bulk of mbufs, initiates refcnt and resets

For API comments, we try use the imperative form (no "s" at the end).
This applies to all comments of the patch.



> + *
> + * @param pool
> + *    memory pool to allocate from
> + * @param mbufs
> + *    Array of pointers to mbuf
> + * @param count
> + *    Array size
> + */
> +static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool, struct rte_mbuf **mbufs, unsigned count)
> +{
> +	unsigned idx;
> +	int rc = 0;
> +
> +	if (unlikely(rc = rte_mempool_get_bulk(pool, (void **)mbufs, count))) {
> +		return rc;
> +	}
> +
> +	for (idx = 0; idx < count; idx++) {
> +		RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> +		rte_mbuf_refcnt_set(mbufs[idx], 1);
> +		rte_pktmbuf_reset(mbufs[idx]);
> +	}
> +	return rc;
> +}
> +
> +/**
> + * Frees a bulk of mbufs into its original mempool.
> + * It is responsibility of caller to update and check refcnt
> + * as well as to check for attached mbufs to be freed
> + *
> + * @param mbufs
> + *    Array of pointers to mbuf
> + * @param count
> + *    Array size
> + */
> +
> +static inline void rte_pktmbuf_bulk_raw_free(struct rte_mbuf **mbufs, unsigned count)
> +{
> +	if (unlikely(count == 0))
> +		return;

Should we really test this? The mbuf layer should be as fast as
possible and should avoid tests when they are not necessary. I
would prefer a comment (+ an assert ?) saying count must be
strictly positive.


> +	rte_mempool_put_bulk(mbufs[0]->pool, (void **)mbufs, count);
> +}

Adding this function is not a problem today because it is the free
function associated to rte_pktmbuf_bulk_raw_alloc().

However, I think that the 'raw' alloc/free functions should be removed
in the future as they are just wrappers to mempool_get/put. There is
a problem with that today because the raw alloc also sets the refcnt,
but I think it could go in pktmbuf_reset(). I plan to do that for dpdk
2.1, what do you think?

Another thing: the fact that the mbufs should belong to the same pool
should be clearly noticed in the comment, as it can lead to really
important bugs. By the way, these bugs wouldn't happen with mempool
API because we have to specify the mempool pointer, so the user is
aware that the mempool may not be the same for all mbufs.


> +
> +/**
> + * Frees a bulk of mbufs into its original mempool.
> + * This function assumes refcnt has been already decremented
> + * as well as the freed mbufs are direct

I don't understand the "assumes refcnt has been already decremented".


> + *
> + * @param mbufs
> + *    Array of pointers to mbuf
> + * @param count
> + *    Array size
> + */
> +
> +static inline void rte_pktmbuf_bulk_free(struct rte_mbuf **mbufs, unsigned count)

empty line here


> +{
> +	if (unlikely(count == 0))
> +		return;
> +	unsigned idx;
> +

I know it's allowed by C99, but I prefer to have variable declarations
at the beginning of a block.


> +	for(idx = 0; idx < count; idx++) {
> +		rte_mbuf_refcnt_update(mbufs[idx], -1);
> +		RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> +	}
> +	rte_mempool_put_bulk(mbufs[0]->pool, (void **)mbufs, count);
> +}
> +
> +/**
> + * Frees chained (scattered) mbufs into its original mempool(s).
> + *
> + * @param head
> + *    The head of mbufs to be freed chain
> + */
> +
> +static inline void rte_pktmbuf_free_chain(struct rte_mbuf *head)

empty line above


> +{
> +	if (likely(head != NULL)) {

I think we should remove this test. The other mbuf functions do not
check this.


> +		struct rte_mbuf *mbufs[head->nb_segs];
> +		unsigned mbufs_count = 0;
> +		struct rte_mbuf *next;
> +
> +		while (head) {
> +			next = head->next;
> +			head->next = NULL; 
> +			if (likely(NULL != (head = __rte_pktmbuf_prefree_seg(head)))) {

replacing the last line by:

head = __rte_pktmbuf_prefree_seg(head);
if (likely(head != NULL)) {

Would be easier to read.



> +				RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(head) == 0);
> +				if (likely((!mbufs_count) || (head->pool == mbufs[0]->pool)))
> +					mbufs[mbufs_count++] = head;
> +				else {
> +					rte_pktmbuf_bulk_raw_free(mbufs, mbufs_count);
> +					mbufs_count = 0;
> +				}
> +			}
> +			head = next;
> +		}
> +		rte_pktmbuf_bulk_raw_free(mbufs, mbufs_count);

If the test "if (unlikely(count == 0))" is removed in
rte_pktmbuf_bulk_raw_free(), it should be added here.


Regards,
Olivier


More information about the dev mailing list