Re: [dpdk-dev] [PATCH 01/13] eal/log: introduce log register macro

[Jerin Jacob <jerinjacobk@gmail.com>]

On Wed, Jun 24, 2020 at 11:40 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Wed, Jun 24, 2020 at 9:13 PM Andrew Rybchenko
> <arybchenko@solarflare.com> wrote:
> >
> > On 6/24/20 6:32 PM, Jerin Jacob wrote:
> > > On Wed, Jun 24, 2020 at 8:56 PM Andrew Rybchenko
> > > <arybchenko@solarflare.com> wrote:
> > >>
> > >> On 6/24/20 4:11 PM, Jerin Jacob wrote:
> > >>> On Wed, Jun 17, 2020 at 3:51 PM Andrew Rybchenko
> > >>> <arybchenko@solarflare.com> wrote:
> > >>>>
> > >>>> On 6/17/20 1:02 PM, David Marchand wrote:
> > >>>>> On Wed, Jun 17, 2020 at 8:30 AM <jerinj@marvell.com> wrote:
> > >>>>>>
> > >>>>>> From: Jerin Jacob <jerinj@marvell.com>
> > >>>>>>
> > >>>>>> Introducing the RTE_LOG_REGISTER macro to avoid the code duplication
> > >>>>>> in the log registration process.
> > >>>>>>
> > >>>>>> It is a wrapper macro for declaring the logtype, register the log and sets
> > >>>>>
> > >>>>> Having the logtype variable declared as part of the macro will force
> > >>>>> us to have global symbols (for the cases where it is needed).
> > >>>>> I'd rather leave the declaration to the caller, and only handle the
> > >>>>> registering part in this macro.
> > >>>>
> > >>>> I agree with David that it is important to avoid global symbols
> > >>>> when it is not needed.
> > >>>
> > >>> David, Andrew,
> > >>>
> > >>> Since it is executed in "constructor" context, it will be always from
> > >>> the global variable. Right?
> > >>> i.e DPDK is not yet initialized in when "constructor" being called.
> > >>> In addition to that, It will be adding more lines of code in the
> > >>> consumer of this MACRO.
> > >>> Thoughts?
> > >>
> > >> The problem is rather 'extern' vs 'static'. Before the patch
> > >> many variables are static, but become externally visible after
> > >> the patch.
> > >
> > > OK. How about RTE_LOG_REGISTER_EXTERN or RTE_LOG_REGISTER_STATIC then?
> > > It will allow less code in the consumer of this macro.
> > > May be default we an make it as static so RTE_LOG_REGISTER and
> > > RTE_LOG_REGISTER_EXTERN
> > > for the different needs.
> > >
> > > Thoughts?
> >
> > Yes, I think it is a possible solution to use static in
> > RTE_LOG_REGISTER and use RTE_LOG_REGISTER_EXTERN
> > for non-static version. If we go this way, I'd prefer
> > the option.
>
> OK.
>
> >
> > Alternative is to keep variable declaration outside,
> > as David suggested, and I tend to agree that it is a
> > bit better. Macro name says 'register'. It is not
> > 'declare and register'. Also it avoids static-vs-extern
> > problem completely. The solution allows to keep the
> > variable declaration untouched and put constructor (macro)
> > at the end of fine where constructors typically reside.
>
> My only concern with that approach is that, We can not save a lot of
> code duplication
> with that scheme. ie. it is [1] vs [2]. We can change the MACRO name
> accordingly if that is a concern. Any suggestions?
>
> Let me know your preference on [1] vs [2], I will stick with for the
> next version.

If there are no other comments, I change RTE_LOG_REGISTER to static version
and RTE_LOG_REGISTER_EXTERN for a non-static version and send the next version.



>
> [1]
>
> 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)
>
> [2]
>
>
> int otx2_logtype_base;
> int otx2_logtype_mbox;
> int otx2_logtype_npa;
> int otx2_logtype_nix;
> int otx2_logtype_npc;
> int otx2_logtype_tm;
> int otx2_logtype_sso;
> int otx2_logtype_tim;
> int otx2_logtype_dpi;
> int otx2_logtype_ep;
>
> 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)