test/hash: rectify slaveid to point to valid cores
Checks
Commit Message
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(-)
Comments
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.
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
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.
> 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
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/
@@ -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 =