[v1,02/32] eal: define the public API for trace support

  From: Jerin Jacob <jerinj@marvell.com>

Define the public API for trace support.
This patch also adds support for the build infrastructure and
update the MAINTAINERS file for the trace subsystem.

Signed-off-by: Jerin Jacob <jerinj@marvell.com>
Signed-off-by: Sunil Kumar Kori <skori@marvell.com>
---
 MAINTAINERS                               |   6 +
 config/common_base                        |   1 +
 config/rte_config.h                       |   1 +
 doc/api/doxy-api-index.md                 |   3 +-
 lib/librte_eal/common/Makefile            |   1 +
 lib/librte_eal/common/eal_common_trace.c  |   6 +
 lib/librte_eal/common/include/rte_trace.h | 525 ++++++++++++++++++++++
 lib/librte_eal/common/meson.build         |   2 +
 lib/librte_eal/freebsd/eal/Makefile       |   1 +
 lib/librte_eal/linux/eal/Makefile         |   1 +
 10 files changed, 546 insertions(+), 1 deletion(-)
 create mode 100644 lib/librte_eal/common/eal_common_trace.c
 create mode 100644 lib/librte_eal/common/include/rte_trace.h
  

On 2020-03-18 20:02, jerinj@marvell.com wrote:
> From: Jerin Jacob <jerinj@marvell.com>
>
> Define the public API for trace support.
> This patch also adds support for the build infrastructure and
> update the MAINTAINERS file for the trace subsystem.
>
> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> Signed-off-by: Sunil Kumar Kori <skori@marvell.com>
> ---
>   MAINTAINERS                               |   6 +
>   config/common_base                        |   1 +
>   config/rte_config.h                       |   1 +
>   doc/api/doxy-api-index.md                 |   3 +-
>   lib/librte_eal/common/Makefile            |   1 +
>   lib/librte_eal/common/eal_common_trace.c  |   6 +
>   lib/librte_eal/common/include/rte_trace.h | 525 ++++++++++++++++++++++
>   lib/librte_eal/common/meson.build         |   2 +
>   lib/librte_eal/freebsd/eal/Makefile       |   1 +
>   lib/librte_eal/linux/eal/Makefile         |   1 +
>   10 files changed, 546 insertions(+), 1 deletion(-)
>   create mode 100644 lib/librte_eal/common/eal_common_trace.c
>   create mode 100644 lib/librte_eal/common/include/rte_trace.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c3785554f..63d85c7da 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -196,6 +196,12 @@ F: app/test/test_string_fns.c
>   F: app/test/test_tailq.c
>   F: app/test/test_version.c
>   
> +Trace
> +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
> +
>   Memory Allocation
>   M: Anatoly Burakov <anatoly.burakov@intel.com>
>   F: lib/librte_eal/common/include/rte_fbarray.h
> diff --git a/config/common_base b/config/common_base
> index 7ca2f28b1..efc195af2 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -99,6 +99,7 @@ CONFIG_RTE_MAX_MEMZONE=2560
>   CONFIG_RTE_MAX_TAILQ=32
>   CONFIG_RTE_ENABLE_ASSERT=n
>   CONFIG_RTE_LOG_DP_LEVEL=RTE_LOG_INFO
> +CONFIG_RTE_ENABLE_TRACE_DP=n
>   CONFIG_RTE_LOG_HISTORY=256
>   CONFIG_RTE_BACKTRACE=y
>   CONFIG_RTE_LIBEAL_USE_HPET=n
> diff --git a/config/rte_config.h b/config/rte_config.h
> index d30786bc0..6b250288c 100644
> --- a/config/rte_config.h
> +++ b/config/rte_config.h
> @@ -42,6 +42,7 @@
>   #define RTE_MAX_MEMZONE 2560
>   #define RTE_MAX_TAILQ 32
>   #define RTE_LOG_DP_LEVEL RTE_LOG_INFO
> +#define RTE_ENABLE_TRACE_DP 0
>   #define RTE_BACKTRACE 1
>   #define RTE_MAX_VFIO_CONTAINERS 64
>   
> diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
> index dff496be0..190f0ccc2 100644
> --- a/doc/api/doxy-api-index.md
> +++ b/doc/api/doxy-api-index.md
> @@ -174,7 +174,8 @@ The public API headers are grouped by topics:
>     [hexdump]            (@ref rte_hexdump.h),
>     [debug]              (@ref rte_debug.h),
>     [log]                (@ref rte_log.h),
> -  [errno]              (@ref rte_errno.h)
> +  [errno]              (@ref rte_errno.h),
> +  [trace]              (@ref rte_trace.h)
>   
>   - **misc**:
>     [EAL config]         (@ref rte_eal.h),
> diff --git a/lib/librte_eal/common/Makefile b/lib/librte_eal/common/Makefile
> index c2c6d92cd..9384d6f6e 100644
> --- a/lib/librte_eal/common/Makefile
> +++ b/lib/librte_eal/common/Makefile
> @@ -9,6 +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_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
> new file mode 100644
> index 000000000..e18ba1c95
> --- /dev/null
> +++ b/lib/librte_eal/common/eal_common_trace.c
> @@ -0,0 +1,6 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2020 Marvell International Ltd.
> + */
> +
> +#include <rte_trace.h>
> +
> diff --git a/lib/librte_eal/common/include/rte_trace.h b/lib/librte_eal/common/include/rte_trace.h
> new file mode 100644
> index 000000000..d008b64f1
> --- /dev/null
> +++ b/lib/librte_eal/common/include/rte_trace.h
> @@ -0,0 +1,525 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2020 Marvell International Ltd.
> + */
> +
> +#ifndef _RTE_TRACE_H_
> +#define _RTE_TRACE_H_
> +
> +/**
> + * @file
> + *
> + * RTE Trace API
> + *
> + * This file provides the trace API to RTE applications.
> + *
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <stdbool.h>
> +#include <stdio.h>
> +
> +#include <rte_common.h>
> +#include <rte_compat.h>
> +
> +/** The trace object. The trace APIs are based on this opaque object. */
> +typedef uint64_t *rte_trace_t;

Wouldn't a forward-declared struct, with the definition hidden from the 
user, be more appropriate? As a bonus, you'll get some type checking.

"struct rte_trace;" here and "struct rte_trace*" in all the APIs. 
"struct rte_trace { uint64_t val; }; in the implementation. Or just cast 
it to a uint64_t *.

typdef:ing pointers is generally considered a no-no, at least if you 
follow the Linux kernel coding conventions.

> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Enumerate trace mode operation.
> + */
> +enum rte_trace_mode_e {
> +	/**
> +	 * In this mode, When no space left in trace buffer, the subsequent
> +	 * events overwrite the old events in the trace buffer.
> +	 */
> +	RTE_TRACE_MODE_OVERWRITE,
> +	/**
> +	 * In this mode, When no space left on trace buffer, the subsequent
> +	 * events shall not be recorded in the trace buffer.
> +	 */
> +	RTE_TRACE_MODE_DISCARD,
> +};

Have you considered having a blocking mode as well, where the thread 
will just wait for space to be freed?

Remove the "_e" from the name. "enum" already tells us it's an enumeration.

> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Test if global trace is enabled.
> + *
> + * @return
> + *    true if global trace is enabled, false otherwise.
> + */
> +__rte_experimental
> +bool rte_trace_global_is_enabled(void);

My impression is that DPDK does:


__rte_experimental bool

rte_trace_global_is_enabled(void);


Now when I check the coding conventions, that's only for function 
definition. Why declaration and definition should be different, I don't 
know.

> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Test if global trace is disabled.
> + *
> + * @return
> + *    true if global trace is disabled, false otherwise.
> + */
> +__rte_experimental
> +bool rte_trace_global_is_disabled(void);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Test if a given trace is invalid.
> + * @param trace
> + *    The trace object.
> + * @return
> + *    true if global trace is invalid, false otherwise.
> + */
> +__rte_experimental
> +bool rte_trace_is_id_invalid(rte_trace_t trace);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Set the global trace level.
> + *
> + * After this call, trace with a level lower or equal than the level
> + * passed as argument will be captured in the trace buffer.
> + *
> + * @param level
> + *   Trace level. A value between RTE_LOG_EMERG (1) and RTE_LOG_DEBUG (8).
> + */
> +__rte_experimental
> +void rte_trace_global_level_set(uint32_t level);

uint32_t means a lot of levels.

> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Get the global trace level.
> + *
> + * @return
> + *   The current global trace level.
> + */
> +__rte_experimental
> +uint32_t rte_trace_global_level_get(void);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Set the global trace mode.
> + *
> + * After this call, All tracepoints will be switched to new mode.
> + *
> + * @param mode
> + *   Trace mode.
> + */
> +__rte_experimental
> +void rte_trace_global_mode_set(enum rte_trace_mode_e mode);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Get the global trace mode.
> + *
> + * @return
> + *   The current global trace mode.
> + */
> +__rte_experimental
> +enum rte_trace_mode_e rte_trace_global_mode_get(void);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Enable recording of the given tracepoint in the trace buffer.
> + *
> + * @param trace
> + *   The tracepoint object to enable.
> + * @return
> + *   - 0: Success.
> + *   - (-ERANGE): Trace object is not registered.
> + *   - (-EACCES): Trace object level is less than the global trace level.
> + */
> +__rte_experimental
> +int rte_trace_enable(rte_trace_t trace);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Disable recording of the given tracepoint in the trace buffer.
> + *
> + * @param trace
> + *   The tracepoint object to disable.
> + * @return
> + *   - 0: Success.
> + *   - (-ERANGE): Trace object is not registered.
> + */
> +__rte_experimental
> +int rte_trace_disable(rte_trace_t trace);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Test if given trace is enabled.
> + *
> + * @param trace
> + *    The trace object.
> + * @return
> + *    true if trace is enabled, false otherwise.
> + */
> +__rte_experimental
> +bool rte_trace_is_enabled(rte_trace_t trace);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Test if given trace is disabled.
> + *
> + * @param trace
> + *    The trace object.
> + * @return
> + *    true if trace is disabled, false otherwise.
> + */
> +__rte_experimental
> +bool rte_trace_is_disabled(rte_trace_t trace);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Set the trace level for the given tracepoint.
> + *
> + * After this call, if passed trace level lower or equal than the global trace
> + * level and this trace is enabled then trace will be captured in the
> + * trace buffer.
> + *
> + * @param trace
> + *    The trace object.
> + * @param level
> + *   Trace level. A value between RTE_LOG_EMERG (1) and RTE_LOG_DEBUG (8).
> + * @return
> + *   - 0: Success.
> + *   - (-EINVAL): Trace object is not registered or invalid trace level.
> + */
> +__rte_experimental
> +int rte_trace_level_set(rte_trace_t trace, uint32_t level);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Get the trace level for the given tracepoint.
> + *
> + * @param trace
> + *    The trace object.
> + * @return
> + *   - A value between RTE_LOG_EMERG (1) and RTE_LOG_DEBUG (8).
> + *   - 0: Trace object is not registered.
> + */
> +__rte_experimental
> +uint32_t rte_trace_level_get(rte_trace_t trace);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Set the trace mode for the given tracepoint.
> + *
> + * @param trace
> + *    The trace object.
> + * @param mode
> + *   Trace mode.
> + * @return
> + *   - 0: Success.
> + *   - (-EINVAL): Trace object is not registered or invalid trace level.
> + */
> +__rte_experimental
> +int rte_trace_mode_set(rte_trace_t trace, enum rte_trace_mode_e mode);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Get the trace mode for the given tracepoint.
> + *
> + * @param trace
> + *    The trace object.
> + * @return
> + *   - Zero or positive: Mode encoded as enum rte_trace_mode_e.
> + *   - (-EINVAL): Trace object is not registered.
> + */
> +__rte_experimental
> +int rte_trace_mode_get(rte_trace_t trace);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Enable/Disable a set of tracepoints based on shell pattern.
Shell pattern means what I think is usually referred to as a glob?
> + *
> + * @param pattern
> + *   The match pattern identifying the tracepoint.
> + * @param enable
> + *    true to enable tracepoint, false to disable the tracepoint, upon match.
> + * @param[out] found
> + *    NULL value allowed, if not NULL, true if match found, false otherwise.
> + * @return
> + *   - 0: Success.
> + *   - (-ERANGE): Trace object is not registered.
> + */
> +__rte_experimental
> +int rte_trace_pattern(const char *pattern, bool enable, bool *found);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Enable/Disable a set of tracepoints based on regular expression.
> + *
> + * @param regex
> + *   A regular expression identifying the tracepoint.
> + * @param enable
> + *    true to enable tracepoint, false to disable the tracepoint, upon match.
> + * @param[out] found
> + *    NULL value allowed, if not NULL, true if match found, false otherwise.
What's the reason of having this output parameter, as opposed to coding 
the information into the return code?
> + * @return
> + *   - 0: Success.
> + *   - (-ERANGE): Trace object is not registered.
> + *   - (-EINVAL): Invalid regular expression rule.
> + */
> +__rte_experimental
> +int rte_trace_regexp(const char *regex, bool enable, bool *found);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Save the trace buffer to the trace directory.
> + *
> + * By default, trace directory will be created at HOME directory and this can be
> + * overridden by --trace-dir EAL parameter.
> + *
> + * @return
> + *   - 0: Success.
> + *   - <0 : Failure.
> + */
> +__rte_experimental
> +int rte_trace_save(void);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Search a trace object from its name.
> + *
> + * @param name
> + *   The name of the tracepoint.
> + * @return
> + *   The tracepoint object or NULL if not found.
> + */
> +__rte_experimental
> +rte_trace_t rte_trace_from_name(const char *name);

Would "rte_trace_by_name" be a better name?

> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Dump the trace metadata to a file.
> + *
> + * @param f
> + *   A pointer to a file for output
> + * @return
> + *   - 0: Success.
> + *   - <0 : Failure.
> + */
> +__rte_experimental
> +int rte_trace_metadata_dump(FILE *f);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + * Dump the trace subsystem status to a file.
> + *
> + * @param f
> + *   A pointer to a file for output
> + */
> +__rte_experimental
> +void rte_trace_dump(FILE *f);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Test if the trace datapath compile-time option is enabled.
> + *
> + * @return
> + *   A positive value if trace datapath enabled, value zero otherwise.
Switch to a bool return type.
> + */
> +static __rte_always_inline int
> +rte_trace_is_dp_enabled(void)
> +{
> +#ifdef RTE_ENABLE_TRACE_DP
> +	return RTE_ENABLE_TRACE_DP;
> +#else
> +	return 0;
> +#endif
> +}
> +
> +/** Macro to define the tracepoint. */
> +#define RTE_TRACE_POINT_DEFINE(tp)\
> +uint64_t __attribute__((section("__rte_trace_point"))) __##tp
> +
> +/**
> + * Macro to define the tracepoint arguments in RTE_TRACE_POINT macro.
> +
> + * @see RTE_TRACE_POINT RTE_TRACE_POINT_DP
> + */
> +#define RTE_TRACE_POINT_ARGS
> +
> +/** @internal Helper Macro to support RTE_TRACE_POINT and RTE_TRACE_POINT_DP */
> +#define __RTE_TRACE_POINT(_mode, _tp, _args, ...)\
> +extern uint64_t __##_tp;\
> +static __rte_always_inline void \
> +_tp _args \
> +{\
> +	__rte_trace_emit_header_##_mode(&__##_tp);\
> +	__VA_ARGS__\
> +}
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Create a tracepoint definition.
> + *
> + * A tracepoint definition defines, for a given tracepoint:
> + * - Its input arguments. They are the C function style parameters to define
> + * the arguments of tracepoint function. These input arguments embedded using
> + * RTE_TRACE_POINT_ARGS macro.
> + * - Its output event fields. They are the sources of event fields that form
> + * the payload of any event that the execution of the tracepoint macro emits
> + * for this particular tracepoint. The application uses rte_trace_ctf_* macros
> + * to emit the output event fields.
> + *
> + * @param tp
> + *   Tracepoint object. Before using the tracepoint, an application needs to
> + * define the tracepoint using RTE_TRACE_POINT_DEFINE() macro.
> + * @param args
> + *   C function style input arguments to define the arguments to tracepoint
> + * function.
> + * @param ...
> + *   Define the payload of trace function. The payload will be formed using
> + * rte_trace_ctf_* macros, Use ";" delimiter between two payloads.
> + *
> + * @see RTE_TRACE_POINT_ARGS, RTE_TRACE_POINT_DEFINE, rte_trace_ctf_*
> + */
> +#define RTE_TRACE_POINT(tp, args, ...)\
> +	__RTE_TRACE_POINT(generic, tp, args, __VA_ARGS__)
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Create a tracepoint definition for the data path.
> + *
> + * Similar to RTE_TRACE_POINT(), except that it is removed at compilation time
> + * using RTE_ENABLE_TRACE_DP configuration parameter.
> + *
> + * @param tp
> + *   Tracepoint object. Before using the tracepoint, an application needs to
> + * define the tracepoint using RTE_TRACE_POINT_DEFINE() macro.
> + * @param args
> + *   C function style input arguments to define the arguments to tracepoint
> + * function.
> + * @param ...
> + *   Define the payload of trace function. The payload will be formed using
> + * rte_trace_ctf_* macros, Use ";" delimiter between two payloads.
> + *
> + * @see rte_trace_is_dp_enabled, RTE_TRACE_POINT()
> + */
> +#define RTE_TRACE_POINT_DP(tp, args, ...)\
> +	__RTE_TRACE_POINT(dp, tp, args, __VA_ARGS__)
> +
> +#ifdef __DOXYGEN__
> +
> +/**
> + * Macro to select rte_trace_ctf_* definition for trace register function.
> + *
> + * rte_trace_ctf_* emits different definitions for trace function.
> + * Application must define RTE_TRACE_POINT_REGISTER_SELECT before including
> + * rte_trace.h in the C file where RTE_TRACE_POINT_REGISTER() used.
> + *
> + * @see RTE_TRACE_POINT_REGISTER()
> + */
> +#define RTE_TRACE_POINT_REGISTER_SELECT
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Register a dynamic tracepoint.
> + *
> + * @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).
> + * @return
> + *   - 0: Successfully registered the tracepoint.
> + *   - <0: Failure to register the tracepoint.
> + *
> + * @see RTE_TRACE_POINT_REGISTER_SELECT
> + */
> +#define RTE_TRACE_POINT_REGISTER(trace, name, level)
> +
> +/** Tracepoint function payload for uint64_t datatype */
> +#define rte_trace_ctf_u64(val)
> +/** Tracepoint function payload for int64_t datatype */
> +#define rte_trace_ctf_i64(val)
> +/** Tracepoint function payload for uint32_t datatype */
> +#define rte_trace_ctf_u32(val)
> +/** Tracepoint function payload for int32_t datatype */
> +#define rte_trace_ctf_i32(val)
> +/** Tracepoint function payload for uint16_t datatype */
> +#define rte_trace_ctf_u16(val)
> +/** Tracepoint function payload for int16_t datatype */
> +#define rte_trace_ctf_i16(val)
> +/** Tracepoint function payload for uint8_t datatype */
> +#define rte_trace_ctf_u8(val)
> +/** Tracepoint function payload for int8_t datatype */
> +#define rte_trace_ctf_i8(val)
> +/** Tracepoint function payload for int datatype */
> +#define rte_trace_ctf_int(val)
> +/** Tracepoint function payload for long datatype */
> +#define rte_trace_ctf_long(val)
> +/** Tracepoint function payload for float datatype */
> +#define rte_trace_ctf_float(val)
> +/** Tracepoint function payload for double datatype */
> +#define rte_trace_ctf_double(val)
> +/** Tracepoint function payload for pointer datatype */
> +#define rte_trace_ctf_ptr(val)
> +/** Tracepoint function payload for string datatype */
> +#define rte_trace_ctf_string(val)
> +
> +#endif /* __DOXYGEN__ */
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* _RTE_TRACE_H_ */
> diff --git a/lib/librte_eal/common/meson.build b/lib/librte_eal/common/meson.build
> index 2b97715a2..30fb9b85f 100644
> --- a/lib/librte_eal/common/meson.build
> +++ b/lib/librte_eal/common/meson.build
> @@ -28,6 +28,7 @@ common_sources = files(
>   	'eal_common_tailqs.c',
>   	'eal_common_thread.c',
>   	'eal_common_timer.c',
> +	'eal_common_trace.c',
>   	'eal_common_uuid.c',
>   	'hotplug_mp.c',
>   	'malloc_elem.c',
> @@ -84,6 +85,7 @@ common_headers = files(
>   	'include/rte_service_component.h',
>   	'include/rte_string_fns.h',
>   	'include/rte_tailq.h',
> +	'include/rte_trace.h',
>   	'include/rte_time.h',
>   	'include/rte_uuid.h',
>   	'include/rte_version.h',
> diff --git a/lib/librte_eal/freebsd/eal/Makefile b/lib/librte_eal/freebsd/eal/Makefile
> index b160b5790..b2fcc4212 100644
> --- a/lib/librte_eal/freebsd/eal/Makefile
> +++ b/lib/librte_eal/freebsd/eal/Makefile
> @@ -60,6 +60,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += eal_common_thread.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += eal_common_proc.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += eal_common_fbarray.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += eal_common_uuid.c
> +SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += eal_common_trace.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += rte_malloc.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += hotplug_mp.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += malloc_elem.c
> diff --git a/lib/librte_eal/linux/eal/Makefile b/lib/librte_eal/linux/eal/Makefile
> index e70cf104a..95470d3bb 100644
> --- a/lib/librte_eal/linux/eal/Makefile
> +++ b/lib/librte_eal/linux/eal/Makefile
> @@ -68,6 +68,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += eal_common_thread.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += eal_common_proc.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += eal_common_fbarray.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += eal_common_uuid.c
> +SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += eal_common_trace.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += rte_malloc.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += hotplug_mp.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += malloc_elem.c
  

On Thu, Mar 19, 2020 at 2:28 AM Mattias Rönnblom
<mattias.ronnblom@ericsson.com> wrote:
>
> On 2020-03-18 20:02, jerinj@marvell.com wrote:
> > From: Jerin Jacob <jerinj@marvell.com>

> > +
> > +#include <rte_common.h>
> > +#include <rte_compat.h>
> > +
> > +/** The trace object. The trace APIs are based on this opaque object. */
> > +typedef uint64_t *rte_trace_t;
>
> Wouldn't a forward-declared struct, with the definition hidden from the
> user, be more appropriate? As a bonus, you'll get some type checking.
>
> "struct rte_trace;" here and "struct rte_trace*" in all the APIs.
> "struct rte_trace { uint64_t val; }; in the implementation. Or just cast
> it to a uint64_t *.

OK. I will remove the typedef then.

>
> typdef:ing pointers is generally considered a no-no, at least if you
> follow the Linux kernel coding conventions.
>
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Enumerate trace mode operation.
> > + */
> > +enum rte_trace_mode_e {
> > +     /**
> > +      * In this mode, When no space left in trace buffer, the subsequent
> > +      * events overwrite the old events in the trace buffer.
> > +      */
> > +     RTE_TRACE_MODE_OVERWRITE,
> > +     /**
> > +      * In this mode, When no space left on trace buffer, the subsequent
> > +      * events shall not be recorded in the trace buffer.
> > +      */
> > +     RTE_TRACE_MODE_DISCARD,
> > +};
>
> Have you considered having a blocking mode as well, where the thread
> will just wait for space to be freed?

The trace buffer is per thread. So there is no waiting.

The new features can be added later as needed by extending the mode.

>
> Remove the "_e" from the name. "enum" already tells us it's an enumeration.

OK. I will remove it in v2.

>
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Test if global trace is enabled.
> > + *
> > + * @return
> > + *    true if global trace is enabled, false otherwise.
> > + */
> > +__rte_experimental
> > +bool rte_trace_global_is_enabled(void);
>
> My impression is that DPDK does:
>
>
> __rte_experimental bool
>
> rte_trace_global_is_enabled(void);
>
>
> Now when I check the coding conventions, that's only for function
> definition. Why declaration and definition should be different, I don't
> know.

I see two patterns.(Both cases __rte_experimental in a new line)

__rte_experimental
bool
rte_trace_global_is_enabled(void);

or

__rte_experimental
bool rte_trace_global_is_enabled(void);

For the prototype case, I prefer a later option and for definition
case the first option.
If there are no specific opinions, I would like to stick to this model



> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Set the global trace level.
> > + *
> > + * After this call, trace with a level lower or equal than the level
> > + * passed as argument will be captured in the trace buffer.
> > + *
> > + * @param level
> > + *   Trace level. A value between RTE_LOG_EMERG (1) and RTE_LOG_DEBUG (8).
> > + */
> > +__rte_experimental
> > +void rte_trace_global_level_set(uint32_t level);
>
> uint32_t means a lot of levels.

Yes. I did this to make compatibly with rte_log level datatype.


> > + *
> > + * @param trace
> > + *    The trace object.
> > + * @return
> > + *   - Zero or positive: Mode encoded as enum rte_trace_mode_e.
> > + *   - (-EINVAL): Trace object is not registered.
> > + */
> > +__rte_experimental
> > +int rte_trace_mode_get(rte_trace_t trace);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Enable/Disable a set of tracepoints based on shell pattern.
> Shell pattern means what I think is usually referred to as a glob?

According, "man 3 fnmatch", it can be both.
No preference here, The rte_log is using shell pattern as the comment
so I thought of
using the same.

Thoughts?


> > + *
> > + * @param pattern
> > + *   The match pattern identifying the tracepoint.
> > + * @param enable
> > + *    true to enable tracepoint, false to disable the tracepoint, upon match.
> > + * @param[out] found
> > + *    NULL value allowed, if not NULL, true if match found, false otherwise.
> > + * @return
> > + *   - 0: Success.
> > + *   - (-ERANGE): Trace object is not registered.
> > + */
> > +__rte_experimental
> > +int rte_trace_pattern(const char *pattern, bool enable, bool *found);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Enable/Disable a set of tracepoints based on regular expression.
> > + *
> > + * @param regex
> > + *   A regular expression identifying the tracepoint.
> > + * @param enable
> > + *    true to enable tracepoint, false to disable the tracepoint, upon match.
> > + * @param[out] found
> > + *    NULL value allowed, if not NULL, true if match found, false otherwise.
> What's the reason of having this output parameter, as opposed to coding
> the information into the return code?

I thought making explicit is good . No strong opinion here.
How about?

0: Success but no pattern found
>0: Sucess and pattern found
<0: error



> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Search a trace object from its name.
> > + *
> > + * @param name
> > + *   The name of the tracepoint.
> > + * @return
> > + *   The tracepoint object or NULL if not found.
> > + */
> > +__rte_experimental
> > +rte_trace_t rte_trace_from_name(const char *name);
>
> Would "rte_trace_by_name" be a better name?

No strong preference. I will change to rte_trace_by_name().

>
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Dump the trace metadata to a file.
> > + *
> > + * @param f
> > + *   A pointer to a file for output
> > + * @return
> > + *   - 0: Success.
> > + *   - <0 : Failure.
> > + */
> > +__rte_experimental
> > +int rte_trace_metadata_dump(FILE *f);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + * Dump the trace subsystem status to a file.
> > + *
> > + * @param f
> > + *   A pointer to a file for output
> > + */
> > +__rte_experimental
> > +void rte_trace_dump(FILE *f);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Test if the trace datapath compile-time option is enabled.
> > + *
> > + * @return
> > + *   A positive value if trace datapath enabled, value zero otherwise.
> Switch to a bool return type.

I thought of avoiding "bool" in fastpath, I will change to bool if
there is no performance
impact.


> > + */
> > +static __rte_always_inline int
> > +rte_trace_is_dp_enabled(void)
> > +{
> > +#ifdef RTE_ENABLE_TRACE_DP
> > +     return RTE_ENABLE_TRACE_DP;
> > +#else
> > +     return 0;
> > +#endif
> > +}
> > +
  

On 2020-03-23 15:29, Jerin Jacob wrote:
> On Thu, Mar 19, 2020 at 2:28 AM Mattias Rönnblom
> <mattias.ronnblom@ericsson.com> wrote:
>> On 2020-03-18 20:02, jerinj@marvell.com wrote:
>>> From: Jerin Jacob <jerinj@marvell.com>
>>> +
>>> +#include <rte_common.h>
>>> +#include <rte_compat.h>
>>> +
>>> +/** The trace object. The trace APIs are based on this opaque object. */
>>> +typedef uint64_t *rte_trace_t;
>> Wouldn't a forward-declared struct, with the definition hidden from the
>> user, be more appropriate? As a bonus, you'll get some type checking.
>>
>> "struct rte_trace;" here and "struct rte_trace*" in all the APIs.
>> "struct rte_trace { uint64_t val; }; in the implementation. Or just cast
>> it to a uint64_t *.
> OK. I will remove the typedef then.
>
>> typdef:ing pointers is generally considered a no-no, at least if you
>> follow the Linux kernel coding conventions.
>>
>>> +
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice
>>> + *
>>> + * Enumerate trace mode operation.
>>> + */
>>> +enum rte_trace_mode_e {
>>> +     /**
>>> +      * In this mode, When no space left in trace buffer, the subsequent
>>> +      * events overwrite the old events in the trace buffer.
>>> +      */
>>> +     RTE_TRACE_MODE_OVERWRITE,
>>> +     /**
>>> +      * In this mode, When no space left on trace buffer, the subsequent
>>> +      * events shall not be recorded in the trace buffer.
>>> +      */
>>> +     RTE_TRACE_MODE_DISCARD,
>>> +};
>> Have you considered having a blocking mode as well, where the thread
>> will just wait for space to be freed?
> The trace buffer is per thread. So there is no waiting.
>
> The new features can be added later as needed by extending the mode.
>
>> Remove the "_e" from the name. "enum" already tells us it's an enumeration.
> OK. I will remove it in v2.
>
>>> +
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice
>>> + *
>>> + * Test if global trace is enabled.
>>> + *
>>> + * @return
>>> + *    true if global trace is enabled, false otherwise.
>>> + */
>>> +__rte_experimental
>>> +bool rte_trace_global_is_enabled(void);
>> My impression is that DPDK does:
>>
>>
>> __rte_experimental bool
>>
>> rte_trace_global_is_enabled(void);
>>
>>
>> Now when I check the coding conventions, that's only for function
>> definition. Why declaration and definition should be different, I don't
>> know.
> I see two patterns.(Both cases __rte_experimental in a new line)
>
> __rte_experimental
> bool
> rte_trace_global_is_enabled(void);
>
> or
>
> __rte_experimental
> bool rte_trace_global_is_enabled(void);
>
> For the prototype case, I prefer a later option and for definition
> case the first option.
> If there are no specific opinions, I would like to stick to this model
>
>
>
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice
>>> + *
>>> + * Set the global trace level.
>>> + *
>>> + * After this call, trace with a level lower or equal than the level
>>> + * passed as argument will be captured in the trace buffer.
>>> + *
>>> + * @param level
>>> + *   Trace level. A value between RTE_LOG_EMERG (1) and RTE_LOG_DEBUG (8).
>>> + */
>>> +__rte_experimental
>>> +void rte_trace_global_level_set(uint32_t level);
>> uint32_t means a lot of levels.
> Yes. I did this to make compatibly with rte_log level datatype.
>
>
>>> + *
>>> + * @param trace
>>> + *    The trace object.
>>> + * @return
>>> + *   - Zero or positive: Mode encoded as enum rte_trace_mode_e.
>>> + *   - (-EINVAL): Trace object is not registered.
>>> + */
>>> +__rte_experimental
>>> +int rte_trace_mode_get(rte_trace_t trace);
>>> +
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice
>>> + *
>>> + * Enable/Disable a set of tracepoints based on shell pattern.
>> Shell pattern means what I think is usually referred to as a glob?
> According, "man 3 fnmatch", it can be both.
> No preference here, The rte_log is using shell pattern as the comment
> so I thought of
> using the same.
>
> Thoughts?
>
If we're using "shell pattern" already, it would make sense to stick 
with this.
>>> + *
>>> + * @param pattern
>>> + *   The match pattern identifying the tracepoint.
>>> + * @param enable
>>> + *    true to enable tracepoint, false to disable the tracepoint, upon match.
>>> + * @param[out] found
>>> + *    NULL value allowed, if not NULL, true if match found, false otherwise.
>>> + * @return
>>> + *   - 0: Success.
>>> + *   - (-ERANGE): Trace object is not registered.
>>> + */
>>> +__rte_experimental
>>> +int rte_trace_pattern(const char *pattern, bool enable, bool *found);
>>> +
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice
>>> + *
>>> + * Enable/Disable a set of tracepoints based on regular expression.
>>> + *
>>> + * @param regex
>>> + *   A regular expression identifying the tracepoint.
>>> + * @param enable
>>> + *    true to enable tracepoint, false to disable the tracepoint, upon match.
>>> + * @param[out] found
>>> + *    NULL value allowed, if not NULL, true if match found, false otherwise.
>> What's the reason of having this output parameter, as opposed to coding
>> the information into the return code?
> I thought making explicit is good . No strong opinion here.
> How about?
>
> 0: Success but no pattern found
>> 0: Sucess and pattern found
> <0: error
>
Sounds like an improvement to me.

>
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice
>>> + *
>>> + * Search a trace object from its name.
>>> + *
>>> + * @param name
>>> + *   The name of the tracepoint.
>>> + * @return
>>> + *   The tracepoint object or NULL if not found.
>>> + */
>>> +__rte_experimental
>>> +rte_trace_t rte_trace_from_name(const char *name);
>> Would "rte_trace_by_name" be a better name?
> No strong preference. I will change to rte_trace_by_name().
>
>>> +
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice
>>> + *
>>> + * Dump the trace metadata to a file.
>>> + *
>>> + * @param f
>>> + *   A pointer to a file for output
>>> + * @return
>>> + *   - 0: Success.
>>> + *   - <0 : Failure.
>>> + */
>>> +__rte_experimental
>>> +int rte_trace_metadata_dump(FILE *f);
>>> +
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice
>>> + * Dump the trace subsystem status to a file.
>>> + *
>>> + * @param f
>>> + *   A pointer to a file for output
>>> + */
>>> +__rte_experimental
>>> +void rte_trace_dump(FILE *f);
>>> +
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice
>>> + *
>>> + * Test if the trace datapath compile-time option is enabled.
>>> + *
>>> + * @return
>>> + *   A positive value if trace datapath enabled, value zero otherwise.
>> Switch to a bool return type.
> I thought of avoiding "bool" in fastpath, I will change to bool if
> there is no performance
> impact.
>
>
>>> + */
>>> +static __rte_always_inline int
>>> +rte_trace_is_dp_enabled(void)
>>> +{
>>> +#ifdef RTE_ENABLE_TRACE_DP
>>> +     return RTE_ENABLE_TRACE_DP;
>>> +#else
>>> +     return 0;
>>> +#endif
>>> +}
>>> +