On 2020-03-18 20:02, jer...@marvell.com wrote:
> From: Jerin Jacob <jer...@marvell.com>
>
> The consumers of trace API defines the tracepoint and registers
> to eal. Internally these tracepoints will be stored in STAILQ
> for future use. This patch implements the tracepoint
> registration function.
>
> Signed-off-by: Jerin Jacob <jer...@marvell.com>
> ---
> MAINTAINERS | 1 +
> lib/librte_eal/common/Makefile | 2 +-
> lib/librte_eal/common/eal_common_trace.c | 107 +++++++++++++++++-
> lib/librte_eal/common/eal_trace.h | 36 ++++++
> lib/librte_eal/common/include/rte_trace.h | 29 +++++
> .../common/include/rte_trace_provider.h | 24 ++++
> .../common/include/rte_trace_register.h | 20 ++++
> lib/librte_eal/common/meson.build | 2 +
> lib/librte_eal/rte_eal_version.map | 1 +
> 9 files changed, 220 insertions(+), 2 deletions(-)
> create mode 100644 lib/librte_eal/common/eal_trace.h
> create mode 100644 lib/librte_eal/common/include/rte_trace_provider.h
> create mode 100644 lib/librte_eal/common/include/rte_trace_register.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 63d85c7da..452fd2c4f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -201,6 +201,7 @@ M: Jerin Jacob <jer...@marvell.com>
> M: Sunil Kumar Kori <sk...@marvell.com>
> F: lib/librte_eal/common/include/rte_trace*.h
> F: lib/librte_eal/common/eal_common_trace*.c
> +F: lib/librte_eal/common/eal_trace.h
>
> Memory Allocation
> M: Anatoly Burakov <anatoly.bura...@intel.com>
> diff --git a/lib/librte_eal/common/Makefile b/lib/librte_eal/common/Makefile
> index 9384d6f6e..8f2f25c1d 100644
> --- a/lib/librte_eal/common/Makefile
> +++ b/lib/librte_eal/common/Makefile
> @@ -9,7 +9,7 @@ INC += rte_debug.h rte_eal.h rte_eal_interrupts.h
> INC += rte_errno.h rte_launch.h rte_lcore.h
> INC += rte_log.h rte_memory.h rte_memzone.h
> INC += rte_per_lcore.h rte_random.h
> -INC += rte_trace.h
> +INC += rte_trace.h rte_trace_provider.h rte_trace_register.h
> INC += rte_tailq.h rte_interrupts.h rte_alarm.h
> INC += rte_string_fns.h rte_version.h
> INC += rte_eal_memconfig.h
> diff --git a/lib/librte_eal/common/eal_common_trace.c
> b/lib/librte_eal/common/eal_common_trace.c
> index e18ba1c95..ddde04de5 100644
> --- a/lib/librte_eal/common/eal_common_trace.c
> +++ b/lib/librte_eal/common/eal_common_trace.c
> @@ -2,5 +2,110 @@
> * Copyright(C) 2020 Marvell International Ltd.
> */
>
> -#include <rte_trace.h>
> +#include <inttypes.h>
> +#include <sys/queue.h>
>
> +#include <rte_common.h>
> +#include <rte_errno.h>
> +#include <rte_lcore.h>
> +#include <rte_per_lcore.h>
> +#include <rte_string_fns.h>
> +
> +#include "eal_trace.h"
> +
> +RTE_DEFINE_PER_LCORE(volatile int, trace_point_sz);
> +RTE_DEFINE_PER_LCORE(char, ctf_field[TRACE_CTF_FIELD_SIZE]);
> +RTE_DEFINE_PER_LCORE(int, ctf_count);
> +
> +static struct trace_point_head tp_list = STAILQ_HEAD_INITIALIZER(tp_list);
> +static struct trace trace;
> +
> +int
> +__rte_trace_point_register(rte_trace_t handle, const char *name, uint32_t
> level,
> + void (*fn)(void))
Maybe a more descriptive name than 'fn' would be in order.
> +{
> + char *field = RTE_PER_LCORE(ctf_field);
> + struct trace_point *tp;
> + uint16_t sz;
> +
> + /* Sanity checks of arguments */
> + if (name == NULL || fn == NULL || handle == NULL) {
> + trace_err("invalid arguments");
> + rte_errno = EINVAL; goto fail;
> + }
> +
> + /* Sanity check of level */
> + if (level > RTE_LOG_DEBUG || level > UINT8_MAX) {
Consider a #define for the max level. If the type was uint8_t, you
wouldn't need to check max at all.
> + trace_err("invalid log level=%d", level);
> + rte_errno = EINVAL; goto fail;
> +
> + }
> +
> + /* Check the size of the trace point object */
> + RTE_PER_LCORE(trace_point_sz) = 0;
> + RTE_PER_LCORE(ctf_count) = 0;
> + fn();
> + if (RTE_PER_LCORE(trace_point_sz) == 0) {
> + trace_err("missing rte_trace_emit_header() in register fn");
> + rte_errno = EBADF; goto fail;
> + }
> +
> + /* Is size overflowed */
> + if (RTE_PER_LCORE(trace_point_sz) > UINT16_MAX) {
> + trace_err("trace point size overflowed");
> + rte_errno = ENOSPC; goto fail;
> + }
> +
> + /* Are we running out of space to store trace points? */
> + if (trace.nb_trace_points > UINT16_MAX) {
> + trace_err("trace point exceeds the max count");
> + rte_errno = ENOSPC; goto fail;
> + }
> +
> + /* Get the size of the trace point */
> + sz = RTE_PER_LCORE(trace_point_sz);
> + tp = calloc(1, sizeof(struct trace_point));
Not rte_zmalloc()? Are secondary processes accessing this memory?
> + if (tp == NULL) {
> + trace_err("fail to allocate trace point memory");
> + rte_errno = ENOMEM; goto fail;
Missing newline.
> + }
> +
> + /* Initialize the trace point */
> + if (rte_strscpy(tp->name, name, TRACE_POINT_NAME_SIZE) < 0) {
> + trace_err("name is too long");
> + rte_errno = E2BIG;
> + goto free;
> + }
> +
> + /* Copy the field data for future use */
> + if (rte_strscpy(tp->ctf_field, field, TRACE_CTF_FIELD_SIZE) < 0) {
> + trace_err("CTF field size is too long");
> + rte_errno = E2BIG;
> + goto free;
> + }
> +
> + /* Clear field memory for the next event */
> + memset(field, 0, TRACE_CTF_FIELD_SIZE);
> +
> + /* Form the trace handle */
> + *handle = sz;
> + *handle |= trace.nb_trace_points << __RTE_TRACE_FIELD_ID_SHIFT;
> + *handle |= (uint64_t)level << __RTE_TRACE_FIELD_LEVEL_SHIFT;
If *handle would be a struct, you could use a bitfield instead, and much
simplify this code.
> +
> + trace.nb_trace_points++;
> + tp->handle = handle;
> +
> + /* Add the trace point at tail */
> + STAILQ_INSERT_TAIL(&tp_list, tp, next);
> + __atomic_thread_fence(__ATOMIC_RELEASE);
> +
> + /* All Good !!! */
> + return 0;
> +free:
> + free(tp);
> +fail:
> + if (trace.register_errno == 0)
> + trace.register_errno = rte_errno;
> +
> + return -rte_errno;
> +}
> diff --git a/lib/librte_eal/common/eal_trace.h
> b/lib/librte_eal/common/eal_trace.h
> new file mode 100644
> index 000000000..9aef536a0
> --- /dev/null
> +++ b/lib/librte_eal/common/eal_trace.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2020 Marvell International Ltd.
> + */
> +
> +#ifndef __EAL_TRACE_H
> +#define __EAL_TRACE_H
> +
> +#include <rte_trace.h>
> +
> +#define trace_err(fmt, args...)\
> + RTE_LOG(ERR, EAL, "%s():%u " fmt "\n",\
> + __func__, __LINE__, ## args)
> +
> +#define trace_crit(fmt, args...)\
> + RTE_LOG(CRIT, EAL, "%s():%u " fmt "\n",\
> + __func__, __LINE__, ## args)
> +
> +#define TRACE_CTF_FIELD_SIZE 384
> +#define TRACE_POINT_NAME_SIZE 64
> +
> +struct trace_point {
> + STAILQ_ENTRY(trace_point) next;
> + rte_trace_t handle;
> + char name[TRACE_POINT_NAME_SIZE];
> + char ctf_field[TRACE_CTF_FIELD_SIZE];
> +};
> +
> +struct trace {
> + int register_errno;
> + uint32_t nb_trace_points;
> +};
> +
> +/* Trace point list functions */
> +STAILQ_HEAD(trace_point_head, trace_point);
> +
> +#endif /* __EAL_TRACE_H */
> diff --git a/lib/librte_eal/common/include/rte_trace.h
> b/lib/librte_eal/common/include/rte_trace.h
> index d008b64f1..da70dfdbb 100644
> --- a/lib/librte_eal/common/include/rte_trace.h
> +++ b/lib/librte_eal/common/include/rte_trace.h
> @@ -518,6 +518,35 @@ _tp _args \
>
> #endif /* __DOXYGEN__ */
>
> +/**
> + * @internal @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Helper function to register a dynamic tracepoint.
> + * Use RTE_TRACE_POINT_REGISTER() macro for tracepoint registration.
> + *
> + * @param trace
> + * The tracepoint object created using RTE_TRACE_POINT_DEFINE().
> + * @param name
> + * The name of the tracepoint object.
> + * @param level
> + * Trace level. A value between RTE_LOG_EMERG (1) and RTE_LOG_DEBUG (8).
> + * @param f
> + * Trace registration function.
> + * @return
> + * - 0: Successfully registered the tracepoint.
> + * - <0: Failure to register the tracepoint.
> + */
> +__rte_experimental
> +int __rte_trace_point_register(rte_trace_t trace, const char *name,
> + uint32_t level, void (*fn)(void));
> +
> +#ifdef RTE_TRACE_POINT_REGISTER_SELECT
> +#include <rte_trace_register.h>
> +#else
> +#include <rte_trace_provider.h>
> +#endif
> +
> #ifdef __cplusplus
> }
> #endif
> diff --git a/lib/librte_eal/common/include/rte_trace_provider.h
> b/lib/librte_eal/common/include/rte_trace_provider.h
> new file mode 100644
> index 000000000..b4da87ba1
> --- /dev/null
> +++ b/lib/librte_eal/common/include/rte_trace_provider.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2020 Marvell International Ltd.
> + */
> +
> +#ifndef _RTE_TRACE_H_
> +#error do not include this file directly, use <rte_trace.h> instead
> +#endif
> +
> +#ifndef _RTE_TRACE_PROVIDER_H_
> +#define _RTE_TRACE_PROVIDER_H_
> +
> +#define __RTE_TRACE_EVENT_HEADER_ID_SHIFT (48)
> +
> +#define __RTE_TRACE_FIELD_ENABLE_MASK (1ULL << 63)
> +#define __RTE_TRACE_FIELD_ENABLE_DISCARD (1ULL << 62)
> +#define __RTE_TRACE_FIELD_SIZE_SHIFT 0
> +#define __RTE_TRACE_FIELD_SIZE_MASK (0xffffULL <<
> __RTE_TRACE_FIELD_SIZE_SHIFT)
> +#define __RTE_TRACE_FIELD_ID_SHIFT (16)
> +#define __RTE_TRACE_FIELD_ID_MASK (0xffffULL << __RTE_TRACE_FIELD_ID_SHIFT)
> +#define __RTE_TRACE_FIELD_LEVEL_SHIFT (32)
> +#define __RTE_TRACE_FIELD_LEVEL_MASK (0xffULL <<
> __RTE_TRACE_FIELD_LEVEL_SHIFT)
> +
> +
> +#endif /* _RTE_TRACE_PROVIDER_H_ */
> diff --git a/lib/librte_eal/common/include/rte_trace_register.h
> b/lib/librte_eal/common/include/rte_trace_register.h
> new file mode 100644
> index 000000000..e9940b414
> --- /dev/null
> +++ b/lib/librte_eal/common/include/rte_trace_register.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2020 Marvell International Ltd.
> + */
> +
> +#ifndef _RTE_TRACE_H_
> +#error do not include this file directly, use <rte_trace.h> instead
> +#endif
> +
> +#ifndef _RTE_TRACE_REGISTER_H_
> +#define _RTE_TRACE_REGISTER_H_
> +
> +#include <rte_per_lcore.h>
> +
> +RTE_DECLARE_PER_LCORE(volatile int, trace_point_sz);
> +
> +#define RTE_TRACE_POINT_REGISTER(trace, name, level)\
> + __rte_trace_point_register(&__##trace, RTE_STR(name),\
> + RTE_LOG_ ## level, (void (*)(void)) trace)
> +
> +#endif /* _RTE_TRACE_REGISTER_H_ */
> diff --git a/lib/librte_eal/common/meson.build
> b/lib/librte_eal/common/meson.build
> index 30fb9b85f..88c14ebe5 100644
> --- a/lib/librte_eal/common/meson.build
> +++ b/lib/librte_eal/common/meson.build
> @@ -86,6 +86,8 @@ common_headers = files(
> 'include/rte_string_fns.h',
> 'include/rte_tailq.h',
> 'include/rte_trace.h',
> + 'include/rte_trace_provider.h',
> + 'include/rte_trace_register.h',
> 'include/rte_time.h',
> 'include/rte_uuid.h',
> 'include/rte_version.h',
> diff --git a/lib/librte_eal/rte_eal_version.map
> b/lib/librte_eal/rte_eal_version.map
> index cadfa6465..d97d14845 100644
> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -338,4 +338,5 @@ EXPERIMENTAL {
>
> # added in 20.05
> rte_thread_getname;
> + __rte_trace_point_register;
> };
