[v3] hash: added a new API to hash to query key id
Checks
Commit Message
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(-)
Comments
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
"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
"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
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>
> 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()?
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.
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.
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.
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;
}
> -----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.");
"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.");
"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.
> -----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.
@@ -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)
{
@@ -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);
@@ -58,5 +58,6 @@ EXPERIMENTAL {
global:
rte_hash_free_key_with_position;
+ rte_hash_max_key_id;
};