From: Olivier MATZ <olivier.matz@6wind.com>
To: "vadim.suraev@gmail.com" <vadim.suraev@gmail.com>, dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] rte_mbuf: bulk allocation and freeing functions + unittest
Date: Mon, 16 Mar 2015 10:50:27 +0100 [thread overview]
Message-ID: <5506A763.1030209@6wind.com> (raw)
In-Reply-To: <1426241664-26458-1-git-send-email-vadim.suraev@gmail.com>
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
next prev parent reply other threads:[~2015-03-16 9:50 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-13 10:14 vadim.suraev
2015-03-16 9:50 ` Olivier MATZ [this message]
2015-03-17 20:22 ` Vadim Suraev
2015-03-17 21:40 ` Vadim Suraev
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5506A763.1030209@6wind.com \
--to=olivier.matz@6wind.com \
--cc=dev@dpdk.org \
--cc=vadim.suraev@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).