Re: [dpdk-dev] [PATCH v6 05/33] eal/trace: implement trace operation APIs

[Jerin Jacob <jerinjacobk@gmail.com>]

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
>