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

[Ferruh Yigit]

On 2/1/2023 8:31 AM, Jerin Jacob wrote:
> On Wed, Feb 1, 2023 at 3:50 AM Ferruh Yigit <ferruh.yigit at 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.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.
>>>
>>
>> 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
>>