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.
>
> #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
