From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Rybchenko Subject: Re: [PATCH v1 2/6] mempool: implement abstract mempool info API Date: Wed, 25 Apr 2018 12:57:41 +0300 Message-ID: References: <1516713372-10572-1-git-send-email-arybchenko@solarflare.com> <1522080779-25472-1-git-send-email-arybchenko@solarflare.com> <1522080779-25472-3-git-send-email-arybchenko@solarflare.com> <20180419164240.dgqwzsc6rubngo3s@platinum> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: , "Artem V. Andreev" To: Olivier Matz Return-path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id AE1084C78 for ; Wed, 25 Apr 2018 11:57:50 +0200 (CEST) In-Reply-To: <20180419164240.dgqwzsc6rubngo3s@platinum> Content-Language: en-GB 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 04/19/2018 07:42 PM, Olivier Matz wrote: > On Mon, Mar 26, 2018 at 05:12:55PM +0100, Andrew Rybchenko wrote: >> From: "Artem V. Andreev" >> >> Primarily, it is intended as a way for the mempool driver to provide >> additional information on how it lays up objects inside the mempool. >> >> Signed-off-by: Artem V. Andreev >> Signed-off-by: Andrew Rybchenko > I think it's a good idea to have a way to query mempool features > or parameters. The approach chosen in this patch looks similar > to what we have with ethdev devinfo, right? Yes. > [...] > >> /** >> + * @warning >> + * @b EXPERIMENTAL: this API may change without prior notice. >> + * >> + * Additional information about the mempool >> + */ >> +struct rte_mempool_info; >> + > [...] > >> +/* wrapper to get additional mempool info */ >> +int >> +rte_mempool_ops_get_info(const struct rte_mempool *mp, >> + struct rte_mempool_info *info) >> +{ >> + struct rte_mempool_ops *ops; >> + >> + ops = rte_mempool_get_ops(mp->ops_index); >> + >> + RTE_FUNC_PTR_OR_ERR_RET(ops->get_info, -ENOTSUP); >> + return ops->get_info(mp, info); >> +} > Thinking in terms of ABI compatibility, it looks that each time we will > add or remove a field, it will break the ABI because the info structure > will change. > > Well, it's maybe nitpicking, because most of the time adding a field in > info structure goes with adding a field in the mempool struct, which > will anyway break the ABI. > > Another approach is to have a function > rte_mempool_info_contig_block_size(mp) whose ABI can be more easily > wrapped with VERSION_SYMBOL(). > > On my side I'm fine with your current approach, especially given how few > usages of VERSION_SYMBOL() we can find in DPDK. But in case you feel > it's better to have a function... I'd prefer to keep current solution. Otherwise it could result in too many different functions to get various information about mempool driver features/characteristics. Also it could be not very convenient to get many parameters. May be we should align info structure size to cache line to avoid size changes in many cases? Typically it will be used on slow path and located on caller stack and adding some bytes more should not be a problem.