+Honnappa Hi Yipeng,
Thank you for reviewing! > On Mar 22, 2019, at 6:48 PM, Wang, Yipeng1 <yipeng1.w...@intel.com> wrote: > > Thanks for the patch! > > Comments inlined: > >> -----Original Message----- >> From: Dharmik Thakkar [mailto:dharmik.thak...@arm.com] >> Sent: Wednesday, March 20, 2019 3:35 PM >> To: Wang, Yipeng1 <yipeng1.w...@intel.com>; Gobriel, Sameh >> <sameh.gobr...@intel.com>; Richardson, Bruce >> <bruce.richard...@intel.com>; De Lara Guarch, Pablo >> <pablo.de.lara.gua...@intel.com>; Mcnamara, John >> <john.mcnam...@intel.com>; Kovacevic, Marko <marko.kovace...@intel.com> >> Cc: dev@dpdk.org; Dharmik Thakkar <dharmik.thak...@arm.com> >> Subject: [PATCH 1/2] hash: add lock free support for extendable bucket >> >> This patch enables lock-free read-write concurrency support for >> extendable bucket feature. >> >> Suggested-by: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com> >> Signed-off-by: Dharmik Thakkar <dharmik.thak...@arm.com> >> Reviewed-by: Ruifeng Wang <ruifeng.w...@arm.com> >> Reviewed-by: Gavin Hu <gavin...@arm.com> >> Reviewed-by: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com> >> --- >> doc/guides/prog_guide/hash_lib.rst | 3 +- >> lib/librte_hash/rte_cuckoo_hash.c | 163 ++++++++++++++++++++--------- >> lib/librte_hash/rte_cuckoo_hash.h | 7 ++ >> 3 files changed, 121 insertions(+), 52 deletions(-) >> >> diff --git a/doc/guides/prog_guide/hash_lib.rst >> b/doc/guides/prog_guide/hash_lib.rst >> index 85a6edfa8b16..b00446e949ba 100644 >> --- a/doc/guides/prog_guide/hash_lib.rst >> +++ b/doc/guides/prog_guide/hash_lib.rst >> @@ -108,8 +108,7 @@ Extendable Bucket Functionality support >> An extra flag is used to enable this functionality (flag is not set by >> default). When the (RTE_HASH_EXTRA_FLAGS_EXT_TABLE) is set >> and >> in the very unlikely case due to excessive hash collisions that a key has >> failed to be inserted, the hash table bucket is extended with a >> linked >> list to insert these failed keys. This feature is important for the >> workloads (e.g. telco workloads) that need to insert up to 100% of the >> -hash table size and can't tolerate any key insertion failure (even if very >> few). Currently the extendable bucket is not supported >> -with the lock-free concurrency implementation >> (RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF). >> +hash table size and can't tolerate any key insertion failure (even if very >> few). > [Wang, Yipeng] I am thinking maybe make it a bit more clear here by adding > something like: > Please note that with the lock-free flag enabled, users need to promptly free > the deleted keys, to maintain the 100% capacity guarantee. > > I want to add this because of the piggy-back mechanism, one un-recycled key > with an un-recycled ext bucket may actually makes in total > of 9 entries unavailable (8 entries in the ext bucket). So it would be useful > to remind the user here. All right. I will add it. >> >> >> @@ -1054,7 +1059,15 @@ __rte_hash_add_key_with_hash(const struct rte_hash >> *h, const void *key, >> /* Check if slot is available */ >> if (likely(cur_bkt->key_idx[i] == EMPTY_SLOT)) { >> cur_bkt->sig_current[i] = short_sig; >> - cur_bkt->key_idx[i] = new_idx; >> + /* Key can be of arbitrary length, so it is >> + * not possible to store it atomically. >> + * Hence the new key element's memory stores >> + * (key as well as data) should be complete >> + * before it is referenced. >> + */ > [Wang, Yipeng] My understanding is this atomic store is to prevent the > signature store leaking after the key_idx store. > But the comment does not exactly describe this reason. I will update the comment. >> + __atomic_store_n(&cur_bkt->key_idx[i], >> + new_idx, >> + __ATOMIC_RELEASE); >> __hash_rw_writer_unlock(h); >> return new_idx - 1; >> } >> @@ -1545,6 +1597,14 @@ rte_hash_free_key_with_position(const struct rte_hash >> *h, >> /* Out of bounds */ >> if (position >= total_entries) >> return -EINVAL; >> + if (h->ext_table_support) { >> + uint32_t index = h->ext_bkt_to_free[position]; > [Wang, Yipeng] I think user can theoretically set > RTE_HASH_EXTRA_FLAGS_NO_FREE_ON_DEL to be 1 > But LF flag to be 0. I think here you assume this function only called when > LF flag is 1. You may need to > Add another condition e.g. if(h->ext_table_support && > h->readwrite_concur_lf_support) Correct. I will update it. >> + if (index) { >> + /* Recycle empty ext bkt to free list. */ >> + rte_ring_sp_enqueue(h->free_ext_bkts, (void >> *)(uintptr_t)index); >> + h->ext_bkt_to_free[position] = 0; >> + } >> + } >> >> if (h->use_local_cache) { >> lcore_id = rte_lcore_id();
