Re: [dpdk-dev] [PATCH 1/2] mbuf: update default Mempool ops with HW active pool

[Olivier MATZ <olivier.matz@6wind.com>]

Hi,

On Fri, Dec 15, 2017 at 03:54:42PM +0530, Hemant Agrawal wrote:
> With this patch the specific HW mempool are no longer required to be
> specified in the config file at compile. A default active hw mempool
> can be detected dynamically and published to default mempools ops
> config at run time. Only one type of HW mempool can be active default.
> 
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---
>  lib/librte_mbuf/rte_mbuf.c | 33 ++++++++++++++++++++++++++++++++-
>  lib/librte_mbuf/rte_mbuf.h | 13 +++++++++++++
>  2 files changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 7543662..e074afa 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -148,6 +148,37 @@ rte_pktmbuf_init(struct rte_mempool *mp,
>  	m->next = NULL;
>  }
>  
> +static const char *active_mbuf_pool_ops_name;
> +
> +int
> +rte_pktmbuf_reg_active_mempool_ops(const char *ops_name)

I think active_mempool is not the best name: it is not always active
if the user forces another one.

Since there is only one pool like this, would "platform_mempool" be a
better name?

For naming, I suggest "pktmbuf" can be "mbuf", it's shorter and there is
no need anymore to differentiate with ctrlmbuf, because ctrlmbuf will be
removed soon.  I also think "register" is clearer than "reg".  So, what
about rte_mbuf_register_platform_mempool_ops()?

> +{
> +	if (active_mbuf_pool_ops_name == NULL) {
> +		active_mbuf_pool_ops_name = ops_name;
> +		return 0;
> +	}
> +	RTE_LOG(ERR, MBUF,
> +		"%s is already registered as active pktmbuf pool ops\n",
> +		active_mbuf_pool_ops_name);
> +	return -EACCES;
> +}
> +
> +/* Return mbuf pool ops name */
> +static const char *
> +rte_pktmbuf_active_mempool_ops(void)
> +{
> +	const char *default_ops = rte_eal_mbuf_default_mempool_ops();
> +
> +	/* If mbuf default ops is same as compile time default
> +	 * Just to be sure that no one has updated it by other means.
> +	 */
> +	if ((strcmp(default_ops, RTE_MBUF_DEFAULT_MEMPOOL_OPS) == 0) &&
> +		(active_mbuf_pool_ops_name != NULL))
> +		return active_mbuf_pool_ops_name;
> +	else
> +		return default_ops;
> +}

The name of this function is confusing because it does not really return
the active mempool. If the user selected a pool with
--mbuf-pool-ops-name, it is returned...

...except if --mbuf-pool-ops-name=<name of default ops> was passed,
which I think is also very confusing.