From: "Qiu, Michael" <michael.qiu@intel.com>
To: Dhana Eadala <edreddy@gmail.com>,
"Richardson, Bruce" <bruce.richardson@intel.com>,
"De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] hash: fix memcmp function pointer in multi-process environment
Date: Thu, 3 Mar 2016 05:44:49 +0000 [thread overview]
Message-ID: <533710CFB86FA344BFBF2D6802E6028622F5BBE8@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <1456976152-15640-1-git-send-email-edreddy@gmail.com>
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
next prev parent reply other threads:[~2016-03-03 5:45 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-03 3:35 Dhana Eadala
2016-03-03 5:44 ` Qiu, Michael [this message]
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=533710CFB86FA344BFBF2D6802E6028622F5BBE8@SHSMSX101.ccr.corp.intel.com \
--to=michael.qiu@intel.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=edreddy@gmail.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).