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 B6D7F58C5 for ; Thu, 22 Sep 2016 09:23:26 +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 1bmyOp-0007gO-H2; Thu, 22 Sep 2016 09:26:28 +0200 To: Ferruh Yigit , dev@dpdk.org References: <20160920161708.9707-1-ferruh.yigit@intel.com> <20160920161708.9707-2-ferruh.yigit@intel.com> <4227c29d-30b3-6c8f-46cb-6aba60add843@6wind.com> <263a5b89-987e-1b40-7eab-a39de596831e@intel.com> From: Olivier Matz Message-ID: <822e5374-b143-f5b4-33ef-2a502ca57aff@6wind.com> Date: Thu, 22 Sep 2016 09:23:18 +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: <263a5b89-987e-1b40-7eab-a39de596831e@intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 2/2] mempool: fix comments for no contiguous flag 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: Thu, 22 Sep 2016 07:23:26 -0000 Hi Ferruh, On 09/21/2016 06:15 PM, Ferruh Yigit wrote: > Hi Olivier, > > On 9/21/2016 4:12 PM, Olivier Matz wrote: >> Hi Ferruh, >> >> On 09/20/2016 06:17 PM, Ferruh Yigit wrote: >>> Fixes: ce94a51ff05c ("mempool: add flag for removing phys contiguous constraint") >>> >>> Signed-off-by: Ferruh Yigit >>> --- >>> lib/librte_mempool/rte_mempool.c | 4 +++- >>> lib/librte_mempool/rte_mempool.h | 3 +++ >>> 2 files changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c >>> index e96d14f..d767f39 100644 >>> --- a/lib/librte_mempool/rte_mempool.c >>> +++ b/lib/librte_mempool/rte_mempool.c >>> @@ -340,7 +340,9 @@ rte_mempool_free_memchunks(struct rte_mempool *mp) >>> } >>> >>> /* Add objects in the pool, using a physically contiguous memory >>> - * zone. Return the number of objects added, or a negative value >>> + * zone if MEMPOOL_F_NO_PHYS_CONTIG flag is not set. >>> + * >>> + * Return the number of objects added, or a negative value >>> * on error. >>> */ >>> int >>> diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h >>> index 6fc331a..291c168 100644 >>> --- a/lib/librte_mempool/rte_mempool.h >>> +++ b/lib/librte_mempool/rte_mempool.h >>> @@ -798,6 +798,9 @@ rte_mempool_free(struct rte_mempool *mp); >>> * Add a virtually and physically contiguous memory chunk in the pool >>> * where objects can be instanciated. >>> * >>> + * mempool flag MEMPOOL_F_NO_PHYS_CONTIG changes behavior of the function >>> + * and removes physical contiguous constraint. >>> + * >>> * @param mp >>> * A pointer to the mempool structure. >>> * @param vaddr >>> >> >> Not sure I'm getting why you add these comments, as I don't see any >> usage of MEMPOOL_F_NO_PHYS_CONTIG in rte_mempool_populate_phys(). >> >> Could you describe what makes you think the API comments are wrong here? > > rte_mempool_populate_phys() now can get RTE_BAD_PHYS_ADDR as "paddr" > variable, and when paddr == RTE_BAD_PHYS_ADDR, function no more > quarantines that populated items are physically continuous. > > Like how this is used in rte_mempool_populate_phys_tab(), if > MEMPOOL_F_NO_PHYS_CONTIG is set rte_mempool_populate_phys() called for > whole virtually continuous memory (len: pg_num * pg_size), so resulting > items may not be in physically continuous addresses, so function comment > becomes wrong. > > Although MEMPOOL_F_NO_PHYS_CONTIG is not used within the > rte_mempool_populate_phys(), as far as I can see flag has a side effect > on the functionality. > > And please help on wording if it is not accurate. OK, so I think we should just talk about paddr in the API comment of rte_mempool_populate_phys(), not about the flag which is already documented in mempool_create*(). I suggest the following: --- a/lib/librte_mempool/rte_mempool.h +++ b/lib/librte_mempool/rte_mempool.h @@ -800,6 +800,10 @@ rte_mempool_free(struct rte_mempool *mp); * Add a virtually and physically contiguous memory chunk in the pool * where objects can be instanciated. * + * If the given physical address is unknown (paddr = RTE_BAD_PHYS_ADDR), + * the chunk doesn't need to be physically contiguous (only virtually), + * and allocated objects may span two pages. + * * @param mp * A pointer to the mempool structure. * @param vaddr What do you think? Regards Olivier