[v1,3/3] test/hash: add readwrite test for ext table

Message ID 1539208085-30756-4-git-send-email-yipeng1.wang@intel.com
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • Improvements over rte hash and tests
Related show

Checks

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

Commit Message

Yipeng Wang Oct. 10, 2018, 9:48 p.m.
This commit improves the readwrite test to consider
extendable table feature and add more functional tests
to cover more corner cases.

Signed-off-by: Yipeng Wang <yipeng1.wang@intel.com>
---
 test/test/test_hash_readwrite.c | 70 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 58 insertions(+), 12 deletions(-)

Comments

Bruce Richardson Oct. 24, 2018, 8:36 p.m. | #1
On Wed, Oct 10, 2018 at 02:48:05PM -0700, Yipeng Wang wrote:
> This commit improves the readwrite test to consider extendable table
> feature and add more functional tests to cover more corner cases.
> 
> Signed-off-by: Yipeng Wang <yipeng1.wang@intel.com> ---
> test/test/test_hash_readwrite.c | 70
> ++++++++++++++++++++++++++++++++++------- 1 file changed, 58
> insertions(+), 12 deletions(-)
> 
With the extension of this test case, and the addition of other test cases
by Honnappa in the other patch sets in this release, we are building up
quite a large set of hash table autotests, some of whose meaning and use is
a bit obscure. Are there any hash tests that you feel could be removed at
this point, to simplify things?

/Bruce
Yipeng Wang Oct. 25, 2018, 1:06 a.m. | #2
Thanks for the comment Bruce,

I agree with you.  As I think about it, I believe the old scaling test can be removed since currently the
multiwriter is supported inside the library, and we already have the multiwriter test.

I will post a V2 version of this patch series to remove that test.

>-----Original Message-----
>From: Richardson, Bruce
>Sent: Wednesday, October 24, 2018 1:37 PM
>To: Wang, Yipeng1 <yipeng1.wang@intel.com>
>Cc: stephen@networkplumber.org; dev@dpdk.org; honnappa.nagarahalli@arm.com; Gobriel, Sameh <sameh.gobriel@intel.com>
>Subject: Re: [PATCH v1 3/3] test/hash: add readwrite test for ext table
>
>On Wed, Oct 10, 2018 at 02:48:05PM -0700, Yipeng Wang wrote:
>> This commit improves the readwrite test to consider extendable table
>> feature and add more functional tests to cover more corner cases.
>>
>> Signed-off-by: Yipeng Wang <yipeng1.wang@intel.com> ---
>> test/test/test_hash_readwrite.c | 70
>> ++++++++++++++++++++++++++++++++++------- 1 file changed, 58
>> insertions(+), 12 deletions(-)
>>
>With the extension of this test case, and the addition of other test cases
>by Honnappa in the other patch sets in this release, we are building up
>quite a large set of hash table autotests, some of whose meaning and use is
>a bit obscure. Are there any hash tests that you feel could be removed at
>this point, to simplify things?
>
>/Bruce
Honnappa Nagarahalli Oct. 26, 2018, 12:23 a.m. | #3
> 
> On Wed, Oct 10, 2018 at 02:48:05PM -0700, Yipeng Wang wrote:
> > This commit improves the readwrite test to consider extendable table
> > feature and add more functional tests to cover more corner cases.
> >
> > Signed-off-by: Yipeng Wang <yipeng1.wang@intel.com> ---
> > test/test/test_hash_readwrite.c | 70
> > ++++++++++++++++++++++++++++++++++------- 1 file changed, 58
> > insertions(+), 12 deletions(-)
> >
> With the extension of this test case, and the addition of other test cases by
> Honnappa in the other patch sets in this release, we are building up quite a
> large set of hash table autotests, some of whose meaning and use is a bit
> obscure. Are there any hash tests that you feel could be removed at this point,
> to simplify things?
> 
(this comment does not apply to this patch)
Looks like your concern is about maintenance of the test code. 
IMO, we need to reduce the number of configuration flags in this library which should reduce the number of test cases.
The flags I think that are not necessary are:
RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT - The tests prove that this gives significant performance boost. IMO, if the platform supports it, it should be enabled without user consent (I am not an expert on TSX).
RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY - Most use cases require this support. Only use case where this is not required is a single thread doing both inserts and lookups. Even if such a use case is valid, the lock over head should be small.

> /Bruce
Bruce Richardson Oct. 26, 2018, 10:12 a.m. | #4
On Fri, Oct 26, 2018 at 12:23:56AM +0000, Honnappa Nagarahalli wrote:
> > 
> > On Wed, Oct 10, 2018 at 02:48:05PM -0700, Yipeng Wang wrote:
> > > This commit improves the readwrite test to consider extendable table
> > > feature and add more functional tests to cover more corner cases.
> > >
> > > Signed-off-by: Yipeng Wang <yipeng1.wang@intel.com> ---
> > > test/test/test_hash_readwrite.c | 70
> > > ++++++++++++++++++++++++++++++++++------- 1 file changed, 58
> > > insertions(+), 12 deletions(-)
> > >
> > With the extension of this test case, and the addition of other test cases by
> > Honnappa in the other patch sets in this release, we are building up quite a
> > large set of hash table autotests, some of whose meaning and use is a bit
> > obscure. Are there any hash tests that you feel could be removed at this point,
> > to simplify things?
> > 
> (this comment does not apply to this patch)
> Looks like your concern is about maintenance of the test code. 
> IMO, we need to reduce the number of configuration flags in this library which should reduce the number of test cases.
> The flags I think that are not necessary are:
> RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT - The tests prove that this gives significant performance boost. IMO, if the platform supports it, it should be enabled without user consent (I am not an expert on TSX).
> RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY - Most use cases require this support. Only use case where this is not required is a single thread doing both inserts and lookups. Even if such a use case is valid, the lock over head should be small.
>
I agree with the idea. What I suggest is that only a single flag should be
needed, and that only for the uncommon case, i.e. where we do not need any
locking of the hash-table. Otherwise the hash should be thread safe by
default and using the most effective locking mechanism for the platform.

Unfortunately, doing this requires an ABI change, but since it only should
affect the create function, it should be doable with function versioning to
keep backward compatibility.

/Bruce
Honnappa Nagarahalli Oct. 29, 2018, 5:54 a.m. | #5
> > > On Wed, Oct 10, 2018 at 02:48:05PM -0700, Yipeng Wang wrote:
> > > > This commit improves the readwrite test to consider extendable
> > > > table feature and add more functional tests to cover more corner cases.
> > > >
> > > > Signed-off-by: Yipeng Wang <yipeng1.wang@intel.com> ---
> > > > test/test/test_hash_readwrite.c | 70
> > > > ++++++++++++++++++++++++++++++++++------- 1 file changed, 58
> > > > insertions(+), 12 deletions(-)
> > > >
> > > With the extension of this test case, and the addition of other test
> > > cases by Honnappa in the other patch sets in this release, we are
> > > building up quite a large set of hash table autotests, some of whose
> > > meaning and use is a bit obscure. Are there any hash tests that you
> > > feel could be removed at this point, to simplify things?
> > >
> > (this comment does not apply to this patch) Looks like your concern is
> > about maintenance of the test code.
> > IMO, we need to reduce the number of configuration flags in this library
> which should reduce the number of test cases.
> > The flags I think that are not necessary are:
> > RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT - The tests prove that
> this gives significant performance boost. IMO, if the platform supports it, it
> should be enabled without user consent (I am not an expert on TSX).
> > RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY - Most use cases require this
> support. Only use case where this is not required is a single thread doing both
> inserts and lookups. Even if such a use case is valid, the lock over head should
> be small.
> >
> I agree with the idea. What I suggest is that only a single flag should be
> needed, and that only for the uncommon case, i.e. where we do not need any
> locking of the hash-table. Otherwise the hash should be thread safe by default
> and using the most effective locking mechanism for the platform.
> 
RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF - should take care of this.

> Unfortunately, doing this requires an ABI change, but since it only should
> affect the create function, it should be doable with function versioning to
> keep backward compatibility.
> 
Looks simple enough. Version the rte_hash_create function and map the existing function to 18.08. The new version of the function will always enable hw_trans_mem_support and rw_concurrency. Should we check to see if these flags are set by the user and print a warning message about deprecation of these flags in the newer version of the function?

> /Bruce
Honnappa Nagarahalli Oct. 31, 2018, 4:21 a.m. | #6
> On Fri, Oct 26, 2018 at 12:23:56AM +0000, Honnappa Nagarahalli wrote:
> > >
> > > On Wed, Oct 10, 2018 at 02:48:05PM -0700, Yipeng Wang wrote:
> > > > This commit improves the readwrite test to consider extendable
> > > > table feature and add more functional tests to cover more corner cases.
> > > >
> > > > Signed-off-by: Yipeng Wang <yipeng1.wang@intel.com> ---
> > > > test/test/test_hash_readwrite.c | 70
> > > > ++++++++++++++++++++++++++++++++++------- 1 file changed, 58
> > > > insertions(+), 12 deletions(-)
> > > >
> > > With the extension of this test case, and the addition of other test
> > > cases by Honnappa in the other patch sets in this release, we are
> > > building up quite a large set of hash table autotests, some of whose
> > > meaning and use is a bit obscure. Are there any hash tests that you
> > > feel could be removed at this point, to simplify things?
> > >
> > (this comment does not apply to this patch) Looks like your concern is
> > about maintenance of the test code.
> > IMO, we need to reduce the number of configuration flags in this library
> which should reduce the number of test cases.
> > The flags I think that are not necessary are:
> > RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT - The tests prove that
> this gives significant performance boost. IMO, if the platform supports it, it
> should be enabled without user consent (I am not an expert on TSX).
> > RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY - Most use cases require this
> support. Only use case where this is not required is a single thread doing both
> inserts and lookups. Even if such a use case is valid, the lock over head should
> be small.
> >
> I agree with the idea. What I suggest is that only a single flag should be
> needed, and that only for the uncommon case, i.e. where we do not need any
> locking of the hash-table. Otherwise the hash should be thread safe by default
> and using the most effective locking mechanism for the platform.
> 
> Unfortunately, doing this requires an ABI change, but since it only should
> affect the create function, it should be doable with function versioning to
> keep backward compatibility.
> 
I have made the changes. It seems to be working fine. I will post it once internal review completes.

We made this change (SHA: 9d033dac7d7cacca9559e0381f99b4c730e80979) to support 'no free on delete'. This was done by introducing another configuration flag 'RTE_HASH_EXTRA_FLAGS_NO_FREE_ON_DEL'. IMO, it makes sense to keep delete and free as two different operations always and deprecate 'free during delete' support. We can provide backward compatibility by making ABI change instead of introducing another configuration flag.

> /Bruce

Patch

diff --git a/test/test/test_hash_readwrite.c b/test/test/test_hash_readwrite.c
index 2a4f7b9..1cb00ad 100644
--- a/test/test/test_hash_readwrite.c
+++ b/test/test/test_hash_readwrite.c
@@ -21,6 +21,8 @@ 
 #define TOTAL_ENTRY (16*1024*1024)
 #define TOTAL_INSERT (15*1024*1024)
 
+#define TOTAL_INSERT_EXT (16*1024*1024)
+
 #define NUM_TEST 3
 unsigned int core_cnt[NUM_TEST] = {2, 4, 8};
 
@@ -59,8 +61,10 @@  test_hash_readwrite_worker(__attribute__((unused)) void *arg)
 	uint64_t i, offset;
 	uint32_t lcore_id = rte_lcore_id();
 	uint64_t begin, cycles;
-	int ret;
+	int *ret;
 
+	ret = rte_malloc(NULL, sizeof(int) *
+				tbl_rw_test_param.num_insert, 0);
 	for (i = 0; i < rte_lcore_count(); i++) {
 		if (slave_core_ids[i] == lcore_id)
 			break;
@@ -79,13 +83,30 @@  test_hash_readwrite_worker(__attribute__((unused)) void *arg)
 				tbl_rw_test_param.keys + i) > 0)
 			break;
 
-		ret = rte_hash_add_key(tbl_rw_test_param.h,
+		ret[i - offset] = rte_hash_add_key(tbl_rw_test_param.h,
 				     tbl_rw_test_param.keys + i);
-		if (ret < 0)
+		if (ret[i - offset] < 0)
+			break;
+
+		/* lookup a random key */
+		uint32_t rand = rte_rand() % (i + 1 - offset);
+
+		if (rte_hash_lookup(tbl_rw_test_param.h,
+				tbl_rw_test_param.keys + rand) != ret[rand])
+			break;
+
+
+		if (rte_hash_del_key(tbl_rw_test_param.h,
+				tbl_rw_test_param.keys + rand) != ret[rand])
+			break;
+
+		ret[rand] = rte_hash_add_key(tbl_rw_test_param.h,
+					tbl_rw_test_param.keys + rand);
+		if (ret[rand] < 0)
 			break;
 
 		if (rte_hash_lookup(tbl_rw_test_param.h,
-				tbl_rw_test_param.keys + i) != ret)
+			tbl_rw_test_param.keys + rand) != ret[rand])
 			break;
 	}
 
@@ -100,7 +121,7 @@  test_hash_readwrite_worker(__attribute__((unused)) void *arg)
 }
 
 static int
-init_params(int use_htm, int use_jhash)
+init_params(int use_ext, int use_htm, int use_jhash)
 {
 	unsigned int i;
 
@@ -127,6 +148,13 @@  init_params(int use_htm, int use_jhash)
 		hash_params.extra_flag =
 			RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY;
 
+	if (use_ext)
+		hash_params.extra_flag |=
+			RTE_HASH_EXTRA_FLAGS_EXT_TABLE;
+	else
+		hash_params.extra_flag &=
+		       ~RTE_HASH_EXTRA_FLAGS_EXT_TABLE;
+
 	hash_params.name = "tests";
 
 	handle = rte_hash_create(&hash_params);
@@ -165,7 +193,7 @@  init_params(int use_htm, int use_jhash)
 }
 
 static int
-test_hash_readwrite_functional(int use_htm)
+test_hash_readwrite_functional(int use_ext, int use_htm)
 {
 	unsigned int i;
 	const void *next_key;
@@ -176,6 +204,7 @@  test_hash_readwrite_functional(int use_htm)
 	uint32_t lost_keys = 0;
 	int use_jhash = 1;
 	int slave_cnt = rte_lcore_count() - 1;
+	uint32_t tot_insert = 0;
 
 	rte_atomic64_init(&gcycles);
 	rte_atomic64_clear(&gcycles);
@@ -183,11 +212,16 @@  test_hash_readwrite_functional(int use_htm)
 	rte_atomic64_init(&ginsertions);
 	rte_atomic64_clear(&ginsertions);
 
-	if (init_params(use_htm, use_jhash) != 0)
+	if (init_params(use_ext, use_htm, use_jhash) != 0)
 		goto err;
 
+	if (use_ext)
+		tot_insert = TOTAL_INSERT_EXT;
+	else
+		tot_insert = TOTAL_INSERT;
+
 	tbl_rw_test_param.num_insert =
-		TOTAL_INSERT / slave_cnt;
+		tot_insert / slave_cnt;
 
 	tbl_rw_test_param.rounded_tot_insert =
 		tbl_rw_test_param.num_insert
@@ -343,7 +377,7 @@  test_hash_readwrite_perf(struct perf *perf_results, int use_htm,
 	rte_atomic64_init(&gwrite_cycles);
 	rte_atomic64_clear(&gwrite_cycles);
 
-	if (init_params(use_htm, use_jhash) != 0)
+	if (init_params(0, use_htm, use_jhash) != 0)
 		goto err;
 
 	/*
@@ -577,7 +611,7 @@  test_hash_readwrite_main(void)
 	 * than writer threads. This is to timing either reader threads or
 	 * writer threads for performance numbers.
 	 */
-	int use_htm, reader_faster;
+	int use_htm, use_ext,  reader_faster;
 	unsigned int i = 0, core_id = 0;
 
 	if (rte_lcore_count() <= 2) {
@@ -600,7 +634,13 @@  test_hash_readwrite_main(void)
 		printf("Test read-write with Hardware transactional memory\n");
 
 		use_htm = 1;
-		if (test_hash_readwrite_functional(use_htm) < 0)
+		use_ext = 0;
+
+		if (test_hash_readwrite_functional(use_ext, use_htm) < 0)
+			return -1;
+
+		use_ext = 1;
+		if (test_hash_readwrite_functional(use_ext, use_htm) < 0)
 			return -1;
 
 		reader_faster = 1;
@@ -619,8 +659,14 @@  test_hash_readwrite_main(void)
 
 	printf("Test read-write without Hardware transactional memory\n");
 	use_htm = 0;
-	if (test_hash_readwrite_functional(use_htm) < 0)
+	use_ext = 0;
+	if (test_hash_readwrite_functional(use_ext, use_htm) < 0)
 		return -1;
+
+	use_ext = 1;
+	if (test_hash_readwrite_functional(use_ext, use_htm) < 0)
+		return -1;
+
 	reader_faster = 1;
 	if (test_hash_readwrite_perf(&non_htm_results, use_htm,
 							reader_faster) < 0)