From mboxrd@z Thu Jan 1 00:00:00 1970 From: santosh Subject: Re: [PATCH 2/4] mempool: detect physical contiguous object in pool Date: Wed, 5 Jul 2017 12:37:56 +0530 Message-ID: References: <20170621173248.1313-1-santosh.shukla@caviumnetworks.com> <20170621173248.1313-3-santosh.shukla@caviumnetworks.com> <20170703183715.3b1c8ff6@platinum> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org, thomas@monjalon.net, hemant.agrawal@nxp.com, jerin.jacob@caviumnetworks.com, bruce.richardson@intel.com To: Olivier Matz Return-path: Received: from NAM03-DM3-obe.outbound.protection.outlook.com (mail-dm3nam03on0083.outbound.protection.outlook.com [104.47.41.83]) by dpdk.org (Postfix) with ESMTP id 1E8781094 for ; Wed, 5 Jul 2017 09:08:14 +0200 (CEST) In-Reply-To: <20170703183715.3b1c8ff6@platinum> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Olivier, On Monday 03 July 2017 10:07 PM, Olivier Matz wrote: > On Wed, 21 Jun 2017 17:32:46 +0000, Santosh Shukla wrote: >> HW mempool blocks may need physical contiguous obj in a pool. > This should be clarified: the memory area containing all the > objects must be physically contiguous, right? ok. >> Introducing MEMPOOL_F_POOL_CONTIG flag for such use-case. The flag >> useful to detect whether all buffer fits within a hugepage or not. If >> not then return -ENOSPC. This way, we make sure that all object within a >> pool is contiguous. >> >> Signed-off-by: Santosh Shukla >> Signed-off-by: Jerin Jacob >> --- >> lib/librte_mempool/rte_mempool.c | 8 ++++++++ >> lib/librte_mempool/rte_mempool.h | 1 + >> 2 files changed, 9 insertions(+) >> >> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c >> index 045baef45..7dec2f51d 100644 >> --- a/lib/librte_mempool/rte_mempool.c >> +++ b/lib/librte_mempool/rte_mempool.c >> @@ -368,6 +368,14 @@ rte_mempool_populate_phys(struct rte_mempool *mp, char *vaddr, >> >> total_elt_sz = mp->header_size + mp->elt_size + mp->trailer_size; >> >> + /* Detect nb_mbuf fit in hugepage */ >> + if (mp->flags & MEMPOOL_F_POOL_CONTIG) { >> + if (len < total_elt_sz * mp->size) { >> + RTE_LOG(ERR, MEMPOOL, "nb_mbufs not fitting in one hugepage,..exit\n"); >> + return -ENOSPC; >> + } >> + } >> + > We should not reference mbuf, we are in mempool code, dealing with > any kind of object. ok, in v2. > Also, len is not necessarily the size of a hugepage, but the size of the > physical area passed to te_mempool_populate_phys(). The idea is to make sure that blk_sz (total_elt_sz * mp->size) fits with in hugepage. So if rte_eal_has_hugepages() is true then 'len' is hugepage size, in-case of non-hugepage this condition would fail. Does that make sense? if so then I'll modify comment and error log. Otherwise could you pl. suggest better approach to detect phys contiguity. >> memhdr = rte_zmalloc("MEMPOOL_MEMHDR", sizeof(*memhdr), 0); >> if (memhdr == NULL) >> return -ENOMEM; >> diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h >> index c3cdc77e4..fd8722e69 100644 >> --- a/lib/librte_mempool/rte_mempool.h >> +++ b/lib/librte_mempool/rte_mempool.h >> @@ -266,6 +266,7 @@ struct rte_mempool { >> #define MEMPOOL_F_SC_GET 0x0008 /**< Default get is "single-consumer".*/ >> #define MEMPOOL_F_POOL_CREATED 0x0010 /**< Internal: pool is created. */ >> #define MEMPOOL_F_NO_PHYS_CONTIG 0x0020 /**< Don't need physically contiguous objs. */ >> +#define MEMPOOL_F_POOL_CONTIG 0x0040 /**< Detect physcially contiguous objs */ > We must highlight here that it's a capability flag. > Following my other comments on the first patch, this define should be > renamed in something else. I suggest: > > #define RTE_MEMPOOL_CAPA_PHYS_CONTIG 0x0001 > > The description should be longer and more accurate. > > I'm also a bit puzzled because this is more a limitation than a > capability. ok with renaming flag but per my [1/4] comment. But i find it makes more sense to - not differentiate PHYS_CONTIG with existing mempool cap flags. >> >> /** >> * @internal When debug is enabled, store some statistics.