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

[David Marchand <david.marchand@redhat.com>]

Hello Jerin,

On Sun, Mar 29, 2020 at 4:43 PM <jerinj@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.

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.

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,


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


- On the MAINTAINERS update:

Trace
M: Jerin Jacob <jerinj@marvell.com>
M: Sunil Kumar Kori <skori@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).


- 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.
But I suppose the current framework is enough and we can work on this later.



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


-- 
David Marchand