[dpdk-dev] [PATCH] mem: unmap unneeded space

[dpdk-dev] [PATCH] mem: unmap unneeded space

[Anatoly Burakov]

When we ask to reserve virtual areas, we usually include
alignment in the mapping size, and that memory ends up
being wasted. Wasting a gigabyte of VA space while trying to
reserve one gigabyte is pretty expensive on 32-bit, so after
we're done mapping, unmap unneeded space.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/eal_common_memory.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_memory.c b/lib/librte_eal/common/eal_common_memory.c
index 24a9ed5..8dd026a 100644
--- a/lib/librte_eal/common/eal_common_memory.c
+++ b/lib/librte_eal/common/eal_common_memory.c
@@ -75,8 +75,13 @@ eal_get_virtual_area(void *requested_addr, size_t *size,
 
 	do {
 		map_sz = no_align ? *size : *size + page_sz;
+		if (map_sz > SIZE_MAX) {
+			RTE_LOG(ERR, EAL, "Map size too big\n");
+			rte_errno = E2BIG;
+			return NULL;
+		}
 
-		mapped_addr = mmap(requested_addr, map_sz, PROT_READ,
+		mapped_addr = mmap(requested_addr, (size_t)map_sz, PROT_READ,
 				mmap_flags, -1, 0);
 		if (mapped_addr == MAP_FAILED && allow_shrink)
 			*size -= page_sz;
@@ -113,8 +118,22 @@ eal_get_virtual_area(void *requested_addr, size_t *size,
 		RTE_LOG(WARNING, EAL, "   This may cause issues with mapping memory into secondary processes\n");
 	}
 
-	if (unmap)
+	if (unmap) {
 		munmap(mapped_addr, map_sz);
+	} else if (!no_align) {
+		void *unmap_pre, *unmap_post, *map_end;
+		size_t pre_len, post_len;
+
+		/* unmap all of the extra space */
+		unmap_pre = mapped_addr;
+		pre_len = RTE_PTR_DIFF(aligned_addr, mapped_addr);
+		map_end = RTE_PTR_ADD(mapped_addr, (size_t)map_sz);
+		unmap_post = RTE_PTR_ADD(aligned_addr, *size);
+		post_len = RTE_PTR_DIFF(map_end, unmap_post);
+
+		munmap(unmap_pre, pre_len);
+		munmap(unmap_post, post_len);
+	}
 
 	RTE_LOG(DEBUG, EAL, "Virtual area found at %p (size = 0x%zx)\n",
 		aligned_addr, *size);
-- 
2.7.4

Re: [dpdk-dev] [PATCH] mem: unmap unneeded space

[Bruce Richardson]

On Thu, Apr 19, 2018 at 05:35:25PM +0100, Anatoly Burakov wrote:
> When we ask to reserve virtual areas, we usually include
> alignment in the mapping size, and that memory ends up
> being wasted. Wasting a gigabyte of VA space while trying to
> reserve one gigabyte is pretty expensive on 32-bit, so after
> we're done mapping, unmap unneeded space.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  lib/librte_eal/common/eal_common_memory.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_memory.c b/lib/librte_eal/common/eal_common_memory.c
> index 24a9ed5..8dd026a 100644
> --- a/lib/librte_eal/common/eal_common_memory.c
> +++ b/lib/librte_eal/common/eal_common_memory.c
> @@ -75,8 +75,13 @@ eal_get_virtual_area(void *requested_addr, size_t *size,
>  
>  	do {
>  		map_sz = no_align ? *size : *size + page_sz;
> +		if (map_sz > SIZE_MAX) {
> +			RTE_LOG(ERR, EAL, "Map size too big\n");
> +			rte_errno = E2BIG;
> +			return NULL;
> +		}
>  
> -		mapped_addr = mmap(requested_addr, map_sz, PROT_READ,
> +		mapped_addr = mmap(requested_addr, (size_t)map_sz, PROT_READ,
>  				mmap_flags, -1, 0);
>  		if (mapped_addr == MAP_FAILED && allow_shrink)
>  			*size -= page_sz;

This part looks like it should be a separate patch, since it's not related
to unmapping.

/Bruce

Re: [dpdk-dev] [PATCH] mem: unmap unneeded space

[Bruce Richardson]

On Thu, Apr 19, 2018 at 05:35:25PM +0100, Anatoly Burakov wrote:
> When we ask to reserve virtual areas, we usually include
> alignment in the mapping size, and that memory ends up
> being wasted. Wasting a gigabyte of VA space while trying to
> reserve one gigabyte is pretty expensive on 32-bit, so after
> we're done mapping, unmap unneeded space.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  lib/librte_eal/common/eal_common_memory.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_memory.c b/lib/librte_eal/common/eal_common_memory.c
> index 24a9ed5..8dd026a 100644
> --- a/lib/librte_eal/common/eal_common_memory.c
> +++ b/lib/librte_eal/common/eal_common_memory.c
> @@ -75,8 +75,13 @@ eal_get_virtual_area(void *requested_addr, size_t *size,
>  
>  	do {
>  		map_sz = no_align ? *size : *size + page_sz;
> +		if (map_sz > SIZE_MAX) {
> +			RTE_LOG(ERR, EAL, "Map size too big\n");
> +			rte_errno = E2BIG;
> +			return NULL;
> +		}
>  
> -		mapped_addr = mmap(requested_addr, map_sz, PROT_READ,
> +		mapped_addr = mmap(requested_addr, (size_t)map_sz, PROT_READ,
>  				mmap_flags, -1, 0);
>  		if (mapped_addr == MAP_FAILED && allow_shrink)
>  			*size -= page_sz;
> @@ -113,8 +118,22 @@ eal_get_virtual_area(void *requested_addr, size_t *size,
>  		RTE_LOG(WARNING, EAL, "   This may cause issues with mapping memory into secondary processes\n");
>  	}
>  
> -	if (unmap)
> +	if (unmap) {
>  		munmap(mapped_addr, map_sz);
> +	} else if (!no_align) {
> +		void *unmap_pre, *unmap_post, *map_end;
> +		size_t pre_len, post_len;
> +
> +		/* unmap all of the extra space */
> +		unmap_pre = mapped_addr;
> +		pre_len = RTE_PTR_DIFF(aligned_addr, mapped_addr);
> +		map_end = RTE_PTR_ADD(mapped_addr, (size_t)map_sz);
> +		unmap_post = RTE_PTR_ADD(aligned_addr, *size);
> +		post_len = RTE_PTR_DIFF(map_end, unmap_post);
> +
> +		munmap(unmap_pre, pre_len);
> +		munmap(unmap_post, post_len);

I suggest you take parts of the commit log and add them as a comment into
the code just after the else if.

Also, for readability, I think it would improve things to put the
munmap(unmap_pre, pre_len) immediately after the calculation of pre_len,
then a black line and then the rest of the code for clearing the post_len
block. I found it harder to decipher trying to keep track of all the
variables simultaneously in the two blocks. Splitting them into a
pre-blocka and a post-block should help here.

/Bruce

[dpdk-dev] [PATCH v2 1/2] mem: check if allocation size is too big

[Anatoly Burakov]

Mapping size is a 64-bit integer, but mmap() will accept size_t for
size mappings. A user could request a mapping with an alignment, which
would have overflown size_t, so check if (size + alignment) will
overflow size_t.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/eal_common_memory.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/eal_common_memory.c b/lib/librte_eal/common/eal_common_memory.c
index 4c943b0..0ac7b33 100644
--- a/lib/librte_eal/common/eal_common_memory.c
+++ b/lib/librte_eal/common/eal_common_memory.c
@@ -75,8 +75,13 @@ eal_get_virtual_area(void *requested_addr, size_t *size,
 
 	do {
 		map_sz = no_align ? *size : *size + page_sz;
+		if (map_sz > SIZE_MAX) {
+			RTE_LOG(ERR, EAL, "Map size too big\n");
+			rte_errno = E2BIG;
+			return NULL;
+		}
 
-		mapped_addr = mmap(requested_addr, map_sz, PROT_READ,
+		mapped_addr = mmap(requested_addr, (size_t)map_sz, PROT_READ,
 				mmap_flags, -1, 0);
 		if (mapped_addr == MAP_FAILED && allow_shrink)
 			*size -= page_sz;
-- 
2.7.4

[dpdk-dev] [PATCH v2 2/2] mem: unmap unneeded space

[Anatoly Burakov]

When we ask to reserve virtual areas, we usually include
alignment in the mapping size, and that memory ends up
being wasted. Wasting a gigabyte of VA space while trying to
reserve one gigabyte is pretty expensive on 32-bit, so after
we're done mapping, unmap unneeded space.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---

Notes:
    v2:
    - Split fix for size_t overflow into separate patch
    - Improve readability of unmapping code
    - Added comment explaining why unmapping is done

 lib/librte_eal/common/eal_common_memory.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/eal_common_memory.c b/lib/librte_eal/common/eal_common_memory.c
index 0ac7b33..60aed4a 100644
--- a/lib/librte_eal/common/eal_common_memory.c
+++ b/lib/librte_eal/common/eal_common_memory.c
@@ -121,8 +121,32 @@ eal_get_virtual_area(void *requested_addr, size_t *size,
 	RTE_LOG(DEBUG, EAL, "Virtual area found at %p (size = 0x%zx)\n",
 		aligned_addr, *size);
 
-	if (unmap)
+	if (unmap) {
 		munmap(mapped_addr, map_sz);
+	} else if (!no_align) {
+		void *map_end, *aligned_end;
+		size_t before_len, after_len;
+
+		/* when we reserve space with alignment, we add alignment to
+		 * mapping size. On 32-bit, if 1GB alignment was requested, this
+		 * would waste 1GB of address space, which is a luxury we cannot
+		 * afford. so, if alignment was performed, check if any unneeded
+		 * address space can be unmapped back.
+		 */
+
+		map_end = RTE_PTR_ADD(mapped_addr, (size_t)map_sz);
+		aligned_end = RTE_PTR_ADD(aligned_addr, *size);
+
+		/* unmap space before aligned mmap address */
+		before_len = RTE_PTR_DIFF(aligned_addr, mapped_addr);
+		if (before_len > 0)
+			munmap(mapped_addr, before_len);
+
+		/* unmap space after aligned end mmap address */
+		after_len = RTE_PTR_DIFF(map_end, aligned_end);
+		if (after_len > 0)
+			munmap(aligned_end, after_len);
+	}
 
 	baseaddr_offset += *size;
 
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2 1/2] mem: check if allocation size is too big

[Bruce Richardson]

On Mon, Apr 30, 2018 at 12:21:42PM +0100, Anatoly Burakov wrote:
> Mapping size is a 64-bit integer, but mmap() will accept size_t for
> size mappings. A user could request a mapping with an alignment, which
> would have overflown size_t, so check if (size + alignment) will
> overflow size_t.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH v2 2/2] mem: unmap unneeded space

[Bruce Richardson]

On Mon, Apr 30, 2018 at 12:21:43PM +0100, Anatoly Burakov wrote:
> When we ask to reserve virtual areas, we usually include
> alignment in the mapping size, and that memory ends up
> being wasted. Wasting a gigabyte of VA space while trying to
> reserve one gigabyte is pretty expensive on 32-bit, so after
> we're done mapping, unmap unneeded space.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
> 
> Notes:
>     v2:
>     - Split fix for size_t overflow into separate patch
>     - Improve readability of unmapping code
>     - Added comment explaining why unmapping is done
> 
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH v2 2/2] mem: unmap unneeded space

[Thomas Monjalon]

30/04/2018 14:50, Bruce Richardson:
> On Mon, Apr 30, 2018 at 12:21:43PM +0100, Anatoly Burakov wrote:
> > When we ask to reserve virtual areas, we usually include
> > alignment in the mapping size, and that memory ends up
> > being wasted. Wasting a gigabyte of VA space while trying to
> > reserve one gigabyte is pretty expensive on 32-bit, so after
> > we're done mapping, unmap unneeded space.
> > 
> > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > ---
> > 
> > Notes:
> >     v2:
> >     - Split fix for size_t overflow into separate patch
> >     - Improve readability of unmapping code
> >     - Added comment explaining why unmapping is done
> > 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>

I am not confident pushing this change post-rc1.
Please can we have more validation tests with this patch?

Re: [dpdk-dev] [PATCH v2 2/2] mem: unmap unneeded space

[Thomas Monjalon]

02/05/2018 23:00, Thomas Monjalon:
> 30/04/2018 14:50, Bruce Richardson:
> > On Mon, Apr 30, 2018 at 12:21:43PM +0100, Anatoly Burakov wrote:
> > > When we ask to reserve virtual areas, we usually include
> > > alignment in the mapping size, and that memory ends up
> > > being wasted. Wasting a gigabyte of VA space while trying to
> > > reserve one gigabyte is pretty expensive on 32-bit, so after
> > > we're done mapping, unmap unneeded space.
> > > 
> > > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > > ---
> > > 
> > > Notes:
> > >     v2:
> > >     - Split fix for size_t overflow into separate patch
> > >     - Improve readability of unmapping code
> > >     - Added comment explaining why unmapping is done
> > > 
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> I am not confident pushing this change post-rc1.
> Please can we have more validation tests with this patch?

Any news about validation tests?

Re: [dpdk-dev] [PATCH v2 2/2] mem: unmap unneeded space

[Thomas Monjalon]

08/05/2018 22:20, Thomas Monjalon:
> 02/05/2018 23:00, Thomas Monjalon:
> > 30/04/2018 14:50, Bruce Richardson:
> > > On Mon, Apr 30, 2018 at 12:21:43PM +0100, Anatoly Burakov wrote:
> > > > When we ask to reserve virtual areas, we usually include
> > > > alignment in the mapping size, and that memory ends up
> > > > being wasted. Wasting a gigabyte of VA space while trying to
> > > > reserve one gigabyte is pretty expensive on 32-bit, so after
> > > > we're done mapping, unmap unneeded space.
> > > > 
> > > > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > > > ---
> > > > 
> > > > Notes:
> > > >     v2:
> > > >     - Split fix for size_t overflow into separate patch
> > > >     - Improve readability of unmapping code
> > > >     - Added comment explaining why unmapping is done
> > > > 
> > > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > 
> > I am not confident pushing this change post-rc1.
> > Please can we have more validation tests with this patch?
> 
> Any news about validation tests?

No news, but I've decided to push it anyway.

Series applied