| Message ID | 1539325918-125438-2-git-send-email-honnappa.nagarahalli@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Address reader-writer concurrency in rte_hash | expand |
>-----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.
Hi Yipeng, Thank you for the review, appreciate your efforts. Thank you, Honnappa > > > >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. Yes, that is correct. Need for lock and need for local cache are separated. > > 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? Agree, I will change the 'multi-writer' name to 'use_local_cache'. I will keep the 'writer_takes_lock' 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. Agree, it might not make much of a performance difference. My goal is to reduce the memory used when multi-writer is not enabled. > > > 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. It is not advertised that multi-writer add is assumed when read-write concurrency is enabled. I think we should be ok. The functionality will be fine. Only difference is that the local-cache will not be enabled without this flag. So, yes, there will be performance implication. > > Otherwise I am OK with this patch.
diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c index 750caf8..7bbe9e9 100644 --- a/lib/librte_hash/rte_cuckoo_hash.c +++ b/lib/librte_hash/rte_cuckoo_hash.c @@ -139,6 +139,7 @@ rte_hash_create(const struct rte_hash_parameters *params) unsigned int hw_trans_mem_support = 0, multi_writer_support = 0; unsigned int ext_table_support = 0; unsigned int readwrite_concur_support = 0; + unsigned int writer_takes_lock = 0; rte_hash_function default_hash_func = (rte_hash_function)rte_jhash; @@ -162,12 +163,14 @@ rte_hash_create(const struct rte_hash_parameters *params) 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) { multi_writer_support = 1; + writer_takes_lock = 1; + } if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY) { readwrite_concur_support = 1; - multi_writer_support = 1; + writer_takes_lock = 1; } if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_EXT_TABLE) @@ -355,6 +358,7 @@ rte_hash_create(const struct rte_hash_parameters *params) h->multi_writer_support = multi_writer_support; h->readwrite_concur_support = readwrite_concur_support; h->ext_table_support = ext_table_support; + h->writer_takes_lock = writer_takes_lock; #if defined(RTE_ARCH_X86) if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE2)) @@ -363,10 +367,11 @@ rte_hash_create(const struct rte_hash_parameters *params) #endif h->sig_cmp_fn = RTE_HASH_COMPARE_SCALAR; - /* Turn on multi-writer only with explicit flag from user and TM - * support. + /* 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->multi_writer_support) { + if (h->writer_takes_lock) { h->readwrite_lock = rte_malloc(NULL, sizeof(rte_rwlock_t), RTE_CACHE_LINE_SIZE); if (h->readwrite_lock == NULL) @@ -425,10 +430,10 @@ rte_hash_free(struct rte_hash *h) rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK); - if (h->multi_writer_support) { + if (h->multi_writer_support) rte_free(h->local_free_slots); + if (h->writer_takes_lock) rte_free(h->readwrite_lock); - } rte_ring_free(h->free_slots); rte_ring_free(h->free_ext_bkts); rte_free(h->key_store); @@ -473,9 +478,9 @@ rte_hash_count(const struct rte_hash *h) static inline void __hash_rw_writer_lock(const struct rte_hash *h) { - if (h->multi_writer_support && h->hw_trans_mem_support) + if (h->writer_takes_lock && h->hw_trans_mem_support) rte_rwlock_write_lock_tm(h->readwrite_lock); - else if (h->multi_writer_support) + else if (h->writer_takes_lock) rte_rwlock_write_lock(h->readwrite_lock); } @@ -491,9 +496,9 @@ __hash_rw_reader_lock(const struct rte_hash *h) static inline void __hash_rw_writer_unlock(const struct rte_hash *h) { - if (h->multi_writer_support && h->hw_trans_mem_support) + if (h->writer_takes_lock && h->hw_trans_mem_support) rte_rwlock_write_unlock_tm(h->readwrite_lock); - else if (h->multi_writer_support) + else if (h->writer_takes_lock) rte_rwlock_write_unlock(h->readwrite_lock); } diff --git a/lib/librte_hash/rte_cuckoo_hash.h b/lib/librte_hash/rte_cuckoo_hash.h index 7753cd8..1137651 100644 --- a/lib/librte_hash/rte_cuckoo_hash.h +++ b/lib/librte_hash/rte_cuckoo_hash.h @@ -166,6 +166,8 @@ struct rte_hash { uint8_t readwrite_concur_support; /**< If read-write concurrency support is enabled */ uint8_t ext_table_support; /**< Enable extendable bucket table */ + uint8_t writer_takes_lock; + /**< Indicates if the writer threads need to take lock */ 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; else hash_params.extra_flag = - RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY; + RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY | + RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD; hash_params.name = "tests";
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(-) -- 2.7.4