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

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 1/31/2023 6:46 PM, Jerin Jacob wrote:
> On Wed, Feb 1, 2023 at 12:09 AM Ferruh Yigit <ferruh.yigit@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.

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