On Thu, Mar 19, 2020 at 2:28 AM Mattias Rönnblom
<mattias.ronnb...@ericsson.com> wrote:
>
> On 2020-03-18 20:02, jer...@marvell.com wrote:
> > From: Jerin Jacob <jer...@marvell.com>
> > +
> > +#include <rte_common.h>
> > +#include <rte_compat.h>
> > +
> > +/** The trace object. The trace APIs are based on this opaque object. */
> > +typedef uint64_t *rte_trace_t;
>
> Wouldn't a forward-declared struct, with the definition hidden from the
> user, be more appropriate? As a bonus, you'll get some type checking.
>
> "struct rte_trace;" here and "struct rte_trace*" in all the APIs.
> "struct rte_trace { uint64_t val; }; in the implementation. Or just cast
> it to a uint64_t *.
OK. I will remove the typedef then.
>
> typdef:ing pointers is generally considered a no-no, at least if you
> follow the Linux kernel coding conventions.
>
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Enumerate trace mode operation.
> > + */
> > +enum rte_trace_mode_e {
> > + /**
> > + * In this mode, When no space left in trace buffer, the subsequent
> > + * events overwrite the old events in the trace buffer.
> > + */
> > + RTE_TRACE_MODE_OVERWRITE,
> > + /**
> > + * In this mode, When no space left on trace buffer, the subsequent
> > + * events shall not be recorded in the trace buffer.
> > + */
> > + RTE_TRACE_MODE_DISCARD,
> > +};
>
> Have you considered having a blocking mode as well, where the thread
> will just wait for space to be freed?
The trace buffer is per thread. So there is no waiting.
The new features can be added later as needed by extending the mode.
>
> Remove the "_e" from the name. "enum" already tells us it's an enumeration.
OK. I will remove it in v2.
>
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Test if global trace is enabled.
> > + *
> > + * @return
> > + * true if global trace is enabled, false otherwise.
> > + */
> > +__rte_experimental
> > +bool rte_trace_global_is_enabled(void);
>
> My impression is that DPDK does:
>
>
> __rte_experimental bool
>
> rte_trace_global_is_enabled(void);
>
>
> Now when I check the coding conventions, that's only for function
> definition. Why declaration and definition should be different, I don't
> know.
I see two patterns.(Both cases __rte_experimental in a new line)
__rte_experimental
bool
rte_trace_global_is_enabled(void);
or
__rte_experimental
bool rte_trace_global_is_enabled(void);
For the prototype case, I prefer a later option and for definition
case the first option.
If there are no specific opinions, I would like to stick to this model
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Set the global trace level.
> > + *
> > + * After this call, trace with a level lower or equal than the level
> > + * passed as argument will be captured in the trace buffer.
> > + *
> > + * @param level
> > + * Trace level. A value between RTE_LOG_EMERG (1) and RTE_LOG_DEBUG (8).
> > + */
> > +__rte_experimental
> > +void rte_trace_global_level_set(uint32_t level);
>
> uint32_t means a lot of levels.
Yes. I did this to make compatibly with rte_log level datatype.
> > + *
> > + * @param trace
> > + * The trace object.
> > + * @return
> > + * - Zero or positive: Mode encoded as enum rte_trace_mode_e.
> > + * - (-EINVAL): Trace object is not registered.
> > + */
> > +__rte_experimental
> > +int rte_trace_mode_get(rte_trace_t trace);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Enable/Disable a set of tracepoints based on shell pattern.
> Shell pattern means what I think is usually referred to as a glob?
According, "man 3 fnmatch", it can be both.
No preference here, The rte_log is using shell pattern as the comment
so I thought of
using the same.
Thoughts?
> > + *
> > + * @param pattern
> > + * The match pattern identifying the tracepoint.
> > + * @param enable
> > + * true to enable tracepoint, false to disable the tracepoint, upon
> > match.
> > + * @param[out] found
> > + * NULL value allowed, if not NULL, true if match found, false
> > otherwise.
> > + * @return
> > + * - 0: Success.
> > + * - (-ERANGE): Trace object is not registered.
> > + */
> > +__rte_experimental
> > +int rte_trace_pattern(const char *pattern, bool enable, bool *found);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Enable/Disable a set of tracepoints based on regular expression.
> > + *
> > + * @param regex
> > + * A regular expression identifying the tracepoint.
> > + * @param enable
> > + * true to enable tracepoint, false to disable the tracepoint, upon
> > match.
> > + * @param[out] found
> > + * NULL value allowed, if not NULL, true if match found, false
> > otherwise.
> What's the reason of having this output parameter, as opposed to coding
> the information into the return code?
I thought making explicit is good . No strong opinion here.
How about?
0: Success but no pattern found
>0: Sucess and pattern found
<0: error
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Search a trace object from its name.
> > + *
> > + * @param name
> > + * The name of the tracepoint.
> > + * @return
> > + * The tracepoint object or NULL if not found.
> > + */
> > +__rte_experimental
> > +rte_trace_t rte_trace_from_name(const char *name);
>
> Would "rte_trace_by_name" be a better name?
No strong preference. I will change to rte_trace_by_name().
>
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Dump the trace metadata to a file.
> > + *
> > + * @param f
> > + * A pointer to a file for output
> > + * @return
> > + * - 0: Success.
> > + * - <0 : Failure.
> > + */
> > +__rte_experimental
> > +int rte_trace_metadata_dump(FILE *f);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + * Dump the trace subsystem status to a file.
> > + *
> > + * @param f
> > + * A pointer to a file for output
> > + */
> > +__rte_experimental
> > +void rte_trace_dump(FILE *f);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Test if the trace datapath compile-time option is enabled.
> > + *
> > + * @return
> > + * A positive value if trace datapath enabled, value zero otherwise.
> Switch to a bool return type.
I thought of avoiding "bool" in fastpath, I will change to bool if
there is no performance
impact.
> > + */
> > +static __rte_always_inline int
> > +rte_trace_is_dp_enabled(void)
> > +{
> > +#ifdef RTE_ENABLE_TRACE_DP
> > + return RTE_ENABLE_TRACE_DP;
> > +#else
> > + return 0;
> > +#endif
> > +}
> > +
