[dpdk-dev] [PATCH 1/2] test/rcu: increase the size of num cores variable

[dpdk-dev] [PATCH 1/2] test/rcu: increase the size of num cores variable

[Honnappa Nagarahalli]

num_cores is of type uint8_t. This results in the following
compilation error.

test_rcu_qsbr_perf.c:649:16: error: comparison is always false
due to limited range of data type [-Werror=type-limits]
  if (num_cores >= RTE_MAX_LCORE) {
                ^~

RTE_MAX_LCORE is set to 256 for armv8 config.

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 ++--
 app/test/test_rcu_qsbr_perf.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/test/test_rcu_qsbr.c b/app/test/test_rcu_qsbr.c
index 943a1e370..0c6267ee9 100644
--- a/app/test/test_rcu_qsbr.c
+++ b/app/test/test_rcu_qsbr.c
@@ -27,7 +27,7 @@
 #define TEST_RCU_QSBR_CNT_INIT 1
 
 uint16_t enabled_core_ids[RTE_MAX_LCORE];
-uint8_t num_cores;
+uint16_t num_cores;
 
 static uint32_t *keys;
 #define TOTAL_ENTRY (1024 * 8)
@@ -891,7 +891,7 @@ static int
 test_rcu_qsbr_mw_mv_mqs(void)
 {
 	int i, j;
-	uint8_t test_cores;
+	uint16_t test_cores;
 
 	writer_done = 0;
 	test_cores = num_cores / 4;
diff --git a/app/test/test_rcu_qsbr_perf.c b/app/test/test_rcu_qsbr_perf.c
index 363365f46..b1a910423 100644
--- a/app/test/test_rcu_qsbr_perf.c
+++ b/app/test/test_rcu_qsbr_perf.c
@@ -17,7 +17,7 @@
 
 /* Check condition and return an error if true. */
 static uint16_t enabled_core_ids[RTE_MAX_LCORE];
-static uint8_t num_cores;
+static uint16_t num_cores;
 
 static uint32_t *keys;
 #define TOTAL_ENTRY (1024 * 8)
-- 
2.17.1

[dpdk-dev] [PATCH 2/2] test/rcu: address test case failure

[Honnappa Nagarahalli]

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(-)

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;
 }
-- 
2.17.1

Re: [dpdk-dev] [dpdk-stable] [PATCH 2/2] test/rcu: address test case failure

[David Marchand]

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?


-- 
David Marchand

Re: [dpdk-dev] [dpdk-stable] [PATCH 2/2] test/rcu: address test case failure

[Honnappa Nagarahalli]

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

Re: [dpdk-dev] [dpdk-stable] [PATCH 2/2] test/rcu: address test case failure

[David Marchand]

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");


-- 
David Marchand

Re: [dpdk-dev] [dpdk-stable] [PATCH 2/2] test/rcu: address test case failure

[Honnappa Nagarahalli]

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

Re: [dpdk-dev] [dpdk-stable] [PATCH 2/2] test/rcu: address test case failure

[David Marchand]

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.

-- 
David Marchand

Re: [dpdk-dev] [PATCH 1/2] test/rcu: increase the size of num cores variable

[David Marchand]

On Fri, Jun 28, 2019 at 5:44 AM Honnappa Nagarahalli <
honnappa.nagarahalli@arm.com> wrote:

> num_cores is of type uint8_t. This results in the following
> compilation error.
>
> test_rcu_qsbr_perf.c:649:16: error: comparison is always false
> due to limited range of data type [-Werror=type-limits]
>   if (num_cores >= RTE_MAX_LCORE) {
>                 ^~
>
> RTE_MAX_LCORE is set to 256 for armv8 config.
>


Weird I did not see this error in travis.
Just tried again:
https://travis-ci.com/david-marchand/dpdk/jobs/211768426



> 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 ++--
>  app/test/test_rcu_qsbr_perf.c | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/app/test/test_rcu_qsbr.c b/app/test/test_rcu_qsbr.c
> index 943a1e370..0c6267ee9 100644
> --- a/app/test/test_rcu_qsbr.c
> +++ b/app/test/test_rcu_qsbr.c
> @@ -27,7 +27,7 @@
>  #define TEST_RCU_QSBR_CNT_INIT 1
>
>  uint16_t enabled_core_ids[RTE_MAX_LCORE];
> -uint8_t num_cores;
> +uint16_t num_cores;
>

If we want to be closer to the eal API, those should be unsigned int.



>  static uint32_t *keys;
>  #define TOTAL_ENTRY (1024 * 8)
> @@ -891,7 +891,7 @@ static int
>  test_rcu_qsbr_mw_mv_mqs(void)
>  {
>         int i, j;
> -       uint8_t test_cores;
> +       uint16_t test_cores;
>
>         writer_done = 0;
>         test_cores = num_cores / 4;
> diff --git a/app/test/test_rcu_qsbr_perf.c b/app/test/test_rcu_qsbr_perf.c
> index 363365f46..b1a910423 100644
> --- a/app/test/test_rcu_qsbr_perf.c
> +++ b/app/test/test_rcu_qsbr_perf.c
> @@ -17,7 +17,7 @@
>
>  /* Check condition and return an error if true. */
>  static uint16_t enabled_core_ids[RTE_MAX_LCORE];
> -static uint8_t num_cores;
> +static uint16_t num_cores;
>
>  static uint32_t *keys;
>  #define TOTAL_ENTRY (1024 * 8)
> --
> 2.17.1
>


-- 
David Marchand

Re: [dpdk-dev] [dpdk-stable] [PATCH 1/2] test/rcu: increase the size of num cores variable

[Thomas Monjalon]

28/06/2019 11:09, David Marchand:
> On Fri, Jun 28, 2019 at 5:44 AM Honnappa Nagarahalli
> > --- a/app/test/test_rcu_qsbr.c
> > +++ b/app/test/test_rcu_qsbr.c
> > @@ -27,7 +27,7 @@
> >  #define TEST_RCU_QSBR_CNT_INIT 1
> >
> >  uint16_t enabled_core_ids[RTE_MAX_LCORE];
> > -uint8_t num_cores;
> > +uint16_t num_cores;
> 
> If we want to be closer to the eal API, those should be unsigned int.

I agree, please use unsigned int where relevant.

Re: [dpdk-dev] [dpdk-stable] [PATCH 1/2] test/rcu: increase the size of num cores variable

[Honnappa Nagarahalli]

> 
> 28/06/2019 11:09, David Marchand:
> > On Fri, Jun 28, 2019 at 5:44 AM Honnappa Nagarahalli
> > > --- a/app/test/test_rcu_qsbr.c
> > > +++ b/app/test/test_rcu_qsbr.c
> > > @@ -27,7 +27,7 @@
> > >  #define TEST_RCU_QSBR_CNT_INIT 1
> > >
> > >  uint16_t enabled_core_ids[RTE_MAX_LCORE]; -uint8_t num_cores;
> > > +uint16_t num_cores;
> >
> > If we want to be closer to the eal API, those should be unsigned int.
> 
> I agree, please use unsigned int where relevant.
Sent v2. There are other similar cases, may be in a follow up patch.

> 
> 

[dpdk-dev] [PATCH v2 1/2] test/rcu: increase the size of num cores variable

[Honnappa Nagarahalli]

num_cores is of type uint8_t. This results in the following
compilation error.

test_rcu_qsbr_perf.c:649:16: error: comparison is always false
due to limited range of data type [-Werror=type-limits]
  if (num_cores >= RTE_MAX_LCORE) {
                ^~

RTE_MAX_LCORE is set to 256 for armv8 config.

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>
---
v2
- Changed type of num_cores to 'unsigned int' and
  related changes (David/Thomas)

 app/test/test_rcu_qsbr.c      |  8 ++++----
 app/test/test_rcu_qsbr_perf.c | 16 ++++++++--------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/app/test/test_rcu_qsbr.c b/app/test/test_rcu_qsbr.c
index 943a1e370..ae359a987 100644
--- a/app/test/test_rcu_qsbr.c
+++ b/app/test/test_rcu_qsbr.c
@@ -27,7 +27,7 @@
 #define TEST_RCU_QSBR_CNT_INIT 1
 
 uint16_t enabled_core_ids[RTE_MAX_LCORE];
-uint8_t num_cores;
+unsigned int num_cores;
 
 static uint32_t *keys;
 #define TOTAL_ENTRY (1024 * 8)
@@ -389,7 +389,7 @@ test_rcu_qsbr_synchronize_reader(void *arg)
 static int
 test_rcu_qsbr_synchronize(void)
 {
-	int i;
+	unsigned int i;
 
 	printf("\nTest rte_rcu_qsbr_synchronize()\n");
 
@@ -890,8 +890,8 @@ test_rcu_qsbr_sw_sv_3qs(void)
 static int
 test_rcu_qsbr_mw_mv_mqs(void)
 {
-	int i, j;
-	uint8_t test_cores;
+	unsigned int i, j;
+	unsigned int test_cores;
 
 	writer_done = 0;
 	test_cores = num_cores / 4;
diff --git a/app/test/test_rcu_qsbr_perf.c b/app/test/test_rcu_qsbr_perf.c
index 363365f46..a085db852 100644
--- a/app/test/test_rcu_qsbr_perf.c
+++ b/app/test/test_rcu_qsbr_perf.c
@@ -17,7 +17,7 @@
 
 /* Check condition and return an error if true. */
 static uint16_t enabled_core_ids[RTE_MAX_LCORE];
-static uint8_t num_cores;
+static unsigned int num_cores;
 
 static uint32_t *keys;
 #define TOTAL_ENTRY (1024 * 8)
@@ -125,8 +125,8 @@ test_rcu_qsbr_writer_perf(void *arg)
 static int
 test_rcu_qsbr_perf(void)
 {
-	int i, sz;
-	int tmp_num_cores;
+	unsigned int i, sz;
+	unsigned int tmp_num_cores;
 
 	writer_done = 0;
 
@@ -188,8 +188,8 @@ test_rcu_qsbr_perf(void)
 static int
 test_rcu_qsbr_rperf(void)
 {
-	int i, sz;
-	int tmp_num_cores;
+	unsigned int i, sz;
+	unsigned int tmp_num_cores;
 
 	rte_atomic64_clear(&updates);
 	rte_atomic64_clear(&update_cycles);
@@ -234,7 +234,7 @@ test_rcu_qsbr_rperf(void)
 static int
 test_rcu_qsbr_wperf(void)
 {
-	int i, sz;
+	unsigned int i, sz;
 
 	rte_atomic64_clear(&checks);
 	rte_atomic64_clear(&check_cycles);
@@ -378,7 +378,7 @@ static int
 test_rcu_qsbr_sw_sv_1qs(void)
 {
 	uint64_t token, begin, cycles;
-	int i, j, tmp_num_cores, sz;
+	unsigned int i, j, tmp_num_cores, sz;
 	int32_t pos;
 
 	writer_done = 0;
@@ -496,7 +496,7 @@ static int
 test_rcu_qsbr_sw_sv_1qs_non_blocking(void)
 {
 	uint64_t token, begin, cycles;
-	int i, j, ret, tmp_num_cores, sz;
+	unsigned int i, j, ret, tmp_num_cores, sz;
 	int32_t pos;
 
 	writer_done = 0;
-- 
2.17.1

[dpdk-dev] [PATCH v2 2/2] test/rcu: address test case failure

[Honnappa Nagarahalli]

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>
Reviewed-by: David Marchand <david.marchand@redhat.com>
---
v2
- Changed the test case result validation (David)

 app/test/test_rcu_qsbr.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/app/test/test_rcu_qsbr.c b/app/test/test_rcu_qsbr.c
index ae359a987..1d273e39d 100644
--- a/app/test/test_rcu_qsbr.c
+++ b/app/test/test_rcu_qsbr.c
@@ -89,13 +89,17 @@ 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");
+	if (RTE_CACHE_LINE_SIZE == 64)
+		TEST_RCU_QSBR_RETURN_IF_ERROR((sz != 8384),
+			"Get Memsize for 128 threads");
+	else if (RTE_CACHE_LINE_SIZE == 128)
+		TEST_RCU_QSBR_RETURN_IF_ERROR((sz != 16768),
+			"Get Memsize for 128 threads");
 
 	return 0;
 }
-- 
2.17.1

Re: [dpdk-dev] [PATCH v2 1/2] test/rcu: increase the size of num cores variable

[David Marchand]

On Fri, Jun 28, 2019 at 6:36 PM Honnappa Nagarahalli <
honnappa.nagarahalli@arm.com> wrote:

> num_cores is of type uint8_t. This results in the following
> compilation error.
>
> test_rcu_qsbr_perf.c:649:16: error: comparison is always false
> due to limited range of data type [-Werror=type-limits]
>   if (num_cores >= RTE_MAX_LCORE) {
>                 ^~
>
> RTE_MAX_LCORE is set to 256 for armv8 config.
>
> 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>
> ---
> v2
> - Changed type of num_cores to 'unsigned int' and
>   related changes (David/Thomas)
>
>  app/test/test_rcu_qsbr.c      |  8 ++++----
>  app/test/test_rcu_qsbr_perf.c | 16 ++++++++--------
>  2 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/app/test/test_rcu_qsbr.c b/app/test/test_rcu_qsbr.c
> index 943a1e370..ae359a987 100644
> --- a/app/test/test_rcu_qsbr.c
> +++ b/app/test/test_rcu_qsbr.c
> @@ -27,7 +27,7 @@
>  #define TEST_RCU_QSBR_CNT_INIT 1
>
>  uint16_t enabled_core_ids[RTE_MAX_LCORE];
> -uint8_t num_cores;
> +unsigned int num_cores;
>
>  static uint32_t *keys;
>  #define TOTAL_ENTRY (1024 * 8)
> @@ -389,7 +389,7 @@ test_rcu_qsbr_synchronize_reader(void *arg)
>  static int
>  test_rcu_qsbr_synchronize(void)
>  {
> -       int i;
> +       unsigned int i;
>
>         printf("\nTest rte_rcu_qsbr_synchronize()\n");
>
> @@ -890,8 +890,8 @@ test_rcu_qsbr_sw_sv_3qs(void)
>  static int
>  test_rcu_qsbr_mw_mv_mqs(void)
>  {
> -       int i, j;
> -       uint8_t test_cores;
> +       unsigned int i, j;
> +       unsigned int test_cores;
>
>         writer_done = 0;
>         test_cores = num_cores / 4;
> diff --git a/app/test/test_rcu_qsbr_perf.c b/app/test/test_rcu_qsbr_perf.c
> index 363365f46..a085db852 100644
> --- a/app/test/test_rcu_qsbr_perf.c
> +++ b/app/test/test_rcu_qsbr_perf.c
> @@ -17,7 +17,7 @@
>
>  /* Check condition and return an error if true. */
>  static uint16_t enabled_core_ids[RTE_MAX_LCORE];
> -static uint8_t num_cores;
> +static unsigned int num_cores;
>
>  static uint32_t *keys;
>  #define TOTAL_ENTRY (1024 * 8)
> @@ -125,8 +125,8 @@ test_rcu_qsbr_writer_perf(void *arg)
>  static int
>  test_rcu_qsbr_perf(void)
>  {
> -       int i, sz;
> -       int tmp_num_cores;
> +       unsigned int i, sz;
>

sz is supposed to be a size_t.


+       unsigned int tmp_num_cores;
>
>         writer_done = 0;
>
> @@ -188,8 +188,8 @@ test_rcu_qsbr_perf(void)
>  static int
>  test_rcu_qsbr_rperf(void)
>  {
> -       int i, sz;
> -       int tmp_num_cores;
> +       unsigned int i, sz;
>

Idem.


+       unsigned int tmp_num_cores;
>
>         rte_atomic64_clear(&updates);
>         rte_atomic64_clear(&update_cycles);
> @@ -234,7 +234,7 @@ test_rcu_qsbr_rperf(void)
>  static int
>  test_rcu_qsbr_wperf(void)
>  {
> -       int i, sz;
> +       unsigned int i, sz;
>
>         rte_atomic64_clear(&checks);
>         rte_atomic64_clear(&check_cycles);
> @@ -378,7 +378,7 @@ static int
>  test_rcu_qsbr_sw_sv_1qs(void)
>  {
>         uint64_t token, begin, cycles;
> -       int i, j, tmp_num_cores, sz;
> +       unsigned int i, j, tmp_num_cores, sz;
>         int32_t pos;
>
>         writer_done = 0;
> @@ -496,7 +496,7 @@ static int
>  test_rcu_qsbr_sw_sv_1qs_non_blocking(void)
>  {
>         uint64_t token, begin, cycles;
> -       int i, j, ret, tmp_num_cores, sz;
> +       unsigned int i, j, ret, tmp_num_cores, sz;
>

Idem.
ret is an int.


        int32_t pos;
>
>         writer_done = 0;
> --
> 2.17.1
>
>
Let's focus on just the issue we want to fix and drop the changes on sz and
ret.

If you send a follow up patch on the issues for sz and ret, please also
consider changing enabled_core_ids as an unsigned int array.
Thanks.

-- 
David Marchand

[dpdk-dev] [PATCH v3 1/2] test/rcu: increase the size of num cores variable

[Honnappa Nagarahalli]

num_cores is of type uint8_t. This results in the following
compilation error.

test_rcu_qsbr_perf.c:649:16: error: comparison is always false
due to limited range of data type [-Werror=type-limits]
  if (num_cores >= RTE_MAX_LCORE) {
                ^~

RTE_MAX_LCORE is set to 256 for armv8 config.

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>
---
v3
- Dropped accidental/incorrect type changes to 'ret' and 'sz' variables (David)

v2
- Changed type of num_cores to 'unsigned int' and
  related changes (David/Thomas)

 app/test/test_rcu_qsbr.c      |  8 ++++----
 app/test/test_rcu_qsbr_perf.c | 19 +++++++++++--------
 2 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/app/test/test_rcu_qsbr.c b/app/test/test_rcu_qsbr.c
index 943a1e370..ae359a987 100644
--- a/app/test/test_rcu_qsbr.c
+++ b/app/test/test_rcu_qsbr.c
@@ -27,7 +27,7 @@
 #define TEST_RCU_QSBR_CNT_INIT 1
 
 uint16_t enabled_core_ids[RTE_MAX_LCORE];
-uint8_t num_cores;
+unsigned int num_cores;
 
 static uint32_t *keys;
 #define TOTAL_ENTRY (1024 * 8)
@@ -389,7 +389,7 @@ test_rcu_qsbr_synchronize_reader(void *arg)
 static int
 test_rcu_qsbr_synchronize(void)
 {
-	int i;
+	unsigned int i;
 
 	printf("\nTest rte_rcu_qsbr_synchronize()\n");
 
@@ -890,8 +890,8 @@ test_rcu_qsbr_sw_sv_3qs(void)
 static int
 test_rcu_qsbr_mw_mv_mqs(void)
 {
-	int i, j;
-	uint8_t test_cores;
+	unsigned int i, j;
+	unsigned int test_cores;
 
 	writer_done = 0;
 	test_cores = num_cores / 4;
diff --git a/app/test/test_rcu_qsbr_perf.c b/app/test/test_rcu_qsbr_perf.c
index 363365f46..cb2d177b7 100644
--- a/app/test/test_rcu_qsbr_perf.c
+++ b/app/test/test_rcu_qsbr_perf.c
@@ -17,7 +17,7 @@
 
 /* Check condition and return an error if true. */
 static uint16_t enabled_core_ids[RTE_MAX_LCORE];
-static uint8_t num_cores;
+static unsigned int num_cores;
 
 static uint32_t *keys;
 #define TOTAL_ENTRY (1024 * 8)
@@ -125,8 +125,8 @@ test_rcu_qsbr_writer_perf(void *arg)
 static int
 test_rcu_qsbr_perf(void)
 {
-	int i, sz;
-	int tmp_num_cores;
+	int sz;
+	unsigned int i, tmp_num_cores;
 
 	writer_done = 0;
 
@@ -188,8 +188,8 @@ test_rcu_qsbr_perf(void)
 static int
 test_rcu_qsbr_rperf(void)
 {
-	int i, sz;
-	int tmp_num_cores;
+	int sz;
+	unsigned int i, tmp_num_cores;
 
 	rte_atomic64_clear(&updates);
 	rte_atomic64_clear(&update_cycles);
@@ -234,7 +234,8 @@ test_rcu_qsbr_rperf(void)
 static int
 test_rcu_qsbr_wperf(void)
 {
-	int i, sz;
+	int sz;
+	unsigned int i;
 
 	rte_atomic64_clear(&checks);
 	rte_atomic64_clear(&check_cycles);
@@ -378,7 +379,8 @@ static int
 test_rcu_qsbr_sw_sv_1qs(void)
 {
 	uint64_t token, begin, cycles;
-	int i, j, tmp_num_cores, sz;
+	int sz;
+	unsigned int i, j, tmp_num_cores;
 	int32_t pos;
 
 	writer_done = 0;
@@ -496,7 +498,8 @@ static int
 test_rcu_qsbr_sw_sv_1qs_non_blocking(void)
 {
 	uint64_t token, begin, cycles;
-	int i, j, ret, tmp_num_cores, sz;
+	int ret, sz;
+	unsigned int i, j, tmp_num_cores;
 	int32_t pos;
 
 	writer_done = 0;
-- 
2.17.1

[dpdk-dev] [PATCH v3 2/2] test/rcu: address test case failure

[Honnappa Nagarahalli]

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>
Reviewed-by: David Marchand <david.marchand@redhat.com>
---
v3
- None

v2
- Changed the test case result validation (David)

 app/test/test_rcu_qsbr.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/app/test/test_rcu_qsbr.c b/app/test/test_rcu_qsbr.c
index ae359a987..1d273e39d 100644
--- a/app/test/test_rcu_qsbr.c
+++ b/app/test/test_rcu_qsbr.c
@@ -89,13 +89,17 @@ 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");
+	if (RTE_CACHE_LINE_SIZE == 64)
+		TEST_RCU_QSBR_RETURN_IF_ERROR((sz != 8384),
+			"Get Memsize for 128 threads");
+	else if (RTE_CACHE_LINE_SIZE == 128)
+		TEST_RCU_QSBR_RETURN_IF_ERROR((sz != 16768),
+			"Get Memsize for 128 threads");
 
 	return 0;
 }
-- 
2.17.1

Re: [dpdk-dev] [PATCH v3 1/2] test/rcu: increase the size of num cores variable

[David Marchand]

On Fri, Jun 28, 2019 at 8:44 PM Honnappa Nagarahalli <
honnappa.nagarahalli@arm.com> wrote:

> num_cores is of type uint8_t. This results in the following
> compilation error.
>
> test_rcu_qsbr_perf.c:649:16: error: comparison is always false
> due to limited range of data type [-Werror=type-limits]
>   if (num_cores >= RTE_MAX_LCORE) {
>                 ^~
>
> RTE_MAX_LCORE is set to 256 for armv8 config.
>
> 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>
> ---
> v3
> - Dropped accidental/incorrect type changes to 'ret' and 'sz' variables
> (David)
>
> v2
> - Changed type of num_cores to 'unsigned int' and
>   related changes (David/Thomas)
>
>  app/test/test_rcu_qsbr.c      |  8 ++++----
>  app/test/test_rcu_qsbr_perf.c | 19 +++++++++++--------
>  2 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/app/test/test_rcu_qsbr.c b/app/test/test_rcu_qsbr.c
> index 943a1e370..ae359a987 100644
> --- a/app/test/test_rcu_qsbr.c
> +++ b/app/test/test_rcu_qsbr.c
> @@ -27,7 +27,7 @@
>  #define TEST_RCU_QSBR_CNT_INIT 1
>
>  uint16_t enabled_core_ids[RTE_MAX_LCORE];
> -uint8_t num_cores;
> +unsigned int num_cores;
>
>  static uint32_t *keys;
>  #define TOTAL_ENTRY (1024 * 8)
> @@ -389,7 +389,7 @@ test_rcu_qsbr_synchronize_reader(void *arg)
>  static int
>  test_rcu_qsbr_synchronize(void)
>  {
> -       int i;
> +       unsigned int i;
>
>         printf("\nTest rte_rcu_qsbr_synchronize()\n");
>
> @@ -890,8 +890,8 @@ test_rcu_qsbr_sw_sv_3qs(void)
>  static int
>  test_rcu_qsbr_mw_mv_mqs(void)
>  {
> -       int i, j;
> -       uint8_t test_cores;
> +       unsigned int i, j;
> +       unsigned int test_cores;
>
>         writer_done = 0;
>         test_cores = num_cores / 4;
> diff --git a/app/test/test_rcu_qsbr_perf.c b/app/test/test_rcu_qsbr_perf.c
> index 363365f46..cb2d177b7 100644
> --- a/app/test/test_rcu_qsbr_perf.c
> +++ b/app/test/test_rcu_qsbr_perf.c
> @@ -17,7 +17,7 @@
>
>  /* Check condition and return an error if true. */
>  static uint16_t enabled_core_ids[RTE_MAX_LCORE];
> -static uint8_t num_cores;
> +static unsigned int num_cores;
>
>  static uint32_t *keys;
>  #define TOTAL_ENTRY (1024 * 8)
> @@ -125,8 +125,8 @@ test_rcu_qsbr_writer_perf(void *arg)
>  static int
>  test_rcu_qsbr_perf(void)
>  {
> -       int i, sz;
> -       int tmp_num_cores;
> +       int sz;
> +       unsigned int i, tmp_num_cores;
>
>         writer_done = 0;
>
> @@ -188,8 +188,8 @@ test_rcu_qsbr_perf(void)
>  static int
>  test_rcu_qsbr_rperf(void)
>  {
> -       int i, sz;
> -       int tmp_num_cores;
> +       int sz;
> +       unsigned int i, tmp_num_cores;
>
>         rte_atomic64_clear(&updates);
>         rte_atomic64_clear(&update_cycles);
> @@ -234,7 +234,8 @@ test_rcu_qsbr_rperf(void)
>  static int
>  test_rcu_qsbr_wperf(void)
>  {
> -       int i, sz;
> +       int sz;
> +       unsigned int i;
>
>         rte_atomic64_clear(&checks);
>         rte_atomic64_clear(&check_cycles);
> @@ -378,7 +379,8 @@ static int
>  test_rcu_qsbr_sw_sv_1qs(void)
>  {
>         uint64_t token, begin, cycles;
> -       int i, j, tmp_num_cores, sz;
> +       int sz;
> +       unsigned int i, j, tmp_num_cores;
>         int32_t pos;
>
>         writer_done = 0;
> @@ -496,7 +498,8 @@ static int
>  test_rcu_qsbr_sw_sv_1qs_non_blocking(void)
>  {
>         uint64_t token, begin, cycles;
> -       int i, j, ret, tmp_num_cores, sz;
> +       int ret, sz;
> +       unsigned int i, j, tmp_num_cores;
>         int32_t pos;
>
>         writer_done = 0;
> --
> 2.17.1
>
>

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


Bon week-end !

-- 
David Marchand

Re: [dpdk-dev] [dpdk-stable] [PATCH v3 1/2] test/rcu: increase the size of num cores variable

[Thomas Monjalon]

28/06/2019 20:54, David Marchand:
> On Fri, Jun 28, 2019 at 8:44 PM Honnappa Nagarahalli <
> honnappa.nagarahalli@arm.com> wrote:
> 
> > num_cores is of type uint8_t. This results in the following
> > compilation error.
> >
> > test_rcu_qsbr_perf.c:649:16: error: comparison is always false
> > due to limited range of data type [-Werror=type-limits]
> >   if (num_cores >= RTE_MAX_LCORE) {
> >                 ^~
> >
> > RTE_MAX_LCORE is set to 256 for armv8 config.
> >
> > 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>
> 
> Thanks.
> Reviewed-by: David Marchand <david.marchand@redhat.com>

Series applied, thanks