Re: [PATCH v5] lib/hash: add defer queue reclaim API

["Abdullah Ömer Yamaç" <aomeryamac@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8261 bytes --]

Hello,
That's my mistake and I will discard the lookup part. While I was
implementing these parts, I thought that readers should not be in a
quiescent state, and I just wanted to make sure that these values were in
use and the reader was in the critical section.

Thanks I will fix ASAP

On Mon, May 13, 2024 at 8:25 AM Honnappa Nagarahalli <
Honnappa.Nagarahalli@arm.com> wrote:

> Hi Abdullah,
>         This looks good, except for one comment inline.
>
> > On Apr 27, 2024, at 2:54 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>
> > ---
> > app/test/test_hash.c       | 90 ++++++++++++++++++++++++++++++++++++++
> > lib/hash/rte_cuckoo_hash.c | 21 +++++++++
> > lib/hash/rte_hash.h        | 25 +++++++++++
> > lib/hash/version.map       |  7 +++
> > 4 files changed, 143 insertions(+)
> >
> > diff --git a/app/test/test_hash.c b/app/test/test_hash.c
> > index d586878a22..e763d0503f 100644
> > --- a/app/test/test_hash.c
> > +++ b/app/test/test_hash.c
> > @@ -2183,6 +2183,93 @@ test_hash_rcu_qsbr_sync_mode(uint8_t ext_bkt)
> >
> > }
> >
> > +/*
> > + * rte_hash_rcu_qsbr_dq_reclaim unit test.
> > + */
> > +static int test_hash_rcu_qsbr_dq_reclaim(void)
> > +{
> > + size_t sz;
> > + int32_t status;
> > + unsigned int total_entries = 8;
> > + unsigned int freed, pending, available;
> > + uint32_t reclaim_keys[8] = {10, 11, 12, 13, 14, 15, 16, 17};
> > + struct rte_hash_rcu_config rcu_cfg = {0};
> > + struct rte_hash_parameters hash_params = {
> > +    .name = "test_hash_rcu_qsbr_dq_reclaim",
> > +    .entries = total_entries,
> > +    .key_len = sizeof(uint32_t),
> > +    .hash_func = NULL,
> > +    .hash_func_init_val = 0,
> > +    .socket_id = 0,
> > + };
> > +
> > + hash_params.extra_flag = RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF;
> > +
> > + g_qsv = NULL;
> > + g_handle = NULL;
> > +
> > + printf("\n# Running RCU QSBR DQ mode, reclaim defer queue functional
> test\n");
> > +
> > + g_handle = rte_hash_create(&hash_params);
> > + RETURN_IF_ERROR_RCU_QSBR(g_handle == NULL, "Hash creation failed");
> > +
> > + /* Create RCU QSBR variable */
> > + sz = rte_rcu_qsbr_get_memsize(RTE_MAX_LCORE);
> > + g_qsv = (struct rte_rcu_qsbr *)rte_zmalloc_socket(NULL, sz,
> RTE_CACHE_LINE_SIZE,
> > +  SOCKET_ID_ANY);
> > + RETURN_IF_ERROR_RCU_QSBR(g_qsv == NULL, "RCU QSBR variable creation
> failed");
> > +
> > + status = rte_rcu_qsbr_init(g_qsv, RTE_MAX_LCORE);
> > + RETURN_IF_ERROR_RCU_QSBR(status != 0, "RCU QSBR variable
> initialization failed");
> > +
> > + rcu_cfg.v = g_qsv;
> > + rcu_cfg.dq_size = total_entries;
> > + rcu_cfg.mode = RTE_HASH_QSBR_MODE_DQ;
> > +
> > + /* Attach RCU QSBR to hash table */
> > + status = rte_hash_rcu_qsbr_add(g_handle, &rcu_cfg);
> > + RETURN_IF_ERROR_RCU_QSBR(status != 0, "Attach RCU QSBR to hash table
> failed");
> > +
> > + /* Register pseudo reader */
> > + status = rte_rcu_qsbr_thread_register(g_qsv, 0);
> > + RETURN_IF_ERROR_RCU_QSBR(status != 0, "RCU QSBR thread registration
> failed");
> > + rte_rcu_qsbr_thread_online(g_qsv, 0);
> > +
> > + /* Fill half of the hash table */
> > + for (size_t i = 0; i < total_entries / 2; i++)
> > + status = rte_hash_add_key(g_handle, &reclaim_keys[i]);
> > +
> > + /* Lookup inserted elements*/
> > + for (size_t i = 0; i < total_entries / 2; i++)
> > + rte_hash_lookup(g_handle, &reclaim_keys[i]);
> Why do we need to lookup the entries?
>
> > +
> > + /* Try to put these elements into the defer queue*/
> > + for (size_t i = 0; i < total_entries / 2; i++)
> > + rte_hash_del_key(g_handle, &reclaim_keys[i]);
> > +
> > + /* Reader quiescent */
> > + rte_rcu_qsbr_quiescent(g_qsv, 0);
> > +
> > + status = rte_hash_add_key(g_handle, &reclaim_keys[0]);
> > + RETURN_IF_ERROR_RCU_QSBR(status < 0, "failed to add key (pos[%u]=%d)",
> 0, status);
> > +
> > + /* This should be (total_entries / 2) + 1 (last add) */
> > + unsigned int hash_size = rte_hash_count(g_handle);
> > +
> > + /* Freed size should be (total_entries / 2) */
> > + rte_hash_rcu_qsbr_dq_reclaim(g_handle, &freed, &pending, &available);
> > +
> > + rte_hash_free(g_handle);
> > + rte_free(g_qsv);
> > +
> > + if (hash_size != (total_entries / 2 + 1) || freed != (total_entries /
> 2)) {
> > + printf("Failed to reclaim defer queue\n");
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > /*
> > * Do all unit and performance tests.
> > */
> > @@ -2261,6 +2348,9 @@ test_hash(void)
> > if (test_hash_rcu_qsbr_sync_mode(1) < 0)
> > return -1;
> >
> > + if (test_hash_rcu_qsbr_dq_reclaim() < 0)
> > + return -1;
> > +
> > return 0;
> > }
> >
> > diff --git a/lib/hash/rte_cuckoo_hash.c b/lib/hash/rte_cuckoo_hash.c
> > index 9cf94645f6..52ccada12a 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, unsigned int
> *freed, unsigned int *pending,
> > + unsigned int *available)
> > +{
> > + int ret;
> > +
> > + if (h == NULL || h->hash_rcu_cfg == NULL) {
> > + HASH_LOG(ERR, "Invalid input parameter");
> > + rte_errno = EINVAL;
> > + return 1;
> > + }
> > +
> > + ret = rte_rcu_qsbr_dq_reclaim(h->dq,
> h->hash_rcu_cfg->max_reclaim_size, freed, pending,
> > +      available);
> > + 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..e1e0375fd5 100644
> > --- a/lib/hash/rte_hash.h
> > +++ b/lib/hash/rte_hash.h
> > @@ -674,6 +674,31 @@ 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.
> > + * @param freed
> > + *   Number of resources that were freed.
> > + * @param pending
> > + *   Number of resources pending on the defer queue.
> > + *   This number might not be accurate if multi-thread safety is
> configured.
> > + * @param available
> > + *   Number of resources that can be added to the defer queue.
> > + *   This number might not be accurate if multi-thread safety is
> configured.
> > + * @return
> > + *   On success - 0
> > + *   On error - 1 with error code set in rte_errno.
> > + *   Possible rte_errno codes are:
> > + *   - EINVAL - invalid pointer
> > + */
> > +__rte_experimental
> > +int rte_hash_rcu_qsbr_dq_reclaim(struct rte_hash *h, unsigned int
> *freed,
> > +    unsigned int *pending,
> > +    unsigned int *available);
> > +
> > #ifdef __cplusplus
> > }
> > #endif
> > diff --git a/lib/hash/version.map b/lib/hash/version.map
> > index 6f4bcdb71b..d348dd9196 100644
> > --- a/lib/hash/version.map
> > +++ b/lib/hash/version.map
> > @@ -53,3 +53,10 @@ INTERNAL {
> > rte_thash_gfni_stub;
> > rte_thash_gfni_bulk_stub;
> > };
> > +
> > +EXPERIMENTAL {
> > + global:
> > +
> > + # added in 24.07
> > + rte_hash_rcu_qsbr_dq_reclaim;
> > +};
> > --
> > 2.34.1
> >
>
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy the
> information in any medium. Thank you.
>

[-- Attachment #2: Type: text/html, Size: 9904 bytes --]