diff mbox series

test/hash: reset iter and found in perf test

Message ID 20181129183855.9361-1-dharmik.thakkar@arm.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers show
Series test/hash: reset iter and found in perf test | expand

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS
ci/checkpatch success coding style OK

Commit Message

Dharmik Thakkar Nov. 29, 2018, 6:38 p.m. UTC
Reset 'iter' and 'tbl_rw_test_param.found' on each iteration
to give correct result for lost and duplicated keys.

Fixes: 0eb3726ebcf14 ("test/hash: add test for read/write concurrency")
Cc: stable@dpdk.org

Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
---
 test/test/test_hash_readwrite.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Ferruh Yigit Jan. 11, 2019, 5:10 p.m. UTC | #1
On 11/29/2018 6:38 PM, Dharmik Thakkar wrote:
> Reset 'iter' and 'tbl_rw_test_param.found' on each iteration
> to give correct result for lost and duplicated keys.
> 
> Fixes: 0eb3726ebcf14 ("test/hash: add test for read/write concurrency")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> ---
>  test/test/test_hash_readwrite.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/test/test/test_hash_readwrite.c b/test/test/test_hash_readwrite.c
> index 6b695ce6e444..be93a2ebd270 100644
> --- a/test/test/test_hash_readwrite.c
> +++ b/test/test/test_hash_readwrite.c
> @@ -361,7 +361,6 @@ test_hash_readwrite_perf(struct perf *perf_results, int use_htm,
>  
>  	const void *next_key;
>  	void *next_data;
> -	uint32_t iter = 0;
>  	int use_jhash = 0;
>  
>  	uint32_t duplicated_keys = 0;
> @@ -536,6 +535,8 @@ test_hash_readwrite_perf(struct perf *perf_results, int use_htm,
>  
>  		rte_eal_mp_wait_lcore();
>  
> +		uint32_t iter = 0;

Logically looks good. Only we don't tend to declare the variables in the middle
of the scope, you may prefer to keep deceleration at its place but set 'iter' to
zero here.

> +		memset(tbl_rw_test_param.found, 0, TOTAL_ENTRY);
>  		while (rte_hash_iterate(tbl_rw_test_param.h,
>  				&next_key, &next_data, &iter) >= 0) {
>  			/* Search for the key in the list of keys added .*/
> @@ -619,7 +620,7 @@ test_hash_readwrite_main(void)
>  	if (rte_lcore_count() <= 2) {
>  		printf("More than two lcores are required "
>  			"to do read write test\n");
> -		return 0;
> +		return -1;

This is something not mentioned in the commit log, changes the default return
value of test when not enough resources provided, cc'ed Yipeng for comment.

If decided to keep this change, please update commit log to mention from it.
Wang, Yipeng1 Jan. 11, 2019, 6:37 p.m. UTC | #2
Thanks for the bug fix! Nice catch!

>-----Original Message-----
>From: Yigit, Ferruh
>Sent: Friday, January 11, 2019 9:10 AM
>To: Dharmik Thakkar <dharmik.thakkar@arm.com>; Richardson, Bruce <bruce.richardson@intel.com>; De Lara Guarch, Pablo
><pablo.de.lara.guarch@intel.com>
>Cc: dev@dpdk.org; stable@dpdk.org; Wang, Yipeng1 <yipeng1.wang@intel.com>
>Subject: Re: [dpdk-stable] [PATCH] test/hash: reset iter and found in perf test
>>  	uint32_t duplicated_keys = 0;
>> @@ -536,6 +535,8 @@ test_hash_readwrite_perf(struct perf *perf_results, int use_htm,
>>
>>  		rte_eal_mp_wait_lcore();
>>
>> +		uint32_t iter = 0;
>
>Logically looks good. Only we don't tend to declare the variables in the middle
>of the scope, you may prefer to keep deceleration at its place but set 'iter' to
>zero here.
[Wang, Yipeng] Agree.
>
>> +		memset(tbl_rw_test_param.found, 0, TOTAL_ENTRY);

[Wang, Yipeng] test failed because of this line I think, the found is uint32_t array, so should be TOTAL_ENTRY * 4, or just change found to be int8

>>  		while (rte_hash_iterate(tbl_rw_test_param.h,
>>  				&next_key, &next_data, &iter) >= 0) {
>>  			/* Search for the key in the list of keys added .*/
>> @@ -619,7 +620,7 @@ test_hash_readwrite_main(void)
>>  	if (rte_lcore_count() <= 2) {
>>  		printf("More than two lcores are required "
>>  			"to do read write test\n");
>> -		return 0;
>> +		return -1;
>
>This is something not mentioned in the commit log, changes the default return
>value of test when not enough resources provided, cc'ed Yipeng for comment.
>
>If decided to keep this change, please update commit log to mention from it.
 [Wang, Yipeng] Yes it should be -1. Thanks for the fix. Please fix the commit msg as Ferruh suggested
Dharmik Thakkar Jan. 14, 2019, 7:12 a.m. UTC | #3
Thank you Ferruh and Yipeng! I will make the recommended changes and update the version.

> On Jan 12, 2019, at 12:07 AM, Wang, Yipeng1 <yipeng1.wang@intel.com> wrote:
> 
> Thanks for the bug fix! Nice catch!
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Friday, January 11, 2019 9:10 AM
>> To: Dharmik Thakkar <dharmik.thakkar@arm.com>; Richardson, Bruce <bruce.richardson@intel.com>; De Lara Guarch, Pablo
>> <pablo.de.lara.guarch@intel.com>
>> Cc: dev@dpdk.org; stable@dpdk.org; Wang, Yipeng1 <yipeng1.wang@intel.com>
>> Subject: Re: [dpdk-stable] [PATCH] test/hash: reset iter and found in perf test
>>> 	uint32_t duplicated_keys = 0;
>>> @@ -536,6 +535,8 @@ test_hash_readwrite_perf(struct perf *perf_results, int use_htm,
>>> 
>>> 		rte_eal_mp_wait_lcore();
>>> 
>>> +		uint32_t iter = 0;
>> 
>> Logically looks good. Only we don't tend to declare the variables in the middle
>> of the scope, you may prefer to keep deceleration at its place but set 'iter' to
>> zero here.
> [Wang, Yipeng] Agree.
>> 
>>> +		memset(tbl_rw_test_param.found, 0, TOTAL_ENTRY);
> 
> [Wang, Yipeng] test failed because of this line I think, the found is uint32_t array, so should be TOTAL_ENTRY * 4, or just change found to be int8
> 
>>> 		while (rte_hash_iterate(tbl_rw_test_param.h,
>>> 				&next_key, &next_data, &iter) >= 0) {
>>> 			/* Search for the key in the list of keys added .*/
>>> @@ -619,7 +620,7 @@ test_hash_readwrite_main(void)
>>> 	if (rte_lcore_count() <= 2) {
>>> 		printf("More than two lcores are required "
>>> 			"to do read write test\n");
>>> -		return 0;
>>> +		return -1;
>> 
>> This is something not mentioned in the commit log, changes the default return
>> value of test when not enough resources provided, cc'ed Yipeng for comment.
>> 
>> If decided to keep this change, please update commit log to mention from it.
> [Wang, Yipeng] Yes it should be -1. Thanks for the fix. Please fix the commit msg as Ferruh suggested
diff mbox series

Patch

diff --git a/test/test/test_hash_readwrite.c b/test/test/test_hash_readwrite.c
index 6b695ce6e444..be93a2ebd270 100644
--- a/test/test/test_hash_readwrite.c
+++ b/test/test/test_hash_readwrite.c
@@ -361,7 +361,6 @@  test_hash_readwrite_perf(struct perf *perf_results, int use_htm,
 
 	const void *next_key;
 	void *next_data;
-	uint32_t iter = 0;
 	int use_jhash = 0;
 
 	uint32_t duplicated_keys = 0;
@@ -536,6 +535,8 @@  test_hash_readwrite_perf(struct perf *perf_results, int use_htm,
 
 		rte_eal_mp_wait_lcore();
 
+		uint32_t iter = 0;
+		memset(tbl_rw_test_param.found, 0, TOTAL_ENTRY);
 		while (rte_hash_iterate(tbl_rw_test_param.h,
 				&next_key, &next_data, &iter) >= 0) {
 			/* Search for the key in the list of keys added .*/
@@ -619,7 +620,7 @@  test_hash_readwrite_main(void)
 	if (rte_lcore_count() <= 2) {
 		printf("More than two lcores are required "
 			"to do read write test\n");
-		return 0;
+		return -1;
 	}
 
 	RTE_LCORE_FOREACH_SLAVE(core_id) {