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

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

[Kumar Amber]

Adding new API function to query the maximum key ID
that could possibly returned by add_key_functions. When
multi_key_add flag 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.
+ *
+ * @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.
+ */
+__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;
 
 };
-- 
2.17.1

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

[Kumar Amber]

Adding new API function to query the maximum key ID
that could possibly returned by rte_hash_add_key and
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.
+ *
+ * @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.
+ */
+__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;
 
 };
-- 
2.17.1

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

[Kumar Amber]

Adding new API function to query the maximum key ID
that could possibly returned by rte_hash_add_key and
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.
+ *
+ * @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.
+ */
+__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;
 
 };
-- 
2.17.1

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

[Amber, Kumar]

Hi all ,

I want to report random untouched unit test cases failed by Jenkins . pls can you guys check why it is unstable ? 
Every upload shows random test failed .

http://mails.dpdk.org/archives/test-report/2019-November/109120.html 

Regards
Amber 

-----Original Message-----
From: dev <dev-bounces@dpdk.org> On Behalf Of Kumar Amber
Sent: Monday, November 25, 2019 4:38 PM
To: dev@dpdk.org
Cc: Wang, Yipeng1 <yipeng1.wang@intel.com>
Subject: [dpdk-dev] [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 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.
+ *
+ * @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.
+ */
+__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;
 
 };
--
2.17.1

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

[Aaron Conole]

"Amber, Kumar" <kumar.amber@intel.com> writes:

> Hi all ,
>
> I want to report random untouched unit test cases failed by Jenkins
> . pls can you guys check why it is unstable ?
> Every upload shows random test failed .

I'll work on the service_autotest patch.

> http://mails.dpdk.org/archives/test-report/2019-November/109120.html 
>
> Regards
> Amber 
>
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Kumar Amber
> Sent: Monday, November 25, 2019 4:38 PM
> To: dev@dpdk.org
> Cc: Wang, Yipeng1 <yipeng1.wang@intel.com>
> Subject: [dpdk-dev] [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 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.
> + *
> + * @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.
> + */
> +__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;
>  
>  };
> --
> 2.17.1

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

[Aaron Conole]

"Amber, Kumar" <kumar.amber@intel.com> writes:

> Hi all ,
>
> I want to report random untouched unit test cases failed by Jenkins
> . pls can you guys check why it is unstable ?
> Every upload shows random test failed .
>
> http://mails.dpdk.org/archives/test-report/2019-November/109120.html 

I think the service core code needs the following patch:
---
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index 79235c03f8..f2efb8c4a5 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -383,7 +383,7 @@ rte_service_may_be_active(uint32_t id)
 	int32_t lcore_count = rte_service_lcore_list(ids, RTE_MAX_LCORE);
 	int i;
 
-	if (!service_valid(id))
+	if (id >= RTE_SERVICE_NUM_MAX || !service_valid(id))
 		return -EINVAL;
 
 	for (i = 0; i < lcore_count; i++) {
@@ -398,7 +398,7 @@ int32_t
 rte_service_run_iter_on_app_lcore(uint32_t id, uint32_t serialize_mt_unsafe)
 {
 	/* run service on calling core, using all-ones as the service mask */
-	if (!service_valid(id))
+	if (id >= RTE_SERVICE_NUM_MAX || !service_valid(id))
 		return -EINVAL;
 
 	struct core_state *cs = &lcore_states[rte_lcore_id()];
---

Harry?

> Regards
> Amber 
>
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Kumar Amber
> Sent: Monday, November 25, 2019 4:38 PM
> To: dev@dpdk.org
> Cc: Wang, Yipeng1 <yipeng1.wang@intel.com>
> Subject: [dpdk-dev] [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 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.
> + *
> + * @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.
> + */
> +__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;
>  
>  };
> --
> 2.17.1

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

[Van Haaren, Harry]

Hey Folks,

> -----Original Message-----
> From: Aaron Conole <aconole@redhat.com>
> Sent: Monday, November 25, 2019 4:59 PM
> To: Amber, Kumar <kumar.amber@intel.com>
> Cc: dev@dpdk.org; Wang, Yipeng1 <yipeng1.wang@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; thomas@monjalon.net; Van Haaren, Harry
> <harry.van.haaren@intel.com>; Thakur, Sham Singh
> <sham.singh.thakur@intel.com>; David Marchand <dmarchan@redhat.com>
> Subject: Re: [dpdk-dev] [PATCH v3] hash: added a new API to hash to query
> key id
> 
> "Amber, Kumar" <kumar.amber@intel.com> writes:
> 
> > Hi all ,
> >
> > I want to report random untouched unit test cases failed by Jenkins
> > . pls can you guys check why it is unstable ?
> > Every upload shows random test failed .
> >
> > http://mails.dpdk.org/archives/test-report/2019-November/109120.html
> 
> I think the service core code needs the following patch:
> ---
> diff --git a/lib/librte_eal/common/rte_service.c
> b/lib/librte_eal/common/rte_service.c
> index 79235c03f8..f2efb8c4a5 100644
> --- a/lib/librte_eal/common/rte_service.c
> +++ b/lib/librte_eal/common/rte_service.c
> @@ -383,7 +383,7 @@ rte_service_may_be_active(uint32_t id)
>  	int32_t lcore_count = rte_service_lcore_list(ids, RTE_MAX_LCORE);
>  	int i;
> 
> -	if (!service_valid(id))
> +	if (id >= RTE_SERVICE_NUM_MAX || !service_valid(id))
>  		return -EINVAL;
> 
>  	for (i = 0; i < lcore_count; i++) {
> @@ -398,7 +398,7 @@ int32_t
>  rte_service_run_iter_on_app_lcore(uint32_t id, uint32_t
> serialize_mt_unsafe)
>  {
>  	/* run service on calling core, using all-ones as the service mask */
> -	if (!service_valid(id))
> +	if (id >= RTE_SERVICE_NUM_MAX || !service_valid(id))
>  		return -EINVAL;
> 
>  	struct core_state *cs = &lcore_states[rte_lcore_id()];
> ---
> 
> Harry?

Apologies - I've been AFK and hence this made no progress in the last weeks.
Thanks Aaron for the suggested patch, I'll review/test now and report back tomorrow.


<snip unrelated patch code>

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

[Thomas Monjalon]

> From: Aaron Conole <aconole@redhat.com>
> > -	if (!service_valid(id))
> > +	if (id >= RTE_SERVICE_NUM_MAX || !service_valid(id))

Why not adding this check in service_valid()?

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

[Aaron Conole]

Thomas Monjalon <thomas@monjalon.net> writes:

>> From: Aaron Conole <aconole@redhat.com>
>> > -	if (!service_valid(id))
>> > +	if (id >= RTE_SERVICE_NUM_MAX || !service_valid(id))
>
> Why not adding this check in service_valid()?

I think the best fix is to use SERVICE_VALID_GET_OR_ERR_RET() in these
places.  For this, I at least want to try and show that there aren't any
further errors.  And my test loop has been running for a while now
without any more errors or segfaults, so I guess it's okay to build a
proper patch.

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

[Wang, Yipeng1]

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.

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

[Aaron Conole]

Aaron Conole <aconole@redhat.com> writes:

> Thomas Monjalon <thomas@monjalon.net> writes:
>
>>> From: Aaron Conole <aconole@redhat.com>
>>> > -	if (!service_valid(id))
>>> > +	if (id >= RTE_SERVICE_NUM_MAX || !service_valid(id))
>>
>> Why not adding this check in service_valid()?
>
> I think the best fix is to use SERVICE_VALID_GET_OR_ERR_RET() in these
> places.  For this, I at least want to try and show that there aren't any
> further errors.  And my test loop has been running for a while now
> without any more errors or segfaults, so I guess it's okay to build a
> proper patch.

This popped up:

EAL: Test assert service_lcore_en_dis_able line 487 failed: Ex-service core function call had no effect.

So I'll spend some time in this area, it seems.

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

[Kumar Amber]

Adding new API function to query the maximum key ID
that could possibly be returned by rte_hash_add_key and
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..41a081f10 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);
 }
 
+int32_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..f93203122 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.
+ *
+ * @param h
+ *  Hash table to query from
+ * @return
+ *   - -EINVAL if parameters are invalid
+ *   - A value indicating the max key of Id key slots present in the table.
+ */
+__rte_experimental
+int32_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..f6de2d8a2 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;
 
 };
-- 
2.17.1

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

[Van Haaren, Harry]

Hi Aaron,

> -----Original Message-----
> From: Aaron Conole <aconole@redhat.com>
> Sent: Monday, November 25, 2019 10:54 PM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Amber, Kumar
> <kumar.amber@intel.com>; dev@dpdk.org; Wang, Yipeng1
> <yipeng1.wang@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>; Thakur,
> Sham Singh <sham.singh.thakur@intel.com>; David Marchand
> <dmarchan@redhat.com>
> Subject: Re: [dpdk-dev] [PATCH v3] hash: added a new API to hash to query
> key id
> 
> Aaron Conole <aconole@redhat.com> writes:
> 
> > Thomas Monjalon <thomas@monjalon.net> writes:
> >
> >>> From: Aaron Conole <aconole@redhat.com>
> >>> > -	if (!service_valid(id))
> >>> > +	if (id >= RTE_SERVICE_NUM_MAX || !service_valid(id))
> >>
> >> Why not adding this check in service_valid()?
> >
> > I think the best fix is to use SERVICE_VALID_GET_OR_ERR_RET() in these
> > places.  For this, I at least want to try and show that there aren't any
> > further errors.  And my test loop has been running for a while now
> > without any more errors or segfaults, so I guess it's okay to build a
> > proper patch.
> 
> This popped up:
> 
> EAL: Test assert service_lcore_en_dis_able line 487 failed: Ex-service core
> function call had no effect.
> 
> So I'll spend some time in this area, it seems.


The below diff makes it 100% reproducible here, failing every time.

It seems like the main thread is returning, before the service thread has returned.

The rte_eal_mp_wait_lcore() call seems to not wait on the service-core, which allows
the main thread to read the "service_remote_launch_flag" value as 0 (before the service-thread writes it to 1).

Adding the delay between the service launch and service write being performed makes this issue much much more likely to occur - so the above description I have confidence in.

What I'm not clear on (yet) is why the eal_mp_wait_lcore() isn't waiting...

-H


diff --git a/app/test/test_service_cores.c b/app/test/test_service_cores.c
index 9fe38f5e0..846ad00d1 100644
--- a/app/test/test_service_cores.c
+++ b/app/test/test_service_cores.c
@@ -445,6 +445,7 @@ static int
 service_remote_launch_func(void *arg)
 {
        RTE_SET_USED(arg);
+       rte_delay_ms(100);
        service_remote_launch_flag = 1;
        return 0;
 }

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

[Van Haaren, Harry]

> -----Original Message-----
> From: Van Haaren, Harry
> Sent: Tuesday, November 26, 2019 1:19 PM
> To: Aaron Conole <aconole@redhat.com>; Thomas Monjalon <thomas@monjalon.net>

<snip>

> > EAL: Test assert service_lcore_en_dis_able line 487 failed: Ex-service
> core
> > function call had no effect.
> >
> > So I'll spend some time in this area, it seems.
> 
> 
> The below diff makes it 100% reproducible here, failing every time.
> 
> It seems like the main thread is returning, before the service thread has
> returned.
> 
> The rte_eal_mp_wait_lcore() call seems to not wait on the service-core,
> which allows
> the main thread to read the "service_remote_launch_flag" value as 0 (before
> the service-thread writes it to 1).
> 
> Adding the delay between the service launch and service write being
> performed makes this issue much much more likely to occur - so the above
> description I have confidence in.
> 
> What I'm not clear on (yet) is why the eal_mp_wait_lcore() isn't waiting...
> 
> -H
> 
> 
> diff --git a/app/test/test_service_cores.c b/app/test/test_service_cores.c
> index 9fe38f5e0..846ad00d1 100644
> --- a/app/test/test_service_cores.c
> +++ b/app/test/test_service_cores.c
> @@ -445,6 +445,7 @@ static int
>  service_remote_launch_func(void *arg)
>  {
>         RTE_SET_USED(arg);
> +       rte_delay_ms(100);
>         service_remote_launch_flag = 1;
>         return 0;
>  }

Diff below seems to fix the problem here; Aaron would you test the below fix in your setup for a while too?
I have a loop running here attempting to reproduce - but before 100% failures and so far 100% passes with the added wait_lcore() call.


diff --git a/app/test/test_service_cores.c b/app/test/test_service_cores.c
index 9fe38f5e0..62ffedb19 100644
--- a/app/test/test_service_cores.c
+++ b/app/test/test_service_cores.c
@@ -445,6 +445,7 @@ static int
 service_remote_launch_func(void *arg)
 {
        RTE_SET_USED(arg);
+       rte_delay_ms(100);
        service_remote_launch_flag = 1;
        return 0;
 }
@@ -483,6 +484,7 @@ service_lcore_en_dis_able(void)
        int ret = rte_eal_remote_launch(service_remote_launch_func, NULL,
                                        slcore_id);
        TEST_ASSERT_EQUAL(0, ret, "Ex-service core remote launch failed.");
+       rte_eal_wait_lcore(slcore_id);
        rte_eal_mp_wait_lcore();
        TEST_ASSERT_EQUAL(1, service_remote_launch_flag,
                        "Ex-service core function call had no effect.");

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

[Aaron Conole]

"Van Haaren, Harry" <harry.van.haaren@intel.com> writes:

>> -----Original Message-----
>> From: Van Haaren, Harry
>> Sent: Tuesday, November 26, 2019 1:19 PM
>> To: Aaron Conole <aconole@redhat.com>; Thomas Monjalon <thomas@monjalon.net>
>
> <snip>
>
>> > EAL: Test assert service_lcore_en_dis_able line 487 failed: Ex-service
>> core
>> > function call had no effect.
>> >
>> > So I'll spend some time in this area, it seems.
>> 
>> 
>> The below diff makes it 100% reproducible here, failing every time.
>> 
>> It seems like the main thread is returning, before the service thread has
>> returned.
>> 
>> The rte_eal_mp_wait_lcore() call seems to not wait on the service-core,
>> which allows
>> the main thread to read the "service_remote_launch_flag" value as 0 (before
>> the service-thread writes it to 1).
>> 
>> Adding the delay between the service launch and service write being
>> performed makes this issue much much more likely to occur - so the above
>> description I have confidence in.
>> 
>> What I'm not clear on (yet) is why the eal_mp_wait_lcore() isn't waiting...
>> 
>> -H
>> 
>> 
>> diff --git a/app/test/test_service_cores.c b/app/test/test_service_cores.c
>> index 9fe38f5e0..846ad00d1 100644
>> --- a/app/test/test_service_cores.c
>> +++ b/app/test/test_service_cores.c
>> @@ -445,6 +445,7 @@ static int
>>  service_remote_launch_func(void *arg)
>>  {
>>         RTE_SET_USED(arg);
>> +       rte_delay_ms(100);
>>         service_remote_launch_flag = 1;
>>         return 0;
>>  }
>
> Diff below seems to fix the problem here; Aaron would you test the below fix in your setup for a while too?
> I have a loop running here attempting to reproduce - but before 100% failures and so far 100% passes with the added wait_lcore() call.
>
>
> diff --git a/app/test/test_service_cores.c b/app/test/test_service_cores.c
> index 9fe38f5e0..62ffedb19 100644
> --- a/app/test/test_service_cores.c
> +++ b/app/test/test_service_cores.c
> @@ -445,6 +445,7 @@ static int
>  service_remote_launch_func(void *arg)
>  {
>         RTE_SET_USED(arg);
> +       rte_delay_ms(100);
>         service_remote_launch_flag = 1;
>         return 0;
>  }
> @@ -483,6 +484,7 @@ service_lcore_en_dis_able(void)
>         int ret = rte_eal_remote_launch(service_remote_launch_func, NULL,
>                                         slcore_id);
>         TEST_ASSERT_EQUAL(0, ret, "Ex-service core remote launch failed.");
> +       rte_eal_wait_lcore(slcore_id);
>         rte_eal_mp_wait_lcore();

Ahh, I see.  Actually, this brings up a question - is the intent for
mp_wait_lcore to cycle through the service cores as well?  Because IIUC,
the issue will be the lcore will be set to ROLE_RTE normally, but
service cores will do: ROLE_SERVICE and then the wait cannot work.

If the idea is that mp_wait_lcore should work (and looking at the test,
it seems like it is the intent?) then it will need to cycle through
service cores, too.  If the intent is that it shouldn't, then we should
remove those calls from the test application to prevent developer from
misunderstanding.

Either way, the documentation for `rte_service_lcore_start` is a bit too
ambiguous and needs to reflect whether the mp_wait_lcore should work.  I
think either it should (which means updating rte_get_next_lcore to
include ROLE_SERVICE), or none of the lcore functions should work, and
we should have an rte_service...() equivalent that should be used.

>         TEST_ASSERT_EQUAL(1, service_remote_launch_flag,
>                         "Ex-service core function call had no effect.");

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

[Aaron Conole]

"Van Haaren, Harry" <harry.van.haaren@intel.com> writes:

> Hi Aaron,
>
>> -----Original Message-----
>> From: Aaron Conole <aconole@redhat.com>
>> Sent: Monday, November 25, 2019 10:54 PM
>> To: Thomas Monjalon <thomas@monjalon.net>
>> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Amber, Kumar
>> <kumar.amber@intel.com>; dev@dpdk.org; Wang, Yipeng1
>> <yipeng1.wang@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>; Thakur,
>> Sham Singh <sham.singh.thakur@intel.com>; David Marchand
>> <dmarchan@redhat.com>
>> Subject: Re: [dpdk-dev] [PATCH v3] hash: added a new API to hash to query
>> key id
>> 
>> Aaron Conole <aconole@redhat.com> writes:
>> 
>> > Thomas Monjalon <thomas@monjalon.net> writes:
>> >
>> >>> From: Aaron Conole <aconole@redhat.com>
>> >>> > -	if (!service_valid(id))
>> >>> > +	if (id >= RTE_SERVICE_NUM_MAX || !service_valid(id))
>> >>
>> >> Why not adding this check in service_valid()?
>> >
>> > I think the best fix is to use SERVICE_VALID_GET_OR_ERR_RET() in these
>> > places.  For this, I at least want to try and show that there aren't any
>> > further errors.  And my test loop has been running for a while now
>> > without any more errors or segfaults, so I guess it's okay to build a
>> > proper patch.
>> 
>> This popped up:
>> 
>> EAL: Test assert service_lcore_en_dis_able line 487 failed: Ex-service core
>> function call had no effect.
>> 
>> So I'll spend some time in this area, it seems.
>
>
> The below diff makes it 100% reproducible here, failing every time.
>
> It seems like the main thread is returning, before the service thread has returned.
>
> The rte_eal_mp_wait_lcore() call seems to not wait on the service-core, which allows
> the main thread to read the "service_remote_launch_flag" value as 0 (before the service-thread writes it to 1).
>
> Adding the delay between the service launch and service write being performed makes this issue much much more likely to occur - so the above description I have confidence in.
>
> What I'm not clear on (yet) is why the eal_mp_wait_lcore() isn't waiting...

As I wrote in the other thread, it's because eal_mp_wait_lcore won't
look at lcores with ROLE_SERVICE.

> -H

I've been running something similar to the suggested patch for 24
minutes now with no failure.  I've also removed the eal_mp_wait_lcore()
call in other areas throughout the test and switched to individual core
waiting "just in case."  I don't think it's the right fix, though.

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

[Wang, Yipeng1]

>-----Original Message-----
>From: Amber, Kumar
>Sent: Monday, November 25, 2019 6:39 PM
>To: dev@dpdk.org
>Cc: Wang, Yipeng1 <yipeng1.wang@intel.com>
>Subject: [PATCH v4] hash: added a new API to hash to query key id
>
>Adding new API function to query the maximum key ID
>that could possibly be returned by rte_hash_add_key and
>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..41a081f10 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);
> }
>
>+int32_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..f93203122 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.
>+ *
>+ * @param h
>+ *  Hash table to query from
>+ * @return
>+ *   - -EINVAL if parameters are invalid
>+ *   - A value indicating the max key of Id key slots present in the table.
>+ */
[Wang, Yipeng] typo: key of id -> key Id of

Other than this:
Acked-by: Yipeng Wang <yipeng1.wang@intel.com>

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

[Van Haaren, Harry]

> -----Original Message-----
> From: Aaron Conole <aconole@redhat.com>
> Sent: Tuesday, November 26, 2019 1:57 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; Amber, Kumar
> <kumar.amber@intel.com>; dev@dpdk.org; Wang, Yipeng1
> <yipeng1.wang@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>; Thakur,
> Sham Singh <sham.singh.thakur@intel.com>; David Marchand
> <dmarchan@redhat.com>
> Subject: Re: [dpdk-dev] [PATCH v3] hash: added a new API to hash to query
> key id
> 
> "Van Haaren, Harry" <harry.van.haaren@intel.com> writes:
> 
> >> -----Original Message-----
> >> From: Van Haaren, Harry
> >> Sent: Tuesday, November 26, 2019 1:19 PM
> >> To: Aaron Conole <aconole@redhat.com>; Thomas Monjalon
> <thomas@monjalon.net>
> >
> > <snip>
> >
> >> > EAL: Test assert service_lcore_en_dis_able line 487 failed: Ex-service
> >> core
> >> > function call had no effect.
> >> >
> >> > So I'll spend some time in this area, it seems.
> >>
> >>
> >> The below diff makes it 100% reproducible here, failing every time.
> >>
> >> It seems like the main thread is returning, before the service thread has
> >> returned.
> >>
> >> The rte_eal_mp_wait_lcore() call seems to not wait on the service-core,
> >> which allows
> >> the main thread to read the "service_remote_launch_flag" value as 0
> (before
> >> the service-thread writes it to 1).
> >>
> >> Adding the delay between the service launch and service write being
> >> performed makes this issue much much more likely to occur - so the above
> >> description I have confidence in.
> >>
> >> What I'm not clear on (yet) is why the eal_mp_wait_lcore() isn't
> waiting...
> >>
> >> -H
> >>
> >>
> >> diff --git a/app/test/test_service_cores.c
> b/app/test/test_service_cores.c
> >> index 9fe38f5e0..846ad00d1 100644
> >> --- a/app/test/test_service_cores.c
> >> +++ b/app/test/test_service_cores.c
> >> @@ -445,6 +445,7 @@ static int
> >>  service_remote_launch_func(void *arg)
> >>  {
> >>         RTE_SET_USED(arg);
> >> +       rte_delay_ms(100);
> >>         service_remote_launch_flag = 1;
> >>         return 0;
> >>  }
> >
> > Diff below seems to fix the problem here; Aaron would you test the below
> fix in your setup for a while too?
> > I have a loop running here attempting to reproduce - but before 100%
> failures and so far 100% passes with the added wait_lcore() call.
> >
> >
> > diff --git a/app/test/test_service_cores.c b/app/test/test_service_cores.c
> > index 9fe38f5e0..62ffedb19 100644
> > --- a/app/test/test_service_cores.c
> > +++ b/app/test/test_service_cores.c
> > @@ -445,6 +445,7 @@ static int
> >  service_remote_launch_func(void *arg)
> >  {
> >         RTE_SET_USED(arg);
> > +       rte_delay_ms(100);
> >         service_remote_launch_flag = 1;
> >         return 0;
> >  }
> > @@ -483,6 +484,7 @@ service_lcore_en_dis_able(void)
> >         int ret = rte_eal_remote_launch(service_remote_launch_func, NULL,
> >                                         slcore_id);
> >         TEST_ASSERT_EQUAL(0, ret, "Ex-service core remote launch
> failed.");
> > +       rte_eal_wait_lcore(slcore_id);
> >         rte_eal_mp_wait_lcore();
> 
> Ahh, I see.  Actually, this brings up a question - is the intent for
> mp_wait_lcore to cycle through the service cores as well?  Because IIUC,
> the issue will be the lcore will be set to ROLE_RTE normally, but
> service cores will do: ROLE_SERVICE and then the wait cannot work.
> 
> If the idea is that mp_wait_lcore should work (and looking at the test,
> it seems like it is the intent?) then it will need to cycle through
> service cores, too.  If the intent is that it shouldn't, then we should
> remove those calls from the test application to prevent developer from
> misunderstanding.
> 
> Either way, the documentation for `rte_service_lcore_start` is a bit too
> ambiguous and needs to reflect whether the mp_wait_lcore should work.  I
> think either it should (which means updating rte_get_next_lcore to
> include ROLE_SERVICE), or none of the lcore functions should work, and
> we should have an rte_service...() equivalent that should be used.


Service cores are meant to be transparent to the application.
The test application testing this particular usage is the corner-case.

The rte_eal_mp_wait_lcore() is correct to NOT wait for service-cores,
as they are not always under application control.

The observed test failure is a bug in the test code, it should use the explicit
rte_eal_wait_lcore() call. I'll send a patch later today.

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

[Thomas Monjalon]

27/11/2019 02:59, Wang, Yipeng1:
> From: Amber, Kumar
> >Adding new API function to query the maximum key ID
> >that could possibly be returned by rte_hash_add_key and
> >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>
> >---
> >+ *   - A value indicating the max key of Id key slots present in the table.
> >+ */
> [Wang, Yipeng] typo: key of id -> key Id of
> 
> Other than this:
> Acked-by: Yipeng Wang <yipeng1.wang@intel.com>

Applied with typo fixed, thanks