Re: [dpdk-dev] [PATCH v3] hash: added a new API to hash to query key id

["Wang, Yipeng1" <yipeng1.wang@intel.com>]

Hi, Amber, Thanks for the patch! A bit comment inlined:

>-----Original Message-----
>From: Amber, Kumar
>Sent: Monday, November 25, 2019 3:08 AM
>To: dev@dpdk.org
>Cc: Wang, Yipeng1 <yipeng1.wang@intel.com>
>Subject: [PATCH v3] hash: added a new API to hash to query key id
>
>Adding new API function to query the maximum key ID
>that could possibly returned by rte_hash_add_key and
 [Wang, Yipeng]  That could possibly "be" returned... 

>rte_hash_add_key_with_hash. When RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD
>is set, the maximum key id is larger than the entry count specified
>by the user.
>
>Signed-off-by: Kumar Amber <kumar.amber@intel.com>
>---
> lib/librte_hash/rte_cuckoo_hash.c    | 15 +++++++++++++++
> lib/librte_hash/rte_hash.h           | 25 +++++++++++++++++++++++--
> lib/librte_hash/rte_hash_version.map |  1 +
> 3 files changed, 39 insertions(+), 2 deletions(-)
>
>diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
>index 87a4c01f2..3a94f10b8 100644
>--- a/lib/librte_hash/rte_cuckoo_hash.c
>+++ b/lib/librte_hash/rte_cuckoo_hash.c
>@@ -506,6 +506,21 @@ rte_hash_hash(const struct rte_hash *h, const void *key)
> 	return h->hash_func(key, h->key_len, h->hash_func_init_val);
> }
>
>+uint32_t
>+rte_hash_max_key_id(const struct rte_hash *h)
>+{
>+	RETURN_IF_TRUE((h == NULL), -EINVAL);
>+	if (h->use_local_cache)
>+		/*
>+		 * Increase number of slots by total number of indices
>+		 * that can be stored in the lcore caches
>+		 */
>+		return (h->entries + ((RTE_MAX_LCORE - 1) *
>+					(LCORE_CACHE_SIZE - 1)));
>+	else
>+		return h->entries;
>+}
>+
> int32_t
> rte_hash_count(const struct rte_hash *h)
> {
>diff --git a/lib/librte_hash/rte_hash.h b/lib/librte_hash/rte_hash.h
>index 0d73370dc..c87861e72 100644
>--- a/lib/librte_hash/rte_hash.h
>+++ b/lib/librte_hash/rte_hash.h
>@@ -164,6 +164,23 @@ rte_hash_reset(struct rte_hash *h);
> int32_t
> rte_hash_count(const struct rte_hash *h);
>
>+/**
>+ * @warning
>+ * @b EXPERIMENTAL: this API may change without prior notice
>+ *
>+ * Return  the maximum key value ID that could possibly be returned by
>+ * rte_hash_add_key function.
[Wang, Yipeng] Seems you have two spaces between "Return" and "the", change to one.
>+ *
>+ * @param h
>+ *  Hash table to query from
>+ * @return
>+ *   - -EINVAL if parameters are invalid
>+ *   - A value indicating the max key Id  key slots present in the table.
[Wang, Yipeng] Two spaces between "Id" and "key". Maybe you want to say key Id "of" key slots?
>+ */
>+__rte_experimental
>+uint32_t
>+rte_hash_max_key_id(const struct rte_hash *h);
>+
> /**
>  * Add a key-value pair to an existing hash table.
>  * This operation is not multi-thread safe
>@@ -234,7 +251,9 @@ rte_hash_add_key_with_hash_data(const struct rte_hash *h, const void *key,
>  *   - -EINVAL if the parameters are invalid.
>  *   - -ENOSPC if there is no space in the hash for this key.
>  *   - A positive value that can be used by the caller as an offset into an
>- *     array of user data. This value is unique for this key.
>+ *     array of user data. This value is unique for this key. This
>+ *     unique key id may be larger than the user specified entry count
>+ *     when RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD flag is set.
>  */
> int32_t
> rte_hash_add_key(const struct rte_hash *h, const void *key);
>@@ -256,7 +275,9 @@ rte_hash_add_key(const struct rte_hash *h, const void *key);
>  *   - -EINVAL if the parameters are invalid.
>  *   - -ENOSPC if there is no space in the hash for this key.
>  *   - A positive value that can be used by the caller as an offset into an
>- *     array of user data. This value is unique for this key.
>+ *     array of user data. This value is unique for this key. This
>+ *     unique key ID may be larger than the user specified entry count
>+ *     when RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD flag is set.
>  */
> int32_t
> rte_hash_add_key_with_hash(const struct rte_hash *h, const void *key, hash_sig_t sig);
>diff --git a/lib/librte_hash/rte_hash_version.map b/lib/librte_hash/rte_hash_version.map
>index 734ae28b0..562ceb8bc 100644
>--- a/lib/librte_hash/rte_hash_version.map
>+++ b/lib/librte_hash/rte_hash_version.map
>@@ -58,5 +58,6 @@ EXPERIMENTAL {
> 	global:
>
> 	rte_hash_free_key_with_position;
>+    rte_hash_max_key_id;
>
[Wang, Yipeng] Fix the Indentation of this line.

[Wang, Yipeng] 
I also realized that rte_add_key returns int32_t but the ID could potentially be uint32_t.
But by default, the entry ID should never overflow the range.
Let's just convert the new API's return to int32_t to be consistent for now.