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

[Jerin Jacob <jerinjacobk@gmail.com>]

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.

[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)