On Tue, Apr 21, 2020 at 6:19 PM David Marchand
<david.march...@redhat.com> wrote:
>
> On Sun, Apr 19, 2020 at 12:02 PM <jer...@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
>
