* Re: [dpdk-dev] [PATCH] mem: unmap unneeded space
2018-04-19 16:35 [dpdk-dev] [PATCH] mem: unmap unneeded space Anatoly Burakov
@ 2018-04-27 16:32 ` Bruce Richardson
2018-04-27 16:38 ` Bruce Richardson
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Bruce Richardson @ 2018-04-27 16:32 UTC (permalink / raw)
To: Anatoly Burakov; +Cc: dev
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [dpdk-dev] [PATCH] mem: unmap unneeded space
2018-04-19 16:35 [dpdk-dev] [PATCH] mem: unmap unneeded space Anatoly Burakov
2018-04-27 16:32 ` Bruce Richardson
@ 2018-04-27 16:38 ` Bruce Richardson
2018-04-30 11:21 ` [dpdk-dev] [PATCH v2 1/2] mem: check if allocation size is too big Anatoly Burakov
2018-04-30 11:21 ` [dpdk-dev] [PATCH v2 2/2] mem: unmap unneeded space Anatoly Burakov
3 siblings, 0 replies; 10+ messages in thread
From: Bruce Richardson @ 2018-04-27 16:38 UTC (permalink / raw)
To: Anatoly Burakov; +Cc: dev
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
^ permalink raw reply [flat|nested] 10+ messages in thread* [dpdk-dev] [PATCH v2 1/2] mem: check if allocation size is too big
2018-04-19 16:35 [dpdk-dev] [PATCH] mem: unmap unneeded space Anatoly Burakov
2018-04-27 16:32 ` Bruce Richardson
2018-04-27 16:38 ` Bruce Richardson
@ 2018-04-30 11:21 ` Anatoly Burakov
2018-04-30 12:49 ` Bruce Richardson
2018-04-30 11:21 ` [dpdk-dev] [PATCH v2 2/2] mem: unmap unneeded space Anatoly Burakov
3 siblings, 1 reply; 10+ messages in thread
From: Anatoly Burakov @ 2018-04-30 11:21 UTC (permalink / raw)
To: dev; +Cc: bruce.richardson
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
^ permalink raw reply [flat|nested] 10+ messages in thread* [dpdk-dev] [PATCH v2 2/2] mem: unmap unneeded space
2018-04-19 16:35 [dpdk-dev] [PATCH] mem: unmap unneeded space Anatoly Burakov
` (2 preceding siblings ...)
2018-04-30 11:21 ` [dpdk-dev] [PATCH v2 1/2] mem: check if allocation size is too big Anatoly Burakov
@ 2018-04-30 11:21 ` Anatoly Burakov
2018-04-30 12:50 ` Bruce Richardson
3 siblings, 1 reply; 10+ messages in thread
From: Anatoly Burakov @ 2018-04-30 11:21 UTC (permalink / raw)
To: dev; +Cc: bruce.richardson
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [dpdk-dev] [PATCH v2 2/2] mem: unmap unneeded space
2018-04-30 11:21 ` [dpdk-dev] [PATCH v2 2/2] mem: unmap unneeded space Anatoly Burakov
@ 2018-04-30 12:50 ` Bruce Richardson
2018-05-02 21:00 ` Thomas Monjalon
0 siblings, 1 reply; 10+ messages in thread
From: Bruce Richardson @ 2018-04-30 12:50 UTC (permalink / raw)
To: Anatoly Burakov; +Cc: dev
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>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-dev] [PATCH v2 2/2] mem: unmap unneeded space
2018-04-30 12:50 ` Bruce Richardson
@ 2018-05-02 21:00 ` Thomas Monjalon
2018-05-08 20:20 ` Thomas Monjalon
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Monjalon @ 2018-05-02 21:00 UTC (permalink / raw)
To: Bruce Richardson, Anatoly Burakov; +Cc: dev
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?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-dev] [PATCH v2 2/2] mem: unmap unneeded space
2018-05-02 21:00 ` Thomas Monjalon
@ 2018-05-08 20:20 ` Thomas Monjalon
2018-05-13 23:33 ` Thomas Monjalon
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Monjalon @ 2018-05-08 20:20 UTC (permalink / raw)
To: Anatoly Burakov; +Cc: dev, Bruce Richardson
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?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-dev] [PATCH v2 2/2] mem: unmap unneeded space
2018-05-08 20:20 ` Thomas Monjalon
@ 2018-05-13 23:33 ` Thomas Monjalon
0 siblings, 0 replies; 10+ messages in thread
From: Thomas Monjalon @ 2018-05-13 23:33 UTC (permalink / raw)
To: Anatoly Burakov; +Cc: dev, Bruce Richardson
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
^ permalink raw reply [flat|nested] 10+ messages in thread