Re: [dpdk-dev] [PATCH 01/11] telemetry: initial telemetry infrastructure

[Shreyansh Jain <shreyansh.jain@nxp.com>]

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.

> +	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?

> +		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: *;
> +};

[...]