[v5,01/13] eal: add param register infrastructure
Checks
Commit Message
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
Comments
I still think all the wording is incorrect.
Please start by describing what is "param", "flag" and "option" in your mind.
They are all mentioned in this file.
Are you sure rte_param is the right name?
16/10/2018 17:57, Kevin Laatz:
> --- /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.
> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +typedef int (*rte_param_cb)(void);
> +
> +/*
> + * Structure containing parameters relating to the function being registered
> + * with EAL.
> + */
> +struct rte_param {
> + TAILQ_ENTRY(rte_param) next; /** The next entry in the TAILQ*/
> + char *eal_option; /** The EAL option */
> + rte_param_cb cb; /** Function pointer of the callback */
> +
> + /**
> + * 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;
> +};
> +
> +/**
> + * @internal Check if the passed option is valid
> + *
> + * @param option
> + * The option to be parsed
> + *
> + * @return
> + * 0 on success
> + * @return
> + * -1 on fail
> + */
> +int
> +rte_param_parse(char *option);
> +
> +/**
> + * @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.
> + */
> +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.
> + *
> + * @return
> + * 0 on success
> + * @return
> + * -1 on fail
> + */
> +int
> +rte_param_init(void);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif
[...]
> --- /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);
> +}
> +
> +int
> +rte_param_init(void)
> +{
> + TAILQ_FOREACH(param, &rte_param_list, next) {
> + if (param->enabled)
> + param->cb();
> + }
> +
> + return 0;
> +}
Hi Thomas,
On Wed, Oct 17, 2018 at 11:41:53AM +0200, Thomas Monjalon wrote:
> I still think all the wording is incorrect.
> Please start by describing what is "param", "flag" and "option" in your mind.
> They are all mentioned in this file.
> Are you sure rte_param is the right name?
>
I suggested the name rte_param, I think the original proposal was
rte_lib_init, which to me unduly diminished the intent of these structures.
I think rte_param seems proper, this is a generic parameter object
description. The integer "enabled" is described as a flag in the
structure, as it is used to flag the init routine down the road to
trigger the init callback associated with this parameter.
eal_option is reminiscent of optarg / optind of getopt() family,
which seems fitting.
I don't mean to overstep Kevin's role defending his work, but given
that I proposed some of this naming and pushed for this direction to be
taken in the first place, I feel I should help explain my propositions.
rte_param could become rte_parameter or rte_option instead, eal_option
could become opt_string or opt_str, and so on, do you have specific
ideas about proper names?
>
> 16/10/2018 17:57, Kevin Laatz:
> > --- /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.
> > + *
Citing --telemetry here is a bad idea, this file is lib-agnostic,
--telemetry is not assured to be relevant.
> > + * 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.
> > + */
17/10/2018 13:45, Gaëtan Rivet:
> Hi Thomas,
>
> On Wed, Oct 17, 2018 at 11:41:53AM +0200, Thomas Monjalon wrote:
> > I still think all the wording is incorrect.
> > Please start by describing what is "param", "flag" and "option" in your mind.
> > They are all mentioned in this file.
> > Are you sure rte_param is the right name?
> >
>
> I suggested the name rte_param, I think the original proposal was
> rte_lib_init, which to me unduly diminished the intent of these structures.
I think the right word is "run-time option".
An option can have a parameter.
If this API is not supporting options with parameters, the name is
really misleading.
> I think rte_param seems proper, this is a generic parameter object
> description. The integer "enabled" is described as a flag in the
> structure, as it is used to flag the init routine down the road to
> trigger the init callback associated with this parameter.
"enabled" can be documented as the result of the option parsing.
If the option is given to rte_eal_init, it becomes enabled.
> eal_option is reminiscent of optarg / optind of getopt() family,
> which seems fitting.
>
> I don't mean to overstep Kevin's role defending his work, but given
> that I proposed some of this naming and pushed for this direction to be
> taken in the first place, I feel I should help explain my propositions.
>
> rte_param could become rte_parameter or rte_option instead, eal_option
> could become opt_string or opt_str, and so on, do you have specific
> ideas about proper names?
rte_option looks OK.
The global picture may be better explained I think.
Any help with wording and documentation is appreciated, thanks.
> > 16/10/2018 17:57, Kevin Laatz:
> > > --- /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.
> > > + *
>
> Citing --telemetry here is a bad idea, this file is lib-agnostic,
> --telemetry is not assured to be relevant.
>
> > > + * 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.
> > > + */
>
>
Hi Thomas,
On 17/10/2018 12:45, Gaëtan Rivet wrote:
> Hi Thomas,
>
> On Wed, Oct 17, 2018 at 11:41:53AM +0200, Thomas Monjalon wrote:
>> I still think all the wording is incorrect.
>> Please start by describing what is "param", "flag" and "option" in your mind.
>> They are all mentioned in this file.
>> Are you sure rte_param is the right name?
> I suggested the name rte_param, I think the original proposal was
> rte_lib_init, which to me unduly diminished the intent of these structures.
>
> I think rte_param seems proper, this is a generic parameter object
> description. The integer "enabled" is described as a flag in the
> structure, as it is used to flag the init routine down the road to
> trigger the init callback associated with this parameter.
The purpose of "enabled" is to avoid redundantly calling all of the
callbacks in rte_param_list. Enabled should always be initialized to 0
by the library (as described in the Doxygen comment) and it will be set
if the 'option' for the callback is passed to EAL when running the
application (this is done in rte_param_parse).
To avoid confusion, I have renamed eal_flag to eal_option in the param
struct and reworded the Doxygen comments accordingly. The only place
flag is mentioned now is for "enabled".
>
> eal_option is reminiscent of optarg / optind of getopt() family,
> which seems fitting.
>
> I don't mean to overstep Kevin's role defending his work, but given
> that I proposed some of this naming and pushed for this direction to be
> taken in the first place, I feel I should help explain my propositions.
>
> rte_param could become rte_parameter or rte_option instead, eal_option
> could become opt_string or opt_str, and so on, do you have specific
> ideas about proper names?
I think rte_param was an improvement on rte_lib_init, for reasons
mentioned by Gaetan.
I am open to renaming it if you have any better, more descriptive name
in mind?
<snip>
Thanks,
Kevin
Hi Thomas,
On 17/10/2018 14:46, Thomas Monjalon wrote:
> 17/10/2018 13:45, Gaëtan Rivet:
>> Hi Thomas,
>>
>> On Wed, Oct 17, 2018 at 11:41:53AM +0200, Thomas Monjalon wrote:
>>> I still think all the wording is incorrect.
>>> Please start by describing what is "param", "flag" and "option" in your mind.
>>> They are all mentioned in this file.
>>> Are you sure rte_param is the right name?
>>>
>> I suggested the name rte_param, I think the original proposal was
>> rte_lib_init, which to me unduly diminished the intent of these structures.
> I think the right word is "run-time option".
> An option can have a parameter.
> If this API is not supporting options with parameters, the name is
> really misleading.
The option will be passed like any other EAL option. Right now it
doesn't support any parameters but in future we could add functionality
like we had with the --vdev solution where we can pass selftest=1 with
the telemetry option.
>
>> I think rte_param seems proper, this is a generic parameter object
>> description. The integer "enabled" is described as a flag in the
>> structure, as it is used to flag the init routine down the road to
>> trigger the init callback associated with this parameter.
> "enabled" can be documented as the result of the option parsing.
> If the option is given to rte_eal_init, it becomes enabled.
The Doxygen comments mention that the flag should initially be set to 0
and will be set to 1 if the option for the relevant callback is passed
to EAL when running your application.
>
>> eal_option is reminiscent of optarg / optind of getopt() family,
>> which seems fitting.
>>
>> I don't mean to overstep Kevin's role defending his work, but given
>> that I proposed some of this naming and pushed for this direction to be
>> taken in the first place, I feel I should help explain my propositions.
>>
>> rte_param could become rte_parameter or rte_option instead, eal_option
>> could become opt_string or opt_str, and so on, do you have specific
>> ideas about proper names?
> rte_option looks OK.
I'm happy to change it to rte_option if we have consensus on it :)
>
> The global picture may be better explained I think.
> Any help with wording and documentation is appreciated, thanks.
Thanks,
Kevin
17/10/2018 16:09, Laatz, Kevin:
> Hi Thomas,
>
>
> On 17/10/2018 14:46, Thomas Monjalon wrote:
> > 17/10/2018 13:45, Gaëtan Rivet:
> >> Hi Thomas,
> >>
> >> On Wed, Oct 17, 2018 at 11:41:53AM +0200, Thomas Monjalon wrote:
> >>> I still think all the wording is incorrect.
> >>> Please start by describing what is "param", "flag" and "option" in your mind.
> >>> They are all mentioned in this file.
> >>> Are you sure rte_param is the right name?
> >>>
> >> I suggested the name rte_param, I think the original proposal was
> >> rte_lib_init, which to me unduly diminished the intent of these structures.
> > I think the right word is "run-time option".
> > An option can have a parameter.
> > If this API is not supporting options with parameters, the name is
> > really misleading.
> The option will be passed like any other EAL option. Right now it
> doesn't support any parameters but in future we could add functionality
> like we had with the --vdev solution where we can pass selftest=1 with
> the telemetry option.
> >
> >> I think rte_param seems proper, this is a generic parameter object
> >> description. The integer "enabled" is described as a flag in the
> >> structure, as it is used to flag the init routine down the road to
> >> trigger the init callback associated with this parameter.
> > "enabled" can be documented as the result of the option parsing.
> > If the option is given to rte_eal_init, it becomes enabled.
> The Doxygen comments mention that the flag should initially be set to 0
> and will be set to 1 if the option for the relevant callback is passed
> to EAL when running your application.
Saying "passed to EAL" is too vague.
You need to differentiate the option parsing and the init.
Both are done in EAL, that's why you need to be more specific.
> >> eal_option is reminiscent of optarg / optind of getopt() family,
> >> which seems fitting.
> >>
> >> I don't mean to overstep Kevin's role defending his work, but given
> >> that I proposed some of this naming and pushed for this direction to be
> >> taken in the first place, I feel I should help explain my propositions.
> >>
> >> rte_param could become rte_parameter or rte_option instead, eal_option
> >> could become opt_string or opt_str, and so on, do you have specific
> >> ideas about proper names?
> > rte_option looks OK.
> I'm happy to change it to rte_option if we have consensus on it :)
OK, thanks
> > The global picture may be better explained I think.
> > Any help with wording and documentation is appreciated, thanks.
>
> Thanks,
> Kevin
>
>
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,
Hi Gaetan,
Thanks for reviewing and providing suggestions.
On 17/10/2018 16:56, Gaëtan Rivet wrote:
>> +
>> +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?
Will add a check to avoid option duplication, good spot. Thanks
>> @@ -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.
I agree that the error message is not helpful. Expanding the return
value to pass the option name or more would be difficult however as we
would need to check for success/fail of the execution of the callback in
rte_option_init(). Since the we don't know what the return type of a
given registered callback is, it is difficult to do the error checking here.
With that in mind, I think it might be best to leave the library's
function do the error checking itself, and changing the return type of
rte_option_init() to void. This way we could get library specific errors
using RTE_LOG, for example. Thoughts?
Best regards,
Kevin
@@ -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
@@ -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;
}
@@ -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
new file mode 100644
@@ -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.
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+typedef int (*rte_param_cb)(void);
+
+/*
+ * Structure containing parameters relating to the function being registered
+ * with EAL.
+ */
+struct rte_param {
+ TAILQ_ENTRY(rte_param) next; /** The next entry in the TAILQ*/
+ char *eal_option; /** The EAL option */
+ rte_param_cb cb; /** Function pointer of the callback */
+
+ /**
+ * 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;
+};
+
+/**
+ * @internal Check if the passed option is valid
+ *
+ * @param option
+ * The option to be parsed
+ *
+ * @return
+ * 0 on success
+ * @return
+ * -1 on fail
+ */
+int
+rte_param_parse(char *option);
+
+/**
+ * @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.
+ */
+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.
+ *
+ * @return
+ * 0 on success
+ * @return
+ * -1 on fail
+ */
+int
+rte_param_init(void);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif
@@ -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',
new file mode 100644
@@ -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);
+}
+
+int
+rte_param_init(void)
+{
+ TAILQ_FOREACH(param, &rte_param_list, next) {
+ if (param->enabled)
+ param->cb();
+ }
+
+ return 0;
+}
@@ -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
@@ -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");
+ return -1;
+ }
+
return fctret;
}
@@ -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;