[dpdk-dev] [PATCH 1/2] mempool: fix returned value on 64 bit after counting objects

[dpdk-dev] [PATCH 1/2] mempool: fix returned value on 64 bit after counting objects

[Adrien Mazarguil]

rte_mempool_xmem_usage()'s return type is ssize_t which has the same
architecture-dependent width as size_t but is signed.

On 64-bit architectures, returning a negative uint32_t value without casting
to ssize_t first does not work as intended, the sign bit is lost and the
returned value is garbage.

This commit fixes an assertion failure in testpmd on 64 bit architectures
when combining --no-huge and --mp-anon outside of Xen Dom0:

 PANIC in mempool_anon_create():
 line 170        assert "elt_num == mp->size" failed

Fixes: 148f963fb532 ("xen: core library changes")

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 lib/librte_mempool/rte_mempool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index 01972ba..d1a02a2 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -361,7 +361,7 @@ rte_mempool_xmem_usage(void *vaddr, uint32_t elt_num, size_t elt_sz,
 	if ((n = rte_mempool_obj_iter(vaddr, elt_num, elt_sz, 1,
 			paddr, pg_num, pg_shift, mempool_lelem_iter,
 			&uv)) != elt_num) {
-		return (-n);
+		return (-(ssize_t)n);
 	}
 
 	uv = RTE_ALIGN_CEIL(uv, pg_sz);
-- 
2.1.0

[dpdk-dev] [PATCH 2/2] mempool: fix pages computation to determine number of objects

[Adrien Mazarguil]

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@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;
 
-		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++)
 			;
 
-- 
2.1.0

Re: [dpdk-dev] [PATCH 2/2] mempool: fix pages computation to determine number of objects

[Ananyev, Konstantin]

Hi Adrien,

> -----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 <adrien.mazarguil@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.
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?
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 {
...


Konstantin


> 
> --
> 2.1.0

Re: [dpdk-dev] [PATCH 2/2] mempool: fix pages computation to determine number of objects

[Adrien Mazarguil]

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 <adrien.mazarguil@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

[dpdk-dev] [PATCHv2] mempool: fix pages computation to determine number of objects

[Konstantin Ananyev]

v2:
- As suggested in comments use slightly different approach for the fix.

In rte_mempool_obj_iter(), when element boundary coincides with page boundary,
even if a single page is required per object, a loop checks that the next page
is contiguous and drops the first oneo therwise.
This commit checks subsequent pages only when several are required per object.

Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
 lib/librte_mempool/rte_mempool.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index 01972ba..ecb03b3 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -156,7 +156,7 @@ rte_mempool_obj_iter(void *vaddr, uint32_t elt_num, size_t elt_sz, size_t align,
 	rte_mempool_obj_iter_t obj_iter, void *obj_iter_arg)
 {
 	uint32_t i, j, k;
-	uint32_t pgn;
+	uint32_t pgn, pgf;
 	uintptr_t end, start, va;
 	uintptr_t pg_sz;
 
@@ -171,10 +171,14 @@ rte_mempool_obj_iter(void *vaddr, uint32_t elt_num, size_t elt_sz, size_t align,
 		start = RTE_ALIGN_CEIL(va, align);
 		end = start + elt_sz;
 
-		pgn = (end >> pg_shift) - (start >> pg_shift);
+		/* index of the first page for the next element. */
+		pgf = (end >> pg_shift) - (start >> pg_shift);
+
+		/* index of the last page for the current element. */
+		pgn = ((end - 1) >> pg_shift) - (start >> pg_shift);
 		pgn += j;
 
-		/* do we have enough space left for the next element. */
+		/* do we have enough space left for the element. */
 		if (pgn >= pg_num)
 			break;
 
@@ -194,7 +198,7 @@ rte_mempool_obj_iter(void *vaddr, uint32_t elt_num, size_t elt_sz, size_t align,
 				obj_iter(obj_iter_arg, (void *)start,
 					(void *)end, i);
 			va = end;
-			j = pgn;
+			j += pgf;
 			i++;
 		} else {
 			va = RTE_ALIGN_CEIL((va + 1), pg_sz);
-- 
1.8.5.3

Re: [dpdk-dev] [PATCHv2] mempool: fix pages computation to determine number of objects

[Adrien Mazarguil]

On Wed, May 27, 2015 at 01:41:09AM +0100, Konstantin Ananyev wrote:
> v2:
> - As suggested in comments use slightly different approach for the fix.
> 
> In rte_mempool_obj_iter(), when element boundary coincides with page boundary,
> even if a single page is required per object, a loop checks that the next page
> is contiguous and drops the first oneo therwise.
> 
> This commit checks subsequent pages only when several are required per object.
> 
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Tested and validated patch which supersedes my previous attempt in the other
thread. There's only a typo in the commit log ("oneo therwise").

Reviewed-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

> ---
>  lib/librte_mempool/rte_mempool.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> index 01972ba..ecb03b3 100644
> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -156,7 +156,7 @@ rte_mempool_obj_iter(void *vaddr, uint32_t elt_num, size_t elt_sz, size_t align,
>  	rte_mempool_obj_iter_t obj_iter, void *obj_iter_arg)
>  {
>  	uint32_t i, j, k;
> -	uint32_t pgn;
> +	uint32_t pgn, pgf;
>  	uintptr_t end, start, va;
>  	uintptr_t pg_sz;
>  
> @@ -171,10 +171,14 @@ rte_mempool_obj_iter(void *vaddr, uint32_t elt_num, size_t elt_sz, size_t align,
>  		start = RTE_ALIGN_CEIL(va, align);
>  		end = start + elt_sz;
>  
> -		pgn = (end >> pg_shift) - (start >> pg_shift);
> +		/* index of the first page for the next element. */
> +		pgf = (end >> pg_shift) - (start >> pg_shift);
> +
> +		/* index of the last page for the current element. */
> +		pgn = ((end - 1) >> pg_shift) - (start >> pg_shift);
>  		pgn += j;
>  
> -		/* do we have enough space left for the next element. */
> +		/* do we have enough space left for the element. */
>  		if (pgn >= pg_num)
>  			break;
>  
> @@ -194,7 +198,7 @@ rte_mempool_obj_iter(void *vaddr, uint32_t elt_num, size_t elt_sz, size_t align,
>  				obj_iter(obj_iter_arg, (void *)start,
>  					(void *)end, i);
>  			va = end;
> -			j = pgn;
> +			j += pgf;
>  			i++;
>  		} else {
>  			va = RTE_ALIGN_CEIL((va + 1), pg_sz);
> -- 
> 1.8.5.3
> 

-- 
Adrien Mazarguil
6WIND

[dpdk-dev] [PATCHv3] mempool: fix pages computation to determine number of objects

[Konstantin Ananyev]

v3:
- Fixed typo in the commit message.

v2:
- As suggested in comments use slightly different approach for the fix.

In rte_mempool_obj_iter(), when element boundary coincides with page boundary,
even if a single page is required per object, a loop checks that the next page
is contiguous and drops the first one otherwise.
This commit checks subsequent pages only when several are required per object.

Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
 lib/librte_mempool/rte_mempool.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index 01972ba..ecb03b3 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -156,7 +156,7 @@ rte_mempool_obj_iter(void *vaddr, uint32_t elt_num, size_t elt_sz, size_t align,
 	rte_mempool_obj_iter_t obj_iter, void *obj_iter_arg)
 {
 	uint32_t i, j, k;
-	uint32_t pgn;
+	uint32_t pgn, pgf;
 	uintptr_t end, start, va;
 	uintptr_t pg_sz;
 
@@ -171,10 +171,14 @@ rte_mempool_obj_iter(void *vaddr, uint32_t elt_num, size_t elt_sz, size_t align,
 		start = RTE_ALIGN_CEIL(va, align);
 		end = start + elt_sz;
 
-		pgn = (end >> pg_shift) - (start >> pg_shift);
+		/* index of the first page for the next element. */
+		pgf = (end >> pg_shift) - (start >> pg_shift);
+
+		/* index of the last page for the current element. */
+		pgn = ((end - 1) >> pg_shift) - (start >> pg_shift);
 		pgn += j;
 
-		/* do we have enough space left for the next element. */
+		/* do we have enough space left for the element. */
 		if (pgn >= pg_num)
 			break;
 
@@ -194,7 +198,7 @@ rte_mempool_obj_iter(void *vaddr, uint32_t elt_num, size_t elt_sz, size_t align,
 				obj_iter(obj_iter_arg, (void *)start,
 					(void *)end, i);
 			va = end;
-			j = pgn;
+			j += pgf;
 			i++;
 		} else {
 			va = RTE_ALIGN_CEIL((va + 1), pg_sz);
-- 
1.8.5.3

Re: [dpdk-dev] [PATCHv3] mempool: fix pages computation to determine number of objects

[Thomas Monjalon]

2015-05-27 09:40, Konstantin Ananyev:
> v3:
> - Fixed typo in the commit message.
> 
> v2:
> - As suggested in comments use slightly different approach for the fix.
> 
> In rte_mempool_obj_iter(), when element boundary coincides with page boundary,
> even if a single page is required per object, a loop checks that the next page
> is contiguous and drops the first one otherwise.
> This commit checks subsequent pages only when several are required per object.
> 
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Applied, thanks

Re: [dpdk-dev] [PATCH 1/2] mempool: fix returned value on 64 bit after counting objects

[Ananyev, 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 1/2] mempool: fix returned value on 64 bit after counting objects
> 
> rte_mempool_xmem_usage()'s return type is ssize_t which has the same
> architecture-dependent width as size_t but is signed.
> 
> On 64-bit architectures, returning a negative uint32_t value without casting
> to ssize_t first does not work as intended, the sign bit is lost and the
> returned value is garbage.
> 
> This commit fixes an assertion failure in testpmd on 64 bit architectures
> when combining --no-huge and --mp-anon outside of Xen Dom0:
> 
>  PANIC in mempool_anon_create():
>  line 170        assert "elt_num == mp->size" failed
> 
> Fixes: 148f963fb532 ("xen: core library changes")
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> ---
>  lib/librte_mempool/rte_mempool.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> index 01972ba..d1a02a2 100644
> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -361,7 +361,7 @@ rte_mempool_xmem_usage(void *vaddr, uint32_t elt_num, size_t elt_sz,
>  	if ((n = rte_mempool_obj_iter(vaddr, elt_num, elt_sz, 1,
>  			paddr, pg_num, pg_shift, mempool_lelem_iter,
>  			&uv)) != elt_num) {
> -		return (-n);
> +		return (-(ssize_t)n);
>  	}
> 
>  	uv = RTE_ALIGN_CEIL(uv, pg_sz);
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.1.0

Re: [dpdk-dev] [PATCH 1/2] mempool: fix returned value on 64 bit after counting objects

[Thomas Monjalon]

2015-05-27 00:43, Ananyev, Konstantin:
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Adrien Mazarguil
> > rte_mempool_xmem_usage()'s return type is ssize_t which has the same
> > architecture-dependent width as size_t but is signed.
> > 
> > On 64-bit architectures, returning a negative uint32_t value without casting
> > to ssize_t first does not work as intended, the sign bit is lost and the
> > returned value is garbage.
> > 
> > This commit fixes an assertion failure in testpmd on 64 bit architectures
> > when combining --no-huge and --mp-anon outside of Xen Dom0:
> > 
> >  PANIC in mempool_anon_create():
> >  line 170        assert "elt_num == mp->size" failed
> > 
> > Fixes: 148f963fb532 ("xen: core library changes")
> > 
> > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> 
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Applied, thanks