[v2] lib/hash: feature reclaim defer queue

  This patch adds a new feature to the hash library to allow the user to
reclaim the defer queue. This is useful when the user wants to force
reclaim resources that are not being used. This API is only available
if the RCU is enabled.

Signed-off-by: Abdullah Ömer Yamaç <aomeryamac@gmail.com>
Acked-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
 lib/hash/rte_cuckoo_hash.c | 23 +++++++++++++++++++++++
 lib/hash/rte_hash.h        | 14 ++++++++++++++
 lib/hash/version.map       |  7 +++++++
 3 files changed, 44 insertions(+)
  

Hello Abdullah,
	Thank you for the patch, few comments inline.

The short commit log could be changed as follows:

"lib/hash: add defer queue reclaim API”

> On Mar 2, 2024, at 3:27 PM, Abdullah Ömer Yamaç <aomeryamac@gmail.com> wrote:
> 
> This patch adds a new feature to the hash library to allow the user to
> reclaim the defer queue. This is useful when the user wants to force
> reclaim resources that are not being used. This API is only available
> if the RCU is enabled.
> 
> Signed-off-by: Abdullah Ömer Yamaç <aomeryamac@gmail.com>
> Acked-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Please add this only after you get an explicit Ack on the patch.

> ---
> lib/hash/rte_cuckoo_hash.c | 23 +++++++++++++++++++++++
> lib/hash/rte_hash.h        | 14 ++++++++++++++
> lib/hash/version.map       |  7 +++++++
> 3 files changed, 44 insertions(+)
> 
> diff --git a/lib/hash/rte_cuckoo_hash.c b/lib/hash/rte_cuckoo_hash.c
> index 9cf94645f6..254fa80cc5 100644
> --- a/lib/hash/rte_cuckoo_hash.c
> +++ b/lib/hash/rte_cuckoo_hash.c
> @@ -1588,6 +1588,27 @@ rte_hash_rcu_qsbr_add(struct rte_hash *h, struct rte_hash_rcu_config *cfg)
> return 0;
> }
> 
> +int
> +rte_hash_rcu_qsbr_dq_reclaim(struct rte_hash *h)
We need to add freed, pending and available parameters to this API. I think this information will be helpful for the users. For ex: in your use case, you could use the pending value to calculate the available hash entries.

> +{
> + int ret;
> +
> + if (h->hash_rcu_cfg == NULL || h->dq == NULL) {
We can skip NULL check for h->dq as the RCU reclaim API makes the same check.

> + rte_errno = EINVAL;
> + return -1;
> + }
> +
> + ret = rte_rcu_qsbr_dq_reclaim(h->dq, h->hash_rcu_cfg->max_reclaim_size, NULL, NULL, NULL);
> + if (ret != 0) {
> + HASH_LOG(ERR,
> + "%s: could not reclaim the defer queue in hash table",
> + __func__);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> static inline void
> remove_entry(const struct rte_hash *h, struct rte_hash_bucket *bkt,
> unsigned int i)
> diff --git a/lib/hash/rte_hash.h b/lib/hash/rte_hash.h
> index 7ecc021111..c119477d50 100644
> --- a/lib/hash/rte_hash.h
> +++ b/lib/hash/rte_hash.h
> @@ -674,6 +674,21 @@ rte_hash_iterate(const struct rte_hash *h, const void **key, void **data, uint32
>  */
> int rte_hash_rcu_qsbr_add(struct rte_hash *h, struct rte_hash_rcu_config *cfg);
> 
> +/**
> + * Reclaim resources from the defer queue.
> + * This API reclaim the resources from the defer queue if rcu is enabled.
> + *
> + * @param h
> + *   the hash object to reclaim resources
> + * @return
> + *   On success - 0
> + *   On error - 1 with error code set in rte_errno.
> + *   Possible rte_errno codes are:
> + *   - EINVAL - invalid pointer or invalid rcu mode
We can remove the ‘invalid rcd mode’.

> + */
> +__rte_experimental
> +int rte_hash_rcu_qsbr_dq_reclaim(struct rte_hash *h);
> +
> #ifdef __cplusplus
> }
> #endif
> diff --git a/lib/hash/version.map b/lib/hash/version.map
> index 6b2afebf6b..cec0e8fc67 100644
> --- a/lib/hash/version.map
> +++ b/lib/hash/version.map
> @@ -48,3 +48,9 @@ DPDK_24 {
> 
> local: *;
> };
> +
> +EXPERIMENTAL {
> + global:
> +
> + rte_hash_rcu_qsbr_dq_reclaim;
> +}
> \ No newline at end of file
> -- 
> 2.34.1
>
  

Just one more question.

On Sun, Mar 3, 2024 at 10:14 PM Honnappa Nagarahalli <
Honnappa.Nagarahalli@arm.com> wrote:

> Hello Abdullah,
>         Thank you for the patch, few comments inline.
>
> The short commit log could be changed as follows:
>
> "lib/hash: add defer queue reclaim API”
>
> > On Mar 2, 2024, at 3:27 PM, Abdullah Ömer Yamaç <aomeryamac@gmail.com>
> wrote:
> >
> > This patch adds a new feature to the hash library to allow the user to
> > reclaim the defer queue. This is useful when the user wants to force
> > reclaim resources that are not being used. This API is only available
> > if the RCU is enabled.
> >
> > Signed-off-by: Abdullah Ömer Yamaç <aomeryamac@gmail.com>
> > Acked-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Please add this only after you get an explicit Ack on the patch.
>
> > ---
> > lib/hash/rte_cuckoo_hash.c | 23 +++++++++++++++++++++++
> > lib/hash/rte_hash.h        | 14 ++++++++++++++
> > lib/hash/version.map       |  7 +++++++
> > 3 files changed, 44 insertions(+)
> >
> > diff --git a/lib/hash/rte_cuckoo_hash.c b/lib/hash/rte_cuckoo_hash.c
> > index 9cf94645f6..254fa80cc5 100644
> > --- a/lib/hash/rte_cuckoo_hash.c
> > +++ b/lib/hash/rte_cuckoo_hash.c
> > @@ -1588,6 +1588,27 @@ rte_hash_rcu_qsbr_add(struct rte_hash *h, struct
> rte_hash_rcu_config *cfg)
> > return 0;
> > }
> >
> > +int
> > +rte_hash_rcu_qsbr_dq_reclaim(struct rte_hash *h)
> We need to add freed, pending and available parameters to this API. I
> think this information will be helpful for the users. For ex: in your use
> case, you could use the pending value to calculate the available hash
> entries.
>
> The second parameter, "Maximum number of resources to free.", should be
available also? I set this value to " h->hash_rcu_cfg->max_reclaim_size",
but it can be a parameter in addition to the above parameters

> > +{
> > + int ret;
> > +
> > + if (h->hash_rcu_cfg == NULL || h->dq == NULL) {
> We can skip NULL check for h->dq as the RCU reclaim API makes the same
> check.
>
> > + rte_errno = EINVAL;
> > + return -1;
> > + }
> > +
> > + ret = rte_rcu_qsbr_dq_reclaim(h->dq,
> h->hash_rcu_cfg->max_reclaim_size, NULL, NULL, NULL);
> > + if (ret != 0) {
> > + HASH_LOG(ERR,
> > + "%s: could not reclaim the defer queue in hash table",
> > + __func__);
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static inline void
> > remove_entry(const struct rte_hash *h, struct rte_hash_bucket *bkt,
> > unsigned int i)
> > diff --git a/lib/hash/rte_hash.h b/lib/hash/rte_hash.h
> > index 7ecc021111..c119477d50 100644
> > --- a/lib/hash/rte_hash.h
> > +++ b/lib/hash/rte_hash.h
> > @@ -674,6 +674,21 @@ rte_hash_iterate(const struct rte_hash *h, const
> void **key, void **data, uint32
> >  */
> > int rte_hash_rcu_qsbr_add(struct rte_hash *h, struct rte_hash_rcu_config
> *cfg);
> >
> > +/**
> > + * Reclaim resources from the defer queue.
> > + * This API reclaim the resources from the defer queue if rcu is
> enabled.
> > + *
> > + * @param h
> > + *   the hash object to reclaim resources
> > + * @return
> > + *   On success - 0
> > + *   On error - 1 with error code set in rte_errno.
> > + *   Possible rte_errno codes are:
> > + *   - EINVAL - invalid pointer or invalid rcu mode
> We can remove the ‘invalid rcd mode’.
>
> > + */
> > +__rte_experimental
> > +int rte_hash_rcu_qsbr_dq_reclaim(struct rte_hash *h);
> > +
> > #ifdef __cplusplus
> > }
> > #endif
> > diff --git a/lib/hash/version.map b/lib/hash/version.map
> > index 6b2afebf6b..cec0e8fc67 100644
> > --- a/lib/hash/version.map
> > +++ b/lib/hash/version.map
> > @@ -48,3 +48,9 @@ DPDK_24 {
> >
> > local: *;
> > };
> > +
> > +EXPERIMENTAL {
> > + global:
> > +
> > + rte_hash_rcu_qsbr_dq_reclaim;
> > +}
> > \ No newline at end of file
> > --
> > 2.34.1
> >
>
>
  

> On Mar 4, 2024, at 2:27 AM, Abdullah Ömer Yamaç <aomeryamac@gmail.com> wrote:
> 
> Just one more question.
> 
> On Sun, Mar 3, 2024 at 10:14 PM Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> wrote:
> Hello Abdullah,
>         Thank you for the patch, few comments inline.
> 
> The short commit log could be changed as follows:
> 
> "lib/hash: add defer queue reclaim API”
> 
> > On Mar 2, 2024, at 3:27 PM, Abdullah Ömer Yamaç <aomeryamac@gmail.com> wrote:
> > 
> > This patch adds a new feature to the hash library to allow the user to
> > reclaim the defer queue. This is useful when the user wants to force
> > reclaim resources that are not being used. This API is only available
> > if the RCU is enabled.
> > 
> > Signed-off-by: Abdullah Ömer Yamaç <aomeryamac@gmail.com>
> > Acked-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Please add this only after you get an explicit Ack on the patch.
> 
> > ---
> > lib/hash/rte_cuckoo_hash.c | 23 +++++++++++++++++++++++
> > lib/hash/rte_hash.h        | 14 ++++++++++++++
> > lib/hash/version.map       |  7 +++++++
> > 3 files changed, 44 insertions(+)
> > 
> > diff --git a/lib/hash/rte_cuckoo_hash.c b/lib/hash/rte_cuckoo_hash.c
> > index 9cf94645f6..254fa80cc5 100644
> > --- a/lib/hash/rte_cuckoo_hash.c
> > +++ b/lib/hash/rte_cuckoo_hash.c
> > @@ -1588,6 +1588,27 @@ rte_hash_rcu_qsbr_add(struct rte_hash *h, struct rte_hash_rcu_config *cfg)
> > return 0;
> > }
> > 
> > +int
> > +rte_hash_rcu_qsbr_dq_reclaim(struct rte_hash *h)
> We need to add freed, pending and available parameters to this API. I think this information will be helpful for the users. For ex: in your use case, you could use the pending value to calculate the available hash entries.
> 
> The second parameter, "Maximum number of resources to free.", should be available also? I set this value to " h->hash_rcu_cfg->max_reclaim_size", but it can be a parameter in addition to the above parameters
We don’t have to expose that in the API. The user has provided that already in max_reclaim_size. So, using "h->hash_rcu_cfg->max_reclaim_size” is enough.

> > +{
> > + int ret;
> > +
> > + if (h->hash_rcu_cfg == NULL || h->dq == NULL) {
> We can skip NULL check for h->dq as the RCU reclaim API makes the same check.
> 
> > + rte_errno = EINVAL;
> > + return -1;
> > + }
> > +
> > + ret = rte_rcu_qsbr_dq_reclaim(h->dq, h->hash_rcu_cfg->max_reclaim_size, NULL, NULL, NULL);
> > + if (ret != 0) {
> > + HASH_LOG(ERR,
> > + "%s: could not reclaim the defer queue in hash table",
> > + __func__);
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static inline void
> > remove_entry(const struct rte_hash *h, struct rte_hash_bucket *bkt,
> > unsigned int i)
> > diff --git a/lib/hash/rte_hash.h b/lib/hash/rte_hash.h
> > index 7ecc021111..c119477d50 100644
> > --- a/lib/hash/rte_hash.h
> > +++ b/lib/hash/rte_hash.h
> > @@ -674,6 +674,21 @@ rte_hash_iterate(const struct rte_hash *h, const void **key, void **data, uint32
> >  */
> > int rte_hash_rcu_qsbr_add(struct rte_hash *h, struct rte_hash_rcu_config *cfg);
> > 
> > +/**
> > + * Reclaim resources from the defer queue.
> > + * This API reclaim the resources from the defer queue if rcu is enabled.
> > + *
> > + * @param h
> > + *   the hash object to reclaim resources
> > + * @return
> > + *   On success - 0
> > + *   On error - 1 with error code set in rte_errno.
> > + *   Possible rte_errno codes are:
> > + *   - EINVAL - invalid pointer or invalid rcu mode
> We can remove the ‘invalid rcd mode’.
> 
> > + */
> > +__rte_experimental
> > +int rte_hash_rcu_qsbr_dq_reclaim(struct rte_hash *h);
> > +
> > #ifdef __cplusplus
> > }
> > #endif
> > diff --git a/lib/hash/version.map b/lib/hash/version.map
> > index 6b2afebf6b..cec0e8fc67 100644
> > --- a/lib/hash/version.map
> > +++ b/lib/hash/version.map
> > @@ -48,3 +48,9 @@ DPDK_24 {
> > 
> > local: *;
> > };
> > +
> > +EXPERIMENTAL {
> > + global:
> > +
> > + rte_hash_rcu_qsbr_dq_reclaim;
> > +}
> > \ No newline at end of file
> > -- 
> > 2.34.1
> > 
>