[PATCH] app/test: fix stack overflow in fib6_perf_autotest

[PATCH] app/test: fix stack overflow in fib6_perf_autotest

[Andre Muezerie]

Test fib6_perf_autotest was hitting a stack overflow on Windows
with MSVC.

The fix is to move some of the data from the stack to the heap.

Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
---
 app/test/test_fib6_perf.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/app/test/test_fib6_perf.c b/app/test/test_fib6_perf.c
index a96a0d6b2c..246bc2d509 100644
--- a/app/test/test_fib6_perf.c
+++ b/app/test/test_fib6_perf.c
@@ -9,6 +9,7 @@
 
 #include <rte_cycles.h>
 #include <rte_random.h>
+#include <rte_malloc.h>
 #include <rte_memory.h>
 #include <rte_fib6.h>
 
@@ -73,8 +74,14 @@ test_fib6_perf(void)
 	uint64_t next_hop_add;
 	int status = 0;
 	int64_t count = 0;
-	struct rte_ipv6_addr ip_batch[NUM_IPS_ENTRIES];
-	uint64_t next_hops[NUM_IPS_ENTRIES];
+
+	struct rte_ipv6_addr *ip_batch = rte_calloc("ip_batch",
+			NUM_IPS_ENTRIES, sizeof(struct rte_ipv6_addr), 0);
+	TEST_FIB_ASSERT(ip_batch != NULL);
+
+	uint64_t *next_hops = rte_calloc("next_hops",
+			NUM_IPS_ENTRIES, sizeof(uint64_t), 0);
+	TEST_FIB_ASSERT(next_hops != NULL);
 
 	conf.type = RTE_FIB6_TRIE;
 	conf.default_nh = 0;
@@ -151,6 +158,9 @@ test_fib6_perf(void)
 
 	rte_fib6_free(fib);
 
+	rte_free(next_hops);
+	rte_free(ip_batch);
+
 	return 0;
 }
 
-- 
2.47.0.vfs.0.3

Re: [PATCH] app/test: fix stack overflow in fib6_perf_autotest

[Stephen Hemminger]

On Mon, 23 Dec 2024 13:10:33 -0800
Andre Muezerie <andremue@linux.microsoft.com> wrote:

> From: Andre Muezerie <andremue@linux.microsoft.com>
> To: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
> Cc: dev@dpdk.org,  Andre Muezerie <andremue@linux.microsoft.com>
> Subject: [PATCH] app/test: fix stack overflow in fib6_perf_autotest
> Date: Mon, 23 Dec 2024 13:10:33 -0800
> X-Mailer: git-send-email 1.8.3.1
> 
> Test fib6_perf_autotest was hitting a stack overflow on Windows
> with MSVC.
> 
> The fix is to move some of the data from the stack to the heap.
> 
> Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>

Use regular malloc please.
rte_malloc comes from hugepages which are more limited and slower to manipulate.

Re: [PATCH] app/test: fix stack overflow in fib6_perf_autotest

[Andre Muezerie]

On Mon, Dec 23, 2024 at 01:30:00PM -0800, Stephen Hemminger wrote:
> On Mon, 23 Dec 2024 13:10:33 -0800
> Andre Muezerie <andremue@linux.microsoft.com> wrote:
> 
> > From: Andre Muezerie <andremue@linux.microsoft.com>
> > To: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
> > Cc: dev@dpdk.org,  Andre Muezerie <andremue@linux.microsoft.com>
> > Subject: [PATCH] app/test: fix stack overflow in fib6_perf_autotest
> > Date: Mon, 23 Dec 2024 13:10:33 -0800
> > X-Mailer: git-send-email 1.8.3.1
> > 
> > Test fib6_perf_autotest was hitting a stack overflow on Windows
> > with MSVC.
> > 
> > The fix is to move some of the data from the stack to the heap.
> > 
> > Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
> 
> Use regular malloc please.
> rte_malloc comes from hugepages which are more limited and slower to manipulate.

I recently submitted a patch for a test with a very similar issue and
during review one of the reviewers encouraged me to use rte_calloc to
allocate memory for the arrays, which I think makes sense (I had used
malloc initially):

https://inbox.dpdk.org/dev/20241218151206.GA25758@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net/

Even though this is a perf test, the code responsible for the memory
allocations is not in the path for which time measurements are being
taken (points between rte_rdtsc calls), so perf for the memory
allocation code is probably not so critical.

That being said, if you still feel strongly that malloc should be used
instead let me know and I can make that change.

Thanks,

Andre Muezerie

Re: [PATCH] app/test: fix stack overflow in fib6_perf_autotest

[Medvedkin, Vladimir]

Hi,

I think using rte_malloc(rte_calloc) here would be a better choice because:

- As Andre mentioned allocation happens outside of the performance 
measurement section

- Due to relatively large size of the memory allocated for 
ip_batch/next_hops some CPUs may experience TLB cache pressure, which 
will affect performance measurement results.

With that being said

Acked-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>

On 24/12/2024 01:47, Andre Muezerie wrote:
> On Mon, Dec 23, 2024 at 01:30:00PM -0800, Stephen Hemminger wrote:
>> On Mon, 23 Dec 2024 13:10:33 -0800
>> Andre Muezerie <andremue@linux.microsoft.com> wrote:
>>
>>> From: Andre Muezerie <andremue@linux.microsoft.com>
>>> To: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
>>> Cc: dev@dpdk.org,  Andre Muezerie <andremue@linux.microsoft.com>
>>> Subject: [PATCH] app/test: fix stack overflow in fib6_perf_autotest
>>> Date: Mon, 23 Dec 2024 13:10:33 -0800
>>> X-Mailer: git-send-email 1.8.3.1
>>>
>>> Test fib6_perf_autotest was hitting a stack overflow on Windows
>>> with MSVC.
>>>
>>> The fix is to move some of the data from the stack to the heap.
>>>
>>> Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
>> Use regular malloc please.
>> rte_malloc comes from hugepages which are more limited and slower to manipulate.
> I recently submitted a patch for a test with a very similar issue and
> during review one of the reviewers encouraged me to use rte_calloc to
> allocate memory for the arrays, which I think makes sense (I had used
> malloc initially):
>
> https://inbox.dpdk.org/dev/20241218151206.GA25758@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net/
>
> Even though this is a perf test, the code responsible for the memory
> allocations is not in the path for which time measurements are being
> taken (points between rte_rdtsc calls), so perf for the memory
> allocation code is probably not so critical.
>
> That being said, if you still feel strongly that malloc should be used
> instead let me know and I can make that change.
>
> Thanks,
>
> Andre Muezerie

-- 
Regards,
Vladimir