[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