> -----Original Message----- > From: Dharmik Thakkar <dharmik.thak...@arm.com> > Sent: Tuesday, August 18, 2020 9:06 PM > To: Wang, Yipeng1 <yipeng1.w...@intel.com>; Gobriel, Sameh > <sameh.gobr...@intel.com>; Richardson, Bruce <bruce.richard...@intel.com>; > Ray Kinsella <m...@ashroe.eu>; Neil Horman <nhor...@tuxdriver.com> > Cc: dev@dpdk.org; n...@arm.com; Dharmik Thakkar > <dharmik.thak...@arm.com> > Subject: [RFC v2] lib/hash: integrate RCU QSBR > > Integrate RCU QSBR to make it easier for the applications to use lock free > algorithm. > > Resource reclamation implementation was split from the original series, and > has already been part of RCU library. Rework the series to base hash > integration on RCU reclamation APIs. > > Refer 'Resource reclamation framework for DPDK' available at [1] to > understand various aspects of integrating RCU library into other libraries. > > [1] https://doc.dpdk.org/guides/prog_guide/rcu_lib.html > > Introduce a new API rte_hash_rcu_qsbr_add for application to register a RCU > variable that hash library will use. > > 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> > --- > v2: > - Remove defer queue related functions and use resource reclamation > APIs from the RCU QSBR library instead > > - Remove patch (net/ixgbe: avoid multpile definitions of 'bool') > from the series as it is already accepted > > --- > lib/Makefile | 2 +- > lib/librte_hash/Makefile | 2 +- > lib/librte_hash/meson.build | 1 + > lib/librte_hash/rte_cuckoo_hash.c | 291 +++++++++++++++++++++------ > lib/librte_hash/rte_cuckoo_hash.h | 8 + > lib/librte_hash/rte_hash.h | 75 ++++++- > lib/librte_hash/rte_hash_version.map | 2 +- > 7 files changed, 308 insertions(+), 73 deletions(-) >
<......> > +/** HASH RCU QSBR configuration structure. */ struct > +rte_hash_rcu_config { > + struct rte_rcu_qsbr *v; /**< RCU QSBR variable. */ > + enum rte_hash_qsbr_mode mode; > + /**< Mode of RCU QSBR. RTE_HASH_QSBR_MODE_xxx > + * '0' for default: create defer queue for reclaim. > + */ > + uint32_t dq_size; > + /**< RCU defer queue size. > + * default: total hash table entries. > + */ > + uint32_t reclaim_thd; /**< Threshold to trigger auto reclaim. */ > + uint32_t reclaim_max; > + /**< Max entries to reclaim in one go. > + * default: RTE_HASH_RCU_DQ_RECLAIM_MAX. > + */ > + void *key_data_ptr; > + /**< Pointer passed to the free function. Typically, this is the > + * pointer to the data structure to which the resource to free > + * (key-data) belongs. This can be NULL. > + */ > + rte_hash_free_key_data free_key_data_func; > + /**< Function to call to free the resource (key-data). */ }; > + [Wang, Yipeng] I guess this is mostly a wrapper of rte_rcu_qsbr_dq_parameters. Personally, I incline to use variable names that match the existing qsbr parameters better. For example, you could still call reclaim_thd as reclaim_limit. And _max to be _size. Thus, people who are already familiar with qsbr can match the meanings better. > /** @internal A hash table structure. */ struct rte_hash; > > @@ -287,7 +329,8 @@ rte_hash_add_key_with_hash(const struct rte_hash *h, > const void *key, hash_sig_t > * Thread safety can be enabled by setting flag during > * table creation. > * If RTE_HASH_EXTRA_FLAGS_NO_FREE_ON_DEL or > - * RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF is enabled, > + * RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF is enabled and > + * internal RCU is NOT enabled, > * the key index returned by rte_hash_add_key_xxx APIs will not be > * freed by this API. rte_hash_free_key_with_position API must be called > * additionally to free the index associated with the key. > @@ -316,7 +359,8 @@ rte_hash_del_key(const struct rte_hash *h, const void > *key); > * Thread safety can be enabled by setting flag during > * table creation. > * If RTE_HASH_EXTRA_FLAGS_NO_FREE_ON_DEL or > - * RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF is enabled, > + * RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF is enabled and > + * internal RCU is NOT enabled, > * the key index returned by rte_hash_add_key_xxx APIs will not be > * freed by this API. rte_hash_free_key_with_position API must be called > * additionally to free the index associated with the key. > @@ -370,7 +414,8 @@ rte_hash_get_key_with_position(const struct rte_hash > *h, const int32_t position, > * only be called from one thread by default. Thread safety > * can be enabled by setting flag during table creation. > * If RTE_HASH_EXTRA_FLAGS_NO_FREE_ON_DEL or > - * RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF is enabled, > + * RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF is enabled and > + * internal RCU is NOT enabled, > * the key index returned by rte_hash_del_key_xxx APIs must be freed > * using this API. This API should be called after all the readers > * have stopped referencing the entry corresponding to this key. > @@ -625,6 +670,28 @@ rte_hash_lookup_bulk(const struct rte_hash *h, const > void **keys, > */ > int32_t > rte_hash_iterate(const struct rte_hash *h, const void **key, void **data, > uint32_t *next); > + > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice > + * > + * Associate RCU QSBR variable with an Hash object. [Wang, Yipeng] To enable RCU we need to call this func. I think you can be more explicit, e.g. "This API should be called to enable the RCU support" > + * > + * @param h > + * the hash object to add RCU QSBR > + * @param cfg > + * RCU QSBR configuration > + * @return > + * On success - 0 > + * On error - 1 with error code set in rte_errno. > + * Possible rte_errno codes are: > + * - EINVAL - invalid pointer > + * - EEXIST - already added QSBR > + * - ENOMEM - memory allocation failure > + */ [Wang, Yipeng] Is there any further requirement for when to call this API? E.g. you could say "this API should be called immediately after rte_hash_create()" > +__rte_experimental > +int rte_hash_rcu_qsbr_add(struct rte_hash *h, > + struct rte_hash_rcu_config *cfg); > #ifdef __cplusplus > } > #endif > diff --git a/lib/librte_hash/rte_hash_version.map > b/lib/librte_hash/rte_hash_version.map > index c0db81014ff9..c6d73080f478 100644 > --- a/lib/librte_hash/rte_hash_version.map > +++ b/lib/librte_hash/rte_hash_version.map > @@ -36,5 +36,5 @@ EXPERIMENTAL { > rte_hash_lookup_with_hash_bulk; > rte_hash_lookup_with_hash_bulk_data; > rte_hash_max_key_id; > - > + rte_hash_rcu_qsbr_add; > }; > -- > 2.17.1 [Wang, Yipeng] Hi, Dharmik, Thanks for the patch. It generally looks good to me. I guess you will revise documentation and the unit test as well after the RFC. That is helpful for users to understand how to use hash appropriately with the RCU lib.