From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <rolette@infiniteio.com>
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 <dev@dpdk.org>; Tue, 8 Sep 2015 15:29:24 +0200 (CEST)
Received: by qkcf65 with SMTP id f65so42799308qkc.3
for <dev@dpdk.org>; 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: <CADNuJVprhcF455jk3Z=siGQLEqq_iP_Za6KAtTou4p6LYe_YKQ@mail.gmail.com>
From: Jay Rolette <rolette@infiniteio.com>
To: "Gonzalez Monroy, Sergio" <sergio.gonzalez.monroy@intel.com>
Content-Type: text/plain; charset=UTF-8
X-Content-Filtered-By: Mailman/MimeDel 2.1.15
Cc: DPDK <dev@dpdk.org>
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 <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=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 <ralf.hoffmann@allegro-packets.com>
>> ---
>> 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
>