From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 0481B2934 for ; Tue, 25 Oct 2016 16:56:49 +0200 (CEST) Received: from lfbn-1-5996-232.w90-110.abo.wanadoo.fr ([90.110.195.232] helo=[192.168.1.13]) by mail.droids-corp.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84_2) (envelope-from ) id 1bz3Cv-00015p-Bi; Tue, 25 Oct 2016 17:00:05 +0200 To: Thomas Monjalon References: <1476351445-18102-1-git-send-email-wei.dai@intel.com> <2601191342CEEE43887BDE71AB9772583F0C12EF@irsmsx105.ger.corp.intel.com> <57FFA2C9.9000603@6wind.com> <2906356.2N5hSckZQm@xps13> Cc: dev@dpdk.org, "Ananyev, Konstantin" , "Dai, Wei" , "Gonzalez Monroy, Sergio" , "Tan, Jianfeng" From: Olivier Matz Message-ID: <7b9f8bf3-7025-8693-71ba-26fe4a00148b@6wind.com> Date: Tue, 25 Oct 2016 16:56:39 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.2.0 MIME-Version: 1.0 In-Reply-To: <2906356.2N5hSckZQm@xps13> Content-Type: text/plain; charset=windows-1252 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 25 Oct 2016 14:56:49 -0000 Hi Thomas, On 10/25/2016 04:37 PM, Thomas Monjalon wrote: > 2016-10-13 17:05, Olivier MATZ: >> 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. > > No answer from Wei Dai. > Please Olivier advise what to do with this patch. > Thanks > I think it's good to have this fix in 16.11. I'm sending a v2 based on Wei's patch. Olivier