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?

Regards,
 Vadim

On Wed, Mar 18, 2015 at 11:56 AM, Ananyev, Konstantin <
konstantin.ananyev-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:

Hi Vadim,

quoted

From: Vadim Suraev [mailto:vadim.suraev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org]
Sent: Wednesday, March 18, 2015 5:19 AM
To: Ananyev, Konstantin
Cc: dev-VfR2kkLFssw@public.gmane.org; olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org; stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org
Subject: Re: [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added +

unittest

quoted

Hi, Konstantin,

quoted

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

This is removed as Olivier commented before:

quoted

quoted

+{

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

quoted

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

quoted

On Wed, Mar 18, 2015 at 1:46 AM, Ananyev, Konstantin <

konstantin.ananyev-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:

quoted

Hi Vadim,

quoted

-----Original Message-----
From: vadim.suraev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org [mailto:vadim.suraev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org]
Sent: Tuesday, March 17, 2015 9:36 PM
To: dev-VfR2kkLFssw@public.gmane.org
Cc: olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org; stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org; Ananyev,

Konstantin; vadim.suraev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

quoted

quoted

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

unittest

quoted

quoted

From: "vadim.suraev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org" <redacted>

This patch adds mbuf bulk allocation/freeing functions and unittest

Signed-off-by: Vadim Suraev
[off-list ref]
---
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

+++++++++++++++++++++++++++++++++++++++++++-

quoted

quoted

 lib/librte_mbuf/rte_mbuf.h |   89

+++++++++++++++++++++++++++++++++++++++++

quoted

quoted

 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;

quoted

quoted

+     struct rte_mbuf *m[mbufs_to_allocate];
+     int ret = 0;
+     unsigned mbuf_count_before_allocation =

rte_mempool_count(pktmbuf_pool);

quoted

quoted

+
+     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",

quoted

quoted

+                     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) {

quoted

quoted

+             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",

quoted

quoted

+                     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) {

quoted

quoted

+             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),

quoted

quoted

                                         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 */

quoted

quoted

      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)

quoted

quoted

 }

 /**
+ * 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:

quoted

all mbufs have to be from the same mempool.

quoted

+ *
+ * @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);

quoted

+             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?

quoted

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

quoted

+                     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:

quoted

quoted

--
1.7.9.5