On Fri, Apr 17, 2020 at 1:57 PM David Marchand
<david.march...@redhat.com> wrote:
>
> Hello Jerin,
Hello David,
>
> On Thu, Apr 16, 2020 at 6:08 PM Jerin Jacob <jerinjac...@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_.
Ack.
>
>
> > 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.
Ack. I will change to rte_trace_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
>
> 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).
OK. +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.
I will make the internal. I prefer to have if (0) c style scheme so
that both sections
of the code goes through the compilation phase. aka No compilation
issue when RTE_ENABLE_TRACE_FP enabled.
We have a separate macro declaration for RTE_TRACE_POINT and
RTE_TRACE_POINT_FP for
Tracepoint selection.
>
>
> > > > # 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.
Yes, if we have rte_trace_point.h as a separate file. I will work on this. Let
me see if I can get it right.
>
> If you don't like it, let's drop this last part.
> Thanks.
Thanks for the review.
>
>
> --
> David Marchand
>
