Re: [dpdk-dev] [PATCH v5 01/13] eal: add param register infrastructure

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

Some suggestions,

On Tue, Oct 16, 2018 at 04:57:50PM +0100, Kevin Laatz wrote:
> This commit adds infrastructure to EAL that allows an application to
> register it's init function with EAL. This allows libraries to be
> initialized at the end of EAL init.
> 
> This infrastructure allows libraries that depend on EAL to be initialized
> as part of EAL init, removing circular dependency issues.
> 
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
> ---
>  lib/librte_eal/bsdapp/eal/Makefile        |  1 +
>  lib/librte_eal/bsdapp/eal/eal.c           | 18 +++++-
>  lib/librte_eal/common/Makefile            |  1 +
>  lib/librte_eal/common/include/rte_param.h | 91 +++++++++++++++++++++++++++++++
>  lib/librte_eal/common/meson.build         |  2 +
>  lib/librte_eal/common/rte_param.c         | 47 ++++++++++++++++
>  lib/librte_eal/linuxapp/eal/Makefile      |  1 +
>  lib/librte_eal/linuxapp/eal/eal.c         | 18 +++++-
>  lib/librte_eal/rte_eal_version.map        |  1 +
>  9 files changed, 178 insertions(+), 2 deletions(-)
>  create mode 100644 lib/librte_eal/common/include/rte_param.h
>  create mode 100644 lib/librte_eal/common/rte_param.c
> 
> diff --git a/lib/librte_eal/bsdapp/eal/Makefile b/lib/librte_eal/bsdapp/eal/Makefile
> index d27da3d..7f4fa7e 100644
> --- a/lib/librte_eal/bsdapp/eal/Makefile
> +++ b/lib/librte_eal/bsdapp/eal/Makefile
> @@ -66,6 +66,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += malloc_elem.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += malloc_heap.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += malloc_mp.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += rte_keepalive.c
> +SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += rte_param.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += rte_service.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += rte_reciprocal.c
>  
> diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
> index d7ae9d6..27b7afc 100644
> --- a/lib/librte_eal/bsdapp/eal/eal.c
> +++ b/lib/librte_eal/bsdapp/eal/eal.c
> @@ -42,6 +42,7 @@
>  #include <rte_devargs.h>
>  #include <rte_version.h>
>  #include <rte_vfio.h>
> +#include <rte_param.h>
>  #include <rte_atomic.h>
>  #include <malloc_heap.h>
>  
> @@ -414,12 +415,20 @@ eal_parse_args(int argc, char **argv)
>  	argvopt = argv;
>  	optind = 1;
>  	optreset = 1;
> +	opterr = 0;
>  
>  	while ((opt = getopt_long(argc, argvopt, eal_short_options,
>  				  eal_long_options, &option_index)) != EOF) {
>  
> -		/* getopt is not happy, stop right now */
> +		/*
> +		 * getopt didn't recognise the option, lets parse the
> +		 * registered options to see if the flag is valid
> +		 */
>  		if (opt == '?') {
> +			ret = rte_param_parse(argv[optind-1]);
> +			if (ret == 0)
> +				continue;
> +
>  			eal_usage(prgname);
>  			ret = -1;
>  			goto out;
> @@ -788,6 +797,13 @@ rte_eal_init(int argc, char **argv)
>  
>  	rte_eal_mcfg_complete();
>  
> +	/* Call each registered callback, if enabled */
> +	ret = rte_param_init();
> +	if (ret < 0) {
> +		rte_eal_init_alert("Callback failed\n");
> +		return -1;
> +	}
> +
>  	return fctret;
>  }
>  
> diff --git a/lib/librte_eal/common/Makefile b/lib/librte_eal/common/Makefile
> index cca6882..8def95a 100644
> --- a/lib/librte_eal/common/Makefile
> +++ b/lib/librte_eal/common/Makefile
> @@ -12,6 +12,7 @@ INC += rte_tailq.h rte_interrupts.h rte_alarm.h
>  INC += rte_string_fns.h rte_version.h
>  INC += rte_eal_memconfig.h rte_malloc_heap.h
>  INC += rte_hexdump.h rte_devargs.h rte_bus.h rte_dev.h rte_class.h
> +INC += rte_param.h
>  INC += rte_pci_dev_feature_defs.h rte_pci_dev_features.h
>  INC += rte_malloc.h rte_keepalive.h rte_time.h
>  INC += rte_service.h rte_service_component.h
> diff --git a/lib/librte_eal/common/include/rte_param.h b/lib/librte_eal/common/include/rte_param.h
> new file mode 100644
> index 0000000..a0635f7
> --- /dev/null
> +++ b/lib/librte_eal/common/include/rte_param.h
> @@ -0,0 +1,91 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2018 Intel Corporation.
> + */
> +
> +#ifndef __INCLUDE_RTE_PARAM_H__
> +#define __INCLUDE_RTE_PARAM_H__
> +
> +/**
> + * @file
> + *
> + * This API introduces the ability to register callbacks with EAL. When
> + * registering a callback, the application also provides an option as part
> + * of the struct used to register. If the option is passed to EAL when
> + * running a DPDK application, the relevant callback will be called at the
> + * end of EAL init.  For example, passing --telemetry will make the
> + * telemetry init be called at the end of EAL init.
> + *
> + * The register API can be used to resolve circular dependency issues
> + * between EAL and the library. The library uses EAL but is also initialized by
> + * EAL. Hence, EAL depends on the init function of the library. The API
> + * introduced in rte_param allows us to register the library init with EAL
> + * (passing a function pointer) and avoid the circular dependency.

This API offers the ability to register options to the EAL command line
and map those options to functions, that will be executed at the end of
EAL initialization. These options will be available as part of the EAL
command line of applications and are dynamically managed.

This is used primarily by DPDK libraries offering command line options.
Currently, this API is limited to registering options without argument.

> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +typedef int (*rte_param_cb)(void);
> +
> +/*

   /**

> + * Structure containing parameters relating to the function being registered
> + * with EAL.

   Structure describing the EAL command line option being registered.

> + */
> +struct rte_param {

As said earlier, rte_option instead.

> +	TAILQ_ENTRY(rte_param) next; /** The next entry in the TAILQ*/
                                    ^ missing a '<' here for doxygen,
                                    and a space after TAILQ.

                                 /**< Next entry. */
                                    or
                                 /**< Next entry in the list. */

                                 Also reads better, TAILQ is
                                 implementation detail, not too useful
                                 in API doc.
                                 Also missing a period.

> +	char *eal_option;            /** The EAL option */
         *opt_str; /**< The option name. */

> +	rte_param_cb cb;             /** Function pointer of the callback */
                 cb; /**< Function called when the option is used. */
> +
> +	/**
> +	 * Enabled flag, should be 0 by default. This is set when the option
> +	 * for the callback is passed to EAL and determines if the callback is
> +	 * called or not.
> +	 */
> +	int enabled;
                 /**< Set when the option is used. */
> +};
> +
> +/**
> + * @internal Check if the passed option is valid

Check if option is registered.

> + *
> + * @param option
> + *  The option to be parsed
> + *
> + * @return
> + *  0 on success
> + * @return
> + *  -1 on fail
> + */
> +int
> +rte_param_parse(char *option);

This should probably be a const char *.
Also, internal functions must not be part of public headers.
Move the prototype to eal_private.h instead.

> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Register a function with EAL. Registering the function will enable the
> + * function to be called at the end of EAL init.
> + *
> + * @param reg_param
> + *  Structure containing various params used to register the function.
> + */

Register an option to the EAL command line.
When recognized, the associated function will
be executed at the end of EAL initialization.

The associated structure must be available the whole time
this option is registered (i.e. not stack memory)

@param option
   Structure describing the option to parse.

> +void __rte_experimental
> +rte_param_register(struct rte_param *reg_param);
> +
> +/**
> + * @internal Iterate through the registered params and call the callback for
> + * the enabled ones.

Iterate through registered options and execute the associated callback
if enabled.

> + *
> + * @return
> + *  0  on success
> + * @return
> + *  -1 on fail
> + */
> +int
> +rte_param_init(void);

This function prototype should be added to eal_private.h instead.

Maybe missing: rte_option_unregister, to be executed in .FINI, if a
plugin is unloaded.

> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif
> diff --git a/lib/librte_eal/common/meson.build b/lib/librte_eal/common/meson.build
> index b7fc984..4069e49 100644
> --- a/lib/librte_eal/common/meson.build
> +++ b/lib/librte_eal/common/meson.build
> @@ -33,6 +33,7 @@ common_sources = files(
>  	'malloc_mp.c',
>  	'rte_keepalive.c',
>  	'rte_malloc.c',
> +	'rte_param.c',
>  	'rte_reciprocal.c',
>  	'rte_service.c'
>  )
> @@ -70,6 +71,7 @@ common_headers = files(
>  	'include/rte_malloc_heap.h',
>  	'include/rte_memory.h',
>  	'include/rte_memzone.h',
> +	'include/rte_param.h',
>  	'include/rte_pci_dev_feature_defs.h',
>  	'include/rte_pci_dev_features.h',
>  	'include/rte_per_lcore.h',
> diff --git a/lib/librte_eal/common/rte_param.c b/lib/librte_eal/common/rte_param.c
> new file mode 100644
> index 0000000..317e371
> --- /dev/null
> +++ b/lib/librte_eal/common/rte_param.c
> @@ -0,0 +1,47 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2018 Intel Corporation.
> + */
> +
> +#include <unistd.h>
> +#include <string.h>
> +
> +#include <rte_eal.h>
> +#include <rte_param.h>
> +
> +TAILQ_HEAD(rte_param_list, rte_param);
> +
> +struct rte_param_list rte_param_list =
> +	TAILQ_HEAD_INITIALIZER(rte_param_list);
> +
> +static struct rte_param *param;
> +
> +int
> +rte_param_parse(char *option)
> +{
> +	/* Check if the option is in the registered inits */
> +	TAILQ_FOREACH(param, &rte_param_list, next) {
> +		if (strcmp(option, param->eal_option) == 0) {
> +			param->enabled = 1;
> +			return 0;
> +		}
> +	}
> +
> +	return -1;
> +}
> +
> +void __rte_experimental
> +rte_param_register(struct rte_param *reg_param)
> +{
> +	TAILQ_INSERT_HEAD(&rte_param_list, reg_param, next);

What happens when an option already exists in the list?

> +}
> +
> +int
> +rte_param_init(void)
> +{
> +	TAILQ_FOREACH(param, &rte_param_list, next) {
> +		if (param->enabled)
> +			param->cb();
> +	}
> +
> +	return 0;
> +}
> diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile
> index 5c16bc4..2bf8b24 100644
> --- a/lib/librte_eal/linuxapp/eal/Makefile
> +++ b/lib/librte_eal/linuxapp/eal/Makefile
> @@ -74,6 +74,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += malloc_elem.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += malloc_heap.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += malloc_mp.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_keepalive.c
> +SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_param.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_service.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_reciprocal.c
>  
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
> index 4a55d3b..e28562b 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -48,6 +48,7 @@
>  #include <rte_atomic.h>
>  #include <malloc_heap.h>
>  #include <rte_vfio.h>
> +#include <rte_param.h>
>  
>  #include "eal_private.h"
>  #include "eal_thread.h"
> @@ -600,12 +601,20 @@ eal_parse_args(int argc, char **argv)
>  
>  	argvopt = argv;
>  	optind = 1;
> +	opterr = 0;
>  
>  	while ((opt = getopt_long(argc, argvopt, eal_short_options,
>  				  eal_long_options, &option_index)) != EOF) {
>  
> -		/* getopt is not happy, stop right now */
> +		/*
> +		 * getopt didn't recognise the option, lets parse the
> +		 * registered options to see if the flag is valid
> +		 */
>  		if (opt == '?') {
> +			ret = rte_param_parse(argv[optind-1]);
> +			if (ret == 0)
> +				continue;
> +
>  			eal_usage(prgname);
>  			ret = -1;
>  			goto out;
> @@ -1071,6 +1080,13 @@ rte_eal_init(int argc, char **argv)
>  
>  	rte_eal_mcfg_complete();
>  
> +	/* Call each registered callback, if enabled */
> +	ret = rte_param_init();
> +	if (ret < 0) {
> +		rte_eal_init_alert("Callback failed\n");

It would be better to be able to know which function failed
and why. "Callback failed" is not helpful to the user.
Maybe rte_option_init() return value should be expanded to allow
for error string, error value to be passed, that could come from the
library itself, or simply printing the option name that is the source of
the error.

> +		return -1;
> +	}
> +
>  	return fctret;
>  }
>  
> diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
> index 73282bb..ccfb8a2 100644
> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -341,6 +341,7 @@ EXPERIMENTAL {
>  	rte_mp_request_sync;
>  	rte_mp_request_async;
>  	rte_mp_sendmsg;
> +	rte_param_register;
>  	rte_service_lcore_attr_get;
>  	rte_service_lcore_attr_reset_all;
>  	rte_service_may_be_active;
> -- 
> 2.9.5
> 

Best regards,
-- 
Gaëtan Rivet
6WIND