[PATCH] test: improve resiliency of malloc autotest

[PATCH] test: improve resiliency of malloc autotest

[Bruce Richardson]

The test case "test_multi_alloc_statistics" was brittle in that it did
some allocations and frees and then checked statistics without
considering the initial state of the malloc heaps. This meant that,
depending on what allocations/frees were done beforehand, the test can
sometimes fail.

We can improve resiliency by running the test using a new malloc heap,
which means it is unaffected by any previous allocations.

Bugzilla ID: 1579
Fixes: a40a1f8231b4 ("app: various tests update")
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_malloc.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/app/test/test_malloc.c b/app/test/test_malloc.c
index 02a7d8ef20..a2f2f6f8a6 100644
--- a/app/test/test_malloc.c
+++ b/app/test/test_malloc.c
@@ -25,6 +25,7 @@
 #include <rte_malloc.h>
 #include <rte_cycles.h>
 #include <rte_random.h>
+#include <rte_eal_paging.h>
 #include <rte_string_fns.h>
 
 #define N 10000
@@ -272,6 +273,28 @@ test_multi_alloc_statistics(void)
 	size_t size = 2048;
 	int align = 1024;
 	int overhead = 0;
+	const size_t heap_size = (1 << 21);
+
+	if (rte_malloc_heap_create(__func__) != 0) {
+		printf("Failed to create test malloc heap\n");
+		return -1;
+	}
+	/* allocate some memory using malloc and add it to our test heap. */
+	void *memory = mmap(NULL, heap_size, PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	if (memory == MAP_FAILED) {
+		printf("Failed to allocate memory\n");
+		return -1;
+	}
+	if (rte_malloc_heap_memory_add(__func__, memory, heap_size, NULL, 1, heap_size) != 0) {
+		printf("Failed to add memory to heap\n");
+		return -1;
+	}
+	socket = rte_malloc_heap_get_socket(__func__);
+	if (socket < 0) {
+		printf("Failed to get socket for test malloc heap.\n");
+		return -1;
+	}
 
 	/* Dynamically calculate the overhead by allocating one cacheline and
 	 * then comparing what was allocated from the heap.
@@ -371,6 +394,11 @@ test_multi_alloc_statistics(void)
 		printf("Malloc statistics are incorrect - freed alloc\n");
 		return -1;
 	}
+
+	/* cleanup */
+	rte_malloc_heap_memory_remove(__func__, memory, heap_size);
+	rte_malloc_heap_destroy(__func__);
+	munmap(memory, heap_size);
 	return 0;
 }
 
-- 
2.43.0

[PATCH v2] test: improve resiliency of malloc autotest

[Bruce Richardson]

The test case "test_multi_alloc_statistics" was brittle in that it did
some allocations and frees and then checked statistics without
considering the initial state of the malloc heaps. This meant that,
depending on what allocations/frees were done beforehand, the test can
sometimes fail.

We can improve resiliency by running the test using a new malloc heap,
which means it is unaffected by any previous allocations.

Bugzilla ID: 1579
Fixes: a40a1f8231b4 ("app: various tests update")
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
v2:
* removed unnecessary extra include
* only added new code for non-windows, since using mmap for allocation.
---
 app/test/test_malloc.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/app/test/test_malloc.c b/app/test/test_malloc.c
index 02a7d8ef20..62e4445ebc 100644
--- a/app/test/test_malloc.c
+++ b/app/test/test_malloc.c
@@ -272,6 +272,30 @@ test_multi_alloc_statistics(void)
 	size_t size = 2048;
 	int align = 1024;
 	int overhead = 0;
+#ifndef RTE_EXEC_ENV_WINDOWS
+	const size_t heap_size = (1 << 21);
+
+	if (rte_malloc_heap_create(__func__) != 0) {
+		printf("Failed to create test malloc heap\n");
+		return -1;
+	}
+	/* allocate some memory using malloc and add it to our test heap. */
+	void *memory = mmap(NULL, heap_size, PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	if (memory == MAP_FAILED) {
+		printf("Failed to allocate memory\n");
+		return -1;
+	}
+	if (rte_malloc_heap_memory_add(__func__, memory, heap_size, NULL, 1, heap_size) != 0) {
+		printf("Failed to add memory to heap\n");
+		return -1;
+	}
+	socket = rte_malloc_heap_get_socket(__func__);
+	if (socket < 0) {
+		printf("Failed to get socket for test malloc heap.\n");
+		return -1;
+	}
+#endif
 
 	/* Dynamically calculate the overhead by allocating one cacheline and
 	 * then comparing what was allocated from the heap.
@@ -371,6 +395,13 @@ test_multi_alloc_statistics(void)
 		printf("Malloc statistics are incorrect - freed alloc\n");
 		return -1;
 	}
+
+#ifndef RTE_EXEC_ENV_WINDOWS
+	/* cleanup */
+	rte_malloc_heap_memory_remove(__func__, memory, heap_size);
+	rte_malloc_heap_destroy(__func__);
+	munmap(memory, heap_size);
+#endif
 	return 0;
 }
 
-- 
2.43.0

Re: [PATCH v2] test: improve resiliency of malloc autotest

[Dmitry Kozlyuk]

2025-01-17 13:52 (UTC+0000), Bruce Richardson:
> The test case "test_multi_alloc_statistics" was brittle in that it did
> some allocations and frees and then checked statistics without
> considering the initial state of the malloc heaps. This meant that,
> depending on what allocations/frees were done beforehand, the test can
> sometimes fail.
> 
> We can improve resiliency by running the test using a new malloc heap,
> which means it is unaffected by any previous allocations.
> 
> Bugzilla ID: 1579
> Fixes: a40a1f8231b4 ("app: various tests update")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
> v2:
> * removed unnecessary extra include
> * only added new code for non-windows, since using mmap for allocation.

Why is it necessary to use `mmap()` and not portable `malloc()`?
Even the comment in the patch says "malloc" :)

Re: [PATCH v2] test: improve resiliency of malloc autotest

[Bruce Richardson]

On Fri, Jan 17, 2025 at 05:20:41PM +0300, Dmitry Kozlyuk wrote:
> 2025-01-17 13:52 (UTC+0000), Bruce Richardson:
> > The test case "test_multi_alloc_statistics" was brittle in that it did
> > some allocations and frees and then checked statistics without
> > considering the initial state of the malloc heaps. This meant that,
> > depending on what allocations/frees were done beforehand, the test can
> > sometimes fail.
> > 
> > We can improve resiliency by running the test using a new malloc heap,
> > which means it is unaffected by any previous allocations.
> > 
> > Bugzilla ID: 1579
> > Fixes: a40a1f8231b4 ("app: various tests update")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> > v2:
> > * removed unnecessary extra include
> > * only added new code for non-windows, since using mmap for allocation.
> 
> Why is it necessary to use `mmap()` and not portable `malloc()`?
> Even the comment in the patch says "malloc" :)

I did originally use malloc, but malloc didn't give us aligned memory so
the call to add the memory to the heap was subsequently failing.

However, I see that the unit tests in the CI are failing on some
architectures, probably because of alignment again, because of using a
single 2MB block of memory. I was going to do a v3 where I queried the
pagesize and used N*pgsize as the parameter to "add" rather than saying
it's a 1x2MB block. Instead, though, I'll rework the code
to use malloc and then manually align instead.

/Bruce

Re: [PATCH v2] test: improve resiliency of malloc autotest

[Stephen Hemminger]

On Fri, 17 Jan 2025 14:26:01 +0000
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Fri, Jan 17, 2025 at 05:20:41PM +0300, Dmitry Kozlyuk wrote:
> > 2025-01-17 13:52 (UTC+0000), Bruce Richardson:  
> > > The test case "test_multi_alloc_statistics" was brittle in that it did
> > > some allocations and frees and then checked statistics without
> > > considering the initial state of the malloc heaps. This meant that,
> > > depending on what allocations/frees were done beforehand, the test can
> > > sometimes fail.
> > > 
> > > We can improve resiliency by running the test using a new malloc heap,
> > > which means it is unaffected by any previous allocations.
> > > 
> > > Bugzilla ID: 1579
> > > Fixes: a40a1f8231b4 ("app: various tests update")
> > > Cc: stable@dpdk.org
> > > 
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > ---
> > > v2:
> > > * removed unnecessary extra include
> > > * only added new code for non-windows, since using mmap for allocation.  
> > 
> > Why is it necessary to use `mmap()` and not portable `malloc()`?
> > Even the comment in the patch says "malloc" :)  
> 
> I did originally use malloc, but malloc didn't give us aligned memory so
> the call to add the memory to the heap was subsequently failing.

Use posix_memalign() or aligned_alloc() for that?

[PATCH v3] test: improve resiliency of malloc autotest

[Bruce Richardson]

The test case "test_multi_alloc_statistics" was brittle in that it did
some allocations and frees and then checked statistics without
considering the initial state of the malloc heaps. This meant that,
depending on what allocations/frees were done beforehand, the test can
sometimes fail.

We can improve resiliency by running the test using a new malloc heap,
which means it is unaffected by any previous allocations.

Bugzilla ID: 1579
Fixes: a40a1f8231b4 ("app: various tests update")
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
v3:
* switched allocation from mmap to malloc allowing it work on windows
* use explicit alignment of the malloc return value to ensure memory
  added to heap is page-aligned.

v2:
* removed unnecessary extra include
* only added new code for non-windows, since using mmap for allocation.
---
 app/test/test_malloc.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/app/test/test_malloc.c b/app/test/test_malloc.c
index 02a7d8ef20..9e73c0da09 100644
--- a/app/test/test_malloc.c
+++ b/app/test/test_malloc.c
@@ -25,6 +25,7 @@
 #include <rte_malloc.h>
 #include <rte_cycles.h>
 #include <rte_random.h>
+#include <rte_eal_paging.h>
 #include <rte_string_fns.h>
 
 #define N 10000
@@ -272,6 +273,34 @@ test_multi_alloc_statistics(void)
 	size_t size = 2048;
 	int align = 1024;
 	int overhead = 0;
+	const size_t pgsz = rte_mem_page_size();
+	const size_t heap_size = (1 << 21);
+
+	if (pgsz < heap_size) {
+		printf("Page size is smaller than heap size\n");
+		return TEST_SKIPPED;
+	}
+
+	if (rte_malloc_heap_create(__func__) != 0) {
+		printf("Failed to create test malloc heap\n");
+		return -1;
+	}
+	/* Allocate some memory using malloc and add it to our test heap. */
+	void *unaligned_memory = malloc(heap_size + pgsz);
+	if (unaligned_memory == NULL) {
+		printf("Failed to allocate memory\n");
+		return -1;
+	}
+	void *memory = RTE_PTR_ALIGN(unaligned_memory, pgsz);
+	if (rte_malloc_heap_memory_add(__func__, memory, heap_size, NULL, 1, heap_size) != 0) {
+		printf("Failed to add memory to heap\n");
+		return -1;
+	}
+	socket = rte_malloc_heap_get_socket(__func__);
+	if (socket < 0) {
+		printf("Failed to get socket for test malloc heap.\n");
+		return -1;
+	}
 
 	/* Dynamically calculate the overhead by allocating one cacheline and
 	 * then comparing what was allocated from the heap.
@@ -371,6 +400,12 @@ test_multi_alloc_statistics(void)
 		printf("Malloc statistics are incorrect - freed alloc\n");
 		return -1;
 	}
+
+	/* cleanup */
+	rte_malloc_heap_memory_remove(__func__, memory, heap_size);
+	rte_malloc_heap_destroy(__func__);
+	free(unaligned_memory);
+
 	return 0;
 }
 
-- 
2.43.0

Re: [PATCH v3] test: improve resiliency of malloc autotest

[fengchengwen]

The new impl don't support re-test, how about add a wrap:
1. rename test_multi_alloc_statistics with do_test_multi_alloc_statistics, and make it take socket as parameter
2. create a new function test_multi_alloc_statistics {
    // prepare a new malloc heap
    ret = do_test_multi_alloc_statistics(socket);
    // free the heap
    return ret;
}

On 2025/1/17 22:40, Bruce Richardson wrote:
> The test case "test_multi_alloc_statistics" was brittle in that it did
> some allocations and frees and then checked statistics without
> considering the initial state of the malloc heaps. This meant that,
> depending on what allocations/frees were done beforehand, the test can
> sometimes fail.
> 
> We can improve resiliency by running the test using a new malloc heap,
> which means it is unaffected by any previous allocations.
> 
> Bugzilla ID: 1579
> Fixes: a40a1f8231b4 ("app: various tests update")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
> v3:
> * switched allocation from mmap to malloc allowing it work on windows
> * use explicit alignment of the malloc return value to ensure memory
>   added to heap is page-aligned.
> 
> v2:
> * removed unnecessary extra include
> * only added new code for non-windows, since using mmap for allocation.
> ---
>  app/test/test_malloc.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/app/test/test_malloc.c b/app/test/test_malloc.c
> index 02a7d8ef20..9e73c0da09 100644
> --- a/app/test/test_malloc.c
> +++ b/app/test/test_malloc.c
> @@ -25,6 +25,7 @@
>  #include <rte_malloc.h>
>  #include <rte_cycles.h>
>  #include <rte_random.h>
> +#include <rte_eal_paging.h>
>  #include <rte_string_fns.h>
>  
>  #define N 10000
> @@ -272,6 +273,34 @@ test_multi_alloc_statistics(void)
>  	size_t size = 2048;
>  	int align = 1024;
>  	int overhead = 0;
> +	const size_t pgsz = rte_mem_page_size();
> +	const size_t heap_size = (1 << 21);
> +
> +	if (pgsz < heap_size) {
> +		printf("Page size is smaller than heap size\n");
> +		return TEST_SKIPPED;
> +	}
> +
> +	if (rte_malloc_heap_create(__func__) != 0) {
> +		printf("Failed to create test malloc heap\n");
> +		return -1;
> +	}
> +	/* Allocate some memory using malloc and add it to our test heap. */
> +	void *unaligned_memory = malloc(heap_size + pgsz);
> +	if (unaligned_memory == NULL) {
> +		printf("Failed to allocate memory\n");
> +		return -1;
> +	}
> +	void *memory = RTE_PTR_ALIGN(unaligned_memory, pgsz);
> +	if (rte_malloc_heap_memory_add(__func__, memory, heap_size, NULL, 1, heap_size) != 0) {
> +		printf("Failed to add memory to heap\n");
> +		return -1;
> +	}
> +	socket = rte_malloc_heap_get_socket(__func__);
> +	if (socket < 0) {
> +		printf("Failed to get socket for test malloc heap.\n");
> +		return -1;
> +	}
>  
>  	/* Dynamically calculate the overhead by allocating one cacheline and
>  	 * then comparing what was allocated from the heap.
> @@ -371,6 +400,12 @@ test_multi_alloc_statistics(void)
>  		printf("Malloc statistics are incorrect - freed alloc\n");
>  		return -1;
>  	}
> +
> +	/* cleanup */
> +	rte_malloc_heap_memory_remove(__func__, memory, heap_size);
> +	rte_malloc_heap_destroy(__func__);
> +	free(unaligned_memory);
> +
>  	return 0;
>  }
>  

Re: [PATCH v3] test: improve resiliency of malloc autotest

[Bruce Richardson]

On Fri, Jan 24, 2025 at 03:18:11PM +0800, fengchengwen wrote:
> The new impl don't support re-test, how about add a wrap:
> 1. rename test_multi_alloc_statistics with do_test_multi_alloc_statistics, and make it take socket as parameter
> 2. create a new function test_multi_alloc_statistics {
>     // prepare a new malloc heap
>     ret = do_test_multi_alloc_statistics(socket);
>     // free the heap
>     return ret;
> }
> 

Can you clarify the issues being seen on re-test? I have just run
malloc_autotest multiple times within the same dpdk-test instance and not
seen any issues.

/Bruce

Re: [PATCH v3] test: improve resiliency of malloc autotest

[fengchengwen]

On 2025/2/6 19:40, Bruce Richardson wrote:
> On Fri, Jan 24, 2025 at 03:18:11PM +0800, fengchengwen wrote:
>> The new impl don't support re-test, how about add a wrap:
>> 1. rename test_multi_alloc_statistics with do_test_multi_alloc_statistics, and make it take socket as parameter
>> 2. create a new function test_multi_alloc_statistics {
>>     // prepare a new malloc heap
>>     ret = do_test_multi_alloc_statistics(socket);
>>     // free the heap
>>     return ret;
>> }
>>
> 
> Can you clarify the issues being seen on re-test? I have just run
> malloc_autotest multiple times within the same dpdk-test instance and not
> seen any issues.

If the middle logic fail, for example:

	if ((post_stats.heap_totalsz_bytes != pre_stats.heap_totalsz_bytes) ||
			(post_stats.heap_freesz_bytes != pre_stats.heap_freesz_bytes) ||
			(post_stats.heap_allocsz_bytes != pre_stats.heap_allocsz_bytes) ||
			(post_stats.alloc_count != pre_stats.alloc_count) ||
			(post_stats.free_count != pre_stats.free_count)) {
		printf("Malloc statistics are incorrect - freed alloc\n");
		return -1;
	}

If the above if branch taken, then retest, the rte_malloc_heap_create(__func__) will
failed because already exist the heap.

> 
> /Bruce
> 

Re: [PATCH v3] test: improve resiliency of malloc autotest

[Bruce Richardson]

On Fri, Feb 07, 2025 at 03:44:56PM +0800, fengchengwen wrote:
> On 2025/2/6 19:40, Bruce Richardson wrote:
> > On Fri, Jan 24, 2025 at 03:18:11PM +0800, fengchengwen wrote:
> >> The new impl don't support re-test, how about add a wrap:
> >> 1. rename test_multi_alloc_statistics with do_test_multi_alloc_statistics, and make it take socket as parameter
> >> 2. create a new function test_multi_alloc_statistics {
> >>     // prepare a new malloc heap
> >>     ret = do_test_multi_alloc_statistics(socket);
> >>     // free the heap
> >>     return ret;
> >> }
> >>
> > 
> > Can you clarify the issues being seen on re-test? I have just run
> > malloc_autotest multiple times within the same dpdk-test instance and not
> > seen any issues.
> 
> If the middle logic fail, for example:
> 
> 	if ((post_stats.heap_totalsz_bytes != pre_stats.heap_totalsz_bytes) ||
> 			(post_stats.heap_freesz_bytes != pre_stats.heap_freesz_bytes) ||
> 			(post_stats.heap_allocsz_bytes != pre_stats.heap_allocsz_bytes) ||
> 			(post_stats.alloc_count != pre_stats.alloc_count) ||
> 			(post_stats.free_count != pre_stats.free_count)) {
> 		printf("Malloc statistics are incorrect - freed alloc\n");
> 		return -1;
> 	}
> 
> If the above if branch taken, then retest, the rte_malloc_heap_create(__func__) will
> failed because already exist the heap.
> 
Ok, retest on failure is broken. Got it. Will rework.

[PATCH] test: improve resiliency of malloc autotest

[Bruce Richardson]

The test case "test_multi_alloc_statistics" was brittle in that it did
some allocations and frees and then checked statistics without
considering the initial state of the malloc heaps. This meant that,
depending on what allocations/frees were done beforehand, the test can
sometimes fail.

We can improve resiliency by running the test using a new malloc heap,
which means it is unaffected by any previous allocations.

Bugzilla ID: 1579
Fixes: a40a1f8231b4 ("app: various tests update")
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
v4:
* added cleanup support in case of failure, to allow multiple runs of
  the test in a single session.

v3:
* switched allocation from mmap to malloc allowing it work on windows
* use explicit alignment of the malloc return value to ensure memory
  added to heap is page-aligned.

v2:
* removed unnecessary extra include
* only added new code for non-windows, since using mmap for allocation.
---
 app/test/test_malloc.c | 77 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 64 insertions(+), 13 deletions(-)

diff --git a/app/test/test_malloc.c b/app/test/test_malloc.c
index 02a7d8ef20..d6f8da3b24 100644
--- a/app/test/test_malloc.c
+++ b/app/test/test_malloc.c
@@ -25,6 +25,7 @@
 #include <rte_malloc.h>
 #include <rte_cycles.h>
 #include <rte_random.h>
+#include <rte_eal_paging.h>
 #include <rte_string_fns.h>
 
 #define N 10000
@@ -267,11 +268,48 @@ test_str_to_size(void)
 static int
 test_multi_alloc_statistics(void)
 {
+	int ret = -1;  /* default return is error, cleared at end on success */
 	int socket = 0;
 	struct rte_malloc_socket_stats pre_stats, post_stats ,first_stats, second_stats;
 	size_t size = 2048;
 	int align = 1024;
 	int overhead = 0;
+	const size_t pgsz = rte_mem_page_size();
+	const size_t heap_size = (1 << 22);
+
+	if (pgsz > heap_size) {
+		printf("Page size (%zu) is bigger than heap size, skipping alloc stats test\n",
+				pgsz);
+		return TEST_SKIPPED;
+	}
+	if (heap_size % pgsz != 0) {
+		printf("Heap size (%zu) is not a multiple of page size (%zu), skipping alloc stats test\n",
+				heap_size, pgsz);
+		return TEST_SKIPPED;
+	}
+
+	if (rte_malloc_heap_create(__func__) != 0) {
+		printf("Failed to create test malloc heap\n");
+		goto end;
+	}
+
+	/* Allocate some memory using malloc and add it to our test heap. */
+	void *unaligned_memory = malloc(heap_size + pgsz);
+	if (unaligned_memory == NULL) {
+		printf("Failed to allocate memory\n");
+		goto cleanup_empty_heap;
+	}
+	void *memory = RTE_PTR_ALIGN(unaligned_memory, pgsz);
+	if (rte_malloc_heap_memory_add(__func__, memory, heap_size, NULL,
+			heap_size / pgsz, pgsz) != 0) {
+		printf("Failed to add memory to heap\n");
+		goto cleanup_allocated_memory;
+	}
+	socket = rte_malloc_heap_get_socket(__func__);
+	if (socket < 0) {
+		printf("Failed to get socket for test malloc heap.\n");
+		goto cleanup_all;
+	}
 
 	/* Dynamically calculate the overhead by allocating one cacheline and
 	 * then comparing what was allocated from the heap.
@@ -280,7 +318,7 @@ test_multi_alloc_statistics(void)
 
 	void *dummy = rte_malloc_socket(NULL, RTE_CACHE_LINE_SIZE, 0, socket);
 	if (dummy == NULL)
-		return -1;
+		goto cleanup_all;
 
 	rte_malloc_get_socket_stats(socket, &post_stats);
 
@@ -295,7 +333,8 @@ test_multi_alloc_statistics(void)
 
 	void *p1 = rte_malloc_socket("stats", size , align, socket);
 	if (!p1)
-		return -1;
+		goto cleanup_all;
+
 	rte_free(p1);
 	rte_malloc_dump_stats(stdout, "stats");
 
@@ -308,7 +347,7 @@ test_multi_alloc_statistics(void)
 			(post_stats.alloc_count != pre_stats.alloc_count) ||
 			(post_stats.free_count != pre_stats.free_count)) {
 		printf("Malloc statistics are incorrect - freed alloc\n");
-		return -1;
+		goto cleanup_all;
 	}
 	/* Check two consecutive allocations */
 	size = 1024;
@@ -316,12 +355,12 @@ test_multi_alloc_statistics(void)
 	rte_malloc_get_socket_stats(socket,&pre_stats);
 	void *p2 = rte_malloc_socket("add", size ,align, socket);
 	if (!p2)
-		return -1;
+		goto cleanup_all;
 	rte_malloc_get_socket_stats(socket,&first_stats);
 
 	void *p3 = rte_malloc_socket("add2", size,align, socket);
 	if (!p3)
-		return -1;
+		goto cleanup_all;
 
 	rte_malloc_get_socket_stats(socket,&second_stats);
 
@@ -333,34 +372,34 @@ test_multi_alloc_statistics(void)
 
 	if(second_stats.heap_totalsz_bytes != first_stats.heap_totalsz_bytes) {
 		printf("Incorrect heap statistics: Total size \n");
-		return -1;
+		goto cleanup_all;
 	}
 	/* Check allocated size is equal to two additions plus overhead */
 	if(second_stats.heap_allocsz_bytes !=
 			size + overhead + first_stats.heap_allocsz_bytes) {
 		printf("Incorrect heap statistics: Allocated size \n");
-		return -1;
+		goto cleanup_all;
 	}
 	/* Check that allocation count increments correctly i.e. +1 */
 	if (second_stats.alloc_count != first_stats.alloc_count + 1) {
 		printf("Incorrect heap statistics: Allocated count \n");
-		return -1;
+		goto cleanup_all;
 	}
 
 	if (second_stats.free_count != first_stats.free_count){
 		printf("Incorrect heap statistics: Free count \n");
-		return -1;
+		goto cleanup_all;
 	}
 
 	/* Make sure that we didn't touch our greatest chunk: 2 * 11M)  */
 	if (post_stats.greatest_free_size != pre_stats.greatest_free_size) {
 		printf("Incorrect heap statistics: Greatest free size \n");
-		return -1;
+		goto cleanup_all;
 	}
 	/* Free size must equal the original free size minus the new allocation*/
 	if (first_stats.heap_freesz_bytes <= second_stats.heap_freesz_bytes) {
 		printf("Incorrect heap statistics: Free size \n");
-		return -1;
+		goto cleanup_all;
 	}
 
 	if ((post_stats.heap_totalsz_bytes != pre_stats.heap_totalsz_bytes) ||
@@ -369,9 +408,21 @@ test_multi_alloc_statistics(void)
 			(post_stats.alloc_count != pre_stats.alloc_count) ||
 			(post_stats.free_count != pre_stats.free_count)) {
 		printf("Malloc statistics are incorrect - freed alloc\n");
-		return -1;
+		goto cleanup_all;
 	}
-	return 0;
+
+	/* set return value as success before cleanup */
+	ret = 0;
+
+	/* cleanup */
+cleanup_all:
+	rte_malloc_heap_memory_remove(__func__, memory, heap_size);
+cleanup_allocated_memory:
+	free(unaligned_memory);
+cleanup_empty_heap:
+	rte_malloc_heap_destroy(__func__);
+end:
+	return ret;
 }
 
 #ifdef RTE_EXEC_ENV_WINDOWS
-- 
2.43.0

[PATCH v4] test: improve resiliency of malloc autotest

[Bruce Richardson]

The test case "test_multi_alloc_statistics" was brittle in that it did
some allocations and frees and then checked statistics without
considering the initial state of the malloc heaps. This meant that,
depending on what allocations/frees were done beforehand, the test can
sometimes fail.

We can improve resiliency by running the test using a new malloc heap,
which means it is unaffected by any previous allocations.

Bugzilla ID: 1579
Fixes: a40a1f8231b4 ("app: various tests update")
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
v4:
* added cleanup support in case of failure, to allow multiple runs of
  the test in a single session.

v3:
* switched allocation from mmap to malloc allowing it work on windows
* use explicit alignment of the malloc return value to ensure memory
  added to heap is page-aligned.

v2:
* removed unnecessary extra include
* only added new code for non-windows, since using mmap for allocation.
---
 app/test/test_malloc.c | 77 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 64 insertions(+), 13 deletions(-)

diff --git a/app/test/test_malloc.c b/app/test/test_malloc.c
index 02a7d8ef20..d6f8da3b24 100644
--- a/app/test/test_malloc.c
+++ b/app/test/test_malloc.c
@@ -25,6 +25,7 @@
 #include <rte_malloc.h>
 #include <rte_cycles.h>
 #include <rte_random.h>
+#include <rte_eal_paging.h>
 #include <rte_string_fns.h>
 
 #define N 10000
@@ -267,11 +268,48 @@ test_str_to_size(void)
 static int
 test_multi_alloc_statistics(void)
 {
+	int ret = -1;  /* default return is error, cleared at end on success */
 	int socket = 0;
 	struct rte_malloc_socket_stats pre_stats, post_stats ,first_stats, second_stats;
 	size_t size = 2048;
 	int align = 1024;
 	int overhead = 0;
+	const size_t pgsz = rte_mem_page_size();
+	const size_t heap_size = (1 << 22);
+
+	if (pgsz > heap_size) {
+		printf("Page size (%zu) is bigger than heap size, skipping alloc stats test\n",
+				pgsz);
+		return TEST_SKIPPED;
+	}
+	if (heap_size % pgsz != 0) {
+		printf("Heap size (%zu) is not a multiple of page size (%zu), skipping alloc stats test\n",
+				heap_size, pgsz);
+		return TEST_SKIPPED;
+	}
+
+	if (rte_malloc_heap_create(__func__) != 0) {
+		printf("Failed to create test malloc heap\n");
+		goto end;
+	}
+
+	/* Allocate some memory using malloc and add it to our test heap. */
+	void *unaligned_memory = malloc(heap_size + pgsz);
+	if (unaligned_memory == NULL) {
+		printf("Failed to allocate memory\n");
+		goto cleanup_empty_heap;
+	}
+	void *memory = RTE_PTR_ALIGN(unaligned_memory, pgsz);
+	if (rte_malloc_heap_memory_add(__func__, memory, heap_size, NULL,
+			heap_size / pgsz, pgsz) != 0) {
+		printf("Failed to add memory to heap\n");
+		goto cleanup_allocated_memory;
+	}
+	socket = rte_malloc_heap_get_socket(__func__);
+	if (socket < 0) {
+		printf("Failed to get socket for test malloc heap.\n");
+		goto cleanup_all;
+	}
 
 	/* Dynamically calculate the overhead by allocating one cacheline and
 	 * then comparing what was allocated from the heap.
@@ -280,7 +318,7 @@ test_multi_alloc_statistics(void)
 
 	void *dummy = rte_malloc_socket(NULL, RTE_CACHE_LINE_SIZE, 0, socket);
 	if (dummy == NULL)
-		return -1;
+		goto cleanup_all;
 
 	rte_malloc_get_socket_stats(socket, &post_stats);
 
@@ -295,7 +333,8 @@ test_multi_alloc_statistics(void)
 
 	void *p1 = rte_malloc_socket("stats", size , align, socket);
 	if (!p1)
-		return -1;
+		goto cleanup_all;
+
 	rte_free(p1);
 	rte_malloc_dump_stats(stdout, "stats");
 
@@ -308,7 +347,7 @@ test_multi_alloc_statistics(void)
 			(post_stats.alloc_count != pre_stats.alloc_count) ||
 			(post_stats.free_count != pre_stats.free_count)) {
 		printf("Malloc statistics are incorrect - freed alloc\n");
-		return -1;
+		goto cleanup_all;
 	}
 	/* Check two consecutive allocations */
 	size = 1024;
@@ -316,12 +355,12 @@ test_multi_alloc_statistics(void)
 	rte_malloc_get_socket_stats(socket,&pre_stats);
 	void *p2 = rte_malloc_socket("add", size ,align, socket);
 	if (!p2)
-		return -1;
+		goto cleanup_all;
 	rte_malloc_get_socket_stats(socket,&first_stats);
 
 	void *p3 = rte_malloc_socket("add2", size,align, socket);
 	if (!p3)
-		return -1;
+		goto cleanup_all;
 
 	rte_malloc_get_socket_stats(socket,&second_stats);
 
@@ -333,34 +372,34 @@ test_multi_alloc_statistics(void)
 
 	if(second_stats.heap_totalsz_bytes != first_stats.heap_totalsz_bytes) {
 		printf("Incorrect heap statistics: Total size \n");
-		return -1;
+		goto cleanup_all;
 	}
 	/* Check allocated size is equal to two additions plus overhead */
 	if(second_stats.heap_allocsz_bytes !=
 			size + overhead + first_stats.heap_allocsz_bytes) {
 		printf("Incorrect heap statistics: Allocated size \n");
-		return -1;
+		goto cleanup_all;
 	}
 	/* Check that allocation count increments correctly i.e. +1 */
 	if (second_stats.alloc_count != first_stats.alloc_count + 1) {
 		printf("Incorrect heap statistics: Allocated count \n");
-		return -1;
+		goto cleanup_all;
 	}
 
 	if (second_stats.free_count != first_stats.free_count){
 		printf("Incorrect heap statistics: Free count \n");
-		return -1;
+		goto cleanup_all;
 	}
 
 	/* Make sure that we didn't touch our greatest chunk: 2 * 11M)  */
 	if (post_stats.greatest_free_size != pre_stats.greatest_free_size) {
 		printf("Incorrect heap statistics: Greatest free size \n");
-		return -1;
+		goto cleanup_all;
 	}
 	/* Free size must equal the original free size minus the new allocation*/
 	if (first_stats.heap_freesz_bytes <= second_stats.heap_freesz_bytes) {
 		printf("Incorrect heap statistics: Free size \n");
-		return -1;
+		goto cleanup_all;
 	}
 
 	if ((post_stats.heap_totalsz_bytes != pre_stats.heap_totalsz_bytes) ||
@@ -369,9 +408,21 @@ test_multi_alloc_statistics(void)
 			(post_stats.alloc_count != pre_stats.alloc_count) ||
 			(post_stats.free_count != pre_stats.free_count)) {
 		printf("Malloc statistics are incorrect - freed alloc\n");
-		return -1;
+		goto cleanup_all;
 	}
-	return 0;
+
+	/* set return value as success before cleanup */
+	ret = 0;
+
+	/* cleanup */
+cleanup_all:
+	rte_malloc_heap_memory_remove(__func__, memory, heap_size);
+cleanup_allocated_memory:
+	free(unaligned_memory);
+cleanup_empty_heap:
+	rte_malloc_heap_destroy(__func__);
+end:
+	return ret;
 }
 
 #ifdef RTE_EXEC_ENV_WINDOWS
-- 
2.43.0