From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi0-f43.google.com (mail-oi0-f43.google.com [209.85.218.43]) by dpdk.org (Postfix) with ESMTP id 7CBD19AA4 for ; Tue, 17 Mar 2015 22:40:06 +0100 (CET) Received: by oier21 with SMTP id r21so20679868oie.1 for ; Tue, 17 Mar 2015 14:40:05 -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=TV6/kOE0NNe6f+54gjAwxVVepJKKuxc5O7OH0dS60wo=; b=vtPtPW9bBLlqRLMvWehYUSzWiRqmgPdoqAFS/MsEssSziLddaOy2X0uUXfyjPaRfN7 C6/Wfd2z3yG5G5yg4FmadLTe6j5JkG3r4+9Is2sulb8u12cMCK4Rws9fV4osOFPyFnyj YS4q2MndtKSWG0NzRtKIJEEzk1QhQaZVM/6iecDaE4g8oE9iFpvigAWxmTdh6fA91EK9 5nyI7UY14YkF6+rF7K+IiqeHAfCu5DbjcKcPjz6WmQ2lXHrXgxPRJf5KCGxioagz/Pt4 12HS+IQYEENtCUA+V4ju+IxlMCMSB/u4C3hXqZiVVlZFLCUqE6lpGai51B92CfAzXa1s Si2Q== MIME-Version: 1.0 X-Received: by 10.182.97.41 with SMTP id dx9mr55167031obb.4.1426628405869; Tue, 17 Mar 2015 14:40:05 -0700 (PDT) Received: by 10.202.105.138 with HTTP; Tue, 17 Mar 2015 14:40:05 -0700 (PDT) In-Reply-To: References: <1426241664-26458-1-git-send-email-vadim.suraev@gmail.com> <5506A763.1030209@6wind.com> Date: Tue, 17 Mar 2015 23:40:05 +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 21:40:07 -0000 >>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 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 > 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 >> > >