On Wed, Apr 1, 2020 at 1:49 PM David Marchand <david.march...@redhat.com> wrote: > > Hello Jerin,
Hello David, Thanks for the review. > > On Sun, Mar 29, 2020 at 4:43 PM <jer...@marvell.com> wrote: > > Items that needs to be sort it out > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > # Makefile and meson.build are updated to allow experimental APIs. > > > > As multiple EXPERIMENTAL symbols, exported by trace library, are > > used in various drivers, lib, app and examples. So to fix compilation > > warning/error, Makefile and meson.build are updated for all required > > components to support EXPERIMENTAL APIs. > > It results same code changes at multiple components as well as > > increases source code line changes in patchset too. > > Suggestions are welcome to resolve this issue with lesser code changes. > > - Regardless of the trace framework, the ALLOW_EXPERIMENTAL_API flag > gates access to APIs so that external users are aware of their status. > I have been considering setting this flag unconditionally for internal > users in the top Makefile/meson for app/ lib/ and drivers/. > I could look at this and prepare a patch about this, but this is not > enough here. It makes sense to me. Let me know when you are planning to send that patch, I will rebase this series on top of that. If you don't have time then I can send the patch too. I assume the patch content will be: 1) Removing experimental API from app, lib, drivers, examples with make and meson 2) Have it enabled at the global level. > With the trace framework, we add experimental symbols in mempool or > ethdev inlines, which then inherit the experimental check. > This would break compilation of external code. Users are then forced > to enable experimental API. I agree with the analysis. > > How about: > * we introduce a global config switch that enables/disables the trace > framework (off by default): the people who want to test it and help > stabilize it will have to deal with the experimental flag for now, > * in 20.11, the trace points are put into the stable ABI, and the > option is removed, IMO, the better alternative would be: Since the trace changes in the "inline" functions of the specific library already disabled under _compile time_ RTE_ENABLE_TRACE_DP flag and it is using RTE_TRACE_POINT_DP() to define the trace unlike slow path trace like RTE_TRACE_POINT(). So I can improve RTE_TRACE_POINT_DP() to make absolute NOP if ALLOW_EXPERIMENTAL_API not defined. On the upside, The tracing code will be enabled by default(runtime it is disabled by default anyway). If some need to fastpath API tracing then ALLOW_EXPERIMENTAL_API need to enable. So this won't break applications. > > > - With the patchset rebased on the current master (could be my fault, > so take it with a grain of salt), the ALLOW_EXPERIMENTAL_API flag is > not passed when compiling the l3fwd example against an installed dpdk. I will check. We have added ALLOW_EXPERIMENTAL_API flag where we got compilation issues. > > > - On the MAINTAINERS update: > > Trace > M: Jerin Jacob <jer...@marvell.com> > M: Sunil Kumar Kori <sk...@marvell.com> > F: lib/librte_eal/include/rte_trace*.h > F: lib/librte_eal/common/eal_common_trace*.c > F: lib/librte_eal/common/eal_trace.h > F: lib/librte_ethdev/ethdev_trace_points.c > F: lib/librte_ethdev/rte_trace_ethdev*.h > F: lib/librte_eventdev/eventdev_trace_points.c > F: lib/librte_eventdev/rte_trace_eventdev*.h > F: lib/librte_cryptodev/cryptodev_trace_points.c > F: lib/librte_cryptodev/rte_trace_cryptodev*.h > F: lib/librte_mempool/mempool_trace_points.c > F: lib/librte_mempool/rte_trace_mempool*.h > > Once we exclude the trace framework, the trace points themselves > should be the responsibility of the component (ethdev, eventdev...) > maintainers (copied them). Yes. I will remove the following from the MAINTAINERS update in the next version. F: lib/librte_ethdev/ethdev_trace_points.c F: lib/librte_ethdev/rte_trace_ethdev*.h F: lib/librte_eventdev/eventdev_trace_points.c F: lib/librte_eventdev/rte_trace_eventdev*.h F: lib/librte_cryptodev/cryptodev_trace_points.c F: lib/librte_cryptodev/rte_trace_cryptodev*.h F: lib/librte_mempool/mempool_trace_points.c F: lib/librte_mempool/rte_trace_mempool*.h > > > - I had something more dynamic in mind for gathering traces: like > enabling/disabling trace points in a running process and getting the > traces on the fly. rte_trace_enable() and rte_trace_disable() already avilable for this. > But I suppose the current framework is enough and we can work on this later. Yes. for all new features. > > > > > > > More details: > > ~~~~~~~~~~~~~ > > > > # The Native DPDK CTF trace support does not have any dependency on > > third-party library. > > The generated output file is compatible with LTTng as both are using > > CTF trace format. > > > > The performance gain comes from: > > 1) exploit dpdk worker thread usage model to avoid atomics and use per > > core variables > > 2) use hugepage, > > 3) avoid a lot function pointers in fast-path etc > > 4) avoid unaligned store for arm64 etc > > > > Features: > > ~~~~~~~~~ > > - APIs and Features are similar to rte_log dynamic framework > > API(expect log prints on stdout vs it dumps on trace file) > > I am not sure about the global and per tracepoint levels. > We must enable each tracepoint anyway, the level is just a commodity. > > I don't have strong arguments against it, but it feels odd to > copy/paste the rte_log framework. I have intentionally kept public API similar to rte_log wherever it is possible. Reason being, 1) In the future, it is easy to replace rte_log with trace _if needed_. 2) Avoid the new API learning curve. /Jerin > > > -- > David Marchand >
