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

[Wang, Yipeng1]

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