Re: [PATCH 01/11] telemetry: initial telemetry infrastructure

["Van Haaren, Harry" <harry.van.haaren@intel.com>]

> From: Shreyansh Jain [mailto:shreyansh.jain@nxp.com]
> Sent: Friday, August 24, 2018 2:04 PM
> To: Power, Ciara <ciara.power@intel.com>; Van Haaren, Harry
> <harry.van.haaren@intel.com>; Archbold, Brian <brian.archbold@intel.com>;
> Kenny, Emma <emma.kenny@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.power@intel.com>
> > Signed-off-by: Brian Archbold <brian.archbold@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