From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-vk0-f54.google.com (mail-vk0-f54.google.com [209.85.213.54]) by dpdk.org (Postfix) with ESMTP id 4F3C92716 for ; Wed, 9 Mar 2016 22:36:18 +0100 (CET) Received: by mail-vk0-f54.google.com with SMTP id k1so72785332vkb.0 for ; Wed, 09 Mar 2016 13:36:18 -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=KD40fuXip9p2XOZi/2ZK8mLW9GgecbzrPVfF/gNckXU=; b=jfs6l0AsEpCThNjzsBVBrMOG++JCtieW52vjFuiKhiT1ps8guwLGwGaU0fM8jyqKTb mXh54uVmujYdzrv9MESwXA4i6z9/jVhpllVemUT/Otqk4P0I7ynzeqSGslkCzr3CN0Z+ Wgfq8jCDQf3S2MxqRhN31RIL6B0fL/swK8Pm9fSdAmGsGCqqm700oZFZIXojJ8z6NTBG IucLAEM4+dXW1i1c5/MxZifaw2HuEd1CGO/zD4iEfqi7e2/r9q4t2E2LGFNlkweccpaM L2XXk+iAxWtNd3dNzik5VeQzBimwojHoXgBMF6IydUVFDRF5K40TTSefZUK7lGLsuulb LMkg== 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=KD40fuXip9p2XOZi/2ZK8mLW9GgecbzrPVfF/gNckXU=; b=H80kRW9WX4hA9jfhE2YKlWVVePdkru/vM2lXAjF1xOLf8hKLXQFWS/Z7+GLh++5muE G5TZtuHJ1HF6WIOvZ59LXxxoF5EwQ1ylfBqjV1S1aLRpQn7W8A9SToDEjZUTliRv/mZz SUv7HVcv1YnGPmgqhMXbR5o1lyLX8OtthLaYlKHrQon4scRnwhVViC6Aj/3kbLv26LHW z9Rad/ApCEquUFekLJONX5+inIcb/hVSZB2L2d+JuSs6GTamdGLIRf+m230wF2/r5GLc M684oR1VSq8aAtyxDm3JGuRKMuje7rpgKCFmNzpXDA/vmnY/q5LKV6MVUsJbd73FZl0i NABg== X-Gm-Message-State: AD7BkJItITPvIEmicygr/AJam2x7T6z0qqMrXhAHb5xjftUjYGPZVFk+s8mazhBOCPMf3RkMtWpmLXmHQNUWRg== MIME-Version: 1.0 X-Received: by 10.31.141.194 with SMTP id p185mr433734vkd.57.1457559377808; Wed, 09 Mar 2016 13:36:17 -0800 (PST) Received: by 10.31.197.135 with HTTP; Wed, 9 Mar 2016 13:36:17 -0800 (PST) In-Reply-To: References: <1456976152-15640-1-git-send-email-edreddy@gmail.com> <533710CFB86FA344BFBF2D6802E6028622F5BBE8@SHSMSX101.ccr.corp.intel.com> Date: Wed, 9 Mar 2016 16:36:17 -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: Wed, 09 Mar 2016 21:36:18 -0000 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 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 > 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 >> > >