From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-vk0-f43.google.com (mail-vk0-f43.google.com [209.85.213.43]) by dpdk.org (Postfix) with ESMTP id 52FD72BAD for ; Fri, 4 Mar 2016 02:00:52 +0100 (CET) Received: by mail-vk0-f43.google.com with SMTP id c3so39533688vkb.3 for ; Thu, 03 Mar 2016 17:00:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc; bh=i8R+dvv+gbVpq4Qs+a6OglCtCXTgv6/6IoImaw7X04w=; b=Zq7IvctJR+9NBShF643DOWTSNZ0pxq+z6t5PGbiTusH5Fwa9D6iY6kLBUgHK+q6D1f EUtrO2OUnsVlgFtiLmlJfhhLQz1kxj2737M4O/F+GqlVPzM7oNk2WaVI8ng0BFBX0Bon B4VDQ/2PWzvjkIzV2+rA2YlJEi/iOfuMTO3h0dyEwLAtUY6TLitb4oC8bi/UzMWj6aPQ 0Ob+7irW/jyt0dZqhKAfRRKaW2bj2s5YL0GINMP+Hwfo6n3h1mhvMQJM+BJXV3cdtUtN 5w23xAZebT9MYyMggEe0on6Hyc6xjDnhXAM6jLV/EgqIoZJvT49OhWW+tOZNguErYfOs wFSw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc; bh=i8R+dvv+gbVpq4Qs+a6OglCtCXTgv6/6IoImaw7X04w=; b=Uhy0YdxEdIY38AWhj2gSXy+XYz7QIKye04KUQk4udJaQFrw3+iXiERV5oT2m3ucYpl xeMtFQKgBKtjjTqZE188Ko0vN8KPt43Dl1JNYXT2ku3NSTF2h81rtGoc220u0cxbKRO7 8u0HCfoKkHsUw7BGQxuTM8pC+iQvc0gatpTxnDu8theFg1Yv0VsNR9fgVz/TXZi13DAL NVlJ2JEa9ABs1Hzd+iyEtgsdiS4AU4e3RyztfNLENeLqSPym9elZ8Svic5tOShbRutrE Ne4kM1H8RbNGQ80Pwc3CrpZ4lpWHa1fTSzrZ1mwJooWC4ld4D0SN1T+Ejb/ZLS7O5/MB uuFQ== X-Gm-Message-State: AD7BkJJ6VmtynDdvHGZE+UCIfxwId30e8LCtueCjHTaidrQJuvAuItjtpW3NfDF7htC74dlOdae+1o1kJAhwig== MIME-Version: 1.0 X-Received: by 10.31.141.75 with SMTP id p72mr4256569vkd.13.1457053251740; Thu, 03 Mar 2016 17:00:51 -0800 (PST) Received: by 10.31.237.129 with HTTP; Thu, 3 Mar 2016 17:00:51 -0800 (PST) In-Reply-To: <533710CFB86FA344BFBF2D6802E6028622F5BBE8@SHSMSX101.ccr.corp.intel.com> References: <1456976152-15640-1-git-send-email-edreddy@gmail.com> <533710CFB86FA344BFBF2D6802E6028622F5BBE8@SHSMSX101.ccr.corp.intel.com> Date: Thu, 3 Mar 2016 20:00:51 -0500 Message-ID: From: Dhananjaya Reddy Eadala To: "Qiu, Michael" Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH] hash: fix memcmp function pointer in multi-process environment X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 04 Mar 2016 01:00:52 -0000 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 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 > > --- > > > > 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 >