[dpdk-dev] hash: validate hash bucket entries while compiling

Message ID 20180531153049.41306-1-honnappa.nagarahalli@arm.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Honnappa Nagarahalli May 31, 2018, 3:30 p.m. UTC
  Validate RTE_HASH_BUCKET_ENTRIES during compilation instead of
run time.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
---
 lib/librte_eal/common/include/rte_common.h | 5 +++++
 lib/librte_hash/rte_cuckoo_hash.c          | 1 -
 lib/librte_hash/rte_cuckoo_hash.h          | 4 ++++
 3 files changed, 9 insertions(+), 1 deletion(-)
  

Comments

Thomas Monjalon July 12, 2018, 7:42 a.m. UTC | #1
Review please?

31/05/2018 17:30, Honnappa Nagarahalli:
> Validate RTE_HASH_BUCKET_ENTRIES during compilation instead of
> run time.
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> ---
>  lib/librte_eal/common/include/rte_common.h | 5 +++++
>  lib/librte_hash/rte_cuckoo_hash.c          | 1 -
>  lib/librte_hash/rte_cuckoo_hash.h          | 4 ++++
>  3 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
> index 434adfd45..a9df7c161 100644
> --- a/lib/librte_eal/common/include/rte_common.h
> +++ b/lib/librte_eal/common/include/rte_common.h
> @@ -293,6 +293,11 @@ rte_combine64ms1b(register uint64_t v)
>  
>  /*********** Macros to work with powers of 2 ********/
>  
> +/**
> + * Macro to return 1 if n is a power of 2, 0 otherwise
> + */
> +#define RTE_IS_POWER_OF_2(n) ((n) && !(((n) - 1) & (n)))
> +
>  /**
>   * Returns true if n is a power of 2
>   * @param n
> diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
> index a07543a29..375e7d208 100644
> --- a/lib/librte_hash/rte_cuckoo_hash.c
> +++ b/lib/librte_hash/rte_cuckoo_hash.c
> @@ -107,7 +107,6 @@ rte_hash_create(const struct rte_hash_parameters *params)
>  	/* Check for valid parameters */
>  	if ((params->entries > RTE_HASH_ENTRIES_MAX) ||
>  			(params->entries < RTE_HASH_BUCKET_ENTRIES) ||
> -			!rte_is_power_of_2(RTE_HASH_BUCKET_ENTRIES) ||
>  			(params->key_len == 0)) {
>  		rte_errno = EINVAL;
>  		RTE_LOG(ERR, HASH, "rte_hash_create has invalid parameters\n");
> diff --git a/lib/librte_hash/rte_cuckoo_hash.h b/lib/librte_hash/rte_cuckoo_hash.h
> index 7a54e5557..bd6ad1bd6 100644
> --- a/lib/librte_hash/rte_cuckoo_hash.h
> +++ b/lib/librte_hash/rte_cuckoo_hash.h
> @@ -97,6 +97,10 @@ enum add_key_case {
>  /** Number of items per bucket. */
>  #define RTE_HASH_BUCKET_ENTRIES		8
>  
> +#if !RTE_IS_POWER_OF_2(RTE_HASH_BUCKET_ENTRIES)
> +#error RTE_HASH_BUCKET_ENTRIES must be a power of 2
> +#endif
> +
>  #define NULL_SIGNATURE			0
>  
>  #define EMPTY_SLOT			0
>
  
De Lara Guarch, Pablo July 12, 2018, 8:05 a.m. UTC | #2
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, July 12, 2018 8:42 AM
> To: Richardson, Bruce <bruce.richardson@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>
> Cc: dev@dpdk.org; Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Subject: Re: [dpdk-dev] [PATCH] hash: validate hash bucket entries while
> compiling
> 
> Review please?
> 
> 31/05/2018 17:30, Honnappa Nagarahalli:
> > Validate RTE_HASH_BUCKET_ENTRIES during compilation instead of run
> > time.
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > ---

Acked-by: Pablo de Lara <Pablo.de.lara.guarch@intel.com>
  
Thomas Monjalon July 12, 2018, 10:43 a.m. UTC | #3
12/07/2018 10:05, De Lara Guarch, Pablo:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 
> > Review please?
> > 
> > 31/05/2018 17:30, Honnappa Nagarahalli:
> > > Validate RTE_HASH_BUCKET_ENTRIES during compilation instead of run
> > > time.
> > >
> > > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > > ---
> 
> Acked-by: Pablo de Lara <Pablo.de.lara.guarch@intel.com>

Applied, thanks
  
Honnappa Nagarahalli July 13, 2018, 8:20 p.m. UTC | #4
Hi Pablo,
Thank you for your response. I recently noticed this macro 'RTE_BUILD_BUG_ON'. Does it make sense to use this macro instead (though the error message will be more cryptic)?

Thank you,
Honnappa

-----Original Message-----
From: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
Sent: Thursday, July 12, 2018 3:05 AM
To: Thomas Monjalon <thomas@monjalon.net>; Richardson, Bruce <bruce.richardson@intel.com>
Cc: dev@dpdk.org; Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
Subject: RE: [dpdk-dev] [PATCH] hash: validate hash bucket entries while compiling



> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, July 12, 2018 8:42 AM
> To: Richardson, Bruce <bruce.richardson@intel.com>; De Lara Guarch,
> Pablo <pablo.de.lara.guarch@intel.com>
> Cc: dev@dpdk.org; Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Subject: Re: [dpdk-dev] [PATCH] hash: validate hash bucket entries
> while compiling
>
> Review please?
>
> 31/05/2018 17:30, Honnappa Nagarahalli:
> > Validate RTE_HASH_BUCKET_ENTRIES during compilation instead of run
> > time.
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > ---

Acked-by: Pablo de Lara <Pablo.de.lara.guarch@intel.com>


IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
  
Thomas Monjalon July 13, 2018, 9:40 p.m. UTC | #5
13/07/2018 22:20, Honnappa Nagarahalli:
> Hi Pablo,
> Thank you for your response. I recently noticed this macro 'RTE_BUILD_BUG_ON'. Does it make sense to use this macro instead (though the error message will be more cryptic)?

Yes, I've hesitated to suggest RTE_BUILD_BUG_ON,
but decided to push your #error solution which should be fine.
  

Patch

diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
index 434adfd45..a9df7c161 100644
--- a/lib/librte_eal/common/include/rte_common.h
+++ b/lib/librte_eal/common/include/rte_common.h
@@ -293,6 +293,11 @@  rte_combine64ms1b(register uint64_t v)
 
 /*********** Macros to work with powers of 2 ********/
 
+/**
+ * Macro to return 1 if n is a power of 2, 0 otherwise
+ */
+#define RTE_IS_POWER_OF_2(n) ((n) && !(((n) - 1) & (n)))
+
 /**
  * Returns true if n is a power of 2
  * @param n
diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index a07543a29..375e7d208 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -107,7 +107,6 @@  rte_hash_create(const struct rte_hash_parameters *params)
 	/* Check for valid parameters */
 	if ((params->entries > RTE_HASH_ENTRIES_MAX) ||
 			(params->entries < RTE_HASH_BUCKET_ENTRIES) ||
-			!rte_is_power_of_2(RTE_HASH_BUCKET_ENTRIES) ||
 			(params->key_len == 0)) {
 		rte_errno = EINVAL;
 		RTE_LOG(ERR, HASH, "rte_hash_create has invalid parameters\n");
diff --git a/lib/librte_hash/rte_cuckoo_hash.h b/lib/librte_hash/rte_cuckoo_hash.h
index 7a54e5557..bd6ad1bd6 100644
--- a/lib/librte_hash/rte_cuckoo_hash.h
+++ b/lib/librte_hash/rte_cuckoo_hash.h
@@ -97,6 +97,10 @@  enum add_key_case {
 /** Number of items per bucket. */
 #define RTE_HASH_BUCKET_ENTRIES		8
 
+#if !RTE_IS_POWER_OF_2(RTE_HASH_BUCKET_ENTRIES)
+#error RTE_HASH_BUCKET_ENTRIES must be a power of 2
+#endif
+
 #define NULL_SIGNATURE			0
 
 #define EMPTY_SLOT			0