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

[Jerin Jacob <jerinjacobk@gmail.com>]

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.