[v2] 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>
---
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(+)
Comments
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" :)
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
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?
@@ -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;
}