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

Vadim Suraev vadim.suraev at gmail.com
Tue Mar 17 22:40:05 CET 2015


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

>I changed to 'assumes refcnt equals 0'

1. I changed to 'assumes refcnt equals 1'
2. I have a doubt about manipulating refcnt in this function. Should it be
the only check/assert or should it be responsibility of the caller?

Regards,
 Vadim.

On Tue, Mar 17, 2015 at 10:22 PM, Vadim Suraev <vadim.suraev at gmail.com>
wrote:

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


More information about the dev mailing list