[v1,03/32] eal/trace: implement trace register API
Checks
Commit Message
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
Comments
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;
> };
On Thu, Mar 19, 2020 at 3:33 PM Mattias Rönnblom
<mattias.ronnblom@ericsson.com> wrote:
>
> On 2020-03-18 20:02, jerinj@marvell.com wrote:
> > From: Jerin Jacob <jerinj@marvell.com>
> >
> > +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.
OK. I will change to "register_fn" in v2.
> > +{
> > + 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.
The reason for keeping level as uint32_t to keep compatibility
with rte_log level datatype. For some reason, rte_log defined level
as uint32_t, So I thought of sticking to that so that we can migrate the
rte_log to rte_trace in future if needed and also making semantics
similar to rte_log.
>
> > + 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?
This been called by the constructor at that time memory
services are not enabled and it is for the per-process like rte_log
scheme.
> > + if (tp == NULL) {
> > + trace_err("fail to allocate trace point memory");
> > + rte_errno = ENOMEM; goto fail;
> Missing newline.
I will fix it in v2.
> > + }
> > +
> > + /* 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.
I thought that initially, Two reasons why I did not do that
1) The flags have been used in fastpath, I prefer to work with flags
in fastpath so that
there is no performance impact using bitfields from the compiler _if any_.
2) In some of the places, I can simply operate on APIs like
__atomic_and_fetch() with flags.
On 2020-03-23 14:37, Jerin Jacob wrote:
>>> + }
>>> +
>>> + /* 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.
> I thought that initially, Two reasons why I did not do that
> 1) The flags have been used in fastpath, I prefer to work with flags
> in fastpath so that
Is it really that obvious that flags are faster than bitfield
operations? I think most modern architectures have machine instructions
for bitfield manipulation.
> there is no performance impact using bitfields from the compiler _if any_.
> 2) In some of the places, I can simply operate on APIs like
> __atomic_and_fetch() with flags.
I think you may still use such atomic operations. Just convert the
struct to a uint64_t, which will essentially be a no-operation, and fire
away.
static uint64_t
__rte_trace_raw(struct trace *t)
{
uint64_t raw;
memcpy(&raw, t, sizeof(struct trace));
return raw;
}
On Mon, Mar 23, 2020 at 8:13 PM Mattias Rönnblom
<mattias.ronnblom@ericsson.com> wrote:
>
> On 2020-03-23 14:37, Jerin Jacob wrote:
> >>> + }
> >>> +
> >>> + /* 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.
> > I thought that initially, Two reasons why I did not do that
> > 1) The flags have been used in fastpath, I prefer to work with flags
> > in fastpath so that
> Is it really that obvious that flags are faster than bitfield
> operations? I think most modern architectures have machine instructions
> for bitfield manipulation.
Add x86 maintainers.
There were comments in ml about bitfield inefficiency usage with x86.
http://patches.dpdk.org/patch/16482/
Search for: Bitfileds are efficient on Octeon. What's about other CPUs
you have in
mind? x86 is not as efficient.
Thoughts from x86 folks.
> > there is no performance impact using bitfields from the compiler _if any_.
> > 2) In some of the places, I can simply operate on APIs like
> > __atomic_and_fetch() with flags.
>
> I think you may still use such atomic operations. Just convert the
> struct to a uint64_t, which will essentially be a no-operation, and fire
> away.
Not sure, We think about the atomic "and" and fetch here.
That memcpy may translate additional load/store based on the compiler
optimization level.(say compiled with -O0)
>
>
> static uint64_t
>
> __rte_trace_raw(struct trace *t)
>
> {
>
> uint64_t raw;
>
> memcpy(&raw, t, sizeof(struct trace));
>
> return raw;
>
> }
>
>
On 2020-03-23 16:08, Jerin Jacob wrote:
> On Mon, Mar 23, 2020 at 8:13 PM Mattias Rönnblom
> <mattias.ronnblom@ericsson.com> wrote:
>> On 2020-03-23 14:37, Jerin Jacob wrote:
>>>>> + }
>>>>> +
>>>>> + /* 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.
>>> I thought that initially, Two reasons why I did not do that
>>> 1) The flags have been used in fastpath, I prefer to work with flags
>>> in fastpath so that
>> Is it really that obvious that flags are faster than bitfield
>> operations? I think most modern architectures have machine instructions
>> for bitfield manipulation.
> Add x86 maintainers.
>
> There were comments in ml about bitfield inefficiency usage with x86.
>
> https://protect2.fireeye.com/v1/url?k=2bd2d3ad-7706d931-2bd29336-8631fc8bdea5-8a1bf17ed26f6ce6&q=1&e=0c620ac5-c028-44d9-a4e8-e04057940075&u=http%3A%2F%2Fpatches.dpdk.org%2Fpatch%2F16482%2F
>
> Search for: Bitfileds are efficient on Octeon. What's about other CPUs
> you have in
> mind? x86 is not as efficient.
I thought both ARM and x86 had bitfield access instructions, but it
looks like I was wrong about x86. x86_64 GCC seems to convert bitfield
read to 'shr' and 'and', just like an open-coded bitfield. Bitfield
write requires more instructions.
> Thoughts from x86 folks.
>
>>> there is no performance impact using bitfields from the compiler _if any_.
>>> 2) In some of the places, I can simply operate on APIs like
>>> __atomic_and_fetch() with flags.
>> I think you may still use such atomic operations. Just convert the
>> struct to a uint64_t, which will essentially be a no-operation, and fire
>> away.
> Not sure, We think about the atomic "and" and fetch here.
> That memcpy may translate additional load/store based on the compiler
> optimization level.(say compiled with -O0)
I would be surprised if that happened on anything but -O0. At least
modern GCC on ARM and x86_64 don't seem to add any loads or stores.
I assume you are not suggesting we should optimize for -O0.
>>
>> static uint64_t
>>
>> __rte_trace_raw(struct trace *t)
>>
>> {
>>
>> uint64_t raw;
>>
>> memcpy(&raw, t, sizeof(struct trace));
>>
>> return raw;
>>
>> }
>>
>>
On Mon, Mar 23, 2020 at 10:14 PM Mattias Rönnblom
<mattias.ronnblom@ericsson.com> wrote:
>
> On 2020-03-23 16:08, Jerin Jacob wrote:
> > On Mon, Mar 23, 2020 at 8:13 PM Mattias Rönnblom
> > <mattias.ronnblom@ericsson.com> wrote:
> >> On 2020-03-23 14:37, Jerin Jacob wrote:
> >>>>> + }
> >>>>> +
> >>>>> + /* 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.
> >>> I thought that initially, Two reasons why I did not do that
> >>> 1) The flags have been used in fastpath, I prefer to work with flags
> >>> in fastpath so that
> >> Is it really that obvious that flags are faster than bitfield
> >> operations? I think most modern architectures have machine instructions
> >> for bitfield manipulation.
> > Add x86 maintainers.
> >
> > There were comments in ml about bitfield inefficiency usage with x86.
> >
> > https://protect2.fireeye.com/v1/url?k=2bd2d3ad-7706d931-2bd29336-8631fc8bdea5-8a1bf17ed26f6ce6&q=1&e=0c620ac5-c028-44d9-a4e8-e04057940075&u=http%3A%2F%2Fpatches.dpdk.org%2Fpatch%2F16482%2F
> >
> > Search for: Bitfileds are efficient on Octeon. What's about other CPUs
> > you have in
> > mind? x86 is not as efficient.
>
>
> I thought both ARM and x86 had bitfield access instructions, but it
> looks like I was wrong about x86. x86_64 GCC seems to convert bitfield
> read to 'shr' and 'and', just like an open-coded bitfield. Bitfield
> write requires more instructions.
Yes. ARM64 has bitfield access instructions. considering x86,
it is better to avoid bitfields.
See below,
>
>
> > Thoughts from x86 folks.
> >
> >>> there is no performance impact using bitfields from the compiler _if any_.
> >>> 2) In some of the places, I can simply operate on APIs like
> >>> __atomic_and_fetch() with flags.
> >> I think you may still use such atomic operations. Just convert the
> >> struct to a uint64_t, which will essentially be a no-operation, and fire
> >> away.
> > Not sure, We think about the atomic "and" and fetch here.
> > That memcpy may translate additional load/store based on the compiler
> > optimization level.(say compiled with -O0)
>
>
> I would be surprised if that happened on anything but -O0. At least
> modern GCC on ARM and x86_64 don't seem to add any loads or stores.
>
>
> I assume you are not suggesting we should optimize for -O0.
No. I was just mentining that, we can not assume the code generation
with -O0. Anyway considering the above point, lets use flags.
@@ -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>
@@ -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
@@ -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))
+{
+ 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) {
+ 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));
+ if (tp == NULL) {
+ trace_err("fail to allocate trace point memory");
+ rte_errno = ENOMEM; goto fail;
+ }
+
+ /* 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;
+
+ 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;
+}
new file mode 100644
@@ -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 */
@@ -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
new file mode 100644
@@ -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_ */
new file mode 100644
@@ -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_ */
@@ -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',
@@ -338,4 +338,5 @@ EXPERIMENTAL {
# added in 20.05
rte_thread_getname;
+ __rte_trace_point_register;
};