DPDK patches and discussions
 help / color / mirror / Atom feed
From: Dhana Eadala <edreddy@gmail.com>
To: bruce.richardson@intel.com, pablo.de.lara.guarch@intel.com,
	michael.qiu@intel.com
Cc: dev@dpdk.org
Subject: [dpdk-dev] [PATCH] hash: fix memcmp function pointer in multi-process environment
Date: Wed,  2 Mar 2016 22:35:52 -0500	[thread overview]
Message-ID: <1456976152-15640-1-git-send-email-edreddy@gmail.com> (raw)

We found a problem in dpdk-2.2 using under multi-process environment.
Here is the brief description how we are using the dpdk:

We have two processes proc1, proc2 using dpdk. These proc1 and proc2 are two different compiled binaries.
proc1 is started as primary process and proc2 as secondary process.

proc1:
Calls srcHash = rte_hash_create("src_hash_name") to create rte_hash structure.
As part of this, this api initalized the rte_hash structure and set the srcHash->rte_hash_cmp_eq to the address of memcmp() from proc1 address space.

proc2:
calls srcHash =  rte_hash_find_existing("src_hash_name"). This returns the rte_hash created by proc1.
This srcHash->rte_hash_cmp_eq still points to the address of memcmp() from proc1 address space.
Later proc2  calls rte_hash_lookup_with_hash(srcHash, (const void*) &key, key.sig);
Under the hood, rte_hash_lookup_with_hash() invokes __rte_hash_lookup_with_hash(), which in turn calls h->rte_hash_cmp_eq(key, k->key, h->key_len).
This leads to a crash as h->rte_hash_cmp_eq is an address from proc1 address space and is invalid address in proc2 address space.

We found, from dpdk documentation, that

"
 The use of function pointers between multiple processes running based of different compiled
 binaries is not supported, since the location of a given function in one process may be different to
 its location in a second. This prevents the librte_hash library from behaving properly as in a  multi-
 threaded instance, since it uses a pointer to the hash function internally.

 To work around this issue, it is recommended that multi-process applications perform the hash
 calculations by directly calling the hashing function from the code and then using the
 rte_hash_add_with_hash()/rte_hash_lookup_with_hash() functions instead of the functions which do
 the hashing internally, such as rte_hash_add()/rte_hash_lookup().
"

We did follow the recommended steps by invoking rte_hash_lookup_with_hash().
It was no issue up to and including dpdk-2.0. In later releases started crashing because rte_hash_cmp_eq is introduced in dpdk-2.1

We fixed it with the following patch and would like to submit the patch to dpdk.org.
Patch is created such that, if anyone wanted to use dpdk in multi-process environment with function pointers not shared, they need to
define RTE_LIB_MP_NO_FUNC_PTR in their Makefile. Without defining this flag in Makefile, it works as it is now.

Signed-off-by: Dhana Eadala <edreddy@gmail.com>
---
 lib/librte_hash/rte_cuckoo_hash.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index 3e3167c..0946777 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -594,7 +594,11 @@ __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);
+#ifdef RTE_LIB_MP_NO_FUNC_PTR
+			if (memcmp(key, k->key, h->key_len) == 0) {
+#else
 			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+#endif
 				/* Enqueue index of free slot back in the ring. */
 				enqueue_slot_back(h, cached_free_slots, slot_id);
 				/* Update data */
@@ -614,7 +618,11 @@ __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);
+#ifdef RTE_LIB_MP_NO_FUNC_PTR
+			if (memcmp(key, k->key, h->key_len) == 0) {
+#else
 			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+#endif
 				/* Enqueue index of free slot back in the ring. */
 				enqueue_slot_back(h, cached_free_slots, slot_id);
 				/* Update data */
@@ -725,7 +733,11 @@ __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);
+#ifdef RTE_LIB_MP_NO_FUNC_PTR
+			if (memcmp (key, k->key, h->key_len) == 0) {
+#else
 			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+#endif
 				if (data != NULL)
 					*data = k->pdata;
 				/*
@@ -748,7 +760,11 @@ __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);
+#ifdef RTE_LIB_MP_NO_FUNC_PTR
+			if (memcmp(key, k->key, h->key_len) == 0) {
+#else
 			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+#endif
 				if (data != NULL)
 					*data = k->pdata;
 				/*
@@ -840,7 +856,11 @@ __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);
+#ifdef RTE_LIB_MP_NO_FUNC_PTR
+			if (memcmp(key, k->key, h->key_len) == 0) {
+#else
 			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+#endif
 				remove_entry(h, bkt, i);
 
 				/*
@@ -863,7 +883,11 @@ __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);
+#ifdef RTE_LIB_MP_NO_FUNC_PTR
+			if (memcmp(key, k->key, h->key_len) == 0) {
+#else
 			if (h->rte_hash_cmp_eq(key, k->key, h->key_len) == 0) {
+#endif
 				remove_entry(h, bkt, i);
 
 				/*
@@ -980,7 +1004,11 @@ lookup_stage3(unsigned idx, const struct rte_hash_key *key_slot, const void * co
 	unsigned hit;
 	unsigned key_idx;
 
+#ifdef RTE_LIB_MP_NO_FUNC_PTR
+	hit = !memcmp(key_slot->key, keys[idx], h->key_len);
+#else
 	hit = !h->rte_hash_cmp_eq(key_slot->key, keys[idx], h->key_len);
+#endif
 	if (data != NULL)
 		data[idx] = key_slot->pdata;
 
-- 
2.5.0

             reply	other threads:[~2016-03-03  3:36 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-03  3:35 Dhana Eadala [this message]
2016-03-03  5:44 ` Qiu, Michael
2016-03-04  1:00   ` Dhananjaya Reddy Eadala
2016-03-09 21:36     ` Dhananjaya Reddy Eadala
2016-03-14  2:16 Dhana Eadala
2016-03-14  2:30 ` 张伟
2016-03-14  4:38 ` 张伟
2016-03-15  0:57   ` Dhananjaya Eadala
2016-03-15  1:02     ` 张伟
2016-03-24 14:00       ` De Lara Guarch, Pablo
2016-04-01 13:14         ` Thomas Monjalon
2016-03-22 11:42 ` Thomas Monjalon
2016-03-22 19:53   ` De Lara Guarch, Pablo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1456976152-15640-1-git-send-email-edreddy@gmail.com \
    --to=edreddy@gmail.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=michael.qiu@intel.com \
    --cc=pablo.de.lara.guarch@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).