[v3] test: improve resiliency of malloc autotest
Checks
Commit Message
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(+)
Comments
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;
> }
>
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
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
>
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.
@@ -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;
}