diff mbox

[dpdk-dev] hash: fix memcmp function pointer in multi-process environment

Message ID 1456976152-15640-1-git-send-email-edreddy@gmail.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Commit Message

Dhana Eadala March 3, 2016, 3:35 a.m. UTC 
  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(+)

Comments

Michael Qiu March 3, 2016, 5:44 a.m. UTC | #1 
On 3/3/2016 11:36 AM, Dhana Eadala wrote:
> 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>
> ---
>

Some comments:

1.  your commit log need to refactor, better to limit every line less
than 80 character.

2. I think you could add the ifdef here in
lib/librte_hash/rte_cuckoo_hash.c :
/*
 * 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)
    /* Select function to compare keys */
    switch (params->key_len) {
    case 16:
        h->rte_hash_cmp_eq = rte_hash_k16_cmp_eq;
        break;
[...]
        break;
    default:
        /* If key is not multiple of 16, use generic memcmp */
        h->rte_hash_cmp_eq = memcmp;
    }
#else
    h->rte_hash_cmp_eq = memcmp;
#endif

So that could remove other #ifdef in those lines.

3. I don't think ask others to write RTE_LIB_MP_NO_FUNC_PTR in makefile
is a good idea, if you really want to do that, please add a doc so that
others could know it.

Thanks,
Michael
Dhana Eadala March 4, 2016, 1 a.m. UTC | #2 
Hi Michael

Please see my answers to your comments here.

1. Sure, I will refactor the commit log to restrict not more than 80
characters.

2. Not sure how we can ifdef at the location you mentioned. Can you please
elaborate more on this.
    We already have similar ifdef protection to what you suggested and with
that protection memcmp is assigned.
    Problem is in using the function pointer to call the compare function.
    So we need protection for invoking compare function, under
multi-process environment.

3. I couldn't come up with any other idea to protect this function pointer
invocation.

Thanks
Dhana




On Thu, Mar 3, 2016 at 12:44 AM, Qiu, Michael <michael.qiu@intel.com> wrote:

> On 3/3/2016 11:36 AM, Dhana Eadala wrote:
> > 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>
> > ---
> >
>
> Some comments:
>
> 1.  your commit log need to refactor, better to limit every line less
> than 80 character.
>
> 2. I think you could add the ifdef here in
> lib/librte_hash/rte_cuckoo_hash.c :
> /*
>  * 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)
>     /* Select function to compare keys */
>     switch (params->key_len) {
>     case 16:
>         h->rte_hash_cmp_eq = rte_hash_k16_cmp_eq;
>         break;
> [...]
>         break;
>     default:
>         /* If key is not multiple of 16, use generic memcmp */
>         h->rte_hash_cmp_eq = memcmp;
>     }
> #else
>     h->rte_hash_cmp_eq = memcmp;
> #endif
>
> So that could remove other #ifdef in those lines.
>
> 3. I don't think ask others to write RTE_LIB_MP_NO_FUNC_PTR in makefile
> is a good idea, if you really want to do that, please add a doc so that
> others could know it.
>
> Thanks,
> Michael
>
Dhana Eadala March 9, 2016, 9:36 p.m. UTC | #3 
Hi Michael

If you agree on the #ifdef protection I explained in my previous mail, I
will re-submit the patch with refactoring the the commit log with less than
80 characters per line.

Thanks
Dhana


On Thu, Mar 3, 2016 at 8:00 PM, Dhananjaya Reddy Eadala <edreddy@gmail.com>
wrote:

> Hi Michael
>
> Please see my answers to your comments here.
>
> 1. Sure, I will refactor the commit log to restrict not more than 80
> characters.
>
> 2. Not sure how we can ifdef at the location you mentioned. Can you please
> elaborate more on this.
>     We already have similar ifdef protection to what you suggested and
> with that protection memcmp is assigned.
>     Problem is in using the function pointer to call the compare function.
>     So we need protection for invoking compare function, under
> multi-process environment.
>
> 3. I couldn't come up with any other idea to protect this function pointer
> invocation.
>
> Thanks
> Dhana
>
>
>
>
> On Thu, Mar 3, 2016 at 12:44 AM, Qiu, Michael <michael.qiu@intel.com>
> wrote:
>
>> On 3/3/2016 11:36 AM, Dhana Eadala wrote:
>> > 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>
>> > ---
>> >
>>
>> Some comments:
>>
>> 1.  your commit log need to refactor, better to limit every line less
>> than 80 character.
>>
>> 2. I think you could add the ifdef here in
>> lib/librte_hash/rte_cuckoo_hash.c :
>> /*
>>  * 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)
>>     /* Select function to compare keys */
>>     switch (params->key_len) {
>>     case 16:
>>         h->rte_hash_cmp_eq = rte_hash_k16_cmp_eq;
>>         break;
>> [...]
>>         break;
>>     default:
>>         /* If key is not multiple of 16, use generic memcmp */
>>         h->rte_hash_cmp_eq = memcmp;
>>     }
>> #else
>>     h->rte_hash_cmp_eq = memcmp;
>> #endif
>>
>> So that could remove other #ifdef in those lines.
>>
>> 3. I don't think ask others to write RTE_LIB_MP_NO_FUNC_PTR in makefile
>> is a good idea, if you really want to do that, please add a doc so that
>> others could know it.
>>
>> Thanks,
>> Michael
>>
>
>
diff mbox

Patch

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;