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

[Thomas Monjalon <thomas@monjalon.net>]

16/04/2020 18:08, Jerin Jacob:
> 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.

Semantically, rte_trace.h must be the API for simple users enabling traces,
while rte_trace_point.h would be used by those adding traces.


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

Is it possible to implement it with a common helper as David suggests?