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

[Thomas Monjalon <thomas@monjalon.net>]

05/05/2020 19:09, Jerin Jacob:
> On Tue, May 5, 2020 at 10:38 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > On Tue, May 5, 2020 at 10:28 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > 05/05/2020 18:46, Jerin Jacob:
> > > > On Tue, May 5, 2020 at 9:58 PM David Marchand <david.marchand@redhat.com> wrote:
> > > > > On Tue, May 5, 2020 at 5:25 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > > > On Tue, May 5, 2020 at 5:56 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > > > > On Tue, May 5, 2020 at 5:06 PM David Marchand <david.marchand@redhat.com> wrote:
> > > > > > > > On Tue, May 5, 2020 at 12:13 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > > > > > > > > Please share the data.
> > > > > > > > > >
> > > > > > > > > > Measured time between first rte_trace_point_register and last one with
> > > > > > > > > > a simple patch:
> > > > > > > > >
> > > > > > > > > I will try to reproduce this, once we finalize on the above synergy
> > > > > > > > > with rte_log.
> > > > > > > >
> > > > > > > > I took the time to provide measure but you won't take the time to look at this.
> > > > > > >
> > > > > > > I will spend time on this. I would like to test with a shared library
> > > > > > > also and more tracepoints.
> > > > > > > I was looking for an agreement on using the constructor for rte_log as
> > > > > > > well(Just make sure the direction is correct).
> > > > > > >
> > > > > > > Next steps:
> > > > > > > - I will analyze the come back on this overhead on this thread.
> > > > > >
> > > > > > I have added 500 constructors for testing the overhead with the shared
> > > > > > build and static build.
> > > > > > My results inline with your results aka negligible overhead.
> > > > > >
> > > > > > David,
> > > > > > Do you have plan for similar RTE_LOG_REGISTER as mentioned earlier?
> > > > > > I would like to have rte_log and rte_trace semantics similar to registration.
> > > > > > If you are not planning to submit the rte_log patch then I can send
> > > > > > one for RC2 cleanup.
> > > > >
> > > > > It won't be possible for me.
> > > >
> > > > I can do that if we agree on the specifics.
> > > >
> > > >
> > > > >
> > > > > Relying on the current rte_log_register is buggy with shared builds,
> > > > > as drivers are calling rte_log_register, then impose a default level
> > > > > without caring about what the user passed.
> > > > > So if we introduce a RTE_LOG_REGISTER macro now at least this must be fixed too.
> > > > >
> > > > > What I wanted to do:
> > > > > - merge rte_log_register_and_pick_level() (experimental) into
> > > > > rte_log_register, doing this should be fine from my pov,
> > > > > - reconsider the relevance of a fallback logtype when registration fails,
> > > > > - shoot the default level per component thing: levels meaning is
> > > > > fragmented across the drivers/libraries because of it, but this will
> > > > > open a big box of stuff,
> > > >
> > > > This you are referring to internal implementation improvement. Right?
> > > > I was referring to remove the current clutter[1]
> > > > If we stick the following as the interface. Then you can do other
> > > > improvements when you get time
> > > > that won't change the consumer code or interference part.
> > > >
> > > > #define RTE_LOG_REGISTER(type, name, level)
> > >
> > > This discussion is interesting but out of scope for rte_trace.
> > > I am also interested in rte_log registration cleanup,
> > > but I know it is too much work for the last weeks of 20.05.
> > >
> > > As Olivier said about rte_trace,
> > > "Since it's a new API, it makes sense to make
> > > it as good as possible for the first version."
> > >
> > > So please let's conclude on this rte_trace patch for 20.05-rc2,
> > > and commit to fix rte_log registration in the first days of 20.08.
> >
> > Why not hold the trace registration patch 2/8 and apply rest for RC2.
> > Once we have synergy between the registration scheme between rte_log
> > and rte_trace
> > apply the patch for RC2.
> 
> I meant, Once we have synergy between the registration scheme between
> rte_log and rte_trace
> apply the patch for _20.08_?

Because of what I wrote above:
As Olivier said about rte_trace,
"Since it's a new API, it makes sense to make
it as good as possible for the first version."

The intent is to show an API as simple as possible
in order to have a maximum of developers integrating it,
and getting more interesting feedbacks.

In other words, we want to make your work shine for prime time.