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 202FD7CF8 for ; Mon, 4 Sep 2017 17:56:39 +0200 (CEST) Received: from lfbn-1-18623-73.w90-103.abo.wanadoo.fr ([90.103.154.73] helo=droids-corp.org) by mail.droids-corp.org with esmtpsa (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1dotpD-0008Nr-Bx; Mon, 04 Sep 2017 18:02:12 +0200 Received: by droids-corp.org (sSMTP sendmail emulation); Mon, 04 Sep 2017 17:56:30 +0200 Date: Mon, 4 Sep 2017 17:56:30 +0200 From: Olivier MATZ To: santosh Cc: dev@dpdk.org, thomas@monjalon.net, jerin.jacob@caviumnetworks.com, hemant.agrawal@nxp.com Message-ID: <20170904155629.usczyoejbozmobkc@neon> References: <20170720134759.4680-1-santosh.shukla@caviumnetworks.com> <20170815060743.21076-1-santosh.shukla@caviumnetworks.com> <20170815060743.21076-5-santosh.shukla@caviumnetworks.com> <20170904143206.uoc5mniecpb3v744@neon> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v4 4/7] mempool: get the mempool capability X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Sep 2017 15:56:39 -0000 On Mon, Sep 04, 2017 at 08:14:39PM +0530, santosh wrote: > Hi Olivier, > > > On Monday 04 September 2017 08:02 PM, Olivier MATZ wrote: > > On Tue, Aug 15, 2017 at 11:37:40AM +0530, Santosh Shukla wrote: > >> Allow mempool to advertise its capability. > >> A handler been introduced called rte_mempool_ops_get_capabilities. > >> - Upon ->get_capabilities call, mempool driver will advertise > >> capability by updating to 'mp->flags'. > >> > >> Signed-off-by: Santosh Shukla > >> Signed-off-by: Jerin Jacob > >> --- > >> lib/librte_mempool/rte_mempool.c | 5 +++++ > >> lib/librte_mempool/rte_mempool.h | 20 ++++++++++++++++++++ > >> lib/librte_mempool/rte_mempool_ops.c | 14 ++++++++++++++ > >> lib/librte_mempool/rte_mempool_version.map | 7 +++++++ > >> 4 files changed, 46 insertions(+) > >> > >> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c > >> index f95c01c00..d518c53de 100644 > >> --- a/lib/librte_mempool/rte_mempool.c > >> +++ b/lib/librte_mempool/rte_mempool.c > >> @@ -529,6 +529,11 @@ rte_mempool_populate_default(struct rte_mempool *mp) > >> if (mp->nb_mem_chunks != 0) > >> return -EEXIST; > >> > >> + /* Get mempool capability */ > >> + ret = rte_mempool_ops_get_capabilities(mp); > >> + if (ret) > >> + RTE_LOG(DEBUG, MEMPOOL, "get_capability not supported for %s\n", mp->name); > >> + > > there is probably a checkpatch error here (80 cols) > > for debug, line over 80 char warning acceptable, right? > anyways, I will reduce verbose to less than 80 in v5. What do you mean by "for debug"? All lines should be shorter than 80 cols, except if that is not possible without spliting a string or making the code hard to read or maintain. > > >> +/** > >> + * @internal wrapper for mempool_ops get_capabilities callback. > >> + * > >> + * @param mp > >> + * Pointer to the memory pool. > >> + * @return > >> + * - 0: Success; Capability updated to mp->flags > >> + * - <0: Error; code of capability function. > >> + */ > >> +int > >> +rte_mempool_ops_get_capabilities(struct rte_mempool *mp); > >> + > > What does "Capability updated to mp->flags" mean? > > it says that external mempool driver has updated his pool capability in mp->flags. > I'll reword in v5. Please, can you explain what does "update" mean? Is it masked? Or-ed? > > > Why not having instead: > > int rte_mempool_ops_get_capabilities(struct rte_mempool *mp, > > unsigned int *flags); > > > > ? > > No strong opinion, But Since we already passing mempool as param why not update > flag info into mp->flag. >>From an API perspective, we expect that a function called "mempool_ops_get_capabilities" returns something. > However I see your, I guess you want explicitly highlight flag as capability update {action} > in second param, in that case how about keeping first mempool param 'const' like below: > > int rte_mempool_ops_get_capabilities(const struct rte_mempool *mp, > unsigned int *flags); > > are you ok with const change in above API. Yes, adding the const makes sense here.