diff mbox series

[2/2] test/rcu: address test case failure

Message ID 20190628034406.5399-2-honnappa.nagarahalli@arm.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [1/2] test/rcu: increase the size of num cores variable |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Honnappa Nagarahalli June 28, 2019, 3:44 a.m. UTC 
  Test case for rte_rcu_qsbr_get_memsize is written specifically
for 128 threads. Do not use RTE_MAX_LCORE as it changes for
different configurations.

Fixes: e6a14121f4ae ("test/rcu: remove arbitrary limit on max core count")
Cc: stable@dpdk.org

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 app/test/test_rcu_qsbr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

David Marchand June 28, 2019, 9:16 a.m. UTC | #1 
On Fri, Jun 28, 2019 at 5:44 AM Honnappa Nagarahalli <
honnappa.nagarahalli@arm.com> wrote:

> Test case for rte_rcu_qsbr_get_memsize is written specifically
> for 128 threads. Do not use RTE_MAX_LCORE as it changes for
> different configurations.
>

Does it mean this test can only work on arm with 256 lcores?
How many cores does this test require?
Honnappa Nagarahalli June 28, 2019, 1:53 p.m. UTC | #2 
On Fri, Jun 28, 2019 at 5:44 AM Honnappa Nagarahalli <honnappa.nagarahalli@arm.com<mailto:honnappa.nagarahalli@arm.com>> wrote:
Test case for rte_rcu_qsbr_get_memsize is written specifically
for 128 threads. Do not use RTE_MAX_LCORE as it changes for
different configurations.

Does it mean this test can only work on arm with 256 lcores?
How many cores does this test require?
[Honnappa] It tests the correctness of the calculation of the memory required. So, it uses the hand calculated number to verify. The hand calculated number is for 128 cores. So, it does not depend on the platform as such.


--
David Marchand
David Marchand June 28, 2019, 2:09 p.m. UTC | #3 
On Fri, Jun 28, 2019 at 3:54 PM Honnappa Nagarahalli <
Honnappa.Nagarahalli@arm.com> wrote:

>
>
> On Fri, Jun 28, 2019 at 5:44 AM Honnappa Nagarahalli <
> honnappa.nagarahalli@arm.com> wrote:
>
> Test case for rte_rcu_qsbr_get_memsize is written specifically
> for 128 threads. Do not use RTE_MAX_LCORE as it changes for
> different configurations.
>
>
>
> Does it mean this test can only work on arm with 256 lcores?
>
> How many cores does this test require?
>
> *[Honnappa] *It tests the correctness of the calculation of the memory
> required. So, it uses the hand calculated number to verify. The hand
> calculated number is for 128 cores. So, it does not depend on the platform
> as such.
>

Ah ah, funny that the default RTE_MAX_LCORE for x86 is 128, and then I did
not see the test failing.
Then ok for this fix.

Reviewed-by: David Marchand <david.marchand@redhat.com>


How about the followup patch:

-       TEST_RCU_QSBR_RETURN_IF_ERROR((sz != 8384 && sz != 16768),
-               "Get Memsize for 128 threads");
+       TEST_RCU_QSBR_RETURN_IF_ERROR(
+#if RTE_CACHE_LINE_SIZE == 64
+                       sz != 8384
+#elif RTE_CACHE_LINE_SIZE == 128
+                       sz != 16768
+#endif
+               , "Get Memsize for 128 threads");
Honnappa Nagarahalli June 28, 2019, 4:36 p.m. UTC | #4 
On Fri, Jun 28, 2019 at 5:44 AM Honnappa Nagarahalli <honnappa.nagarahalli@arm.com<mailto:honnappa.nagarahalli@arm.com>> wrote:
Test case for rte_rcu_qsbr_get_memsize is written specifically
for 128 threads. Do not use RTE_MAX_LCORE as it changes for
different configurations.

Does it mean this test can only work on arm with 256 lcores?
How many cores does this test require?
[Honnappa] It tests the correctness of the calculation of the memory required. So, it uses the hand calculated number to verify. The hand calculated number is for 128 cores. So, it does not depend on the platform as such.

Ah ah, funny that the default RTE_MAX_LCORE for x86 is 128, and then I did not see the test failing.
Then ok for this fix.

Reviewed-by: David Marchand <david.marchand@redhat.com<mailto:david.marchand@redhat.com>>


How about the followup patch:

-       TEST_RCU_QSBR_RETURN_IF_ERROR((sz != 8384 && sz != 16768),
-               "Get Memsize for 128 threads");
+       TEST_RCU_QSBR_RETURN_IF_ERROR(
+#if RTE_CACHE_LINE_SIZE == 64
+                       sz != 8384
+#elif RTE_CACHE_LINE_SIZE == 128
+                       sz != 16768
+#endif
+               , "Get Memsize for 128 threads");
[Honnappa] Added this change to V2, but slightly differently


--
David Marchand
David Marchand June 28, 2019, 4:54 p.m. UTC | #5 
On Fri, Jun 28, 2019 at 6:37 PM Honnappa Nagarahalli <
Honnappa.Nagarahalli@arm.com> wrote:

> On Fri, Jun 28, 2019 at 5:44 AM Honnappa Nagarahalli <
> honnappa.nagarahalli@arm.com<mailto:honnappa.nagarahalli@arm.com>> wrote:
> Test case for rte_rcu_qsbr_get_memsize is written specifically
> for 128 threads. Do not use RTE_MAX_LCORE as it changes for
> different configurations.
>
> Does it mean this test can only work on arm with 256 lcores?
> How many cores does this test require?
> [Honnappa] It tests the correctness of the calculation of the memory
> required. So, it uses the hand calculated number to verify. The hand
> calculated number is for 128 cores. So, it does not depend on the platform
> as such.
>
> Ah ah, funny that the default RTE_MAX_LCORE for x86 is 128, and then I did
> not see the test failing.
> Then ok for this fix.
>
> Reviewed-by: David Marchand <david.marchand@redhat.com<mailto:
> david.marchand@redhat.com>>
>
>
> How about the followup patch:
>
> -       TEST_RCU_QSBR_RETURN_IF_ERROR((sz != 8384 && sz != 16768),
> -               "Get Memsize for 128 threads");
> +       TEST_RCU_QSBR_RETURN_IF_ERROR(
> +#if RTE_CACHE_LINE_SIZE == 64
> +                       sz != 8384
> +#elif RTE_CACHE_LINE_SIZE == 128
> +                       sz != 16768
> +#endif
> +               , "Get Memsize for 128 threads");
> [Honnappa] Added this change to V2, but slightly differently
>


Yep saw it.
Thanks.
diff mbox series

Patch

diff --git a/app/test/test_rcu_qsbr.c b/app/test/test_rcu_qsbr.c
index 0c6267ee9..5d92fbee8 100644
--- a/app/test/test_rcu_qsbr.c
+++ b/app/test/test_rcu_qsbr.c
@@ -89,13 +89,13 @@  test_rcu_qsbr_get_memsize(void)
 	sz = rte_rcu_qsbr_get_memsize(0);
 	TEST_RCU_QSBR_RETURN_IF_ERROR((sz != 1), "Get Memsize for 0 threads");
 
-	sz = rte_rcu_qsbr_get_memsize(RTE_MAX_LCORE);
+	sz = rte_rcu_qsbr_get_memsize(128);
 	/* For 128 threads,
 	 * for machines with cache line size of 64B - 8384
 	 * for machines with cache line size of 128 - 16768
 	 */
 	TEST_RCU_QSBR_RETURN_IF_ERROR((sz != 8384 && sz != 16768),
-		"Get Memsize");
+		"Get Memsize for 128 threads");
 
 	return 0;
 }