From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH 01/11] telemetry: initial telemetry infrastructure Date: Fri, 24 Aug 2018 18:33:43 +0530 Message-ID: References: <1535026093-101872-1-git-send-email-ciara.power@intel.com> <1535026093-101872-2-git-send-email-ciara.power@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org To: Ciara Power , harry.van.haaren@intel.com, brian.archbold@intel.com, emma.kenny@intel.com Return-path: Received: from EUR01-DB5-obe.outbound.protection.outlook.com (mail-db5eur01on0082.outbound.protection.outlook.com [104.47.2.82]) by dpdk.org (Postfix) with ESMTP id B88302C60 for ; Fri, 24 Aug 2018 15:04:11 +0200 (CEST) In-Reply-To: <1535026093-101872-2-git-send-email-ciara.power@intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Thursday 23 August 2018 05:38 PM, Ciara Power wrote: > This patch adds the infrastructure and initial code for the > telemetry library. > > A virtual device is used for telemetry, which is included in > this patch. To use telemetry, a command-line argument must be > used - "--vdev=telemetry". > > 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 > --- [...] /rte_pmd_telemetry_version.map > new file mode 100644 > index 0000000..a73e0f2 > --- /dev/null > +++ b/drivers/telemetry/telemetry/rte_pmd_telemetry_version.map > @@ -0,0 +1,9 @@ > +DPDK_18.05 { ^^^^^ I think you want 18.11 here. > + global: > + > + telemetry_probe; > + telemetry_remove; > + > + local: *; > + > +}; [...] > 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_telemetry/Makefile b/lib/librte_telemetry/Makefile > new file mode 100644 > index 0000000..bda3788 > --- /dev/null > +++ b/lib/librte_telemetry/Makefile > @@ -0,0 +1,26 @@ > +# 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 > + > +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..8d7b0e3 > --- /dev/null > +++ b/lib/librte_telemetry/rte_telemetry.c > @@ -0,0 +1,108 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2018 Intel Corporation > + */ > + > +#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 = (struct telemetry_impl *)userdata; > + if (!telemetry) { > + TELEMETRY_LOG_WARN("Warning - TELEMETRY could not be " > + "initialised\n"); Your 'TELEMETRY_LOG_WARNING' already includes a '\n' in its definition. This would add another one. Can you re-check? > + return -1; > + } > + > + return 0; > +} > + > +static void > +*rte_telemetry_run_thread_func(void *userdata) > +{ > + int ret; > + struct telemetry_impl *telemetry = (struct telemetry_impl *)userdata; > + > + if (!telemetry) { > + TELEMETRY_LOG_ERR("Error - %s passed a NULL instance\n", > + __func__); I might be picky - but this is an internal function spawned using rte_ctrl_thread_create which already has a check whether the argument (static_telemetry) is NULL or not. So, this is like duplicating that work. > + pthread_exit(0); > + } > + > + while (telemetry->thread_status) { > + rte_telemetry_run(telemetry); > + ret = usleep(SLEEP_TIME); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Error - Calling thread could not be" > + " put to sleep\n"); If the calling thread couldn't be put to sleep, you would continue looping without sleeping? Wouldn't that simply hog your CPU? Or, is that expected design? > + } > + pthread_exit(0); > +} > + > +int32_t > +rte_telemetry_init(uint32_t socket_id) > +{ > + int ret; > + pthread_attr_t attr; > + const char *telemetry_ctrl_thread = "telemetry"; > + > + if (static_telemetry) { > + TELEMETRY_LOG_WARN("Warning - TELEMETRY structure already " > + "initialised\n"); > + return -EALREADY; > + } > + > + static_telemetry = calloc(1, sizeof(struct telemetry_impl)); > + if (!static_telemetry) { > + TELEMETRY_LOG_ERR("Error - Memory could not be allocated\n"); > + return -ENOMEM; > + } > + > + static_telemetry->socket_id = 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("Error - TELEMETRY cleanup failed\n"); > + 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; Maybe if you could use be a little more liberal with new lines, it would be slightly easier to read. But again, that is my personal opinion. > +} > + > +int telemetry_log_level; > +RTE_INIT(rte_telemetry_log_init); > + > +static void > +rte_telemetry_log_init(void) > +{ > + telemetry_log_level = rte_log_register("lib.telemetry"); > + if (telemetry_log_level >= 0) > + rte_log_set_level(telemetry_log_level, RTE_LOG_ERR); > +} [...] > +#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..efd437d > --- /dev/null > +++ b/lib/librte_telemetry/rte_telemetry_version.map > @@ -0,0 +1,6 @@ > +DPDK_18.05 { ^^^^^^ I think you want it to be 18.11 here. > + global: > + > + rte_telemetry_init; > + local: *; > +}; [...]