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

[Jerin Jacob <jerinjacobk@gmail.com>]

On Thu, Apr 16, 2020 at 7:09 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> 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...

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)

I will summarise the public API and file name details. Let's finalize.

# rte_trace.h will have

rte_trace_global_is_enabled
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


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

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


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

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

 The reason for rte_tracepoint_emit_(u|i)(8|16|32|64) to get compile
to check to correct time type used.
See:
rte_trace_point_emit_u32 defintion has
RTE_BUILD_BUG_ON(sizeof(type) != sizeof(typeof(uint32_t)));


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

OK. I will remove is_disabled()


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

OK. I add it in git commit.


>