Re: [dpdk-dev] [PATCH v1 02/32] eal: define the public API for trace support

["Mattias Rönnblom" <mattias.ronnblom@ericsson.com>]

On 2020-03-23 15:29, Jerin Jacob wrote:
> On Thu, Mar 19, 2020 at 2:28 AM Mattias Rönnblom
> <mattias.ronnblom@ericsson.com> wrote:
>> On 2020-03-18 20:02, jerinj@marvell.com wrote:
>>> From: Jerin Jacob <jerinj@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?
>
If we're using "shell pattern" already, it would make sense to stick 
with this.
>>> + *
>>> + * @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
>
Sounds like an improvement to me.

>
>>> +/**
>>> + * @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
>>> +}
>>> +