Re: [dpdk-dev] [PATCH v5 00/33] DPDK Trace support

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

On Wed, Apr 15, 2020 at 4:40 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > - What do you think of splitting the API in two headers, thinking
> > about who will use them?
> > * rte_trace.h (rte_trace_ prefix for all functions/macros/types) for
> > users of the trace framework that want to
> >  * get the status of the whole trace subsystem,
> >  * enable/disable tracepoints by pattern/regexp,
> >  * dump the current events,
> > * rte_tracepoint.h (rte_tracepoint_ prefix for all
> > functions/macros/types) for developers that want to add tracepoints to
> > their code
>
> # Initially, I thought of doing the same.
> Later realized that some of the definitions such as following
>
> 1)
> /** The trace object. The trace APIs are based on this opaque object. */
> typedef uint64_t rte_trace_t;

As a user, I would ask the trace framework to enable tracepoints by
calling rte_trace_pattern()/rte_trace_regexp().
This does not require the tracepoint descriptor to be exposed in rte_trace.h.


If some application wants to store/manipulate the descriptors, then it
will rely on rte_tracepoint.h where the rte_tracepoint_t opaque object
and API are:
- rte_tracepoint_lookup (currently named rte_trace_by_name)
- rte_tracepoint_enable
- rte_tracepoint_disable
- rte_tracepoint_is_invalid (currently named rte_trace_id_is_invalid,
should be private, as discussed)
- rte_tracepoint_is_enabled
- RTE_TRACEPOINT/_FP macros
- rte_tracepoint_register etc...


>
> 2) rte_trace_fp_is_enabled()

As a user, what information would this give me?
"Some fastpath tracepoints are not available"

Moving to rte_tracepoint.h is enough to me.


> # Regarding the API change the following to rte_tracepoint_*
>
> #define rte_trace_ctf_u64(val)
> #define rte_trace_ctf_i64(val)
> #define rte_trace_ctf_u32(val)
> #define rte_trace_ctf_i32(val)
> #define rte_trace_ctf_u16(val)
> #define rte_trace_ctf_i16(val)
> #define rte_trace_ctf_u8(val)
> #define rte_trace_ctf_i8(val)
> #define rte_trace_ctf_int(val)
> #define rte_trace_ctf_long(val)
> #define rte_trace_ctf_float(val)
> #define rte_trace_ctf_double(val)
> #define rte_trace_ctf_ptr(val)
> #define rte_trace_ctf_string(val)
> It could be done. Just concerned the length of API will be more. like
> rte_trace_point_ctf_u64
> If you have a strong opinion on this then I can change it.

I don't like mentioning ctf here.

I went with a git grep -l rte_trace_ctf |xargs sed -i -e
's/rte_trace_ctf_/rte_tracepoint_emit_/g'.
If we keep one rte_tracepoint_emit_ per line in tracepoint
declarations, the length is not an issue by looking at how they are
used.

Example:
RTE_TRACEPOINT(
        rte_trace_lib_eal_intr_disable,
        RTE_TRACEPOINT_ARGS(const struct rte_intr_handle *handle, int rc),
        rte_tracepoint_emit_int(rc);
        rte_tracepoint_emit_int(handle->vfio_dev_fd);
        rte_tracepoint_emit_int(handle->fd);
        rte_tracepoint_emit_int(handle->type);
        rte_tracepoint_emit_u32(handle->max_intr);
        rte_tracepoint_emit_u32(handle->nb_efd);
)


Besides, we don't need to define all those
rte_tracepoint_emit_(u|i)(8|16|32|64) helpers in
rte_tracepoint_provider.h and rte_tracepoint_register.h.
If we define a helper rte_tracepoint_emit_data(type, in) in
rte_tracepoint.h, then the "provider" and "register" headers must only
define how to emit a header (generic and fp cases), then
rte_tracepoint_emit_data and rte_tracepoint_emit_string.


> > - Having functions "is_disabled" has little value when a "is_enabled"
> > counterpart exists.
>
> I thought so. I like to have "is_enabled". But in the code,
> "is_disabled" has been used more.
> Then finally added both in API. Please share your preference, I will
> do the same in v6.

is_enabled is enough.


> > - I did not get why we put the trace descriptors in a specific elf
> > section, can you explain the benefits?
>
> Those are global variables of size 8B. Since it is used in fastpath.
> I just want all of them to same area for
> 1) It is a mostly a read-only area. Not mix with other "write" global variables
> 2) Chances that same subsystem FP variables comes same fastpath cache line.
> In short, We will be more predictable performance number from build to build i.e
> not depended on other global variables from build to build.

Ok, thanks for the explanation.
It is worth a few words in the associated commitlog for posterity.


> > - I can see no protection on the tracepoint list. Could we have issues
> > with control/application threads that dpdk does not control, dynamic
> > loading of libraries.. ?
>
> Based on my understanding, all constructors in the .so file should be
> called in eal_plugins_init()
> one by one. So there is no synchronization issue. eal_trace_init()
> which is called after
> eal_plugins_init(). So all the .so files with constructor should be
> called in eal_plugins_init()
> be it DPDK shared lib or non DPDK shared lib.
> rte_log_register() does the similar thing.

As long as we register in constructor, this is ok.
Time will tell us if users want to use this in a different way.


> > - Following comment on v4 and the removal of the mode per tracepoint
> > api, don't we need to put the current select mode in each tracepoint
> > descriptor when registering a trace point ?
>
> RTE_TRACE_POINT_REGISTER has only trace and name.
> RTE_TRACE_POINT_REGISTER(trace, name)
>
> Not sure why we need to add "mode" in the trace register.

I thought of registers happening out of a constructor.
But as long as we are in constructors and thanks to the
eal_trace_init() after plugin load, the mode is set in all
descriptors.
So we are fine.


-- 
David Marchand