Re: [dpdk-dev] [RFC v2] lib/hash: integrate RCU QSBR

[Dharmik Thakkar <Dharmik.Thakkar@arm.com>]

Hi Yipeng,
Thank you for the comments!

> On Aug 31, 2020, at 3:47 PM, Wang, Yipeng1 <yipeng1.wang@intel.com> wrote:
> 
>> -----Original Message-----
>> From: Dharmik Thakkar <dharmik.thakkar@arm.com>
>> Sent: Tuesday, August 18, 2020 9:06 PM
>> To: Wang, Yipeng1 <yipeng1.wang@intel.com>; Gobriel, Sameh
>> <sameh.gobriel@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>;
>> Ray Kinsella <mdr@ashroe.eu>; Neil Horman <nhorman@tuxdriver.com>
>> Cc: dev@dpdk.org; nd@arm.com; Dharmik Thakkar
>> <dharmik.thakkar@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.nagarahalli@arm.com>
>> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
>> Reviewed-by: Ruifeng Wang <ruifeng.wang@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.
> 

Makes sense. I will update it.

> 
>> /** @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"
> 

Yes.

>> + *
>> + * @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()"
> 

Sure, I will add further guidelines/requirements.

>> +__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. 

That’s great. I will convert it to a patch.

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

Yes, I will add the documentation and unit test patches.