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

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

Hello Jerin,

On Thu, Apr 16, 2020 at 6:08 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> From the prototype onwards, Myself shuffled abound multiple times on
> the API name to satisfying
> names.
>
> If you would like to classify based on the tracepoint object
> dependency to a new header file, it is fine.
> Let's go the last round for API naming details.
>
> I think, trace being the domain, IMO, it better to call the trace
> point API with rte_trace_point_*
> and trace point object to rte_trace_point_t (vs rte_tracepoint_t)

Ok, let's go with rte_trace_point_.


> I will summarise the public API and file name details. Let's finalize.
>
> # rte_trace.h will have
>
> rte_trace_global_is_enabled

global_ does not make sense anymore.


> rte_trace_mode_set
> rte_trace_mode_get
> rte_trace_pattern
> rte_trace_regexp
> rte_trace_save
> rte_trace_metadata_dump
> rte_trace_dump
>
> # rte_trace_point.h will have all operation related to rte_trace_point_t object
>
> # rte_trace_provider.h renamed rte_trace_point_provider.h
> # rte_trace_register.h renamed to rte_trace_point_register.h
> # rte_trace_eal.h renamed to rte_trace_point_eal.h

Ok.


> > > 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.
>
> IMO, semantically not correct as we are splitting based on some definition.

What is rte_trace_fp_is_enabled() supposed to do?
"Test if the trace datapath compile-time option is enabled."

One implication is that dpdk must be compiled with RTE_ENABLE_TRACE_FP
enabled for users to make use of fp tracepoints later.
It would impose restrictions to final users when they did not compile
the dpdk package themselves.


>
> How about,
> 1) Not expose this API
> OR
> 2) rte_trace_point.h includes the rte_trace.h
>
>

My vote is 1).

On how to do without it, this should be enough in rte_trace_point_provider.h:

#ifdef RTE_ENABLE_TRACE_FP
#define __rte_trace_emit_header_fp(t) \
do { \
        RTE_SET_USED(t); \
        return; \
} while (0)
#else
#define __rte_trace_emit_header_fp(t) \
        __rte_trace_emit_header(t) \
#endif

This way, the user can choose where he enables fp tracepoints in his
code by placing a #undef/#define RTE_ENABLE_TRACE_FP before including
the tracepoints headers.


> > > # 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.
>
> OK to remove ctf to make it as rte_trace_point_emit_*. OK?

Ok.


>
> >
> > 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);
> > )
> >
> >

Reading again what I wrote.

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

This part still stands.

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

But this part is inconsistent, I will blame my son (EINTR/EAGAIN).

I meant: we can have all per-type helpers in rte_trace_point.h.

rte_trace_point_register.h and rte_trace_point_provider.h both define
a macro (with a common signature) rte_trace_point_emit_data(type, in).

This way, the rte_trace_point_emit_data implementation in
rte_trace_point_register.h can do the type checking.
rte_trace_point_provider.h macro just ignores the type argument.

If you don't like it, let's drop this last part.
Thanks.


-- 
David Marchand