[v6,05/33] eal/trace: implement trace operation APIs
Checks
Commit Message
From: Jerin Jacob <jerinj@marvell.com>
This patch implements the following public trace APIs.
- rte_trace_is_enabled()
- rte_trace_mode_get()
- rte_trace_mode_set()
- rte_trace_pattern()
- rte_trace_point_disable()
- rte_trace_point_enable()
- rte_trace_point_is_enabled()
- rte_trace_point_lookup()
- rte_trace_regexp()
Signed-off-by: Jerin Jacob <jerinj@marvell.com>
Signed-off-by: Sunil Kumar Kori <skori@marvell.com>
---
lib/librte_eal/common/eal_common_trace.c | 147 +++++++++++++++++++++++
lib/librte_eal/common/eal_trace.h | 10 ++
lib/librte_eal/rte_eal_version.map | 9 ++
3 files changed, 166 insertions(+)
Comments
On Sun, Apr 19, 2020 at 12:02 PM <jerinj@marvell.com> wrote:
> diff --git a/lib/librte_eal/common/eal_common_trace.c b/lib/librte_eal/common/eal_common_trace.c
> index 5c5cbd2a1..1ca702f68 100644
> --- a/lib/librte_eal/common/eal_common_trace.c
> +++ b/lib/librte_eal/common/eal_common_trace.c
> @@ -3,7 +3,9 @@
> */
>
> #include <inttypes.h>
> +#include <fnmatch.h>
> #include <sys/queue.h>
> +#include <regex.h>
Alphabetical sort when possible.
>
> #include <rte_common.h>
> #include <rte_errno.h>
> @@ -20,6 +22,151 @@ static RTE_DEFINE_PER_LCORE(int, ctf_count);
> static struct trace_point_head tp_list = STAILQ_HEAD_INITIALIZER(tp_list);
> static struct trace trace;
>
> +bool
> +rte_trace_is_enabled(void)
> +{
> + return trace.status;
> +}
> +
> +static void
> +trace_mode_set(rte_trace_point_t *trace, enum rte_trace_mode mode)
> +{
> + if (mode == RTE_TRACE_MODE_OVERWRITE)
> + __atomic_and_fetch(trace, ~__RTE_TRACE_FIELD_ENABLE_DISCARD,
> + __ATOMIC_RELEASE);
> + else
> + __atomic_or_fetch(trace, __RTE_TRACE_FIELD_ENABLE_DISCARD,
> + __ATOMIC_RELEASE);
> +}
> +
> +void
> +rte_trace_mode_set(enum rte_trace_mode mode)
> +{
> + struct trace_point *tp;
> +
> + if (rte_trace_is_enabled() == false)
> + return;
rte_trace_is_enabled() returns a boolean, no need to test its value, should be:
if (!rte_trace_is_enabled())
> +
> + STAILQ_FOREACH(tp, &tp_list, next)
> + trace_mode_set(tp->handle, mode);
> +
> + trace.mode = mode;
> +}
> +
> +enum
> +rte_trace_mode rte_trace_mode_get(void)
> +{
> + return trace.mode;
> +}
> +
> +static bool
> +trace_point_is_invalid(rte_trace_point_t *t)
> +{
> + if (t == NULL)
> + return false;
Should be "return true"?
Or maybe simply rewrite as:
static bool
trace_point_is_invalid(rte_trace_point_t *t)
{
return (t == NULL) || (trace_id_get(t) >= trace.nb_trace_points);
}
> +
> + if (trace_id_get(t) >= trace.nb_trace_points)
> + return true;
> +
> + return false;
> +}
> +
> +bool
> +rte_trace_point_is_enabled(rte_trace_point_t *trace)
> +{
> + uint64_t val;
> +
> + if (trace_point_is_invalid(trace))
> + return false;
> +
> + val = __atomic_load_n(trace, __ATOMIC_ACQUIRE);
> + return val & __RTE_TRACE_FIELD_ENABLE_MASK;
We return a boolean, should be
return (val & __RTE_TRACE_FIELD_ENABLE_MASK) != 0;
> +}
> +
> +int
> +rte_trace_point_enable(rte_trace_point_t *trace)
> +{
> + if (trace_point_is_invalid(trace))
> + return -ERANGE;
> +
> + __atomic_or_fetch(trace, __RTE_TRACE_FIELD_ENABLE_MASK,
> + __ATOMIC_RELEASE);
> + return 0;
> +}
> +
> +int
> +rte_trace_point_disable(rte_trace_point_t *trace)
> +{
> + if (trace_point_is_invalid(trace))
> + return -ERANGE;
> +
> + __atomic_and_fetch(trace, ~__RTE_TRACE_FIELD_ENABLE_MASK,
> + __ATOMIC_RELEASE);
> + return 0;
> +}
For both rte_trace_point_enable/disable, we only check tracepoint validity.
Can't we just return a boolean, do we expect many error codes?
> +
> +int
> +rte_trace_pattern(const char *pattern, bool enable)
> +{
> + struct trace_point *tp;
> + int rc = 0, found = 0;
> +
> + STAILQ_FOREACH(tp, &tp_list, next) {
> + if (fnmatch(pattern, tp->name, 0) == 0) {
> + if (enable)
> + rc = rte_trace_point_enable(tp->handle);
> + else
> + rc = rte_trace_point_disable(tp->handle);
> + found = 1;
> + }
> + if (rc < 0)
> + return rc;
> + }
> +
> + return rc | found;
> +}
> +
> +int
> +rte_trace_regexp(const char *regex, bool enable)
> +{
> + struct trace_point *tp;
> + int rc = 0, found = 0;
> + regex_t r;
> +
> + if (regcomp(&r, regex, 0) != 0)
> + return -EINVAL;
> +
> + STAILQ_FOREACH(tp, &tp_list, next) {
> + if (regexec(&r, tp->name, 0, NULL, 0) == 0) {
> + if (enable)
> + rc = rte_trace_point_enable(tp->handle);
> + else
> + rc = rte_trace_point_disable(tp->handle);
> + found = 1;
> + }
> + if (rc < 0)
> + return rc;
> + }
> + regfree(&r);
> +
> + return rc | found;
> +}
For both rte_trace_pattern/rte_trace_regexp, tp is a member of tp_list.
It means this tracepoint went through proper registration.
Hence, checking for a return value from rte_trace_point_(en|dis)able
is unneeded.
On Tue, Apr 21, 2020 at 6:19 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Sun, Apr 19, 2020 at 12:02 PM <jerinj@marvell.com> wrote:
> > diff --git a/lib/librte_eal/common/eal_common_trace.c b/lib/librte_eal/common/eal_common_trace.c
> > index 5c5cbd2a1..1ca702f68 100644
> > --- a/lib/librte_eal/common/eal_common_trace.c
> > +++ b/lib/librte_eal/common/eal_common_trace.c
> > @@ -3,7 +3,9 @@
> > */
> >
> > #include <inttypes.h>
> > +#include <fnmatch.h>
> > #include <sys/queue.h>
> > +#include <regex.h>
>
> Alphabetical sort when possible.
Ack. Wil fix it v7
>
> >
> > #include <rte_common.h>
> > #include <rte_errno.h>
> > @@ -20,6 +22,151 @@ static RTE_DEFINE_PER_LCORE(int, ctf_count);
> > static struct trace_point_head tp_list = STAILQ_HEAD_INITIALIZER(tp_list);
> > static struct trace trace;
> >
> > +bool
> > +rte_trace_is_enabled(void)
> > +{
> > + return trace.status;
> > +}
> > +
> > +static void
> > +trace_mode_set(rte_trace_point_t *trace, enum rte_trace_mode mode)
> > +{
> > + if (mode == RTE_TRACE_MODE_OVERWRITE)
> > + __atomic_and_fetch(trace, ~__RTE_TRACE_FIELD_ENABLE_DISCARD,
> > + __ATOMIC_RELEASE);
> > + else
> > + __atomic_or_fetch(trace, __RTE_TRACE_FIELD_ENABLE_DISCARD,
> > + __ATOMIC_RELEASE);
> > +}
> > +
> > +void
> > +rte_trace_mode_set(enum rte_trace_mode mode)
> > +{
> > + struct trace_point *tp;
> > +
> > + if (rte_trace_is_enabled() == false)
> > + return;
>
> rte_trace_is_enabled() returns a boolean, no need to test its value, should be:
> if (!rte_trace_is_enabled())
I like the ! scheme. I thought, DPDK community like == false scheme.
I will change it in v7.
>
>
> > +
> > + STAILQ_FOREACH(tp, &tp_list, next)
> > + trace_mode_set(tp->handle, mode);
> > +
> > + trace.mode = mode;
> > +}
> > +
> > +enum
> > +rte_trace_mode rte_trace_mode_get(void)
> > +{
> > + return trace.mode;
> > +}
> > +
> > +static bool
> > +trace_point_is_invalid(rte_trace_point_t *t)
> > +{
> > + if (t == NULL)
> > + return false;
>
> Should be "return true"?
>
> Or maybe simply rewrite as:
>
> static bool
> trace_point_is_invalid(rte_trace_point_t *t)
> {
> return (t == NULL) || (trace_id_get(t) >= trace.nb_trace_points);
> }
Ack.
>
> > +
> > + if (trace_id_get(t) >= trace.nb_trace_points)
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > +bool
> > +rte_trace_point_is_enabled(rte_trace_point_t *trace)
> > +{
> > + uint64_t val;
> > +
> > + if (trace_point_is_invalid(trace))
> > + return false;
> > +
> > + val = __atomic_load_n(trace, __ATOMIC_ACQUIRE);
> > + return val & __RTE_TRACE_FIELD_ENABLE_MASK;
>
> We return a boolean, should be
> return (val & __RTE_TRACE_FIELD_ENABLE_MASK) != 0;
Ack.
>
>
> > +}
> > +
> > +int
> > +rte_trace_point_enable(rte_trace_point_t *trace)
> > +{
> > + if (trace_point_is_invalid(trace))
> > + return -ERANGE;
> > +
> > + __atomic_or_fetch(trace, __RTE_TRACE_FIELD_ENABLE_MASK,
> > + __ATOMIC_RELEASE);
> > + return 0;
> > +}
> > +
> > +int
> > +rte_trace_point_disable(rte_trace_point_t *trace)
> > +{
> > + if (trace_point_is_invalid(trace))
> > + return -ERANGE;
> > +
> > + __atomic_and_fetch(trace, ~__RTE_TRACE_FIELD_ENABLE_MASK,
> > + __ATOMIC_RELEASE);
> > + return 0;
> > +}
>
> For both rte_trace_point_enable/disable, we only check tracepoint validity.
> Can't we just return a boolean, do we expect many error codes?
I think, it is better to return "int" so that we may not ABI the
change in future.
>
>
> > +
> > +int
> > +rte_trace_pattern(const char *pattern, bool enable)
> > +{
> > + struct trace_point *tp;
> > + int rc = 0, found = 0;
> > +
> > + STAILQ_FOREACH(tp, &tp_list, next) {
> > + if (fnmatch(pattern, tp->name, 0) == 0) {
> > + if (enable)
> > + rc = rte_trace_point_enable(tp->handle);
> > + else
> > + rc = rte_trace_point_disable(tp->handle);
> > + found = 1;
> > + }
> > + if (rc < 0)
> > + return rc;
> > + }
> > +
> > + return rc | found;
> > +}
> > +
> > +int
> > +rte_trace_regexp(const char *regex, bool enable)
> > +{
> > + struct trace_point *tp;
> > + int rc = 0, found = 0;
> > + regex_t r;
> > +
> > + if (regcomp(&r, regex, 0) != 0)
> > + return -EINVAL;
> > +
> > + STAILQ_FOREACH(tp, &tp_list, next) {
> > + if (regexec(&r, tp->name, 0, NULL, 0) == 0) {
> > + if (enable)
> > + rc = rte_trace_point_enable(tp->handle);
> > + else
> > + rc = rte_trace_point_disable(tp->handle);
> > + found = 1;
> > + }
> > + if (rc < 0)
> > + return rc;
> > + }
> > + regfree(&r);
> > +
> > + return rc | found;
> > +}
>
> For both rte_trace_pattern/rte_trace_regexp, tp is a member of tp_list.
> It means this tracepoint went through proper registration.
> Hence, checking for a return value from rte_trace_point_(en|dis)able
> is unneeded.
Yes. I thought of it, But I think, it is safe to check the return code
as it absorbs any
future rte_trace_point_enable() changes. It slow-path code, so it OK IMO.
>
>
> --
> David Marchand
>
On Tue, Apr 21, 2020 at 3:47 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > @@ -20,6 +22,151 @@ static RTE_DEFINE_PER_LCORE(int, ctf_count);
> > > static struct trace_point_head tp_list = STAILQ_HEAD_INITIALIZER(tp_list);
> > > static struct trace trace;
> > >
> > > +bool
> > > +rte_trace_is_enabled(void)
> > > +{
> > > + return trace.status;
> > > +}
> > > +
> > > +static void
> > > +trace_mode_set(rte_trace_point_t *trace, enum rte_trace_mode mode)
> > > +{
> > > + if (mode == RTE_TRACE_MODE_OVERWRITE)
> > > + __atomic_and_fetch(trace, ~__RTE_TRACE_FIELD_ENABLE_DISCARD,
> > > + __ATOMIC_RELEASE);
> > > + else
> > > + __atomic_or_fetch(trace, __RTE_TRACE_FIELD_ENABLE_DISCARD,
> > > + __ATOMIC_RELEASE);
> > > +}
> > > +
> > > +void
> > > +rte_trace_mode_set(enum rte_trace_mode mode)
> > > +{
> > > + struct trace_point *tp;
> > > +
> > > + if (rte_trace_is_enabled() == false)
> > > + return;
> >
> > rte_trace_is_enabled() returns a boolean, no need to test its value, should be:
> > if (!rte_trace_is_enabled())
>
> I like the ! scheme. I thought, DPDK community like == false scheme.
> I will change it in v7.
Not obvious, but I understand this part as talking about the boolean case:
https://doc.dpdk.org/guides/contributing/coding_style.html#null-pointers
"Do not use ! for tests unless it is a boolean, for example, use:"
> > > +int
> > > +rte_trace_point_enable(rte_trace_point_t *trace)
> > > +{
> > > + if (trace_point_is_invalid(trace))
> > > + return -ERANGE;
> > > +
> > > + __atomic_or_fetch(trace, __RTE_TRACE_FIELD_ENABLE_MASK,
> > > + __ATOMIC_RELEASE);
> > > + return 0;
> > > +}
> > > +
> > > +int
> > > +rte_trace_point_disable(rte_trace_point_t *trace)
> > > +{
> > > + if (trace_point_is_invalid(trace))
> > > + return -ERANGE;
> > > +
> > > + __atomic_and_fetch(trace, ~__RTE_TRACE_FIELD_ENABLE_MASK,
> > > + __ATOMIC_RELEASE);
> > > + return 0;
> > > +}
> >
> > For both rte_trace_point_enable/disable, we only check tracepoint validity.
> > Can't we just return a boolean, do we expect many error codes?
>
> I think, it is better to return "int" so that we may not ABI the
> change in future.
Ok, fair enough.
> > > +
> > > +int
> > > +rte_trace_pattern(const char *pattern, bool enable)
> > > +{
> > > + struct trace_point *tp;
> > > + int rc = 0, found = 0;
> > > +
> > > + STAILQ_FOREACH(tp, &tp_list, next) {
> > > + if (fnmatch(pattern, tp->name, 0) == 0) {
> > > + if (enable)
> > > + rc = rte_trace_point_enable(tp->handle);
> > > + else
> > > + rc = rte_trace_point_disable(tp->handle);
> > > + found = 1;
> > > + }
> > > + if (rc < 0)
> > > + return rc;
> > > + }
> > > +
> > > + return rc | found;
> > > +}
> > > +
> > > +int
> > > +rte_trace_regexp(const char *regex, bool enable)
> > > +{
> > > + struct trace_point *tp;
> > > + int rc = 0, found = 0;
> > > + regex_t r;
> > > +
> > > + if (regcomp(&r, regex, 0) != 0)
> > > + return -EINVAL;
> > > +
> > > + STAILQ_FOREACH(tp, &tp_list, next) {
> > > + if (regexec(&r, tp->name, 0, NULL, 0) == 0) {
> > > + if (enable)
> > > + rc = rte_trace_point_enable(tp->handle);
> > > + else
> > > + rc = rte_trace_point_disable(tp->handle);
> > > + found = 1;
> > > + }
> > > + if (rc < 0)
> > > + return rc;
> > > + }
> > > + regfree(&r);
> > > +
> > > + return rc | found;
> > > +}
> >
> > For both rte_trace_pattern/rte_trace_regexp, tp is a member of tp_list.
> > It means this tracepoint went through proper registration.
> > Hence, checking for a return value from rte_trace_point_(en|dis)able
> > is unneeded.
>
> Yes. I thought of it, But I think, it is safe to check the return code
> as it absorbs any
> future rte_trace_point_enable() changes. It slow-path code, so it OK IMO.
Would be odd to predict what happened if we broke for this loop on one
tracepoint error, but ok to leave as is.
@@ -3,7 +3,9 @@
*/
#include <inttypes.h>
+#include <fnmatch.h>
#include <sys/queue.h>
+#include <regex.h>
#include <rte_common.h>
#include <rte_errno.h>
@@ -20,6 +22,151 @@ static RTE_DEFINE_PER_LCORE(int, ctf_count);
static struct trace_point_head tp_list = STAILQ_HEAD_INITIALIZER(tp_list);
static struct trace trace;
+bool
+rte_trace_is_enabled(void)
+{
+ return trace.status;
+}
+
+static void
+trace_mode_set(rte_trace_point_t *trace, enum rte_trace_mode mode)
+{
+ if (mode == RTE_TRACE_MODE_OVERWRITE)
+ __atomic_and_fetch(trace, ~__RTE_TRACE_FIELD_ENABLE_DISCARD,
+ __ATOMIC_RELEASE);
+ else
+ __atomic_or_fetch(trace, __RTE_TRACE_FIELD_ENABLE_DISCARD,
+ __ATOMIC_RELEASE);
+}
+
+void
+rte_trace_mode_set(enum rte_trace_mode mode)
+{
+ struct trace_point *tp;
+
+ if (rte_trace_is_enabled() == false)
+ return;
+
+ STAILQ_FOREACH(tp, &tp_list, next)
+ trace_mode_set(tp->handle, mode);
+
+ trace.mode = mode;
+}
+
+enum
+rte_trace_mode rte_trace_mode_get(void)
+{
+ return trace.mode;
+}
+
+static bool
+trace_point_is_invalid(rte_trace_point_t *t)
+{
+ if (t == NULL)
+ return false;
+
+ if (trace_id_get(t) >= trace.nb_trace_points)
+ return true;
+
+ return false;
+}
+
+bool
+rte_trace_point_is_enabled(rte_trace_point_t *trace)
+{
+ uint64_t val;
+
+ if (trace_point_is_invalid(trace))
+ return false;
+
+ val = __atomic_load_n(trace, __ATOMIC_ACQUIRE);
+ return val & __RTE_TRACE_FIELD_ENABLE_MASK;
+}
+
+int
+rte_trace_point_enable(rte_trace_point_t *trace)
+{
+ if (trace_point_is_invalid(trace))
+ return -ERANGE;
+
+ __atomic_or_fetch(trace, __RTE_TRACE_FIELD_ENABLE_MASK,
+ __ATOMIC_RELEASE);
+ return 0;
+}
+
+int
+rte_trace_point_disable(rte_trace_point_t *trace)
+{
+ if (trace_point_is_invalid(trace))
+ return -ERANGE;
+
+ __atomic_and_fetch(trace, ~__RTE_TRACE_FIELD_ENABLE_MASK,
+ __ATOMIC_RELEASE);
+ return 0;
+}
+
+int
+rte_trace_pattern(const char *pattern, bool enable)
+{
+ struct trace_point *tp;
+ int rc = 0, found = 0;
+
+ STAILQ_FOREACH(tp, &tp_list, next) {
+ if (fnmatch(pattern, tp->name, 0) == 0) {
+ if (enable)
+ rc = rte_trace_point_enable(tp->handle);
+ else
+ rc = rte_trace_point_disable(tp->handle);
+ found = 1;
+ }
+ if (rc < 0)
+ return rc;
+ }
+
+ return rc | found;
+}
+
+int
+rte_trace_regexp(const char *regex, bool enable)
+{
+ struct trace_point *tp;
+ int rc = 0, found = 0;
+ regex_t r;
+
+ if (regcomp(&r, regex, 0) != 0)
+ return -EINVAL;
+
+ STAILQ_FOREACH(tp, &tp_list, next) {
+ if (regexec(&r, tp->name, 0, NULL, 0) == 0) {
+ if (enable)
+ rc = rte_trace_point_enable(tp->handle);
+ else
+ rc = rte_trace_point_disable(tp->handle);
+ found = 1;
+ }
+ if (rc < 0)
+ return rc;
+ }
+ regfree(&r);
+
+ return rc | found;
+}
+
+rte_trace_point_t *
+rte_trace_point_lookup(const char *name)
+{
+ struct trace_point *tp;
+
+ if (name == NULL)
+ return NULL;
+
+ STAILQ_FOREACH(tp, &tp_list, next)
+ if (strncmp(tp->name, name, TRACE_POINT_NAME_SIZE) == 0)
+ return tp->handle;
+
+ return NULL;
+}
+
int
__rte_trace_point_register(rte_trace_point_t *handle, const char *name,
void (*register_fn)(void))
@@ -28,9 +28,19 @@ struct trace_point {
struct trace {
int register_errno;
+ bool status;
+ enum rte_trace_mode mode;
uint32_t nb_trace_points;
};
+/* Helper functions */
+static inline uint16_t
+trace_id_get(rte_trace_point_t *trace)
+{
+ return (*trace & __RTE_TRACE_FIELD_ID_MASK) >>
+ __RTE_TRACE_FIELD_ID_SHIFT;
+}
+
/* Trace point list functions */
STAILQ_HEAD(trace_point_head, trace_point);
@@ -341,4 +341,13 @@ EXPERIMENTAL {
per_lcore_trace_point_sz;
rte_log_can_log;
rte_thread_getname;
+ rte_trace_is_enabled;
+ rte_trace_mode_get;
+ rte_trace_mode_set;
+ rte_trace_pattern;
+ rte_trace_point_disable;
+ rte_trace_point_enable;
+ rte_trace_point_is_enabled;
+ rte_trace_point_lookup;
+ rte_trace_regexp;
};