diff mbox series

[v2] eal: change alloc_sz calculation which may cause unnecessarily allocation

Message ID 20220728094103.137862-1-fidaullah.noonari@emumba.com (mailing list archive)
State Accepted, archived
Delegated to: David Marchand
Headers
Series [v2] eal: change alloc_sz calculation which may cause unnecessarily allocation |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS

Commit Message

Fidaullah Noonari July 28, 2022, 9:41 a.m. UTC 
The amount of memory to allocate from the system for heap expansion
was calculated in a way that may yield one page more than needed.
This could hit the allocation limit from the system or EAL.
The allocation would fail despite enough memory being available.

In response to mail:
http://inbox.dpdk.org/dev/CAEYuUWCnRZNwxiOHEeTHw0Gy9aFJRLZtvAG9g=smuUvUEMcFXg@mail.gmail.com/

Signed-off-by: Fidaullah Noonari <fidaullah.noonari@emumba.com>
---
 lib/eal/common/malloc_heap.c | 2 +-
 lib/eal/common/malloc_mp.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Dmitry Kozlyuk July 28, 2022, 6:03 p.m. UTC | #1 
2022-07-28 14:41 (UTC+0500), Fidaullah Noonari:
> The amount of memory to allocate from the system for heap expansion
> was calculated in a way that may yield one page more than needed.
> This could hit the allocation limit from the system or EAL.
> The allocation would fail despite enough memory being available.
> 
> In response to mail:
> http://inbox.dpdk.org/dev/CAEYuUWCnRZNwxiOHEeTHw0Gy9aFJRLZtvAG9g=smuUvUEMcFXg@mail.gmail.com/
> 
> Signed-off-by: Fidaullah Noonari <fidaullah.noonari@emumba.com>

Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
David Marchand Sept. 21, 2022, 1:35 p.m. UTC | #2 
On Thu, Jul 28, 2022 at 8:03 PM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
>
> 2022-07-28 14:41 (UTC+0500), Fidaullah Noonari:
> > The amount of memory to allocate from the system for heap expansion
> > was calculated in a way that may yield one page more than needed.
> > This could hit the allocation limit from the system or EAL.
> > The allocation would fail despite enough memory being available.
> >
> > In response to mail:
> > http://inbox.dpdk.org/dev/CAEYuUWCnRZNwxiOHEeTHw0Gy9aFJRLZtvAG9g=smuUvUEMcFXg@mail.gmail.com/
> >
> > Signed-off-by: Fidaullah Noonari <fidaullah.noonari@emumba.com>
>
> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
>

Do you have a reproducer?
This sounds like a fix, should we backport it?
Dmitry Kozlyuk Sept. 21, 2022, 10:52 p.m. UTC | #3 
2022-09-21 15:35 (UTC+0200), David Marchand:
> On Thu, Jul 28, 2022 at 8:03 PM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
> >
> > 2022-07-28 14:41 (UTC+0500), Fidaullah Noonari:  
> > > The amount of memory to allocate from the system for heap expansion
> > > was calculated in a way that may yield one page more than needed.
> > > This could hit the allocation limit from the system or EAL.
> > > The allocation would fail despite enough memory being available.
> > >
> > > In response to mail:
> > > http://inbox.dpdk.org/dev/CAEYuUWCnRZNwxiOHEeTHw0Gy9aFJRLZtvAG9g=smuUvUEMcFXg@mail.gmail.com/
> > >
> > > Signed-off-by: Fidaullah Noonari <fidaullah.noonari@emumba.com>  
> >
> > Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> >  
> 
> Do you have a reproducer?
> This sounds like a fix, should we backport it?

We should, thanks for the catch.

Reproducer:

#include <rte_eal.h>
#include <rte_malloc.h>
#include <stdio.h>

int
main(int argc, char **argv)
{
	static const size_t hugepage_sz = 2 * (1 << 20);
	static const size_t malloc_elem_overhead = 128;
	static const size_t size = hugepage_sz - malloc_elem_overhead;
	struct rte_malloc_socket_stats stats_before, stats_after;
	size_t alloc_delta;
	int ret;

	ret = rte_eal_init(argc, argv);
	if (ret < 0)
		rte_panic("Cannot init EAL\n");
	rte_malloc_get_socket_stats(0, &stats_before);
	(void)rte_malloc(NULL, size, 0);
	rte_malloc_get_socket_stats(0, &stats_after);
	alloc_delta = stats_after.heap_totalsz_bytes - stats_before.heap_totalsz_bytes;
	puts(alloc_delta == hugepage_sz ? "fixed" : "affected");
	rte_eal_cleanup();
	return 0;
}
David Marchand Sept. 26, 2022, 9:18 a.m. UTC | #4 
On Thu, Sep 22, 2022 at 12:52 AM Dmitry Kozlyuk
<dmitry.kozliuk@gmail.com> wrote:
>
> 2022-09-21 15:35 (UTC+0200), David Marchand:
> > On Thu, Jul 28, 2022 at 8:03 PM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
> > >
> > > 2022-07-28 14:41 (UTC+0500), Fidaullah Noonari:
> > > > The amount of memory to allocate from the system for heap expansion
> > > > was calculated in a way that may yield one page more than needed.
> > > > This could hit the allocation limit from the system or EAL.
> > > > The allocation would fail despite enough memory being available.
> > > >
> > > > In response to mail:
> > > > http://inbox.dpdk.org/dev/CAEYuUWCnRZNwxiOHEeTHw0Gy9aFJRLZtvAG9g=smuUvUEMcFXg@mail.gmail.com/
> > > >
> > > > Signed-off-by: Fidaullah Noonari <fidaullah.noonari@emumba.com>
> > >
> > > Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > >
> >
> > Do you have a reproducer?
> > This sounds like a fix, should we backport it?
>
> We should, thanks for the catch.
>
> Reproducer:
>
> #include <rte_eal.h>
> #include <rte_malloc.h>
> #include <stdio.h>
>
> int
> main(int argc, char **argv)
> {
>         static const size_t hugepage_sz = 2 * (1 << 20);
>         static const size_t malloc_elem_overhead = 128;
>         static const size_t size = hugepage_sz - malloc_elem_overhead;
>         struct rte_malloc_socket_stats stats_before, stats_after;
>         size_t alloc_delta;
>         int ret;
>
>         ret = rte_eal_init(argc, argv);
>         if (ret < 0)
>                 rte_panic("Cannot init EAL\n");
>         rte_malloc_get_socket_stats(0, &stats_before);
>         (void)rte_malloc(NULL, size, 0);
>         rte_malloc_get_socket_stats(0, &stats_after);
>         alloc_delta = stats_after.heap_totalsz_bytes - stats_before.heap_totalsz_bytes;
>         puts(alloc_delta == hugepage_sz ? "fixed" : "affected");
>         rte_eal_cleanup();
>         return 0;

Thanks Dmitry.
I'll put a note about this reproducer in the commitlog.

> }
>
David Marchand Sept. 26, 2022, 9:20 a.m. UTC | #5 
On Thu, Jul 28, 2022 at 8:03 PM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
>
> 2022-07-28 14:41 (UTC+0500), Fidaullah Noonari:
> > The amount of memory to allocate from the system for heap expansion
> > was calculated in a way that may yield one page more than needed.
> > This could hit the allocation limit from the system or EAL.
> > The allocation would fail despite enough memory being available.
> >
> > In response to mail:
> > http://inbox.dpdk.org/dev/CAEYuUWCnRZNwxiOHEeTHw0Gy9aFJRLZtvAG9g=smuUvUEMcFXg@mail.gmail.com/

Cc: stable@dpdk.org

> >
> > Signed-off-by: Fidaullah Noonari <fidaullah.noonari@emumba.com>
> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

I added a note about the reproducer (for testers) and applied.
Thanks!
diff mbox series

Patch

diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
index 27a52266ad..d7c410b786 100644
--- a/lib/eal/common/malloc_heap.c
+++ b/lib/eal/common/malloc_heap.c
@@ -402,7 +402,7 @@  try_expand_heap_primary(struct malloc_heap *heap, uint64_t pg_sz,
 	int n_segs;
 	bool callback_triggered = false;
 
-	alloc_sz = RTE_ALIGN_CEIL(align + elt_size +
+	alloc_sz = RTE_ALIGN_CEIL(RTE_ALIGN_CEIL(elt_size, align) +
 			MALLOC_ELEM_OVERHEAD, pg_sz);
 	n_segs = alloc_sz / pg_sz;
 
diff --git a/lib/eal/common/malloc_mp.c b/lib/eal/common/malloc_mp.c
index 2b8eb51067..15d85e2b2b 100644
--- a/lib/eal/common/malloc_mp.c
+++ b/lib/eal/common/malloc_mp.c
@@ -249,7 +249,7 @@  handle_alloc_request(const struct malloc_mp_req *m,
 		return -1;
 	}
 
-	alloc_sz = RTE_ALIGN_CEIL(ar->align + ar->elt_size +
+	alloc_sz = RTE_ALIGN_CEIL(RTE_ALIGN_CEIL(ar->elt_size, ar->align) +
 			MALLOC_ELEM_OVERHEAD, ar->page_sz);
 	n_segs = alloc_sz / ar->page_sz;