[v4] eal: add build-time option to omit trace

Message ID 20240924133957.1505113-1-mb@smartsharesystems.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v4] eal: add build-time option to omit trace |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/github-robot: build success github build: passed
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-marvell-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS

Commit Message

Morten Brørup Sept. 24, 2024, 1:39 p.m. UTC
Some applications want to omit the trace feature.
Either to reduce the memory footprint, to reduce the exposed attack
surface, or for other reasons.

This patch adds an option in rte_config.h to include or omit trace in the
build. Trace is included by default.

Omitting trace works by omitting all trace points.
For API and ABI compatibility, the trace feature itself remains.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
v4:
* Added check for generic trace enabled when registering trace points, in
  RTE_INIT. (Jerin Jacob)
* Test application uses function instead of macro to check if generic
  trace is enabled. (Jerin Jacob)
* Performance test application uses function to check if generic trace is
  enabled.
v3:
* Simpler version with much fewer ifdefs. (Jerin Jacob)
v2:
* Added/modified macros required for building applications with trace
  omitted.
---
 app/test/test_trace.c                      |  4 ++++
 app/test/test_trace_perf.c                 |  5 +++++
 config/rte_config.h                        |  1 +
 lib/eal/include/rte_trace_point.h          | 21 +++++++++++++++++++++
 lib/eal/include/rte_trace_point_register.h |  2 ++
 5 files changed, 33 insertions(+)
  

Comments

Stephen Hemminger Sept. 24, 2024, 3:30 p.m. UTC | #1
On Tue, 24 Sep 2024 13:39:57 +0000
Morten Brørup <mb@smartsharesystems.com> wrote:

> Some applications want to omit the trace feature.
> Either to reduce the memory footprint, to reduce the exposed attack
> surface, or for other reasons.
> 
> This patch adds an option in rte_config.h to include or omit trace in the
> build. Trace is included by default.
> 
> Omitting trace works by omitting all trace points.
> For API and ABI compatibility, the trace feature itself remains.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>

This is good compact solution. In future, it would be nice if DPDK could
use static keys to isolate rarely used features. But static keys require
runtime modification of instructions.

https://docs.kernel.org/6.7/staging/static-keys.html

Acked-by: Stephen Hemminger <stephen@networkplumber.org>
  
Jerin Jacob Sept. 24, 2024, 3:41 p.m. UTC | #2
On Tue, Sep 24, 2024 at 7:10 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> Some applications want to omit the trace feature.
> Either to reduce the memory footprint, to reduce the exposed attack
> surface, or for other reasons.
>
> This patch adds an option in rte_config.h to include or omit trace in the
> build. Trace is included by default.
>
> Omitting trace works by omitting all trace points.
> For API and ABI compatibility, the trace feature itself remains.
>
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
> v4:
> * Added check for generic trace enabled when registering trace points, in
>   RTE_INIT. (Jerin Jacob)
> * Test application uses function instead of macro to check if generic
>   trace is enabled. (Jerin Jacob)
> * Performance test application uses function to check if generic trace is
>   enabled.
> v3:
> * Simpler version with much fewer ifdefs. (Jerin Jacob)
> v2:
> * Added/modified macros required for building applications with trace
>   omitted.

>
> +/**
> + * @internal

Since it is used in app/test. Can we remove it as internal and make
symbol as rte_trace_point_is_enabled

> + *
> + * Test if the tracepoint compile-time option is enabled.
> + *
> + * @return
> + *   true if tracepoint enabled, false otherwise.
> + */
> +__rte_experimental
> +static __rte_always_inline bool
> +__rte_trace_point_generic_is_enabled(void)

Do we need to keep _generic_

Rest looks good to me.
  
Morten Brørup Sept. 24, 2024, 3:53 p.m. UTC | #3
> From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> Sent: Tuesday, 24 September 2024 11.42
> 
> On Tue, Sep 24, 2024 at 7:10 PM Morten Brørup <mb@smartsharesystems.com>
> wrote:
> >
> > Some applications want to omit the trace feature.
> > Either to reduce the memory footprint, to reduce the exposed attack
> > surface, or for other reasons.
> >
> > This patch adds an option in rte_config.h to include or omit trace in
> the
> > build. Trace is included by default.
> >
> > Omitting trace works by omitting all trace points.
> > For API and ABI compatibility, the trace feature itself remains.
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> > v4:
> > * Added check for generic trace enabled when registering trace points,
> in
> >   RTE_INIT. (Jerin Jacob)
> > * Test application uses function instead of macro to check if generic
> >   trace is enabled. (Jerin Jacob)
> > * Performance test application uses function to check if generic trace
> is
> >   enabled.
> > v3:
> > * Simpler version with much fewer ifdefs. (Jerin Jacob)
> > v2:
> > * Added/modified macros required for building applications with trace
> >   omitted.
> 
> >
> > +/**
> > + * @internal
> 
> Since it is used in app/test. Can we remove it as internal and make
> symbol as rte_trace_point_is_enabled

I don't think we should make functions public if only used for test purposes.

Do you think it is useful for normal usage too? Does rte_trace_is_enabled() not suffice?

> 
> > + *
> > + * Test if the tracepoint compile-time option is enabled.
> > + *
> > + * @return
> > + *   true if tracepoint enabled, false otherwise.
> > + */
> > +__rte_experimental
> > +static __rte_always_inline bool
> > +__rte_trace_point_generic_is_enabled(void)
> 
> Do we need to keep _generic_

Other internal functions have _generic_ too, so I added it.
If we decide to make it public, I agree _generic_ should be removed.

> 
> Rest looks good to me.
  
Jerin Jacob Sept. 24, 2024, 3:57 p.m. UTC | #4
On Tue, Sep 24, 2024 at 9:23 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > Sent: Tuesday, 24 September 2024 11.42
> >
> > On Tue, Sep 24, 2024 at 7:10 PM Morten Brørup <mb@smartsharesystems.com>
> > wrote:
> > >
> > > Some applications want to omit the trace feature.
> > > Either to reduce the memory footprint, to reduce the exposed attack
> > > surface, or for other reasons.
> > >
> > > This patch adds an option in rte_config.h to include or omit trace in
> > the
> > > build. Trace is included by default.
> > >
> > > Omitting trace works by omitting all trace points.
> > > For API and ABI compatibility, the trace feature itself remains.
> > >
> > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > ---
> > > v4:
> > > * Added check for generic trace enabled when registering trace points,
> > in
> > >   RTE_INIT. (Jerin Jacob)
> > > * Test application uses function instead of macro to check if generic
> > >   trace is enabled. (Jerin Jacob)
> > > * Performance test application uses function to check if generic trace
> > is
> > >   enabled.
> > > v3:
> > > * Simpler version with much fewer ifdefs. (Jerin Jacob)
> > > v2:
> > > * Added/modified macros required for building applications with trace
> > >   omitted.
> >
> > >
> > > +/**
> > > + * @internal
> >
> > Since it is used in app/test. Can we remove it as internal and make
> > symbol as rte_trace_point_is_enabled
>
> I don't think we should make functions public if only used for test purposes.
>
> Do you think it is useful for normal usage too? Does rte_trace_is_enabled() not suffice?

It will be good for an app to know, Is trace feature disabled if the
application cares about.
Yes. rte_trace_is_enabled() suffice.


>
> >
> > > + *
> > > + * Test if the tracepoint compile-time option is enabled.
> > > + *
> > > + * @return
> > > + *   true if tracepoint enabled, false otherwise.
> > > + */
> > > +__rte_experimental
> > > +static __rte_always_inline bool
> > > +__rte_trace_point_generic_is_enabled(void)
> >
> > Do we need to keep _generic_
>
> Other internal functions have _generic_ too, so I added it.
> If we decide to make it public, I agree _generic_ should be removed.
>
> >
> > Rest looks good to me.
  
Morten Brørup Oct. 1, 2024, 1:49 p.m. UTC | #5
Jerin,

If you have no further comments, please add review/ack tag, to help Thomas see that the patch has been accepted by the maintainer, and can be merged.
  
Jerin Jacob Oct. 1, 2024, 2:05 p.m. UTC | #6
On Tue, Oct 1, 2024 at 7:19 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> Jerin,
>
> If you have no further comments, please add review/ack tag, to help Thomas see that the patch has been accepted by the maintainer, and can be merged.

There was a comment to make the function as rte_trace_is_enabled() and
remove internal. The rest looks good to me. I will Ack in the next
version.


>
  
Morten Brørup Oct. 1, 2024, 2:14 p.m. UTC | #7
> From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> Sent: Tuesday, 1 October 2024 16.05
> 
> On Tue, Oct 1, 2024 at 7:19 PM Morten Brørup <mb@smartsharesystems.com>
> wrote:
> >
> > Jerin,
> >
> > If you have no further comments, please add review/ack tag, to help
> Thomas see that the patch has been accepted by the maintainer, and can
> be merged.
> 
> There was a comment to make the function as rte_trace_is_enabled() and
> remove internal. The rest looks good to me. I will Ack in the next
> version.

Perhaps my reply to that comment was unclear... such a public function already exists in the previous API:
https://elixir.bootlin.com/dpdk/v24.07/source/lib/eal/include/rte_trace.h#L36

That function tells if trace enabled at both build time and runtime, and returns false if not.

A separate public function to tell if trace is enabled at build time seems like overkill to me. Is that what you are asking for?
  
Jerin Jacob Oct. 1, 2024, 3:01 p.m. UTC | #8
On Tue, Oct 1, 2024 at 7:44 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > Sent: Tuesday, 1 October 2024 16.05
> >
> > On Tue, Oct 1, 2024 at 7:19 PM Morten Brørup <mb@smartsharesystems.com>
> > wrote:
> > >
> > > Jerin,
> > >
> > > If you have no further comments, please add review/ack tag, to help
> > Thomas see that the patch has been accepted by the maintainer, and can
> > be merged.
> >
> > There was a comment to make the function as rte_trace_is_enabled() and
> > remove internal. The rest looks good to me. I will Ack in the next
> > version.
>
> Perhaps my reply to that comment was unclear... such a public function already exists in the previous API:

I see. It was not clear.

> https://elixir.bootlin.com/dpdk/v24.07/source/lib/eal/include/rte_trace.h#L36
>
> That function tells if trace enabled at both build time and runtime, and returns false if not.
>
> A separate public function to tell if trace is enabled at build time seems like overkill to me. Is that what you are asking for?

No. Just use rte_trace_is_enabled() in app/test instead of
__rte_trace_point_generic_is_enabled() as it is internal.
  
Morten Brørup Oct. 1, 2024, 4:01 p.m. UTC | #9
> From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> Sent: Tuesday, 1 October 2024 17.02
> 
> On Tue, Oct 1, 2024 at 7:44 PM Morten Brørup <mb@smartsharesystems.com>
> wrote:
> >
> > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > > Sent: Tuesday, 1 October 2024 16.05
> > >
> > > On Tue, Oct 1, 2024 at 7:19 PM Morten Brørup
> <mb@smartsharesystems.com>
> > > wrote:
> > > >
> > > > Jerin,
> > > >
> > > > If you have no further comments, please add review/ack tag, to
> help
> > > Thomas see that the patch has been accepted by the maintainer, and
> can
> > > be merged.
> > >
> > > There was a comment to make the function as rte_trace_is_enabled()
> and
> > > remove internal. The rest looks good to me. I will Ack in the next
> > > version.
> >
> > Perhaps my reply to that comment was unclear... such a public
> function already exists in the previous API:
> 
> I see. It was not clear.
> 
> >
> https://elixir.bootlin.com/dpdk/v24.07/source/lib/eal/include/rte_trace
> .h#L36
> >
> > That function tells if trace enabled at both build time and runtime,
> and returns false if not.
> >
> > A separate public function to tell if trace is enabled at build time
> seems like overkill to me. Is that what you are asking for?
> 
> No. Just use rte_trace_is_enabled() in app/test instead of
> __rte_trace_point_generic_is_enabled() as it is internal.

Just tested it, and it didn't have the wanted effect.
I think rte_trace_is_enabled() returns false until at least one tracepoint has been enabled, which seems like a good optimization.
But it also means that we cannot use it to replace __rte_trace_point_generic_is_enabled() in test/app, because no tracepoints have been enabled at this point of execution, so it returns false here.

I looked around in the code, and cannot find a method without looking at internals, or duplicating a test case.

I could test if rte_trace_point_lookup("app.dpdk.test.tp") returns non-NULL, but that would duplicate the same test in test_trace_points_lookup().

What do you think...
Keep using internal function __rte_trace_point_generic_is_enabled(),
test rte_trace_point_lookup("app.dpdk.test.tp") != NULL,
or any other idea?
  
Jerin Jacob Oct. 1, 2024, 4:06 p.m. UTC | #10
On Tue, Oct 1, 2024 at 9:32 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > Sent: Tuesday, 1 October 2024 17.02
> >
> > On Tue, Oct 1, 2024 at 7:44 PM Morten Brørup <mb@smartsharesystems.com>
> > wrote:
> > >
> > > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > > > Sent: Tuesday, 1 October 2024 16.05
> > > >
> > > > On Tue, Oct 1, 2024 at 7:19 PM Morten Brørup
> > <mb@smartsharesystems.com>
> > > > wrote:
> > > > >
> > > > > Jerin,
> > > > >
> > > > > If you have no further comments, please add review/ack tag, to
> > help
> > > > Thomas see that the patch has been accepted by the maintainer, and
> > can
> > > > be merged.
> > > >
> > > > There was a comment to make the function as rte_trace_is_enabled()
> > and
> > > > remove internal. The rest looks good to me. I will Ack in the next
> > > > version.
> > >
> > > Perhaps my reply to that comment was unclear... such a public
> > function already exists in the previous API:
> >
> > I see. It was not clear.
> >
> > >
> > https://elixir.bootlin.com/dpdk/v24.07/source/lib/eal/include/rte_trace
> > .h#L36
> > >
> > > That function tells if trace enabled at both build time and runtime,
> > and returns false if not.
> > >
> > > A separate public function to tell if trace is enabled at build time
> > seems like overkill to me. Is that what you are asking for?
> >
> > No. Just use rte_trace_is_enabled() in app/test instead of
> > __rte_trace_point_generic_is_enabled() as it is internal.
>
> Just tested it, and it didn't have the wanted effect.
> I think rte_trace_is_enabled() returns false until at least one tracepoint has been enabled, which seems like a good optimization.
> But it also means that we cannot use it to replace __rte_trace_point_generic_is_enabled() in test/app, because no tracepoints have been enabled at this point of execution, so it returns false here.
>
> I looked around in the code, and cannot find a method without looking at internals, or duplicating a test case.
>
> I could test if rte_trace_point_lookup("app.dpdk.test.tp") returns non-NULL, but that would duplicate the same test in test_trace_points_lookup().
>
> What do you think...
> Keep using internal function __rte_trace_point_generic_is_enabled(),
> test rte_trace_point_lookup("app.dpdk.test.tp") != NULL,
> or any other idea?

How about the following, it is anyway the correct thing to do

bool
rte_trace_is_enabled(void)
{
 +       if (__rte_trace_point_generic_is_enabled() == false)
  +              return false;
        return rte_atomic_load_explicit(&trace.status,
rte_memory_order_acquire) != 0;
}


>
  
Morten Brørup Oct. 1, 2024, 4:15 p.m. UTC | #11
> From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> Sent: Tuesday, 1 October 2024 18.06
> 
> On Tue, Oct 1, 2024 at 9:32 PM Morten Brørup <mb@smartsharesystems.com>
> wrote:
> >
> > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > > Sent: Tuesday, 1 October 2024 17.02
> > >
> > > On Tue, Oct 1, 2024 at 7:44 PM Morten Brørup
> <mb@smartsharesystems.com>
> > > wrote:
> > > >
> > > > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > > > > Sent: Tuesday, 1 October 2024 16.05
> > > > >
> > > > > On Tue, Oct 1, 2024 at 7:19 PM Morten Brørup
> > > <mb@smartsharesystems.com>
> > > > > wrote:
> > > > > >
> > > > > > Jerin,
> > > > > >
> > > > > > If you have no further comments, please add review/ack tag,
> to
> > > help
> > > > > Thomas see that the patch has been accepted by the maintainer,
> and
> > > can
> > > > > be merged.
> > > > >
> > > > > There was a comment to make the function as
> rte_trace_is_enabled()
> > > and
> > > > > remove internal. The rest looks good to me. I will Ack in the
> next
> > > > > version.
> > > >
> > > > Perhaps my reply to that comment was unclear... such a public
> > > function already exists in the previous API:
> > >
> > > I see. It was not clear.
> > >
> > > >
> > >
> https://elixir.bootlin.com/dpdk/v24.07/source/lib/eal/include/rte_trace
> > > .h#L36
> > > >
> > > > That function tells if trace enabled at both build time and
> runtime,
> > > and returns false if not.
> > > >
> > > > A separate public function to tell if trace is enabled at build
> time
> > > seems like overkill to me. Is that what you are asking for?
> > >
> > > No. Just use rte_trace_is_enabled() in app/test instead of
> > > __rte_trace_point_generic_is_enabled() as it is internal.
> >
> > Just tested it, and it didn't have the wanted effect.
> > I think rte_trace_is_enabled() returns false until at least one
> tracepoint has been enabled, which seems like a good optimization.
> > But it also means that we cannot use it to replace
> __rte_trace_point_generic_is_enabled() in test/app, because no
> tracepoints have been enabled at this point of execution, so it returns
> false here.
> >
> > I looked around in the code, and cannot find a method without looking
> at internals, or duplicating a test case.
> >
> > I could test if rte_trace_point_lookup("app.dpdk.test.tp") returns
> non-NULL, but that would duplicate the same test in
> test_trace_points_lookup().
> >
> > What do you think...
> > Keep using internal function __rte_trace_point_generic_is_enabled(),
> > test rte_trace_point_lookup("app.dpdk.test.tp") != NULL,
> > or any other idea?
> 
> How about the following, it is anyway the correct thing to do
> 
> bool
> rte_trace_is_enabled(void)
> {
>  +       if (__rte_trace_point_generic_is_enabled() == false)
>   +              return false;
>         return rte_atomic_load_explicit(&trace.status,
> rte_memory_order_acquire) != 0;
> }
> 

It's the opposite that's causing problems:
Even when built with trace, rte_trace_is_enabled() returns false, because no trace points have been enabled when the test application checks if it should run test cases or not. At this point, trace.status is zero, so it skips testing.

We don't need to add the test for rte_trace_point_generic_is_enabled()==false in rte_trace_is_enabled(), because nothing can increase trace.status if no tracepoints exist. (As far as I understand.)
  
Jerin Jacob Oct. 2, 2024, 4:17 p.m. UTC | #12
On Tue, Oct 1, 2024 at 9:45 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > Sent: Tuesday, 1 October 2024 18.06
> >
> > On Tue, Oct 1, 2024 at 9:32 PM Morten Brørup <mb@smartsharesystems.com>
> > wrote:
> > >
> > > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > > > Sent: Tuesday, 1 October 2024 17.02
> > > >
> > > > On Tue, Oct 1, 2024 at 7:44 PM Morten Brørup
> > <mb@smartsharesystems.com>
> > > > wrote:
> > > > >
> > > > > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > > > > > Sent: Tuesday, 1 October 2024 16.05
> > > > > >
> > > > > > On Tue, Oct 1, 2024 at 7:19 PM Morten Brørup
> > > > <mb@smartsharesystems.com>
> > > > > > wrote:
> > > > > > >
> > > > > > > Jerin,
> > > > > > >
> > > > > > > If you have no further comments, please add review/ack tag,
> > to
> > > > help
> > > > > > Thomas see that the patch has been accepted by the maintainer,
> > and
> > > > can
> > > > > > be merged.
> > > > > >
> > > > > > There was a comment to make the function as
> > rte_trace_is_enabled()
> > > > and
> > > > > > remove internal. The rest looks good to me. I will Ack in the
> > next
> > > > > > version.
> > > > >
> > > > > Perhaps my reply to that comment was unclear... such a public
> > > > function already exists in the previous API:
> > > >
> > > > I see. It was not clear.
> > > >
> > > > >
> > > >
> > https://elixir.bootlin.com/dpdk/v24.07/source/lib/eal/include/rte_trace
> > > > .h#L36
> > > > >
> > > > > That function tells if trace enabled at both build time and
> > runtime,
> > > > and returns false if not.
> > > > >
> > > > > A separate public function to tell if trace is enabled at build
> > time
> > > > seems like overkill to me. Is that what you are asking for?
> > > >
> > > > No. Just use rte_trace_is_enabled() in app/test instead of
> > > > __rte_trace_point_generic_is_enabled() as it is internal.
> > >
> > > Just tested it, and it didn't have the wanted effect.
> > > I think rte_trace_is_enabled() returns false until at least one
> > tracepoint has been enabled, which seems like a good optimization.
> > > But it also means that we cannot use it to replace
> > __rte_trace_point_generic_is_enabled() in test/app, because no
> > tracepoints have been enabled at this point of execution, so it returns
> > false here.
> > >
> > > I looked around in the code, and cannot find a method without looking
> > at internals, or duplicating a test case.
> > >
> > > I could test if rte_trace_point_lookup("app.dpdk.test.tp") returns
> > non-NULL, but that would duplicate the same test in
> > test_trace_points_lookup().
> > >
> > > What do you think...
> > > Keep using internal function __rte_trace_point_generic_is_enabled(),
> > > test rte_trace_point_lookup("app.dpdk.test.tp") != NULL,
> > > or any other idea?
> >
> > How about the following, it is anyway the correct thing to do
> >
> > bool
> > rte_trace_is_enabled(void)
> > {
> >  +       if (__rte_trace_point_generic_is_enabled() == false)
> >   +              return false;
> >         return rte_atomic_load_explicit(&trace.status,
> > rte_memory_order_acquire) != 0;
> > }
> >
>
> It's the opposite that's causing problems:
> Even when built with trace, rte_trace_is_enabled() returns false, because no trace points have been enabled when the test application checks if it should run test cases or not. At this point, trace.status is zero, so it skips testing.
>
> We don't need to add the test for rte_trace_point_generic_is_enabled()==false in rte_trace_is_enabled(), because nothing can increase trace.status if no tracepoints exist. (As far as I understand.)

OK. I see. IMO, It is not good to expose internal symbol to app/test.
How about,
Changing __rte_trace_point_generic_is_enabled() as
rte_trace_feature_is_enabled() or similar. This variant is more like
feature is disabled at compiled time or not? And make it as public and
use in app/test.


>
  
Morten Brørup Oct. 6, 2024, 12:58 p.m. UTC | #13
> From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> Sent: Wednesday, 2 October 2024 18.18
> 
> On Tue, Oct 1, 2024 at 9:45 PM Morten Brørup <mb@smartsharesystems.com>
> wrote:
> >
> > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > > Sent: Tuesday, 1 October 2024 18.06
> > >
> > > On Tue, Oct 1, 2024 at 9:32 PM Morten Brørup
> <mb@smartsharesystems.com>
> > > wrote:
> > > >
> > > > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > > > > Sent: Tuesday, 1 October 2024 17.02
> > > > >
> > > > > On Tue, Oct 1, 2024 at 7:44 PM Morten Brørup
> > > <mb@smartsharesystems.com>
> > > > > wrote:
> > > > > >
> > > > > > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > > > > > > Sent: Tuesday, 1 October 2024 16.05
> > > > > > >
> > > > > > > On Tue, Oct 1, 2024 at 7:19 PM Morten Brørup
> > > > > <mb@smartsharesystems.com>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > Jerin,
> > > > > > > >
> > > > > > > > If you have no further comments, please add review/ack
> tag,
> > > to
> > > > > help
> > > > > > > Thomas see that the patch has been accepted by the
> maintainer,
> > > and
> > > > > can
> > > > > > > be merged.
> > > > > > >
> > > > > > > There was a comment to make the function as
> > > rte_trace_is_enabled()
> > > > > and
> > > > > > > remove internal. The rest looks good to me. I will Ack in
> the
> > > next
> > > > > > > version.
> > > > > >
> > > > > > Perhaps my reply to that comment was unclear... such a public
> > > > > function already exists in the previous API:
> > > > >
> > > > > I see. It was not clear.
> > > > >
> > > > > >
> > > > >
> > >
> https://elixir.bootlin.com/dpdk/v24.07/source/lib/eal/include/rte_trace
> > > > > .h#L36
> > > > > >
> > > > > > That function tells if trace enabled at both build time and
> > > runtime,
> > > > > and returns false if not.
> > > > > >
> > > > > > A separate public function to tell if trace is enabled at
> build
> > > time
> > > > > seems like overkill to me. Is that what you are asking for?
> > > > >
> > > > > No. Just use rte_trace_is_enabled() in app/test instead of
> > > > > __rte_trace_point_generic_is_enabled() as it is internal.
> > > >
> > > > Just tested it, and it didn't have the wanted effect.
> > > > I think rte_trace_is_enabled() returns false until at least one
> > > tracepoint has been enabled, which seems like a good optimization.
> > > > But it also means that we cannot use it to replace
> > > __rte_trace_point_generic_is_enabled() in test/app, because no
> > > tracepoints have been enabled at this point of execution, so it
> returns
> > > false here.
> > > >
> > > > I looked around in the code, and cannot find a method without
> looking
> > > at internals, or duplicating a test case.
> > > >
> > > > I could test if rte_trace_point_lookup("app.dpdk.test.tp")
> returns
> > > non-NULL, but that would duplicate the same test in
> > > test_trace_points_lookup().
> > > >
> > > > What do you think...
> > > > Keep using internal function
> __rte_trace_point_generic_is_enabled(),
> > > > test rte_trace_point_lookup("app.dpdk.test.tp") != NULL,
> > > > or any other idea?
> > >
> > > How about the following, it is anyway the correct thing to do
> > >
> > > bool
> > > rte_trace_is_enabled(void)
> > > {
> > >  +       if (__rte_trace_point_generic_is_enabled() == false)
> > >   +              return false;
> > >         return rte_atomic_load_explicit(&trace.status,
> > > rte_memory_order_acquire) != 0;
> > > }
> > >
> >
> > It's the opposite that's causing problems:
> > Even when built with trace, rte_trace_is_enabled() returns false,
> because no trace points have been enabled when the test application
> checks if it should run test cases or not. At this point, trace.status
> is zero, so it skips testing.
> >
> > We don't need to add the test for
> rte_trace_point_generic_is_enabled()==false in rte_trace_is_enabled(),
> because nothing can increase trace.status if no tracepoints exist. (As
> far as I understand.)
> 
> OK. I see. IMO, It is not good to expose internal symbol to app/test.

Agree. Fixed in v5.

> How about,
> Changing __rte_trace_point_generic_is_enabled() as
> rte_trace_feature_is_enabled() or similar. This variant is more like
> feature is disabled at compiled time or not? And make it as public and
> use in app/test.

In v5, I have added the public function rte_trace_feature_is_enabled(), which returns true iff the application is built with trace enabled (this is tested using inline), and the DPDK - in case it is built separately - is built with trace enabled (this is compiled with the trace lib's C file).

App/test now uses this function instead of the internal one.

__rte_trace_point_generic_is_enabled() needs to remain static inline, due to the way it is used for omitting trace at build time.

PS: Both trace_autotest and trace_perf_autotest have been tested with and without RTE_TRACE, and they behave as expected.
  

Patch

diff --git a/app/test/test_trace.c b/app/test/test_trace.c
index 00809f433b..26656fe024 100644
--- a/app/test/test_trace.c
+++ b/app/test/test_trace.c
@@ -245,6 +245,10 @@  static struct unit_test_suite trace_tests = {
 static int
 test_trace(void)
 {
+	if (!__rte_trace_point_generic_is_enabled()) {
+		printf("Trace omitted at build-time, skipping test\n");
+		return TEST_SKIPPED;
+	}
 	return unit_test_suite_runner(&trace_tests);
 }
 
diff --git a/app/test/test_trace_perf.c b/app/test/test_trace_perf.c
index 8257cc02be..504574ef8a 100644
--- a/app/test/test_trace_perf.c
+++ b/app/test/test_trace_perf.c
@@ -150,6 +150,11 @@  test_trace_perf(void)
 	struct test_data *data;
 	size_t sz;
 
+	if (!__rte_trace_point_generic_is_enabled()) {
+		printf("Trace omitted at build-time, skipping test\n");
+		return TEST_SKIPPED;
+	}
+
 	nb_cores = rte_lcore_count();
 	nb_workers = nb_cores - 1;
 	if (nb_cores < 2) {
diff --git a/config/rte_config.h b/config/rte_config.h
index dd7bb0d35b..fd6f8a2f1a 100644
--- a/config/rte_config.h
+++ b/config/rte_config.h
@@ -49,6 +49,7 @@ 
 #define RTE_MAX_TAILQ 32
 #define RTE_LOG_DP_LEVEL RTE_LOG_INFO
 #define RTE_MAX_VFIO_CONTAINERS 64
+#define RTE_TRACE 1
 
 /* bsd module defines */
 #define RTE_CONTIGMEM_MAX_NUM_BUFS 64
diff --git a/lib/eal/include/rte_trace_point.h b/lib/eal/include/rte_trace_point.h
index 41e2a7f99e..b80688ce89 100644
--- a/lib/eal/include/rte_trace_point.h
+++ b/lib/eal/include/rte_trace_point.h
@@ -212,6 +212,25 @@  bool rte_trace_point_is_enabled(rte_trace_point_t *tp);
 __rte_experimental
 rte_trace_point_t *rte_trace_point_lookup(const char *name);
 
+/**
+ * @internal
+ *
+ * Test if the tracepoint compile-time option is enabled.
+ *
+ * @return
+ *   true if tracepoint enabled, false otherwise.
+ */
+__rte_experimental
+static __rte_always_inline bool
+__rte_trace_point_generic_is_enabled(void)
+{
+#ifdef RTE_TRACE
+	return true;
+#else
+	return false;
+#endif
+}
+
 /**
  * @internal
  *
@@ -359,6 +378,8 @@  __rte_trace_point_emit_ev_header(void *mem, uint64_t in)
 #define __rte_trace_point_emit_header_generic(t) \
 void *mem; \
 do { \
+	if (!__rte_trace_point_generic_is_enabled()) \
+		return; \
 	const uint64_t val = rte_atomic_load_explicit(t, rte_memory_order_acquire); \
 	if (likely(!(val & __RTE_TRACE_FIELD_ENABLE_MASK))) \
 		return; \
diff --git a/lib/eal/include/rte_trace_point_register.h b/lib/eal/include/rte_trace_point_register.h
index 41260e5964..429b993fc2 100644
--- a/lib/eal/include/rte_trace_point_register.h
+++ b/lib/eal/include/rte_trace_point_register.h
@@ -23,6 +23,8 @@  rte_trace_point_t __rte_section("__rte_trace_point") __##trace; \
 static const char __##trace##_name[] = RTE_STR(name); \
 RTE_INIT(trace##_init) \
 { \
+	if (!__rte_trace_point_generic_is_enabled()) \
+		return; \
 	__rte_trace_point_register(&__##trace, __##trace##_name, \
 		(void (*)(void)) trace); \
 }