From: "Wang, Yipeng1" <yipeng1.wang@intel.com>
To: Dharmik Thakkar <dharmik.thakkar@arm.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" <dev@dpdk.org>, "nd@arm.com" <nd@arm.com>
Subject: Re: [dpdk-dev] [RFC v2] lib/hash: integrate RCU QSBR
Date: Mon, 31 Aug 2020 20:47:14 +0000 [thread overview]
Message-ID: <BYAPR11MB3494723D6D46279CC20DB3B3C3510@BYAPR11MB3494.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20200819040537.1792-1-dharmik.thakkar@arm.com>
> -----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.
> /** @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.
next prev parent reply other threads:[~2020-08-31 20:48 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-01 6:58 [dpdk-dev] [RFC 0/3] RCU integration with hash library Dharmik Thakkar
2019-09-01 6:58 ` [dpdk-dev] [RFC 1/3] net/ixgbe: avoid multpile definitions of 'bool' Dharmik Thakkar
2019-09-05 15:26 ` Stephen Hemminger
2019-09-05 20:10 ` Dharmik Thakkar
2019-09-05 20:23 ` Stephen Hemminger
2019-09-01 6:58 ` [dpdk-dev] [RFC 2/3] lib/hash: integrate RCU QSBR Dharmik Thakkar
2019-09-01 6:58 ` [dpdk-dev] [RFC 3/3] test/hash: add tests for integrated " Dharmik Thakkar
2019-09-05 21:31 ` [dpdk-dev] [RFC 0/3] RCU integration with hash library Wang, Yipeng1
2020-08-19 4:05 ` [dpdk-dev] [RFC v2] lib/hash: integrate RCU QSBR Dharmik Thakkar
2020-08-25 19:59 ` Dharmik Thakkar
2020-08-31 20:47 ` Wang, Yipeng1 [this message]
2020-09-01 22:01 ` Dharmik Thakkar
2020-10-16 13:53 ` David Marchand
2020-10-16 15:04 ` Dharmik Thakkar
2020-10-16 17:38 ` [dpdk-dev] [PATCH 0/3] hash: " Dharmik Thakkar
2020-10-16 17:38 ` [dpdk-dev] [PATCH v3 1/3] lib/hash: " Dharmik Thakkar
2020-10-19 9:43 ` Kinsella, Ray
2020-10-16 17:38 ` [dpdk-dev] [PATCH v3 2/3] test/hash: replace rte atomic with C11 atomic APIs Dharmik Thakkar
2020-10-16 17:38 ` [dpdk-dev] [PATCH v3 3/3] test/hash: add tests for integrated RCU QSBR Dharmik Thakkar
2020-10-19 14:48 ` [dpdk-dev] [PATCH 0/3] hash: integrate " David Marchand
2020-10-19 16:35 ` [dpdk-dev] [PATCH v4 " Dharmik Thakkar
2020-10-19 16:35 ` [dpdk-dev] [PATCH v4 1/3] lib/hash: " Dharmik Thakkar
2020-10-19 16:35 ` [dpdk-dev] [PATCH v4 2/3] test/hash: replace rte atomic with C11 atomic APIs Dharmik Thakkar
2020-10-19 16:35 ` [dpdk-dev] [PATCH v4 3/3] test/hash: add tests for integrated RCU QSBR Dharmik Thakkar
2020-10-19 21:05 ` [dpdk-dev] [PATCH v4 0/3] hash: integrate " David Marchand
2020-10-20 4:08 ` Dharmik Thakkar
2020-10-20 16:12 ` [dpdk-dev] [PATCH v5 0/4] " Dharmik Thakkar
2020-10-20 16:12 ` [dpdk-dev] [PATCH v5 1/4] rcu: build on Windows Dharmik Thakkar
2020-10-20 16:12 ` [dpdk-dev] [PATCH v5 2/4] lib/hash: integrate RCU QSBR Dharmik Thakkar
2020-10-21 2:42 ` Wang, Yipeng1
2020-10-21 4:52 ` Dharmik Thakkar
2020-10-20 16:13 ` [dpdk-dev] [PATCH v5 3/4] test/hash: replace rte atomic with C11 atomic APIs Dharmik Thakkar
2020-10-21 2:52 ` Wang, Yipeng1
2020-10-20 16:13 ` [dpdk-dev] [PATCH v5 4/4] test/hash: add tests for integrated RCU QSBR Dharmik Thakkar
2020-10-21 3:54 ` Wang, Yipeng1
2020-10-21 4:55 ` Dharmik Thakkar
2020-10-21 22:34 ` Honnappa Nagarahalli
2020-10-21 22:50 ` [dpdk-dev] [PATCH v6 0/4] hash: integrate " Dharmik Thakkar
2020-10-21 22:50 ` [dpdk-dev] [PATCH v6 1/4] rcu: build on Windows Dharmik Thakkar
2020-10-22 9:05 ` David Marchand
2020-10-22 18:35 ` Kadam, Pallavi
2020-10-21 22:50 ` [dpdk-dev] [PATCH v6 2/4] lib/hash: integrate RCU QSBR Dharmik Thakkar
2020-10-23 21:46 ` Dharmik Thakkar
2020-10-23 22:08 ` Wang, Yipeng1
2020-10-21 22:50 ` [dpdk-dev] [PATCH v6 3/4] test/hash: replace rte atomic with C11 atomic APIs Dharmik Thakkar
2020-10-21 22:50 ` [dpdk-dev] [PATCH v6 4/4] test/hash: add tests for integrated RCU QSBR Dharmik Thakkar
2020-10-23 22:11 ` Wang, Yipeng1
2020-10-24 9:09 ` [dpdk-dev] [PATCH v6 0/4] hash: integrate " David Marchand
2020-10-26 13:56 ` Dharmik Thakkar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BYAPR11MB3494723D6D46279CC20DB3B3C3510@BYAPR11MB3494.namprd11.prod.outlook.com \
--to=yipeng1.wang@intel.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=dharmik.thakkar@arm.com \
--cc=mdr@ashroe.eu \
--cc=nd@arm.com \
--cc=nhorman@tuxdriver.com \
--cc=sameh.gobriel@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).