Re: [dpdk-dev] [PATCH v3 3/7] mbuf: add pool ops name selection API helpers

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

On Thu, Jan 18, 2018 at 06:56:28PM +0530, Hemant Agrawal wrote:
> This patch add support for various mempool ops config helper APIs.
> 
> 1.User defined mempool ops
> 2.Platform detected HW mempool ops (active).
> 3.Best selection of mempool ops by looking into user defined,
>   platform registered and compile time configured.
> 
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---

...

> --- /dev/null
> +++ b/lib/librte_mbuf/rte_mbuf_pool_ops.c
> @@ -0,0 +1,68 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2018 NXP
> + */
> +
> +#include <string.h>
> +#include <rte_eal.h>
> +#include <rte_mbuf.h>
> +#include <rte_errno.h>
> +#include <rte_mbuf_pool_ops.h>
> +#include <rte_malloc.h>
> +
> +static char *plat_mbuf_pool_ops_name;

I have some doubts about secondary processes.

Maybe it's ok if the loaded driver and eal arguments are exactly the
same in the secondary process. It would be safer to use a named memzone
for that.

It would be even safer to not use secondary processes ;)


> +
> +int
> +rte_mbuf_register_platform_mempool_ops(const char *ops_name)
> +{

We have "register" for platform and "set" for user.
I think "set" should be used everywhere.

> +	if (plat_mbuf_pool_ops_name == NULL) {
> +		plat_mbuf_pool_ops_name =
> +			rte_malloc(NULL, RTE_MEMPOOL_OPS_NAMESIZE, 0);
> +		if (plat_mbuf_pool_ops_name == NULL)
> +			return -ENOMEM;
> +		strcpy((char *)plat_mbuf_pool_ops_name, ops_name);

If strlen(ops_name) >= RTE_MEMPOOL_OPS_NAMESIZE, this may lead to
bad behavior.

I suggest to simply do a strdup() instead.


> +		return 0;
> +	} else if (strcmp(plat_mbuf_pool_ops_name, ops_name) == 0) {
> +		return 0;
> +	}
> +
> +	RTE_LOG(ERR, MBUF,
> +		"%s is already registered as platform mbuf pool ops\n",
> +		plat_mbuf_pool_ops_name);

So, this log means that a we should try to never have 2 drivers registering
different platform drivers on the same machine, right?

So this API is kind of reserved for network processors and should not be
used in usual PCI PMDs?


> +	return -EEXIST;
> +}
> +
> +const char *
> +rte_mbuf_platform_mempool_ops(void)
> +{
> +	return (const char *)plat_mbuf_pool_ops_name;

cast is not required

> +}
> +
> +void
> +rte_mbuf_set_user_mempool_ops(const char *ops_name)
> +{
> +	rte_eal_set_mbuf_user_mempool_ops(ops_name);
> +}

Instead of calling the EAL API, we can set a static variable as
for platform ops.

> +
> +const char *
> +rte_mbuf_user_mempool_ops(void)
> +{
> +	return rte_eal_mbuf_default_mempool_ops();
> +}

And here, I suggest instead:

	rte_mbuf_user_mempool_ops(void)
	{
		if (user_mbuf_pool_ops_name != NULL)
			return user_mbuf_pool_ops_name;
		return rte_eal_mbuf_default_mempool_ops();
	}

i.e. rte_eal_mbuf_default_mempool_ops() remains the ops passed as
command line arguments.


> +
> +/* Return mbuf pool ops name */
> +const char *
> +rte_mbuf_best_mempool_ops(void)
> +{
> +	/* User defined mempool ops takes the priority */
> +	const char *best_ops = rte_mbuf_user_mempool_ops();
> +	if (best_ops)
> +		return best_ops;
> +
> +	/* Next choice is platform configured mempool ops */
> +	best_ops = rte_mbuf_platform_mempool_ops();
> +	if (best_ops)
> +		return best_ops;
> +
> +	/* Last choice is to use the compile time config pool */
> +	return RTE_MBUF_DEFAULT_MEMPOOL_OPS;
> +}

I like this function, this is much clearer than what we have today :)