Re: [dpdk-dev] [PATCH v3 1/7] hash: separate multi-writer from rw-concurrency

["Wang, Yipeng1" <yipeng1.wang@intel.com>]

>-----Original Message-----
>From: Honnappa Nagarahalli [mailto:honnappa.nagarahalli@arm.com]
>Sent: Thursday, October 11, 2018 11:32 PM
>To: Richardson, Bruce <bruce.richardson@intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
>Cc: dev@dpdk.org; Wang, Yipeng1 <yipeng1.wang@intel.com>; honnappa.nagarahalli@arm.com; dharmik.thakkar@arm.com;
>gavin.hu@arm.com; nd@arm.com
>Subject: [PATCH v3 1/7] hash: separate multi-writer from rw-concurrency
>
>RW concurrency is required with single writer and multiple reader
>usecase as well. Hence, multi-writer should not be enabled by default when
>RW concurrency is enabled.
>
>Fixes: f2e3001b53ec ("hash: support read/write concurrency")
>Cc: yipeng1.wang@intel.com
>
>Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>Reviewed-by: Gavin Hu <gavin.hu@arm.com>
>---
> lib/librte_hash/rte_cuckoo_hash.c | 27 ++++++++++++++++-----------
> lib/librte_hash/rte_cuckoo_hash.h |  2 ++
> test/test/test_hash_readwrite.c   |  6 ++++--
> 3 files changed, 22 insertions(+), 13 deletions(-)
>
>+	uint8_t writer_takes_lock;
>+	/**< Indicates if the writer threads need to take lock */

[Wang, Yipeng] 
Our understanding is that the difference between writer_takes_lock and multi_writer_support flag now is that for the multi-writer case
we still have the local cache for key-data pair slot. Please correct me if I am wrong.

But the name is confusing because writer_takes_lock implies multi-writer support. Especially the comment here says
that writer needs a lock, which means multi-writer is supported. So conceptually it does not have different meaning than the multi_writer_support
by just reading the name.

If you want to distinguish these two implementation (with vs. without cache), maybe change the name of multi-writer flag to use_local_cache flag?
And the previous locking mechanism need to enable this flag for performance reasons, while the LF does not.
Or just keep the cache for both cases, and I don't think the local cache will add too much overhead.

> 	rte_hash_function hash_func;    /**< Function used to calculate hash. */
> 	uint32_t hash_func_init_val;    /**< Init value used by hash_func. */
> 	rte_hash_cmp_eq_t rte_hash_custom_cmp_eq;
>diff --git a/test/test/test_hash_readwrite.c b/test/test/test_hash_readwrite.c
>index 2a4f7b9..a8fadd0 100644
>--- a/test/test/test_hash_readwrite.c
>+++ b/test/test/test_hash_readwrite.c
>@@ -122,10 +122,12 @@ init_params(int use_htm, int use_jhash)
> 	if (use_htm)
> 		hash_params.extra_flag =
> 			RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT |
>-			RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY;
>+			RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY |
>+			RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD;
[Wang, Yipeng] 
Could you double check that if current applications do not change their code, there is
no functional issue will be introduced by this change, otherwise this would be an API change.
I believe it will have performance implication though.

Otherwise I am OK with this patch.