On 2/1/2023 8:31 AM, Jerin Jacob wrote:

On Wed, Feb 1, 2023 at 3:50 AM Ferruh Yigit [off-list ref] wrote:

quoted

On 1/31/2023 6:46 PM, Jerin Jacob wrote:

quoted

On Wed, Feb 1, 2023 at 12:09 AM Ferruh Yigit [off-list ref] wrote:

quoted

On 1/30/2023 4:01 PM, Ankur Dwivedi wrote:

<...>

quoted

quoted

quoted

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.

quoted

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.

quoted

quoted

quoted

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