> From: Jerin Jacob [mailto:jerinjac...@gmail.com] > Sent: Wednesday, 2 October 2024 18.18 > > On Tue, Oct 1, 2024 at 9:45 PM Morten Brørup <m...@smartsharesystems.com> > wrote: > > > > > From: Jerin Jacob [mailto:jerinjac...@gmail.com] > > > Sent: Tuesday, 1 October 2024 18.06 > > > > > > On Tue, Oct 1, 2024 at 9:32 PM Morten Brørup > <m...@smartsharesystems.com> > > > wrote: > > > > > > > > > From: Jerin Jacob [mailto:jerinjac...@gmail.com] > > > > > Sent: Tuesday, 1 October 2024 17.02 > > > > > > > > > > On Tue, Oct 1, 2024 at 7:44 PM Morten Brørup > > > <m...@smartsharesystems.com> > > > > > wrote: > > > > > > > > > > > > > From: Jerin Jacob [mailto:jerinjac...@gmail.com] > > > > > > > Sent: Tuesday, 1 October 2024 16.05 > > > > > > > > > > > > > > On Tue, Oct 1, 2024 at 7:19 PM Morten Brørup > > > > > <m...@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.
