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