[v4,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>
---
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 | 78 +++++++++++++++++++++++++++++++
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, 165 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
> -----Original Message-----
> From: Laatz, Kevin
> Sent: Thursday, October 11, 2018 9:58 AM
> To: dev@dpdk.org
> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>;
> stephen@networkplumber.org; gaetan.rivet@6wind.com; shreyansh.jain@nxp.com;
> thomas@monjalon.net; mattias.ronnblom@ericsson.com; Richardson, Bruce
> <bruce.richardson@intel.com>; Laatz, Kevin <kevin.laatz@intel.com>
> Subject: [PATCH v4 01/13] eal: add param register infrastructure
>
> 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>
Hi,
11/10/2018 18:58, Kevin Laatz:
> 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.
Let's try to have a clear documentation for this new infra.
> +/**
> + * @file
> + *
> + * This API introduces the ability to register callbacks with EAL. When
You should explain when the callback is called, and what is the role of
the callback.
> + * registering a callback, the application also provides a flag as part of the
> + * struct used to register. If the flag is passed to EAL when ruuning a DPDK
What do you call a flag? Are you talking about an option to be parsed?
> + * 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 issue between
> + * EAL and the library.
You need to explain what is the circular dependency issue.
[...]
> +struct rte_param {
Please add a global comment for this struct, explain what it represents.
> + TAILQ_ENTRY(rte_param) next; /** The next entry in the TAILQ*/
> + char *eal_flag; /** The EAL flag */
eal_flag, The EAL flag
Hum...
Please use different words to explain what is a flag.
If it is something to be parsed by getopt, it should be called
an option, not a flag.
> + char *help_text; /** Help text for the callback */
What the help text is used for? When is it printed?
> + rte_param_cb cb; /** Function pointer of the callback */
> + int enabled; /** Enabled flag, should be 0 by default */
What means enabled in this context?
> +};
> +
> +/**
> + * @internal Check if the passed flag is valid
> + *
> + * @param flag
> + * The flag to be parsed
Here too, you need to be more explicit about flag meaning.
> + *
> + * @return
> + * 0 on success
> + * @return
> + * -1 on fail
> + */
> +int
> +rte_param_parse(char *flag);
> +
> +/**
> + * @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
> + * rte_param structure
No, this is not a helpful comment.
> + */
> +void __rte_experimental
> +rte_param_register(struct rte_param *reg_param);
Hi Thomas,
Thanks for reviewing, see replies below.
On 16/10/2018 14:42, Thomas Monjalon wrote:
> Hi,
>
> 11/10/2018 18:58, Kevin Laatz:
>> 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.
> Let's try to have a clear documentation for this new infra.
Are you looking for a doc file here? Or just better Doxygen comments?
>
>> +/**
>> + * @file
>> + *
>> + * This API introduces the ability to register callbacks with EAL. When
> You should explain when the callback is called, and what is the role of
> the callback.
I explained this below with telemetry as an example, maybe it needs to
be extended a little.
>
>> + * registering a callback, the application also provides a flag as part of the
>> + * struct used to register. If the flag is passed to EAL when ruuning a DPDK
> What do you call a flag? Are you talking about an option to be parsed?
Agreed. The wording was unfortunate here. Will change to 'option' to
make it clear it is related to getopt
>
>> + * 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 issue between
>> + * EAL and the library.
> You need to explain what is the circular dependency issue.
I wrote the Doxygen trying to keep it generic. I can make it telemetry
specific if it will be clearer?
>
> [...]
>> +struct rte_param {
> Please add a global comment for this struct, explain what it represents.
Good spot, thanks.
>> + TAILQ_ENTRY(rte_param) next; /** The next entry in the TAILQ*/
>> + char *eal_flag; /** The EAL flag */
> eal_flag, The EAL flag
> Hum...
> Please use different words to explain what is a flag.
> If it is something to be parsed by getopt, it should be called
> an option, not a flag.
>
>> + char *help_text; /** Help text for the callback */
> What the help text is used for? When is it printed?
The help text is currently not being used. It was added speculatively.
>
>> + rte_param_cb cb; /** Function pointer of the callback */
>> + int enabled; /** Enabled flag, should be 0 by default */
> What means enabled in this context?
Enabled means that the option, for example --telemetry, was passed and
that we need to call the
callback at the end of EAL init. If the option was not passed for a
given callback, then the callback
will not be called.
>
>> +};
>> +
>> +/**
>> + * @internal Check if the passed flag is valid
>> + *
>> + * @param flag
>> + * The flag to be parsed
> Here too, you need to be more explicit about flag meaning.
>
>> + *
>> + * @return
>> + * 0 on success
>> + * @return
>> + * -1 on fail
>> + */
>> +int
>> +rte_param_parse(char *flag);
>> +
>> +/**
>> + * @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
>> + * rte_param structure
> No, this is not a helpful comment.
>
>> + */
>> +void __rte_experimental
>> +rte_param_register(struct rte_param *reg_param);
>
>
16/10/2018 16:20, Laatz, Kevin:
> Hi Thomas,
>
> Thanks for reviewing, see replies below.
>
>
> On 16/10/2018 14:42, Thomas Monjalon wrote:
> > Hi,
> >
> > 11/10/2018 18:58, Kevin Laatz:
> >> 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.
> > Let's try to have a clear documentation for this new infra.
> Are you looking for a doc file here? Or just better Doxygen comments?
Just better doxygen comments.
> >> +/**
> >> + * @file
> >> + *
> >> + * This API introduces the ability to register callbacks with EAL. When
> > You should explain when the callback is called, and what is the role of
> > the callback.
> I explained this below with telemetry as an example, maybe it needs to
> be extended a little.
> >
> >> + * registering a callback, the application also provides a flag as part of the
> >> + * struct used to register. If the flag is passed to EAL when ruuning a DPDK
> > What do you call a flag? Are you talking about an option to be parsed?
> Agreed. The wording was unfortunate here. Will change to 'option' to
> make it clear it is related to getopt
OK, so you may need to change the file name, and struct/variable names.
> >> + * 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 issue between
> >> + * EAL and the library.
> > You need to explain what is the circular dependency issue.
> I wrote the Doxygen trying to keep it generic. I can make it telemetry
> specific if it will be clearer?
No need to be specific. You can explain the library uses EAL and
is initialized by EAL too.
> > [...]
> >> +struct rte_param {
> > Please add a global comment for this struct, explain what it represents.
> Good spot, thanks.
> >> + TAILQ_ENTRY(rte_param) next; /** The next entry in the TAILQ*/
> >> + char *eal_flag; /** The EAL flag */
> > eal_flag, The EAL flag
> > Hum...
> > Please use different words to explain what is a flag.
> > If it is something to be parsed by getopt, it should be called
> > an option, not a flag.
> >
> >> + char *help_text; /** Help text for the callback */
> > What the help text is used for? When is it printed?
> The help text is currently not being used. It was added speculatively.
Please do not add things speculatively.
> >> + rte_param_cb cb; /** Function pointer of the callback */
> >> + int enabled; /** Enabled flag, should be 0 by default */
> > What means enabled in this context?
> Enabled means that the option, for example --telemetry, was passed and
> that we need to call the
> callback at the end of EAL init. If the option was not passed for a
> given callback, then the callback
> will not be called.
OK, you need to explain it in the comment.
> >> +};
> >> +
> >> +/**
> >> + * @internal Check if the passed flag is valid
> >> + *
> >> + * @param flag
> >> + * The flag to be parsed
> > Here too, you need to be more explicit about flag meaning.
> >
> >> + *
> >> + * @return
> >> + * 0 on success
> >> + * @return
> >> + * -1 on fail
> >> + */
> >> +int
> >> +rte_param_parse(char *flag);
> >> +
> >> +/**
> >> + * @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
> >> + * rte_param structure
> > No, this is not a helpful comment.
> >
> >> + */
> >> +void __rte_experimental
> >> +rte_param_register(struct rte_param *reg_param);
@@ -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,78 @@
+/* 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 a flag as part of the
+ * struct used to register. If the flag is passed to EAL when ruuning 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 issue between
+ * EAL and the library.
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+typedef int (*rte_param_cb)(void);
+
+struct rte_param {
+ TAILQ_ENTRY(rte_param) next; /** The next entry in the TAILQ*/
+ char *eal_flag; /** The EAL flag */
+ char *help_text; /** Help text for the callback */
+ rte_param_cb cb; /** Function pointer of the callback */
+ int enabled; /** Enabled flag, should be 0 by default */
+};
+
+/**
+ * @internal Check if the passed flag is valid
+ *
+ * @param flag
+ * The flag to be parsed
+ *
+ * @return
+ * 0 on success
+ * @return
+ * -1 on fail
+ */
+int
+rte_param_parse(char *flag);
+
+/**
+ * @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
+ * rte_param structure
+ */
+void __rte_experimental
+rte_param_register(struct rte_param *reg_param);
+
+/**
+ * @internal Iterate through the registered params and init 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 *flag)
+{
+ /* Check if the flag is in the registered inits */
+ TAILQ_FOREACH(param, &rte_param_list, next) {
+ if (strcmp(flag, param->eal_flag) == 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;