Re: [dpdk-dev] [PATCH v1 03/32] eal/trace: implement trace register API

["Mattias Rönnblom" <mattias.ronnblom@ericsson.com>]

On 2020-03-18 20:02, jerinj@marvell.com wrote:
> From: Jerin Jacob <jerinj@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 <jerinj@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 <jerinj@marvell.com>
>   M: Sunil Kumar Kori <skori@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.burakov@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;
>   };