[dpdk-dev] [PATCH v1 0/3] Improvements over rte hash and tests

[dpdk-dev] [PATCH v1 0/3] Improvements over rte hash and tests

[Yipeng Wang]

This patch set depends on another rte hash patch set:
http://patchwork.dpdk.org/cover/46106/

This patch set includes three commits to improve rte_hash
and its unit tests.

1. Remove unnecessary pause code in rte_reset function.
2 and 3. Improve the unit test.

Signed-off-by: Yipeng Wang <yipeng1.wang@intel.com>

Yipeng Wang (3):
  hash: fix unnecessary pause
  test/hash: change multiwriter test to use jhash
  test/hash: add readwrite test for ext table

 lib/librte_hash/rte_cuckoo_hash.c |  6 ++--
 test/test/test_hash_multiwriter.c | 15 ++++++---
 test/test/test_hash_readwrite.c   | 70 ++++++++++++++++++++++++++++++++-------
 3 files changed, 71 insertions(+), 20 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH v1 1/3] hash: fix unnecessary pause

[Yipeng Wang]

There is a rte_pause in hash table reset function.
Since the loop is not a polling loop on shared
data structure, the rte_pause is not needed.

Fixes: b26473ff8f4a ("hash: add reset function")
Cc: stable@dpdk.org

Signed-off-by: Yipeng Wang <yipeng1.wang@intel.com>
---
 lib/librte_hash/rte_cuckoo_hash.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index 750caf8..ea8107e 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -521,14 +521,14 @@ rte_hash_reset(struct rte_hash *h)
 
 	/* clear the free ring */
 	while (rte_ring_dequeue(h->free_slots, &ptr) == 0)
-		rte_pause();
+		continue;
 
 	/* clear free extendable bucket ring and memory */
 	if (h->ext_table_support) {
 		memset(h->buckets_ext, 0, h->num_buckets *
 						sizeof(struct rte_hash_bucket));
 		while (rte_ring_dequeue(h->free_ext_bkts, &ptr) == 0)
-			rte_pause();
+			continue;
 	}
 
 	/* Repopulate the free slots ring. Entry zero is reserved for key misses */
-- 
2.7.4

[dpdk-dev] [PATCH v1 2/3] test/hash: change multiwriter test to use jhash

[Yipeng Wang]

With sequential key, the test will cover more corner
cases with jhash instead of crc hash, since jhash
generates more random hash pattern on sequential key.
It is useful for functional verification.

Signed-off-by: Yipeng Wang <yipeng1.wang@intel.com>
---
 test/test/test_hash_multiwriter.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/test/test/test_hash_multiwriter.c b/test/test/test_hash_multiwriter.c
index 6a3eb10..7f2deae 100644
--- a/test/test/test_hash_multiwriter.c
+++ b/test/test/test_hash_multiwriter.c
@@ -12,6 +12,7 @@
 #include <rte_malloc.h>
 #include <rte_random.h>
 #include <rte_spinlock.h>
+#include <rte_jhash.h>
 
 #include "test.h"
 
@@ -95,7 +96,7 @@ test_hash_multiwriter_worker(void *arg)
 
 
 static int
-test_hash_multiwriter(void)
+test_hash_multiwriter(int use_jhash)
 {
 	unsigned int i, rounded_nb_total_tsx_insertion;
 	static unsigned calledCount = 1;
@@ -108,10 +109,14 @@ test_hash_multiwriter(void)
 	struct rte_hash_parameters hash_params = {
 		.entries = nb_entries,
 		.key_len = sizeof(uint32_t),
-		.hash_func = rte_hash_crc,
 		.hash_func_init_val = 0,
 		.socket_id = rte_socket_id(),
 	};
+	if (use_jhash)
+		hash_params.hash_func = rte_jhash;
+	else
+		hash_params.hash_func = rte_hash_crc;
+
 	if (use_htm)
 		hash_params.extra_flag =
 			RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT
@@ -259,6 +264,8 @@ test_hash_multiwriter(void)
 static int
 test_hash_multiwriter_main(void)
 {
+	int use_jhash = 1;
+
 	if (rte_lcore_count() == 1) {
 		printf("More than one lcore is required to do multiwriter test\n");
 		return 0;
@@ -278,13 +285,13 @@ test_hash_multiwriter_main(void)
 		printf("Test multi-writer with Hardware transactional memory\n");
 
 		use_htm = 1;
-		if (test_hash_multiwriter() < 0)
+		if (test_hash_multiwriter(use_jhash) < 0)
 			return -1;
 	}
 
 	printf("Test multi-writer without Hardware transactional memory\n");
 	use_htm = 0;
-	if (test_hash_multiwriter() < 0)
+	if (test_hash_multiwriter(use_jhash) < 0)
 		return -1;
 
 	return 0;
-- 
2.7.4

Re: [dpdk-dev] [PATCH v1 2/3] test/hash: change multiwriter test to use jhash

[Bruce Richardson]

On Wed, Oct 10, 2018 at 02:48:04PM -0700, Yipeng Wang wrote:
> With sequential key, the test will cover more corner
> cases with jhash instead of crc hash, since jhash
> generates more random hash pattern on sequential key.
> It is useful for functional verification.
> 
> Signed-off-by: Yipeng Wang <yipeng1.wang@intel.com>
> ---
>  test/test/test_hash_multiwriter.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
Looking at the code in the patch, it appears the function is always now
called with use_jhash == 1. Why not just do a one-line change to switch the
hard-coded crc hash to jhash, rather than hardcoding that later in the file
and having a branch with a never-taken leg?

/Bruce

[dpdk-dev] [PATCH v1 3/3] test/hash: add readwrite test for ext table

[Yipeng Wang]

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

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

Re: [dpdk-dev] [PATCH v1 3/3] test/hash: add readwrite test for ext table

[Bruce Richardson]

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

Re: [dpdk-dev] [PATCH v1 3/3] test/hash: add readwrite test for ext table

[Wang, Yipeng1]

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

Re: [dpdk-dev] [PATCH v1 3/3] test/hash: add readwrite test for ext table

[Honnappa Nagarahalli]

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

Re: [dpdk-dev] [PATCH v1 3/3] test/hash: add readwrite test for ext table

[Bruce Richardson]

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

Re: [dpdk-dev] [PATCH v1 3/3] test/hash: add readwrite test for ext table

[Honnappa Nagarahalli]

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

Re: [dpdk-dev] [PATCH v1 3/3] test/hash: add readwrite test for ext table

[Honnappa Nagarahalli]

> 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

[dpdk-dev] [PATCH 0/3] hash: deprecate lock ellision and read/write concurreny flags

[Honnappa Nagarahalli]

Various configuration flags in rte_hash library result in increase of
number of test cases. Configuration flags for enabling transactional
memory use and read/write concurrency are not required. These features
should be supported by default. Please refer to [1] for more context.

This patch marks these flags for deprecation in 19.02 release and cleans
up the test cases.

[1] http://mails.dpdk.org/archives/dev/2018-October/117268.html

Honnappa Nagarahalli (3):
  hash: deprecate lock ellision and read/write concurreny flags
  test/hash: stop using lock ellision and read/write concurreny flags
  doc/hash: deprecate lock ellision and read/write concurreny flags

 doc/guides/rel_notes/deprecation.rst |   5 +
 lib/librte_hash/rte_cuckoo_hash.c    | 319 ++++++++++++++++++++++++++-
 lib/librte_hash/rte_hash.h           |  20 +-
 lib/librte_hash/rte_hash_version.map |   7 +
 test/test/test_hash_multiwriter.c    |  20 +-
 test/test/test_hash_perf.c           |  40 ++--
 test/test/test_hash_readwrite.c      |  84 ++-----
 test/test/test_hash_readwrite_lf.c   | 189 ++++------------
 8 files changed, 430 insertions(+), 254 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH 1/3] hash: deprecate lock ellision and read/write concurreny flags

[Honnappa Nagarahalli]

Hash library should provide read/write concurrency by default
as most of the use cases require read/write concurrency. Hence
the flag RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY is deprecated.
The library will decide if locking is required to provide
the concurrency based on other configuration flags.

If a lock is used to provide the read/write concurrency, best
possible locking mechanism available on the platform should
be used. Hence, the flag RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT
is deprecated. The library will use transactional memory
if the platform supports it.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
---
 lib/librte_hash/rte_cuckoo_hash.c    | 319 ++++++++++++++++++++++++++-
 lib/librte_hash/rte_hash.h           |  22 +-
 lib/librte_hash/rte_hash_version.map |   7 +
 3 files changed, 345 insertions(+), 3 deletions(-)

diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index 5ddcccd87..a11de22be 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -121,7 +121,7 @@ get_alt_bucket_index(const struct rte_hash *h,
 }
 
 struct rte_hash *
-rte_hash_create(const struct rte_hash_parameters *params)
+rte_hash_create_v20(const struct rte_hash_parameters *params)
 {
 	struct rte_hash *h = NULL;
 	struct rte_tailq_entry *te = NULL;
@@ -446,6 +446,323 @@ rte_hash_create(const struct rte_hash_parameters *params)
 	rte_free(tbl_chng_cnt);
 	return NULL;
 }
+VERSION_SYMBOL(rte_hash_create, _v20, 2.0);
+
+struct rte_hash *
+rte_hash_create_v1811(const struct rte_hash_parameters *params)
+{
+	struct rte_hash *h = NULL;
+	struct rte_tailq_entry *te = NULL;
+	struct rte_hash_list *hash_list;
+	struct rte_ring *r = NULL;
+	struct rte_ring *r_ext = NULL;
+	char hash_name[RTE_HASH_NAMESIZE];
+	void *k = NULL;
+	void *buckets = NULL;
+	void *buckets_ext = NULL;
+	char ring_name[RTE_RING_NAMESIZE];
+	char ext_ring_name[RTE_RING_NAMESIZE];
+	unsigned num_key_slots;
+	unsigned i;
+	unsigned int use_local_cache = 0;
+	unsigned int ext_table_support = 0;
+	unsigned int readwrite_concur_support = 1;
+	unsigned int writer_takes_lock = 1;
+	unsigned int no_free_on_del = 0;
+	uint32_t *tbl_chng_cnt = NULL;
+	unsigned int readwrite_concur_lf_support = 0;
+
+	rte_hash_function default_hash_func = (rte_hash_function)rte_jhash;
+
+	hash_list = RTE_TAILQ_CAST(rte_hash_tailq.head, rte_hash_list);
+
+	if (params == NULL) {
+		RTE_LOG(ERR, HASH, "rte_hash_create has no parameters\n");
+		return NULL;
+	}
+
+	/* Check for valid parameters */
+	if ((params->entries > RTE_HASH_ENTRIES_MAX) ||
+			(params->entries < RTE_HASH_BUCKET_ENTRIES) ||
+			(params->key_len == 0)) {
+		rte_errno = EINVAL;
+		RTE_LOG(ERR, HASH, "rte_hash_create has invalid parameters\n");
+		return NULL;
+	}
+
+	/* Validate correct usage of extra options */
+	if ((params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF) &&
+	    (params->extra_flag & RTE_HASH_EXTRA_FLAGS_EXT_TABLE)) {
+		rte_errno = EINVAL;
+		RTE_LOG(ERR, HASH, "rte_hash_create: extendable bucket "
+			"feature not supported with rw concurrency "
+			"lock free\n");
+		return NULL;
+	}
+
+	/* Check extra flags field to check extra options. */
+	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD)
+		use_local_cache = 1;
+
+	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_EXT_TABLE)
+		ext_table_support = 1;
+
+	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_NO_FREE_ON_DEL)
+		no_free_on_del = 1;
+
+	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF) {
+		/* Do not use lock for RW concurrency */
+		readwrite_concur_support = 0;
+		readwrite_concur_lf_support = 1;
+		/* Enable not freeing internal memory/index on delete */
+		no_free_on_del = 1;
+	}
+
+	if ((params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF) &&
+	    !(params->extra_flag & RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD))
+		writer_takes_lock = 0;
+
+	/* Store all keys and leave the first entry as a dummy entry for lookup_bulk */
+	if (use_local_cache)
+		/*
+		 * Increase number of slots by total number of indices
+		 * that can be stored in the lcore caches
+		 * except for the first cache
+		 */
+		num_key_slots = params->entries + (RTE_MAX_LCORE - 1) *
+					(LCORE_CACHE_SIZE - 1) + 1;
+	else
+		num_key_slots = params->entries + 1;
+
+	snprintf(ring_name, sizeof(ring_name), "HT_%s", params->name);
+	/* Create ring (Dummy slot index is not enqueued) */
+	r = rte_ring_create(ring_name, rte_align32pow2(num_key_slots),
+			params->socket_id, 0);
+	if (r == NULL) {
+		RTE_LOG(ERR, HASH, "memory allocation failed\n");
+		goto err;
+	}
+
+	const uint32_t num_buckets = rte_align32pow2(params->entries) /
+						RTE_HASH_BUCKET_ENTRIES;
+
+	/* Create ring for extendable buckets. */
+	if (ext_table_support) {
+		snprintf(ext_ring_name, sizeof(ext_ring_name), "HT_EXT_%s",
+								params->name);
+		r_ext = rte_ring_create(ext_ring_name,
+				rte_align32pow2(num_buckets + 1),
+				params->socket_id, 0);
+
+		if (r_ext == NULL) {
+			RTE_LOG(ERR, HASH, "ext buckets memory allocation "
+								"failed\n");
+			goto err;
+		}
+	}
+
+	snprintf(hash_name, sizeof(hash_name), "HT_%s", params->name);
+
+	rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
+
+	/* guarantee there's no existing: this is normally already checked
+	 * by ring creation above */
+	TAILQ_FOREACH(te, hash_list, next) {
+		h = (struct rte_hash *) te->data;
+		if (strncmp(params->name, h->name, RTE_HASH_NAMESIZE) == 0)
+			break;
+	}
+	h = NULL;
+	if (te != NULL) {
+		rte_errno = EEXIST;
+		te = NULL;
+		goto err_unlock;
+	}
+
+	te = rte_zmalloc("HASH_TAILQ_ENTRY", sizeof(*te), 0);
+	if (te == NULL) {
+		RTE_LOG(ERR, HASH, "tailq entry allocation failed\n");
+		goto err_unlock;
+	}
+
+	h = (struct rte_hash *)rte_zmalloc_socket(hash_name, sizeof(struct rte_hash),
+					RTE_CACHE_LINE_SIZE, params->socket_id);
+
+	if (h == NULL) {
+		RTE_LOG(ERR, HASH, "memory allocation failed\n");
+		goto err_unlock;
+	}
+
+	buckets = rte_zmalloc_socket(NULL,
+				num_buckets * sizeof(struct rte_hash_bucket),
+				RTE_CACHE_LINE_SIZE, params->socket_id);
+
+	if (buckets == NULL) {
+		RTE_LOG(ERR, HASH, "buckets memory allocation failed\n");
+		goto err_unlock;
+	}
+
+	/* Allocate same number of extendable buckets */
+	if (ext_table_support) {
+		buckets_ext = rte_zmalloc_socket(NULL,
+				num_buckets * sizeof(struct rte_hash_bucket),
+				RTE_CACHE_LINE_SIZE, params->socket_id);
+		if (buckets_ext == NULL) {
+			RTE_LOG(ERR, HASH, "ext buckets memory allocation "
+							"failed\n");
+			goto err_unlock;
+		}
+		/* Populate ext bkt ring. We reserve 0 similar to the
+		 * key-data slot, just in case in future we want to
+		 * use bucket index for the linked list and 0 means NULL
+		 * for next bucket
+		 */
+		for (i = 1; i <= num_buckets; i++)
+			rte_ring_sp_enqueue(r_ext, (void *)((uintptr_t) i));
+	}
+
+	const uint32_t key_entry_size =
+		RTE_ALIGN(sizeof(struct rte_hash_key) + params->key_len,
+			  KEY_ALIGNMENT);
+	const uint64_t key_tbl_size = (uint64_t) key_entry_size * num_key_slots;
+
+	k = rte_zmalloc_socket(NULL, key_tbl_size,
+			RTE_CACHE_LINE_SIZE, params->socket_id);
+
+	if (k == NULL) {
+		RTE_LOG(ERR, HASH, "memory allocation failed\n");
+		goto err_unlock;
+	}
+
+	tbl_chng_cnt = rte_zmalloc_socket(NULL, sizeof(uint32_t),
+			RTE_CACHE_LINE_SIZE, params->socket_id);
+
+	if (tbl_chng_cnt == NULL) {
+		RTE_LOG(ERR, HASH, "memory allocation failed\n");
+		goto err_unlock;
+	}
+
+/*
+ * If x86 architecture is used, select appropriate compare function,
+ * which may use x86 intrinsics, otherwise use memcmp
+ */
+#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
+	/* Select function to compare keys */
+	switch (params->key_len) {
+	case 16:
+		h->cmp_jump_table_idx = KEY_16_BYTES;
+		break;
+	case 32:
+		h->cmp_jump_table_idx = KEY_32_BYTES;
+		break;
+	case 48:
+		h->cmp_jump_table_idx = KEY_48_BYTES;
+		break;
+	case 64:
+		h->cmp_jump_table_idx = KEY_64_BYTES;
+		break;
+	case 80:
+		h->cmp_jump_table_idx = KEY_80_BYTES;
+		break;
+	case 96:
+		h->cmp_jump_table_idx = KEY_96_BYTES;
+		break;
+	case 112:
+		h->cmp_jump_table_idx = KEY_112_BYTES;
+		break;
+	case 128:
+		h->cmp_jump_table_idx = KEY_128_BYTES;
+		break;
+	default:
+		/* If key is not multiple of 16, use generic memcmp */
+		h->cmp_jump_table_idx = KEY_OTHER_BYTES;
+	}
+#else
+	h->cmp_jump_table_idx = KEY_OTHER_BYTES;
+#endif
+
+	if (use_local_cache) {
+		h->local_free_slots = rte_zmalloc_socket(NULL,
+				sizeof(struct lcore_cache) * RTE_MAX_LCORE,
+				RTE_CACHE_LINE_SIZE, params->socket_id);
+	}
+
+	/* Default hash function */
+#if defined(RTE_ARCH_X86)
+	default_hash_func = (rte_hash_function)rte_hash_crc;
+#elif defined(RTE_ARCH_ARM64)
+	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_CRC32))
+		default_hash_func = (rte_hash_function)rte_hash_crc;
+#endif
+	/* Setup hash context */
+	snprintf(h->name, sizeof(h->name), "%s", params->name);
+	h->entries = params->entries;
+	h->key_len = params->key_len;
+	h->key_entry_size = key_entry_size;
+	h->hash_func_init_val = params->hash_func_init_val;
+
+	h->num_buckets = num_buckets;
+	h->bucket_bitmask = h->num_buckets - 1;
+	h->buckets = buckets;
+	h->buckets_ext = buckets_ext;
+	h->free_ext_bkts = r_ext;
+	h->hash_func = (params->hash_func == NULL) ?
+		default_hash_func : params->hash_func;
+	h->key_store = k;
+	h->free_slots = r;
+	h->tbl_chng_cnt = tbl_chng_cnt;
+	*h->tbl_chng_cnt = 0;
+	h->hw_trans_mem_support = rte_tm_supported();
+	h->use_local_cache = use_local_cache;
+	h->readwrite_concur_support = readwrite_concur_support;
+	h->ext_table_support = ext_table_support;
+	h->writer_takes_lock = writer_takes_lock;
+	h->no_free_on_del = no_free_on_del;
+	h->readwrite_concur_lf_support = readwrite_concur_lf_support;
+
+#if defined(RTE_ARCH_X86)
+	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE2))
+		h->sig_cmp_fn = RTE_HASH_COMPARE_SSE;
+	else
+#endif
+		h->sig_cmp_fn = RTE_HASH_COMPARE_SCALAR;
+
+	if (h->writer_takes_lock) {
+		h->readwrite_lock = rte_malloc(NULL, sizeof(rte_rwlock_t),
+						RTE_CACHE_LINE_SIZE);
+		if (h->readwrite_lock == NULL)
+			goto err_unlock;
+
+		rte_rwlock_init(h->readwrite_lock);
+	}
+
+	/* Populate free slots ring. Entry zero is reserved for key misses. */
+	for (i = 1; i < num_key_slots; i++)
+		rte_ring_sp_enqueue(r, (void *)((uintptr_t) i));
+
+	te->data = (void *) h;
+	TAILQ_INSERT_TAIL(hash_list, te, next);
+	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
+
+	return h;
+err_unlock:
+	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
+err:
+	rte_ring_free(r);
+	rte_ring_free(r_ext);
+	rte_free(te);
+	rte_free(h);
+	rte_free(buckets);
+	rte_free(buckets_ext);
+	rte_free(k);
+	rte_free(tbl_chng_cnt);
+	return NULL;
+}
+BIND_DEFAULT_SYMBOL(rte_hash_create, _v1811, 18.11);
+MAP_STATIC_SYMBOL(
+	struct rte_hash *rte_hash_create(
+		const struct rte_hash_parameters *params),
+		rte_hash_create_v1811);
 
 void
 rte_hash_free(struct rte_hash *h)
diff --git a/lib/librte_hash/rte_hash.h b/lib/librte_hash/rte_hash.h
index c93d1a137..93c7019ec 100644
--- a/lib/librte_hash/rte_hash.h
+++ b/lib/librte_hash/rte_hash.h
@@ -30,13 +30,27 @@ extern "C" {
 #define RTE_HASH_LOOKUP_BULK_MAX		64
 #define RTE_HASH_LOOKUP_MULTI_MAX		RTE_HASH_LOOKUP_BULK_MAX
 
-/** Enable Hardware transactional memory support. */
+/**
+ * @deprecated
+ * This define will be removed in the next release.
+ * If the target platform supports hardware transactional memory
+ * it will be used without user consent as it provides the best possible
+ * performance.
+ *
+ * Enable Hardware transactional memory support.
+ */
 #define RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT	0x01
 
 /** Default behavior of insertion, single writer/multi writer */
 #define RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD 0x02
 
-/** Flag to support reader writer concurrency */
+/**
+ * @deprecated
+ * This define will be removed in the next release.
+ * This library should be thread-safe by default.
+ *
+ * Flag to support reader writer concurrency
+ */
 #define RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY 0x04
 
 /** Flag to indicate the extendabe bucket table feature should be used */
@@ -105,6 +119,10 @@ struct rte_hash;
  */
 struct rte_hash *
 rte_hash_create(const struct rte_hash_parameters *params);
+struct rte_hash *
+rte_hash_create_v20(const struct rte_hash_parameters *params);
+struct rte_hash *
+rte_hash_create_v1811(const struct rte_hash_parameters *params);
 
 /**
  * Set a new hash compare function other than the default one.
diff --git a/lib/librte_hash/rte_hash_version.map b/lib/librte_hash/rte_hash_version.map
index 734ae28b0..c72469099 100644
--- a/lib/librte_hash/rte_hash_version.map
+++ b/lib/librte_hash/rte_hash_version.map
@@ -54,6 +54,13 @@ DPDK_18.08 {
 
 } DPDK_16.07;
 
+DPDK_18.11 {
+	global:
+
+	rte_hash_create;
+
+} DPDK_18.08;
+
 EXPERIMENTAL {
 	global:
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH 1/3] hash: deprecate lock ellision and read/write concurreny flags

[Bruce Richardson]

On Wed, Oct 31, 2018 at 11:54:52PM -0500, Honnappa Nagarahalli wrote:
> Hash library should provide read/write concurrency by default
> as most of the use cases require read/write concurrency. Hence
> the flag RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY is deprecated.
> The library will decide if locking is required to provide
> the concurrency based on other configuration flags.
> 
> If a lock is used to provide the read/write concurrency, best
> possible locking mechanism available on the platform should
> be used. Hence, the flag RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT
> is deprecated. The library will use transactional memory
> if the platform supports it.
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> ---
>  lib/librte_hash/rte_cuckoo_hash.c    | 319 ++++++++++++++++++++++++++-
>  lib/librte_hash/rte_hash.h           |  22 +-
>  lib/librte_hash/rte_hash_version.map |   7 +
>  3 files changed, 345 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
> index 5ddcccd87..a11de22be 100644
> --- a/lib/librte_hash/rte_cuckoo_hash.c
> +++ b/lib/librte_hash/rte_cuckoo_hash.c
> @@ -121,7 +121,7 @@ get_alt_bucket_index(const struct rte_hash *h,
>  }
>  
>  struct rte_hash *
> -rte_hash_create(const struct rte_hash_parameters *params)
> +rte_hash_create_v20(const struct rte_hash_parameters *params)
>  {
>  	struct rte_hash *h = NULL;
>  	struct rte_tailq_entry *te = NULL;
> @@ -446,6 +446,323 @@ rte_hash_create(const struct rte_hash_parameters *params)
>  	rte_free(tbl_chng_cnt);
>  	return NULL;
>  }
> +VERSION_SYMBOL(rte_hash_create, _v20, 2.0);
> +

To make reviewing this easier, can I ask if in V2 you can put the v18.11
function first, before the existing one. Hopefully that means that the "new"
function from git's point of view is the existing one, showing it as a
block add that we can pretty much skip reviewing. The benefit of this is that
the changes in the v1811 should then show as line-by-line diffs in the
patch, so we can easily review the changes made in the new case. It's hard
to spot them in the whole function below.

> +struct rte_hash *
> +rte_hash_create_v1811(const struct rte_hash_parameters *params)
> +{
> +	struct rte_hash *h = NULL;
> +	struct rte_tailq_entry *te = NULL;
> +	struct rte_hash_list *hash_list;
> +	struct rte_ring *r = NULL;
> +	struct rte_ring *r_ext = NULL;
> +	char hash_name[RTE_HASH_NAMESIZE];
> +	void *k = NULL;
> +	void *buckets = NULL;
> +	void *buckets_ext = NULL;
> +	char ring_name[RTE_RING_NAMESIZE];
> +	char ext_ring_name[RTE_RING_NAMESIZE];
> +	unsigned num_key_slots;
> +	unsigned i;
> +	unsigned int use_local_cache = 0;
> +	unsigned int ext_table_support = 0;
> +	unsigned int readwrite_concur_support = 1;
> +	unsigned int writer_takes_lock = 1;
> +	unsigned int no_free_on_del = 0;
> +	uint32_t *tbl_chng_cnt = NULL;
> +	unsigned int readwrite_concur_lf_support = 0;
> +
> +	rte_hash_function default_hash_func = (rte_hash_function)rte_jhash;
> +
> +	hash_list = RTE_TAILQ_CAST(rte_hash_tailq.head, rte_hash_list);
> +
> +	if (params == NULL) {
> +		RTE_LOG(ERR, HASH, "rte_hash_create has no parameters\n");
> +		return NULL;
> +	}
> +
> +	/* Check for valid parameters */
> +	if ((params->entries > RTE_HASH_ENTRIES_MAX) ||
> +			(params->entries < RTE_HASH_BUCKET_ENTRIES) ||
> +			(params->key_len == 0)) {
> +		rte_errno = EINVAL;
> +		RTE_LOG(ERR, HASH, "rte_hash_create has invalid parameters\n");
> +		return NULL;
> +	}
> +
> +	/* Validate correct usage of extra options */
> +	if ((params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF) &&
> +	    (params->extra_flag & RTE_HASH_EXTRA_FLAGS_EXT_TABLE)) {
> +		rte_errno = EINVAL;
> +		RTE_LOG(ERR, HASH, "rte_hash_create: extendable bucket "
> +			"feature not supported with rw concurrency "
> +			"lock free\n");

Please don't split the error text across lines. If you put it on a line by
itself, hopefully checkpatch should not complain. If checkpatch does
complain about the long line, just ignore it!
[Yes, I know this is copied from original code, but since it appears as new
code in this patch, we should fix it]

> +		return NULL;
> +	}
> +
> +	/* Check extra flags field to check extra options. */
> +	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD)
> +		use_local_cache = 1;
> +
> +	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_EXT_TABLE)
> +		ext_table_support = 1;
> +
> +	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_NO_FREE_ON_DEL)
> +		no_free_on_del = 1;
> +
> +	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF) {
> +		/* Do not use lock for RW concurrency */
> +		readwrite_concur_support = 0;
> +		readwrite_concur_lf_support = 1;
> +		/* Enable not freeing internal memory/index on delete */
> +		no_free_on_del = 1;
> +	}
> +
> +	if ((params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF) &&
> +	    !(params->extra_flag & RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD))
> +		writer_takes_lock = 0;
> +
> +	/* Store all keys and leave the first entry as a dummy entry for lookup_bulk */
> +	if (use_local_cache)
> +		/*
> +		 * Increase number of slots by total number of indices
> +		 * that can be stored in the lcore caches
> +		 * except for the first cache
> +		 */
> +		num_key_slots = params->entries + (RTE_MAX_LCORE - 1) *
> +					(LCORE_CACHE_SIZE - 1) + 1;
> +	else
> +		num_key_slots = params->entries + 1;
> +
> +	snprintf(ring_name, sizeof(ring_name), "HT_%s", params->name);
> +	/* Create ring (Dummy slot index is not enqueued) */
> +	r = rte_ring_create(ring_name, rte_align32pow2(num_key_slots),
> +			params->socket_id, 0);
> +	if (r == NULL) {
> +		RTE_LOG(ERR, HASH, "memory allocation failed\n");
> +		goto err;
> +	}
> +
> +	const uint32_t num_buckets = rte_align32pow2(params->entries) /
> +						RTE_HASH_BUCKET_ENTRIES;
> +
> +	/* Create ring for extendable buckets. */
> +	if (ext_table_support) {
> +		snprintf(ext_ring_name, sizeof(ext_ring_name), "HT_EXT_%s",
> +								params->name);
> +		r_ext = rte_ring_create(ext_ring_name,
> +				rte_align32pow2(num_buckets + 1),
> +				params->socket_id, 0);
> +
> +		if (r_ext == NULL) {
> +			RTE_LOG(ERR, HASH, "ext buckets memory allocation "
> +								"failed\n");
> +			goto err;
> +		}
> +	}
> +
> +	snprintf(hash_name, sizeof(hash_name), "HT_%s", params->name);
> +
> +	rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
> +
> +	/* guarantee there's no existing: this is normally already checked
> +	 * by ring creation above */
> +	TAILQ_FOREACH(te, hash_list, next) {
> +		h = (struct rte_hash *) te->data;
> +		if (strncmp(params->name, h->name, RTE_HASH_NAMESIZE) == 0)
> +			break;
> +	}
> +	h = NULL;
> +	if (te != NULL) {
> +		rte_errno = EEXIST;
> +		te = NULL;
> +		goto err_unlock;
> +	}
> +
> +	te = rte_zmalloc("HASH_TAILQ_ENTRY", sizeof(*te), 0);
> +	if (te == NULL) {
> +		RTE_LOG(ERR, HASH, "tailq entry allocation failed\n");
> +		goto err_unlock;
> +	}
> +
> +	h = (struct rte_hash *)rte_zmalloc_socket(hash_name, sizeof(struct rte_hash),
> +					RTE_CACHE_LINE_SIZE, params->socket_id);
> +
> +	if (h == NULL) {
> +		RTE_LOG(ERR, HASH, "memory allocation failed\n");
> +		goto err_unlock;
> +	}
> +
> +	buckets = rte_zmalloc_socket(NULL,
> +				num_buckets * sizeof(struct rte_hash_bucket),
> +				RTE_CACHE_LINE_SIZE, params->socket_id);
> +
> +	if (buckets == NULL) {
> +		RTE_LOG(ERR, HASH, "buckets memory allocation failed\n");
> +		goto err_unlock;
> +	}
> +
> +	/* Allocate same number of extendable buckets */
> +	if (ext_table_support) {
> +		buckets_ext = rte_zmalloc_socket(NULL,
> +				num_buckets * sizeof(struct rte_hash_bucket),
> +				RTE_CACHE_LINE_SIZE, params->socket_id);
> +		if (buckets_ext == NULL) {
> +			RTE_LOG(ERR, HASH, "ext buckets memory allocation "
> +							"failed\n");
> +			goto err_unlock;
> +		}
> +		/* Populate ext bkt ring. We reserve 0 similar to the
> +		 * key-data slot, just in case in future we want to
> +		 * use bucket index for the linked list and 0 means NULL
> +		 * for next bucket
> +		 */
> +		for (i = 1; i <= num_buckets; i++)
> +			rte_ring_sp_enqueue(r_ext, (void *)((uintptr_t) i));
> +	}
> +
> +	const uint32_t key_entry_size =
> +		RTE_ALIGN(sizeof(struct rte_hash_key) + params->key_len,
> +			  KEY_ALIGNMENT);
> +	const uint64_t key_tbl_size = (uint64_t) key_entry_size * num_key_slots;
> +
> +	k = rte_zmalloc_socket(NULL, key_tbl_size,
> +			RTE_CACHE_LINE_SIZE, params->socket_id);
> +
> +	if (k == NULL) {
> +		RTE_LOG(ERR, HASH, "memory allocation failed\n");
> +		goto err_unlock;
> +	}
> +
> +	tbl_chng_cnt = rte_zmalloc_socket(NULL, sizeof(uint32_t),
> +			RTE_CACHE_LINE_SIZE, params->socket_id);
> +
> +	if (tbl_chng_cnt == NULL) {
> +		RTE_LOG(ERR, HASH, "memory allocation failed\n");
> +		goto err_unlock;
> +	}
> +
> +/*
> + * If x86 architecture is used, select appropriate compare function,
> + * which may use x86 intrinsics, otherwise use memcmp
> + */
> +#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
> +	/* Select function to compare keys */
> +	switch (params->key_len) {
> +	case 16:
> +		h->cmp_jump_table_idx = KEY_16_BYTES;
> +		break;
> +	case 32:
> +		h->cmp_jump_table_idx = KEY_32_BYTES;
> +		break;
> +	case 48:
> +		h->cmp_jump_table_idx = KEY_48_BYTES;
> +		break;
> +	case 64:
> +		h->cmp_jump_table_idx = KEY_64_BYTES;
> +		break;
> +	case 80:
> +		h->cmp_jump_table_idx = KEY_80_BYTES;
> +		break;
> +	case 96:
> +		h->cmp_jump_table_idx = KEY_96_BYTES;
> +		break;
> +	case 112:
> +		h->cmp_jump_table_idx = KEY_112_BYTES;
> +		break;
> +	case 128:
> +		h->cmp_jump_table_idx = KEY_128_BYTES;
> +		break;
> +	default:
> +		/* If key is not multiple of 16, use generic memcmp */
> +		h->cmp_jump_table_idx = KEY_OTHER_BYTES;
> +	}
> +#else
> +	h->cmp_jump_table_idx = KEY_OTHER_BYTES;
> +#endif
> +
> +	if (use_local_cache) {
> +		h->local_free_slots = rte_zmalloc_socket(NULL,
> +				sizeof(struct lcore_cache) * RTE_MAX_LCORE,
> +				RTE_CACHE_LINE_SIZE, params->socket_id);
> +	}
> +
> +	/* Default hash function */
> +#if defined(RTE_ARCH_X86)
> +	default_hash_func = (rte_hash_function)rte_hash_crc;
> +#elif defined(RTE_ARCH_ARM64)
> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_CRC32))
> +		default_hash_func = (rte_hash_function)rte_hash_crc;
> +#endif
> +	/* Setup hash context */
> +	snprintf(h->name, sizeof(h->name), "%s", params->name);
> +	h->entries = params->entries;
> +	h->key_len = params->key_len;
> +	h->key_entry_size = key_entry_size;
> +	h->hash_func_init_val = params->hash_func_init_val;
> +
> +	h->num_buckets = num_buckets;
> +	h->bucket_bitmask = h->num_buckets - 1;
> +	h->buckets = buckets;
> +	h->buckets_ext = buckets_ext;
> +	h->free_ext_bkts = r_ext;
> +	h->hash_func = (params->hash_func == NULL) ?
> +		default_hash_func : params->hash_func;
> +	h->key_store = k;
> +	h->free_slots = r;
> +	h->tbl_chng_cnt = tbl_chng_cnt;
> +	*h->tbl_chng_cnt = 0;
> +	h->hw_trans_mem_support = rte_tm_supported();
> +	h->use_local_cache = use_local_cache;
> +	h->readwrite_concur_support = readwrite_concur_support;
> +	h->ext_table_support = ext_table_support;
> +	h->writer_takes_lock = writer_takes_lock;
> +	h->no_free_on_del = no_free_on_del;
> +	h->readwrite_concur_lf_support = readwrite_concur_lf_support;
> +
> +#if defined(RTE_ARCH_X86)
> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE2))
> +		h->sig_cmp_fn = RTE_HASH_COMPARE_SSE;
> +	else
> +#endif
> +		h->sig_cmp_fn = RTE_HASH_COMPARE_SCALAR;
> +
> +	if (h->writer_takes_lock) {
> +		h->readwrite_lock = rte_malloc(NULL, sizeof(rte_rwlock_t),
> +						RTE_CACHE_LINE_SIZE);
> +		if (h->readwrite_lock == NULL)
> +			goto err_unlock;
> +
> +		rte_rwlock_init(h->readwrite_lock);
> +	}
> +
> +	/* Populate free slots ring. Entry zero is reserved for key misses. */
> +	for (i = 1; i < num_key_slots; i++)
> +		rte_ring_sp_enqueue(r, (void *)((uintptr_t) i));
> +
> +	te->data = (void *) h;
> +	TAILQ_INSERT_TAIL(hash_list, te, next);
> +	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
> +
> +	return h;
> +err_unlock:
> +	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
> +err:
> +	rte_ring_free(r);
> +	rte_ring_free(r_ext);
> +	rte_free(te);
> +	rte_free(h);
> +	rte_free(buckets);
> +	rte_free(buckets_ext);
> +	rte_free(k);
> +	rte_free(tbl_chng_cnt);
> +	return NULL;
> +}
> +BIND_DEFAULT_SYMBOL(rte_hash_create, _v1811, 18.11);
> +MAP_STATIC_SYMBOL(
> +	struct rte_hash *rte_hash_create(
> +		const struct rte_hash_parameters *params),
> +		rte_hash_create_v1811);
>  
>  void
>  rte_hash_free(struct rte_hash *h)
> diff --git a/lib/librte_hash/rte_hash.h b/lib/librte_hash/rte_hash.h
> index c93d1a137..93c7019ec 100644
> --- a/lib/librte_hash/rte_hash.h
> +++ b/lib/librte_hash/rte_hash.h
> @@ -30,13 +30,27 @@ extern "C" {
>  #define RTE_HASH_LOOKUP_BULK_MAX		64
>  #define RTE_HASH_LOOKUP_MULTI_MAX		RTE_HASH_LOOKUP_BULK_MAX
>  
> -/** Enable Hardware transactional memory support. */
> +/**
> + * @deprecated
> + * This define will be removed in the next release.
> + * If the target platform supports hardware transactional memory
> + * it will be used without user consent as it provides the best possible
> + * performance.
> + *
> + * Enable Hardware transactional memory support.
> + */
>  #define RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT	0x01
>  
>  /** Default behavior of insertion, single writer/multi writer */
>  #define RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD 0x02
>  
> -/** Flag to support reader writer concurrency */
> +/**
> + * @deprecated
> + * This define will be removed in the next release.
> + * This library should be thread-safe by default.
> + *
> + * Flag to support reader writer concurrency
> + */
>  #define RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY 0x04
>  

Do we not need/want to add some new flags to disable these features, in
case there are cases where either RW concurrency, or transaction memory is
unwanted?

>  /** Flag to indicate the extendabe bucket table feature should be used */
> @@ -105,6 +119,10 @@ struct rte_hash;
>   */
>  struct rte_hash *
>  rte_hash_create(const struct rte_hash_parameters *params);
> +struct rte_hash *
> +rte_hash_create_v20(const struct rte_hash_parameters *params);
> +struct rte_hash *
> +rte_hash_create_v1811(const struct rte_hash_parameters *params);
>  
>  /**
>   * Set a new hash compare function other than the default one.
> diff --git a/lib/librte_hash/rte_hash_version.map b/lib/librte_hash/rte_hash_version.map
> index 734ae28b0..c72469099 100644
> --- a/lib/librte_hash/rte_hash_version.map
> +++ b/lib/librte_hash/rte_hash_version.map
> @@ -54,6 +54,13 @@ DPDK_18.08 {
>  
>  } DPDK_16.07;
>  
> +DPDK_18.11 {
> +	global:
> +
> +	rte_hash_create;
> +
> +} DPDK_18.08;
> +
>  EXPERIMENTAL {
>  	global:
>  
> -- 
> 2.17.1
> 

Re: [dpdk-dev] [PATCH 1/3] hash: deprecate lock ellision and read/write concurreny flags

[Bruce Richardson]

On Thu, Nov 01, 2018 at 09:45:31AM +0000, Bruce Richardson wrote:
> On Wed, Oct 31, 2018 at 11:54:52PM -0500, Honnappa Nagarahalli wrote:
> > Hash library should provide read/write concurrency by default
> > as most of the use cases require read/write concurrency. Hence
> > the flag RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY is deprecated.
> > The library will decide if locking is required to provide
> > the concurrency based on other configuration flags.
> > 
> > If a lock is used to provide the read/write concurrency, best
> > possible locking mechanism available on the platform should
> > be used. Hence, the flag RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT
> > is deprecated. The library will use transactional memory
> > if the platform supports it.
> > 
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > ---
> >  lib/librte_hash/rte_cuckoo_hash.c    | 319 ++++++++++++++++++++++++++-
> >  lib/librte_hash/rte_hash.h           |  22 +-
> >  lib/librte_hash/rte_hash_version.map |   7 +
> >  3 files changed, 345 insertions(+), 3 deletions(-)
> > 
> > diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
> > index 5ddcccd87..a11de22be 100644
> > --- a/lib/librte_hash/rte_cuckoo_hash.c
> > +++ b/lib/librte_hash/rte_cuckoo_hash.c
> > @@ -121,7 +121,7 @@ get_alt_bucket_index(const struct rte_hash *h,
> >  }
> >  
> >  struct rte_hash *
> > -rte_hash_create(const struct rte_hash_parameters *params)
> > +rte_hash_create_v20(const struct rte_hash_parameters *params)
> >  {
> >  	struct rte_hash *h = NULL;
> >  	struct rte_tailq_entry *te = NULL;
> > @@ -446,6 +446,323 @@ rte_hash_create(const struct rte_hash_parameters *params)
> >  	rte_free(tbl_chng_cnt);
> >  	return NULL;
> >  }
> > +VERSION_SYMBOL(rte_hash_create, _v20, 2.0);
> > +
> 
> To make reviewing this easier, can I ask if in V2 you can put the v18.11
> function first, before the existing one. Hopefully that means that the "new"
> function from git's point of view is the existing one, showing it as a
> block add that we can pretty much skip reviewing. The benefit of this is that
> the changes in the v1811 should then show as line-by-line diffs in the
> patch, so we can easily review the changes made in the new case. It's hard
> to spot them in the whole function below.
> 
> > +struct rte_hash *
> > +rte_hash_create_v1811(const struct rte_hash_parameters *params)
> > +{
> > +	struct rte_hash *h = NULL;
> > +	struct rte_tailq_entry *te = NULL;
> > +	struct rte_hash_list *hash_list;
> > +	struct rte_ring *r = NULL;
> > +	struct rte_ring *r_ext = NULL;
> > +	char hash_name[RTE_HASH_NAMESIZE];
> > +	void *k = NULL;
> > +	void *buckets = NULL;
> > +	void *buckets_ext = NULL;
> > +	char ring_name[RTE_RING_NAMESIZE];
> > +	char ext_ring_name[RTE_RING_NAMESIZE];
> > +	unsigned num_key_slots;
> > +	unsigned i;
> > +	unsigned int use_local_cache = 0;
> > +	unsigned int ext_table_support = 0;
> > +	unsigned int readwrite_concur_support = 1;
> > +	unsigned int writer_takes_lock = 1;
> > +	unsigned int no_free_on_del = 0;
> > +	uint32_t *tbl_chng_cnt = NULL;
> > +	unsigned int readwrite_concur_lf_support = 0;
> > +
> > +	rte_hash_function default_hash_func = (rte_hash_function)rte_jhash;
> > +
> > +	hash_list = RTE_TAILQ_CAST(rte_hash_tailq.head, rte_hash_list);
> > +
> > +	if (params == NULL) {
> > +		RTE_LOG(ERR, HASH, "rte_hash_create has no parameters\n");
> > +		return NULL;
> > +	}
> > +
> > +	/* Check for valid parameters */
> > +	if ((params->entries > RTE_HASH_ENTRIES_MAX) ||
> > +			(params->entries < RTE_HASH_BUCKET_ENTRIES) ||
> > +			(params->key_len == 0)) {
> > +		rte_errno = EINVAL;
> > +		RTE_LOG(ERR, HASH, "rte_hash_create has invalid parameters\n");
> > +		return NULL;
> > +	}
> > +
> > +	/* Validate correct usage of extra options */
> > +	if ((params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF) &&
> > +	    (params->extra_flag & RTE_HASH_EXTRA_FLAGS_EXT_TABLE)) {
> > +		rte_errno = EINVAL;
> > +		RTE_LOG(ERR, HASH, "rte_hash_create: extendable bucket "
> > +			"feature not supported with rw concurrency "
> > +			"lock free\n");
> 
> Please don't split the error text across lines. If you put it on a line by
> itself, hopefully checkpatch should not complain. If checkpatch does
> complain about the long line, just ignore it!
> [Yes, I know this is copied from original code, but since it appears as new
> code in this patch, we should fix it]
> 
> > +		return NULL;
> > +	}
> > +
> > +	/* Check extra flags field to check extra options. */
> > +	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD)
> > +		use_local_cache = 1;
> > +
> > +	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_EXT_TABLE)
> > +		ext_table_support = 1;
> > +
> > +	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_NO_FREE_ON_DEL)
> > +		no_free_on_del = 1;
> > +
> > +	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF) {
> > +		/* Do not use lock for RW concurrency */
> > +		readwrite_concur_support = 0;
> > +		readwrite_concur_lf_support = 1;
> > +		/* Enable not freeing internal memory/index on delete */
> > +		no_free_on_del = 1;
> > +	}
> > +
> > +	if ((params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF) &&
> > +	    !(params->extra_flag & RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD))
> > +		writer_takes_lock = 0;
> > +
> > +	/* Store all keys and leave the first entry as a dummy entry for lookup_bulk */
> > +	if (use_local_cache)
> > +		/*
> > +		 * Increase number of slots by total number of indices
> > +		 * that can be stored in the lcore caches
> > +		 * except for the first cache
> > +		 */
> > +		num_key_slots = params->entries + (RTE_MAX_LCORE - 1) *
> > +					(LCORE_CACHE_SIZE - 1) + 1;
> > +	else
> > +		num_key_slots = params->entries + 1;
> > +
> > +	snprintf(ring_name, sizeof(ring_name), "HT_%s", params->name);
> > +	/* Create ring (Dummy slot index is not enqueued) */
> > +	r = rte_ring_create(ring_name, rte_align32pow2(num_key_slots),
> > +			params->socket_id, 0);
> > +	if (r == NULL) {
> > +		RTE_LOG(ERR, HASH, "memory allocation failed\n");
> > +		goto err;
> > +	}
> > +
> > +	const uint32_t num_buckets = rte_align32pow2(params->entries) /
> > +						RTE_HASH_BUCKET_ENTRIES;
> > +
> > +	/* Create ring for extendable buckets. */
> > +	if (ext_table_support) {
> > +		snprintf(ext_ring_name, sizeof(ext_ring_name), "HT_EXT_%s",
> > +								params->name);
> > +		r_ext = rte_ring_create(ext_ring_name,
> > +				rte_align32pow2(num_buckets + 1),
> > +				params->socket_id, 0);
> > +
> > +		if (r_ext == NULL) {
> > +			RTE_LOG(ERR, HASH, "ext buckets memory allocation "
> > +								"failed\n");
> > +			goto err;
> > +		}
> > +	}
> > +
> > +	snprintf(hash_name, sizeof(hash_name), "HT_%s", params->name);
> > +
> > +	rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
> > +
> > +	/* guarantee there's no existing: this is normally already checked
> > +	 * by ring creation above */
> > +	TAILQ_FOREACH(te, hash_list, next) {
> > +		h = (struct rte_hash *) te->data;
> > +		if (strncmp(params->name, h->name, RTE_HASH_NAMESIZE) == 0)
> > +			break;
> > +	}
> > +	h = NULL;
> > +	if (te != NULL) {
> > +		rte_errno = EEXIST;
> > +		te = NULL;
> > +		goto err_unlock;
> > +	}
> > +
> > +	te = rte_zmalloc("HASH_TAILQ_ENTRY", sizeof(*te), 0);
> > +	if (te == NULL) {
> > +		RTE_LOG(ERR, HASH, "tailq entry allocation failed\n");
> > +		goto err_unlock;
> > +	}
> > +
> > +	h = (struct rte_hash *)rte_zmalloc_socket(hash_name, sizeof(struct rte_hash),
> > +					RTE_CACHE_LINE_SIZE, params->socket_id);
> > +
> > +	if (h == NULL) {
> > +		RTE_LOG(ERR, HASH, "memory allocation failed\n");
> > +		goto err_unlock;
> > +	}
> > +
> > +	buckets = rte_zmalloc_socket(NULL,
> > +				num_buckets * sizeof(struct rte_hash_bucket),
> > +				RTE_CACHE_LINE_SIZE, params->socket_id);
> > +
> > +	if (buckets == NULL) {
> > +		RTE_LOG(ERR, HASH, "buckets memory allocation failed\n");
> > +		goto err_unlock;
> > +	}
> > +
> > +	/* Allocate same number of extendable buckets */
> > +	if (ext_table_support) {
> > +		buckets_ext = rte_zmalloc_socket(NULL,
> > +				num_buckets * sizeof(struct rte_hash_bucket),
> > +				RTE_CACHE_LINE_SIZE, params->socket_id);
> > +		if (buckets_ext == NULL) {
> > +			RTE_LOG(ERR, HASH, "ext buckets memory allocation "
> > +							"failed\n");
> > +			goto err_unlock;
> > +		}
> > +		/* Populate ext bkt ring. We reserve 0 similar to the
> > +		 * key-data slot, just in case in future we want to
> > +		 * use bucket index for the linked list and 0 means NULL
> > +		 * for next bucket
> > +		 */
> > +		for (i = 1; i <= num_buckets; i++)
> > +			rte_ring_sp_enqueue(r_ext, (void *)((uintptr_t) i));
> > +	}
> > +
> > +	const uint32_t key_entry_size =
> > +		RTE_ALIGN(sizeof(struct rte_hash_key) + params->key_len,
> > +			  KEY_ALIGNMENT);
> > +	const uint64_t key_tbl_size = (uint64_t) key_entry_size * num_key_slots;
> > +
> > +	k = rte_zmalloc_socket(NULL, key_tbl_size,
> > +			RTE_CACHE_LINE_SIZE, params->socket_id);
> > +
> > +	if (k == NULL) {
> > +		RTE_LOG(ERR, HASH, "memory allocation failed\n");
> > +		goto err_unlock;
> > +	}
> > +
> > +	tbl_chng_cnt = rte_zmalloc_socket(NULL, sizeof(uint32_t),
> > +			RTE_CACHE_LINE_SIZE, params->socket_id);
> > +
> > +	if (tbl_chng_cnt == NULL) {
> > +		RTE_LOG(ERR, HASH, "memory allocation failed\n");
> > +		goto err_unlock;
> > +	}
> > +
> > +/*
> > + * If x86 architecture is used, select appropriate compare function,
> > + * which may use x86 intrinsics, otherwise use memcmp
> > + */
> > +#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
> > +	/* Select function to compare keys */
> > +	switch (params->key_len) {
> > +	case 16:
> > +		h->cmp_jump_table_idx = KEY_16_BYTES;
> > +		break;
> > +	case 32:
> > +		h->cmp_jump_table_idx = KEY_32_BYTES;
> > +		break;
> > +	case 48:
> > +		h->cmp_jump_table_idx = KEY_48_BYTES;
> > +		break;
> > +	case 64:
> > +		h->cmp_jump_table_idx = KEY_64_BYTES;
> > +		break;
> > +	case 80:
> > +		h->cmp_jump_table_idx = KEY_80_BYTES;
> > +		break;
> > +	case 96:
> > +		h->cmp_jump_table_idx = KEY_96_BYTES;
> > +		break;
> > +	case 112:
> > +		h->cmp_jump_table_idx = KEY_112_BYTES;
> > +		break;
> > +	case 128:
> > +		h->cmp_jump_table_idx = KEY_128_BYTES;
> > +		break;
> > +	default:
> > +		/* If key is not multiple of 16, use generic memcmp */
> > +		h->cmp_jump_table_idx = KEY_OTHER_BYTES;
> > +	}
> > +#else
> > +	h->cmp_jump_table_idx = KEY_OTHER_BYTES;
> > +#endif
> > +
> > +	if (use_local_cache) {
> > +		h->local_free_slots = rte_zmalloc_socket(NULL,
> > +				sizeof(struct lcore_cache) * RTE_MAX_LCORE,
> > +				RTE_CACHE_LINE_SIZE, params->socket_id);
> > +	}
> > +
> > +	/* Default hash function */
> > +#if defined(RTE_ARCH_X86)
> > +	default_hash_func = (rte_hash_function)rte_hash_crc;
> > +#elif defined(RTE_ARCH_ARM64)
> > +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_CRC32))
> > +		default_hash_func = (rte_hash_function)rte_hash_crc;
> > +#endif
> > +	/* Setup hash context */
> > +	snprintf(h->name, sizeof(h->name), "%s", params->name);
> > +	h->entries = params->entries;
> > +	h->key_len = params->key_len;
> > +	h->key_entry_size = key_entry_size;
> > +	h->hash_func_init_val = params->hash_func_init_val;
> > +
> > +	h->num_buckets = num_buckets;
> > +	h->bucket_bitmask = h->num_buckets - 1;
> > +	h->buckets = buckets;
> > +	h->buckets_ext = buckets_ext;
> > +	h->free_ext_bkts = r_ext;
> > +	h->hash_func = (params->hash_func == NULL) ?
> > +		default_hash_func : params->hash_func;
> > +	h->key_store = k;
> > +	h->free_slots = r;
> > +	h->tbl_chng_cnt = tbl_chng_cnt;
> > +	*h->tbl_chng_cnt = 0;
> > +	h->hw_trans_mem_support = rte_tm_supported();
> > +	h->use_local_cache = use_local_cache;
> > +	h->readwrite_concur_support = readwrite_concur_support;
> > +	h->ext_table_support = ext_table_support;
> > +	h->writer_takes_lock = writer_takes_lock;
> > +	h->no_free_on_del = no_free_on_del;
> > +	h->readwrite_concur_lf_support = readwrite_concur_lf_support;
> > +
> > +#if defined(RTE_ARCH_X86)
> > +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE2))
> > +		h->sig_cmp_fn = RTE_HASH_COMPARE_SSE;
> > +	else
> > +#endif
> > +		h->sig_cmp_fn = RTE_HASH_COMPARE_SCALAR;
> > +
> > +	if (h->writer_takes_lock) {
> > +		h->readwrite_lock = rte_malloc(NULL, sizeof(rte_rwlock_t),
> > +						RTE_CACHE_LINE_SIZE);
> > +		if (h->readwrite_lock == NULL)
> > +			goto err_unlock;
> > +
> > +		rte_rwlock_init(h->readwrite_lock);
> > +	}
> > +
> > +	/* Populate free slots ring. Entry zero is reserved for key misses. */
> > +	for (i = 1; i < num_key_slots; i++)
> > +		rte_ring_sp_enqueue(r, (void *)((uintptr_t) i));
> > +
> > +	te->data = (void *) h;
> > +	TAILQ_INSERT_TAIL(hash_list, te, next);
> > +	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
> > +
> > +	return h;
> > +err_unlock:
> > +	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
> > +err:
> > +	rte_ring_free(r);
> > +	rte_ring_free(r_ext);
> > +	rte_free(te);
> > +	rte_free(h);
> > +	rte_free(buckets);
> > +	rte_free(buckets_ext);
> > +	rte_free(k);
> > +	rte_free(tbl_chng_cnt);
> > +	return NULL;
> > +}
> > +BIND_DEFAULT_SYMBOL(rte_hash_create, _v1811, 18.11);
> > +MAP_STATIC_SYMBOL(
> > +	struct rte_hash *rte_hash_create(
> > +		const struct rte_hash_parameters *params),
> > +		rte_hash_create_v1811);
> >  
> >  void
> >  rte_hash_free(struct rte_hash *h)
> > diff --git a/lib/librte_hash/rte_hash.h b/lib/librte_hash/rte_hash.h
> > index c93d1a137..93c7019ec 100644
> > --- a/lib/librte_hash/rte_hash.h
> > +++ b/lib/librte_hash/rte_hash.h
> > @@ -30,13 +30,27 @@ extern "C" {
> >  #define RTE_HASH_LOOKUP_BULK_MAX		64
> >  #define RTE_HASH_LOOKUP_MULTI_MAX		RTE_HASH_LOOKUP_BULK_MAX
> >  
> > -/** Enable Hardware transactional memory support. */
> > +/**
> > + * @deprecated
> > + * This define will be removed in the next release.
> > + * If the target platform supports hardware transactional memory
> > + * it will be used without user consent as it provides the best possible
> > + * performance.
> > + *
> > + * Enable Hardware transactional memory support.
> > + */
> >  #define RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT	0x01
> >  
> >  /** Default behavior of insertion, single writer/multi writer */
> >  #define RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD 0x02
> >  
> > -/** Flag to support reader writer concurrency */
> > +/**
> > + * @deprecated
> > + * This define will be removed in the next release.
> > + * This library should be thread-safe by default.
> > + *
> > + * Flag to support reader writer concurrency
> > + */
> >  #define RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY 0x04
> >  
> 
> Do we not need/want to add some new flags to disable these features, in
> case there are cases where either RW concurrency, or transaction memory is
> unwanted?
> 
Commenting more on my own comment - I'm looking for suggestions here from
others. Perhaps we should take the YAGNI principle and not add in such
flags unless someone comes with a use-case that needs it. Having negative
flags where before we had positive could be confusing.

Re: [dpdk-dev] [PATCH 1/3] hash: deprecate lock ellision and read/write concurreny flags

[Honnappa Nagarahalli]

> >
> > diff --git a/lib/librte_hash/rte_cuckoo_hash.c
> > b/lib/librte_hash/rte_cuckoo_hash.c
> > index 5ddcccd87..a11de22be 100644
> > --- a/lib/librte_hash/rte_cuckoo_hash.c
> > +++ b/lib/librte_hash/rte_cuckoo_hash.c
> > @@ -121,7 +121,7 @@ get_alt_bucket_index(const struct rte_hash *h,  }
> >
> >  struct rte_hash *
> > -rte_hash_create(const struct rte_hash_parameters *params)
> > +rte_hash_create_v20(const struct rte_hash_parameters *params)
> >  {
> >  	struct rte_hash *h = NULL;
> >  	struct rte_tailq_entry *te = NULL;
> > @@ -446,6 +446,323 @@ rte_hash_create(const struct
> rte_hash_parameters *params)
> >  	rte_free(tbl_chng_cnt);
> >  	return NULL;
> >  }
> > +VERSION_SYMBOL(rte_hash_create, _v20, 2.0);
> > +
> 
> To make reviewing this easier, can I ask if in V2 you can put the v18.11
> function first, before the existing one. Hopefully that means that the "new"
> function from git's point of view is the existing one, showing it as a block
> add that we can pretty much skip reviewing. The benefit of this is that the
> changes in the v1811 should then show as line-by-line diffs in the patch, so
> we can easily review the changes made in the new case. It's hard to spot
> them in the whole function below.
> 
I agree it is painful to review. I tried what you suggested here, it still shows v18.11 function as a complete new block of code. I will create another commit in the series. The first commit will have just the exact duplication of the function (but renamed to v18.11). The next commit will have all the required changes for the v18.11 function and deprecation related changes. We can squash both of them once the review completes.
Let me know if there is a better solution.

> > +struct rte_hash *
> > +rte_hash_create_v1811(const struct rte_hash_parameters *params) {
> > +	struct rte_hash *h = NULL;
> > +	struct rte_tailq_entry *te = NULL;
> > +	struct rte_hash_list *hash_list;
> > +	struct rte_ring *r = NULL;
> > +	struct rte_ring *r_ext = NULL;
> > +	char hash_name[RTE_HASH_NAMESIZE];
> > +	void *k = NULL;
> > +	void *buckets = NULL;
> > +	void *buckets_ext = NULL;
> > +	char ring_name[RTE_RING_NAMESIZE];
> > +	char ext_ring_name[RTE_RING_NAMESIZE];
> > +	unsigned num_key_slots;
> > +	unsigned i;
> > +
> > +	/* Validate correct usage of extra options */
> > +	if ((params->extra_flag &
> RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF) &&
> > +	    (params->extra_flag & RTE_HASH_EXTRA_FLAGS_EXT_TABLE)) {
> > +		rte_errno = EINVAL;
> > +		RTE_LOG(ERR, HASH, "rte_hash_create: extendable bucket "
> > +			"feature not supported with rw concurrency "
> > +			"lock free\n");
> 
> Please don't split the error text across lines. If you put it on a line by itself,
> hopefully checkpatch should not complain. If checkpatch does complain
> about the long line, just ignore it!
> [Yes, I know this is copied from original code, but since it appears as new
> code in this patch, we should fix it]
> 
Ok, I can do that. There are other check patch issues which I did not fix (thinking it is old code), will fix those as well.

> > +		return NULL;
> > +	}
> > +
> > +	/* Check extra flags field to check extra options. */
> > +	if (params->extra_flag &
> RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD)
> > +		use_local_cache = 1;
> > +
> >
> >  void
> >  rte_hash_free(struct rte_hash *h)
> > diff --git a/lib/librte_hash/rte_hash.h b/lib/librte_hash/rte_hash.h
> > index c93d1a137..93c7019ec 100644
> > --- a/lib/librte_hash/rte_hash.h
> > +++ b/lib/librte_hash/rte_hash.h
> > @@ -30,13 +30,27 @@ extern "C" {
> >  #define RTE_HASH_LOOKUP_BULK_MAX		64
> >  #define RTE_HASH_LOOKUP_MULTI_MAX
> 	RTE_HASH_LOOKUP_BULK_MAX
> >
> > -/** Enable Hardware transactional memory support. */
> > +/**
> > + * @deprecated
> > + * This define will be removed in the next release.
> > + * If the target platform supports hardware transactional memory
> > + * it will be used without user consent as it provides the best
> > +possible
> > + * performance.
> > + *
> > + * Enable Hardware transactional memory support.
> > + */
> >  #define RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT	0x01
> >
> >  /** Default behavior of insertion, single writer/multi writer */
> > #define RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD 0x02
> >
> > -/** Flag to support reader writer concurrency */
> > +/**
> > + * @deprecated
> > + * This define will be removed in the next release.
> > + * This library should be thread-safe by default.
> > + *
> > + * Flag to support reader writer concurrency  */
> >  #define RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY 0x04
> >
> 
> Do we not need/want to add some new flags to disable these features, in
> case there are cases where either RW concurrency, or transaction memory is
> unwanted?
> 
I cannot think of a case where RW concurrency is not required, except for single thread use case. In the single thread use case, the existing flags provide the most optimal solution. Hopefully, it will be clear from the changes.
I am not sure on if there is a case for no wanting transactional memory while using lock based RW concurrency. We have a flag for non-lock based RW concurrency in which case transactional memory will not be used.
IMO, I do not see a need for additional flags.
 
> >  /** Flag to indicate the extendabe bucket table feature should be
> > used */ @@ -105,6 +119,10 @@ struct rte_hash;
> >   */
> >  struct rte_hash *
> >  rte_hash_create(const struct rte_hash_parameters *params);
> > +struct rte_hash *
> > +rte_hash_create_v20(const struct rte_hash_parameters *params); struct
> > +rte_hash * rte_hash_create_v1811(const struct rte_hash_parameters
> > +*params);
> >
> >  /**
> >   * Set a new hash compare function other than the default one.
> > diff --git a/lib/librte_hash/rte_hash_version.map
> > b/lib/librte_hash/rte_hash_version.map
> > index 734ae28b0..c72469099 100644
> > --- a/lib/librte_hash/rte_hash_version.map
> > +++ b/lib/librte_hash/rte_hash_version.map
> > @@ -54,6 +54,13 @@ DPDK_18.08 {
> >
> >  } DPDK_16.07;
> >
> > +DPDK_18.11 {
> > +	global:
> > +
> > +	rte_hash_create;
> > +
> > +} DPDK_18.08;
> > +
> >  EXPERIMENTAL {
> >  	global:
> >
> > --
> > 2.17.1
> >

Re: [dpdk-dev] [PATCH 1/3] hash: deprecate lock ellision and read/write concurreny flags

[Bruce Richardson]

On Thu, Nov 01, 2018 at 07:43:29PM +0000, Honnappa Nagarahalli wrote:
> > >
> > > diff --git a/lib/librte_hash/rte_cuckoo_hash.c
> > > b/lib/librte_hash/rte_cuckoo_hash.c
> > > index 5ddcccd87..a11de22be 100644
> > > --- a/lib/librte_hash/rte_cuckoo_hash.c
> > > +++ b/lib/librte_hash/rte_cuckoo_hash.c
> > > @@ -121,7 +121,7 @@ get_alt_bucket_index(const struct rte_hash *h,  }
> > >
> > >  struct rte_hash *
> > > -rte_hash_create(const struct rte_hash_parameters *params)
> > > +rte_hash_create_v20(const struct rte_hash_parameters *params)
> > >  {
> > >  	struct rte_hash *h = NULL;
> > >  	struct rte_tailq_entry *te = NULL;
> > > @@ -446,6 +446,323 @@ rte_hash_create(const struct
> > rte_hash_parameters *params)
> > >  	rte_free(tbl_chng_cnt);
> > >  	return NULL;
> > >  }
> > > +VERSION_SYMBOL(rte_hash_create, _v20, 2.0);
> > > +
> > 
> > To make reviewing this easier, can I ask if in V2 you can put the v18.11
> > function first, before the existing one. Hopefully that means that the "new"
> > function from git's point of view is the existing one, showing it as a block
> > add that we can pretty much skip reviewing. The benefit of this is that the
> > changes in the v1811 should then show as line-by-line diffs in the patch, so
> > we can easily review the changes made in the new case. It's hard to spot
> > them in the whole function below.
> > 
> I agree it is painful to review. I tried what you suggested here, it still shows v18.11 function as a complete new block of code. I will create another commit in the series. The first commit will have just the exact duplication of the function (but renamed to v18.11). The next commit will have all the required changes for the v18.11 function and deprecation related changes. We can squash both of them once the review completes.
> Let me know if there is a better solution.

Thanks. I'll leave the squashing up to Thomas to decide, because even on
later review e.g. if we ever need to bisect, having the commits separate
could help.

> 
> > > +struct rte_hash *
> > > +rte_hash_create_v1811(const struct rte_hash_parameters *params) {
> > > +	struct rte_hash *h = NULL;
> > > +	struct rte_tailq_entry *te = NULL;
> > > +	struct rte_hash_list *hash_list;
> > > +	struct rte_ring *r = NULL;
> > > +	struct rte_ring *r_ext = NULL;
> > > +	char hash_name[RTE_HASH_NAMESIZE];
> > > +	void *k = NULL;
> > > +	void *buckets = NULL;
> > > +	void *buckets_ext = NULL;
> > > +	char ring_name[RTE_RING_NAMESIZE];
> > > +	char ext_ring_name[RTE_RING_NAMESIZE];
> > > +	unsigned num_key_slots;
> > > +	unsigned i;
> > > +
> > > +	/* Validate correct usage of extra options */
> > > +	if ((params->extra_flag &
> > RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF) &&
> > > +	    (params->extra_flag & RTE_HASH_EXTRA_FLAGS_EXT_TABLE)) {
> > > +		rte_errno = EINVAL;
> > > +		RTE_LOG(ERR, HASH, "rte_hash_create: extendable bucket "
> > > +			"feature not supported with rw concurrency "
> > > +			"lock free\n");
> > 
> > Please don't split the error text across lines. If you put it on a line by itself,
> > hopefully checkpatch should not complain. If checkpatch does complain
> > about the long line, just ignore it!
> > [Yes, I know this is copied from original code, but since it appears as new
> > code in this patch, we should fix it]
> > 
> Ok, I can do that. There are other check patch issues which I did not fix (thinking it is old code), will fix those as well.
> 
> > > +		return NULL;
> > > +	}
> > > +
> > > +	/* Check extra flags field to check extra options. */
> > > +	if (params->extra_flag &
> > RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD)
> > > +		use_local_cache = 1;
> > > +
> > >
> > >  void
> > >  rte_hash_free(struct rte_hash *h)
> > > diff --git a/lib/librte_hash/rte_hash.h b/lib/librte_hash/rte_hash.h
> > > index c93d1a137..93c7019ec 100644
> > > --- a/lib/librte_hash/rte_hash.h
> > > +++ b/lib/librte_hash/rte_hash.h
> > > @@ -30,13 +30,27 @@ extern "C" {
> > >  #define RTE_HASH_LOOKUP_BULK_MAX		64
> > >  #define RTE_HASH_LOOKUP_MULTI_MAX
> > 	RTE_HASH_LOOKUP_BULK_MAX
> > >
> > > -/** Enable Hardware transactional memory support. */
> > > +/**
> > > + * @deprecated
> > > + * This define will be removed in the next release.
> > > + * If the target platform supports hardware transactional memory
> > > + * it will be used without user consent as it provides the best
> > > +possible
> > > + * performance.
> > > + *
> > > + * Enable Hardware transactional memory support.
> > > + */
> > >  #define RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT	0x01
> > >
> > >  /** Default behavior of insertion, single writer/multi writer */
> > > #define RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD 0x02
> > >
> > > -/** Flag to support reader writer concurrency */
> > > +/**
> > > + * @deprecated
> > > + * This define will be removed in the next release.
> > > + * This library should be thread-safe by default.
> > > + *
> > > + * Flag to support reader writer concurrency  */
> > >  #define RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY 0x04
> > >
> > 
> > Do we not need/want to add some new flags to disable these features, in
> > case there are cases where either RW concurrency, or transaction memory is
> > unwanted?
> > 
> I cannot think of a case where RW concurrency is not required, except for single thread use case. In the single thread use case, the existing flags provide the most optimal solution. Hopefully, it will be clear from the changes.
> I am not sure on if there is a case for no wanting transactional memory while using lock based RW concurrency. We have a flag for non-lock based RW concurrency in which case transactional memory will not be used.
> IMO, I do not see a need for additional flags.
>  

I think for testing purposes at least, we need an option to disable TSX.
When assessing performance impacting changes to the hash, the developer
needs to evaluate the impact in both TSX and non-TSX scenarios.

/Bruce

[dpdk-dev] [PATCH v2 0/4] hash: deprecate lock ellision and read/write concurreny flags

[Honnappa Nagarahalli]

Various configuration flags in rte_hash library result in increase of
number of test cases. Configuration flags for enabling transactional
memory use and read/write concurrency are not required. These features
should be supported by default. Please refer to [1] for more context.

This patch marks these flags for deprecation in 19.02 release and cleans
up the test cases.

[1] http://mails.dpdk.org/archives/dev/2018-October/117268.html

Honnappa Nagarahalli (4):
  hash: prepare for deprecation of flags
  hash: deprecate lock ellision and read/write concurreny flags
  test/hash: stop using lock ellision and read/write concurreny flags
  doc/hash: deprecate lock ellision and read/write concurreny flags

 doc/guides/rel_notes/deprecation.rst |   5 +
 lib/librte_hash/rte_cuckoo_hash.c    | 321 ++++++++++++++++++++++++++-
 lib/librte_hash/rte_hash.h           |  20 +-
 lib/librte_hash/rte_hash_version.map |   7 +
 test/test/test_hash_multiwriter.c    |  20 +-
 test/test/test_hash_perf.c           |  40 ++--
 test/test/test_hash_readwrite.c      |  84 ++-----
 test/test/test_hash_readwrite_lf.c   | 189 ++++------------
 8 files changed, 432 insertions(+), 254 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH v2 1/4] hash: prepare for deprecation of flags

[Honnappa Nagarahalli]

Lock ellision and read/write concurreny flags need to be deprecated.
Create the new version of the function and fix checkpatch issues.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
---
 lib/librte_hash/rte_cuckoo_hash.c | 329 ++++++++++++++++++++++++++++++
 lib/librte_hash/rte_hash.h        |   2 +
 2 files changed, 331 insertions(+)

diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index 5ddcccd87..ec3b519ba 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -447,6 +447,335 @@ rte_hash_create(const struct rte_hash_parameters *params)
 	return NULL;
 }
 
+struct rte_hash *
+rte_hash_create_v1811(const struct rte_hash_parameters *params)
+{
+	struct rte_hash *h = NULL;
+	struct rte_tailq_entry *te = NULL;
+	struct rte_hash_list *hash_list;
+	struct rte_ring *r = NULL;
+	struct rte_ring *r_ext = NULL;
+	char hash_name[RTE_HASH_NAMESIZE];
+	void *k = NULL;
+	void *buckets = NULL;
+	void *buckets_ext = NULL;
+	char ring_name[RTE_RING_NAMESIZE];
+	char ext_ring_name[RTE_RING_NAMESIZE];
+	unsigned num_key_slots;
+	unsigned i;
+	unsigned int hw_trans_mem_support = 0, use_local_cache = 0;
+	unsigned int ext_table_support = 0;
+	unsigned int readwrite_concur_support = 0;
+	unsigned int writer_takes_lock = 0;
+	unsigned int no_free_on_del = 0;
+	uint32_t *tbl_chng_cnt = NULL;
+	unsigned int readwrite_concur_lf_support = 0;
+
+	rte_hash_function default_hash_func = (rte_hash_function)rte_jhash;
+
+	hash_list = RTE_TAILQ_CAST(rte_hash_tailq.head, rte_hash_list);
+
+	if (params == NULL) {
+		RTE_LOG(ERR, HASH, "%s: no parameters\n", __func__);
+		return NULL;
+	}
+
+	/* Check for valid parameters */
+	if ((params->entries > RTE_HASH_ENTRIES_MAX) ||
+			(params->entries < RTE_HASH_BUCKET_ENTRIES) ||
+			(params->key_len == 0)) {
+		rte_errno = EINVAL;
+		RTE_LOG(ERR, HASH, "%s: invalid parameters\n", __func__);
+		return NULL;
+	}
+
+	/* Validate correct usage of extra options */
+	if ((params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY) &&
+	    (params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF)) {
+		rte_errno = EINVAL;
+		RTE_LOG(ERR, HASH, "rte_hash_create: choose rw concurrency or "
+			"rw concurrency lock free\n");
+		return NULL;
+	}
+
+	if ((params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF) &&
+	    (params->extra_flag & RTE_HASH_EXTRA_FLAGS_EXT_TABLE)) {
+		rte_errno = EINVAL;
+		RTE_LOG(ERR, HASH, "%s: extendable bucket feature not supported with rw concurrency lock free\n", __func__);
+		return NULL;
+	}
+
+	/* Check extra flags field to check extra options. */
+	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT)
+		hw_trans_mem_support = 1;
+
+	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD) {
+		use_local_cache = 1;
+		writer_takes_lock = 1;
+	}
+
+	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY) {
+		readwrite_concur_support = 1;
+		writer_takes_lock = 1;
+	}
+
+	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_EXT_TABLE)
+		ext_table_support = 1;
+
+	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_NO_FREE_ON_DEL)
+		no_free_on_del = 1;
+
+	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF) {
+		readwrite_concur_lf_support = 1;
+		/* Enable not freeing internal memory/index on delete */
+		no_free_on_del = 1;
+	}
+
+	/* Store all keys and leave the first entry as a dummy entry
+	 * for lookup_bulk.
+	 */
+	if (use_local_cache)
+		/*
+		 * Increase number of slots by total number of indices
+		 * that can be stored in the lcore caches
+		 * except for the first cache
+		 */
+		num_key_slots = params->entries + (RTE_MAX_LCORE - 1) *
+					(LCORE_CACHE_SIZE - 1) + 1;
+	else
+		num_key_slots = params->entries + 1;
+
+	snprintf(ring_name, sizeof(ring_name), "HT_%s", params->name);
+	/* Create ring (Dummy slot index is not enqueued) */
+	r = rte_ring_create(ring_name, rte_align32pow2(num_key_slots),
+			params->socket_id, 0);
+	if (r == NULL) {
+		RTE_LOG(ERR, HASH, "memory allocation failed\n");
+		goto err;
+	}
+
+	const uint32_t num_buckets = rte_align32pow2(params->entries) /
+						RTE_HASH_BUCKET_ENTRIES;
+
+	/* Create ring for extendable buckets. */
+	if (ext_table_support) {
+		snprintf(ext_ring_name, sizeof(ext_ring_name), "HT_EXT_%s",
+								params->name);
+		r_ext = rte_ring_create(ext_ring_name,
+				rte_align32pow2(num_buckets + 1),
+				params->socket_id, 0);
+
+		if (r_ext == NULL) {
+			RTE_LOG(ERR, HASH, "ext buckets memory allocation "
+								"failed\n");
+			goto err;
+		}
+	}
+
+	snprintf(hash_name, sizeof(hash_name), "HT_%s", params->name);
+
+	rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
+
+	/* guarantee there's no existing: this is normally already checked
+	 * by ring creation above.
+	 */
+	TAILQ_FOREACH(te, hash_list, next) {
+		h = (struct rte_hash *) te->data;
+		if (strncmp(params->name, h->name, RTE_HASH_NAMESIZE) == 0)
+			break;
+	}
+	h = NULL;
+	if (te != NULL) {
+		rte_errno = EEXIST;
+		te = NULL;
+		goto err_unlock;
+	}
+
+	te = rte_zmalloc("HASH_TAILQ_ENTRY", sizeof(*te), 0);
+	if (te == NULL) {
+		RTE_LOG(ERR, HASH, "tailq entry allocation failed\n");
+		goto err_unlock;
+	}
+
+	h = (struct rte_hash *)rte_zmalloc_socket(hash_name,
+					sizeof(struct rte_hash),
+					RTE_CACHE_LINE_SIZE, params->socket_id);
+
+	if (h == NULL) {
+		RTE_LOG(ERR, HASH, "memory allocation failed\n");
+		goto err_unlock;
+	}
+
+	buckets = rte_zmalloc_socket(NULL,
+				num_buckets * sizeof(struct rte_hash_bucket),
+				RTE_CACHE_LINE_SIZE, params->socket_id);
+
+	if (buckets == NULL) {
+		RTE_LOG(ERR, HASH, "buckets memory allocation failed\n");
+		goto err_unlock;
+	}
+
+	/* Allocate same number of extendable buckets */
+	if (ext_table_support) {
+		buckets_ext = rte_zmalloc_socket(NULL,
+				num_buckets * sizeof(struct rte_hash_bucket),
+				RTE_CACHE_LINE_SIZE, params->socket_id);
+		if (buckets_ext == NULL) {
+			RTE_LOG(ERR, HASH, "ext buckets memory allocation "
+							"failed\n");
+			goto err_unlock;
+		}
+		/* Populate ext bkt ring. We reserve 0 similar to the
+		 * key-data slot, just in case in future we want to
+		 * use bucket index for the linked list and 0 means NULL
+		 * for next bucket
+		 */
+		for (i = 1; i <= num_buckets; i++)
+			rte_ring_sp_enqueue(r_ext, (void *)((uintptr_t) i));
+	}
+
+	const uint32_t key_entry_size =
+		RTE_ALIGN(sizeof(struct rte_hash_key) + params->key_len,
+			  KEY_ALIGNMENT);
+	const uint64_t key_tbl_size = (uint64_t) key_entry_size * num_key_slots;
+
+	k = rte_zmalloc_socket(NULL, key_tbl_size,
+			RTE_CACHE_LINE_SIZE, params->socket_id);
+
+	if (k == NULL) {
+		RTE_LOG(ERR, HASH, "memory allocation failed\n");
+		goto err_unlock;
+	}
+
+	tbl_chng_cnt = rte_zmalloc_socket(NULL, sizeof(uint32_t),
+			RTE_CACHE_LINE_SIZE, params->socket_id);
+
+	if (tbl_chng_cnt == NULL) {
+		RTE_LOG(ERR, HASH, "memory allocation failed\n");
+		goto err_unlock;
+	}
+
+/*
+ * If x86 architecture is used, select appropriate compare function,
+ * which may use x86 intrinsics, otherwise use memcmp
+ */
+#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
+	/* Select function to compare keys */
+	switch (params->key_len) {
+	case 16:
+		h->cmp_jump_table_idx = KEY_16_BYTES;
+		break;
+	case 32:
+		h->cmp_jump_table_idx = KEY_32_BYTES;
+		break;
+	case 48:
+		h->cmp_jump_table_idx = KEY_48_BYTES;
+		break;
+	case 64:
+		h->cmp_jump_table_idx = KEY_64_BYTES;
+		break;
+	case 80:
+		h->cmp_jump_table_idx = KEY_80_BYTES;
+		break;
+	case 96:
+		h->cmp_jump_table_idx = KEY_96_BYTES;
+		break;
+	case 112:
+		h->cmp_jump_table_idx = KEY_112_BYTES;
+		break;
+	case 128:
+		h->cmp_jump_table_idx = KEY_128_BYTES;
+		break;
+	default:
+		/* If key is not multiple of 16, use generic memcmp */
+		h->cmp_jump_table_idx = KEY_OTHER_BYTES;
+	}
+#else
+	h->cmp_jump_table_idx = KEY_OTHER_BYTES;
+#endif
+
+	if (use_local_cache) {
+		h->local_free_slots = rte_zmalloc_socket(NULL,
+				sizeof(struct lcore_cache) * RTE_MAX_LCORE,
+				RTE_CACHE_LINE_SIZE, params->socket_id);
+	}
+
+	/* Default hash function */
+#if defined(RTE_ARCH_X86)
+	default_hash_func = (rte_hash_function)rte_hash_crc;
+#elif defined(RTE_ARCH_ARM64)
+	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_CRC32))
+		default_hash_func = (rte_hash_function)rte_hash_crc;
+#endif
+	/* Setup hash context */
+	snprintf(h->name, sizeof(h->name), "%s", params->name);
+	h->entries = params->entries;
+	h->key_len = params->key_len;
+	h->key_entry_size = key_entry_size;
+	h->hash_func_init_val = params->hash_func_init_val;
+
+	h->num_buckets = num_buckets;
+	h->bucket_bitmask = h->num_buckets - 1;
+	h->buckets = buckets;
+	h->buckets_ext = buckets_ext;
+	h->free_ext_bkts = r_ext;
+	h->hash_func = (params->hash_func == NULL) ?
+		default_hash_func : params->hash_func;
+	h->key_store = k;
+	h->free_slots = r;
+	h->tbl_chng_cnt = tbl_chng_cnt;
+	*h->tbl_chng_cnt = 0;
+	h->hw_trans_mem_support = hw_trans_mem_support;
+	h->use_local_cache = use_local_cache;
+	h->readwrite_concur_support = readwrite_concur_support;
+	h->ext_table_support = ext_table_support;
+	h->writer_takes_lock = writer_takes_lock;
+	h->no_free_on_del = no_free_on_del;
+	h->readwrite_concur_lf_support = readwrite_concur_lf_support;
+
+#if defined(RTE_ARCH_X86)
+	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE2))
+		h->sig_cmp_fn = RTE_HASH_COMPARE_SSE;
+	else
+#endif
+		h->sig_cmp_fn = RTE_HASH_COMPARE_SCALAR;
+
+	/* Writer threads need to take the lock when:
+	 * 1) RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY is enabled OR
+	 * 2) RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD is enabled
+	 */
+	if (h->writer_takes_lock) {
+		h->readwrite_lock = rte_malloc(NULL, sizeof(rte_rwlock_t),
+						RTE_CACHE_LINE_SIZE);
+		if (h->readwrite_lock == NULL)
+			goto err_unlock;
+
+		rte_rwlock_init(h->readwrite_lock);
+	}
+
+	/* Populate free slots ring. Entry zero is reserved for key misses. */
+	for (i = 1; i < num_key_slots; i++)
+		rte_ring_sp_enqueue(r, (void *)((uintptr_t) i));
+
+	te->data = (void *) h;
+	TAILQ_INSERT_TAIL(hash_list, te, next);
+	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
+
+	return h;
+err_unlock:
+	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
+err:
+	rte_ring_free(r);
+	rte_ring_free(r_ext);
+	rte_free(te);
+	rte_free(h);
+	rte_free(buckets);
+	rte_free(buckets_ext);
+	rte_free(k);
+	rte_free(tbl_chng_cnt);
+	return NULL;
+}
+
 void
 rte_hash_free(struct rte_hash *h)
 {
diff --git a/lib/librte_hash/rte_hash.h b/lib/librte_hash/rte_hash.h
index c93d1a137..f049b9dd0 100644
--- a/lib/librte_hash/rte_hash.h
+++ b/lib/librte_hash/rte_hash.h
@@ -105,6 +105,8 @@ struct rte_hash;
  */
 struct rte_hash *
 rte_hash_create(const struct rte_hash_parameters *params);
+struct rte_hash *
+rte_hash_create_v1811(const struct rte_hash_parameters *params);
 
 /**
  * Set a new hash compare function other than the default one.
-- 
2.17.1

Re: [dpdk-dev] [PATCH v2 1/4] hash: prepare for deprecation of flags

[Bruce Richardson]

On Thu, Nov 01, 2018 at 06:25:19PM -0500, Honnappa Nagarahalli wrote:
> Lock ellision and read/write concurreny flags need to be deprecated.
typo: concurrency.

> Create the new version of the function and fix checkpatch issues.
> 
Maybe emphasise that the new version is identical to the old, in case this
patch does not get squashed into next.
"Create a copy of the hash_create function, identical to the original
except for fixing checkpatch issues. Later patches will update this copy"

> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> ---

[dpdk-dev] [PATCH v2 2/4] hash: deprecate lock ellision and read/write concurreny flags

[Honnappa Nagarahalli]

Hash library should provide read/write concurrency by default
as most of the use cases require read/write concurrency. Hence
the flag RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY is deprecated.
The library will decide if locking is required to provide
the concurrency based on other configuration flags.

If a lock is used to provide the read/write concurrency, best
possible locking mechanism available on the platform should
be used. Hence, the flag RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT
is deprecated. The library will use transactional memory
if the platform supports it.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
---
 lib/librte_hash/rte_cuckoo_hash.c    | 46 +++++++++++-----------------
 lib/librte_hash/rte_hash.h           | 20 ++++++++++--
 lib/librte_hash/rte_hash_version.map |  7 +++++
 3 files changed, 43 insertions(+), 30 deletions(-)

diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index ec3b519ba..7f97cddd4 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -121,7 +121,7 @@ get_alt_bucket_index(const struct rte_hash *h,
 }
 
 struct rte_hash *
-rte_hash_create(const struct rte_hash_parameters *params)
+rte_hash_create_v20(const struct rte_hash_parameters *params)
 {
 	struct rte_hash *h = NULL;
 	struct rte_tailq_entry *te = NULL;
@@ -446,6 +446,7 @@ rte_hash_create(const struct rte_hash_parameters *params)
 	rte_free(tbl_chng_cnt);
 	return NULL;
 }
+VERSION_SYMBOL(rte_hash_create, _v20, 2.0);
 
 struct rte_hash *
 rte_hash_create_v1811(const struct rte_hash_parameters *params)
@@ -463,10 +464,10 @@ rte_hash_create_v1811(const struct rte_hash_parameters *params)
 	char ext_ring_name[RTE_RING_NAMESIZE];
 	unsigned num_key_slots;
 	unsigned i;
-	unsigned int hw_trans_mem_support = 0, use_local_cache = 0;
+	unsigned int use_local_cache = 0;
 	unsigned int ext_table_support = 0;
-	unsigned int readwrite_concur_support = 0;
-	unsigned int writer_takes_lock = 0;
+	unsigned int readwrite_concur_support = 1;
+	unsigned int writer_takes_lock = 1;
 	unsigned int no_free_on_del = 0;
 	uint32_t *tbl_chng_cnt = NULL;
 	unsigned int readwrite_concur_lf_support = 0;
@@ -490,14 +491,6 @@ rte_hash_create_v1811(const struct rte_hash_parameters *params)
 	}
 
 	/* Validate correct usage of extra options */
-	if ((params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY) &&
-	    (params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF)) {
-		rte_errno = EINVAL;
-		RTE_LOG(ERR, HASH, "rte_hash_create: choose rw concurrency or "
-			"rw concurrency lock free\n");
-		return NULL;
-	}
-
 	if ((params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF) &&
 	    (params->extra_flag & RTE_HASH_EXTRA_FLAGS_EXT_TABLE)) {
 		rte_errno = EINVAL;
@@ -506,18 +499,8 @@ rte_hash_create_v1811(const struct rte_hash_parameters *params)
 	}
 
 	/* Check extra flags field to check extra options. */
-	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT)
-		hw_trans_mem_support = 1;
-
-	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD) {
+	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD)
 		use_local_cache = 1;
-		writer_takes_lock = 1;
-	}
-
-	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY) {
-		readwrite_concur_support = 1;
-		writer_takes_lock = 1;
-	}
 
 	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_EXT_TABLE)
 		ext_table_support = 1;
@@ -526,11 +509,17 @@ rte_hash_create_v1811(const struct rte_hash_parameters *params)
 		no_free_on_del = 1;
 
 	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF) {
+		/* Do not use lock for RW concurrency */
+		readwrite_concur_support = 0;
 		readwrite_concur_lf_support = 1;
 		/* Enable not freeing internal memory/index on delete */
 		no_free_on_del = 1;
 	}
 
+	if ((params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF) &&
+	    !(params->extra_flag & RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD))
+		writer_takes_lock = 0;
+
 	/* Store all keys and leave the first entry as a dummy entry
 	 * for lookup_bulk.
 	 */
@@ -725,7 +714,7 @@ rte_hash_create_v1811(const struct rte_hash_parameters *params)
 	h->free_slots = r;
 	h->tbl_chng_cnt = tbl_chng_cnt;
 	*h->tbl_chng_cnt = 0;
-	h->hw_trans_mem_support = hw_trans_mem_support;
+	h->hw_trans_mem_support = rte_tm_supported();
 	h->use_local_cache = use_local_cache;
 	h->readwrite_concur_support = readwrite_concur_support;
 	h->ext_table_support = ext_table_support;
@@ -740,10 +729,6 @@ rte_hash_create_v1811(const struct rte_hash_parameters *params)
 #endif
 		h->sig_cmp_fn = RTE_HASH_COMPARE_SCALAR;
 
-	/* Writer threads need to take the lock when:
-	 * 1) RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY is enabled OR
-	 * 2) RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD is enabled
-	 */
 	if (h->writer_takes_lock) {
 		h->readwrite_lock = rte_malloc(NULL, sizeof(rte_rwlock_t),
 						RTE_CACHE_LINE_SIZE);
@@ -775,6 +760,11 @@ rte_hash_create_v1811(const struct rte_hash_parameters *params)
 	rte_free(tbl_chng_cnt);
 	return NULL;
 }
+BIND_DEFAULT_SYMBOL(rte_hash_create, _v1811, 18.11);
+MAP_STATIC_SYMBOL(
+	struct rte_hash *rte_hash_create(
+		const struct rte_hash_parameters *params),
+		rte_hash_create_v1811);
 
 void
 rte_hash_free(struct rte_hash *h)
diff --git a/lib/librte_hash/rte_hash.h b/lib/librte_hash/rte_hash.h
index f049b9dd0..93c7019ec 100644
--- a/lib/librte_hash/rte_hash.h
+++ b/lib/librte_hash/rte_hash.h
@@ -30,13 +30,27 @@ extern "C" {
 #define RTE_HASH_LOOKUP_BULK_MAX		64
 #define RTE_HASH_LOOKUP_MULTI_MAX		RTE_HASH_LOOKUP_BULK_MAX
 
-/** Enable Hardware transactional memory support. */
+/**
+ * @deprecated
+ * This define will be removed in the next release.
+ * If the target platform supports hardware transactional memory
+ * it will be used without user consent as it provides the best possible
+ * performance.
+ *
+ * Enable Hardware transactional memory support.
+ */
 #define RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT	0x01
 
 /** Default behavior of insertion, single writer/multi writer */
 #define RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD 0x02
 
-/** Flag to support reader writer concurrency */
+/**
+ * @deprecated
+ * This define will be removed in the next release.
+ * This library should be thread-safe by default.
+ *
+ * Flag to support reader writer concurrency
+ */
 #define RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY 0x04
 
 /** Flag to indicate the extendabe bucket table feature should be used */
@@ -106,6 +120,8 @@ struct rte_hash;
 struct rte_hash *
 rte_hash_create(const struct rte_hash_parameters *params);
 struct rte_hash *
+rte_hash_create_v20(const struct rte_hash_parameters *params);
+struct rte_hash *
 rte_hash_create_v1811(const struct rte_hash_parameters *params);
 
 /**
diff --git a/lib/librte_hash/rte_hash_version.map b/lib/librte_hash/rte_hash_version.map
index 734ae28b0..c72469099 100644
--- a/lib/librte_hash/rte_hash_version.map
+++ b/lib/librte_hash/rte_hash_version.map
@@ -54,6 +54,13 @@ DPDK_18.08 {
 
 } DPDK_16.07;
 
+DPDK_18.11 {
+	global:
+
+	rte_hash_create;
+
+} DPDK_18.08;
+
 EXPERIMENTAL {
 	global:
 
-- 
2.17.1

[dpdk-dev] [PATCH v2 3/4] test/hash: stop using lock ellision and read/write concurreny flags

[Honnappa Nagarahalli]

With the deprecation of RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY and
RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT flags, the test cases
can be simplified. This results in shorter run times.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
---
 lib/librte_hash/rte_hash.h         |   2 -
 test/test/test_hash_multiwriter.c  |  20 +--
 test/test/test_hash_perf.c         |  40 +++---
 test/test/test_hash_readwrite.c    |  84 ++++---------
 test/test/test_hash_readwrite_lf.c | 189 ++++++-----------------------
 5 files changed, 82 insertions(+), 253 deletions(-)

diff --git a/lib/librte_hash/rte_hash.h b/lib/librte_hash/rte_hash.h
index 93c7019ec..e7e3397d5 100644
--- a/lib/librte_hash/rte_hash.h
+++ b/lib/librte_hash/rte_hash.h
@@ -32,7 +32,6 @@ extern "C" {
 
 /**
  * @deprecated
- * This define will be removed in the next release.
  * If the target platform supports hardware transactional memory
  * it will be used without user consent as it provides the best possible
  * performance.
@@ -46,7 +45,6 @@ extern "C" {
 
 /**
  * @deprecated
- * This define will be removed in the next release.
  * This library should be thread-safe by default.
  *
  * Flag to support reader writer concurrency
diff --git a/test/test/test_hash_multiwriter.c b/test/test/test_hash_multiwriter.c
index d447f6dca..5469fb705 100644
--- a/test/test/test_hash_multiwriter.c
+++ b/test/test/test_hash_multiwriter.c
@@ -46,8 +46,6 @@ uint32_t rounded_nb_total_tsx_insertion;
 static rte_atomic64_t gcycles;
 static rte_atomic64_t ginsertions;
 
-static int use_htm;
-
 static int
 test_hash_multiwriter_worker(void *arg)
 {
@@ -113,13 +111,9 @@ test_hash_multiwriter(void)
 		.hash_func_init_val = 0,
 		.socket_id = rte_socket_id(),
 	};
-	if (use_htm)
-		hash_params.extra_flag =
-			RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT
-				| RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD;
-	else
-		hash_params.extra_flag =
-			RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD;
+
+	hash_params.extra_flag =
+		RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD;
 
 	struct rte_hash *handle;
 	char name[RTE_HASH_NAMESIZE];
@@ -272,19 +266,15 @@ test_hash_multiwriter_main(void)
 	if (!rte_tm_supported()) {
 		printf("Hardware transactional memory (lock elision) "
 			"is NOT supported\n");
+		printf("Test multi-writer without Hardware transactional "
+			"memory\n");
 	} else {
 		printf("Hardware transactional memory (lock elision) "
 			"is supported\n");
 
 		printf("Test multi-writer with Hardware transactional memory\n");
-
-		use_htm = 1;
-		if (test_hash_multiwriter() < 0)
-			return -1;
 	}
 
-	printf("Test multi-writer without Hardware transactional memory\n");
-	use_htm = 0;
 	if (test_hash_multiwriter() < 0)
 		return -1;
 
diff --git a/test/test/test_hash_perf.c b/test/test/test_hash_perf.c
index 525211180..03facd22a 100644
--- a/test/test/test_hash_perf.c
+++ b/test/test/test_hash_perf.c
@@ -79,7 +79,7 @@ static struct rte_hash_parameters ut_params = {
 
 static int
 create_table(unsigned int with_data, unsigned int table_index,
-		unsigned int with_locks, unsigned int ext)
+		unsigned int ext)
 {
 	char name[RTE_HASH_NAMESIZE];
 
@@ -89,17 +89,11 @@ create_table(unsigned int with_data, unsigned int table_index,
 	else
 		sprintf(name, "test_hash%d", hashtest_key_lens[table_index]);
 
-
-	if (with_locks)
-		ut_params.extra_flag =
-			RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT
-				| RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY;
+	if (ext)
+		ut_params.extra_flag = RTE_HASH_EXTRA_FLAGS_EXT_TABLE;
 	else
 		ut_params.extra_flag = 0;
 
-	if (ext)
-		ut_params.extra_flag |= RTE_HASH_EXTRA_FLAGS_EXT_TABLE;
-
 	ut_params.name = name;
 	ut_params.key_len = hashtest_key_lens[table_index];
 	ut_params.socket_id = rte_socket_id();
@@ -516,8 +510,7 @@ reset_table(unsigned table_index)
 }
 
 static int
-run_all_tbl_perf_tests(unsigned int with_pushes, unsigned int with_locks,
-						unsigned int ext)
+run_all_tbl_perf_tests(unsigned int with_pushes, unsigned int ext)
 {
 	unsigned i, j, with_data, with_hash;
 
@@ -526,7 +519,7 @@ run_all_tbl_perf_tests(unsigned int with_pushes, unsigned int with_locks,
 
 	for (with_data = 0; with_data <= 1; with_data++) {
 		for (i = 0; i < NUM_KEYSIZES; i++) {
-			if (create_table(with_data, i, with_locks, ext) < 0)
+			if (create_table(with_data, i, ext) < 0)
 				return -1;
 
 			if (get_input_keys(with_pushes, i, ext) < 0)
@@ -669,25 +662,20 @@ fbk_hash_perf_test(void)
 static int
 test_hash_perf(void)
 {
-	unsigned int with_pushes, with_locks;
-	for (with_locks = 0; with_locks <= 1; with_locks++) {
-		if (with_locks)
-			printf("\nWith locks in the code\n");
+	unsigned int with_pushes;
+
+	for (with_pushes = 0; with_pushes <= 1; with_pushes++) {
+		if (with_pushes == 0)
+			printf("\nALL ELEMENTS IN PRIMARY LOCATION\n");
 		else
-			printf("\nWithout locks in the code\n");
-		for (with_pushes = 0; with_pushes <= 1; with_pushes++) {
-			if (with_pushes == 0)
-				printf("\nALL ELEMENTS IN PRIMARY LOCATION\n");
-			else
-				printf("\nELEMENTS IN PRIMARY OR SECONDARY LOCATION\n");
-			if (run_all_tbl_perf_tests(with_pushes, with_locks, 0) < 0)
-				return -1;
-		}
+			printf("\nELEMENTS IN PRIMARY OR SECONDARY LOCATION\n");
+		if (run_all_tbl_perf_tests(with_pushes, 0) < 0)
+			return -1;
 	}
 
 	printf("\n EXTENDABLE BUCKETS PERFORMANCE\n");
 
-	if (run_all_tbl_perf_tests(1, 0, 1) < 0)
+	if (run_all_tbl_perf_tests(1, 1) < 0)
 		return -1;
 
 	if (fbk_hash_perf_test() < 0)
diff --git a/test/test/test_hash_readwrite.c b/test/test/test_hash_readwrite.c
index 01f986cf4..d389723aa 100644
--- a/test/test/test_hash_readwrite.c
+++ b/test/test/test_hash_readwrite.c
@@ -35,7 +35,7 @@ struct perf {
 	uint32_t read_write_w[NUM_TEST];
 };
 
-static struct perf htm_results, non_htm_results;
+static struct perf results;
 
 struct {
 	uint32_t *keys;
@@ -121,7 +121,7 @@ test_hash_readwrite_worker(__attribute__((unused)) void *arg)
 }
 
 static int
-init_params(int use_ext, int use_htm, int use_jhash)
+init_params(int use_ext, int use_jhash)
 {
 	unsigned int i;
 
@@ -140,15 +140,8 @@ init_params(int use_ext, int use_htm, int use_jhash)
 	else
 		hash_params.hash_func = rte_hash_crc;
 
-	if (use_htm)
-		hash_params.extra_flag =
-			RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT |
-			RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY |
-			RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD;
-	else
-		hash_params.extra_flag =
-			RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY |
-			RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD;
+	hash_params.extra_flag =
+		RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD;
 
 	if (use_ext)
 		hash_params.extra_flag |=
@@ -195,7 +188,7 @@ init_params(int use_ext, int use_htm, int use_jhash)
 }
 
 static int
-test_hash_readwrite_functional(int use_ext, int use_htm)
+test_hash_readwrite_functional(int use_ext)
 {
 	unsigned int i;
 	const void *next_key;
@@ -214,7 +207,7 @@ test_hash_readwrite_functional(int use_ext, int use_htm)
 	rte_atomic64_init(&ginsertions);
 	rte_atomic64_clear(&ginsertions);
 
-	if (init_params(use_ext, use_htm, use_jhash) != 0)
+	if (init_params(use_ext, use_jhash) != 0)
 		goto err;
 
 	if (use_ext)
@@ -351,8 +344,7 @@ test_rw_writer(void *arg)
 }
 
 static int
-test_hash_readwrite_perf(struct perf *perf_results, int use_htm,
-							int reader_faster)
+test_hash_readwrite_perf(struct perf *perf_results, int reader_faster)
 {
 	unsigned int n;
 	int ret;
@@ -379,7 +371,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(0, use_htm, use_jhash) != 0)
+	if (init_params(0, use_jhash) != 0)
 		goto err;
 
 	/*
@@ -608,12 +600,11 @@ test_hash_readwrite_main(void)
 {
 	/*
 	 * Variables used to choose different tests.
-	 * use_htm indicates if hardware transactional memory should be used.
 	 * reader_faster indicates if the reader threads should finish earlier
 	 * than writer threads. This is to timing either reader threads or
 	 * writer threads for performance numbers.
 	 */
-	int use_htm, use_ext,  reader_faster;
+	int use_ext,  reader_faster;
 	unsigned int i = 0, core_id = 0;
 
 	if (rte_lcore_count() <= 2) {
@@ -634,69 +625,40 @@ test_hash_readwrite_main(void)
 			"is supported\n");
 
 		printf("Test read-write with Hardware transactional memory\n");
-
-		use_htm = 1;
-		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(&htm_results, use_htm,
-							reader_faster) < 0)
-			return -1;
-
-		reader_faster = 0;
-		if (test_hash_readwrite_perf(&htm_results, use_htm,
-							reader_faster) < 0)
-			return -1;
 	} else {
 		printf("Hardware transactional memory (lock elision) "
 			"is NOT supported\n");
+
+		printf("Test read-write without Hardware transactional "
+			"memory\n");
 	}
 
-	printf("Test read-write without Hardware transactional memory\n");
-	use_htm = 0;
 	use_ext = 0;
-	if (test_hash_readwrite_functional(use_ext, use_htm) < 0)
+	if (test_hash_readwrite_functional(use_ext) < 0)
 		return -1;
 
 	use_ext = 1;
-	if (test_hash_readwrite_functional(use_ext, use_htm) < 0)
+	if (test_hash_readwrite_functional(use_ext) < 0)
 		return -1;
 
 	reader_faster = 1;
-	if (test_hash_readwrite_perf(&non_htm_results, use_htm,
-							reader_faster) < 0)
+	if (test_hash_readwrite_perf(&results, reader_faster) < 0)
 		return -1;
+
 	reader_faster = 0;
-	if (test_hash_readwrite_perf(&non_htm_results, use_htm,
-							reader_faster) < 0)
+	if (test_hash_readwrite_perf(&results, reader_faster) < 0)
 		return -1;
 
 	printf("Results summary:\n");
 
-	printf("single read: %u\n", htm_results.single_read);
-	printf("single write: %u\n", htm_results.single_write);
+	printf("single read: %u\n", results.single_read);
+	printf("single write: %u\n", results.single_write);
 	for (i = 0; i < NUM_TEST; i++) {
 		printf("core_cnt: %u\n", core_cnt[i]);
-		printf("HTM:\n");
-		printf("read only: %u\n", htm_results.read_only[i]);
-		printf("write only: %u\n", htm_results.write_only[i]);
-		printf("read-write read: %u\n", htm_results.read_write_r[i]);
-		printf("read-write write: %u\n", htm_results.read_write_w[i]);
-
-		printf("non HTM:\n");
-		printf("read only: %u\n", non_htm_results.read_only[i]);
-		printf("write only: %u\n", non_htm_results.write_only[i]);
-		printf("read-write read: %u\n",
-			non_htm_results.read_write_r[i]);
-		printf("read-write write: %u\n",
-			non_htm_results.read_write_w[i]);
+		printf("read only: %u\n", results.read_only[i]);
+		printf("write only: %u\n", results.write_only[i]);
+		printf("read-write read: %u\n", results.read_write_r[i]);
+		printf("read-write write: %u\n", results.read_write_w[i]);
 	}
 
 	return 0;
diff --git a/test/test/test_hash_readwrite_lf.c b/test/test/test_hash_readwrite_lf.c
index cbfd93226..9d459aeb8 100644
--- a/test/test/test_hash_readwrite_lf.c
+++ b/test/test/test_hash_readwrite_lf.c
@@ -22,20 +22,9 @@
 
 #define BULK_LOOKUP_SIZE 32
 
-#define RUN_WITH_HTM_DISABLED 0
-
-#if (RUN_WITH_HTM_DISABLED)
-
-#define TOTAL_ENTRY (5*1024)
-#define TOTAL_INSERT (5*1024)
-
-#else
-
 #define TOTAL_ENTRY (4*1024*1024)
 #define TOTAL_INSERT (4*1024*1024)
 
-#endif
-
 #define READ_FAIL 1
 #define READ_PASS_NO_KEY_SHIFTS 2
 #define READ_PASS_SHIFT_PATH 4
@@ -53,7 +42,7 @@ struct rwc_perf {
 	uint32_t multi_rw[NUM_TEST - 1][2][NUM_TEST];
 };
 
-static struct rwc_perf rwc_lf_results, rwc_non_lf_results;
+static struct rwc_perf rwc_lf_results;
 
 struct {
 	uint32_t *keys;
@@ -395,7 +384,7 @@ generate_keys(void)
 }
 
 static int
-init_params(int rwc_lf, int use_jhash, int htm)
+init_params(int use_jhash)
 {
 	struct rte_hash *handle;
 
@@ -411,19 +400,8 @@ init_params(int rwc_lf, int use_jhash, int htm)
 	else
 		hash_params.hash_func = rte_hash_crc;
 
-	if (rwc_lf)
-		hash_params.extra_flag =
-			RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF |
-			RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD;
-	else if (htm)
-		hash_params.extra_flag =
-			RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT |
-			RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY |
-			RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD;
-	else
-		hash_params.extra_flag =
-			RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY |
-			RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD;
+	hash_params.extra_flag = RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD |
+					RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF;
 
 	hash_params.name = "tests";
 
@@ -580,8 +558,7 @@ test_rwc_multi_writer(__attribute__((unused)) void *arg)
  * Reader(s) lookup keys present in the table.
  */
 static int
-test_hash_add_no_ks_lookup_hit(struct rwc_perf *rwc_perf_results, int rwc_lf,
-				int htm)
+test_hash_add_no_ks_lookup_hit(struct rwc_perf *rwc_perf_results)
 {
 	unsigned int n, m;
 	uint64_t i;
@@ -592,7 +569,7 @@ test_hash_add_no_ks_lookup_hit(struct rwc_perf *rwc_perf_results, int rwc_lf,
 	rte_atomic64_init(&greads);
 	rte_atomic64_init(&gread_cycles);
 
-	if (init_params(rwc_lf, use_jhash, htm) != 0)
+	if (init_params(use_jhash) != 0)
 		goto err;
 	printf("\nTest: Hash add - no key-shifts, read - hit\n");
 	for (m = 0; m < 2; m++) {
@@ -649,8 +626,7 @@ test_hash_add_no_ks_lookup_hit(struct rwc_perf *rwc_perf_results, int rwc_lf,
  * 'Main' thread adds with no key-shifts.
  */
 static int
-test_hash_add_no_ks_lookup_miss(struct rwc_perf *rwc_perf_results, int rwc_lf,
-				int htm)
+test_hash_add_no_ks_lookup_miss(struct rwc_perf *rwc_perf_results)
 {
 	unsigned int n, m;
 	uint64_t i;
@@ -662,7 +638,7 @@ test_hash_add_no_ks_lookup_miss(struct rwc_perf *rwc_perf_results, int rwc_lf,
 	rte_atomic64_init(&greads);
 	rte_atomic64_init(&gread_cycles);
 
-	if (init_params(rwc_lf, use_jhash, htm) != 0)
+	if (init_params(use_jhash) != 0)
 		goto err;
 	printf("\nTest: Hash add - no key-shifts, Hash lookup - miss\n");
 	for (m = 0; m < 2; m++) {
@@ -721,8 +697,7 @@ test_hash_add_no_ks_lookup_miss(struct rwc_perf *rwc_perf_results, int rwc_lf,
  * shift path  while 'Main' thread adds keys causing key-shifts.
  */
 static int
-test_hash_add_ks_lookup_hit_non_sp(struct rwc_perf *rwc_perf_results,
-				    int rwc_lf, int htm)
+test_hash_add_ks_lookup_hit_non_sp(struct rwc_perf *rwc_perf_results)
 {
 	unsigned int n, m;
 	uint64_t i;
@@ -734,7 +709,7 @@ test_hash_add_ks_lookup_hit_non_sp(struct rwc_perf *rwc_perf_results,
 	rte_atomic64_init(&greads);
 	rte_atomic64_init(&gread_cycles);
 
-	if (init_params(rwc_lf, use_jhash, htm) != 0)
+	if (init_params(use_jhash) != 0)
 		goto err;
 	printf("\nTest: Hash add - key shift, Hash lookup - hit"
 	       " (non-shift-path)\n");
@@ -797,8 +772,7 @@ test_hash_add_ks_lookup_hit_non_sp(struct rwc_perf *rwc_perf_results,
  * 'Main' thread adds keys causing key-shifts.
  */
 static int
-test_hash_add_ks_lookup_hit_sp(struct rwc_perf *rwc_perf_results, int rwc_lf,
-				int htm)
+test_hash_add_ks_lookup_hit_sp(struct rwc_perf *rwc_perf_results)
 {
 	unsigned int n, m;
 	uint64_t i;
@@ -810,7 +784,7 @@ test_hash_add_ks_lookup_hit_sp(struct rwc_perf *rwc_perf_results, int rwc_lf,
 	rte_atomic64_init(&greads);
 	rte_atomic64_init(&gread_cycles);
 
-	if (init_params(rwc_lf, use_jhash, htm) != 0)
+	if (init_params(use_jhash) != 0)
 		goto err;
 	printf("\nTest: Hash add - key shift, Hash lookup - hit (shift-path)"
 	       "\n");
@@ -873,8 +847,7 @@ test_hash_add_ks_lookup_hit_sp(struct rwc_perf *rwc_perf_results, int rwc_lf,
  * 'Main' thread adds keys causing key-shifts.
  */
 static int
-test_hash_add_ks_lookup_miss(struct rwc_perf *rwc_perf_results, int rwc_lf, int
-			     htm)
+test_hash_add_ks_lookup_miss(struct rwc_perf *rwc_perf_results)
 {
 	unsigned int n, m;
 	uint64_t i;
@@ -886,7 +859,7 @@ test_hash_add_ks_lookup_miss(struct rwc_perf *rwc_perf_results, int rwc_lf, int
 	rte_atomic64_init(&greads);
 	rte_atomic64_init(&gread_cycles);
 
-	if (init_params(rwc_lf, use_jhash, htm) != 0)
+	if (init_params(use_jhash) != 0)
 		goto err;
 	printf("\nTest: Hash add - key shift, Hash lookup - miss\n");
 	for (m = 0; m < 2; m++) {
@@ -948,8 +921,7 @@ test_hash_add_ks_lookup_miss(struct rwc_perf *rwc_perf_results, int rwc_lf, int
  * Writers are running in parallel, on different data plane cores.
  */
 static int
-test_hash_multi_add_lookup(struct rwc_perf *rwc_perf_results, int rwc_lf,
-			   int htm)
+test_hash_multi_add_lookup(struct rwc_perf *rwc_perf_results)
 {
 	unsigned int n, m, k;
 	uint64_t i;
@@ -960,7 +932,7 @@ test_hash_multi_add_lookup(struct rwc_perf *rwc_perf_results, int rwc_lf,
 	rte_atomic64_init(&greads);
 	rte_atomic64_init(&gread_cycles);
 
-	if (init_params(rwc_lf, use_jhash, htm) != 0)
+	if (init_params(use_jhash) != 0)
 		goto err;
 	printf("\nTest: Multi-add-lookup\n");
 	uint8_t pos_core;
@@ -1048,14 +1020,6 @@ test_hash_multi_add_lookup(struct rwc_perf *rwc_perf_results, int rwc_lf,
 static int
 test_hash_readwrite_lf_main(void)
 {
-	/*
-	 * Variables used to choose different tests.
-	 * rwc_lf indicates if read-write concurrency lock-free support is
-	 * enabled.
-	 * htm indicates if Hardware transactional memory support is enabled.
-	 */
-	int rwc_lf = 0;
-	int htm;
 	int use_jhash = 0;
 	if (rte_lcore_count() == 1) {
 		printf("More than one lcore is required "
@@ -1065,12 +1029,7 @@ test_hash_readwrite_lf_main(void)
 
 	setlocale(LC_NUMERIC, "");
 
-	if (rte_tm_supported())
-		htm = 1;
-	else
-		htm = 0;
-
-	if (init_params(rwc_lf, use_jhash, htm) != 0)
+	if (init_params(use_jhash) != 0)
 		return -1;
 	if (generate_keys() != 0)
 		return -1;
@@ -1078,133 +1037,65 @@ test_hash_readwrite_lf_main(void)
 		return -1;
 
 	if (RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF) {
-		rwc_lf = 1;
-		printf("Test lookup with read-write concurrency lock free support"
-		       " enabled\n");
-		if (test_hash_add_no_ks_lookup_hit(&rwc_lf_results, rwc_lf,
-							htm) < 0)
+		printf("Test lookup with read-write concurrency lock free "
+			"support enabled\n");
+		if (test_hash_add_no_ks_lookup_hit(&rwc_lf_results) < 0)
 			return -1;
-		if (test_hash_add_no_ks_lookup_miss(&rwc_lf_results, rwc_lf,
-							htm) < 0)
+		if (test_hash_add_no_ks_lookup_miss(&rwc_lf_results) < 0)
 			return -1;
-		if (test_hash_add_ks_lookup_hit_non_sp(&rwc_lf_results, rwc_lf,
-							htm) < 0)
+		if (test_hash_add_ks_lookup_hit_non_sp(&rwc_lf_results) < 0)
 			return -1;
-		if (test_hash_add_ks_lookup_hit_sp(&rwc_lf_results, rwc_lf,
-							htm) < 0)
+		if (test_hash_add_ks_lookup_hit_sp(&rwc_lf_results) < 0)
 			return -1;
-		if (test_hash_add_ks_lookup_miss(&rwc_lf_results, rwc_lf, htm)
-							< 0)
+		if (test_hash_add_ks_lookup_miss(&rwc_lf_results) < 0)
 			return -1;
-		if (test_hash_multi_add_lookup(&rwc_lf_results, rwc_lf, htm)
-							< 0)
+		if (test_hash_multi_add_lookup(&rwc_lf_results) < 0)
 			return -1;
 	}
-	printf("\nTest lookup with read-write concurrency lock free support"
-	       " disabled\n");
-	rwc_lf = 0;
-	if (!htm) {
-		printf("With HTM Disabled\n");
-		if (!RUN_WITH_HTM_DISABLED) {
-			printf("Enable RUN_WITH_HTM_DISABLED to test with"
-			       " lock-free disabled");
-			goto results;
-		}
-	} else
-		printf("With HTM Enabled\n");
-	if (test_hash_add_no_ks_lookup_hit(&rwc_non_lf_results, rwc_lf, htm)
-						< 0)
-		return -1;
-	if (test_hash_add_no_ks_lookup_miss(&rwc_non_lf_results, rwc_lf, htm)
-						< 0)
-		return -1;
-	if (test_hash_add_ks_lookup_hit_non_sp(&rwc_non_lf_results, rwc_lf,
-						htm) < 0)
-		return -1;
-	if (test_hash_add_ks_lookup_hit_sp(&rwc_non_lf_results, rwc_lf, htm)
-						< 0)
-		return -1;
-	if (test_hash_add_ks_lookup_miss(&rwc_non_lf_results, rwc_lf, htm) < 0)
-		return -1;
-	if (test_hash_multi_add_lookup(&rwc_non_lf_results, rwc_lf, htm) < 0)
-		return -1;
-results:
+
 	printf("\n\t\t\t\t\t\t********** Results summary **********\n\n");
 	int i, j, k;
 	for (j = 0; j < 2; j++) {
 		if (j == 1)
 			printf("\n\t\t\t\t\t#######********** Bulk Lookup "
 			       "**********#######\n\n");
-		printf("_______\t\t_______\t\t_________\t___\t\t_________\t\t"
-			"\t\t\t\t_________________\n");
-		printf("Writers\t\tReaders\t\tLock-free\tHTM\t\tTest-case\t\t\t"
+		printf("_______\t\t_______\t\t_________\t\t\t"
+			"\t\t\t_________________\n");
+		printf("Writers\t\tReaders\t\tTest-case\t\t\t"
 		       "\t\t\tCycles per lookup\n");
-		printf("_______\t\t_______\t\t_________\t___\t\t_________\t\t\t"
+		printf("_______\t\t_______\t\t_________\t\t\t"
 		       "\t\t\t_________________\n");
 		for (i = 0; i < NUM_TEST; i++) {
 			printf("%u\t\t%u\t\t", 1, rwc_core_cnt[i]);
-			printf("Enabled\t\t");
-			printf("N/A\t\t");
 			printf("Hash add - no key-shifts, lookup - hit\t\t\t\t"
-				"%u\n\t\t\t\t\t\t\t\t",
+				"%u\n\t\t\t\t",
 				rwc_lf_results.w_no_ks_r_hit[j][i]);
 			printf("Hash add - no key-shifts, lookup - miss\t\t\t\t"
-				"%u\n\t\t\t\t\t\t\t\t",
+				"%u\n\t\t\t\t",
 				rwc_lf_results.w_no_ks_r_miss[j][i]);
 			printf("Hash add - key-shifts, lookup - hit"
-			       "(non-shift-path)\t\t%u\n\t\t\t\t\t\t\t\t",
+			       "(non-shift-path)\t\t%u\n\t\t\t\t",
 			       rwc_lf_results.w_ks_r_hit_nsp[j][i]);
 			printf("Hash add - key-shifts, lookup - hit "
-			       "(shift-path)\t\t%u\n\t\t\t\t\t\t\t\t",
+			       "(shift-path)\t\t%u\n\t\t\t\t",
 			       rwc_lf_results.w_ks_r_hit_sp[j][i]);
 			printf("Hash add - key-shifts, Hash lookup miss\t\t\t\t"
-				"%u\n\n\t\t\t\t",
+				"%u\n\n",
 				rwc_lf_results.w_ks_r_miss[j][i]);
 
-			printf("Disabled\t");
-			if (htm)
-				printf("Enabled\t\t");
-			else
-				printf("Disabled\t");
-			printf("Hash add - no key-shifts, lookup - hit\t\t\t\t"
-				"%u\n\t\t\t\t\t\t\t\t",
-				rwc_non_lf_results.w_no_ks_r_hit[j][i]);
-			printf("Hash add - no key-shifts, lookup - miss\t\t\t\t"
-				"%u\n\t\t\t\t\t\t\t\t",
-				rwc_non_lf_results.w_no_ks_r_miss[j][i]);
-			printf("Hash add - key-shifts, lookup - hit "
-			       "(non-shift-path)\t\t%u\n\t\t\t\t\t\t\t\t",
-			       rwc_non_lf_results.w_ks_r_hit_nsp[j][i]);
-			printf("Hash add - key-shifts, lookup - hit "
-			       "(shift-path)\t\t%u\n\t\t\t\t\t\t\t\t",
-			       rwc_non_lf_results.w_ks_r_hit_sp[j][i]);
-			printf("Hash add - key-shifts, Hash lookup miss\t\t\t\t"
-			       "%u\n", rwc_non_lf_results.w_ks_r_miss[j][i]);
-
-			printf("_______\t\t_______\t\t_________\t___\t\t"
-			       "_________\t\t\t\t\t\t_________________\n");
+			printf("_______\t\t_______\t\t_________\t\t\t"
+			       "\t\t\t_________________\n");
 		}
 
 		for (i = 1; i < NUM_TEST; i++) {
 			for (k = 0; k < NUM_TEST; k++) {
 				printf("%u", rwc_core_cnt[i]);
 				printf("\t\t%u\t\t", rwc_core_cnt[k]);
-				printf("Enabled\t\t");
-				printf("N/A\t\t");
-				printf("Multi-add-lookup\t\t\t\t\t\t%u\n\n\t\t"
-				       "\t\t",
+				printf("Multi-add-lookup\t\t\t\t\t\t%u\n\n",
 				       rwc_lf_results.multi_rw[i][j][k]);
-				printf("Disabled\t");
-				if (htm)
-					printf("Enabled\t\t");
-				else
-					printf("Disabled\t");
-				printf("Multi-add-lookup\t\t\t\t\t\t%u\n",
-				       rwc_non_lf_results.multi_rw[i][j][k]);
 
-				printf("_______\t\t_______\t\t_________\t___"
-				       "\t\t_________\t\t\t\t\t\t"
-				       "_________________\n");
+				printf("_______\t\t_______\t\t_________\t\t\t"
+				       "\t\t\t_________________\n");
 			}
 		}
 	}
-- 
2.17.1

[dpdk-dev] [PATCH v2 4/4] doc/hash: deprecate lock ellision and read/write concurreny flags

[Honnappa Nagarahalli]

RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY and
RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT configuration flags are
deprecated. Reader/Writer concurrency is provided by default.
Transactional memory will be used if the platform supports it.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
---
 doc/guides/rel_notes/deprecation.rst | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 34b28234c..d34cca260 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -55,3 +55,8 @@ Deprecation Notices
   - ``rte_pdump_set_socket_dir`` will be removed;
   - The parameter, ``path``, of ``rte_pdump_init`` will be removed;
   - The enum ``rte_pdump_socktype`` will be removed.
+
+* hash: The configuration flags RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY and
+  RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT will be removed in v19.02.
+  Reader/writer concurrency will be supported by default. The library will
+  use transactional memory if the platform supports it.
-- 
2.17.1

Re: [dpdk-dev] [PATCH v2 4/4] doc/hash: deprecate lock ellision and read/write concurreny flags

[Bruce Richardson]

On Thu, Nov 01, 2018 at 06:25:22PM -0500, Honnappa Nagarahalli wrote:
> RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY and
> RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT configuration flags are
> deprecated. Reader/Writer concurrency is provided by default.
> Transactional memory will be used if the platform supports it.
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> ---
>  doc/guides/rel_notes/deprecation.rst | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 34b28234c..d34cca260 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -55,3 +55,8 @@ Deprecation Notices
>    - ``rte_pdump_set_socket_dir`` will be removed;
>    - The parameter, ``path``, of ``rte_pdump_init`` will be removed;
>    - The enum ``rte_pdump_socktype`` will be removed.
> +
> +* hash: The configuration flags RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY and
> +  RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT will be removed in v19.02.
> +  Reader/writer concurrency will be supported by default. The library will
> +  use transactional memory if the platform supports it.
> -- 
I think we need more documentation than this. The programmers guide also
needs an update (probably in patch 2) to record the changes in the flags
used on hash create.

/Bruce

Re: [dpdk-dev] [PATCH v2 0/4] hash: deprecate lock ellision and read/write concurreny flags

[Bruce Richardson]

On Thu, Nov 01, 2018 at 06:25:18PM -0500, Honnappa Nagarahalli wrote:
> Various configuration flags in rte_hash library result in increase of
> number of test cases. Configuration flags for enabling transactional
> memory use and read/write concurrency are not required. These features
> should be supported by default. Please refer to [1] for more context.
> 
> This patch marks these flags for deprecation in 19.02 release and cleans
> up the test cases.
> 
> [1] http://mails.dpdk.org/archives/dev/2018-October/117268.html
> 
> Honnappa Nagarahalli (4): hash: prepare for deprecation of flags hash:
> deprecate lock ellision and read/write concurreny flags test/hash: stop
> using lock ellision and read/write concurreny flags doc/hash: deprecate
> lock ellision and read/write concurreny flags
> 
While I'd like to reduce the flags and do cleanup, I'm a little concerned
about putting this scope of changes in so late in the release. I wonder if
less drastic changes could work as well for this release, and do the
cleanup later.
For example, rather than deprecating the flags now, how about just change
the default for when no flags are set? If user has set flags, follow the
existing path - if flags is set to zero, then have the defaults be to use
RW concurrency or TSX.

/Bruce

Re: [dpdk-dev] [PATCH v2 0/4] hash: deprecate lock ellision and read/write concurreny flags

[Honnappa Nagarahalli]

> 
> On Thu, Nov 01, 2018 at 06:25:18PM -0500, Honnappa Nagarahalli wrote:
> > Various configuration flags in rte_hash library result in increase of
> > number of test cases. Configuration flags for enabling transactional
> > memory use and read/write concurrency are not required. These features
> > should be supported by default. Please refer to [1] for more context.
> >
> > This patch marks these flags for deprecation in 19.02 release and
> > cleans up the test cases.
> >
> > [1] http://mails.dpdk.org/archives/dev/2018-October/117268.html
> >
> > Honnappa Nagarahalli (4): hash: prepare for deprecation of flags hash:
> > deprecate lock ellision and read/write concurreny flags test/hash:
> > stop using lock ellision and read/write concurreny flags doc/hash:
> > deprecate lock ellision and read/write concurreny flags
> >
> While I'd like to reduce the flags and do cleanup, I'm a little concerned about
> putting this scope of changes in so late in the release. I wonder if less drastic
> changes could work as well for this release, and do the cleanup later.
Thank you Bruce for the review. This patch series is not fixing any user related problems, let us skip this for 18.11. It will give us time as well to think through and get this right.

> For example, rather than deprecating the flags now, how about just change
> the default for when no flags are set? If user has set flags, follow the existing
> path - if flags is set to zero, then have the defaults be to use RW concurrency
> or TSX.
This changes the behavior of the library and what the flags mean, still requires ABI change, but does not need deprecation of flags (I guess this is what you meant). However, it will not solve the problem of losing the capability to disable TSX.

> 
> /Bruce

Re: [dpdk-dev] [PATCH v2 0/4] hash: deprecate lock ellision and read/write concurreny flags

[Yigit, Ferruh]

On 11/2/2018 5:38 PM, Honnappa Nagarahalli wrote:
>>
>> On Thu, Nov 01, 2018 at 06:25:18PM -0500, Honnappa Nagarahalli wrote:
>>> Various configuration flags in rte_hash library result in increase of
>>> number of test cases. Configuration flags for enabling transactional
>>> memory use and read/write concurrency are not required. These features
>>> should be supported by default. Please refer to [1] for more context.
>>>
>>> This patch marks these flags for deprecation in 19.02 release and
>>> cleans up the test cases.
>>>
>>> [1] http://mails.dpdk.org/archives/dev/2018-October/117268.html
>>>
>>> Honnappa Nagarahalli (4): hash: prepare for deprecation of flags hash:
>>> deprecate lock ellision and read/write concurreny flags test/hash:
>>> stop using lock ellision and read/write concurreny flags doc/hash:
>>> deprecate lock ellision and read/write concurreny flags
>>>
>> While I'd like to reduce the flags and do cleanup, I'm a little concerned about
>> putting this scope of changes in so late in the release. I wonder if less drastic
>> changes could work as well for this release, and do the cleanup later.
> Thank you Bruce for the review. This patch series is not fixing any user related problems, let us skip this for 18.11. It will give us time as well to think through and get this right.

There was no update in the scope of 19.02, I guess it will skip for 19.02 too.
Patchset updated as "Change requested" in patchwork.

> 
>> For example, rather than deprecating the flags now, how about just change
>> the default for when no flags are set? If user has set flags, follow the existing
>> path - if flags is set to zero, then have the defaults be to use RW concurrency
>> or TSX.
> This changes the behavior of the library and what the flags mean, still requires ABI change, but does not need deprecation of flags (I guess this is what you meant). However, it will not solve the problem of losing the capability to disable TSX.
> 
>>
>> /Bruce

[dpdk-dev] [PATCH 2/3] test/hash: stop using lock ellision and read/write concurreny flags

[Honnappa Nagarahalli]

With the deprecation of RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY and
RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT flags, the test cases
can be simplified. This results in shorter run times.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
---
 lib/librte_hash/rte_hash.h         |   2 -
 test/test/test_hash_multiwriter.c  |  20 +--
 test/test/test_hash_perf.c         |  40 +++---
 test/test/test_hash_readwrite.c    |  84 ++++---------
 test/test/test_hash_readwrite_lf.c | 189 ++++++-----------------------
 5 files changed, 82 insertions(+), 253 deletions(-)

diff --git a/lib/librte_hash/rte_hash.h b/lib/librte_hash/rte_hash.h
index 93c7019ec..e7e3397d5 100644
--- a/lib/librte_hash/rte_hash.h
+++ b/lib/librte_hash/rte_hash.h
@@ -32,7 +32,6 @@ extern "C" {
 
 /**
  * @deprecated
- * This define will be removed in the next release.
  * If the target platform supports hardware transactional memory
  * it will be used without user consent as it provides the best possible
  * performance.
@@ -46,7 +45,6 @@ extern "C" {
 
 /**
  * @deprecated
- * This define will be removed in the next release.
  * This library should be thread-safe by default.
  *
  * Flag to support reader writer concurrency
diff --git a/test/test/test_hash_multiwriter.c b/test/test/test_hash_multiwriter.c
index d447f6dca..5469fb705 100644
--- a/test/test/test_hash_multiwriter.c
+++ b/test/test/test_hash_multiwriter.c
@@ -46,8 +46,6 @@ uint32_t rounded_nb_total_tsx_insertion;
 static rte_atomic64_t gcycles;
 static rte_atomic64_t ginsertions;
 
-static int use_htm;
-
 static int
 test_hash_multiwriter_worker(void *arg)
 {
@@ -113,13 +111,9 @@ test_hash_multiwriter(void)
 		.hash_func_init_val = 0,
 		.socket_id = rte_socket_id(),
 	};
-	if (use_htm)
-		hash_params.extra_flag =
-			RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT
-				| RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD;
-	else
-		hash_params.extra_flag =
-			RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD;
+
+	hash_params.extra_flag =
+		RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD;
 
 	struct rte_hash *handle;
 	char name[RTE_HASH_NAMESIZE];
@@ -272,19 +266,15 @@ test_hash_multiwriter_main(void)
 	if (!rte_tm_supported()) {
 		printf("Hardware transactional memory (lock elision) "
 			"is NOT supported\n");
+		printf("Test multi-writer without Hardware transactional "
+			"memory\n");
 	} else {
 		printf("Hardware transactional memory (lock elision) "
 			"is supported\n");
 
 		printf("Test multi-writer with Hardware transactional memory\n");
-
-		use_htm = 1;
-		if (test_hash_multiwriter() < 0)
-			return -1;
 	}
 
-	printf("Test multi-writer without Hardware transactional memory\n");
-	use_htm = 0;
 	if (test_hash_multiwriter() < 0)
 		return -1;
 
diff --git a/test/test/test_hash_perf.c b/test/test/test_hash_perf.c
index 525211180..03facd22a 100644
--- a/test/test/test_hash_perf.c
+++ b/test/test/test_hash_perf.c
@@ -79,7 +79,7 @@ static struct rte_hash_parameters ut_params = {
 
 static int
 create_table(unsigned int with_data, unsigned int table_index,
-		unsigned int with_locks, unsigned int ext)
+		unsigned int ext)
 {
 	char name[RTE_HASH_NAMESIZE];
 
@@ -89,17 +89,11 @@ create_table(unsigned int with_data, unsigned int table_index,
 	else
 		sprintf(name, "test_hash%d", hashtest_key_lens[table_index]);
 
-
-	if (with_locks)
-		ut_params.extra_flag =
-			RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT
-				| RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY;
+	if (ext)
+		ut_params.extra_flag = RTE_HASH_EXTRA_FLAGS_EXT_TABLE;
 	else
 		ut_params.extra_flag = 0;
 
-	if (ext)
-		ut_params.extra_flag |= RTE_HASH_EXTRA_FLAGS_EXT_TABLE;
-
 	ut_params.name = name;
 	ut_params.key_len = hashtest_key_lens[table_index];
 	ut_params.socket_id = rte_socket_id();
@@ -516,8 +510,7 @@ reset_table(unsigned table_index)
 }
 
 static int
-run_all_tbl_perf_tests(unsigned int with_pushes, unsigned int with_locks,
-						unsigned int ext)
+run_all_tbl_perf_tests(unsigned int with_pushes, unsigned int ext)
 {
 	unsigned i, j, with_data, with_hash;
 
@@ -526,7 +519,7 @@ run_all_tbl_perf_tests(unsigned int with_pushes, unsigned int with_locks,
 
 	for (with_data = 0; with_data <= 1; with_data++) {
 		for (i = 0; i < NUM_KEYSIZES; i++) {
-			if (create_table(with_data, i, with_locks, ext) < 0)
+			if (create_table(with_data, i, ext) < 0)
 				return -1;
 
 			if (get_input_keys(with_pushes, i, ext) < 0)
@@ -669,25 +662,20 @@ fbk_hash_perf_test(void)
 static int
 test_hash_perf(void)
 {
-	unsigned int with_pushes, with_locks;
-	for (with_locks = 0; with_locks <= 1; with_locks++) {
-		if (with_locks)
-			printf("\nWith locks in the code\n");
+	unsigned int with_pushes;
+
+	for (with_pushes = 0; with_pushes <= 1; with_pushes++) {
+		if (with_pushes == 0)
+			printf("\nALL ELEMENTS IN PRIMARY LOCATION\n");
 		else
-			printf("\nWithout locks in the code\n");
-		for (with_pushes = 0; with_pushes <= 1; with_pushes++) {
-			if (with_pushes == 0)
-				printf("\nALL ELEMENTS IN PRIMARY LOCATION\n");
-			else
-				printf("\nELEMENTS IN PRIMARY OR SECONDARY LOCATION\n");
-			if (run_all_tbl_perf_tests(with_pushes, with_locks, 0) < 0)
-				return -1;
-		}
+			printf("\nELEMENTS IN PRIMARY OR SECONDARY LOCATION\n");
+		if (run_all_tbl_perf_tests(with_pushes, 0) < 0)
+			return -1;
 	}
 
 	printf("\n EXTENDABLE BUCKETS PERFORMANCE\n");
 
-	if (run_all_tbl_perf_tests(1, 0, 1) < 0)
+	if (run_all_tbl_perf_tests(1, 1) < 0)
 		return -1;
 
 	if (fbk_hash_perf_test() < 0)
diff --git a/test/test/test_hash_readwrite.c b/test/test/test_hash_readwrite.c
index 01f986cf4..d389723aa 100644
--- a/test/test/test_hash_readwrite.c
+++ b/test/test/test_hash_readwrite.c
@@ -35,7 +35,7 @@ struct perf {
 	uint32_t read_write_w[NUM_TEST];
 };
 
-static struct perf htm_results, non_htm_results;
+static struct perf results;
 
 struct {
 	uint32_t *keys;
@@ -121,7 +121,7 @@ test_hash_readwrite_worker(__attribute__((unused)) void *arg)
 }
 
 static int
-init_params(int use_ext, int use_htm, int use_jhash)
+init_params(int use_ext, int use_jhash)
 {
 	unsigned int i;
 
@@ -140,15 +140,8 @@ init_params(int use_ext, int use_htm, int use_jhash)
 	else
 		hash_params.hash_func = rte_hash_crc;
 
-	if (use_htm)
-		hash_params.extra_flag =
-			RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT |
-			RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY |
-			RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD;
-	else
-		hash_params.extra_flag =
-			RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY |
-			RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD;
+	hash_params.extra_flag =
+		RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD;
 
 	if (use_ext)
 		hash_params.extra_flag |=
@@ -195,7 +188,7 @@ init_params(int use_ext, int use_htm, int use_jhash)
 }
 
 static int
-test_hash_readwrite_functional(int use_ext, int use_htm)
+test_hash_readwrite_functional(int use_ext)
 {
 	unsigned int i;
 	const void *next_key;
@@ -214,7 +207,7 @@ test_hash_readwrite_functional(int use_ext, int use_htm)
 	rte_atomic64_init(&ginsertions);
 	rte_atomic64_clear(&ginsertions);
 
-	if (init_params(use_ext, use_htm, use_jhash) != 0)
+	if (init_params(use_ext, use_jhash) != 0)
 		goto err;
 
 	if (use_ext)
@@ -351,8 +344,7 @@ test_rw_writer(void *arg)
 }
 
 static int
-test_hash_readwrite_perf(struct perf *perf_results, int use_htm,
-							int reader_faster)
+test_hash_readwrite_perf(struct perf *perf_results, int reader_faster)
 {
 	unsigned int n;
 	int ret;
@@ -379,7 +371,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(0, use_htm, use_jhash) != 0)
+	if (init_params(0, use_jhash) != 0)
 		goto err;
 
 	/*
@@ -608,12 +600,11 @@ test_hash_readwrite_main(void)
 {
 	/*
 	 * Variables used to choose different tests.
-	 * use_htm indicates if hardware transactional memory should be used.
 	 * reader_faster indicates if the reader threads should finish earlier
 	 * than writer threads. This is to timing either reader threads or
 	 * writer threads for performance numbers.
 	 */
-	int use_htm, use_ext,  reader_faster;
+	int use_ext,  reader_faster;
 	unsigned int i = 0, core_id = 0;
 
 	if (rte_lcore_count() <= 2) {
@@ -634,69 +625,40 @@ test_hash_readwrite_main(void)
 			"is supported\n");
 
 		printf("Test read-write with Hardware transactional memory\n");
-
-		use_htm = 1;
-		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(&htm_results, use_htm,
-							reader_faster) < 0)
-			return -1;
-
-		reader_faster = 0;
-		if (test_hash_readwrite_perf(&htm_results, use_htm,
-							reader_faster) < 0)
-			return -1;
 	} else {
 		printf("Hardware transactional memory (lock elision) "
 			"is NOT supported\n");
+
+		printf("Test read-write without Hardware transactional "
+			"memory\n");
 	}
 
-	printf("Test read-write without Hardware transactional memory\n");
-	use_htm = 0;
 	use_ext = 0;
-	if (test_hash_readwrite_functional(use_ext, use_htm) < 0)
+	if (test_hash_readwrite_functional(use_ext) < 0)
 		return -1;
 
 	use_ext = 1;
-	if (test_hash_readwrite_functional(use_ext, use_htm) < 0)
+	if (test_hash_readwrite_functional(use_ext) < 0)
 		return -1;
 
 	reader_faster = 1;
-	if (test_hash_readwrite_perf(&non_htm_results, use_htm,
-							reader_faster) < 0)
+	if (test_hash_readwrite_perf(&results, reader_faster) < 0)
 		return -1;
+
 	reader_faster = 0;
-	if (test_hash_readwrite_perf(&non_htm_results, use_htm,
-							reader_faster) < 0)
+	if (test_hash_readwrite_perf(&results, reader_faster) < 0)
 		return -1;
 
 	printf("Results summary:\n");
 
-	printf("single read: %u\n", htm_results.single_read);
-	printf("single write: %u\n", htm_results.single_write);
+	printf("single read: %u\n", results.single_read);
+	printf("single write: %u\n", results.single_write);
 	for (i = 0; i < NUM_TEST; i++) {
 		printf("core_cnt: %u\n", core_cnt[i]);
-		printf("HTM:\n");
-		printf("read only: %u\n", htm_results.read_only[i]);
-		printf("write only: %u\n", htm_results.write_only[i]);
-		printf("read-write read: %u\n", htm_results.read_write_r[i]);
-		printf("read-write write: %u\n", htm_results.read_write_w[i]);
-
-		printf("non HTM:\n");
-		printf("read only: %u\n", non_htm_results.read_only[i]);
-		printf("write only: %u\n", non_htm_results.write_only[i]);
-		printf("read-write read: %u\n",
-			non_htm_results.read_write_r[i]);
-		printf("read-write write: %u\n",
-			non_htm_results.read_write_w[i]);
+		printf("read only: %u\n", results.read_only[i]);
+		printf("write only: %u\n", results.write_only[i]);
+		printf("read-write read: %u\n", results.read_write_r[i]);
+		printf("read-write write: %u\n", results.read_write_w[i]);
 	}
 
 	return 0;
diff --git a/test/test/test_hash_readwrite_lf.c b/test/test/test_hash_readwrite_lf.c
index cbfd93226..9d459aeb8 100644
--- a/test/test/test_hash_readwrite_lf.c
+++ b/test/test/test_hash_readwrite_lf.c
@@ -22,20 +22,9 @@
 
 #define BULK_LOOKUP_SIZE 32
 
-#define RUN_WITH_HTM_DISABLED 0
-
-#if (RUN_WITH_HTM_DISABLED)
-
-#define TOTAL_ENTRY (5*1024)
-#define TOTAL_INSERT (5*1024)
-
-#else
-
 #define TOTAL_ENTRY (4*1024*1024)
 #define TOTAL_INSERT (4*1024*1024)
 
-#endif
-
 #define READ_FAIL 1
 #define READ_PASS_NO_KEY_SHIFTS 2
 #define READ_PASS_SHIFT_PATH 4
@@ -53,7 +42,7 @@ struct rwc_perf {
 	uint32_t multi_rw[NUM_TEST - 1][2][NUM_TEST];
 };
 
-static struct rwc_perf rwc_lf_results, rwc_non_lf_results;
+static struct rwc_perf rwc_lf_results;
 
 struct {
 	uint32_t *keys;
@@ -395,7 +384,7 @@ generate_keys(void)
 }
 
 static int
-init_params(int rwc_lf, int use_jhash, int htm)
+init_params(int use_jhash)
 {
 	struct rte_hash *handle;
 
@@ -411,19 +400,8 @@ init_params(int rwc_lf, int use_jhash, int htm)
 	else
 		hash_params.hash_func = rte_hash_crc;
 
-	if (rwc_lf)
-		hash_params.extra_flag =
-			RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF |
-			RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD;
-	else if (htm)
-		hash_params.extra_flag =
-			RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT |
-			RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY |
-			RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD;
-	else
-		hash_params.extra_flag =
-			RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY |
-			RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD;
+	hash_params.extra_flag = RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD |
+					RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF;
 
 	hash_params.name = "tests";
 
@@ -580,8 +558,7 @@ test_rwc_multi_writer(__attribute__((unused)) void *arg)
  * Reader(s) lookup keys present in the table.
  */
 static int
-test_hash_add_no_ks_lookup_hit(struct rwc_perf *rwc_perf_results, int rwc_lf,
-				int htm)
+test_hash_add_no_ks_lookup_hit(struct rwc_perf *rwc_perf_results)
 {
 	unsigned int n, m;
 	uint64_t i;
@@ -592,7 +569,7 @@ test_hash_add_no_ks_lookup_hit(struct rwc_perf *rwc_perf_results, int rwc_lf,
 	rte_atomic64_init(&greads);
 	rte_atomic64_init(&gread_cycles);
 
-	if (init_params(rwc_lf, use_jhash, htm) != 0)
+	if (init_params(use_jhash) != 0)
 		goto err;
 	printf("\nTest: Hash add - no key-shifts, read - hit\n");
 	for (m = 0; m < 2; m++) {
@@ -649,8 +626,7 @@ test_hash_add_no_ks_lookup_hit(struct rwc_perf *rwc_perf_results, int rwc_lf,
  * 'Main' thread adds with no key-shifts.
  */
 static int
-test_hash_add_no_ks_lookup_miss(struct rwc_perf *rwc_perf_results, int rwc_lf,
-				int htm)
+test_hash_add_no_ks_lookup_miss(struct rwc_perf *rwc_perf_results)
 {
 	unsigned int n, m;
 	uint64_t i;
@@ -662,7 +638,7 @@ test_hash_add_no_ks_lookup_miss(struct rwc_perf *rwc_perf_results, int rwc_lf,
 	rte_atomic64_init(&greads);
 	rte_atomic64_init(&gread_cycles);
 
-	if (init_params(rwc_lf, use_jhash, htm) != 0)
+	if (init_params(use_jhash) != 0)
 		goto err;
 	printf("\nTest: Hash add - no key-shifts, Hash lookup - miss\n");
 	for (m = 0; m < 2; m++) {
@@ -721,8 +697,7 @@ test_hash_add_no_ks_lookup_miss(struct rwc_perf *rwc_perf_results, int rwc_lf,
  * shift path  while 'Main' thread adds keys causing key-shifts.
  */
 static int
-test_hash_add_ks_lookup_hit_non_sp(struct rwc_perf *rwc_perf_results,
-				    int rwc_lf, int htm)
+test_hash_add_ks_lookup_hit_non_sp(struct rwc_perf *rwc_perf_results)
 {
 	unsigned int n, m;
 	uint64_t i;
@@ -734,7 +709,7 @@ test_hash_add_ks_lookup_hit_non_sp(struct rwc_perf *rwc_perf_results,
 	rte_atomic64_init(&greads);
 	rte_atomic64_init(&gread_cycles);
 
-	if (init_params(rwc_lf, use_jhash, htm) != 0)
+	if (init_params(use_jhash) != 0)
 		goto err;
 	printf("\nTest: Hash add - key shift, Hash lookup - hit"
 	       " (non-shift-path)\n");
@@ -797,8 +772,7 @@ test_hash_add_ks_lookup_hit_non_sp(struct rwc_perf *rwc_perf_results,
  * 'Main' thread adds keys causing key-shifts.
  */
 static int
-test_hash_add_ks_lookup_hit_sp(struct rwc_perf *rwc_perf_results, int rwc_lf,
-				int htm)
+test_hash_add_ks_lookup_hit_sp(struct rwc_perf *rwc_perf_results)
 {
 	unsigned int n, m;
 	uint64_t i;
@@ -810,7 +784,7 @@ test_hash_add_ks_lookup_hit_sp(struct rwc_perf *rwc_perf_results, int rwc_lf,
 	rte_atomic64_init(&greads);
 	rte_atomic64_init(&gread_cycles);
 
-	if (init_params(rwc_lf, use_jhash, htm) != 0)
+	if (init_params(use_jhash) != 0)
 		goto err;
 	printf("\nTest: Hash add - key shift, Hash lookup - hit (shift-path)"
 	       "\n");
@@ -873,8 +847,7 @@ test_hash_add_ks_lookup_hit_sp(struct rwc_perf *rwc_perf_results, int rwc_lf,
  * 'Main' thread adds keys causing key-shifts.
  */
 static int
-test_hash_add_ks_lookup_miss(struct rwc_perf *rwc_perf_results, int rwc_lf, int
-			     htm)
+test_hash_add_ks_lookup_miss(struct rwc_perf *rwc_perf_results)
 {
 	unsigned int n, m;
 	uint64_t i;
@@ -886,7 +859,7 @@ test_hash_add_ks_lookup_miss(struct rwc_perf *rwc_perf_results, int rwc_lf, int
 	rte_atomic64_init(&greads);
 	rte_atomic64_init(&gread_cycles);
 
-	if (init_params(rwc_lf, use_jhash, htm) != 0)
+	if (init_params(use_jhash) != 0)
 		goto err;
 	printf("\nTest: Hash add - key shift, Hash lookup - miss\n");
 	for (m = 0; m < 2; m++) {
@@ -948,8 +921,7 @@ test_hash_add_ks_lookup_miss(struct rwc_perf *rwc_perf_results, int rwc_lf, int
  * Writers are running in parallel, on different data plane cores.
  */
 static int
-test_hash_multi_add_lookup(struct rwc_perf *rwc_perf_results, int rwc_lf,
-			   int htm)
+test_hash_multi_add_lookup(struct rwc_perf *rwc_perf_results)
 {
 	unsigned int n, m, k;
 	uint64_t i;
@@ -960,7 +932,7 @@ test_hash_multi_add_lookup(struct rwc_perf *rwc_perf_results, int rwc_lf,
 	rte_atomic64_init(&greads);
 	rte_atomic64_init(&gread_cycles);
 
-	if (init_params(rwc_lf, use_jhash, htm) != 0)
+	if (init_params(use_jhash) != 0)
 		goto err;
 	printf("\nTest: Multi-add-lookup\n");
 	uint8_t pos_core;
@@ -1048,14 +1020,6 @@ test_hash_multi_add_lookup(struct rwc_perf *rwc_perf_results, int rwc_lf,
 static int
 test_hash_readwrite_lf_main(void)
 {
-	/*
-	 * Variables used to choose different tests.
-	 * rwc_lf indicates if read-write concurrency lock-free support is
-	 * enabled.
-	 * htm indicates if Hardware transactional memory support is enabled.
-	 */
-	int rwc_lf = 0;
-	int htm;
 	int use_jhash = 0;
 	if (rte_lcore_count() == 1) {
 		printf("More than one lcore is required "
@@ -1065,12 +1029,7 @@ test_hash_readwrite_lf_main(void)
 
 	setlocale(LC_NUMERIC, "");
 
-	if (rte_tm_supported())
-		htm = 1;
-	else
-		htm = 0;
-
-	if (init_params(rwc_lf, use_jhash, htm) != 0)
+	if (init_params(use_jhash) != 0)
 		return -1;
 	if (generate_keys() != 0)
 		return -1;
@@ -1078,133 +1037,65 @@ test_hash_readwrite_lf_main(void)
 		return -1;
 
 	if (RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF) {
-		rwc_lf = 1;
-		printf("Test lookup with read-write concurrency lock free support"
-		       " enabled\n");
-		if (test_hash_add_no_ks_lookup_hit(&rwc_lf_results, rwc_lf,
-							htm) < 0)
+		printf("Test lookup with read-write concurrency lock free "
+			"support enabled\n");
+		if (test_hash_add_no_ks_lookup_hit(&rwc_lf_results) < 0)
 			return -1;
-		if (test_hash_add_no_ks_lookup_miss(&rwc_lf_results, rwc_lf,
-							htm) < 0)
+		if (test_hash_add_no_ks_lookup_miss(&rwc_lf_results) < 0)
 			return -1;
-		if (test_hash_add_ks_lookup_hit_non_sp(&rwc_lf_results, rwc_lf,
-							htm) < 0)
+		if (test_hash_add_ks_lookup_hit_non_sp(&rwc_lf_results) < 0)
 			return -1;
-		if (test_hash_add_ks_lookup_hit_sp(&rwc_lf_results, rwc_lf,
-							htm) < 0)
+		if (test_hash_add_ks_lookup_hit_sp(&rwc_lf_results) < 0)
 			return -1;
-		if (test_hash_add_ks_lookup_miss(&rwc_lf_results, rwc_lf, htm)
-							< 0)
+		if (test_hash_add_ks_lookup_miss(&rwc_lf_results) < 0)
 			return -1;
-		if (test_hash_multi_add_lookup(&rwc_lf_results, rwc_lf, htm)
-							< 0)
+		if (test_hash_multi_add_lookup(&rwc_lf_results) < 0)
 			return -1;
 	}
-	printf("\nTest lookup with read-write concurrency lock free support"
-	       " disabled\n");
-	rwc_lf = 0;
-	if (!htm) {
-		printf("With HTM Disabled\n");
-		if (!RUN_WITH_HTM_DISABLED) {
-			printf("Enable RUN_WITH_HTM_DISABLED to test with"
-			       " lock-free disabled");
-			goto results;
-		}
-	} else
-		printf("With HTM Enabled\n");
-	if (test_hash_add_no_ks_lookup_hit(&rwc_non_lf_results, rwc_lf, htm)
-						< 0)
-		return -1;
-	if (test_hash_add_no_ks_lookup_miss(&rwc_non_lf_results, rwc_lf, htm)
-						< 0)
-		return -1;
-	if (test_hash_add_ks_lookup_hit_non_sp(&rwc_non_lf_results, rwc_lf,
-						htm) < 0)
-		return -1;
-	if (test_hash_add_ks_lookup_hit_sp(&rwc_non_lf_results, rwc_lf, htm)
-						< 0)
-		return -1;
-	if (test_hash_add_ks_lookup_miss(&rwc_non_lf_results, rwc_lf, htm) < 0)
-		return -1;
-	if (test_hash_multi_add_lookup(&rwc_non_lf_results, rwc_lf, htm) < 0)
-		return -1;
-results:
+
 	printf("\n\t\t\t\t\t\t********** Results summary **********\n\n");
 	int i, j, k;
 	for (j = 0; j < 2; j++) {
 		if (j == 1)
 			printf("\n\t\t\t\t\t#######********** Bulk Lookup "
 			       "**********#######\n\n");
-		printf("_______\t\t_______\t\t_________\t___\t\t_________\t\t"
-			"\t\t\t\t_________________\n");
-		printf("Writers\t\tReaders\t\tLock-free\tHTM\t\tTest-case\t\t\t"
+		printf("_______\t\t_______\t\t_________\t\t\t"
+			"\t\t\t_________________\n");
+		printf("Writers\t\tReaders\t\tTest-case\t\t\t"
 		       "\t\t\tCycles per lookup\n");
-		printf("_______\t\t_______\t\t_________\t___\t\t_________\t\t\t"
+		printf("_______\t\t_______\t\t_________\t\t\t"
 		       "\t\t\t_________________\n");
 		for (i = 0; i < NUM_TEST; i++) {
 			printf("%u\t\t%u\t\t", 1, rwc_core_cnt[i]);
-			printf("Enabled\t\t");
-			printf("N/A\t\t");
 			printf("Hash add - no key-shifts, lookup - hit\t\t\t\t"
-				"%u\n\t\t\t\t\t\t\t\t",
+				"%u\n\t\t\t\t",
 				rwc_lf_results.w_no_ks_r_hit[j][i]);
 			printf("Hash add - no key-shifts, lookup - miss\t\t\t\t"
-				"%u\n\t\t\t\t\t\t\t\t",
+				"%u\n\t\t\t\t",
 				rwc_lf_results.w_no_ks_r_miss[j][i]);
 			printf("Hash add - key-shifts, lookup - hit"
-			       "(non-shift-path)\t\t%u\n\t\t\t\t\t\t\t\t",
+			       "(non-shift-path)\t\t%u\n\t\t\t\t",
 			       rwc_lf_results.w_ks_r_hit_nsp[j][i]);
 			printf("Hash add - key-shifts, lookup - hit "
-			       "(shift-path)\t\t%u\n\t\t\t\t\t\t\t\t",
+			       "(shift-path)\t\t%u\n\t\t\t\t",
 			       rwc_lf_results.w_ks_r_hit_sp[j][i]);
 			printf("Hash add - key-shifts, Hash lookup miss\t\t\t\t"
-				"%u\n\n\t\t\t\t",
+				"%u\n\n",
 				rwc_lf_results.w_ks_r_miss[j][i]);
 
-			printf("Disabled\t");
-			if (htm)
-				printf("Enabled\t\t");
-			else
-				printf("Disabled\t");
-			printf("Hash add - no key-shifts, lookup - hit\t\t\t\t"
-				"%u\n\t\t\t\t\t\t\t\t",
-				rwc_non_lf_results.w_no_ks_r_hit[j][i]);
-			printf("Hash add - no key-shifts, lookup - miss\t\t\t\t"
-				"%u\n\t\t\t\t\t\t\t\t",
-				rwc_non_lf_results.w_no_ks_r_miss[j][i]);
-			printf("Hash add - key-shifts, lookup - hit "
-			       "(non-shift-path)\t\t%u\n\t\t\t\t\t\t\t\t",
-			       rwc_non_lf_results.w_ks_r_hit_nsp[j][i]);
-			printf("Hash add - key-shifts, lookup - hit "
-			       "(shift-path)\t\t%u\n\t\t\t\t\t\t\t\t",
-			       rwc_non_lf_results.w_ks_r_hit_sp[j][i]);
-			printf("Hash add - key-shifts, Hash lookup miss\t\t\t\t"
-			       "%u\n", rwc_non_lf_results.w_ks_r_miss[j][i]);
-
-			printf("_______\t\t_______\t\t_________\t___\t\t"
-			       "_________\t\t\t\t\t\t_________________\n");
+			printf("_______\t\t_______\t\t_________\t\t\t"
+			       "\t\t\t_________________\n");
 		}
 
 		for (i = 1; i < NUM_TEST; i++) {
 			for (k = 0; k < NUM_TEST; k++) {
 				printf("%u", rwc_core_cnt[i]);
 				printf("\t\t%u\t\t", rwc_core_cnt[k]);
-				printf("Enabled\t\t");
-				printf("N/A\t\t");
-				printf("Multi-add-lookup\t\t\t\t\t\t%u\n\n\t\t"
-				       "\t\t",
+				printf("Multi-add-lookup\t\t\t\t\t\t%u\n\n",
 				       rwc_lf_results.multi_rw[i][j][k]);
-				printf("Disabled\t");
-				if (htm)
-					printf("Enabled\t\t");
-				else
-					printf("Disabled\t");
-				printf("Multi-add-lookup\t\t\t\t\t\t%u\n",
-				       rwc_non_lf_results.multi_rw[i][j][k]);
 
-				printf("_______\t\t_______\t\t_________\t___"
-				       "\t\t_________\t\t\t\t\t\t"
-				       "_________________\n");
+				printf("_______\t\t_______\t\t_________\t\t\t"
+				       "\t\t\t_________________\n");
 			}
 		}
 	}
-- 
2.17.1

[dpdk-dev] [PATCH 3/3] doc/hash: deprecate lock ellision and read/write concurreny flags

[Honnappa Nagarahalli]

RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY and
RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT configuration flags are
deprecated. Reader/Writer concurrency is provided by default.
Transactional memory will be used if the platform supports it.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
---
 doc/guides/rel_notes/deprecation.rst | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 34b28234c..d34cca260 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -55,3 +55,8 @@ Deprecation Notices
   - ``rte_pdump_set_socket_dir`` will be removed;
   - The parameter, ``path``, of ``rte_pdump_init`` will be removed;
   - The enum ``rte_pdump_socktype`` will be removed.
+
+* hash: The configuration flags RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY and
+  RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT will be removed in v19.02.
+  Reader/writer concurrency will be supported by default. The library will
+  use transactional memory if the platform supports it.
-- 
2.17.1