On Fri, Apr 10, 2020 at 4:38 PM Jerin Jacob <jerinjac...@gmail.com> wrote: > > - I am still looking at the event record mode. > > I just wonder why we have this notion per tracepoint. > > The documentation talks about it being an attribute of the trace > > buffers, and the described behavior looks fine. > > But if we can set this mode per tracepoints, once a trace buffer is > > full, won't it be hard to figure out why some events have been caught > > and not others? > > In fastpath, I tried to avoid reading multiple control variables to > improve performance. > i.e the tracepoint(uint64_t) has all control information. So it has > given a feature to control mode per tracepoint. > I thought it may be useful. No strong option here. > > If you think, it is not usefull and creates more problems, I will > change to the following: > > 1) Remove int rte_trace_mode_set(rte_trace_t trace, enum rte_trace_mode_e > mode); > 2) Change > From: > void rte_trace_global_mode_set(enum rte_trace_mode_e mode); > to: > void rte_trace_mode_set(enum rte_trace_mode_e mode);
Yes, you can keep the information in the tracepoint descriptor and expose a single api that impacts all tracepoints. +1 for me. > > - Another comment, on the form, I can see that we talk about dataplane > > tracepoints and fastpath API. > > Is there a difference? or could everything be named in a consistent > > No. Both are the same. > > > way (the headers would have to be aligned too)? > > Personally I like the fast path. The log calls it as DP( see RTE_LOG_DP). > In order to have consistency, I will pick the DP and change it everywhere to > DP, > and header files to xxxxx_dp.h from xxxx_fp.h. I too have a preference for "fast path". Thomas ? > > - Do we really need to export the rte_trace_lib_eal_generic_* trace points? > > They seem more like examples and I don't see a usage outside of the tests. > > This generic tracepoint for any quick debug, For example, if some need to add > tracepoint to just emit string in the middle of debugging a problem, > they can use rte_trace_lib_eal_generic_str. > That's the reason for exporting it. Ok, let's leave it as is, I need to think about it. -- David Marchand
