[dpdk-dev] rte_mbuf: bulk allocation and freeing functions + unittest
Commit Message
From: "vadim.suraev@gmail.com" <vadim.suraev@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@gmail.com <vadim.suraev@gmail.com>
---
app/test/test_mbuf.c | 73 ++++++++++++++++++++++++++++++++
lib/librte_mbuf/rte_mbuf.h | 101 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 174 insertions(+)
Comments
Hi Vadim,
Please see some comments below.
On 03/13/2015 11:14 AM, vadim.suraev@gmail.com wrote:
> From: "vadim.suraev@gmail.com" <vadim.suraev@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@gmail.com <vadim.suraev@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
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@6wind.com>
wrote:
> Hi Vadim,
>
> Please see some comments below.
>
> On 03/13/2015 11:14 AM, vadim.suraev@gmail.com wrote:
> > From: "vadim.suraev@gmail.com" <vadim.suraev@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@gmail.com <vadim.suraev@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
>
>>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@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@6wind.com>
> wrote:
>
>> Hi Vadim,
>>
>> Please see some comments below.
>>
>> On 03/13/2015 11:14 AM, vadim.suraev@gmail.com wrote:
>> > From: "vadim.suraev@gmail.com" <vadim.suraev@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@gmail.com <vadim.suraev@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
>>
>
>
@@ -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 */
+ 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;
+ }
+ /* 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");
@@ -825,6 +825,107 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
}
/**
+ * Allocates a bulk of mbufs, initiates 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;
+
+ 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;
+ rte_mempool_put_bulk(mbufs[0]->pool, (void **)mbufs, count);
+}
+
+/**
+ * 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
+ *
+ * @param mbufs
+ * Array of pointers to mbuf
+ * @param count
+ * Array size
+ */
+
+static inline void rte_pktmbuf_bulk_free(struct rte_mbuf **mbufs, unsigned count)
+{
+ if (unlikely(count == 0))
+ return;
+ unsigned idx;
+
+ 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)
+{
+ if (likely(head != NULL)) {
+ 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)))) {
+ 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);
+ }
+}
+
+/**
* Creates a "clone" of the given packet mbuf.
*
* Walks through all segments of the given packet mbuf, and for each of them: