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

["Morten Brørup" <mb@smartsharesystems.com>]

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