[dpdk-stable] [PATCH] test/hash: rectify slaveid to point to valid cores

patches for DPDK stable branches
 help / color / mirror / Atom feed

* [dpdk-stable] [PATCH] test/hash: rectify slaveid to point to valid cores
@ 2019-05-31 23:27 Dharmik Thakkar
  2019-06-01 15:59 ` David Marchand
  0 siblings, 1 reply; 6+ messages in thread
From: Dharmik Thakkar @ 2019-05-31 23:27 UTC (permalink / raw)
  To: Yipeng Wang, Sameh Gobriel, Bruce Richardson, Pablo de Lara
  Cc: dev, Dharmik Thakkar, stable

This patch rectifies slave_id passed to rte_eal_wait_lcore()
to point to valid cores in read-write lock-free concurrency test.

It also replaces a 'for' loop with RTE_LCORE_FOREACH API.

Fixes: dfd9d5537e876 ("test/hash: use existing lcore API")
Cc: stable@dpdk.org

Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 app/test/test_hash_readwrite_lf.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/app/test/test_hash_readwrite_lf.c b/app/test/test_hash_readwrite_lf.c
index 343a338b4ea8..af1ee9c34394 100644
--- a/app/test/test_hash_readwrite_lf.c
+++ b/app/test/test_hash_readwrite_lf.c
@@ -126,11 +126,9 @@ get_enabled_cores_list(void)
 	uint32_t i = 0;
 	uint16_t core_id;
 	uint32_t max_cores = rte_lcore_count();
-	for (core_id = 0; core_id < RTE_MAX_LCORE && i < max_cores; core_id++) {
-		if (rte_lcore_is_enabled(core_id)) {
-			enabled_core_ids[i] = core_id;
-			i++;
-		}
+	RTE_LCORE_FOREACH(core_id) {
+		enabled_core_ids[i] = core_id;
+		i++;
 	}
 
 	if (i != max_cores) {
@@ -738,7 +736,7 @@ test_hash_add_no_ks_lookup_hit(struct rwc_perf *rwc_perf_results, int rwc_lf,
 							enabled_core_ids[i]);
 
 			for (i = 1; i <= rwc_core_cnt[n]; i++)
-				if (rte_eal_wait_lcore(i) < 0)
+				if (rte_eal_wait_lcore(enabled_core_ids[i]) < 0)
 					goto err;
 
 			unsigned long long cycles_per_lookup =
@@ -810,7 +808,7 @@ test_hash_add_no_ks_lookup_miss(struct rwc_perf *rwc_perf_results, int rwc_lf,
 			if (ret < 0)
 				goto err;
 			for (i = 1; i <= rwc_core_cnt[n]; i++)
-				if (rte_eal_wait_lcore(i) < 0)
+				if (rte_eal_wait_lcore(enabled_core_ids[i]) < 0)
 					goto err;
 
 			unsigned long long cycles_per_lookup =
@@ -886,7 +884,7 @@ test_hash_add_ks_lookup_hit_non_sp(struct rwc_perf *rwc_perf_results,
 			if (ret < 0)
 				goto err;
 			for (i = 1; i <= rwc_core_cnt[n]; i++)
-				if (rte_eal_wait_lcore(i) < 0)
+				if (rte_eal_wait_lcore(enabled_core_ids[i]) < 0)
 					goto err;
 
 			unsigned long long cycles_per_lookup =
@@ -962,7 +960,7 @@ test_hash_add_ks_lookup_hit_sp(struct rwc_perf *rwc_perf_results, int rwc_lf,
 			if (ret < 0)
 				goto err;
 			for (i = 1; i <= rwc_core_cnt[n]; i++)
-				if (rte_eal_wait_lcore(i) < 0)
+				if (rte_eal_wait_lcore(enabled_core_ids[i]) < 0)
 					goto err;
 
 			unsigned long long cycles_per_lookup =
@@ -1037,7 +1035,7 @@ test_hash_add_ks_lookup_miss(struct rwc_perf *rwc_perf_results, int rwc_lf, int
 			if (ret < 0)
 				goto err;
 			for (i = 1; i <= rwc_core_cnt[n]; i++)
-				if (rte_eal_wait_lcore(i) < 0)
+				if (rte_eal_wait_lcore(enabled_core_ids[i]) < 0)
 					goto err;
 
 			unsigned long long cycles_per_lookup =
@@ -1132,12 +1130,12 @@ test_hash_multi_add_lookup(struct rwc_perf *rwc_perf_results, int rwc_lf,
 				for (i = rwc_core_cnt[n] + 1;
 				     i <= rwc_core_cnt[m] + rwc_core_cnt[n];
 				     i++)
-					rte_eal_wait_lcore(i);
+					rte_eal_wait_lcore(enabled_core_ids[i]);
 
 				writer_done = 1;
 
 				for (i = 1; i <= rwc_core_cnt[n]; i++)
-					if (rte_eal_wait_lcore(i) < 0)
+					if (rte_eal_wait_lcore(enabled_core_ids[i]) < 0)
 						goto err;
 
 				unsigned long long cycles_per_lookup =
@@ -1221,7 +1219,7 @@ test_hash_add_ks_lookup_hit_extbkt(struct rwc_perf *rwc_perf_results,
 			writer_done = 1;
 
 			for (i = 1; i <= rwc_core_cnt[n]; i++)
-				if (rte_eal_wait_lcore(i) < 0)
+				if (rte_eal_wait_lcore(enabled_core_ids[i]) < 0)
 					goto err;
 
 			unsigned long long cycles_per_lookup =
-- 
2.17.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-stable] [PATCH] test/hash: rectify slaveid to point to valid cores
  2019-05-31 23:27 [dpdk-stable] [PATCH] test/hash: rectify slaveid to point to valid cores Dharmik Thakkar
@ 2019-06-01 15:59 ` David Marchand
  2019-06-03 16:56   ` Dharmik Thakkar
  0 siblings, 1 reply; 6+ messages in thread
From: David Marchand @ 2019-06-01 15:59 UTC (permalink / raw)
  To: Dharmik Thakkar
  Cc: Yipeng Wang, Sameh Gobriel, Bruce Richardson, Pablo de Lara, dev,
	dpdk stable, Michael Santana

On Sat, Jun 1, 2019 at 1:28 AM Dharmik Thakkar <dharmik.thakkar@arm.com>
wrote:

> This patch rectifies slave_id passed to rte_eal_wait_lcore()
> to point to valid cores in read-write lock-free concurrency test.
>
> It also replaces a 'for' loop with RTE_LCORE_FOREACH API.
>
> Fixes: dfd9d5537e876 ("test/hash: use existing lcore API")
>

This incriminated commit only converts direct access to lcore_config into
call to rte_eal_wait_lcore.
So it did not introduce the issue you want to fix.

The Fixes: tag should probably be:
Fixes: c7eb0972e74b ("test/hash: add lock-free r/w concurrency")


Cc: stable@dpdk.org
>
> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
>  app/test/test_hash_readwrite_lf.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/app/test/test_hash_readwrite_lf.c
> b/app/test/test_hash_readwrite_lf.c
> index 343a338b4ea8..af1ee9c34394 100644
> --- a/app/test/test_hash_readwrite_lf.c
> +++ b/app/test/test_hash_readwrite_lf.c
> @@ -126,11 +126,9 @@ get_enabled_cores_list(void)
>         uint32_t i = 0;
>         uint16_t core_id;
>         uint32_t max_cores = rte_lcore_count();
> -       for (core_id = 0; core_id < RTE_MAX_LCORE && i < max_cores;
> core_id++) {
> -               if (rte_lcore_is_enabled(core_id)) {
> -                       enabled_core_ids[i] = core_id;
> -                       i++;
> -               }
> +       RTE_LCORE_FOREACH(core_id) {
> +               enabled_core_ids[i] = core_id;
> +               i++;
>         }
>
>         if (i != max_cores) {
> @@ -738,7 +736,7 @@ test_hash_add_no_ks_lookup_hit(struct rwc_perf
> *rwc_perf_results, int rwc_lf,
>
> enabled_core_ids[i]);
>
>                         for (i = 1; i <= rwc_core_cnt[n]; i++)
> -                               if (rte_eal_wait_lcore(i) < 0)
> +                               if
> (rte_eal_wait_lcore(enabled_core_ids[i]) < 0)
>                                         goto err;
>
>                         unsigned long long cycles_per_lookup =
> @@ -810,7 +808,7 @@ test_hash_add_no_ks_lookup_miss(struct rwc_perf
> *rwc_perf_results, int rwc_lf,
>                         if (ret < 0)
>                                 goto err;
>                         for (i = 1; i <= rwc_core_cnt[n]; i++)
> -                               if (rte_eal_wait_lcore(i) < 0)
> +                               if
> (rte_eal_wait_lcore(enabled_core_ids[i]) < 0)
>                                         goto err;
>
>                         unsigned long long cycles_per_lookup =
> @@ -886,7 +884,7 @@ test_hash_add_ks_lookup_hit_non_sp(struct rwc_perf
> *rwc_perf_results,
>                         if (ret < 0)
>                                 goto err;
>                         for (i = 1; i <= rwc_core_cnt[n]; i++)
> -                               if (rte_eal_wait_lcore(i) < 0)
> +                               if
> (rte_eal_wait_lcore(enabled_core_ids[i]) < 0)
>                                         goto err;
>
>                         unsigned long long cycles_per_lookup =
> @@ -962,7 +960,7 @@ test_hash_add_ks_lookup_hit_sp(struct rwc_perf
> *rwc_perf_results, int rwc_lf,
>                         if (ret < 0)
>                                 goto err;
>                         for (i = 1; i <= rwc_core_cnt[n]; i++)
> -                               if (rte_eal_wait_lcore(i) < 0)
> +                               if
> (rte_eal_wait_lcore(enabled_core_ids[i]) < 0)
>                                         goto err;
>
>                         unsigned long long cycles_per_lookup =
> @@ -1037,7 +1035,7 @@ test_hash_add_ks_lookup_miss(struct rwc_perf
> *rwc_perf_results, int rwc_lf, int
>                         if (ret < 0)
>                                 goto err;
>                         for (i = 1; i <= rwc_core_cnt[n]; i++)
> -                               if (rte_eal_wait_lcore(i) < 0)
> +                               if
> (rte_eal_wait_lcore(enabled_core_ids[i]) < 0)
>                                         goto err;
>
>                         unsigned long long cycles_per_lookup =
> @@ -1132,12 +1130,12 @@ test_hash_multi_add_lookup(struct rwc_perf
> *rwc_perf_results, int rwc_lf,
>                                 for (i = rwc_core_cnt[n] + 1;
>                                      i <= rwc_core_cnt[m] +
> rwc_core_cnt[n];
>                                      i++)
> -                                       rte_eal_wait_lcore(i);
> +
>  rte_eal_wait_lcore(enabled_core_ids[i]);
>
>                                 writer_done = 1;
>
>                                 for (i = 1; i <= rwc_core_cnt[n]; i++)
> -                                       if (rte_eal_wait_lcore(i) < 0)
> +                                       if
> (rte_eal_wait_lcore(enabled_core_ids[i]) < 0)
>                                                 goto err;
>
>                                 unsigned long long cycles_per_lookup =
>

Checkpatch complains here.


@@ -1221,7 +1219,7 @@ test_hash_add_ks_lookup_hit_extbkt(struct rwc_perf
> *rwc_perf_results,
>                         writer_done = 1;
>
>                         for (i = 1; i <= rwc_core_cnt[n]; i++)
> -                               if (rte_eal_wait_lcore(i) < 0)
> +                               if
> (rte_eal_wait_lcore(enabled_core_ids[i]) < 0)
>                                         goto err;
>
>                         unsigned long long cycles_per_lookup =
> --
> 2.17.1
>
>
The rest looks good to me.

We have accumulated quite some fixes with Michael on app/test.
Do you mind if I take your patch as part of our series?
I would change the Fixes: tag, fix the checkpatch warning, and send it next
week.

Bon week-end.


-- 
David Marchand

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-stable] [PATCH] test/hash: rectify slaveid to point to valid cores
  2019-06-01 15:59 ` David Marchand
@ 2019-06-03 16:56   ` Dharmik Thakkar
  2019-06-03 19:31     ` David Marchand
  2019-06-05 17:30     ` Thomas Monjalon
  0 siblings, 2 replies; 6+ messages in thread
From: Dharmik Thakkar @ 2019-06-03 16:56 UTC (permalink / raw)
  To: David Marchand
  Cc: Yipeng Wang, Sameh Gobriel, Bruce Richardson, Pablo de Lara, dev,
	dpdk stable, Michael Santana, nd

Hi David,

Thank you for your comments!

> On Jun 1, 2019, at 10:59 AM, David Marchand <david.marchand@redhat.com> wrote:
> 
> On Sat, Jun 1, 2019 at 1:28 AM Dharmik Thakkar <dharmik.thakkar@arm.com> wrote:
> This patch rectifies slave_id passed to rte_eal_wait_lcore()
> to point to valid cores in read-write lock-free concurrency test.
> 
> It also replaces a 'for' loop with RTE_LCORE_FOREACH API.
> 
> Fixes: dfd9d5537e876 ("test/hash: use existing lcore API")
> 
> This incriminated commit only converts direct access to lcore_config into call to rte_eal_wait_lcore.
> So it did not introduce the issue you want to fix.
> 
> The Fixes: tag should probably be:
> Fixes: c7eb0972e74b ("test/hash: add lock-free r/w concurrency”)
Yes, you are essentially correct. However, 'git blame' pointed to dfd9d5537e876 ("test/hash: use existing lcore API”).
> 
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
>  app/test/test_hash_readwrite_lf.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/app/test/test_hash_readwrite_lf.c b/app/test/test_hash_readwrite_lf.c
> index 343a338b4ea8..af1ee9c34394 100644
> --- a/app/test/test_hash_readwrite_lf.c
> +++ b/app/test/test_hash_readwrite_lf.c
> @@ -126,11 +126,9 @@ get_enabled_cores_list(void)
>         uint32_t i = 0;
>         uint16_t core_id;
>         uint32_t max_cores = rte_lcore_count();
> -       for (core_id = 0; core_id < RTE_MAX_LCORE && i < max_cores; core_id++) {
> -               if (rte_lcore_is_enabled(core_id)) {
> -                       enabled_core_ids[i] = core_id;
> -                       i++;
> -               }
> +       RTE_LCORE_FOREACH(core_id) {
> +               enabled_core_ids[i] = core_id;
> +               i++;
>         }
> 
>         if (i != max_cores) {
> @@ -738,7 +736,7 @@ test_hash_add_no_ks_lookup_hit(struct rwc_perf *rwc_perf_results, int rwc_lf,
>                                                         enabled_core_ids[i]);
> 
>                         for (i = 1; i <= rwc_core_cnt[n]; i++)
> -                               if (rte_eal_wait_lcore(i) < 0)
> +                               if (rte_eal_wait_lcore(enabled_core_ids[i]) < 0)
>                                         goto err;
> 
>                         unsigned long long cycles_per_lookup =
> @@ -810,7 +808,7 @@ test_hash_add_no_ks_lookup_miss(struct rwc_perf *rwc_perf_results, int rwc_lf,
>                         if (ret < 0)
>                                 goto err;
>                         for (i = 1; i <= rwc_core_cnt[n]; i++)
> -                               if (rte_eal_wait_lcore(i) < 0)
> +                               if (rte_eal_wait_lcore(enabled_core_ids[i]) < 0)
>                                         goto err;
> 
>                         unsigned long long cycles_per_lookup =
> @@ -886,7 +884,7 @@ test_hash_add_ks_lookup_hit_non_sp(struct rwc_perf *rwc_perf_results,
>                         if (ret < 0)
>                                 goto err;
>                         for (i = 1; i <= rwc_core_cnt[n]; i++)
> -                               if (rte_eal_wait_lcore(i) < 0)
> +                               if (rte_eal_wait_lcore(enabled_core_ids[i]) < 0)
>                                         goto err;
> 
>                         unsigned long long cycles_per_lookup =
> @@ -962,7 +960,7 @@ test_hash_add_ks_lookup_hit_sp(struct rwc_perf *rwc_perf_results, int rwc_lf,
>                         if (ret < 0)
>                                 goto err;
>                         for (i = 1; i <= rwc_core_cnt[n]; i++)
> -                               if (rte_eal_wait_lcore(i) < 0)
> +                               if (rte_eal_wait_lcore(enabled_core_ids[i]) < 0)
>                                         goto err;
> 
>                         unsigned long long cycles_per_lookup =
> @@ -1037,7 +1035,7 @@ test_hash_add_ks_lookup_miss(struct rwc_perf *rwc_perf_results, int rwc_lf, int
>                         if (ret < 0)
>                                 goto err;
>                         for (i = 1; i <= rwc_core_cnt[n]; i++)
> -                               if (rte_eal_wait_lcore(i) < 0)
> +                               if (rte_eal_wait_lcore(enabled_core_ids[i]) < 0)
>                                         goto err;
> 
>                         unsigned long long cycles_per_lookup =
> @@ -1132,12 +1130,12 @@ test_hash_multi_add_lookup(struct rwc_perf *rwc_perf_results, int rwc_lf,
>                                 for (i = rwc_core_cnt[n] + 1;
>                                      i <= rwc_core_cnt[m] + rwc_core_cnt[n];
>                                      i++)
> -                                       rte_eal_wait_lcore(i);
> +                                       rte_eal_wait_lcore(enabled_core_ids[i]);
> 
>                                 writer_done = 1;
> 
>                                 for (i = 1; i <= rwc_core_cnt[n]; i++)
> -                                       if (rte_eal_wait_lcore(i) < 0)
> +                                       if (rte_eal_wait_lcore(enabled_core_ids[i]) < 0)
>                                                 goto err;
> 
>                                 unsigned long long cycles_per_lookup =
> 
> Checkpatch complains here.
I believe you are referring to the warning of line over 80 characters in Line 1138. I think it should be fine.
> 
> 
> @@ -1221,7 +1219,7 @@ test_hash_add_ks_lookup_hit_extbkt(struct rwc_perf *rwc_perf_results,
>                         writer_done = 1;
> 
>                         for (i = 1; i <= rwc_core_cnt[n]; i++)
> -                               if (rte_eal_wait_lcore(i) < 0)
> +                               if (rte_eal_wait_lcore(enabled_core_ids[i]) < 0)
>                                         goto err;
> 
>                         unsigned long long cycles_per_lookup =
> -- 
> 2.17.1
> 
> 
> The rest looks good to me.
> 
> We have accumulated quite some fixes with Michael on app/test.
> Do you mind if I take your patch as part of our series?
Please, go ahead.
> I would change the Fixes: tag, fix the checkpatch warning, and send it next week.
I am not sure about this. Do we simply go by 'git blame' or insert the commit that truly introduced the error?
> 
> Bon week-end.
Thank you! You have a great week ahead.
> 
> 
> -- 
> David Marchand


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-stable] [PATCH] test/hash: rectify slaveid to point to valid cores
  2019-06-03 16:56   ` Dharmik Thakkar
@ 2019-06-03 19:31     ` David Marchand
  2019-06-03 20:43       ` Dharmik Thakkar
  2019-06-05 17:30     ` Thomas Monjalon
  1 sibling, 1 reply; 6+ messages in thread
From: David Marchand @ 2019-06-03 19:31 UTC (permalink / raw)
  To: Dharmik Thakkar
  Cc: Yipeng Wang, Sameh Gobriel, Bruce Richardson, Pablo de Lara, dev,
	dpdk stable, Michael Santana, nd

On Mon, Jun 3, 2019 at 6:57 PM Dharmik Thakkar <Dharmik.Thakkar@arm.com>
wrote:

> > On Jun 1, 2019, at 10:59 AM, David Marchand <david.marchand@redhat.com>
> wrote:
> > I would change the Fixes: tag, fix the checkpatch warning, and send it
> next week.
> I am not sure about this. Do we simply go by 'git blame' or insert the
> commit that truly introduced the error?
>

http://doc.dpdk.org/guides/contributing/patches.html#commit-messages-body
See the part about Fixes:.

The purpose of this tag is to point at the commit that introduced an issue
so that stable maintainers can catch it with scripts and backport it in the
relevant branches.


-- 
David Marchand

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-stable] [PATCH] test/hash: rectify slaveid to point to valid cores
  2019-06-03 19:31     ` David Marchand
@ 2019-06-03 20:43       ` Dharmik Thakkar
  0 siblings, 0 replies; 6+ messages in thread
From: Dharmik Thakkar @ 2019-06-03 20:43 UTC (permalink / raw)
  To: David Marchand
  Cc: Yipeng Wang, Sameh Gobriel, Bruce Richardson, Pablo de Lara, dev,
	dpdk stable, Michael Santana, nd



> On Jun 3, 2019, at 2:31 PM, David Marchand <david.marchand@redhat.com> wrote:
> 
> 
> On Mon, Jun 3, 2019 at 6:57 PM Dharmik Thakkar <Dharmik.Thakkar@arm.com> wrote:
> > On Jun 1, 2019, at 10:59 AM, David Marchand <david.marchand@redhat.com> wrote:
> > I would change the Fixes: tag, fix the checkpatch warning, and send it next week.
> I am not sure about this. Do we simply go by 'git blame' or insert the commit that truly introduced the error?
> 
> http://doc.dpdk.org/guides/contributing/patches.html#commit-messages-body
> See the part about Fixes:.
> 
> The purpose of this tag is to point at the commit that introduced an issue so that stable maintainers can catch it with scripts and backport it in the relevant branches.
Got it. Thank you!
> 
> 
> -- 
> David Marchand


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-stable] [PATCH] test/hash: rectify slaveid to point to valid cores
  2019-06-03 16:56   ` Dharmik Thakkar
  2019-06-03 19:31     ` David Marchand
@ 2019-06-05 17:30     ` Thomas Monjalon
  1 sibling, 0 replies; 6+ messages in thread
From: Thomas Monjalon @ 2019-06-05 17:30 UTC (permalink / raw)
  To: Dharmik Thakkar, David Marchand
  Cc: stable, Yipeng Wang, Sameh Gobriel, Bruce Richardson,
	Pablo de Lara, dev, Michael Santana, nd

03/06/2019 18:56, Dharmik Thakkar:
> > On Jun 1, 2019, at 10:59 AM, David Marchand <david.marchand@redhat.com> wrote:
> > We have accumulated quite some fixes with Michael on app/test.
> > Do you mind if I take your patch as part of our series?
> 
> Please, go ahead.

Superseded by https://patches.dpdk.org/patch/54336/



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-06-05 17:30 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-31 23:27 [dpdk-stable] [PATCH] test/hash: rectify slaveid to point to valid cores Dharmik Thakkar
2019-06-01 15:59 ` David Marchand
2019-06-03 16:56   ` Dharmik Thakkar
2019-06-03 19:31     ` David Marchand
2019-06-03 20:43       ` Dharmik Thakkar
2019-06-05 17:30     ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).