> From: Shreyansh Jain [mailto:shreyansh.j...@nxp.com]
> Sent: Friday, August 24, 2018 2:04 PM
> To: Power, Ciara <ciara.po...@intel.com>; Van Haaren, Harry
> <harry.van.haa...@intel.com>; Archbold, Brian <brian.archb...@intel.com>;
> Kenny, Emma <emma.ke...@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 01/11] telemetry: initial telemetry
> infrastructure
>
> 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 <ciara.po...@intel.com>
> > Signed-off-by: Brian Archbold <brian.archb...@intel.com>
> > ---
>
> [...]
>
> /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.
Yes indeed.
> > + 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 <unistd.h>
> > +
> > +#include <rte_eal.h>
> > +#include <rte_ethdev.h>
> > +#include <rte_metrics.h>
> > +
> > +#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?
Yes, as Stephen noted too. Will fix!
>
> > + 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?
Will look into this.
> > + }
> > + 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.
Thanks for the input, will be kept in mind.
> > +}
> > +
> > +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.
Correct.
Thanks for review, hopefully see you at Userspace where
there will be a lightning talk about this patchset.
Cheers, -Harry
