From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrien Mazarguil Subject: Re: [PATCH 2/2] mempool: fix pages computation to determine number of objects Date: Tue, 26 May 2015 11:14:18 +0200 Message-ID: <20150526091418.GI26795@6wind.com> References: <1432571266-25840-1-git-send-email-adrien.mazarguil@6wind.com> <1432571266-25840-2-git-send-email-adrien.mazarguil@6wind.com> <2601191342CEEE43887BDE71AB97725821431892@irsmsx105.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "dev@dpdk.org" To: "Ananyev, Konstantin" Return-path: Received: from mail-wg0-f48.google.com (mail-wg0-f48.google.com [74.125.82.48]) by dpdk.org (Postfix) with ESMTP id D848D3B5 for ; Tue, 26 May 2015 11:14:24 +0200 (CEST) Received: by wgme6 with SMTP id e6so22576074wgm.2 for ; Tue, 26 May 2015 02:14:24 -0700 (PDT) Content-Disposition: inline In-Reply-To: <2601191342CEEE43887BDE71AB97725821431892@irsmsx105.ger.corp.intel.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Mon, May 25, 2015 at 06:20:03PM +0000, Ananyev, Konstantin wrote: > Hi Adrien, Hi Konstantin, > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Adrien Mazarguil > > Sent: Monday, May 25, 2015 5:28 PM > > To: dev@dpdk.org > > Subject: [dpdk-dev] [PATCH 2/2] mempool: fix pages computation to determine number of objects > > > > In rte_mempool_obj_iter(), even when a single page is required per object, > > a loop checks that the the next page is contiguous and drops the first one > > otherwise. This commit checks subsequent pages only when several are > > required per object. > > > > Also a minor fix for the amount of remaining space that prevents using the > > entire region. > > > > Fixes: 148f963fb532 ("xen: core library changes") > > > > Signed-off-by: Adrien Mazarguil > > --- > > lib/librte_mempool/rte_mempool.c | 11 ++++++++--- > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c > > index d1a02a2..3c1efec 100644 > > --- a/lib/librte_mempool/rte_mempool.c > > +++ b/lib/librte_mempool/rte_mempool.c > > @@ -175,12 +175,17 @@ rte_mempool_obj_iter(void *vaddr, uint32_t elt_num, size_t elt_sz, size_t align, > > pgn += j; > > > > /* do we have enough space left for the next element. */ > > - if (pgn >= pg_num) > > + if (pgn > pg_num) > > break; > > Hmm, that doesn't look right. > Suppose: > start==0; end==5120; pg_shift==12; pg_num == 1; > So: > pgn = 1; // (5120>>12)-(0>>12) > > And we end-up accessing element that is beyond allocated memory. > > > > > - for (k = j; > > + /* > > + * Compute k so that (k - j) is the number of contiguous > > + * pages starting from index j. Note that there is at least > > + * one page. > > + */ > > + for (k = j + 1; > > k != pgn && > > - paddr[k] + pg_sz == paddr[k + 1]; > > + paddr[k - 1] + pg_sz == paddr[k]; > > k++) > > ; > > > Again, suppose: > j==0; start==0; end==2048; pg_shift==12; pg_num == 2; > So: > pgn = 0; > k = 1; > and the loop goes beyond paddr[] boundaries. > > The problem here, I think that you treat pgn as number of pages, while it is index of last page to be used. Well, I misunderstood the logic here, to me pgn was the number of pages necessary for the current object on top of the number of pages used so far. Assuming a single object uses at least one page (assuming 4K pages), pgn wasn't supposed to be zero. > As I understand, what you are trying to fix here, is a situation when end is a multiply of page size (end == N * pg_sz), right? This and also when (end - start) < page size. > Then, probably something simple like that would do: > > - pgn = (end >> pg_shift) - (start >> pg_shift); > + pgn = (end - 1 >> pg_shift) - (start >> pg_shift); > + pg_next = (end >> pg_shift) - (start >> pg_shift); > ... > if (k == pgn) { > if (obj_iter != NULL) > obj_iter(obj_iter_arg, (void *)start, > (void *)end, i); > va = end; > - j = pgn; > + j = pg_next; > i++; > } else { > ... That does not seem to be enough to solve the issue in my scenario, I get weird results (j never reaches pg_num). I'll come up with a new patch that takes your comment into account, hopefully covering all cases. Thanks, -- Adrien Mazarguil 6WIND