From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ob0-f177.google.com (mail-ob0-f177.google.com [209.85.214.177]) by dpdk.org (Postfix) with ESMTP id 2CEFD9AA0 for ; Tue, 17 Mar 2015 21:22:24 +0100 (CET) Received: by obcxo2 with SMTP id xo2so16763038obc.0 for ; Tue, 17 Mar 2015 13:22:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=fjV7rF8GsDRwC7dKXHjcA+85QsFOXnAnBk52uGvYvgw=; b=f7cZDZh5oe8GKac3oFDH5jT5p3evGeWgu2sujC9O5vknDEPkrMemEc4QgtNbdLNYo6 msarPQGh+ADyqBWBL3LS90qQznmen1FaPR1HwEtAEgGNjFCxTTz6jG/mcukkSTy9eAor rE6x46FikJjV2cwvyfxz9ZdnXHcocu048q0/wgiheRZ/h5J+Y7S+JtQKqz/ZW4B3gydl XrbRAEQinzxFzFksrYPTNF+1jy8ugjoJlykEHLcpvD7/I0H7ZcnKFv8uyHb5MJXZkz64 PtpzpTws1J4gsGlS9NYS7/wJ/lUMJr/kEAIs023aU6wQSqQcwwII0tMeIXk/yp/vUPTx su5Q== MIME-Version: 1.0 X-Received: by 10.60.112.65 with SMTP id io1mr44961140oeb.66.1426623743632; Tue, 17 Mar 2015 13:22:23 -0700 (PDT) Received: by 10.202.105.138 with HTTP; Tue, 17 Mar 2015 13:22:23 -0700 (PDT) In-Reply-To: <5506A763.1030209@6wind.com> References: <1426241664-26458-1-git-send-email-vadim.suraev@gmail.com> <5506A763.1030209@6wind.com> Date: Tue, 17 Mar 2015 22:22:23 +0200 Message-ID: From: Vadim Suraev To: Olivier MATZ Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH] rte_mbuf: bulk allocation and freeing functions + unittest X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Mar 2015 20:22:24 -0000 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 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" > > > > - 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 > > --- > > 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 > + 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 > + 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 >