Re: [dpdk-dev] [PATCH 2/8] trace: simplify trace point registration

[Jerin Jacob <jerinjacobk@gmail.com>]

On Tue, May 5, 2020 at 3:01 AM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 04/05/2020 19:54, Jerin Jacob:
> > On Mon, May 4, 2020 at 11:10 PM David Marchand
> > > On Mon, May 4, 2020 at 7:19 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > On Mon, May 4, 2020 at 10:38 PM David Marchand
> > > > > On Mon, May 4, 2020 at 4:39 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > > > On Mon, May 4, 2020 at 7:34 PM David Marchand <david.marchand@redhat.com> wrote:
> > > > > > > On Mon, May 4, 2020 at 4:47 AM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > > > > > On Mon, May 4, 2020 at 2:02 AM David Marchand <david.marchand@redhat.com> wrote:
> > > > > > > > >
> > > > > > > > > RTE_TRACE_POINT_DEFINE and RTE_TRACE_POINT_REGISTER must come in pairs.
> > > > > > > > > Merge them and let RTE_TRACE_POINT_REGISTER handle the constructor part.
> > > > > > > >
> > > > > > > >
> > > > > > > > Initially, I thought of doing the same. But, later I realized that
> > > > > > > > this largely grows the number of constructors been called.
> > > > > > > > I had concerns about the boot time of the application and/or loading
> > > > > > > > the shared library, that the reason why spitting
> > > > > > > > as two so that constructor registers a burst of traces like rte_log.
> > > > > > >
> > > > > > > I am a bit skeptical.
> > > > > > > In terms of cycles and looking at __rte_trace_point_register() (which
> > > > > > > calls malloc), the cost of calling multiple constructors instead of
> > > > > > > one is negligible.
> > > > > >
> > > > > > We will have a lot tracepoints latter, each one translates to the
> > > > > > constructor may not be a good
> > > > > > improvement. The scope is limited only to register function so IMO it
> > > > > > is okay to have split
> > > > > > just like rte_log. I don't see any reason why it has to be a different
> > > > > > than rte_log.
> > > > >
> > > > > What is similar to rte_log?
> > > > > There is neither RTE_LOG_REGISTER macro, nor two-steps declaration of
> > > > > dynamic logtypes.
> > > >
> > > >
> > > > Here is an example of rte_log registration. Which has _one_
> > > > constructor and N number of
> > > > rte_log_register() underneath.
> > >
> > > rte_log is one thing, rte_trace is already different.
> > >
> > > There is _no macro_ in rte_log for registration.
> > > The reason being in that a rte_log logtype is a simple integer without
> > > any special declaration requiring a macro.
> >
> > I just wrapped in macro for convincing, but it has the same semantics.
> > global variable and API/macro to register.
> >
> >
> > >
> > > For tracepoints, we have a special two steps thing: the tracepoint
> > > handle must be derived from the tracepoint name.
> > > Then this handle must be registered.
> > > What I proposed is to make life easier for developers that want to add
> > > tracepoints and I suppose you noticed patch 1 of this series.
> >
> > To reduce the constructors. I don't want trace libraries to add lot of
> > constructors.
> > I don't think it simplifies a lot as the scope of only for registration.
> >
> >
> > >
> > >
> > > > > > One of the thought process is, we probably remove the constructor
> > > > > > scheme to all other with DPDK
> > > > > > and replace it with a more register scheme. In such a case, we can
> > > > > > skip calling the constructor all tother
> > > > > > when trace is disabled.
> > > > >
> > > > > Sorry, but I have a hard time understanding your point.
> > > > > Are you talking about application boot time?
> > > >
> > > > Yes. The optimization of application boottime time in case of static
> > > > binary and/or shared library(.so) load time.
> > >
> > > As Thomas mentioned, do you have numbers?
> >
> > No. But I know, it is obvious that current code is better in terms of
> > boot time than the proposed one.
> > I would like to not add a lot of constructor for trace as the FIRST
> > module in DPDK.
>
> No, it is not obvious.
> The version from David looks simpler to use and to understand.
> Without any number, I consider usability (and maintenance) wins.

Thanks for the feedback.

As the trace maintainer, I would like not to explode constructor usage
for trace library.
My reasoning, We could do trace registration without this constructor scheme.

If you think, it is better usability, lets add an option for rte_log
for the constructor
scheme. Analyze the impact wrt boot time and cross-platform pov as the log
has a lot of entries to test. If the usage makes sense then it should make sense
for rte_log too. I would like to NOT have trace to be the first
library to explode
with the constructor scheme. I suggest removing this specific patch from RC2 and
revisit later.


>
>