On Mon, May 25, 2015 at 06:20:03PM +0000, Ananyev, Konstantin wrote: > Hi Adrien,
Hi Konstantin, > > -----Original Message----- > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Adrien Mazarguil > > Sent: Monday, May 25, 2015 5:28 PM > > To: dev at 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 <adrien.mazarguil at 6wind.com> > > --- > > 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
