Hi Yipeng,
Thank you for the comments!
> On Aug 31, 2020, at 3:47 PM, Wang, Yipeng1 <yipeng1.w...@intel.com> wrote:
>
>> -----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.
>
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.
