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

[David Marchand <david.marchand@redhat.com>]

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.


-- 
David Marchand