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

["Qiu, Michael" <michael.qiu@intel.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