On 2/1/2023 8:31 AM, Jerin Jacob wrote: > 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 >
With "enable_trace_fp" build option "rte_ethdev_trace_fp.h" dependency is still there, but wrapping with DEBUG macro can prevent it for non-debug use cases. Anyway, "rte_ethdev_trace_fp.h" dependency is already there before this patch, so OK to not change it, and I am OK to move forward by making all trace points and trace related header internal as much as possible. >> >>> >>>> 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 >>
