From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by dpdk.org (Postfix) with ESMTP id E73241B44F for ; Thu, 4 Oct 2018 16:13:23 +0200 (CEST) Received: by mail-wr1-f66.google.com with SMTP id y16so10116194wrw.3 for ; Thu, 04 Oct 2018 07:13:23 -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=czmNqqd7GTNVfuget17wPK6S1AZN1S01K+EzRMCWTOw=; b=kNUgQ9MKDsM7AZnHAus3RQ35Vg+tmaNS3R0BLa3+i23zSm24R0waca1vSlJETFEzYs 972EdoScreIAZgJQabsPJ+kLcEHsNJm0sAU4j4iy+7qQo+1XSkxJa77Iwi55CLpcqVuj mfCA0WLcBOMiejqUQEZdH6KRbwVg7gtKylX9rzo54+uoO7Kvys0rQay9ZDwmT8DDwIjz KjyhsRkX5Zf+HXmGELLmIhTmS5zDtjL2iUkrvLTZyOCybOqaruJtbgPJ97I96KpXVGjL flt+hl1p8+WVQY/YBlvjKUegAib6JQqiSnvr6Nn1TmijTZbl2YBLJ+wJoXgJKsyucpt0 bNFw== 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=czmNqqd7GTNVfuget17wPK6S1AZN1S01K+EzRMCWTOw=; b=Rsk42ETlVJlhhPFgLe1YShB1c6o/KwDXB9DIUIZW5lGtsMuPkIi+qvRHLHniGV9HFT V1RFzezrJJwEu4GSzhcZH4O5UNI04PoC0TQiZv5YKX8oTMxJTe39l+7tioJSoaPcgd3K 9k7xh1dSlKvGERB0r1Qx1nco9r8djTaJ/8jbDTDaOU6ueOGt+wgDlZok8TwZWo1aN+nX GUaVq8NUypXNneK6mhpYFBh5bXjAW9bK9oKEj5qQYFieSkmSXzzE3z/cj336rZunNnZV 3Baa08ESKVDn6q8tbMpYBN3IxB28Lf2Jg5awMwdsChkgxlOH6FRsfw3y4ouTtDfFtnMe YI5Q== X-Gm-Message-State: ABuFfojo4qXduc7N8cK8uun6+Ikb0UtclmfGr5egu4Z/hbEoatLSObA7 xTR389gTgJ5buA74YVPORXZgMg== X-Google-Smtp-Source: ACcGV61NKLjYCDo9TsoMVQNQgsTWsz9/hG6BE2bhvGi7zSmkDNUXUzvfiCuEPVHAkDqVpLKxyh41yg== X-Received: by 2002:a5d:628c:: with SMTP id k12-v6mr4718342wru.83.1538662403189; Thu, 04 Oct 2018 07:13:23 -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 y184-v6sm4822026wmg.17.2018.10.04.07.13.22 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 04 Oct 2018 07:13:22 -0700 (PDT) Date: Thu, 4 Oct 2018 16:13:04 +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, Ciara Power , Brian Archbold Message-ID: <20181004141304.vzus4xqkf6v7wp3q@bidouze.vm.6wind.com> References: <1535026093-101872-1-git-send-email-ciara.power@intel.com> <20181003173612.67101-1-kevin.laatz@intel.com> <20181003173612.67101-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: <20181003173612.67101-2-kevin.laatz@intel.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v2 01/10] telemetry: initial telemetry 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: Thu, 04 Oct 2018 14:13:24 -0000 Hi, On Wed, Oct 03, 2018 at 06:36:03PM +0100, Kevin Laatz wrote: > From: Ciara Power > > This patch adds the infrastructure and initial code for the telemetry > library. > > The telemetry init is registered with eal_init(). We can then check to see > if --telemetry was passed as an eal flag. If --telemetry was parsed, then > we call telemetry init at the end of eal init. > > Control threads are used to get CPU cycles for telemetry, which are > configured in this patch also. > > Signed-off-by: Ciara Power > Signed-off-by: Brian Archbold > Signed-off-by: Kevin Laatz > --- > config/common_base | 5 ++ > lib/Makefile | 2 + > lib/librte_eal/common/include/rte_eal.h | 19 ++++ > lib/librte_eal/linuxapp/eal/eal.c | 37 +++++++- > lib/librte_eal/rte_eal_version.map | 7 ++ > lib/librte_telemetry/Makefile | 28 ++++++ > lib/librte_telemetry/meson.build | 7 ++ > lib/librte_telemetry/rte_telemetry.c | 117 +++++++++++++++++++++++++ > lib/librte_telemetry/rte_telemetry.h | 36 ++++++++ > lib/librte_telemetry/rte_telemetry_internal.h | 32 +++++++ > lib/librte_telemetry/rte_telemetry_version.map | 6 ++ > lib/meson.build | 2 +- > mk/rte.app.mk | 1 + > 13 files changed, 297 insertions(+), 2 deletions(-) > create mode 100644 lib/librte_telemetry/Makefile > create mode 100644 lib/librte_telemetry/meson.build > create mode 100644 lib/librte_telemetry/rte_telemetry.c > create mode 100644 lib/librte_telemetry/rte_telemetry.h > create mode 100644 lib/librte_telemetry/rte_telemetry_internal.h > create mode 100644 lib/librte_telemetry/rte_telemetry_version.map > > diff --git a/config/common_base b/config/common_base > index 4bcbaf9..682f8bf 100644 > --- a/config/common_base > +++ b/config/common_base > @@ -716,6 +716,11 @@ CONFIG_RTE_LIBRTE_HASH=y > CONFIG_RTE_LIBRTE_HASH_DEBUG=n > > # > +# Compile librte_telemetry > +# > +CONFIG_RTE_LIBRTE_TELEMETRY=y > + > +# > # Compile librte_efd > # > CONFIG_RTE_LIBRTE_EFD=y > diff --git a/lib/Makefile b/lib/Makefile > index afa604e..8cbd035 100644 > --- a/lib/Makefile > +++ b/lib/Makefile > @@ -105,6 +105,8 @@ DEPDIRS-librte_gso := librte_eal librte_mbuf librte_ethdev librte_net > DEPDIRS-librte_gso += librte_mempool > DIRS-$(CONFIG_RTE_LIBRTE_BPF) += librte_bpf > DEPDIRS-librte_bpf := librte_eal librte_mempool librte_mbuf librte_ethdev > +DIRS-$(CONFIG_RTE_LIBRTE_TELEMETRY) += librte_telemetry > +DEPDIRS-librte_telemetry := librte_eal librte_metrics librte_ethdev > > ifeq ($(CONFIG_RTE_EXEC_ENV_LINUXAPP),y) > DIRS-$(CONFIG_RTE_LIBRTE_KNI) += librte_kni > diff --git a/lib/librte_eal/common/include/rte_eal.h b/lib/librte_eal/common/include/rte_eal.h > index e114dcb..5929a34 100644 > --- a/lib/librte_eal/common/include/rte_eal.h > +++ b/lib/librte_eal/common/include/rte_eal.h > @@ -498,6 +498,25 @@ enum rte_iova_mode rte_eal_iova_mode(void); > const char * > rte_eal_mbuf_user_pool_ops(void); > thanks for introducing this, I think this can be useful. However, this deserves its own commit. > +typedef int (*rte_lib_init_fn)(void); > + > +typedef struct rte_lib_init_params { This could be used to add arbitrary params, not only library init functions. This structure should be named "struct rte_param" instead (singular). > + TAILQ_ENTRY(rte_lib_init_params) next; > + char eal_flag[32]; > + char help_text[80]; You don't need to enforce length limit here. These structures will be allocated statically, those two arrays could simply be pointers to static strings. > + rte_lib_init_fn lib_init; Considering the more generic "rte_param" name, "rte_param_cb cb;" might be more suited. > + int enabled; > +} rte_lib_init_params; > + > +/** > + * @internal Register a libraries init function > + * This API is not EAL internal, it is public API. > + * @param reg_init > + * Structure containing the eal flag, the lib help string and the init > + * function pointer for the library. > + */ > +void rte_lib_init_register(struct rte_lib_init_params *reg_init); rte_param_register() > + > #ifdef __cplusplus > } > #endif > diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c > index e59ac65..b9113c7 100644 > --- a/lib/librte_eal/linuxapp/eal/eal.c > +++ b/lib/librte_eal/linuxapp/eal/eal.c > @@ -97,6 +97,13 @@ static char runtime_dir[PATH_MAX]; > > static const char *default_runtime_dir = "/var/run"; > > +TAILQ_HEAD(rte_lib_init_list, rte_lib_init_params); > + > +struct rte_lib_init_list rte_lib_init_list = > + TAILQ_HEAD_INITIALIZER(rte_lib_init_list); > + > +rte_lib_init_params *lib_init_params; > + You should not have these only in linuxapp env. You need to add a compilation unit in lib/librte_eal/common/ which will contain the list head and register implementation, that will be linked from both linuxapp and bsdapp targets. > int > eal_create_runtime_dir(void) > { > @@ -570,7 +577,7 @@ eal_log_level_parse(int argc, char **argv) > static int > eal_parse_args(int argc, char **argv) > { > - int opt, ret; > + int opt, ret, valid_opt; > char **argvopt; > int option_index; > char *prgname = argv[0]; > @@ -580,12 +587,27 @@ 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 */ This comment should be at least rewritten, or removed. > if (opt == '?') { > + valid_opt = 0; > + /* Check if the flag is in the registered lib inits */ > + TAILQ_FOREACH(lib_init_params, &rte_lib_init_list, next) { > + if (strcmp(argv[optind-1], > + lib_init_params->eal_flag) == 0) { > + lib_init_params->enabled = 1; > + valid_opt = 1; > + opterr = 0; > + } > + } > + > + if (valid_opt) > + continue; > + A single helper function should be implemented, "rte_param_parse()", that would return different codes for (error | opt not found | opt found), and would be called here and in bsdapp. > eal_usage(prgname); > ret = -1; > goto out; > @@ -786,6 +808,13 @@ static void rte_eal_init_alert(const char *msg) > RTE_LOG(ERR, EAL, "%s\n", msg); > } > > +void > +rte_lib_init_register(struct rte_lib_init_params *reg_init) > +{ > + TAILQ_INSERT_HEAD(&rte_lib_init_list, reg_init, next); > +} > + > + This should be in the common rte_param.c compilation unit. > /* Launch threads, called at application init(). */ > int > rte_eal_init(int argc, char **argv) > @@ -1051,6 +1080,12 @@ rte_eal_init(int argc, char **argv) > > rte_eal_mcfg_complete(); > > + /* Call the init function for each registered and enabled lib */ > + TAILQ_FOREACH(lib_init_params, &rte_lib_init_list, next) { > + if (lib_init_params->enabled) > + lib_init_params->lib_init(); > + } > + A helper "rte_param_init()" should be written instead, within which you would call the parameter callback, but also check the return value and stop init() on error. > return fctret; > } > > diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map > index 344a43d..914d0fa 100644 > --- a/lib/librte_eal/rte_eal_version.map > +++ b/lib/librte_eal/rte_eal_version.map > @@ -262,6 +262,13 @@ DPDK_18.08 { > > } DPDK_18.05; > > +DPDK_18.11 { > + global: > + > + rte_lib_init_register; > + > +} DPDK_18.08; > + > EXPERIMENTAL { > global: > > diff --git a/lib/librte_telemetry/Makefile b/lib/librte_telemetry/Makefile > new file mode 100644 > index 0000000..0d61361 > --- /dev/null > +++ b/lib/librte_telemetry/Makefile > @@ -0,0 +1,28 @@ > +# SPDX-License-Identifier: BSD-3-Clause > +# Copyright(c) 2018 Intel Corporation > + > +include $(RTE_SDK)/mk/rte.vars.mk > + > +# library name > +LIB = librte_telemetry.a > + > +CFLAGS += -O3 > +CFLAGS += -I$(SRCDIR) > +CFLAGS += -DALLOW_EXPERIMENTAL_API > + > +LDLIBS += -lrte_eal -lrte_ethdev > +LDLIBS += -lrte_metrics > +LDLIBS += -lpthread > +LDLIBS += -ljansson > + > +EXPORT_MAP := rte_telemetry_version.map > + > +LIBABIVER := 1 > + > +# library source files > +SRCS-$(CONFIG_RTE_LIBRTE_TELEMETRY) := rte_telemetry.c > + > +# export include files > +SYMLINK-$(CONFIG_RTE_LIBRTE_TELEMETRY)-include := rte_telemetry.h > + > +include $(RTE_SDK)/mk/rte.lib.mk > diff --git a/lib/librte_telemetry/meson.build b/lib/librte_telemetry/meson.build > new file mode 100644 > index 0000000..7716076 > --- /dev/null > +++ b/lib/librte_telemetry/meson.build > @@ -0,0 +1,7 @@ > +# SPDX-License-Identifier: BSD-3-Clause > +# Copyright(c) 2018 Intel Corporation > + > +sources = files('rte_telemetry.c') > +headers = files('rte_telemetry.h', 'rte_telemetry_internal.h') > +deps += ['metrics', 'ethdev'] > +cflags += '-DALLOW_EXPERIMENTAL_API' > diff --git a/lib/librte_telemetry/rte_telemetry.c b/lib/librte_telemetry/rte_telemetry.c > new file mode 100644 > index 0000000..d9ffec2 > --- /dev/null > +++ b/lib/librte_telemetry/rte_telemetry.c > @@ -0,0 +1,117 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2018 Intel Corporation > + */ > + > +#include > +#include > + > +#include > +#include > +#include > + > +#include "rte_telemetry.h" > +#include "rte_telemetry_internal.h" > + > +#define SLEEP_TIME 10 > + > +static telemetry_impl *static_telemetry; > + > +static int32_t > +rte_telemetry_run(void *userdata) > +{ > + struct telemetry_impl *telemetry = userdata; > + > + if (!telemetry) { > + TELEMETRY_LOG_WARN("TELEMETRY could not be initialised"); > + return -1; > + } You have already dereferenced telemetry->thread_status in the caller, this check will never trigger. __rte_unused on userdata might be used while waiting for the actual implementation to happen. > + > + return 0; > +} > + > +static void > +*rte_telemetry_run_thread_func(void *userdata) > +{ > + int ret; > + struct telemetry_impl *telemetry = userdata; > + > + if (!telemetry) { > + TELEMETRY_LOG_ERR("%s passed a NULL instance", __func__); > + pthread_exit(0); > + } You already checked the calloc return before spawning the thread, this will never trigger. > + > + while (telemetry->thread_status) { > + rte_telemetry_run(telemetry); > + ret = usleep(SLEEP_TIME); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Calling thread could not be put to sleep"); > + } > + pthread_exit(0); > +} > + > +int32_t > +rte_telemetry_init() > +{ > + int ret; > + pthread_attr_t attr; > + const char *telemetry_ctrl_thread = "telemetry"; The thread name is never re-used and its actual name is shorter than the variable used to reference it, you might as well use the value itself in the function call. > + > + if (static_telemetry) { > + TELEMETRY_LOG_WARN("TELEMETRY structure already initialised"); > + return -EALREADY; > + } > + > + static_telemetry = calloc(1, sizeof(struct telemetry_impl)); > + if (!static_telemetry) { > + TELEMETRY_LOG_ERR("Memory could not be allocated"); > + return -ENOMEM; > + } > + > + static_telemetry->socket_id = rte_socket_id(); > + rte_metrics_init(static_telemetry->socket_id); > + pthread_attr_init(&attr); > + ret = rte_ctrl_thread_create(&static_telemetry->thread_id, > + telemetry_ctrl_thread, &attr, rte_telemetry_run_thread_func, > + (void *)static_telemetry); > + static_telemetry->thread_status = 1; > + > + if (ret < 0) { > + ret = rte_telemetry_cleanup(); > + if (ret < 0) > + TELEMETRY_LOG_ERR("TELEMETRY cleanup failed"); > + return -EPERM; > + } > + > + return 0; > +} > + > +int32_t > +rte_telemetry_cleanup(void) > +{ > + struct telemetry_impl *telemetry = static_telemetry; > + telemetry->thread_status = 0; > + pthread_join(telemetry->thread_id, NULL); > + free(telemetry); > + static_telemetry = NULL; > + return 0; > +} > + > +int telemetry_log_level; > +RTE_INIT(rte_telemetry_register); > + > +static struct rte_lib_init_params lib_init_params = { > + .eal_flag = "--telemetry", > + .help_text = "Telemetry lib", > + .lib_init = &rte_telemetry_init, > + .enabled = 0 > +}; > + > +static void > +rte_telemetry_register(void) > +{ > + telemetry_log_level = rte_log_register("lib.telemetry"); > + if (telemetry_log_level >= 0) > + rte_log_set_level(telemetry_log_level, RTE_LOG_ERR); > + > + rte_lib_init_register(&lib_init_params); > +} > diff --git a/lib/librte_telemetry/rte_telemetry.h b/lib/librte_telemetry/rte_telemetry.h > new file mode 100644 > index 0000000..f7ecb7b > --- /dev/null > +++ b/lib/librte_telemetry/rte_telemetry.h > @@ -0,0 +1,36 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2018 Intel Corporation > + */ > + > +#include > + > +#ifndef _RTE_TELEMETRY_H_ > +#define _RTE_TELEMETRY_H_ > + > +/** > + * Get the telemetry_impl structure device pointer initialised. API description should not reference implementation details. A pointer being initialized is the kind of information the function is trying to abstract from the user, it's counter-productive to force it back in the documentation. A note telling that rte_telemetry, using rte_metrics, would initialize the latter might be interesting. > + * > + * @return > + * 0 on successful initialisation. > + * @return > + * -ENOMEM on memory allocation error > + * @return > + * -EPERM on unknown error failure > + * @return > + * -EALREADY if Telemetry is already initialised. > + */ > +int32_t I think there have already been a remark on it, but why int32_t? Errno is evaluated to an int type. > +rte_telemetry_init(void); > + > +/** > + * Clean up and free memory. > + * > + * @return > + * 0 on success > + * @return > + * -EPERM on failure > + */ > +int32_t > +rte_telemetry_cleanup(void); > + > +#endif > diff --git a/lib/librte_telemetry/rte_telemetry_internal.h b/lib/librte_telemetry/rte_telemetry_internal.h > new file mode 100644 > index 0000000..4e810a8 > --- /dev/null > +++ b/lib/librte_telemetry/rte_telemetry_internal.h > @@ -0,0 +1,32 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2018 Intel Corporation > + */ > + > +#include > + > +#ifndef _RTE_TELEMETRY_INTERNAL_H_ > +#define _RTE_TELEMETRY_INTERNAL_H_ > + > +/* Logging Macros */ > +extern int telemetry_log_level; > + > +#define TELEMETRY_LOG(level, fmt, args...) \ > + rte_log(RTE_LOG_ ##level, telemetry_log_level, "%s(): "fmt "\n", \ > + __func__, ##args) > + > +#define TELEMETRY_LOG_ERR(fmt, args...) \ > + TELEMETRY_LOG(ERR, fmt, ## args) > + > +#define TELEMETRY_LOG_WARN(fmt, args...) \ > + TELEMETRY_LOG(WARNING, fmt, ## args) > + > +#define TELEMETRY_LOG_INFO(fmt, args...) \ > + TELEMETRY_LOG(INFO, fmt, ## args) > + > +typedef struct telemetry_impl { > + pthread_t thread_id; > + int thread_status; volatile to disable possible optimization on the while loop? > + uint32_t socket_id; > +} telemetry_impl; > + > +#endif > diff --git a/lib/librte_telemetry/rte_telemetry_version.map b/lib/librte_telemetry/rte_telemetry_version.map > new file mode 100644 > index 0000000..992d227 > --- /dev/null > +++ b/lib/librte_telemetry/rte_telemetry_version.map > @@ -0,0 +1,6 @@ > +DPDK_18.11 { > + global: > + > + rte_telemetry_init; > + local: *; > +}; > diff --git a/lib/meson.build b/lib/meson.build > index eb91f10..fc84b2f 100644 > --- a/lib/meson.build > +++ b/lib/meson.build > @@ -24,7 +24,7 @@ libraries = [ 'compat', # just a header, used for versioning > # add pkt framework libs which use other libs from above > 'port', 'table', 'pipeline', > # flow_classify lib depends on pkt framework table lib > - 'flow_classify', 'bpf'] > + 'flow_classify', 'bpf', 'telemetry'] > > default_cflags = machine_args > if cc.has_argument('-Wno-format-truncation') > diff --git a/mk/rte.app.mk b/mk/rte.app.mk > index de33883..1223a85 100644 > --- a/mk/rte.app.mk > +++ b/mk/rte.app.mk > @@ -80,6 +80,7 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_SECURITY) += -lrte_security > _LDLIBS-$(CONFIG_RTE_LIBRTE_COMPRESSDEV) += -lrte_compressdev > _LDLIBS-$(CONFIG_RTE_LIBRTE_EVENTDEV) += -lrte_eventdev > _LDLIBS-$(CONFIG_RTE_LIBRTE_RAWDEV) += -lrte_rawdev > +_LDLIBS-$(CONFIG_RTE_LIBRTE_TELEMETRY) += -lrte_metrics -lrte_telemetry > _LDLIBS-$(CONFIG_RTE_LIBRTE_TIMER) += -lrte_timer > _LDLIBS-$(CONFIG_RTE_LIBRTE_MEMPOOL) += -lrte_mempool > _LDLIBS-$(CONFIG_RTE_DRIVER_MEMPOOL_RING) += -lrte_mempool_ring > -- > 2.9.5 > -- Gaëtan Rivet 6WIND