From mboxrd@z Thu Jan 1 00:00:00 1970 From: santosh Subject: Re: [PATCH v4 4/7] mempool: get the mempool capability Date: Mon, 4 Sep 2017 21:59:09 +0530 Message-ID: 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> <20170904155629.usczyoejbozmobkc@neon> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org, thomas@monjalon.net, jerin.jacob@caviumnetworks.com, hemant.agrawal@nxp.com To: Olivier MATZ Return-path: Received: from NAM02-SN1-obe.outbound.protection.outlook.com (mail-sn1nam02on0071.outbound.protection.outlook.com [104.47.36.71]) by dpdk.org (Postfix) with ESMTP id 9B53F7D4B for ; Mon, 4 Sep 2017 18:29:33 +0200 (CEST) In-Reply-To: <20170904155629.usczyoejbozmobkc@neon> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Monday 04 September 2017 09:26 PM, Olivier MATZ wrote: > 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? 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. Current API return info: 0 : for success ..meaning driver supports capability and advertised same by Or-ing to mp->flags (now in v5, mempool driver will update to second flag param) <0 : error. Is return info fine with you for v5. Pl. confirm. Thanks. >> 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. queued v6, Thanks.