[01/11] telemetry: initial telemetry infrastructure
Checks
Commit Message
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>
---
config/common_base | 5 +
drivers/Makefile | 3 +
drivers/meson.build | 3 +-
drivers/telemetry/Makefile | 8 ++
drivers/telemetry/meson.build | 6 ++
drivers/telemetry/telemetry/Makefile | 26 +++++
drivers/telemetry/telemetry/meson.build | 5 +
.../telemetry/rte_pmd_telemetry_version.map | 9 ++
drivers/telemetry/telemetry/telemetry_driver.c | 40 ++++++++
lib/Makefile | 2 +
lib/librte_telemetry/Makefile | 26 +++++
lib/librte_telemetry/meson.build | 7 ++
lib/librte_telemetry/rte_telemetry.c | 108 +++++++++++++++++++++
lib/librte_telemetry/rte_telemetry.h | 40 ++++++++
lib/librte_telemetry/rte_telemetry_internal.h | 32 ++++++
lib/librte_telemetry/rte_telemetry_version.map | 6 ++
lib/meson.build | 2 +-
mk/rte.app.mk | 2 +
18 files changed, 328 insertions(+), 2 deletions(-)
create mode 100644 drivers/telemetry/Makefile
create mode 100644 drivers/telemetry/meson.build
create mode 100644 drivers/telemetry/telemetry/Makefile
create mode 100644 drivers/telemetry/telemetry/meson.build
create mode 100644 drivers/telemetry/telemetry/rte_pmd_telemetry_version.map
create mode 100644 drivers/telemetry/telemetry/telemetry_driver.c
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
Comments
On Thu, 23 Aug 2018 13:08:03 +0100
Ciara Power <ciara.power@intel.com> wrote:
> +
> +static int
> +telemetry_probe(struct rte_vdev_device *vdev)
> +{
> + int ret;
> +
> + RTE_SET_USED(vdev);
> + ret = rte_telemetry_init(rte_socket_id());
> + if (ret < 0) {
> + printf("Error - Telemetry initialisation failed\n");
> + return -1;
> + }
> + return 0;
> +}
> +
> +static int
> +telemetry_remove(struct rte_vdev_device *vdev)
> +{
> + const char *name;
> + name = rte_vdev_device_name(vdev);
> + printf("Uninitialising the device: %s\n", name);
Please use DPDK (rte) logging for all messages.
On Thu, 23 Aug 2018 13:08:03 +0100
Ciara Power <ciara.power@intel.com> wrote:
> +static int32_t
> +rte_telemetry_run(void *userdata)
> +{
> + struct telemetry_impl *telemetry = (struct telemetry_impl *)userdata;
Casting a void * pointer is unnecessary in C.
Assignment has an implicit cast.
On Thu, 23 Aug 2018 13:08:03 +0100
Ciara Power <ciara.power@intel.com> wrote:
> + TELEMETRY_LOG_ERR("Error - Calling thread could not be"
> + " put to sleep\n");
Don't break strings across lines. It makes it harder for developers who want
to use tools to look for log message in source. Checkpatch and friends allow for long
line exception for string literals.
On Thu, 23 Aug 2018 13:08:03 +0100
Ciara Power <ciara.power@intel.com> wrote:
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2018 Intel Corporation
> + */
> +
> +#include <stdint.h>
> +
> +#ifndef _RTE_TELEMETRY_H_
> +#define _RTE_TELEMETRY_H_
> +
> +/**
> + * Get the telemetry_impl structure device pointer initialised.
> + *
> + * @param socket_id
> + * Unsigned integer representing the socket id to be used
> + * for the telemetry structure.
> + *
> + * @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
> +rte_telemetry_init(uint32_t socket_id);
> +
> +/**
> + * Clean up and free memory.
> + *
> + * @return
> + * 0 on success
> + * @return
> + * -EPERM on failure
> + */
> +int32_t
> +rte_telemetry_cleanup(void);
> +
Can this be done with RTE_INIT (i.e automatic constructor).
On Thu, 23 Aug 2018 13:08:03 +0100
Ciara Power <ciara.power@intel.com> wrote:
> +/* 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;
> + uint32_t socket_id;
> +} telemetry_impl;
> +
Your logging macros follow the standard DPDK style. Including automatically
adding a new line. But as I look at the code, many of the TELEMETRY_LOG calls
have a newline in the format. Therefore your log messages will be double spaced.
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: *;
> +};
[...]
Hi Ciara,
On Thu, Aug 23, 2018 at 01:08:03PM +0100, 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".
>
Why use a virtual device?
It seems that you are only using the device semantic as a way to carry
around a tag telling the DPDK framework to init your library once it has
finished its initialization.
I guess you wanted to avoid having to add the call to rte_telemetry_init
to all applications. In the absence of a proper EAL option framework,
you can workaround by adding a --telemetry EAL parameter, setting a flag
on, and checking this flag from librte_telemetry, within a routine
declared with RTE_INIT_PRIO.
I only see afterward the selftest being triggered via kvargs. I haven't
yet looked at the testing done, but if it is only unit test, the "test"
app would be better suited. If it is integration testing to verify the
behavior of the library with other PMDs, you probably need specific
context, thus selftest being insufficient on its own and useless for
other users.
> 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>
Regards,
> 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
Hi Gaetan,
> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Tuesday, August 28, 2018 12:47 PM
> To: Power, Ciara <ciara.power@intel.com>
> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Archbold, Brian
> <brian.archbold@intel.com>; Kenny, Emma <emma.kenny@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 01/11] telemetry: initial telemetry
> infrastructure
>
> Hi Ciara,
>
> On Thu, Aug 23, 2018 at 01:08:03PM +0100, 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".
> >
>
> Why use a virtual device?
>
> It seems that you are only using the device semantic as a way to carry
> around a tag telling the DPDK framework to init your library once it has
> finished its initialization.
>
> I guess you wanted to avoid having to add the call to rte_telemetry_init
> to all applications. In the absence of a proper EAL option framework,
> you can workaround by adding a --telemetry EAL parameter, setting a flag
> on, and checking this flag from librte_telemetry, within a routine
> declared with RTE_INIT_PRIO.
I suppose that an EAL --flag could work too, it would mean that EAL would
depend on this library. The --vdev trick keeps the library standalone.
I don't have a strong opinion either way. :)
> I only see afterward the selftest being triggered via kvargs. I haven't
> yet looked at the testing done, but if it is only unit test, the "test"
> app would be better suited. If it is integration testing to verify the
> behavior of the library with other PMDs, you probably need specific
> context, thus selftest being insufficient on its own and useless for
> other users.
Correct, self tests are triggered by kvargs. This same model is used
in eg: eventdev PMDs to run selftests, where the tests are pretty complex
and specific to the device under test.
Again, I don't have a strong opinion but I don't see any issue with it
being included in the vdev / telemetry library. We could write a shim
test that the "official" test binary runs the telemetry tests if that is
your concern?
> > 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>
>
> Regards,
> --
> Gaëtan Rivet
> 6WIND
Thanks for review, and there's a lightning talk at Userspace so please
do provide input there too :) -Harry
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Friday, August 24, 2018 12:22 AM
> To: Power, Ciara <ciara.power@intel.com>
> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Archbold, Brian
> <brian.archbold@intel.com>; Kenny, Emma <emma.kenny@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 01/11] telemetry: initial telemetry
> infrastructure
>
> On Thu, 23 Aug 2018 13:08:03 +0100
> Ciara Power <ciara.power@intel.com> wrote:
>
> > +/* 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;
> > + uint32_t socket_id;
> > +} telemetry_impl;
> > +
>
> Your logging macros follow the standard DPDK style. Including automatically
> adding a new line. But as I look at the code, many of the TELEMETRY_LOG
> calls
> have a newline in the format. Therefore your log messages will be double
> spaced.
Correct, will get that fixed.
Thanks for reviewing! Looking forward to Userspace, curious to hear of
your use-cases for Telemetry lib, assuming you have some in mind.
On Tue, Aug 28, 2018 at 04:54:33PM +0000, Van Haaren, Harry wrote:
> Hi Gaetan,
>
> > -----Original Message-----
> > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > Sent: Tuesday, August 28, 2018 12:47 PM
> > To: Power, Ciara <ciara.power@intel.com>
> > Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Archbold, Brian
> > <brian.archbold@intel.com>; Kenny, Emma <emma.kenny@intel.com>; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 01/11] telemetry: initial telemetry
> > infrastructure
> >
> > Hi Ciara,
> >
> > On Thu, Aug 23, 2018 at 01:08:03PM +0100, 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".
> > >
> >
> > Why use a virtual device?
> >
> > It seems that you are only using the device semantic as a way to carry
> > around a tag telling the DPDK framework to init your library once it has
> > finished its initialization.
> >
> > I guess you wanted to avoid having to add the call to rte_telemetry_init
> > to all applications. In the absence of a proper EAL option framework,
> > you can workaround by adding a --telemetry EAL parameter, setting a flag
> > on, and checking this flag from librte_telemetry, within a routine
> > declared with RTE_INIT_PRIO.
>
> I suppose that an EAL --flag could work too, it would mean that EAL would
> depend on this library. The --vdev trick keeps the library standalone.
>
> I don't have a strong opinion either way. :)
>
This was done already for specific EAL configuration items such as
vfio intr_mode or PCI uio configuration.
Of course this is ugly, but the --telemetry parameter can exist without
compiling the lib. You can add a warning if the TELEMETRY Mconfig
item is not set to mitigate. The main issue is that you need to add
getters because you cannot declare an external *struct internal_config*
reference.
I agree this is awkward, and this is exactly the reason we need a
way for libraries to register options in the EAL, but this is not
yet done.
The virtual device solution however is a crutch used to emulate this
absent framework. This will complicate developping the proper solution
and its adoption once done. I would not be clear then to the dev that they
can translate the telemetry shim parameter to the new framework, without
having to rework the whole infrastructure of the lib (and this is without
talking about reworking the build system to remove the telemetry driver).
Even having to add a new driver subsection only for telemetry is awkward.
So we might certainly wait for second or third opinions, but I am firmly
convinced it would be easier in order to maintain the project (both from EAL
and systems standpoint and library standpoint) without the vdev trick.
>
> > I only see afterward the selftest being triggered via kvargs. I haven't
> > yet looked at the testing done, but if it is only unit test, the "test"
> > app would be better suited. If it is integration testing to verify the
> > behavior of the library with other PMDs, you probably need specific
> > context, thus selftest being insufficient on its own and useless for
> > other users.
>
> Correct, self tests are triggered by kvargs. This same model is used
> in eg: eventdev PMDs to run selftests, where the tests are pretty complex
> and specific to the device under test.
>
> Again, I don't have a strong opinion but I don't see any issue with it
> being included in the vdev / telemetry library. We could write a shim
> test that the "official" test binary runs the telemetry tests if that is
> your concern?
>
>
Okay, I have no strong opinion about this (actually I prefer having the
test code close to the code-under-test), but eventdev can spawn device
objects to drive the test and provide configuration.
It would be more complicated using the same logic with a pure library,
without the vdev.
> > > 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>
> >
> > Regards,
> > --
> > Gaëtan Rivet
> > 6WIND
>
> Thanks for review, and there's a lightning talk at Userspace so please
> do provide input there too :) -Harry
@@ -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
@@ -21,5 +21,8 @@ DIRS-$(CONFIG_RTE_LIBRTE_EVENTDEV) += event
DEPDIRS-event := common bus mempool net
DIRS-$(CONFIG_RTE_LIBRTE_RAWDEV) += raw
DEPDIRS-raw := common bus mempool net event
+DIRS-$(CONFIG_RTE_LIBRTE_TELEMETRY) += telemetry
+DEPDIRS-telemetry := common bus mempool
+
include $(RTE_SDK)/mk/rte.subdir.mk
@@ -9,7 +9,8 @@ driver_classes = ['common',
'crypto', # depends on common, bus and mempool (net in future).
'compress', # depends on common, bus, mempool.
'event', # depends on common, bus, mempool and net.
- 'raw'] # depends on common, bus, mempool, net and event.
+ 'raw', # depends on common, bus, mempool, net and event.
+ 'telemetry'] # depends on common, bus, and mempool.
default_cflags = machine_args
if cc.has_argument('-Wno-format-truncation')
new file mode 100644
@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2018 Intel Corporation
+
+include $(RTE_SDK)/mk/rte.vars.mk
+
+DIRS-$(CONFIG_RTE_LIBRTE_TELEMETRY) += telemetry
+
+include $(RTE_SDK)/mk/rte.subdir.mk
new file mode 100644
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2018 Intel Corporation
+
+drivers = ['telemetry']
+config_flag_fmt = 'RTE_LIBRTE_@0@_PMD'
+driver_name_fmt = 'rte_pmd_@0@'
new file mode 100644
@@ -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_pmd_telemetry.a
+
+# build flags
+CFLAGS += -O3
+CFLAGS += $(WERROR_FLAGS)
+
+LDLIBS += -lrte_eal -lrte_mbuf
+LDLIBS += -lrte_telemetry
+LDLIBS += -lrte_bus_vdev
+
+# versioning export map
+EXPORT_MAP := rte_pmd_telemetry_version.map
+
+# library version
+LIBABIVER := 1
+
+# library source files
+SRCS-$(CONFIG_RTE_LIBRTE_TELEMETRY) := telemetry_driver.c
+
+include $(RTE_SDK)/mk/rte.lib.mk
new file mode 100644
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2018 Intel Corporation
+
+sources = files('telemetry_driver.c')
+deps += ['telemetry', 'bus_pci', 'bus_vdev']
new file mode 100644
@@ -0,0 +1,9 @@
+DPDK_18.05 {
+ global:
+
+ telemetry_probe;
+ telemetry_remove;
+
+ local: *;
+
+};
new file mode 100644
@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2018 Intel Corporation
+ */
+
+#include <stdio.h>
+
+#include <rte_telemetry.h>
+#include <rte_malloc.h>
+#include <rte_bus_vdev.h>
+#include <rte_lcore.h>
+
+static int
+telemetry_probe(struct rte_vdev_device *vdev)
+{
+ int ret;
+
+ RTE_SET_USED(vdev);
+ ret = rte_telemetry_init(rte_socket_id());
+ if (ret < 0) {
+ printf("Error - Telemetry initialisation failed\n");
+ return -1;
+ }
+ return 0;
+}
+
+static int
+telemetry_remove(struct rte_vdev_device *vdev)
+{
+ const char *name;
+ name = rte_vdev_device_name(vdev);
+ printf("Uninitialising the device: %s\n", name);
+ return 0;
+}
+
+static struct rte_vdev_driver pmd_telemetry_drv = {
+ .probe = telemetry_probe,
+ .remove = telemetry_remove
+};
+
+RTE_PMD_REGISTER_VDEV(telemetry, pmd_telemetry_drv);
@@ -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
new file mode 100644
@@ -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
new file mode 100644
@@ -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'
new file mode 100644
@@ -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");
+ 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__);
+ 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");
+ }
+ 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;
+}
+
+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);
+}
new file mode 100644
@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2018 Intel Corporation
+ */
+
+#include <stdint.h>
+
+#ifndef _RTE_TELEMETRY_H_
+#define _RTE_TELEMETRY_H_
+
+/**
+ * Get the telemetry_impl structure device pointer initialised.
+ *
+ * @param socket_id
+ * Unsigned integer representing the socket id to be used
+ * for the telemetry structure.
+ *
+ * @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
+rte_telemetry_init(uint32_t socket_id);
+
+/**
+ * Clean up and free memory.
+ *
+ * @return
+ * 0 on success
+ * @return
+ * -EPERM on failure
+ */
+int32_t
+rte_telemetry_cleanup(void);
+
+#endif
new file mode 100644
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2018 Intel Corporation
+ */
+
+#include <rte_log.h>
+
+#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;
+ uint32_t socket_id;
+} telemetry_impl;
+
+#endif
new file mode 100644
@@ -0,0 +1,6 @@
+DPDK_18.05 {
+ global:
+
+ rte_telemetry_init;
+ local: *;
+};
@@ -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')
@@ -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
@@ -164,6 +165,7 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_SOFTNIC) += -lrte_pmd_softnic
endif
_LDLIBS-$(CONFIG_RTE_LIBRTE_SFC_EFX_PMD) += -lrte_pmd_sfc_efx
_LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_SZEDATA2) += -lrte_pmd_szedata2 -lsze2
+_LDLIBS-$(CONFIG_RTE_LIBRTE_TELEMETRY) += -lrte_pmd_telemetry
_LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_TAP) += -lrte_pmd_tap
_LDLIBS-$(CONFIG_RTE_LIBRTE_THUNDERX_NICVF_PMD) += -lrte_pmd_thunderx_nicvf
_LDLIBS-$(CONFIG_RTE_LIBRTE_VDEV_NETVSC_PMD) += -lrte_pmd_vdev_netvsc