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

[Jerin Jacob <jerinjacobk@gmail.com>]

On Thu, Apr 16, 2020 at 9:53 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 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.

Yes. The problem is where will be the definition of
rte_trace_fp_is_enabled() come?
It is used for both needs.


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

It is already present.

See.

#define __rte_trace_emit_datatype(in, type)\
do {\
        RTE_BUILD_BUG_ON(sizeof(type) != sizeof(typeof(in)));\
        __rte_trace_emit_ctf_field(sizeof(type), RTE_STR(in), RTE_STR(type));\
} while (0)

#define rte_trace_ctf_u64(in) __rte_trace_emit_datatype(in, uint64_t)
#define rte_trace_ctf_i64(in) __rte_trace_emit_datatype(in, int64_t)
#define rte_trace_ctf_u32(in) __rte_trace_emit_datatype(in, uint32_t)
#define rte_trace_ctf_i32(in) __rte_trace_emit_datatype(in, int32_t)
#define rte_trace_ctf_u16(in) __rte_trace_emit_datatype(in, uint16_t)
#define rte_trace_ctf_i16(in) __rte_trace_emit_datatype(in, int16_t)
#define rte_trace_ctf_u8(in) __rte_trace_emit_datatype(in, uint8_t)
#define rte_trace_ctf_i8(in) __rte_trace_emit_datatype(in, int8_t)
#define rte_trace_ctf_int(in) __rte_trace_emit_datatype(in, int32_t)
#define rte_trace_ctf_long(in) __rte_trace_emit_datatype(in, long)
#define rte_trace_ctf_float(in) __rte_trace_emit_datatype(in, float)
#define rte_trace_ctf_double(in) __rte_trace_emit_datatype(in, double)
#define rte_trace_ctf_ptr(in) __rte_trace_emit_datatype(in, uintptr_t)

>
>
>