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

[David Marchand]

On Tue, Apr 21, 2020 at 3:47 PM Jerin Jacob <jerinjacobk at 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.

-- 
David Marchand