[dpdk-dev] [PATCH] hash: fix to support multi process

[dpdk-dev] [PATCH] hash: fix to support multi process

[Pablo de Lara]

Hash library used a function pointer to choose a different
key compare function, depending on the key size.
As a result, multiple processes could not use the same hash table,
as the function addresses vary from one process to another.

Instead, a jump table is used, so each process has its own
function addresses, accessing this table with an index stored
in the hash table (note that using a custom key compare function
is not supported in multi-process mode).

Fixes: 48a399119619 ("hash: replace with cuckoo hash implementation")

Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
---
 doc/guides/rel_notes/release_16_04.rst |  5 ++
 lib/librte_hash/rte_cuckoo_hash.c      | 85 ++++++++++++++++++++++++++--------
 2 files changed, 71 insertions(+), 19 deletions(-)

diff --git a/doc/guides/rel_notes/release_16_04.rst b/doc/guides/rel_notes/release_16_04.rst
index 2785b29..c4359dd 100644
--- a/doc/guides/rel_notes/release_16_04.rst
+++ b/doc/guides/rel_notes/release_16_04.rst
@@ -351,6 +351,11 @@ Libraries
   Fix crc32c hash functions to return a valid crc32c value for data lengths
   not multiple of 4 bytes.
 
+* **hash: Fixed hash library to support multi-process mode.**
+
+  Fix hash library to support multi-process mode, using a jump table,
+  instead of storing a function pointer to the key compare function.
+
 * **librte_port: Fixed segmentation fault for ring and ethdev writer nodrop.**
 
   Fixed core dump issue on txq and swq when dropless is set to yes.
diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index 71b5b76..38c19ab 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -102,6 +102,41 @@ EAL_REGISTER_TAILQ(rte_hash_tailq)
 
 #define LCORE_CACHE_SIZE		8
 
+/*
+ * All different options to select a key compare function,
+ * based on the key size and custom function.
+ */
+enum cmp_jump_table_case {
+	KEY_CUSTOM = 0,
+	KEY_16_BYTES,
+	KEY_32_BYTES,
+	KEY_48_BYTES,
+	KEY_64_BYTES,
+	KEY_80_BYTES,
+	KEY_96_BYTES,
+	KEY_112_BYTES,
+	KEY_128_BYTES,
+	KEY_OTHER_BYTES,
+	NUM_KEY_CMP_CASES,
+};
+
+/*
+ * Table storing all different key compare functions
+ * (multi-process supported)
+ */
+rte_hash_cmp_eq_t cmp_jump_table[NUM_KEY_CMP_CASES] = {
+	NULL,
+	rte_hash_k16_cmp_eq,
+	rte_hash_k32_cmp_eq,
+	rte_hash_k48_cmp_eq,
+	rte_hash_k64_cmp_eq,
+	rte_hash_k80_cmp_eq,
+	rte_hash_k96_cmp_eq,
+	rte_hash_k112_cmp_eq,
+	rte_hash_k128_cmp_eq,
+	memcmp
+};
+
 struct lcore_cache {
 	unsigned len; /**< Cache len */
 	void *objs[LCORE_CACHE_SIZE]; /**< Cache objects */
@@ -115,7 +150,10 @@ struct rte_hash {
 	uint32_t key_len;               /**< Length of hash key. */
 	rte_hash_function hash_func;    /**< Function used to calculate hash. */
 	uint32_t hash_func_init_val;    /**< Init value used by hash_func. */
-	rte_hash_cmp_eq_t rte_hash_cmp_eq; /**< Function used to compare keys. */
+	rte_hash_cmp_eq_t rte_hash_custom_cmp_eq;
+	/**< Custom function used to compare keys. */
+	enum cmp_jump_table_case cmp_jump_table_idx;
+	/**< Indicates which compare function to use. */
 	uint32_t bucket_bitmask;        /**< Bitmask for getting bucket index
 						from hash signature. */
 	uint32_t key_entry_size;         /**< Size of each key entry. */
@@ -187,7 +225,16 @@ rte_hash_find_existing(const char *name)
 
 void rte_hash_set_cmp_func(struct rte_hash *h, rte_hash_cmp_eq_t func)
 {
-	h->rte_hash_cmp_eq = func;
+	h->rte_hash_custom_cmp_eq = func;
+}
+
+static inline int
+rte_hash_cmp_eq(const void *key1, const void *key2, const struct rte_hash *h)
+{
+	if (h->cmp_jump_table_idx == KEY_CUSTOM)
+		return h->rte_hash_custom_cmp_eq(key1, key2, h->key_len);
+	else
+		return cmp_jump_table[h->cmp_jump_table_idx](key1, key2, h->key_len);
 }
 
 struct rte_hash *
@@ -292,35 +339,35 @@ rte_hash_create(const struct rte_hash_parameters *params)
 	/* Select function to compare keys */
 	switch (params->key_len) {
 	case 16:
-		h->rte_hash_cmp_eq = rte_hash_k16_cmp_eq;
+		h->cmp_jump_table_idx = KEY_16_BYTES;
 		break;
 	case 32:
-		h->rte_hash_cmp_eq = rte_hash_k32_cmp_eq;
+		h->cmp_jump_table_idx = KEY_32_BYTES;
 		break;
 	case 48:
-		h->rte_hash_cmp_eq = rte_hash_k48_cmp_eq;
+		h->cmp_jump_table_idx = KEY_48_BYTES;
 		break;
 	case 64:
-		h->rte_hash_cmp_eq = rte_hash_k64_cmp_eq;
+		h->cmp_jump_table_idx = KEY_64_BYTES;
 		break;
 	case 80:
-		h->rte_hash_cmp_eq = rte_hash_k80_cmp_eq;
+		h->cmp_jump_table_idx = KEY_80_BYTES;
 		break;
 	case 96:
-		h->rte_hash_cmp_eq = rte_hash_k96_cmp_eq;
+		h->cmp_jump_table_idx = KEY_96_BYTES;
 		break;
 	case 112:
-		h->rte_hash_cmp_eq = rte_hash_k112_cmp_eq;
+		h->cmp_jump_table_idx = KEY_112_BYTES;
 		break;
 	case 128:
-		h->rte_hash_cmp_eq = rte_hash_k128_cmp_eq;
+		h->cmp_jump_table_idx = KEY_128_BYTES;
 		break;
 	default:
 		/* If key is not multiple of 16, use generic memcmp */
-		h->rte_hash_cmp_eq = memcmp;
+		h->cmp_jump_table_idx = KEY_OTHER_BYTES;
 	}
 #else
-	h->rte_hash_cmp_eq = memcmp;
+	h->cmp_jump_table_idx = KEY_OTHER_BYTES;
 #endif
 
 	snprintf(ring_name, sizeof(ring_name), "HT_%s", params->name);
@@ -594,7 +641,7 @@ __rte_hash_add_key_with_hash(const struct rte_hash *h, const void *key,
 				prim_bkt->signatures[i].alt == alt_hash) {
 			k = (struct rte_hash_key *) ((char *)keys +
 					prim_bkt->key_idx[i] * h->key_entry_size);
-			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				/* Enqueue index of free slot back in the ring. */
 				enqueue_slot_back(h, cached_free_slots, slot_id);
 				/* Update data */
@@ -614,7 +661,7 @@ __rte_hash_add_key_with_hash(const struct rte_hash *h, const void *key,
 				sec_bkt->signatures[i].current == alt_hash) {
 			k = (struct rte_hash_key *) ((char *)keys +
 					sec_bkt->key_idx[i] * h->key_entry_size);
-			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				/* Enqueue index of free slot back in the ring. */
 				enqueue_slot_back(h, cached_free_slots, slot_id);
 				/* Update data */
@@ -725,7 +772,7 @@ __rte_hash_lookup_with_hash(const struct rte_hash *h, const void *key,
 				bkt->signatures[i].sig != NULL_SIGNATURE) {
 			k = (struct rte_hash_key *) ((char *)keys +
 					bkt->key_idx[i] * h->key_entry_size);
-			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				if (data != NULL)
 					*data = k->pdata;
 				/*
@@ -748,7 +795,7 @@ __rte_hash_lookup_with_hash(const struct rte_hash *h, const void *key,
 				bkt->signatures[i].alt == sig) {
 			k = (struct rte_hash_key *) ((char *)keys +
 					bkt->key_idx[i] * h->key_entry_size);
-			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				if (data != NULL)
 					*data = k->pdata;
 				/*
@@ -840,7 +887,7 @@ __rte_hash_del_key_with_hash(const struct rte_hash *h, const void *key,
 				bkt->signatures[i].sig != NULL_SIGNATURE) {
 			k = (struct rte_hash_key *) ((char *)keys +
 					bkt->key_idx[i] * h->key_entry_size);
-			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				remove_entry(h, bkt, i);
 
 				/*
@@ -863,7 +910,7 @@ __rte_hash_del_key_with_hash(const struct rte_hash *h, const void *key,
 				bkt->signatures[i].sig != NULL_SIGNATURE) {
 			k = (struct rte_hash_key *) ((char *)keys +
 					bkt->key_idx[i] * h->key_entry_size);
-			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				remove_entry(h, bkt, i);
 
 				/*
@@ -980,7 +1027,7 @@ lookup_stage3(unsigned idx, const struct rte_hash_key *key_slot, const void * co
 	unsigned hit;
 	unsigned key_idx;
 
-	hit = !h->rte_hash_cmp_eq(key_slot->key, keys[idx], h->key_len);
+	hit = !rte_hash_cmp_eq(key_slot->key, keys[idx], h);
 	if (data != NULL)
 		data[idx] = key_slot->pdata;
 
-- 
2.5.0

Re: [dpdk-dev] [PATCH] hash: fix to support multi process

[Bruce Richardson]

On Thu, Mar 24, 2016 at 01:56:44PM +0000, Pablo de Lara wrote:
> Hash library used a function pointer to choose a different
> key compare function, depending on the key size.
> As a result, multiple processes could not use the same hash table,
> as the function addresses vary from one process to another.
> 
> Instead, a jump table is used, so each process has its own
> function addresses, accessing this table with an index stored
> in the hash table (note that using a custom key compare function
> is not supported in multi-process mode).
> 
> Fixes: 48a399119619 ("hash: replace with cuckoo hash implementation")
> 
> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

This is a hard problem to solve, but this probably looks the least-worst solution
to it. Pablo, I assume the performance hit for lookups is pretty small here?

One small nit in the code below, and a comment on the doc change. Otherwise,

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

> ---
>  doc/guides/rel_notes/release_16_04.rst |  5 ++
>  lib/librte_hash/rte_cuckoo_hash.c      | 85 ++++++++++++++++++++++++++--------
>  2 files changed, 71 insertions(+), 19 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/release_16_04.rst b/doc/guides/rel_notes/release_16_04.rst
> index 2785b29..c4359dd 100644
> --- a/doc/guides/rel_notes/release_16_04.rst
> +++ b/doc/guides/rel_notes/release_16_04.rst
> @@ -351,6 +351,11 @@ Libraries
>    Fix crc32c hash functions to return a valid crc32c value for data lengths
>    not multiple of 4 bytes.
>  
> +* **hash: Fixed hash library to support multi-process mode.**
> +
> +  Fix hash library to support multi-process mode, using a jump table,
> +  instead of storing a function pointer to the key compare function.
> +
You probably need to clarify here and in the other docs (e.g. API doc) that:
* multi-process mode only works with the built-in functions and key sizes.
* a custom function not in the jump table can be used but only in single-process
mode.

>  * **librte_port: Fixed segmentation fault for ring and ethdev writer nodrop.**
>  
>    Fixed core dump issue on txq and swq when dropless is set to yes.
> diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
> index 71b5b76..38c19ab 100644
> --- a/lib/librte_hash/rte_cuckoo_hash.c
> +++ b/lib/librte_hash/rte_cuckoo_hash.c
> @@ -102,6 +102,41 @@ EAL_REGISTER_TAILQ(rte_hash_tailq)
>  
>  #define LCORE_CACHE_SIZE		8
>  
> +/*
> + * All different options to select a key compare function,
> + * based on the key size and custom function.
> + */
> +enum cmp_jump_table_case {
> +	KEY_CUSTOM = 0,
> +	KEY_16_BYTES,
> +	KEY_32_BYTES,
> +	KEY_48_BYTES,
> +	KEY_64_BYTES,
> +	KEY_80_BYTES,
> +	KEY_96_BYTES,
> +	KEY_112_BYTES,
> +	KEY_128_BYTES,
> +	KEY_OTHER_BYTES,
> +	NUM_KEY_CMP_CASES,
> +};
> +
> +/*
> + * Table storing all different key compare functions
> + * (multi-process supported)
> + */
> +rte_hash_cmp_eq_t cmp_jump_table[NUM_KEY_CMP_CASES] = {
> +	NULL,
> +	rte_hash_k16_cmp_eq,
> +	rte_hash_k32_cmp_eq,
> +	rte_hash_k48_cmp_eq,
> +	rte_hash_k64_cmp_eq,
> +	rte_hash_k80_cmp_eq,
> +	rte_hash_k96_cmp_eq,
> +	rte_hash_k112_cmp_eq,
> +	rte_hash_k128_cmp_eq,
> +	memcmp
> +};

Array should be const.

/Bruce

Re: [dpdk-dev] [PATCH] hash: fix to support multi process

[De Lara Guarch, Pablo]

Hi,

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Friday, March 25, 2016 2:41 PM
> To: De Lara Guarch, Pablo
> Cc: dev@dpdk.org; edreddy@gmail.com
> Subject: Re: [PATCH] hash: fix to support multi process
> 
> On Thu, Mar 24, 2016 at 01:56:44PM +0000, Pablo de Lara wrote:
> > Hash library used a function pointer to choose a different
> > key compare function, depending on the key size.
> > As a result, multiple processes could not use the same hash table,
> > as the function addresses vary from one process to another.
> >
> > Instead, a jump table is used, so each process has its own
> > function addresses, accessing this table with an index stored
> > in the hash table (note that using a custom key compare function
> > is not supported in multi-process mode).
> >
> > Fixes: 48a399119619 ("hash: replace with cuckoo hash implementation")
> >
> > Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> 
> This is a hard problem to solve, but this probably looks the least-worst
> solution
> to it. Pablo, I assume the performance hit for lookups is pretty small here?

Yes, performance degradation is negligible.
> 
> One small nit in the code below, and a comment on the doc change.

Will send a v2 with the changes. Anyway, I was waiting for Djama and Zhang
to give some feedback here and see if it works for them.

Thanks,
Pablo
> Otherwise,
> 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> > ---
> >  doc/guides/rel_notes/release_16_04.rst |  5 ++
> >  lib/librte_hash/rte_cuckoo_hash.c      | 85 ++++++++++++++++++++++++++-
> -------
> >  2 files changed, 71 insertions(+), 19 deletions(-)
> >
> > diff --git a/doc/guides/rel_notes/release_16_04.rst
> b/doc/guides/rel_notes/release_16_04.rst
> > index 2785b29..c4359dd 100644
> > --- a/doc/guides/rel_notes/release_16_04.rst
> > +++ b/doc/guides/rel_notes/release_16_04.rst
> > @@ -351,6 +351,11 @@ Libraries
> >    Fix crc32c hash functions to return a valid crc32c value for data lengths
> >    not multiple of 4 bytes.
> >
> > +* **hash: Fixed hash library to support multi-process mode.**
> > +
> > +  Fix hash library to support multi-process mode, using a jump table,
> > +  instead of storing a function pointer to the key compare function.
> > +
> You probably need to clarify here and in the other docs (e.g. API doc) that:
> * multi-process mode only works with the built-in functions and key sizes.
> * a custom function not in the jump table can be used but only in single-
> process
> mode.
> 
> >  * **librte_port: Fixed segmentation fault for ring and ethdev writer
> nodrop.**
> >
> >    Fixed core dump issue on txq and swq when dropless is set to yes.
> > diff --git a/lib/librte_hash/rte_cuckoo_hash.c
> b/lib/librte_hash/rte_cuckoo_hash.c
> > index 71b5b76..38c19ab 100644
> > --- a/lib/librte_hash/rte_cuckoo_hash.c
> > +++ b/lib/librte_hash/rte_cuckoo_hash.c
> > @@ -102,6 +102,41 @@ EAL_REGISTER_TAILQ(rte_hash_tailq)
> >
> >  #define LCORE_CACHE_SIZE		8
> >
> > +/*
> > + * All different options to select a key compare function,
> > + * based on the key size and custom function.
> > + */
> > +enum cmp_jump_table_case {
> > +	KEY_CUSTOM = 0,
> > +	KEY_16_BYTES,
> > +	KEY_32_BYTES,
> > +	KEY_48_BYTES,
> > +	KEY_64_BYTES,
> > +	KEY_80_BYTES,
> > +	KEY_96_BYTES,
> > +	KEY_112_BYTES,
> > +	KEY_128_BYTES,
> > +	KEY_OTHER_BYTES,
> > +	NUM_KEY_CMP_CASES,
> > +};
> > +
> > +/*
> > + * Table storing all different key compare functions
> > + * (multi-process supported)
> > + */
> > +rte_hash_cmp_eq_t cmp_jump_table[NUM_KEY_CMP_CASES] = {
> > +	NULL,
> > +	rte_hash_k16_cmp_eq,
> > +	rte_hash_k32_cmp_eq,
> > +	rte_hash_k48_cmp_eq,
> > +	rte_hash_k64_cmp_eq,
> > +	rte_hash_k80_cmp_eq,
> > +	rte_hash_k96_cmp_eq,
> > +	rte_hash_k112_cmp_eq,
> > +	rte_hash_k128_cmp_eq,
> > +	memcmp
> > +};
> 
> Array should be const.
> 
> /Bruce

[dpdk-dev] [PATCH v2] hash: fix to support multi process

[Pablo de Lara]

Hash library used a function pointer to choose a different
key compare function, depending on the key size.
As a result, multiple processes could not use the same hash table,
as the function addresses vary from one process to another.

Instead, a jump table is used, so each process has its own
function addresses, accessing this table with an index stored
in the hash table (note that using a custom key compare function
is not supported in multi-process mode).

Fixes: 48a399119619 ("hash: replace with cuckoo hash implementation")

Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---

Changes in v2:
- Added missing const
- Added extra info in documentation

 doc/guides/prog_guide/hash_lib.rst     |  8 ++++
 doc/guides/rel_notes/release_16_04.rst |  8 ++++
 lib/librte_hash/rte_cuckoo_hash.c      | 85 ++++++++++++++++++++++++++--------
 3 files changed, 82 insertions(+), 19 deletions(-)

diff --git a/doc/guides/prog_guide/hash_lib.rst b/doc/guides/prog_guide/hash_lib.rst
index 8f2cdbf..7944640 100644
--- a/doc/guides/prog_guide/hash_lib.rst
+++ b/doc/guides/prog_guide/hash_lib.rst
@@ -87,6 +87,14 @@ or stored in the hash table itself.
 The example hash tables in the L2/L3 Forwarding sample applications defines which port to forward a packet to based on a packet flow identified by the five-tuple lookup.
 However, this table could also be used for more sophisticated features and provide many other functions and actions that could be performed on the packets and flows.
 
+Multi-process support
+---------------------
+
+The hash library can be used in a multi-process environment, minding that only lookups are thread-safe.
+The only function that can only be used in single-process mode is rte_hash_set_cmp_func(), which sets up
+a custom compare function, which is assigned to a function pointer (therefore, it is not supported in
+multi-process mode).
+
 Implementation Details
 ----------------------
 
diff --git a/doc/guides/rel_notes/release_16_04.rst b/doc/guides/rel_notes/release_16_04.rst
index 79d76e1..e6cc34d 100644
--- a/doc/guides/rel_notes/release_16_04.rst
+++ b/doc/guides/rel_notes/release_16_04.rst
@@ -404,6 +404,14 @@ Libraries
   Fix crc32c hash functions to return a valid crc32c value for data lengths
   not multiple of 4 bytes.
 
+* **hash: Fixed hash library to support multi-process mode.**
+
+  Fix hash library to support multi-process mode, using a jump table,
+  instead of storing a function pointer to the key compare function.
+  Multi-process mode only works with the built-in compare functions,
+  however a custom compare function (not in the jump table) can only
+  be used in single-process mode.
+
 * **librte_port: Fixed segmentation fault for ring and ethdev writer nodrop.**
 
   Fixed core dump issue on txq and swq when dropless is set to yes.
diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index 71b5b76..bdba557 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -102,6 +102,41 @@ EAL_REGISTER_TAILQ(rte_hash_tailq)
 
 #define LCORE_CACHE_SIZE		8
 
+/*
+ * All different options to select a key compare function,
+ * based on the key size and custom function.
+ */
+enum cmp_jump_table_case {
+	KEY_CUSTOM = 0,
+	KEY_16_BYTES,
+	KEY_32_BYTES,
+	KEY_48_BYTES,
+	KEY_64_BYTES,
+	KEY_80_BYTES,
+	KEY_96_BYTES,
+	KEY_112_BYTES,
+	KEY_128_BYTES,
+	KEY_OTHER_BYTES,
+	NUM_KEY_CMP_CASES,
+};
+
+/*
+ * Table storing all different key compare functions
+ * (multi-process supported)
+ */
+const rte_hash_cmp_eq_t cmp_jump_table[NUM_KEY_CMP_CASES] = {
+	NULL,
+	rte_hash_k16_cmp_eq,
+	rte_hash_k32_cmp_eq,
+	rte_hash_k48_cmp_eq,
+	rte_hash_k64_cmp_eq,
+	rte_hash_k80_cmp_eq,
+	rte_hash_k96_cmp_eq,
+	rte_hash_k112_cmp_eq,
+	rte_hash_k128_cmp_eq,
+	memcmp
+};
+
 struct lcore_cache {
 	unsigned len; /**< Cache len */
 	void *objs[LCORE_CACHE_SIZE]; /**< Cache objects */
@@ -115,7 +150,10 @@ struct rte_hash {
 	uint32_t key_len;               /**< Length of hash key. */
 	rte_hash_function hash_func;    /**< Function used to calculate hash. */
 	uint32_t hash_func_init_val;    /**< Init value used by hash_func. */
-	rte_hash_cmp_eq_t rte_hash_cmp_eq; /**< Function used to compare keys. */
+	rte_hash_cmp_eq_t rte_hash_custom_cmp_eq;
+	/**< Custom function used to compare keys. */
+	enum cmp_jump_table_case cmp_jump_table_idx;
+	/**< Indicates which compare function to use. */
 	uint32_t bucket_bitmask;        /**< Bitmask for getting bucket index
 						from hash signature. */
 	uint32_t key_entry_size;         /**< Size of each key entry. */
@@ -187,7 +225,16 @@ rte_hash_find_existing(const char *name)
 
 void rte_hash_set_cmp_func(struct rte_hash *h, rte_hash_cmp_eq_t func)
 {
-	h->rte_hash_cmp_eq = func;
+	h->rte_hash_custom_cmp_eq = func;
+}
+
+static inline int
+rte_hash_cmp_eq(const void *key1, const void *key2, const struct rte_hash *h)
+{
+	if (h->cmp_jump_table_idx == KEY_CUSTOM)
+		return h->rte_hash_custom_cmp_eq(key1, key2, h->key_len);
+	else
+		return cmp_jump_table[h->cmp_jump_table_idx](key1, key2, h->key_len);
 }
 
 struct rte_hash *
@@ -292,35 +339,35 @@ rte_hash_create(const struct rte_hash_parameters *params)
 	/* Select function to compare keys */
 	switch (params->key_len) {
 	case 16:
-		h->rte_hash_cmp_eq = rte_hash_k16_cmp_eq;
+		h->cmp_jump_table_idx = KEY_16_BYTES;
 		break;
 	case 32:
-		h->rte_hash_cmp_eq = rte_hash_k32_cmp_eq;
+		h->cmp_jump_table_idx = KEY_32_BYTES;
 		break;
 	case 48:
-		h->rte_hash_cmp_eq = rte_hash_k48_cmp_eq;
+		h->cmp_jump_table_idx = KEY_48_BYTES;
 		break;
 	case 64:
-		h->rte_hash_cmp_eq = rte_hash_k64_cmp_eq;
+		h->cmp_jump_table_idx = KEY_64_BYTES;
 		break;
 	case 80:
-		h->rte_hash_cmp_eq = rte_hash_k80_cmp_eq;
+		h->cmp_jump_table_idx = KEY_80_BYTES;
 		break;
 	case 96:
-		h->rte_hash_cmp_eq = rte_hash_k96_cmp_eq;
+		h->cmp_jump_table_idx = KEY_96_BYTES;
 		break;
 	case 112:
-		h->rte_hash_cmp_eq = rte_hash_k112_cmp_eq;
+		h->cmp_jump_table_idx = KEY_112_BYTES;
 		break;
 	case 128:
-		h->rte_hash_cmp_eq = rte_hash_k128_cmp_eq;
+		h->cmp_jump_table_idx = KEY_128_BYTES;
 		break;
 	default:
 		/* If key is not multiple of 16, use generic memcmp */
-		h->rte_hash_cmp_eq = memcmp;
+		h->cmp_jump_table_idx = KEY_OTHER_BYTES;
 	}
 #else
-	h->rte_hash_cmp_eq = memcmp;
+	h->cmp_jump_table_idx = KEY_OTHER_BYTES;
 #endif
 
 	snprintf(ring_name, sizeof(ring_name), "HT_%s", params->name);
@@ -594,7 +641,7 @@ __rte_hash_add_key_with_hash(const struct rte_hash *h, const void *key,
 				prim_bkt->signatures[i].alt == alt_hash) {
 			k = (struct rte_hash_key *) ((char *)keys +
 					prim_bkt->key_idx[i] * h->key_entry_size);
-			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				/* Enqueue index of free slot back in the ring. */
 				enqueue_slot_back(h, cached_free_slots, slot_id);
 				/* Update data */
@@ -614,7 +661,7 @@ __rte_hash_add_key_with_hash(const struct rte_hash *h, const void *key,
 				sec_bkt->signatures[i].current == alt_hash) {
 			k = (struct rte_hash_key *) ((char *)keys +
 					sec_bkt->key_idx[i] * h->key_entry_size);
-			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				/* Enqueue index of free slot back in the ring. */
 				enqueue_slot_back(h, cached_free_slots, slot_id);
 				/* Update data */
@@ -725,7 +772,7 @@ __rte_hash_lookup_with_hash(const struct rte_hash *h, const void *key,
 				bkt->signatures[i].sig != NULL_SIGNATURE) {
 			k = (struct rte_hash_key *) ((char *)keys +
 					bkt->key_idx[i] * h->key_entry_size);
-			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				if (data != NULL)
 					*data = k->pdata;
 				/*
@@ -748,7 +795,7 @@ __rte_hash_lookup_with_hash(const struct rte_hash *h, const void *key,
 				bkt->signatures[i].alt == sig) {
 			k = (struct rte_hash_key *) ((char *)keys +
 					bkt->key_idx[i] * h->key_entry_size);
-			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				if (data != NULL)
 					*data = k->pdata;
 				/*
@@ -840,7 +887,7 @@ __rte_hash_del_key_with_hash(const struct rte_hash *h, const void *key,
 				bkt->signatures[i].sig != NULL_SIGNATURE) {
 			k = (struct rte_hash_key *) ((char *)keys +
 					bkt->key_idx[i] * h->key_entry_size);
-			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				remove_entry(h, bkt, i);
 
 				/*
@@ -863,7 +910,7 @@ __rte_hash_del_key_with_hash(const struct rte_hash *h, const void *key,
 				bkt->signatures[i].sig != NULL_SIGNATURE) {
 			k = (struct rte_hash_key *) ((char *)keys +
 					bkt->key_idx[i] * h->key_entry_size);
-			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				remove_entry(h, bkt, i);
 
 				/*
@@ -980,7 +1027,7 @@ lookup_stage3(unsigned idx, const struct rte_hash_key *key_slot, const void * co
 	unsigned hit;
 	unsigned key_idx;
 
-	hit = !h->rte_hash_cmp_eq(key_slot->key, keys[idx], h->key_len);
+	hit = !rte_hash_cmp_eq(key_slot->key, keys[idx], h);
 	if (data != NULL)
 		data[idx] = key_slot->pdata;
 
-- 
2.5.5

Re: [dpdk-dev] [PATCH v2] hash: fix to support multi process

[Thomas Monjalon]

Hi Pablo,

There is a build issue for arm-armv7a-linuxapp-gcc:
	lib/librte_hash/rte_cuckoo_hash.c:129:2: error:
		'rte_hash_k16_cmp_eq' undeclared here

The functions rte_hash_k*_cmp_eq are defined only for x86 and ARM64.

Note that RTE_ARCH_X86 could be used instead of
RTE_ARCH_X86_64 || RTE_ARCH_I686 || RTE_ARCH_X86_X32 in librte_hash.

Re: [dpdk-dev] [PATCH v2] hash: fix to support multi process

[De Lara Guarch, Pablo]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, April 01, 2016 2:07 PM
> To: De Lara Guarch, Pablo
> Cc: dev@dpdk.org; Richardson, Bruce
> Subject: Re: [dpdk-dev] [PATCH v2] hash: fix to support multi process
> 
> Hi Pablo,
> 
> There is a build issue for arm-armv7a-linuxapp-gcc:
> 	lib/librte_hash/rte_cuckoo_hash.c:129:2: error:
> 		'rte_hash_k16_cmp_eq' undeclared here
> 
> The functions rte_hash_k*_cmp_eq are defined only for x86 and ARM64.
> 
> Note that RTE_ARCH_X86 could be used instead of
> RTE_ARCH_X86_64 || RTE_ARCH_I686 || RTE_ARCH_X86_X32 in librte_hash.

Good catch. I will submit a v3 shortly.

Thanks also for the suggestion,
Pablo

[dpdk-dev] [PATCH v3 0/2] Multi-process support in hash library

[Pablo de Lara]

This patchset fixes the hash library to support multi-process.

It also replaces several RTE_ARCH_* with RTE_ARCH_X86, for simplicity.

Changes in v3:
- Added new #ifdefs, to fix ARMv7 compilation
- Replaced RTE_ARCH_* for X86 macros

Changes in v2:
- Added missing const
- Added extra info in documentation

Pablo de Lara (2):
  hash: use RTE_ARCH_X86 macro for X86 systems
  hash: fix to support multi process

 doc/guides/prog_guide/hash_lib.rst     |   8 +++
 doc/guides/rel_notes/release_16_04.rst |   8 +++
 lib/librte_hash/rte_cuckoo_hash.c      | 112 ++++++++++++++++++++++++++-------
 3 files changed, 106 insertions(+), 22 deletions(-)

-- 
2.5.5

[dpdk-dev] [PATCH v3 1/2] hash: use RTE_ARCH_X86 macro for X86 systems

[Pablo de Lara]

Instead of using RTE_ARCH_X86_64, RTE_ARCH_X86_32
and RTE_ARCH_I686, use directly RTTE_ARCH_X86

Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
---
 lib/librte_hash/rte_cuckoo_hash.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index 71b5b76..b9b0b65 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -59,7 +59,7 @@
 #include <rte_compat.h>
 
 #include "rte_hash.h"
-#if defined(RTE_ARCH_X86_64) || defined(RTE_ARCH_I686) || defined(RTE_ARCH_X86_X32)
+#if defined(RTE_ARCH_X86)
 #include "rte_cmp_x86.h"
 #endif
 
@@ -287,8 +287,7 @@ rte_hash_create(const struct rte_hash_parameters *params)
  * If x86 architecture is used, select appropriate compare function,
  * which may use x86 instrinsics, otherwise use memcmp
  */
-#if defined(RTE_ARCH_X86_64) || defined(RTE_ARCH_I686) ||\
-	 defined(RTE_ARCH_X86_X32) || defined(RTE_ARCH_ARM64)
+#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
 	/* Select function to compare keys */
 	switch (params->key_len) {
 	case 16:
-- 
2.5.5

[dpdk-dev] [PATCH v3 2/2] hash: fix to support multi process

[Pablo de Lara]

Hash library used a function pointer to choose a different
key compare function, depending on the key size.
As a result, multiple processes could not use the same hash table,
as the function addresses vary from one process to another.

Instead, a jump table is used, so each process has its own
function addresses, accessing this table with an index stored
in the hash table (note that using a custom key compare function
is not supported in multi-process mode).

Fixes: 48a399119619 ("hash: replace with cuckoo hash implementation")

Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 doc/guides/prog_guide/hash_lib.rst     |   8 +++
 doc/guides/rel_notes/release_16_04.rst |   8 +++
 lib/librte_hash/rte_cuckoo_hash.c      | 107 +++++++++++++++++++++++++++------
 3 files changed, 104 insertions(+), 19 deletions(-)

diff --git a/doc/guides/prog_guide/hash_lib.rst b/doc/guides/prog_guide/hash_lib.rst
index 8f2cdbf..7944640 100644
--- a/doc/guides/prog_guide/hash_lib.rst
+++ b/doc/guides/prog_guide/hash_lib.rst
@@ -87,6 +87,14 @@ or stored in the hash table itself.
 The example hash tables in the L2/L3 Forwarding sample applications defines which port to forward a packet to based on a packet flow identified by the five-tuple lookup.
 However, this table could also be used for more sophisticated features and provide many other functions and actions that could be performed on the packets and flows.
 
+Multi-process support
+---------------------
+
+The hash library can be used in a multi-process environment, minding that only lookups are thread-safe.
+The only function that can only be used in single-process mode is rte_hash_set_cmp_func(), which sets up
+a custom compare function, which is assigned to a function pointer (therefore, it is not supported in
+multi-process mode).
+
 Implementation Details
 ----------------------
 
diff --git a/doc/guides/rel_notes/release_16_04.rst b/doc/guides/rel_notes/release_16_04.rst
index 79d76e1..e6cc34d 100644
--- a/doc/guides/rel_notes/release_16_04.rst
+++ b/doc/guides/rel_notes/release_16_04.rst
@@ -404,6 +404,14 @@ Libraries
   Fix crc32c hash functions to return a valid crc32c value for data lengths
   not multiple of 4 bytes.
 
+* **hash: Fixed hash library to support multi-process mode.**
+
+  Fix hash library to support multi-process mode, using a jump table,
+  instead of storing a function pointer to the key compare function.
+  Multi-process mode only works with the built-in compare functions,
+  however a custom compare function (not in the jump table) can only
+  be used in single-process mode.
+
 * **librte_port: Fixed segmentation fault for ring and ethdev writer nodrop.**
 
   Fixed core dump issue on txq and swq when dropless is set to yes.
diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index b9b0b65..04e95b7 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -102,6 +102,63 @@ EAL_REGISTER_TAILQ(rte_hash_tailq)
 
 #define LCORE_CACHE_SIZE		8
 
+#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
+/*
+ * All different options to select a key compare function,
+ * based on the key size and custom function.
+ */
+enum cmp_jump_table_case {
+	KEY_CUSTOM = 0,
+	KEY_16_BYTES,
+	KEY_32_BYTES,
+	KEY_48_BYTES,
+	KEY_64_BYTES,
+	KEY_80_BYTES,
+	KEY_96_BYTES,
+	KEY_112_BYTES,
+	KEY_128_BYTES,
+	KEY_OTHER_BYTES,
+	NUM_KEY_CMP_CASES,
+};
+
+/*
+ * Table storing all different key compare functions
+ * (multi-process supported)
+ */
+const rte_hash_cmp_eq_t cmp_jump_table[NUM_KEY_CMP_CASES] = {
+	NULL,
+	rte_hash_k16_cmp_eq,
+	rte_hash_k32_cmp_eq,
+	rte_hash_k48_cmp_eq,
+	rte_hash_k64_cmp_eq,
+	rte_hash_k80_cmp_eq,
+	rte_hash_k96_cmp_eq,
+	rte_hash_k112_cmp_eq,
+	rte_hash_k128_cmp_eq,
+	memcmp
+};
+#else
+/*
+ * All different options to select a key compare function,
+ * based on the key size and custom function.
+ */
+enum cmp_jump_table_case {
+	KEY_CUSTOM = 0,
+	KEY_OTHER_BYTES,
+	NUM_KEY_CMP_CASES,
+};
+
+/*
+ * Table storing all different key compare functions
+ * (multi-process supported)
+ */
+const rte_hash_cmp_eq_t cmp_jump_table[NUM_KEY_CMP_CASES] = {
+	NULL,
+	memcmp
+};
+
+#endif
+
 struct lcore_cache {
 	unsigned len; /**< Cache len */
 	void *objs[LCORE_CACHE_SIZE]; /**< Cache objects */
@@ -115,7 +172,10 @@ struct rte_hash {
 	uint32_t key_len;               /**< Length of hash key. */
 	rte_hash_function hash_func;    /**< Function used to calculate hash. */
 	uint32_t hash_func_init_val;    /**< Init value used by hash_func. */
-	rte_hash_cmp_eq_t rte_hash_cmp_eq; /**< Function used to compare keys. */
+	rte_hash_cmp_eq_t rte_hash_custom_cmp_eq;
+	/**< Custom function used to compare keys. */
+	enum cmp_jump_table_case cmp_jump_table_idx;
+	/**< Indicates which compare function to use. */
 	uint32_t bucket_bitmask;        /**< Bitmask for getting bucket index
 						from hash signature. */
 	uint32_t key_entry_size;         /**< Size of each key entry. */
@@ -187,7 +247,16 @@ rte_hash_find_existing(const char *name)
 
 void rte_hash_set_cmp_func(struct rte_hash *h, rte_hash_cmp_eq_t func)
 {
-	h->rte_hash_cmp_eq = func;
+	h->rte_hash_custom_cmp_eq = func;
+}
+
+static inline int
+rte_hash_cmp_eq(const void *key1, const void *key2, const struct rte_hash *h)
+{
+	if (h->cmp_jump_table_idx == KEY_CUSTOM)
+		return h->rte_hash_custom_cmp_eq(key1, key2, h->key_len);
+	else
+		return cmp_jump_table[h->cmp_jump_table_idx](key1, key2, h->key_len);
 }
 
 struct rte_hash *
@@ -291,35 +360,35 @@ rte_hash_create(const struct rte_hash_parameters *params)
 	/* Select function to compare keys */
 	switch (params->key_len) {
 	case 16:
-		h->rte_hash_cmp_eq = rte_hash_k16_cmp_eq;
+		h->cmp_jump_table_idx = KEY_16_BYTES;
 		break;
 	case 32:
-		h->rte_hash_cmp_eq = rte_hash_k32_cmp_eq;
+		h->cmp_jump_table_idx = KEY_32_BYTES;
 		break;
 	case 48:
-		h->rte_hash_cmp_eq = rte_hash_k48_cmp_eq;
+		h->cmp_jump_table_idx = KEY_48_BYTES;
 		break;
 	case 64:
-		h->rte_hash_cmp_eq = rte_hash_k64_cmp_eq;
+		h->cmp_jump_table_idx = KEY_64_BYTES;
 		break;
 	case 80:
-		h->rte_hash_cmp_eq = rte_hash_k80_cmp_eq;
+		h->cmp_jump_table_idx = KEY_80_BYTES;
 		break;
 	case 96:
-		h->rte_hash_cmp_eq = rte_hash_k96_cmp_eq;
+		h->cmp_jump_table_idx = KEY_96_BYTES;
 		break;
 	case 112:
-		h->rte_hash_cmp_eq = rte_hash_k112_cmp_eq;
+		h->cmp_jump_table_idx = KEY_112_BYTES;
 		break;
 	case 128:
-		h->rte_hash_cmp_eq = rte_hash_k128_cmp_eq;
+		h->cmp_jump_table_idx = KEY_128_BYTES;
 		break;
 	default:
 		/* If key is not multiple of 16, use generic memcmp */
-		h->rte_hash_cmp_eq = memcmp;
+		h->cmp_jump_table_idx = KEY_OTHER_BYTES;
 	}
 #else
-	h->rte_hash_cmp_eq = memcmp;
+	h->cmp_jump_table_idx = KEY_OTHER_BYTES;
 #endif
 
 	snprintf(ring_name, sizeof(ring_name), "HT_%s", params->name);
@@ -593,7 +662,7 @@ __rte_hash_add_key_with_hash(const struct rte_hash *h, const void *key,
 				prim_bkt->signatures[i].alt == alt_hash) {
 			k = (struct rte_hash_key *) ((char *)keys +
 					prim_bkt->key_idx[i] * h->key_entry_size);
-			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				/* Enqueue index of free slot back in the ring. */
 				enqueue_slot_back(h, cached_free_slots, slot_id);
 				/* Update data */
@@ -613,7 +682,7 @@ __rte_hash_add_key_with_hash(const struct rte_hash *h, const void *key,
 				sec_bkt->signatures[i].current == alt_hash) {
 			k = (struct rte_hash_key *) ((char *)keys +
 					sec_bkt->key_idx[i] * h->key_entry_size);
-			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				/* Enqueue index of free slot back in the ring. */
 				enqueue_slot_back(h, cached_free_slots, slot_id);
 				/* Update data */
@@ -724,7 +793,7 @@ __rte_hash_lookup_with_hash(const struct rte_hash *h, const void *key,
 				bkt->signatures[i].sig != NULL_SIGNATURE) {
 			k = (struct rte_hash_key *) ((char *)keys +
 					bkt->key_idx[i] * h->key_entry_size);
-			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				if (data != NULL)
 					*data = k->pdata;
 				/*
@@ -747,7 +816,7 @@ __rte_hash_lookup_with_hash(const struct rte_hash *h, const void *key,
 				bkt->signatures[i].alt == sig) {
 			k = (struct rte_hash_key *) ((char *)keys +
 					bkt->key_idx[i] * h->key_entry_size);
-			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				if (data != NULL)
 					*data = k->pdata;
 				/*
@@ -839,7 +908,7 @@ __rte_hash_del_key_with_hash(const struct rte_hash *h, const void *key,
 				bkt->signatures[i].sig != NULL_SIGNATURE) {
 			k = (struct rte_hash_key *) ((char *)keys +
 					bkt->key_idx[i] * h->key_entry_size);
-			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				remove_entry(h, bkt, i);
 
 				/*
@@ -862,7 +931,7 @@ __rte_hash_del_key_with_hash(const struct rte_hash *h, const void *key,
 				bkt->signatures[i].sig != NULL_SIGNATURE) {
 			k = (struct rte_hash_key *) ((char *)keys +
 					bkt->key_idx[i] * h->key_entry_size);
-			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				remove_entry(h, bkt, i);
 
 				/*
@@ -979,7 +1048,7 @@ lookup_stage3(unsigned idx, const struct rte_hash_key *key_slot, const void * co
 	unsigned hit;
 	unsigned key_idx;
 
-	hit = !h->rte_hash_cmp_eq(key_slot->key, keys[idx], h->key_len);
+	hit = !rte_hash_cmp_eq(key_slot->key, keys[idx], h);
 	if (data != NULL)
 		data[idx] = key_slot->pdata;
 
-- 
2.5.5

Re: [dpdk-dev] [PATCH v3 0/2] Multi-process support in hash library

[Thomas Monjalon]

2016-04-01 16:03, Pablo de Lara:
> This patchset fixes the hash library to support multi-process.
> 
> It also replaces several RTE_ARCH_* with RTE_ARCH_X86, for simplicity.
> 
> Changes in v3:
> - Added new #ifdefs, to fix ARMv7 compilation
> - Replaced RTE_ARCH_* for X86 macros
> 
> Changes in v2:
> - Added missing const
> - Added extra info in documentation
> 
> Pablo de Lara (2):
>   hash: use RTE_ARCH_X86 macro for X86 systems
>   hash: fix to support multi process

Applied, thanks