From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk0-f179.google.com (mail-qk0-f179.google.com [209.85.220.179]) by dpdk.org (Postfix) with ESMTP id 699BA8D9E for ; Tue, 8 Sep 2015 15:29:24 +0200 (CEST) Received: by qkcf65 with SMTP id f65so42799308qkc.3 for ; Tue, 08 Sep 2015 06:29:23 -0700 (PDT) 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:content-type; bh=6iLN5Z04vJMx4yt4xOQGfbMDogY3Vx0hN1J55Oxy6ek=; b=V2ZzCxHis9+djrYnsPzZZxSRyZ9L3sm4/gt6mlUZr1NmsA4cAUqzl4nbTauC6PIFwU rteFZeg6K6IPDT4/00qVkAbdRozrt2i8CDz2ebGuddu3ejLPjKYgOjnX5xpJ/qbV/cXv S6lElTNC/ZYNm+s29q7/oxYLQ4e2HtE40b9ATTkt9GWGvik4gfjqMRDcar1YwCC2IaVT cdjG6ddB8d4YpXxdNP0BHmroXoYQKfUqhPLK931tFei3CaJ+XGe3HhRJZr2ZWF6pXaml W5VswujR/4JuTTcyPVLg/wNJx+6hgOncPCJf1r3ZN2moUR0fgcgZzYToIVmWHLAS841K Euaw== X-Gm-Message-State: ALoCoQkwL8rCFvElF++8LfWM5MU5H/DE1Zf7CS0+fbzNXUPOlNDgbidBfQqMiK56g6ARyU7Jbitc MIME-Version: 1.0 X-Received: by 10.55.51.84 with SMTP id z81mr35855328qkz.21.1441718963765; Tue, 08 Sep 2015 06:29:23 -0700 (PDT) Received: by 10.140.89.82 with HTTP; Tue, 8 Sep 2015 06:29:23 -0700 (PDT) In-Reply-To: <55EED876.9050307@intel.com> References: <1441361677-10271-1-git-send-email-ralf.hoffmann@allegro-packets.com> <55EED876.9050307@intel.com> Date: Tue, 8 Sep 2015 08:29:23 -0500 Message-ID: From: Jay Rolette To: "Gonzalez Monroy, Sergio" Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Cc: DPDK Subject: Re: [dpdk-dev] [PATCH v1] change hugepage sorting to avoid overlapping memcpy 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: Tue, 08 Sep 2015 13:29:24 -0000 Most of the code in sort_by_physaddr() should be replaced by a call to qsort() instead. Less code and gets rid of an O(n^2) sort. It's only init code, but given how long EAL init takes, every bit helps. I submitted a patch for this close to a year ago: http://dpdk.org/dev/patchwork/patch/2061/ Jay On Tue, Sep 8, 2015 at 7:45 AM, Gonzalez Monroy, Sergio < sergio.gonzalez.monroy@intel.com> wrote: > Hi Ralf, > > Just a few comments/suggestions: > > Add 'eal/linux:' to the commit title, ie: > "eal/linux: change hugepage sorting to avoid overlapping memcpy" > > On 04/09/2015 11:14, Ralf Hoffmann wrote: > >> with only one hugepage or already sorted hugepage addresses, the sort >> function called memcpy with same src and dst pointer. Debugging with >> valgrind will issue a warning about overlapping area. This patch changes >> the bubble sort to avoid this behavior. Also, the function cannot fail >> any longer. >> >> Signed-off-by: Ralf Hoffmann >> --- >> lib/librte_eal/linuxapp/eal/eal_memory.c | 27 >> +++++++++++++-------------- >> 1 file changed, 13 insertions(+), 14 deletions(-) >> >> diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c >> b/lib/librte_eal/linuxapp/eal/eal_memory.c >> index ac2745e..6d01f61 100644 >> --- a/lib/librte_eal/linuxapp/eal/eal_memory.c >> +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c >> @@ -699,25 +699,25 @@ error: >> * higher address first on powerpc). We use a slow algorithm, but we >> won't >> * have millions of pages, and this is only done at init time. >> */ >> -static int >> +static void >> sort_by_physaddr(struct hugepage_file *hugepg_tbl, struct hugepage_info >> *hpi) >> { >> unsigned i, j; >> - int compare_idx; >> + unsigned compare_idx; >> uint64_t compare_addr; >> struct hugepage_file tmp; >> for (i = 0; i < hpi->num_pages[0]; i++) { >> - compare_addr = 0; >> - compare_idx = -1; >> + compare_addr = hugepg_tbl[i].physaddr; >> + compare_idx = i; >> /* >> - * browse all entries starting at 'i', and find the >> + * browse all entries starting at 'i+1', and find the >> * entry with the smallest addr >> */ >> - for (j=i; j< hpi->num_pages[0]; j++) { >> + for (j=i + 1; j < hpi->num_pages[0]; j++) { >> > Although there are many style/checkpatch issues in current code, we try to > fix them > in new patches. > In that regard, checkpatch complains about above line with: > ERROR:SPACING: spaces required around that '=' > > - if (compare_addr == 0 || >> + if ( >> #ifdef RTE_ARCH_PPC_64 >> hugepg_tbl[j].physaddr > compare_addr) { >> #else >> @@ -728,10 +728,9 @@ sort_by_physaddr(struct hugepage_file *hugepg_tbl, >> struct hugepage_info *hpi) >> } >> } >> - /* should not happen */ >> - if (compare_idx == -1) { >> - RTE_LOG(ERR, EAL, "%s(): error in physaddr >> sorting\n", __func__); >> - return -1; >> + if (compare_idx == i) { >> + /* no smaller page found */ >> + continue; >> } >> /* swap the 2 entries in the table */ >> @@ -741,7 +740,8 @@ sort_by_physaddr(struct hugepage_file *hugepg_tbl, >> struct hugepage_info *hpi) >> sizeof(struct hugepage_file)); >> memcpy(&hugepg_tbl[i], &tmp, sizeof(struct >> hugepage_file)); >> } >> - return 0; >> + >> + return; >> } >> > I reckon checkpatch is not picking this one because the end-of-function is > not part of the patch, > but it is a warning: > WARNING:RETURN_VOID: void function return statements are not generally > useful > > /* >> @@ -1164,8 +1164,7 @@ rte_eal_hugepage_init(void) >> goto fail; >> } >> - if (sort_by_physaddr(&tmp_hp[hp_offset], hpi) < 0) >> - goto fail; >> + sort_by_physaddr(&tmp_hp[hp_offset], hpi); >> #ifdef RTE_EAL_SINGLE_FILE_SEGMENTS >> /* remap all hugepages into single file segments */ >> >> >> > Thanks, > Sergio >