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

[Vadim Suraev]

Hi, Olivier,

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

I changed to 'assumes refcnt equals 0'

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

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

raw* functions in this patch seem to be redundant, removed it.

Regarding the rest of comments, applied and re-posted the patch.
Regards,
 Vadim.

On Mon, Mar 16, 2015 at 11:50 AM, Olivier MATZ <olivier.matz at 6wind.com>
wrote:

> Hi Vadim,
>
> Please see some comments below.
>
> On 03/13/2015 11:14 AM, vadim.suraev at gmail.com wrote:
> > From: "vadim.suraev at gmail.com" <vadim.suraev at gmail.com>
> >
> > - an API function to allocate a bulk of rte_mbufs
> > - an API function to free a bulk of rte_mbufs
> > - an API function to free a chained rte_mbuf
> > - unittest for aboce
> >
> > Signed-off-by: vadim.suraev at gmail.com <vadim.suraev at gmail.com>
> > ---
> >  app/test/test_mbuf.c       |   73 ++++++++++++++++++++++++++++++++
> >  lib/librte_mbuf/rte_mbuf.h |  101
> ++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 174 insertions(+)
> >
> > diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
> > index 1ff66cb..a557705 100644
> > --- a/app/test/test_mbuf.c
> > +++ b/app/test/test_mbuf.c
> > @@ -405,6 +405,67 @@ test_pktmbuf_pool(void)
> >       return ret;
> >  }
> >
> > +/* test pktmbuf bulk allocation and freeing
> > +*/
> > +static int
> > +test_pktmbuf_pool_bulk(void)
> > +{
> > +     unsigned i;
> > +     unsigned mbufs_to_allocate = NB_MBUF - 32; /* size of mempool -
> size of local cache, otherwise may fail */
>
> Can you add a constant for local cache size?
>
>
> > +     struct rte_mbuf *m[mbufs_to_allocate];
> > +     int ret = 0;
> > +     unsigned mbuf_count_before_allocation =
> rte_mempool_count(pktmbuf_pool);
> > +
> > +     for (i=0; i<mbufs_to_allocate; i++)
> > +             m[i] = NULL;
> > +     /* alloc NB_MBUF-32 mbufs */
> > +     if ((ret = rte_pktmbuf_alloc_bulk(pktmbuf_pool, m,
> mbufs_to_allocate))) {
> > +             printf("cannot allocate %d mbufs bulk mempool_count=%d
> ret=%d\n", mbufs_to_allocate, rte_mempool_count(pktmbuf_pool), ret);
> > +             return -1;
> > +     }
> > +     if ((rte_mempool_count(pktmbuf_pool) + mbufs_to_allocate) !=
> mbuf_count_before_allocation) {
> > +             printf("mempool count %d + allocated %d != initial %d\n",
> > +                     rte_mempool_count(pktmbuf_pool),
> mbufs_to_allocate, mbuf_count_before_allocation);
> > +             return -1;
> > +     }
>
> Could you verify your modifications with checkpatch? It will triggers
> warnings for lines exceeding 80 columns or missing spaces around
> operators (even though it's like this in the rest of the file).
>
>
> > +     /* free them */
> > +     rte_pktmbuf_bulk_free(m, mbufs_to_allocate);
> > +
> > +     if (rte_mempool_count(pktmbuf_pool)  !=
> mbuf_count_before_allocation) {
> > +             printf("mempool count %d != initial %d\n",
> > +                     rte_mempool_count(pktmbuf_pool),
> mbuf_count_before_allocation);
> > +             return -1;
> > +     }
> > +     for (i=0; i<mbufs_to_allocate; i++)
> > +             m[i] = NULL;
> > +
> > +     /* alloc NB_MBUF-32 mbufs */
> > +     if ((ret = rte_pktmbuf_alloc_bulk(pktmbuf_pool, m,
> mbufs_to_allocate))) {
> > +             printf("cannot allocate %d mbufs bulk mempool_count=%d
> ret=%d\n", mbufs_to_allocate, rte_mempool_count(pktmbuf_pool), ret);
> > +             return -1;
> > +     }
> > +     if ((rte_mempool_count(pktmbuf_pool) + mbufs_to_allocate) !=
> mbuf_count_before_allocation) {
> > +             printf("mempool count %d + allocated %d != initial %d\n",
> > +                     rte_mempool_count(pktmbuf_pool),
> mbufs_to_allocate, mbuf_count_before_allocation);
> > +             return -1;
> > +     }
> > +
> > +     /* chain it */
> > +     for (i=0; i< mbufs_to_allocate - 1; i++) {
> > +             m[i]->next = m[i + 1];
> > +             m[0]->nb_segs++;
> > +     }
> > +     /* free them */
> > +     rte_pktmbuf_free_chain(m[0]);
> > +
> > +     if (rte_mempool_count(pktmbuf_pool)  !=
> mbuf_count_before_allocation) {
> > +             printf("mempool count %d != initial %d\n",
> > +                     rte_mempool_count(pktmbuf_pool),
> mbuf_count_before_allocation);
> > +             return -1;
> > +     }
> > +     return ret;
> > +}
> > +
> >  /*
> >   * test that the pointer to the data on a packet mbuf is set properly
> >   */
> > @@ -790,6 +851,18 @@ test_mbuf(void)
> >               return -1;
> >       }
> >
> > +     /* test bulk allocation and freeing */
> > +     if (test_pktmbuf_pool_bulk() < 0) {
> > +             printf("test_pktmbuf_pool_bulk() failed\n");
> > +             return -1;
> > +     }
> > +
> > +     /* once again to ensure all mbufs were freed */
> > +     if (test_pktmbuf_pool_bulk() < 0) {
> > +             printf("test_pktmbuf_pool_bulk() failed\n");
> > +             return -1;
> > +     }
> > +
> >       /* test that the pointer to the data on a packet mbuf is set
> properly */
> >       if (test_pktmbuf_pool_ptr() < 0) {
> >               printf("test_pktmbuf_pool_ptr() failed\n");
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index 17ba791..482920e 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -825,6 +825,107 @@ static inline void rte_pktmbuf_free(struct
> rte_mbuf *m)
> >  }
> >
> >  /**
> > + * Allocates a bulk of mbufs, initiates refcnt and resets
>
> For API comments, we try use the imperative form (no "s" at the end).
> This applies to all comments of the patch.
>
>
>
> > + *
> > + * @param pool
> > + *    memory pool to allocate from
> > + * @param mbufs
> > + *    Array of pointers to mbuf
> > + * @param count
> > + *    Array size
> > + */
> > +static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
> struct rte_mbuf **mbufs, unsigned count)
> > +{
> > +     unsigned idx;
> > +     int rc = 0;
> > +
> > +     if (unlikely(rc = rte_mempool_get_bulk(pool, (void **)mbufs,
> count))) {
> > +             return rc;
> > +     }
> > +
> > +     for (idx = 0; idx < count; idx++) {
> > +             RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > +             rte_mbuf_refcnt_set(mbufs[idx], 1);
> > +             rte_pktmbuf_reset(mbufs[idx]);
> > +     }
> > +     return rc;
> > +}
> > +
> > +/**
> > + * Frees a bulk of mbufs into its original mempool.
> > + * It is responsibility of caller to update and check refcnt
> > + * as well as to check for attached mbufs to be freed
> > + *
> > + * @param mbufs
> > + *    Array of pointers to mbuf
> > + * @param count
> > + *    Array size
> > + */
> > +
> > +static inline void rte_pktmbuf_bulk_raw_free(struct rte_mbuf **mbufs,
> unsigned count)
> > +{
> > +     if (unlikely(count == 0))
> > +             return;
>
> Should we really test this? The mbuf layer should be as fast as
> possible and should avoid tests when they are not necessary. I
> would prefer a comment (+ an assert ?) saying count must be
> strictly positive.
>
>
> > +     rte_mempool_put_bulk(mbufs[0]->pool, (void **)mbufs, count);
> > +}
>
> Adding this function is not a problem today because it is the free
> function associated to rte_pktmbuf_bulk_raw_alloc().
>
> However, I think that the 'raw' alloc/free functions should be removed
> in the future as they are just wrappers to mempool_get/put. There is
> a problem with that today because the raw alloc also sets the refcnt,
> but I think it could go in pktmbuf_reset(). I plan to do that for dpdk
> 2.1, what do you think?
>
> Another thing: the fact that the mbufs should belong to the same pool
> should be clearly noticed in the comment, as it can lead to really
> important bugs. By the way, these bugs wouldn't happen with mempool
> API because we have to specify the mempool pointer, so the user is
> aware that the mempool may not be the same for all mbufs.
>
>
> > +
> > +/**
> > + * Frees a bulk of mbufs into its original mempool.
> > + * This function assumes refcnt has been already decremented
> > + * as well as the freed mbufs are direct
>
> I don't understand the "assumes refcnt has been already decremented".
>
>
> > + *
> > + * @param mbufs
> > + *    Array of pointers to mbuf
> > + * @param count
> > + *    Array size
> > + */
> > +
> > +static inline void rte_pktmbuf_bulk_free(struct rte_mbuf **mbufs,
> unsigned count)
>
> empty line here
>
>
> > +{
> > +     if (unlikely(count == 0))
> > +             return;
> > +     unsigned idx;
> > +
>
> I know it's allowed by C99, but I prefer to have variable declarations
> at the beginning of a block.
>
>
> > +     for(idx = 0; idx < count; idx++) {
> > +             rte_mbuf_refcnt_update(mbufs[idx], -1);
> > +             RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 0);
> > +     }
> > +     rte_mempool_put_bulk(mbufs[0]->pool, (void **)mbufs, count);
> > +}
> > +
> > +/**
> > + * Frees chained (scattered) mbufs into its original mempool(s).
> > + *
> > + * @param head
> > + *    The head of mbufs to be freed chain
> > + */
> > +
> > +static inline void rte_pktmbuf_free_chain(struct rte_mbuf *head)
>
> empty line above
>
>
> > +{
> > +     if (likely(head != NULL)) {
>
> I think we should remove this test. The other mbuf functions do not
> check this.
>
>
> > +             struct rte_mbuf *mbufs[head->nb_segs];
> > +             unsigned mbufs_count = 0;
> > +             struct rte_mbuf *next;
> > +
> > +             while (head) {
> > +                     next = head->next;
> > +                     head->next = NULL;
> > +                     if (likely(NULL != (head =
> __rte_pktmbuf_prefree_seg(head)))) {
>
> replacing the last line by:
>
> head = __rte_pktmbuf_prefree_seg(head);
> if (likely(head != NULL)) {
>
> Would be easier to read.
>
>
>
> > +                             RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(head)
> == 0);
> > +                             if (likely((!mbufs_count) || (head->pool
> == mbufs[0]->pool)))
> > +                                     mbufs[mbufs_count++] = head;
> > +                             else {
> > +                                     rte_pktmbuf_bulk_raw_free(mbufs,
> mbufs_count);
> > +                                     mbufs_count = 0;
> > +                             }
> > +                     }
> > +                     head = next;
> > +             }
> > +             rte_pktmbuf_bulk_raw_free(mbufs, mbufs_count);
>
> If the test "if (unlikely(count == 0))" is removed in
> rte_pktmbuf_bulk_raw_free(), it should be added here.
>
>
> Regards,
> Olivier
>