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

[Jerin Jacob <jerinjacobk@gmail.com>]

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)

[1]
> > -/**
> > - * @internal
> > - */
> > -int otx2_logtype_base;
> > -/**
> > - * @internal
> > - */
> > -int otx2_logtype_mbox;
> > -/**
> > - * @internal
> > - */
> > -int otx2_logtype_npa;
> > -/**
> > - * @internal
> > - */
> > -int otx2_logtype_nix;
> > -/**
> > - * @internal
> > - */
> > -int otx2_logtype_npc;
> > -/**
> > - * @internal
> > - */
> > -int otx2_logtype_tm;
> > -/**
> > - * @internal
> > - */
> > -int otx2_logtype_sso;
> > -/**
> > - * @internal
> > - */
> > -int otx2_logtype_tim;
> > -/**
> > - * @internal
> > - */
> > -int otx2_logtype_dpi;
> > -/**
> > - * @internal
> > - */
> > -int otx2_logtype_ep;
> > -
> > -RTE_INIT(otx2_log_init);
> > -static void
> > -otx2_log_init(void)
> > -{
> > -       otx2_logtype_base = rte_log_register("pmd.octeontx2.base");
> > -       if (otx2_logtype_base >= 0)
> > -               rte_log_set_level(otx2_logtype_base, RTE_LOG_NOTICE);
> > -
> > -       otx2_logtype_mbox = rte_log_register("pmd.octeontx2.mbox");
> > -       if (otx2_logtype_mbox >= 0)
> > -               rte_log_set_level(otx2_logtype_mbox, RTE_LOG_NOTICE);
> > -
> > -       otx2_logtype_npa = rte_log_register("pmd.mempool.octeontx2");
> > -       if (otx2_logtype_npa >= 0)
> > -               rte_log_set_level(otx2_logtype_npa, RTE_LOG_NOTICE);
> > -
> > -       otx2_logtype_nix = rte_log_register("pmd.net.octeontx2");
> > -       if (otx2_logtype_nix >= 0)
> > -               rte_log_set_level(otx2_logtype_nix, RTE_LOG_NOTICE);
> > -
> > -       otx2_logtype_npc = rte_log_register("pmd.net.octeontx2.flow");
> > -       if (otx2_logtype_npc >= 0)
> > -               rte_log_set_level(otx2_logtype_npc, RTE_LOG_NOTICE);
> > -
> > -       otx2_logtype_tm = rte_log_register("pmd.net.octeontx2.tm");
> > -       if (otx2_logtype_tm >= 0)
> > -               rte_log_set_level(otx2_logtype_tm, RTE_LOG_NOTICE);
> > -
> > -       otx2_logtype_sso = rte_log_register("pmd.event.octeontx2");
> > -       if (otx2_logtype_sso >= 0)
> > -               rte_log_set_level(otx2_logtype_sso, RTE_LOG_NOTICE);
> > -
> > -       otx2_logtype_tim = rte_log_register("pmd.event.octeontx2.timer");
> > -       if (otx2_logtype_tim >= 0)
> > -               rte_log_set_level(otx2_logtype_tim, RTE_LOG_NOTICE);
> > -
> > -       otx2_logtype_dpi = rte_log_register("pmd.raw.octeontx2.dpi");
> > -       if (otx2_logtype_dpi >= 0)
> > -               rte_log_set_level(otx2_logtype_dpi, RTE_LOG_NOTICE);
> > -
> > -       otx2_logtype_ep = rte_log_register("pmd.raw.octeontx2.ep");
> > -       if (otx2_logtype_ep >= 0)
> > -               rte_log_set_level(otx2_logtype_ep, RTE_LOG_NOTICE);
> > -
> > -}
> > +RTE_LOG_REGISTER(otx2_logtype_base, pmd.octeontx2.base, NOTICE);
> > +RTE_LOG_REGISTER(otx2_logtype_mbox, pmd.octeontx2.mbox, NOTICE);
> > +RTE_LOG_REGISTER(otx2_logtype_npa, pmd.mempool.octeontx2, NOTICE);
> > +RTE_LOG_REGISTER(otx2_logtype_nix, pmd.net.octeontx2, NOTICE);
> > +RTE_LOG_REGISTER(otx2_logtype_npc, pmd.net.octeontx2.flow, NOTICE);
> > +RTE_LOG_REGISTER(otx2_logtype_tm, pmd.net.octeontx2.tm, NOTICE);
> > +RTE_LOG_REGISTER(otx2_logtype_sso, pmd.event.octeontx2, NOTICE);
> > +RTE_LOG_REGISTER(otx2_logtype_tim, pmd.event.octeontx2.timer, NOTICE);
> > +RTE_LOG_REGISTER(otx2_logtype_dpi, pmd.raw.octeontx2.dpi, NOTICE);
> > +RTE_LOG_REGISTER(otx2_logtype_ep, pmd.raw.octeontx2.ep, NOTICE);
> >
> > diff --git a/lib/librte_eal/include/rte_log.h b/lib/librte_eal/include/rte_log.h
> > index 1789ede56..22fc3802f 100644
> > --- a/lib/librte_eal/include/rte_log.h
> > +++ b/lib/librte_eal/include/rte_log.h
> > @@ -376,6 +376,15 @@ int rte_vlog(uint32_t level, uint32_t logtype,
> > const char *format, va_list ap)
> >                  RTE_LOGTYPE_ ## t, # t ": " __VA_ARGS__) :     \
> >          0)
> >
> > +#define RTE_LOG_REGISTER(type, name, level)                            \
> > +int type;                                                              \
> > +RTE_INIT(__##type)                                                     \
> > +{                                                                      \
> > +       type = rte_log_register(RTE_STR(name));                         \
> > +       if (type >= 0)                                                  \
> > +               rte_log_set_level(type, RTE_LOG_##level);               \
> > +}
> > +
>



>
>
>
> --
> David Marchand
>