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

[Dhananjaya Reddy Eadala <edreddy@gmail.com>]

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
>