From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <olivier.matz@6wind.com>
Received: from proxy.6wind.com (host.76.145.23.62.rev.coltfrance.com
 [62.23.145.76]) by dpdk.org (Postfix) with ESMTP id C614929CF
 for <dev@dpdk.org>; Thu, 13 Oct 2016 17:05:49 +0200 (CEST)
Received: from [10.16.0.195] (unknown [10.16.0.195])
 by proxy.6wind.com (Postfix) with ESMTP id 3F24425073;
 Thu, 13 Oct 2016 17:05:47 +0200 (CEST)
To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
 "Dai, Wei" <wei.dai@intel.com>, "dev@dpdk.org" <dev@dpdk.org>,
 "Gonzalez Monroy, Sergio" <sergio.gonzalez.monroy@intel.com>,
 "Tan, Jianfeng" <jianfeng.tan@intel.com>
References: <1476351445-18102-1-git-send-email-wei.dai@intel.com>
 <2601191342CEEE43887BDE71AB9772583F0C1178@irsmsx105.ger.corp.intel.com>
 <2601191342CEEE43887BDE71AB9772583F0C11A5@irsmsx105.ger.corp.intel.com>
 <49759EB36A64CF4892C1AFEC9231E8D63A2D3E10@PGSMSX106.gar.corp.intel.com>
 <2601191342CEEE43887BDE71AB9772583F0C12EF@irsmsx105.ger.corp.intel.com>
From: Olivier MATZ <olivier.matz@6wind.com>
Message-ID: <57FFA2C9.9000603@6wind.com>
Date: Thu, 13 Oct 2016 17:05:45 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Icedove/38.6.0
MIME-Version: 1.0
In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0C12EF@irsmsx105.ger.corp.intel.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [dpdk-dev] [PATCH] mempool: fix search of maximum contiguous
 pages
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: Thu, 13 Oct 2016 15:05:49 -0000

Hi Wei,

On 10/13/2016 02:31 PM, Ananyev, Konstantin wrote:
>
>>
>>>>> diff --git a/lib/librte_mempool/rte_mempool.c
>>>>> b/lib/librte_mempool/rte_mempool.c
>>>>> index 71017e1..e3e254a 100644
>>>>> --- a/lib/librte_mempool/rte_mempool.c
>>>>> +++ b/lib/librte_mempool/rte_mempool.c
>>>>> @@ -426,9 +426,12 @@ rte_mempool_populate_phys_tab(struct
>>>>> rte_mempool *mp, char *vaddr,
>>>>>
>>>>>   	for (i = 0; i < pg_num && mp->populated_size < mp->size; i += n) {
>>>>>
>>>>> +		phys_addr_t paddr_next;
>>>>> +		paddr_next = paddr[i] + pg_sz;
>>>>> +
>>>>>   		/* populate with the largest group of contiguous pages */
>>>>>   		for (n = 1; (i + n) < pg_num &&
>>>>> -			     paddr[i] + pg_sz == paddr[i+n]; n++)
>>>>> +			     paddr_next == paddr[i+n]; n++, paddr_next += pg_sz)
>>>>>   			;
>>>>
>>>> Good catch.
>>>> Why not just paddr[i + n - 1] != paddr[i + n]?
>>>
>>> Sorry, I meant 'paddr[i + n - 1] + pg_sz == paddr[i+n]' off course.
>>>
>>>> Then you don't need extra variable (paddr_next) here.
>>>> Konstantin
>>
>> Thank you, Konstantin
>> 'paddr[i + n - 1] + pg_sz = paddr[i + n]' also can fix it and have straight meaning.
>> But I assume that my revision with paddr_next += pg_sz may have a bit better performance.
>
> I don't think there would be any real difference, again it is not performance critical code-path.
>
>> By the way, paddr[i] + n * pg_sz = paddr[i + n] can also resolve it.
>
> Yes, that's one seems even better for me - make things more clear.

Thank you for fixing this.

My vote would go for "addr[i + n - 1] + pg_sz == paddr[i + n]"

If you feel "paddr[i] + n * pg_sz = paddr[i + n]" is clearer, I have no 
problem with it either.

Regards,
Olivier