DPDK patches and discussions
 help / color / mirror / Atom feed
From: Dhananjaya Reddy Eadala <edreddy@gmail.com>
To: "Qiu, Michael" <michael.qiu@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 20:00:51 -0500	[thread overview]
Message-ID: <CAF0y47GVqZTrTLk8Xj_dikyyt1KGbUshmz+FDtN2uEyHjHox_g@mail.gmail.com> (raw)
In-Reply-To: <533710CFB86FA344BFBF2D6802E6028622F5BBE8@SHSMSX101.ccr.corp.intel.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
>

  reply	other threads:[~2016-03-04  1:00 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
2016-03-04  1:00   ` Dhananjaya Reddy Eadala [this message]
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=CAF0y47GVqZTrTLk8Xj_dikyyt1KGbUshmz+FDtN2uEyHjHox_g@mail.gmail.com \
    --to=edreddy@gmail.com \
    --cc=dev@dpdk.org \
    --cc=michael.qiu@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).