> -----Original Message-----
> From: Dharmik Thakkar <[email protected]>
> Sent: Tuesday, August 18, 2020 9:06 PM
> To: Wang, Yipeng1 <[email protected]>; Gobriel, Sameh
> <[email protected]>; Richardson, Bruce <[email protected]>;
> Ray Kinsella <[email protected]>; Neil Horman <[email protected]>
> Cc: [email protected]; [email protected]; Dharmik Thakkar
> <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Dharmik Thakkar <[email protected]>
> Reviewed-by: Ruifeng Wang <[email protected]>
> ---
> 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.
