From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 86A478D9E for ; Tue, 8 Sep 2015 16:24:36 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga102.jf.intel.com with ESMTP; 08 Sep 2015 07:24:34 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.17,490,1437462000"; d="scan'208,217";a="800671735" Received: from smonroyx-mobl.ger.corp.intel.com (HELO [10.237.220.57]) ([10.237.220.57]) by fmsmga002.fm.intel.com with ESMTP; 08 Sep 2015 07:24:28 -0700 Message-ID: <55EEEF99.2000708@intel.com> Date: Tue, 08 Sep 2015 15:24:26 +0100 From: "Gonzalez Monroy, Sergio" User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Jay Rolette References: <1441361677-10271-1-git-send-email-ralf.hoffmann@allegro-packets.com> <55EED876.9050307@intel.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 14:24:37 -0000 On 08/09/2015 14:29, Jay Rolette wrote: > 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. > Fair enough. Actually, we already use qsort in lib/librte_eal/linuxapp/eal/eal_hugeapge_info.c > I submitted a patch for this close to a year ago: > http://dpdk.org/dev/patchwork/patch/2061/ > I just had a quick look at it and seems to be archived with 'Changes Requested' status. I will comment on it. Sergio > Jay > > On Tue, Sep 8, 2015 at 7:45 AM, Gonzalez Monroy, Sergio > > 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 > >