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