[dpdk-dev] [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added + unittest

[dpdk-dev] [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added + unittest

[vadim.suraev]

From: "vadim.suraev@gmail.com" <vadim.suraev@gmail.com>

This patch adds mbuf bulk allocation/freeing functions and unittest

Signed-off-by: Vadim Suraev
<vadim.suraev@gmail.com>
---
New in v2:
    - function rte_pktmbuf_alloc_bulk added
    - function rte_pktmbuf_bulk_free added
    - function rte_pktmbuf_free_chain added
    - applied reviewers' comments

 app/test/test_mbuf.c       |   94 +++++++++++++++++++++++++++++++++++++++++++-
 lib/librte_mbuf/rte_mbuf.h |   91 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 184 insertions(+), 1 deletion(-)

diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index 1ff66cb..b20c6a4 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -77,6 +77,7 @@
 #define REFCNT_RING_SIZE        (REFCNT_MBUF_NUM * REFCNT_MAX_REF)
 
 #define MAKE_STRING(x)          # x
+#define MBUF_POOL_LOCAL_CACHE_SIZE 32
 
 static struct rte_mempool *pktmbuf_pool = NULL;
 
@@ -405,6 +406,84 @@ test_pktmbuf_pool(void)
 	return ret;
 }
 
+/* test pktmbuf bulk allocation and freeing
+*/
+static int
+test_pktmbuf_pool_bulk(void)
+{
+	unsigned i;
+	/* size of mempool - size of local cache, otherwise may fail */
+	unsigned mbufs_to_allocate = NB_MBUF - MBUF_POOL_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-MBUF_POOL_LOCAL_CACHE_SIZE mbufs */
+	ret = rte_pktmbuf_alloc_bulk(pktmbuf_pool, m, mbufs_to_allocate);
+	if (ret) {
+		printf("cannot allocate %d mbufs bulk mempool_cnt=%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;
+	}
+	/* 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-MBUF_POOL_LOCAL_CACHE_SIZE mbufs */
+	ret = rte_pktmbuf_alloc_bulk(pktmbuf_pool, m, mbufs_to_allocate);
+	if (ret) {
+		printf("cannot allocate %d mbufs bulk mempool_cnt=%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
  */
@@ -766,7 +845,8 @@ test_mbuf(void)
 	if (pktmbuf_pool == NULL) {
 		pktmbuf_pool =
 			rte_mempool_create("test_pktmbuf_pool", NB_MBUF,
-					   MBUF_SIZE, 32,
+					   MBUF_SIZE,
+					   MBUF_POOL_LOCAL_CACHE_SIZE,
 					   sizeof(struct rte_pktmbuf_pool_private),
 					   rte_pktmbuf_pool_init, NULL,
 					   rte_pktmbuf_init, NULL,
@@ -790,6 +870,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..fabeae2 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -825,6 +825,97 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
 }
 
 /**
+ * Allocate a bulk of mbufs, initiate refcnt and resets
+ *
+ * @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;
+
+	rc = rte_mempool_get_bulk(pool, (void **)mbufs, count);
+	if (unlikely(rc))
+		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;
+}
+
+/**
+ * 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);
+	}
+	rte_mempool_put_bulk(mbufs[0]->pool, (void **)mbufs, count);
+}
+
+/**
+ * Free chained (scattered) mbufs into its original mempool(s).
+ *
+ * @param head
+ *    The head of mbufs to be freed chain. Must not be NULL
+ */
+static inline void rte_pktmbuf_free_chain(struct rte_mbuf *head)
+{
+	struct rte_mbuf *mbufs[head->nb_segs];
+	unsigned mbufs_count = 0;
+	struct rte_mbuf *next;
+
+	while (head) {
+		next = head->next;
+		head = __rte_pktmbuf_prefree_seg(head);
+		if (likely(head != NULL)) {
+			head->next = NULL;
+			if (likely((!mbufs_count) ||
+				   (head->pool == mbufs[0]->pool)))
+				mbufs[mbufs_count++] = head;
+			else {
+				rte_mempool_put_bulk(mbufs[0]->pool,
+						     (void **)mbufs,
+						     mbufs_count);
+				mbufs_count = 0;
+			}
+		}
+		head = next;
+	}
+	if (mbufs_count > 0)
+		rte_mempool_put_bulk(mbufs[0]->pool,
+				     (void **)mbufs,
+				     mbufs_count);
+}
+
+/**
  * Creates a "clone" of the given packet mbuf.
  *
  * Walks through all segments of the given packet mbuf, and for each of them:
-- 
1.7.9.5

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

[Neil Horman]

On Wed, Mar 18, 2015 at 10:21:18PM +0200, vadim.suraev@gmail.com wrote:
> From: "vadim.suraev@gmail.com" <vadim.suraev@gmail.com>
> 
> This patch adds mbuf bulk allocation/freeing functions and unittest
> 
> Signed-off-by: Vadim Suraev
> <vadim.suraev@gmail.com>
> ---
> New in v2:
>     - function rte_pktmbuf_alloc_bulk added
>     - function rte_pktmbuf_bulk_free added
>     - function rte_pktmbuf_free_chain added
>     - applied reviewers' comments
> 
>  app/test/test_mbuf.c       |   94 +++++++++++++++++++++++++++++++++++++++++++-
>  lib/librte_mbuf/rte_mbuf.h |   91 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 184 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
> index 1ff66cb..b20c6a4 100644
> --- a/app/test/test_mbuf.c
> +++ b/app/test/test_mbuf.c
> @@ -77,6 +77,7 @@
>  #define REFCNT_RING_SIZE        (REFCNT_MBUF_NUM * REFCNT_MAX_REF)
>  
>  #define MAKE_STRING(x)          # x
> +#define MBUF_POOL_LOCAL_CACHE_SIZE 32
>  
>  static struct rte_mempool *pktmbuf_pool = NULL;
>  
> @@ -405,6 +406,84 @@ test_pktmbuf_pool(void)
>  	return ret;
>  }
>  
><snip>
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 17ba791..fabeae2 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -825,6 +825,97 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
>  }
>  
>  /**
> + * Allocate a bulk of mbufs, initiate refcnt and resets
> + *
> + * @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;
> +
> +	rc = rte_mempool_get_bulk(pool, (void **)mbufs, count);
> +	if (unlikely(rc))
> +		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;
> +}
> +
> +/**
> + * 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.

Neil

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

[Olivier MATZ]

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

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

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ
> Sent: Thursday, March 19, 2015 8:41 AM
> To: Neil Horman; vadim.suraev@gmail.com
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added + unittest
> 
> 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.

I also don't see anything wrong with that function.
As long, as user makes sure that all mbufs in the bulk satisfy the required conditions it should be ok, I think.
Of course, it's usage is limited, but I suppose the author has some scenario in mind, when introduced it.

Konstantin

> 
> Regards,
> Olivier

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

[Neil Horman]

On Thu, Mar 19, 2015 at 09:41:25AM +0100, Olivier MATZ wrote:
> 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.
> 


That last condition is my issue with this patch, that the user has to know what
refcnts are.  It makes this api useful for little more than the test case that
is provided with it.  Its irritating enough that for singly allocated mbufs the
user has to know what the refcount of a buffer is before freeing, but at least
they can macrotize a {rte_pktmbuf_refcnt_update; if(rte_pktmbuf_refct_read) then
free} operation.

With this, you've placed the user in charge of not only doing that, but also of
managing the relationship between pktmbufs and the pool they came from.  while
that makes sense for the test case, it really doesn't in any general use case in
which packet processing is ever deferred or queued, because it means that the
application is now responsible for holding a pointer to every packet it
allocates and checking its refcount periodically until it completes.

There is never any reason that an application won't need to do this management,
so making it the purview of the application to handle rather than properly
integrating that functionality in the library is really a false savings.

Regards
Neil

> Regards,
> Olivier
> 
> 

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

[Olivier MATZ]

Hi Neil,

On 03/19/2015 02:16 PM, Neil Horman wrote:
>> 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.
> 
> 
> That last condition is my issue with this patch, that the user has to know what
> refcnts are.  It makes this api useful for little more than the test case that
> is provided with it.  Its irritating enough that for singly allocated mbufs the
> user has to know what the refcount of a buffer is before freeing, but at least
> they can macrotize a {rte_pktmbuf_refcnt_update; if(rte_pktmbuf_refct_read) then
> free} operation.
> 
> With this, you've placed the user in charge of not only doing that, but also of
> managing the relationship between pktmbufs and the pool they came from.  while
> that makes sense for the test case, it really doesn't in any general use case in
> which packet processing is ever deferred or queued, because it means that the
> application is now responsible for holding a pointer to every packet it
> allocates and checking its refcount periodically until it completes.
> 
> There is never any reason that an application won't need to do this management,
> so making it the purview of the application to handle rather than properly
> integrating that functionality in the library is really a false savings.

There are some places where you know that the prerequisites are met,
so you can save cycles by using this function.

>From what I imagine, if in a driver you allocate mbufs, chain them and
for some reason you realize you have to free them, you can use this
function instead of freeing them one by one.

Also, as it's up to the application to decide how many mbuf pools are
created, and whether indirect mbufs are used or not, the application
can take the short path of using this function in some conditions.

Vadim, maybe you have another reason or use case for adding this
function? Could you detail why you need it and how it improves your
use case?

Regards,
Olivier

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

[Vadim Suraev]

Hi, Olivier,
No, I personally need to free a chain using mempool bulk. If I'm not
mistaken, it has been proposed by one of reviewers to have lower level
function, so it was done (I'm sorry if misunderstood)
Regards,
Vadim.
On Mar 23, 2015 8:44 PM, "Olivier MATZ" <olivier.matz@6wind.com> wrote:

> Hi Neil,
>
> On 03/19/2015 02:16 PM, Neil Horman wrote:
> >> 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.
> >
> >
> > That last condition is my issue with this patch, that the user has to
> know what
> > refcnts are.  It makes this api useful for little more than the test
> case that
> > is provided with it.  Its irritating enough that for singly allocated
> mbufs the
> > user has to know what the refcount of a buffer is before freeing, but at
> least
> > they can macrotize a {rte_pktmbuf_refcnt_update;
> if(rte_pktmbuf_refct_read) then
> > free} operation.
> >
> > With this, you've placed the user in charge of not only doing that, but
> also of
> > managing the relationship between pktmbufs and the pool they came from.
> while
> > that makes sense for the test case, it really doesn't in any general use
> case in
> > which packet processing is ever deferred or queued, because it means
> that the
> > application is now responsible for holding a pointer to every packet it
> > allocates and checking its refcount periodically until it completes.
> >
> > There is never any reason that an application won't need to do this
> management,
> > so making it the purview of the application to handle rather than
> properly
> > integrating that functionality in the library is really a false savings.
>
> There are some places where you know that the prerequisites are met,
> so you can save cycles by using this function.
>
> From what I imagine, if in a driver you allocate mbufs, chain them and
> for some reason you realize you have to free them, you can use this
> function instead of freeing them one by one.
>
> Also, as it's up to the application to decide how many mbuf pools are
> created, and whether indirect mbufs are used or not, the application
> can take the short path of using this function in some conditions.
>
> Vadim, maybe you have another reason or use case for adding this
> function? Could you detail why you need it and how it improves your
> use case?
>
> Regards,
> Olivier
>

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

[Neil Horman]

On Mon, Mar 23, 2015 at 05:44:02PM +0100, Olivier MATZ wrote:
> Hi Neil,
> 
> On 03/19/2015 02:16 PM, Neil Horman wrote:
> >> 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.
> > 
> > 
> > That last condition is my issue with this patch, that the user has to know what
> > refcnts are.  It makes this api useful for little more than the test case that
> > is provided with it.  Its irritating enough that for singly allocated mbufs the
> > user has to know what the refcount of a buffer is before freeing, but at least
> > they can macrotize a {rte_pktmbuf_refcnt_update; if(rte_pktmbuf_refct_read) then
> > free} operation.
> > 
> > With this, you've placed the user in charge of not only doing that, but also of
> > managing the relationship between pktmbufs and the pool they came from.  while
> > that makes sense for the test case, it really doesn't in any general use case in
> > which packet processing is ever deferred or queued, because it means that the
> > application is now responsible for holding a pointer to every packet it
> > allocates and checking its refcount periodically until it completes.
> > 
> > There is never any reason that an application won't need to do this management,
> > so making it the purview of the application to handle rather than properly
> > integrating that functionality in the library is really a false savings.
> 
> There are some places where you know that the prerequisites are met,
> so you can save cycles by using this function.
> 
> From what I imagine, if in a driver you allocate mbufs, chain them and
> for some reason you realize you have to free them, you can use this
> function instead of freeing them one by one.
> 
> Also, as it's up to the application to decide how many mbuf pools are
> created, and whether indirect mbufs are used or not, the application
> can take the short path of using this function in some conditions.
> 
> Vadim, maybe you have another reason or use case for adding this
> function? Could you detail why you need it and how it improves your
> use case?
> 
> Regards,
> Olivier
> 

So, I think we're making different points here.

You seem to be justifying the API as it exists by finding use cases that fit
into its documented restrictions (direct buffers, refcounts at 1, etc), which
severely limit that use case set.

My assertion is that those restrictions were created because it was
inconvienient to code using the reference count as intended.  I'm saying lets
augment the reference counting mechanism so that we can use these specially
allocated mbufs in a wider variety of use cases beyond the limited set they are
currently good for

Neil

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

[Ananyev, Konstantin]

Hi Vadim,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Vadim Suraev
> Sent: Monday, March 23, 2015 5:31 PM
> To: Olivier MATZ
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added + unittest
> 
> Hi, Olivier,
> No, I personally need to free a chain using mempool bulk. If I'm not
> mistaken, it has been proposed by one of reviewers to have lower level
> function, so it was done (I'm sorry if misunderstood)

Was it me?
As I remember, I said it would be good to create rte_pktmbuf_bulk_free() or rte_pktmbuf_seg_bulk_free() -
that would free a bulk of mbufs segments in the same manner as rte_pktmbuf_free_chain() does:
count number of consecutive mbufs from the same mempool to be freed and then put them back into the pool at one go.
Such function would be useful inside PMD code.
In fact we already using analogy of such function inside vPMD TX code.
Though from my point, such function should be generic as rte_pktmbuf_free_chain() -
no special limitations like all mbufs from one pool, refcnt==1, etc.
So if it was me who confused you - I am sorry.
Konstantin

> Regards,
> Vadim.
> On Mar 23, 2015 8:44 PM, "Olivier MATZ" <olivier.matz@6wind.com> wrote:
> 
> > Hi Neil,
> >
> > On 03/19/2015 02:16 PM, Neil Horman wrote:
> > >> 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.
> > >
> > >
> > > That last condition is my issue with this patch, that the user has to
> > know what
> > > refcnts are.  It makes this api useful for little more than the test
> > case that
> > > is provided with it.  Its irritating enough that for singly allocated
> > mbufs the
> > > user has to know what the refcount of a buffer is before freeing, but at
> > least
> > > they can macrotize a {rte_pktmbuf_refcnt_update;
> > if(rte_pktmbuf_refct_read) then
> > > free} operation.
> > >
> > > With this, you've placed the user in charge of not only doing that, but
> > also of
> > > managing the relationship between pktmbufs and the pool they came from.
> > while
> > > that makes sense for the test case, it really doesn't in any general use
> > case in
> > > which packet processing is ever deferred or queued, because it means
> > that the
> > > application is now responsible for holding a pointer to every packet it
> > > allocates and checking its refcount periodically until it completes.
> > >
> > > There is never any reason that an application won't need to do this
> > management,
> > > so making it the purview of the application to handle rather than
> > properly
> > > integrating that functionality in the library is really a false savings.
> >
> > There are some places where you know that the prerequisites are met,
> > so you can save cycles by using this function.
> >
> > From what I imagine, if in a driver you allocate mbufs, chain them and
> > for some reason you realize you have to free them, you can use this
> > function instead of freeing them one by one.
> >
> > Also, as it's up to the application to decide how many mbuf pools are
> > created, and whether indirect mbufs are used or not, the application
> > can take the short path of using this function in some conditions.
> >
> > Vadim, maybe you have another reason or use case for adding this
> > function? Could you detail why you need it and how it improves your
> > use case?
> >
> > Regards,
> > Olivier
> >

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

[Vadim Suraev]

Hi, Konstantin,

>Though from my point, such function should be generic as
rte_pktmbuf_free_chain() -
>no special limitations like all mbufs from one pool, refcnt==1, etc.
I misunderstood, I'll rework.
Regards,
 Vadim.

On Tue, Mar 24, 2015 at 1:48 AM, Ananyev, Konstantin <
konstantin.ananyev@intel.com> wrote:

> Hi Vadim,
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Vadim Suraev
> > Sent: Monday, March 23, 2015 5:31 PM
> > To: Olivier MATZ
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2] rte_mbuf: mbuf bulk alloc/free
> functions added + unittest
> >
> > Hi, Olivier,
> > No, I personally need to free a chain using mempool bulk. If I'm not
> > mistaken, it has been proposed by one of reviewers to have lower level
> > function, so it was done (I'm sorry if misunderstood)
>
> Was it me?
> As I remember, I said it would be good to create rte_pktmbuf_bulk_free()
> or rte_pktmbuf_seg_bulk_free() -
> that would free a bulk of mbufs segments in the same manner as
> rte_pktmbuf_free_chain() does:
> count number of consecutive mbufs from the same mempool to be freed and
> then put them back into the pool at one go.
> Such function would be useful inside PMD code.
> In fact we already using analogy of such function inside vPMD TX code.
> Though from my point, such function should be generic as
> rte_pktmbuf_free_chain() -
> no special limitations like all mbufs from one pool, refcnt==1, etc.
> So if it was me who confused you - I am sorry.
> Konstantin
>
> > Regards,
> > Vadim.
> > On Mar 23, 2015 8:44 PM, "Olivier MATZ" <olivier.matz@6wind.com> wrote:
> >
> > > Hi Neil,
> > >
> > > On 03/19/2015 02:16 PM, Neil Horman wrote:
> > > >> 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.
> > > >
> > > >
> > > > That last condition is my issue with this patch, that the user has to
> > > know what
> > > > refcnts are.  It makes this api useful for little more than the test
> > > case that
> > > > is provided with it.  Its irritating enough that for singly allocated
> > > mbufs the
> > > > user has to know what the refcount of a buffer is before freeing,
> but at
> > > least
> > > > they can macrotize a {rte_pktmbuf_refcnt_update;
> > > if(rte_pktmbuf_refct_read) then
> > > > free} operation.
> > > >
> > > > With this, you've placed the user in charge of not only doing that,
> but
> > > also of
> > > > managing the relationship between pktmbufs and the pool they came
> from.
> > > while
> > > > that makes sense for the test case, it really doesn't in any general
> use
> > > case in
> > > > which packet processing is ever deferred or queued, because it means
> > > that the
> > > > application is now responsible for holding a pointer to every packet
> it
> > > > allocates and checking its refcount periodically until it completes.
> > > >
> > > > There is never any reason that an application won't need to do this
> > > management,
> > > > so making it the purview of the application to handle rather than
> > > properly
> > > > integrating that functionality in the library is really a false
> savings.
> > >
> > > There are some places where you know that the prerequisites are met,
> > > so you can save cycles by using this function.
> > >
> > > From what I imagine, if in a driver you allocate mbufs, chain them and
> > > for some reason you realize you have to free them, you can use this
> > > function instead of freeing them one by one.
> > >
> > > Also, as it's up to the application to decide how many mbuf pools are
> > > created, and whether indirect mbufs are used or not, the application
> > > can take the short path of using this function in some conditions.
> > >
> > > Vadim, maybe you have another reason or use case for adding this
> > > function? Could you detail why you need it and how it improves your
> > > use case?
> > >
> > > Regards,
> > > Olivier
> > >
>

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

[Ananyev, Konstantin]

Hi Vadim,
 
> From: Vadim Suraev [mailto:vadim.suraev@gmail.com]
> Sent: Tuesday, March 24, 2015 7:53 AM
> To: Ananyev, Konstantin
> Cc: Olivier MATZ; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added + unittest
> 
> Hi, Konstantin,
> 
> >Though from my point, such function should be generic as rte_pktmbuf_free_chain() -
> >no special limitations like all mbufs from one pool, refcnt==1, etc.
> I misunderstood, I'll rework.

Ok, thanks.
Konstantin

> Regards,
>  Vadim.
> 
> On Tue, Mar 24, 2015 at 1:48 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:
> Hi Vadim,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Vadim Suraev
> > Sent: Monday, March 23, 2015 5:31 PM
> > To: Olivier MATZ
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added + unittest
> >
> > Hi, Olivier,
> > No, I personally need to free a chain using mempool bulk. If I'm not
> > mistaken, it has been proposed by one of reviewers to have lower level
> > function, so it was done (I'm sorry if misunderstood)
> 
> Was it me?
> As I remember, I said it would be good to create rte_pktmbuf_bulk_free() or rte_pktmbuf_seg_bulk_free() -
> that would free a bulk of mbufs segments in the same manner as rte_pktmbuf_free_chain() does:
> count number of consecutive mbufs from the same mempool to be freed and then put them back into the pool at one go.
> Such function would be useful inside PMD code.
> In fact we already using analogy of such function inside vPMD TX code.
> Though from my point, such function should be generic as rte_pktmbuf_free_chain() -
> no special limitations like all mbufs from one pool, refcnt==1, etc.
> So if it was me who confused you - I am sorry.
> Konstantin
> 
> > Regards,
> > Vadim.
> > On Mar 23, 2015 8:44 PM, "Olivier MATZ" <olivier.matz@6wind.com> wrote:
> >
> > > Hi Neil,
> > >
> > > On 03/19/2015 02:16 PM, Neil Horman wrote:
> > > >> 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.
> > > >
> > > >
> > > > That last condition is my issue with this patch, that the user has to
> > > know what
> > > > refcnts are.  It makes this api useful for little more than the test
> > > case that
> > > > is provided with it.  Its irritating enough that for singly allocated
> > > mbufs the
> > > > user has to know what the refcount of a buffer is before freeing, but at
> > > least
> > > > they can macrotize a {rte_pktmbuf_refcnt_update;
> > > if(rte_pktmbuf_refct_read) then
> > > > free} operation.
> > > >
> > > > With this, you've placed the user in charge of not only doing that, but
> > > also of
> > > > managing the relationship between pktmbufs and the pool they came from.
> > > while
> > > > that makes sense for the test case, it really doesn't in any general use
> > > case in
> > > > which packet processing is ever deferred or queued, because it means
> > > that the
> > > > application is now responsible for holding a pointer to every packet it
> > > > allocates and checking its refcount periodically until it completes.
> > > >
> > > > There is never any reason that an application won't need to do this
> > > management,
> > > > so making it the purview of the application to handle rather than
> > > properly
> > > > integrating that functionality in the library is really a false savings.
> > >
> > > There are some places where you know that the prerequisites are met,
> > > so you can save cycles by using this function.
> > >
> > > From what I imagine, if in a driver you allocate mbufs, chain them and
> > > for some reason you realize you have to free them, you can use this
> > > function instead of freeing them one by one.
> > >
> > > Also, as it's up to the application to decide how many mbuf pools are
> > > created, and whether indirect mbufs are used or not, the application
> > > can take the short path of using this function in some conditions.
> > >
> > > Vadim, maybe you have another reason or use case for adding this
> > > function? Could you detail why you need it and how it improves your
> > > use case?
> > >
> > > Regards,
> > > Olivier
> > >

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

[Vadim Suraev]

Hi, Neil

>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 thought again and it looks to me that if mempool_cache is enabled,
rte_pktmbuf_bulk_free and  are redundant because the logic would be very
similar to already implemented in rte_mempool. Probably the only
rte_pktmbuf_alloc_bulk makes sense in this patch?

Regards,
 Vadim.

On Wed, Mar 18, 2015 at 10:58 PM, Neil Horman <nhorman@tuxdriver.com> wrote:

> On Wed, Mar 18, 2015 at 10:21:18PM +0200, vadim.suraev@gmail.com wrote:
> > From: "vadim.suraev@gmail.com" <vadim.suraev@gmail.com>
> >
> > This patch adds mbuf bulk allocation/freeing functions and unittest
> >
> > Signed-off-by: Vadim Suraev
> > <vadim.suraev@gmail.com>
> > ---
> > New in v2:
> >     - function rte_pktmbuf_alloc_bulk added
> >     - function rte_pktmbuf_bulk_free added
> >     - function rte_pktmbuf_free_chain added
> >     - applied reviewers' comments
> >
> >  app/test/test_mbuf.c       |   94
> +++++++++++++++++++++++++++++++++++++++++++-
> >  lib/librte_mbuf/rte_mbuf.h |   91
> ++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 184 insertions(+), 1 deletion(-)
> >
> > diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
> > index 1ff66cb..b20c6a4 100644
> > --- a/app/test/test_mbuf.c
> > +++ b/app/test/test_mbuf.c
> > @@ -77,6 +77,7 @@
> >  #define REFCNT_RING_SIZE        (REFCNT_MBUF_NUM * REFCNT_MAX_REF)
> >
> >  #define MAKE_STRING(x)          # x
> > +#define MBUF_POOL_LOCAL_CACHE_SIZE 32
> >
> >  static struct rte_mempool *pktmbuf_pool = NULL;
> >
> > @@ -405,6 +406,84 @@ test_pktmbuf_pool(void)
> >       return ret;
> >  }
> >
> ><snip>
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index 17ba791..fabeae2 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -825,6 +825,97 @@ static inline void rte_pktmbuf_free(struct rte_mbuf
> *m)
> >  }
> >
> >  /**
> > + * Allocate a bulk of mbufs, initiate refcnt and resets
> > + *
> > + * @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;
> > +
> > +     rc = rte_mempool_get_bulk(pool, (void **)mbufs, count);
> > +     if (unlikely(rc))
> > +             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;
> > +}
> > +
> > +/**
> > + * 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.
>
> Neil
>
>

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

[Neil Horman]

On Mon, Mar 30, 2015 at 10:04:20PM +0300, Vadim Suraev wrote:
> Hi, Neil
> 
> >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 thought again and it looks to me that if mempool_cache is enabled,
> rte_pktmbuf_bulk_free and  are redundant because the logic would be very
> similar to already implemented in rte_mempool. Probably the only
> rte_pktmbuf_alloc_bulk makes sense in this patch?
> 
> Regards,
>  Vadim.
> 
Looking at it, yes, I agree, using an externally allocated large contiguous
block of memory, mapped with rte_mempool_xmem_create, then allocating with
rte_pktmbuf_alloc would likely work in exactly the same way.  I'd argue that
even the bulk alloc function isn't really needed, as its implementation seems
like it would just be a for loop with 2-3 lines in it.

Neil

> On Wed, Mar 18, 2015 at 10:58 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> > On Wed, Mar 18, 2015 at 10:21:18PM +0200, vadim.suraev@gmail.com wrote:
> > > From: "vadim.suraev@gmail.com" <vadim.suraev@gmail.com>
> > >
> > > This patch adds mbuf bulk allocation/freeing functions and unittest
> > >
> > > Signed-off-by: Vadim Suraev
> > > <vadim.suraev@gmail.com>
> > > ---
> > > New in v2:
> > >     - function rte_pktmbuf_alloc_bulk added
> > >     - function rte_pktmbuf_bulk_free added
> > >     - function rte_pktmbuf_free_chain added
> > >     - applied reviewers' comments
> > >
> > >  app/test/test_mbuf.c       |   94
> > +++++++++++++++++++++++++++++++++++++++++++-
> > >  lib/librte_mbuf/rte_mbuf.h |   91
> > ++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 184 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
> > > index 1ff66cb..b20c6a4 100644
> > > --- a/app/test/test_mbuf.c
> > > +++ b/app/test/test_mbuf.c
> > > @@ -77,6 +77,7 @@
> > >  #define REFCNT_RING_SIZE        (REFCNT_MBUF_NUM * REFCNT_MAX_REF)
> > >
> > >  #define MAKE_STRING(x)          # x
> > > +#define MBUF_POOL_LOCAL_CACHE_SIZE 32
> > >
> > >  static struct rte_mempool *pktmbuf_pool = NULL;
> > >
> > > @@ -405,6 +406,84 @@ test_pktmbuf_pool(void)
> > >       return ret;
> > >  }
> > >
> > ><snip>
> > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > > index 17ba791..fabeae2 100644
> > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > @@ -825,6 +825,97 @@ static inline void rte_pktmbuf_free(struct rte_mbuf
> > *m)
> > >  }
> > >
> > >  /**
> > > + * Allocate a bulk of mbufs, initiate refcnt and resets
> > > + *
> > > + * @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;
> > > +
> > > +     rc = rte_mempool_get_bulk(pool, (void **)mbufs, count);
> > > +     if (unlikely(rc))
> > > +             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;
> > > +}
> > > +
> > > +/**
> > > + * 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.
> >
> > Neil
> >
> >

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

[Olivier MATZ]

Hi Konstantin,

On 03/19/2015 11:47 AM, Ananyev, Konstantin wrote:
>>>> Hi, Konstantin,
>>>>
>>>> Got it. To make the same, nulling the next should be inside of the block as you said.
>>>> One question raises here: If a segment in the chain has refcnt > 1 (so its next is not assigned NULL), and the next segment has
>> refcnt
>>>> == 1 (so it is freed), do you think this scenario is real/should be considered? If so, the former can be safely freed only by calling
>>>> rte_pktmbuf_free_seg which does not iterate. So why to keep next pointing to something?
>>>
>>> I think we need it, not just to keep things the same with  rte_pktmbuf_free(), but because it is a right thing to do.
>>> Let say you have a packet in 2 mbufs chained together, both mbufs have refcnt==2.
>>> Then:
>>> rte_pktmbuf_free(firs_mbuf);
>>> rte_pktmbuf_free(firs_mbuf);
>>>
>>> Would work correctly and free both mbufs back to the mempool.
>>>
>>> While after:
>>> rte_pktmbuf_free_chain(first_mbuf);
>>> rte_pktmbuf_free_chain(first_mbuf);
>>>
>>> We would have first_mbuf freed back into the mempool, while second would get lost(memory leaking).
>>> Basically free() shouldn't modify any filed inside mbuf, except refcnt if rte_mbuf_refcnt_update(m, -1) > 0
>>>
>>> About your case, when: first_mbuf->refcnt==2 and second_mbuf->refcnt==1.
>>> Right now, rte_pktmbuf_free() can't handle such cases properly,
>>> and, as I know, such situation is not considered as valid one.
>>
>> I'm not sure I understand what you are saying. To me, the case you are
>> describing is similar to the case below, and it should work properly:
>>
>> 	/* allocate a packet and clone it. After that, m1 has a
>> 	 * refcnt of 2 */
>> 	m1 = rte_pktmbuf_alloc();
>> 	clone1 = rte_pktmbuf_clone(m1);
>>
>> 	/* allocate another packet */
>> 	m2 = rte_pktmbuf_alloc();
>>
>> 	/* chain m2 after m1, updating fields like total length.
>> 	 * After that, m1 has 2 segments, the first one has a refcnt
>> 	 * of 1 and the second has a refcnt of 2 */
>> 	mbuf_concat(m1, m2);
>>
>> 	/* This will decrement the refcnt on the first segment and
>> 	 * free the second segment */
>> 	rte_pktmbuf_free(m1);
>>
>> 	/* free the indirect mbuf, and as the refcnt is 1 on the
>> 	 * direct mbuf (m1), also release it */
>> 	rte_pktmbuf_free(clone1);
>>
>> Am I missing something?
>
> The scenario you described would work I believe,  as second reference for m1 is from indirect mbuf.
> So  rte_pktmbuf_free(clone1) would just call  __rte_mbuf_raw_free(m1).
>
> The scenario I am talking about is:
> No indirect mbufs pointing to m1 data buffer.
> m1->next == m2; m1->refcnt==2;
> m2->next == NULL; m2->rectn==1;
>
> And then:
> rte_pktmbuf_free(m1);  //after that m2 is freed, but m1->next == m2
> rte_pktmbuf_free(m1); //would call rte_pktmbuf_free_seg(m2)
>
> That one would not work correctly, and I think considered as invalid case right now.

Ok, I agree this is invalid and should not happen.

Thanks,
Olivier

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

[Ananyev, Konstantin]

Hi Olivier,

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Thursday, March 19, 2015 8:13 AM
> To: Ananyev, Konstantin; vadim.suraev@gmail.com
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added + unittest
> 
> Hi Konstantin,
> 
> On 03/18/2015 04:13 PM, Ananyev, Konstantin wrote:
> >
> >> From: Vadim Suraev [mailto:vadim.suraev@gmail.com]
> >> Sent: Wednesday, March 18, 2015 10:41 AM
> >> To: Ananyev, Konstantin
> >> Cc: dev@dpdk.org
> >> Subject: Re: [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added + unittest
> >>
> >> Hi, Konstantin,
> >>
> >> Got it. To make the same, nulling the next should be inside of the block as you said.
> >> One question raises here: If a segment in the chain has refcnt > 1 (so its next is not assigned NULL), and the next segment has
> refcnt
> >> == 1 (so it is freed), do you think this scenario is real/should be considered? If so, the former can be safely freed only by calling
> >> rte_pktmbuf_free_seg which does not iterate. So why to keep next pointing to something?
> >
> > I think we need it, not just to keep things the same with  rte_pktmbuf_free(), but because it is a right thing to do.
> > Let say you have a packet in 2 mbufs chained together, both mbufs have refcnt==2.
> > Then:
> > rte_pktmbuf_free(firs_mbuf);
> > rte_pktmbuf_free(firs_mbuf);
> >
> > Would work correctly and free both mbufs back to the mempool.
> >
> > While after:
> > rte_pktmbuf_free_chain(first_mbuf);
> > rte_pktmbuf_free_chain(first_mbuf);
> >
> > We would have first_mbuf freed back into the mempool, while second would get lost(memory leaking).
> > Basically free() shouldn't modify any filed inside mbuf, except refcnt if rte_mbuf_refcnt_update(m, -1) > 0
> >
> > About your case, when: first_mbuf->refcnt==2 and second_mbuf->refcnt==1.
> > Right now, rte_pktmbuf_free() can't handle such cases properly,
> > and, as I know, such situation is not considered as valid one.
> 
> I'm not sure I understand what you are saying. To me, the case you are
> describing is similar to the case below, and it should work properly:
> 
> 	/* allocate a packet and clone it. After that, m1 has a
> 	 * refcnt of 2 */
> 	m1 = rte_pktmbuf_alloc();
> 	clone1 = rte_pktmbuf_clone(m1);
> 
> 	/* allocate another packet */
> 	m2 = rte_pktmbuf_alloc();
> 
> 	/* chain m2 after m1, updating fields like total length.
> 	 * After that, m1 has 2 segments, the first one has a refcnt
> 	 * of 1 and the second has a refcnt of 2 */
> 	mbuf_concat(m1, m2);
> 
> 	/* This will decrement the refcnt on the first segment and
> 	 * free the second segment */
> 	rte_pktmbuf_free(m1);
> 
> 	/* free the indirect mbuf, and as the refcnt is 1 on the
> 	 * direct mbuf (m1), also release it */
> 	rte_pktmbuf_free(clone1);
> 
> Am I missing something?

The scenario you described would work I believe,  as second reference for m1 is from indirect mbuf.
So  rte_pktmbuf_free(clone1) would just call  __rte_mbuf_raw_free(m1).

The scenario I am talking about is:
No indirect mbufs pointing to m1 data buffer.
m1->next == m2; m1->refcnt==2;
m2->next == NULL; m2->rectn==1; 

And then:
rte_pktmbuf_free(m1);  //after that m2 is freed, but m1->next == m2
rte_pktmbuf_free(m1); //would call rte_pktmbuf_free_seg(m2) 

That one would not work correctly, and I think considered as invalid case right now.
Konstantin


> 
> Thanks,
> Olivier

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

[Olivier MATZ]

Hi Konstantin,

On 03/18/2015 04:13 PM, Ananyev, Konstantin wrote:
>
>> From: Vadim Suraev [mailto:vadim.suraev@gmail.com]
>> Sent: Wednesday, March 18, 2015 10:41 AM
>> To: Ananyev, Konstantin
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added + unittest
>>
>> Hi, Konstantin,
>>
>> Got it. To make the same, nulling the next should be inside of the block as you said.
>> One question raises here: If a segment in the chain has refcnt > 1 (so its next is not assigned NULL), and the next segment has refcnt
>> == 1 (so it is freed), do you think this scenario is real/should be considered? If so, the former can be safely freed only by calling
>> rte_pktmbuf_free_seg which does not iterate. So why to keep next pointing to something?
>
> I think we need it, not just to keep things the same with  rte_pktmbuf_free(), but because it is a right thing to do.
> Let say you have a packet in 2 mbufs chained together, both mbufs have refcnt==2.
> Then:
> rte_pktmbuf_free(firs_mbuf);
> rte_pktmbuf_free(firs_mbuf);
>
> Would work correctly and free both mbufs back to the mempool.
>
> While after:
> rte_pktmbuf_free_chain(first_mbuf);
> rte_pktmbuf_free_chain(first_mbuf);
>
> We would have first_mbuf freed back into the mempool, while second would get lost(memory leaking).
> Basically free() shouldn't modify any filed inside mbuf, except refcnt if rte_mbuf_refcnt_update(m, -1) > 0
>
> About your case, when: first_mbuf->refcnt==2 and second_mbuf->refcnt==1.
> Right now, rte_pktmbuf_free() can't handle such cases properly,
> and, as I know, such situation is not considered as valid one.

I'm not sure I understand what you are saying. To me, the case you are
describing is similar to the case below, and it should work properly:

	/* allocate a packet and clone it. After that, m1 has a
	 * refcnt of 2 */
	m1 = rte_pktmbuf_alloc();
	clone1 = rte_pktmbuf_clone(m1);

	/* allocate another packet */
	m2 = rte_pktmbuf_alloc();

	/* chain m2 after m1, updating fields like total length.
	 * After that, m1 has 2 segments, the first one has a refcnt
	 * of 1 and the second has a refcnt of 2 */
	mbuf_concat(m1, m2);

	/* This will decrement the refcnt on the first segment and
	 * free the second segment */
	rte_pktmbuf_free(m1);

	/* free the indirect mbuf, and as the refcnt is 1 on the
	 * direct mbuf (m1), also release it */
	rte_pktmbuf_free(clone1);

Am I missing something?

Thanks,
Olivier

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

[Ananyev, Konstantin]

 
> From: Vadim Suraev [mailto:vadim.suraev@gmail.com]
> Sent: Wednesday, March 18, 2015 10:41 AM
> To: Ananyev, Konstantin
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added + unittest
> 
> Hi, Konstantin,
> 
> Got it. To make the same, nulling the next should be inside of the block as you said.
> One question raises here: If a segment in the chain has refcnt > 1 (so its next is not assigned NULL), and the next segment has refcnt
> == 1 (so it is freed), do you think this scenario is real/should be considered? If so, the former can be safely freed only by calling
> rte_pktmbuf_free_seg which does not iterate. So why to keep next pointing to something?

I think we need it, not just to keep things the same with  rte_pktmbuf_free(), but because it is a right thing to do.
Let say you have a packet in 2 mbufs chained together, both mbufs have refcnt==2.
Then:
rte_pktmbuf_free(firs_mbuf);
rte_pktmbuf_free(firs_mbuf);

Would work correctly and free both mbufs back to the mempool.

While after:
rte_pktmbuf_free_chain(first_mbuf);
rte_pktmbuf_free_chain(first_mbuf);

We would have first_mbuf freed back into the mempool, while second would get lost(memory leaking).
Basically free() shouldn't modify any filed inside mbuf, except refcnt if rte_mbuf_refcnt_update(m, -1) > 0

About your case, when: first_mbuf->refcnt==2 and second_mbuf->refcnt==1.
Right now, rte_pktmbuf_free() can't handle such cases properly,
and, as I know, such situation is not considered as valid one.

Konstantin

> Regards,
>  Vadim
> 
> On Wed, Mar 18, 2015 at 11:56 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:
> 
> Hi Vadim,
> 
> 
> > From: Vadim Suraev [mailto:vadim.suraev@gmail.com]
> > Sent: Wednesday, March 18, 2015 5:19 AM
> > To: Ananyev, Konstantin
> > Cc: dev@dpdk.org; olivier.matz@6wind.com; stephen@networkplumber.org
> > Subject: Re: [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added + unittest
> >
> > Hi, Konstantin,
> >
> > >Shouldn't the line above be inside if (head != NULL) {...} block?
> > This is removed as Olivier commented before:
> >
> > >> +{
> > > +     if (likely(head != NULL)) {
> >
> > >I think we should remove this test. The other mbuf functions do not
> > >check this.
> > Regards,
> >  Vadim.
> 
> I meant that in my opinion it should be:
> 
> while (head) {
>              next = head->next;
> -             head->next = NULL;
> 
>              head = __rte_pktmbuf_prefree_seg(head);
>              if (likely(head != NULL)) {
> +                  head->next = NULL;
>                      RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(head) == 0);
> 
> Same as rte_pktmbuf_free() doing.
> 
> Konstantin
> 
> >
> > On Wed, Mar 18, 2015 at 1:46 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:
> > Hi Vadim,
> >
> > > -----Original Message-----
> > > From: vadim.suraev@gmail.com [mailto:vadim.suraev@gmail.com]
> > > Sent: Tuesday, March 17, 2015 9:36 PM
> > > To: dev@dpdk.org
> > > Cc: olivier.matz@6wind.com; stephen@networkplumber.org; Ananyev, Konstantin; vadim.suraev@gmail.com
> > > Subject: [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added + unittest
> > >
> > > From: "vadim.suraev@gmail.com" <vadim.suraev@gmail.com>
> > >
> > > This patch adds mbuf bulk allocation/freeing functions and unittest
> > >
> > > Signed-off-by: Vadim Suraev
> > > <vadim.suraev@gmail.com>
> > > ---
> > > New in v2:
> > >     - function rte_pktmbuf_alloc_bulk added
> > >     - function rte_pktmbuf_bulk_free added
> > >     - function rte_pktmbuf_free_chain added
> > >     - applied reviewers' comments
> > >
> > >  app/test/test_mbuf.c       |   94 +++++++++++++++++++++++++++++++++++++++++++-
> > >  lib/librte_mbuf/rte_mbuf.h |   89 +++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 182 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
> > > index 1ff66cb..b20c6a4 100644
> > > --- a/app/test/test_mbuf.c
> > > +++ b/app/test/test_mbuf.c
> > > @@ -77,6 +77,7 @@
> > >  #define REFCNT_RING_SIZE        (REFCNT_MBUF_NUM * REFCNT_MAX_REF)
> > >
> > >  #define MAKE_STRING(x)          # x
> > > +#define MBUF_POOL_LOCAL_CACHE_SIZE 32
> > >
> > >  static struct rte_mempool *pktmbuf_pool = NULL;
> > >
> > > @@ -405,6 +406,84 @@ test_pktmbuf_pool(void)
> > >       return ret;
> > >  }
> > >
> > > +/* test pktmbuf bulk allocation and freeing
> > > +*/
> > > +static int
> > > +test_pktmbuf_pool_bulk(void)
> > > +{
> > > +     unsigned i;
> > > +     /* size of mempool - size of local cache, otherwise may fail */
> > > +     unsigned mbufs_to_allocate = NB_MBUF - MBUF_POOL_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-MBUF_POOL_LOCAL_CACHE_SIZE mbufs */
> > > +     ret = rte_pktmbuf_alloc_bulk(pktmbuf_pool, m, mbufs_to_allocate);
> > > +     if (ret) {
> > > +             printf("cannot allocate %d mbufs bulk mempool_cnt=%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;
> > > +     }
> > > +     /* 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-MBUF_POOL_LOCAL_CACHE_SIZE mbufs */
> > > +     ret = rte_pktmbuf_alloc_bulk(pktmbuf_pool, m, mbufs_to_allocate);
> > > +     if (ret) {
> > > +             printf("cannot allocate %d mbufs bulk mempool_cnt=%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
> > >   */
> > > @@ -766,7 +845,8 @@ test_mbuf(void)
> > >       if (pktmbuf_pool == NULL) {
> > >               pktmbuf_pool =
> > >                       rte_mempool_create("test_pktmbuf_pool", NB_MBUF,
> > > -                                        MBUF_SIZE, 32,
> > > +                                        MBUF_SIZE,
> > > +                                        MBUF_POOL_LOCAL_CACHE_SIZE,
> > >                                          sizeof(struct rte_pktmbuf_pool_private),
> > >                                          rte_pktmbuf_pool_init, NULL,
> > >                                          rte_pktmbuf_init, NULL,
> > > @@ -790,6 +870,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..995237d 100644
> > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > @@ -825,6 +825,95 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
> > >  }
> > >
> > >  /**
> > > + * Allocate a bulk of mbufs, initiate refcnt and resets
> > > + *
> > > + * @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;
> > > +
> > > +     rc = rte_mempool_get_bulk(pool, (void **)mbufs, count);
> > > +     if (unlikely(rc))
> > > +             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;
> > > +}
> > > +
> > > +/**
> > > + * Free a bulk of mbufs into its original mempool.
> > > + * This function assumes refcnt equals 1
> > > + * as well as the freed mbufs are direct
> > I think your forgot to mention in comments one more requirement for that function:
> > all mbufs have to be from 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_refcnt_update(mbufs[idx], -1);
> >
> > You can do just:
> > rte_mbuf_refcnt_set(m, 0);
> > here and move your assert above it.
> > Something like:
> > RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 1);
> > rte_mbuf_refcnt_set(m, 0);
> >
> > Also probably would be a good thing to add one more assert here,
> > something like:
> > RTE_MBUF_ASSERT(mbufs[idx]->pool == mufs[0]->pool);
> >
> > > +             RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > > +     }
> > > +     rte_mempool_put_bulk(mbufs[0]->pool, (void **)mbufs, count);
> > > +}
> > > +
> > > +/**
> > > + * Free chained (scattered) mbufs into its original mempool(s).
> > > + *
> > > + * @param head
> > > + *    The head of mbufs to be freed chain. Must not be NULL
> > > + */
> > > +static inline void rte_pktmbuf_free_chain(struct rte_mbuf *head)
> > > +{
> > > +     struct rte_mbuf *mbufs[head->nb_segs];
> > > +     unsigned mbufs_count = 0;
> > > +     struct rte_mbuf *next;
> > > +
> > > +     while (head) {
> > > +             next = head->next;
> > > +             head->next = NULL;
> >
> > Shouldn't the line above be inside if (head != NULL) {...} block?
> >
> > > +             head = __rte_pktmbuf_prefree_seg(head);
> > > +             if (likely(head != NULL)) {
> > > +                     RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(head) == 0);
> >
> > I don't think there is any use of the assert above.
> > If prefree_seg returns non-NULL value, it sets refcnt to 0 for that mbuf.
> >
> > > +                     if (likely((!mbufs_count) ||
> > > +                                (head->pool == mbufs[0]->pool)))
> > > +                             mbufs[mbufs_count++] = head;
> > > +                     else {
> > > +                             rte_mempool_put_bulk(mbufs[0]->pool,
> > > +                                                  (void **)mbufs,
> > > +                                                  mbufs_count);
> > > +                             mbufs_count = 0;
> > > +                     }
> > > +             }
> > > +             head = next;
> > > +     }
> > > +     if (mbufs_count > 0)
> > > +             rte_mempool_put_bulk(mbufs[0]->pool,
> > > +                                  (void **)mbufs,
> > > +                                  mbufs_count);
> > > +}
> > > +
> > > +/**
> > >   * Creates a "clone" of the given packet mbuf.
> > >   *
> > >   * Walks through all segments of the given packet mbuf, and for each of them:
> > > --
> > > 1.7.9.5

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

[Vadim Suraev]

Hi, Konstantin,

Got it. To make the same, nulling the next should be inside of the block as
you said.
One question raises here: If a segment in the chain has refcnt > 1 (so its
next is not assigned NULL), and the next segment has refcnt == 1 (so it is
freed), do you think this scenario is real/should be considered? If so, the
former can be safely freed only by calling rte_pktmbuf_free_seg which does
not iterate. So why to keep next pointing to something?

Regards,
 Vadim

On Wed, Mar 18, 2015 at 11:56 AM, Ananyev, Konstantin <
konstantin.ananyev@intel.com> wrote:

>
> Hi Vadim,
>
>
> > From: Vadim Suraev [mailto:vadim.suraev@gmail.com]
> > Sent: Wednesday, March 18, 2015 5:19 AM
> > To: Ananyev, Konstantin
> > Cc: dev@dpdk.org; olivier.matz@6wind.com; stephen@networkplumber.org
> > Subject: Re: [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added +
> unittest
> >
> > Hi, Konstantin,
> >
> > >Shouldn't the line above be inside if (head != NULL) {...} block?
> > This is removed as Olivier commented before:
> >
> > >> +{
> > > +     if (likely(head != NULL)) {
> >
> > >I think we should remove this test. The other mbuf functions do not
> > >check this.
> > Regards,
> >  Vadim.
>
> I meant that in my opinion it should be:
>
> while (head) {
>              next = head->next;
> -             head->next = NULL;
>
>              head = __rte_pktmbuf_prefree_seg(head);
>              if (likely(head != NULL)) {
> +                  head->next = NULL;
>                      RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(head) == 0);
>
> Same as rte_pktmbuf_free() doing.
>
> Konstantin
>
> >
> > On Wed, Mar 18, 2015 at 1:46 AM, Ananyev, Konstantin <
> konstantin.ananyev@intel.com> wrote:
> > Hi Vadim,
> >
> > > -----Original Message-----
> > > From: vadim.suraev@gmail.com [mailto:vadim.suraev@gmail.com]
> > > Sent: Tuesday, March 17, 2015 9:36 PM
> > > To: dev@dpdk.org
> > > Cc: olivier.matz@6wind.com; stephen@networkplumber.org; Ananyev,
> Konstantin; vadim.suraev@gmail.com
> > > Subject: [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added +
> unittest
> > >
> > > From: "vadim.suraev@gmail.com" <vadim.suraev@gmail.com>
> > >
> > > This patch adds mbuf bulk allocation/freeing functions and unittest
> > >
> > > Signed-off-by: Vadim Suraev
> > > <vadim.suraev@gmail.com>
> > > ---
> > > New in v2:
> > >     - function rte_pktmbuf_alloc_bulk added
> > >     - function rte_pktmbuf_bulk_free added
> > >     - function rte_pktmbuf_free_chain added
> > >     - applied reviewers' comments
> > >
> > >  app/test/test_mbuf.c       |   94
> +++++++++++++++++++++++++++++++++++++++++++-
> > >  lib/librte_mbuf/rte_mbuf.h |   89
> +++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 182 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
> > > index 1ff66cb..b20c6a4 100644
> > > --- a/app/test/test_mbuf.c
> > > +++ b/app/test/test_mbuf.c
> > > @@ -77,6 +77,7 @@
> > >  #define REFCNT_RING_SIZE        (REFCNT_MBUF_NUM * REFCNT_MAX_REF)
> > >
> > >  #define MAKE_STRING(x)          # x
> > > +#define MBUF_POOL_LOCAL_CACHE_SIZE 32
> > >
> > >  static struct rte_mempool *pktmbuf_pool = NULL;
> > >
> > > @@ -405,6 +406,84 @@ test_pktmbuf_pool(void)
> > >       return ret;
> > >  }
> > >
> > > +/* test pktmbuf bulk allocation and freeing
> > > +*/
> > > +static int
> > > +test_pktmbuf_pool_bulk(void)
> > > +{
> > > +     unsigned i;
> > > +     /* size of mempool - size of local cache, otherwise may fail */
> > > +     unsigned mbufs_to_allocate = NB_MBUF -
> MBUF_POOL_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-MBUF_POOL_LOCAL_CACHE_SIZE mbufs */
> > > +     ret = rte_pktmbuf_alloc_bulk(pktmbuf_pool, m, mbufs_to_allocate);
> > > +     if (ret) {
> > > +             printf("cannot allocate %d mbufs bulk mempool_cnt=%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;
> > > +     }
> > > +     /* 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-MBUF_POOL_LOCAL_CACHE_SIZE mbufs */
> > > +     ret = rte_pktmbuf_alloc_bulk(pktmbuf_pool, m, mbufs_to_allocate);
> > > +     if (ret) {
> > > +             printf("cannot allocate %d mbufs bulk mempool_cnt=%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
> > >   */
> > > @@ -766,7 +845,8 @@ test_mbuf(void)
> > >       if (pktmbuf_pool == NULL) {
> > >               pktmbuf_pool =
> > >                       rte_mempool_create("test_pktmbuf_pool", NB_MBUF,
> > > -                                        MBUF_SIZE, 32,
> > > +                                        MBUF_SIZE,
> > > +                                        MBUF_POOL_LOCAL_CACHE_SIZE,
> > >                                          sizeof(struct
> rte_pktmbuf_pool_private),
> > >                                          rte_pktmbuf_pool_init, NULL,
> > >                                          rte_pktmbuf_init, NULL,
> > > @@ -790,6 +870,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..995237d 100644
> > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > @@ -825,6 +825,95 @@ static inline void rte_pktmbuf_free(struct
> rte_mbuf *m)
> > >  }
> > >
> > >  /**
> > > + * Allocate a bulk of mbufs, initiate refcnt and resets
> > > + *
> > > + * @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;
> > > +
> > > +     rc = rte_mempool_get_bulk(pool, (void **)mbufs, count);
> > > +     if (unlikely(rc))
> > > +             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;
> > > +}
> > > +
> > > +/**
> > > + * Free a bulk of mbufs into its original mempool.
> > > + * This function assumes refcnt equals 1
> > > + * as well as the freed mbufs are direct
> > I think your forgot to mention in comments one more requirement for that
> function:
> > all mbufs have to be from 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_refcnt_update(mbufs[idx], -1);
> >
> > You can do just:
> > rte_mbuf_refcnt_set(m, 0);
> > here and move your assert above it.
> > Something like:
> > RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 1);
> > rte_mbuf_refcnt_set(m, 0);
> >
> > Also probably would be a good thing to add one more assert here,
> > something like:
> > RTE_MBUF_ASSERT(mbufs[idx]->pool == mufs[0]->pool);
> >
> > > +             RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > > +     }
> > > +     rte_mempool_put_bulk(mbufs[0]->pool, (void **)mbufs, count);
> > > +}
> > > +
> > > +/**
> > > + * Free chained (scattered) mbufs into its original mempool(s).
> > > + *
> > > + * @param head
> > > + *    The head of mbufs to be freed chain. Must not be NULL
> > > + */
> > > +static inline void rte_pktmbuf_free_chain(struct rte_mbuf *head)
> > > +{
> > > +     struct rte_mbuf *mbufs[head->nb_segs];
> > > +     unsigned mbufs_count = 0;
> > > +     struct rte_mbuf *next;
> > > +
> > > +     while (head) {
> > > +             next = head->next;
> > > +             head->next = NULL;
> >
> > Shouldn't the line above be inside if (head != NULL) {...} block?
> >
> > > +             head = __rte_pktmbuf_prefree_seg(head);
> > > +             if (likely(head != NULL)) {
> > > +                     RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(head) == 0);
> >
> > I don't think there is any use of the assert above.
> > If prefree_seg returns non-NULL value, it sets refcnt to 0 for that mbuf.
> >
> > > +                     if (likely((!mbufs_count) ||
> > > +                                (head->pool == mbufs[0]->pool)))
> > > +                             mbufs[mbufs_count++] = head;
> > > +                     else {
> > > +                             rte_mempool_put_bulk(mbufs[0]->pool,
> > > +                                                  (void **)mbufs,
> > > +                                                  mbufs_count);
> > > +                             mbufs_count = 0;
> > > +                     }
> > > +             }
> > > +             head = next;
> > > +     }
> > > +     if (mbufs_count > 0)
> > > +             rte_mempool_put_bulk(mbufs[0]->pool,
> > > +                                  (void **)mbufs,
> > > +                                  mbufs_count);
> > > +}
> > > +
> > > +/**
> > >   * Creates a "clone" of the given packet mbuf.
> > >   *
> > >   * Walks through all segments of the given packet mbuf, and for each
> of them:
> > > --
> > > 1.7.9.5
>
>

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

[Ananyev, Konstantin]

Hi Vadim,

 
> From: Vadim Suraev [mailto:vadim.suraev@gmail.com]
> Sent: Wednesday, March 18, 2015 5:19 AM
> To: Ananyev, Konstantin
> Cc: dev@dpdk.org; olivier.matz@6wind.com; stephen@networkplumber.org
> Subject: Re: [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added + unittest
> 
> Hi, Konstantin,
> 
> >Shouldn't the line above be inside if (head != NULL) {...} block?
> This is removed as Olivier commented before:
> 
> >> +{
> > +     if (likely(head != NULL)) {
> 
> >I think we should remove this test. The other mbuf functions do not
> >check this.
> Regards,
>  Vadim.

I meant that in my opinion it should be:

while (head) {
             next = head->next;
-             head->next = NULL;
 
             head = __rte_pktmbuf_prefree_seg(head);
             if (likely(head != NULL)) {
+                  head->next = NULL;           
                     RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(head) == 0);

Same as rte_pktmbuf_free() doing.

Konstantin

> 
> On Wed, Mar 18, 2015 at 1:46 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:
> Hi Vadim,
> 
> > -----Original Message-----
> > From: vadim.suraev@gmail.com [mailto:vadim.suraev@gmail.com]
> > Sent: Tuesday, March 17, 2015 9:36 PM
> > To: dev@dpdk.org
> > Cc: olivier.matz@6wind.com; stephen@networkplumber.org; Ananyev, Konstantin; vadim.suraev@gmail.com
> > Subject: [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added + unittest
> >
> > From: "vadim.suraev@gmail.com" <vadim.suraev@gmail.com>
> >
> > This patch adds mbuf bulk allocation/freeing functions and unittest
> >
> > Signed-off-by: Vadim Suraev
> > <vadim.suraev@gmail.com>
> > ---
> > New in v2:
> >     - function rte_pktmbuf_alloc_bulk added
> >     - function rte_pktmbuf_bulk_free added
> >     - function rte_pktmbuf_free_chain added
> >     - applied reviewers' comments
> >
> >  app/test/test_mbuf.c       |   94 +++++++++++++++++++++++++++++++++++++++++++-
> >  lib/librte_mbuf/rte_mbuf.h |   89 +++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 182 insertions(+), 1 deletion(-)
> >
> > diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
> > index 1ff66cb..b20c6a4 100644
> > --- a/app/test/test_mbuf.c
> > +++ b/app/test/test_mbuf.c
> > @@ -77,6 +77,7 @@
> >  #define REFCNT_RING_SIZE        (REFCNT_MBUF_NUM * REFCNT_MAX_REF)
> >
> >  #define MAKE_STRING(x)          # x
> > +#define MBUF_POOL_LOCAL_CACHE_SIZE 32
> >
> >  static struct rte_mempool *pktmbuf_pool = NULL;
> >
> > @@ -405,6 +406,84 @@ test_pktmbuf_pool(void)
> >       return ret;
> >  }
> >
> > +/* test pktmbuf bulk allocation and freeing
> > +*/
> > +static int
> > +test_pktmbuf_pool_bulk(void)
> > +{
> > +     unsigned i;
> > +     /* size of mempool - size of local cache, otherwise may fail */
> > +     unsigned mbufs_to_allocate = NB_MBUF - MBUF_POOL_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-MBUF_POOL_LOCAL_CACHE_SIZE mbufs */
> > +     ret = rte_pktmbuf_alloc_bulk(pktmbuf_pool, m, mbufs_to_allocate);
> > +     if (ret) {
> > +             printf("cannot allocate %d mbufs bulk mempool_cnt=%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;
> > +     }
> > +     /* 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-MBUF_POOL_LOCAL_CACHE_SIZE mbufs */
> > +     ret = rte_pktmbuf_alloc_bulk(pktmbuf_pool, m, mbufs_to_allocate);
> > +     if (ret) {
> > +             printf("cannot allocate %d mbufs bulk mempool_cnt=%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
> >   */
> > @@ -766,7 +845,8 @@ test_mbuf(void)
> >       if (pktmbuf_pool == NULL) {
> >               pktmbuf_pool =
> >                       rte_mempool_create("test_pktmbuf_pool", NB_MBUF,
> > -                                        MBUF_SIZE, 32,
> > +                                        MBUF_SIZE,
> > +                                        MBUF_POOL_LOCAL_CACHE_SIZE,
> >                                          sizeof(struct rte_pktmbuf_pool_private),
> >                                          rte_pktmbuf_pool_init, NULL,
> >                                          rte_pktmbuf_init, NULL,
> > @@ -790,6 +870,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..995237d 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -825,6 +825,95 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
> >  }
> >
> >  /**
> > + * Allocate a bulk of mbufs, initiate refcnt and resets
> > + *
> > + * @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;
> > +
> > +     rc = rte_mempool_get_bulk(pool, (void **)mbufs, count);
> > +     if (unlikely(rc))
> > +             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;
> > +}
> > +
> > +/**
> > + * Free a bulk of mbufs into its original mempool.
> > + * This function assumes refcnt equals 1
> > + * as well as the freed mbufs are direct
> I think your forgot to mention in comments one more requirement for that function:
> all mbufs have to be from 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_refcnt_update(mbufs[idx], -1);
> 
> You can do just:
> rte_mbuf_refcnt_set(m, 0);
> here and move your assert above it.
> Something like:
> RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 1);
> rte_mbuf_refcnt_set(m, 0);
> 
> Also probably would be a good thing to add one more assert here,
> something like:
> RTE_MBUF_ASSERT(mbufs[idx]->pool == mufs[0]->pool);
> 
> > +             RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > +     }
> > +     rte_mempool_put_bulk(mbufs[0]->pool, (void **)mbufs, count);
> > +}
> > +
> > +/**
> > + * Free chained (scattered) mbufs into its original mempool(s).
> > + *
> > + * @param head
> > + *    The head of mbufs to be freed chain. Must not be NULL
> > + */
> > +static inline void rte_pktmbuf_free_chain(struct rte_mbuf *head)
> > +{
> > +     struct rte_mbuf *mbufs[head->nb_segs];
> > +     unsigned mbufs_count = 0;
> > +     struct rte_mbuf *next;
> > +
> > +     while (head) {
> > +             next = head->next;
> > +             head->next = NULL;
> 
> Shouldn't the line above be inside if (head != NULL) {...} block?
> 
> > +             head = __rte_pktmbuf_prefree_seg(head);
> > +             if (likely(head != NULL)) {
> > +                     RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(head) == 0);
> 
> I don't think there is any use of the assert above.
> If prefree_seg returns non-NULL value, it sets refcnt to 0 for that mbuf.
> 
> > +                     if (likely((!mbufs_count) ||
> > +                                (head->pool == mbufs[0]->pool)))
> > +                             mbufs[mbufs_count++] = head;
> > +                     else {
> > +                             rte_mempool_put_bulk(mbufs[0]->pool,
> > +                                                  (void **)mbufs,
> > +                                                  mbufs_count);
> > +                             mbufs_count = 0;
> > +                     }
> > +             }
> > +             head = next;
> > +     }
> > +     if (mbufs_count > 0)
> > +             rte_mempool_put_bulk(mbufs[0]->pool,
> > +                                  (void **)mbufs,
> > +                                  mbufs_count);
> > +}
> > +
> > +/**
> >   * Creates a "clone" of the given packet mbuf.
> >   *
> >   * Walks through all segments of the given packet mbuf, and for each of them:
> > --
> > 1.7.9.5

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

[Vadim Suraev]

Hi, Konstantin,

>Shouldn't the line above be inside if (head != NULL) {...} block?

This is removed as Olivier commented before:

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

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

Regards,
 Vadim.

On Wed, Mar 18, 2015 at 1:46 AM, Ananyev, Konstantin <
konstantin.ananyev@intel.com> wrote:

> Hi Vadim,
>
> > -----Original Message-----
> > From: vadim.suraev@gmail.com [mailto:vadim.suraev@gmail.com]
> > Sent: Tuesday, March 17, 2015 9:36 PM
> > To: dev@dpdk.org
> > Cc: olivier.matz@6wind.com; stephen@networkplumber.org; Ananyev,
> Konstantin; vadim.suraev@gmail.com
> > Subject: [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added +
> unittest
> >
> > From: "vadim.suraev@gmail.com" <vadim.suraev@gmail.com>
> >
> > This patch adds mbuf bulk allocation/freeing functions and unittest
> >
> > Signed-off-by: Vadim Suraev
> > <vadim.suraev@gmail.com>
> > ---
> > New in v2:
> >     - function rte_pktmbuf_alloc_bulk added
> >     - function rte_pktmbuf_bulk_free added
> >     - function rte_pktmbuf_free_chain added
> >     - applied reviewers' comments
> >
> >  app/test/test_mbuf.c       |   94
> +++++++++++++++++++++++++++++++++++++++++++-
> >  lib/librte_mbuf/rte_mbuf.h |   89
> +++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 182 insertions(+), 1 deletion(-)
> >
> > diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
> > index 1ff66cb..b20c6a4 100644
> > --- a/app/test/test_mbuf.c
> > +++ b/app/test/test_mbuf.c
> > @@ -77,6 +77,7 @@
> >  #define REFCNT_RING_SIZE        (REFCNT_MBUF_NUM * REFCNT_MAX_REF)
> >
> >  #define MAKE_STRING(x)          # x
> > +#define MBUF_POOL_LOCAL_CACHE_SIZE 32
> >
> >  static struct rte_mempool *pktmbuf_pool = NULL;
> >
> > @@ -405,6 +406,84 @@ test_pktmbuf_pool(void)
> >       return ret;
> >  }
> >
> > +/* test pktmbuf bulk allocation and freeing
> > +*/
> > +static int
> > +test_pktmbuf_pool_bulk(void)
> > +{
> > +     unsigned i;
> > +     /* size of mempool - size of local cache, otherwise may fail */
> > +     unsigned mbufs_to_allocate = NB_MBUF - MBUF_POOL_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-MBUF_POOL_LOCAL_CACHE_SIZE mbufs */
> > +     ret = rte_pktmbuf_alloc_bulk(pktmbuf_pool, m, mbufs_to_allocate);
> > +     if (ret) {
> > +             printf("cannot allocate %d mbufs bulk mempool_cnt=%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;
> > +     }
> > +     /* 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-MBUF_POOL_LOCAL_CACHE_SIZE mbufs */
> > +     ret = rte_pktmbuf_alloc_bulk(pktmbuf_pool, m, mbufs_to_allocate);
> > +     if (ret) {
> > +             printf("cannot allocate %d mbufs bulk mempool_cnt=%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
> >   */
> > @@ -766,7 +845,8 @@ test_mbuf(void)
> >       if (pktmbuf_pool == NULL) {
> >               pktmbuf_pool =
> >                       rte_mempool_create("test_pktmbuf_pool", NB_MBUF,
> > -                                        MBUF_SIZE, 32,
> > +                                        MBUF_SIZE,
> > +                                        MBUF_POOL_LOCAL_CACHE_SIZE,
> >                                          sizeof(struct
> rte_pktmbuf_pool_private),
> >                                          rte_pktmbuf_pool_init, NULL,
> >                                          rte_pktmbuf_init, NULL,
> > @@ -790,6 +870,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..995237d 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -825,6 +825,95 @@ static inline void rte_pktmbuf_free(struct rte_mbuf
> *m)
> >  }
> >
> >  /**
> > + * Allocate a bulk of mbufs, initiate refcnt and resets
> > + *
> > + * @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;
> > +
> > +     rc = rte_mempool_get_bulk(pool, (void **)mbufs, count);
> > +     if (unlikely(rc))
> > +             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;
> > +}
> > +
> > +/**
> > + * Free a bulk of mbufs into its original mempool.
> > + * This function assumes refcnt equals 1
> > + * as well as the freed mbufs are direct
>
> I think your forgot to mention in comments one more requirement for that
> function:
> all mbufs have to be from 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_refcnt_update(mbufs[idx], -1);
>
> You can do just:
> rte_mbuf_refcnt_set(m, 0);
> here and move your assert above it.
> Something like:
> RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 1);
> rte_mbuf_refcnt_set(m, 0);
>
> Also probably would be a good thing to add one more assert here,
> something like:
> RTE_MBUF_ASSERT(mbufs[idx]->pool == mufs[0]->pool);
>
> > +             RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > +     }
> > +     rte_mempool_put_bulk(mbufs[0]->pool, (void **)mbufs, count);
> > +}
> > +
> > +/**
> > + * Free chained (scattered) mbufs into its original mempool(s).
> > + *
> > + * @param head
> > + *    The head of mbufs to be freed chain. Must not be NULL
> > + */
> > +static inline void rte_pktmbuf_free_chain(struct rte_mbuf *head)
> > +{
> > +     struct rte_mbuf *mbufs[head->nb_segs];
> > +     unsigned mbufs_count = 0;
> > +     struct rte_mbuf *next;
> > +
> > +     while (head) {
> > +             next = head->next;
> > +             head->next = NULL;
>
> Shouldn't the line above be inside if (head != NULL) {...} block?
>
> > +             head = __rte_pktmbuf_prefree_seg(head);
> > +             if (likely(head != NULL)) {
> > +                     RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(head) == 0);
>
> I don't think there is any use of the assert above.
> If prefree_seg returns non-NULL value, it sets refcnt to 0 for that mbuf.
>
> > +                     if (likely((!mbufs_count) ||
> > +                                (head->pool == mbufs[0]->pool)))
> > +                             mbufs[mbufs_count++] = head;
> > +                     else {
> > +                             rte_mempool_put_bulk(mbufs[0]->pool,
> > +                                                  (void **)mbufs,
> > +                                                  mbufs_count);
> > +                             mbufs_count = 0;
> > +                     }
> > +             }
> > +             head = next;
> > +     }
> > +     if (mbufs_count > 0)
> > +             rte_mempool_put_bulk(mbufs[0]->pool,
> > +                                  (void **)mbufs,
> > +                                  mbufs_count);
> > +}
> > +
> > +/**
> >   * Creates a "clone" of the given packet mbuf.
> >   *
> >   * Walks through all segments of the given packet mbuf, and for each of
> them:
> > --
> > 1.7.9.5
>
>

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

[Ananyev, Konstantin]

Hi Vadim,

> -----Original Message-----
> From: vadim.suraev@gmail.com [mailto:vadim.suraev@gmail.com]
> Sent: Tuesday, March 17, 2015 9:36 PM
> To: dev@dpdk.org
> Cc: olivier.matz@6wind.com; stephen@networkplumber.org; Ananyev, Konstantin; vadim.suraev@gmail.com
> Subject: [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added + unittest
> 
> From: "vadim.suraev@gmail.com" <vadim.suraev@gmail.com>
> 
> This patch adds mbuf bulk allocation/freeing functions and unittest
> 
> Signed-off-by: Vadim Suraev
> <vadim.suraev@gmail.com>
> ---
> New in v2:
>     - function rte_pktmbuf_alloc_bulk added
>     - function rte_pktmbuf_bulk_free added
>     - function rte_pktmbuf_free_chain added
>     - applied reviewers' comments
> 
>  app/test/test_mbuf.c       |   94 +++++++++++++++++++++++++++++++++++++++++++-
>  lib/librte_mbuf/rte_mbuf.h |   89 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 182 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
> index 1ff66cb..b20c6a4 100644
> --- a/app/test/test_mbuf.c
> +++ b/app/test/test_mbuf.c
> @@ -77,6 +77,7 @@
>  #define REFCNT_RING_SIZE        (REFCNT_MBUF_NUM * REFCNT_MAX_REF)
> 
>  #define MAKE_STRING(x)          # x
> +#define MBUF_POOL_LOCAL_CACHE_SIZE 32
> 
>  static struct rte_mempool *pktmbuf_pool = NULL;
> 
> @@ -405,6 +406,84 @@ test_pktmbuf_pool(void)
>  	return ret;
>  }
> 
> +/* test pktmbuf bulk allocation and freeing
> +*/
> +static int
> +test_pktmbuf_pool_bulk(void)
> +{
> +	unsigned i;
> +	/* size of mempool - size of local cache, otherwise may fail */
> +	unsigned mbufs_to_allocate = NB_MBUF - MBUF_POOL_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-MBUF_POOL_LOCAL_CACHE_SIZE mbufs */
> +	ret = rte_pktmbuf_alloc_bulk(pktmbuf_pool, m, mbufs_to_allocate);
> +	if (ret) {
> +		printf("cannot allocate %d mbufs bulk mempool_cnt=%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;
> +	}
> +	/* 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-MBUF_POOL_LOCAL_CACHE_SIZE mbufs */
> +	ret = rte_pktmbuf_alloc_bulk(pktmbuf_pool, m, mbufs_to_allocate);
> +	if (ret) {
> +		printf("cannot allocate %d mbufs bulk mempool_cnt=%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
>   */
> @@ -766,7 +845,8 @@ test_mbuf(void)
>  	if (pktmbuf_pool == NULL) {
>  		pktmbuf_pool =
>  			rte_mempool_create("test_pktmbuf_pool", NB_MBUF,
> -					   MBUF_SIZE, 32,
> +					   MBUF_SIZE,
> +					   MBUF_POOL_LOCAL_CACHE_SIZE,
>  					   sizeof(struct rte_pktmbuf_pool_private),
>  					   rte_pktmbuf_pool_init, NULL,
>  					   rte_pktmbuf_init, NULL,
> @@ -790,6 +870,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..995237d 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -825,6 +825,95 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
>  }
> 
>  /**
> + * Allocate a bulk of mbufs, initiate refcnt and resets
> + *
> + * @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;
> +
> +	rc = rte_mempool_get_bulk(pool, (void **)mbufs, count);
> +	if (unlikely(rc))
> +		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;
> +}
> +
> +/**
> + * Free a bulk of mbufs into its original mempool.
> + * This function assumes refcnt equals 1
> + * as well as the freed mbufs are direct

I think your forgot to mention in comments one more requirement for that function:
all mbufs have to be from 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_refcnt_update(mbufs[idx], -1);

You can do just:
rte_mbuf_refcnt_set(m, 0);
here and move your assert above it.
Something like:
RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 1);
rte_mbuf_refcnt_set(m, 0);

Also probably would be a good thing to add one more assert here,
something like:
RTE_MBUF_ASSERT(mbufs[idx]->pool == mufs[0]->pool);

> +		RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> +	}
> +	rte_mempool_put_bulk(mbufs[0]->pool, (void **)mbufs, count);
> +}
> +
> +/**
> + * Free chained (scattered) mbufs into its original mempool(s).
> + *
> + * @param head
> + *    The head of mbufs to be freed chain. Must not be NULL
> + */
> +static inline void rte_pktmbuf_free_chain(struct rte_mbuf *head)
> +{
> +	struct rte_mbuf *mbufs[head->nb_segs];
> +	unsigned mbufs_count = 0;
> +	struct rte_mbuf *next;
> +
> +	while (head) {
> +		next = head->next;
> +		head->next = NULL;

Shouldn't the line above be inside if (head != NULL) {...} block?

> +		head = __rte_pktmbuf_prefree_seg(head);
> +		if (likely(head != NULL)) {
> +			RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(head) == 0);

I don't think there is any use of the assert above.
If prefree_seg returns non-NULL value, it sets refcnt to 0 for that mbuf.

> +			if (likely((!mbufs_count) ||
> +				   (head->pool == mbufs[0]->pool)))
> +				mbufs[mbufs_count++] = head;
> +			else {
> +				rte_mempool_put_bulk(mbufs[0]->pool,
> +						     (void **)mbufs,
> +						     mbufs_count);
> +				mbufs_count = 0;
> +			}
> +		}
> +		head = next;
> +	}
> +	if (mbufs_count > 0)
> +		rte_mempool_put_bulk(mbufs[0]->pool,
> +				     (void **)mbufs,
> +				     mbufs_count);
> +}
> +
> +/**
>   * Creates a "clone" of the given packet mbuf.
>   *
>   * Walks through all segments of the given packet mbuf, and for each of them:
> --
> 1.7.9.5

[dpdk-dev] [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added + unittest

[vadim.suraev]

From: "vadim.suraev@gmail.com" <vadim.suraev@gmail.com>

This patch adds mbuf bulk allocation/freeing functions and unittest

Signed-off-by: Vadim Suraev
<vadim.suraev@gmail.com>
---
New in v2:
    - function rte_pktmbuf_alloc_bulk added
    - function rte_pktmbuf_bulk_free added
    - function rte_pktmbuf_free_chain added
    - applied reviewers' comments

 app/test/test_mbuf.c       |   94 +++++++++++++++++++++++++++++++++++++++++++-
 lib/librte_mbuf/rte_mbuf.h |   89 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 182 insertions(+), 1 deletion(-)

diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index 1ff66cb..b20c6a4 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -77,6 +77,7 @@
 #define REFCNT_RING_SIZE        (REFCNT_MBUF_NUM * REFCNT_MAX_REF)
 
 #define MAKE_STRING(x)          # x
+#define MBUF_POOL_LOCAL_CACHE_SIZE 32
 
 static struct rte_mempool *pktmbuf_pool = NULL;
 
@@ -405,6 +406,84 @@ test_pktmbuf_pool(void)
 	return ret;
 }
 
+/* test pktmbuf bulk allocation and freeing
+*/
+static int
+test_pktmbuf_pool_bulk(void)
+{
+	unsigned i;
+	/* size of mempool - size of local cache, otherwise may fail */
+	unsigned mbufs_to_allocate = NB_MBUF - MBUF_POOL_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-MBUF_POOL_LOCAL_CACHE_SIZE mbufs */
+	ret = rte_pktmbuf_alloc_bulk(pktmbuf_pool, m, mbufs_to_allocate);
+	if (ret) {
+		printf("cannot allocate %d mbufs bulk mempool_cnt=%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;
+	}
+	/* 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-MBUF_POOL_LOCAL_CACHE_SIZE mbufs */
+	ret = rte_pktmbuf_alloc_bulk(pktmbuf_pool, m, mbufs_to_allocate);
+	if (ret) {
+		printf("cannot allocate %d mbufs bulk mempool_cnt=%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
  */
@@ -766,7 +845,8 @@ test_mbuf(void)
 	if (pktmbuf_pool == NULL) {
 		pktmbuf_pool =
 			rte_mempool_create("test_pktmbuf_pool", NB_MBUF,
-					   MBUF_SIZE, 32,
+					   MBUF_SIZE,
+					   MBUF_POOL_LOCAL_CACHE_SIZE,
 					   sizeof(struct rte_pktmbuf_pool_private),
 					   rte_pktmbuf_pool_init, NULL,
 					   rte_pktmbuf_init, NULL,
@@ -790,6 +870,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..995237d 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -825,6 +825,95 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
 }
 
 /**
+ * Allocate a bulk of mbufs, initiate refcnt and resets
+ *
+ * @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;
+
+	rc = rte_mempool_get_bulk(pool, (void **)mbufs, count);
+	if (unlikely(rc))
+		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;
+}
+
+/**
+ * Free a bulk of mbufs into its original mempool.
+ * This function assumes refcnt equals 1
+ * as well as the freed mbufs are direct
+ *
+ * @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_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);
+}
+
+/**
+ * Free chained (scattered) mbufs into its original mempool(s).
+ *
+ * @param head
+ *    The head of mbufs to be freed chain. Must not be NULL
+ */
+static inline void rte_pktmbuf_free_chain(struct rte_mbuf *head)
+{
+	struct rte_mbuf *mbufs[head->nb_segs];
+	unsigned mbufs_count = 0;
+	struct rte_mbuf *next;
+
+	while (head) {
+		next = head->next;
+		head->next = NULL;
+		head = __rte_pktmbuf_prefree_seg(head);
+		if (likely(head != NULL)) {
+			RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(head) == 0);
+			if (likely((!mbufs_count) ||
+				   (head->pool == mbufs[0]->pool)))
+				mbufs[mbufs_count++] = head;
+			else {
+				rte_mempool_put_bulk(mbufs[0]->pool,
+						     (void **)mbufs,
+						     mbufs_count);
+				mbufs_count = 0;
+			}
+		}
+		head = next;
+	}
+	if (mbufs_count > 0)
+		rte_mempool_put_bulk(mbufs[0]->pool,
+				     (void **)mbufs,
+				     mbufs_count);
+}
+
+/**
  * Creates a "clone" of the given packet mbuf.
  *
  * Walks through all segments of the given packet mbuf, and for each of them:
-- 
1.7.9.5