[EXT] Re: [PATCH v6 2/6] ethdev: add trace points for ethdev (part one)

[Jerin Jacob]

On Wed, Feb 1, 2023 at 12:09 AM Ferruh Yigit <ferruh.yigit at amd.com> wrote:
>
> On 1/30/2023 4:01 PM, Ankur Dwivedi wrote:
>
> <...>
>
> >>> diff --git a/lib/ethdev/meson.build b/lib/ethdev/meson.build index
> >>> 39250b5da1..f5c0865023 100644
> >>> --- a/lib/ethdev/meson.build
> >>> +++ b/lib/ethdev/meson.build
> >>> @@ -24,6 +24,7 @@ headers = files(
> >>>          'rte_ethdev.h',
> >>>          'rte_ethdev_trace.h',
> >>>          'rte_ethdev_trace_fp.h',
> >>> +        'rte_ethdev_trace_fp_burst.h',
> >>
> >> Why these trace headers are public?
> >> Aren't trace points only used by the APIs, so I expect them to be internal, so
> >> applications shouldn't need them. Why they are expsed to user.
> > 'rte_ethdev_trace.h' can be made as internal. Not sure about 'rte_ethdev_trace_fp.h' and 'rte_ethdev_trace_fp_burst.h' as the tracepoints in fast path may be called from public inline functions.
>
> Trace calls used by inline functions needs to be public, in this case at
> least 'rte_ethdev_trace_fp_burst.h' needs to be public.
>
> Can you please at least move all trace points that are called by inline
> functions to the same file, 'rte_ethdev_trace_fp_burst.h', to reduce
> number of the header files to make public? Feel free to rename header if
> required.
>
> Meanwhile not sure about adding new header as dependency to end user.
> @Jerin, @Andrew, what do you think

rte_ethdev_trace_fp_burst.h will be installed through ninja install
and application does not need to directly include this. So this scheme
is OK. Right? I dont see any downside.


> 1) to move these trace points to 'rte_ethdev_core.h'
> OR
> 2) disable trace calls in inline functions on compile time, possibly
> with existing 'RTE_ETHDEV_DEBUG_*' macro