From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) by dpdk.org (Postfix) with ESMTP id 977FD1B115 for ; Wed, 17 Oct 2018 17:56:22 +0200 (CEST) Received: by mail-wm1-f65.google.com with SMTP id 193-v6so2718893wme.3 for ; Wed, 17 Oct 2018 08:56:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=BjaMIdJoS32SiTKCfwVvJLgAa71ZJvGi2hP63cDNafE=; b=rsbyrxIkjgAfdd80RySxPcwOZTBInIoQh1elac080rLPC4ARgiAcMrE5hQhNmRmqI+ h/j1Tbj9H7AurCV1ywhMRMAqOHQP4ySGb+2aA63YswPM0lEOCorjTwvTRIjRUHgXaiaf qgBrwXolUNCjxdqXLZ3P7YXwFCRtu80y2SECQoPboX54rA83992Os39RdXyV70dtaOtY oiXombys8vWKcNm1jQJJHMwzaIgnN4raRhOipFgPyqCLpJtI2C9xm21JgN5106/BXgJy JIA5V6Yo6GdvXplYPv4syG/XFwIzDtI9xT0P5qYsI53kqEnCGIUswO5cfLpp2h7UYubr G32A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=BjaMIdJoS32SiTKCfwVvJLgAa71ZJvGi2hP63cDNafE=; b=tZHMWzz+zIdK2YDj+Nt8/lKh0HdCk0X4ZTf1znov6G0oTlx6Xk/LOXmpi62wt4uQXF uXnDEWezFALUg9iKTSucfSUChJ8aLmKG1pKmu5Qk99tatweF66n2FWpW2/ttrpHjA2QY Jwl1PfRTjalA1iEMON+WOCPHlhu+3DPGH4fO6a3EGYCcJ/AJlIAYQEtPnxfpKgXjr9mG aPoJAHS3rjSQfKQLsUC95BafsgyemhaLieZ6CLgcienBYMEBwNQ19xCl69m0i3mLV79B Df+ILv/59IuG3fM1TaQrNVwyYfBcSBETOQSDZOXryCtcT0RrEVyHXvbwxI72QKjyVqGQ O3sg== X-Gm-Message-State: ABuFfohrIeKhysR42E++M68vkCBl1NaZS2KSsonzQE7V1pcNqlOOXK/l eO4/2ugTYqSUxoX5h+bEsKRyRA== X-Google-Smtp-Source: ACcGV61HAdwAq651hOiMh0OygTcyJW8GIxFYBuglD/VlfO0f331e5v7/rjVUOzrpnX8mrDz/RNUr7Q== X-Received: by 2002:a1c:c683:: with SMTP id w125-v6mr3410069wmf.117.1539791781599; Wed, 17 Oct 2018 08:56:21 -0700 (PDT) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id y64-v6sm1382236wmy.35.2018.10.17.08.56.20 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 17 Oct 2018 08:56:20 -0700 (PDT) Date: Wed, 17 Oct 2018 17:56:02 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Kevin Laatz Cc: dev@dpdk.org, harry.van.haaren@intel.com, stephen@networkplumber.org, shreyansh.jain@nxp.com, thomas@monjalon.net, mattias.ronnblom@ericsson.com, bruce.richardson@intel.com Message-ID: <20181017155602.kz2kpxx76cwutnbg@bidouze.vm.6wind.com> References: <20181011165837.81030-1-kevin.laatz@intel.com> <20181016155802.2067-1-kevin.laatz@intel.com> <20181016155802.2067-2-kevin.laatz@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20181016155802.2067-2-kevin.laatz@intel.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v5 01/13] eal: add param register infrastructure X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Oct 2018 15:56:22 -0000 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 > Acked-by: Harry van Haaren > --- > 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 > #include > #include > +#include > #include > #include > > @@ -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 > +#include > + > +#include > +#include > + > +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 > #include > #include > +#include > > #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