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

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

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
>>
>
>