On Wed, Feb 1, 2023 at 3:50 AM Ferruh Yigit <ferruh.yi...@amd.com> wrote: > > On 1/31/2023 6:46 PM, Jerin Jacob wrote: > > On Wed, Feb 1, 2023 at 12:09 AM Ferruh Yigit <ferruh.yi...@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. > > > > Right. It is installed automatically with above meson change, and it is > included by 'rte_ethdev.h', so user doesn't need to include it > explicitly. Overall there is no functional problem here. > > Only this header file needs to be included (directly or indirectly) by > every application that use ethdev. I would much prefer to have an > internal header but not able to because of technical reasons (inline > functions). > After lots of effort we did to hide as much ethdev internals as we can, > now we are exposing some new trace stuff to user. > > As we can't prevent header to be public, I am just questioning below > options to reduce exposure of this header, hoping that it may lead a > better solution.
Yes. All non-inline function can goto internal header file. I think, it make sense to have separated header file _fp functions using inline functions to avoid cluttering main 'rte_ethdev.h' file. > disable trace calls in inline functions on compile time, possibly > with existing 'RTE_ETHDEV_DEBUG_*' macro Disabling trace calls to inline functions, already possible with "enable_trace_fp" build option. So it will be possible to wrap around that to virtually to disable > > > > >> 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 >
