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.agra...@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 :)
