[01/13] eal/log: introduce log register macro
diff mbox series

Message ID 20200617063047.1555518-2-jerinj@marvell.com
State Superseded
Delegated to: Thomas Monjalon
Headers show
Series
  • rte_log registration usage improvement
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-testing fail Testing issues
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-nxp-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/checkpatch success coding style OK

Commit Message

Jerin Jacob Kollanukkaran June 17, 2020, 6:30 a.m. UTC
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
it's level in the constructor context.

Signed-off-by: Jerin Jacob <jerinj@marvell.com>
---
 lib/librte_eal/include/rte_log.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

Comments

Thomas Monjalon June 17, 2020, 10 a.m. UTC | #1
17/06/2020 08:30, jerinj@marvell.com:
> 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
> it's level in the constructor context.
> 
> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> ---
> --- a/lib/librte_eal/include/rte_log.h
> +++ b/lib/librte_eal/include/rte_log.h
> +#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);	\
> +}

It should use rte_log_register_type_and_pick_level()
which works for drivers loaded later in the init sequence.

rte_log_register() should be deprecated.
David Marchand June 17, 2020, 10:02 a.m. UTC | #2
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.

> it's level in the constructor context.
>
Andrew Rybchenko June 17, 2020, 10:21 a.m. UTC | #3
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.

>> it's level in the constructor context.
>>
> 
>
Andrew Rybchenko June 17, 2020, 10:26 a.m. UTC | #4
On 6/17/20 1:00 PM, Thomas Monjalon wrote:
> 17/06/2020 08:30, jerinj@marvell.com:
>> 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
>> it's level in the constructor context.
>>
>> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
>> ---
>> --- a/lib/librte_eal/include/rte_log.h
>> +++ b/lib/librte_eal/include/rte_log.h
>> +#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);	\
>> +}
> 
> It should use rte_log_register_type_and_pick_level()
> which works for drivers loaded later in the init sequence.

Yes, rte_log_register_type_and_pick_level() should be used.
Otherwise, if a driver is loaded (the constructor is called)
after --log-level options processing, the level set using
the option is not applied.

> rte_log_register() should be deprecated.

+1
Sachin Saxena (OSS) June 21, 2020, 9:30 a.m. UTC | #5
On 17-Jun-20 12:00 PM, 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
> it's level in the constructor context.
>
> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> ---
>   lib/librte_eal/include/rte_log.h | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
>
> diff --git a/lib/librte_eal/include/rte_log.h b/lib/librte_eal/include/rte_log.h
> index 1789ede56..4dc357074 100644
> --- a/lib/librte_eal/include/rte_log.h
> +++ b/lib/librte_eal/include/rte_log.h
> @@ -376,6 +376,31 @@ int rte_vlog(uint32_t level, uint32_t logtype, const char *format, va_list ap)
>   		 RTE_LOGTYPE_ ## t, # t ": " __VA_ARGS__) :	\
>   	 0)
>   
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Register a dynamic log type in constructor context with its name and level.
> + *
> + * It is a wrapper macro for declaring the logtype, register the log and sets
> + * it's level in the constructor context.
> + *
> + * @param type
> + *   The log type identifier
> + * @param name
> + *    Name for the log type to be registered
> + * @param level
> + *   Log level. A value between EMERG (1) and DEBUG (8).
> + */
> +#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);	\
> +}
> +
>   #ifdef __cplusplus
>   }
>   #endif
Do we like to add some way of notifying the driver (may be simple print) 
regarding failure case of "rte_log_*" API?
Jerin Jacob June 24, 2020, 1:08 p.m. UTC | #6
On Wed, Jun 17, 2020 at 3:56 PM Andrew Rybchenko
<arybchenko@solarflare.com> wrote:
>
> On 6/17/20 1:00 PM, Thomas Monjalon wrote:
> > 17/06/2020 08:30, jerinj@marvell.com:
> >> 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
> >> it's level in the constructor context.
> >>
> >> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> >> ---
> >> --- a/lib/librte_eal/include/rte_log.h
> >> +++ b/lib/librte_eal/include/rte_log.h
> >> +#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);       \
> >> +}
> >
> > It should use rte_log_register_type_and_pick_level()
> > which works for drivers loaded later in the init sequence.
>
> Yes, rte_log_register_type_and_pick_level() should be used.
> Otherwise, if a driver is loaded (the constructor is called)
> after --log-level options processing, the level set using
> the option is not applied.

OK. I will change it in v2.

>
> > rte_log_register() should be deprecated.
>
> +1
>
Jerin Jacob June 24, 2020, 1:11 p.m. UTC | #7
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?


>
> >> it's level in the constructor context.
> >>
> >
> >
>
Andrew Rybchenko June 24, 2020, 3:26 p.m. UTC | #8
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.

>>
>>>> it's level in the constructor context.
>>>>
>>>
>>>
>>
Jerin Jacob June 24, 2020, 3:32 p.m. UTC | #9
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?


>
> >>
> >>>> it's level in the constructor context.
> >>>>
> >>>
> >>>
> >>
>
Andrew Rybchenko June 24, 2020, 3:43 p.m. UTC | #10
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.

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.
Jerin Jacob June 24, 2020, 6:10 p.m. UTC | #11
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)
Jerin Jacob June 26, 2020, 11:16 a.m. UTC | #12
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)
David Marchand June 26, 2020, 11:42 a.m. UTC | #13
On Fri, Jun 26, 2020 at 1:16 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > 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.

- Having a macro that does more than what its name tells is inconvenient.
The trace framework differentiates declaration and registration.
Why merge things in the log framework?

Saving one line is not worth it.


- Having components set log levels at init time in the macro is a bug to me.
This has been worked around with
rte_log_register/rte_log_register_and_pick_level but the initial
problem is that rte_log_set_level* should only be called by the user.

There should be a default level for all dpdk components (which means a
common interpretation of each level), then the user chooses which logs
he wants to see.

At the moment, let's say I am looking at a live system, by default, we have:

id 0: lib.eal, level is info
id 1: lib.malloc, level is info
id 2: lib.ring, level is info
id 3: lib.mempool, level is info
id 4: lib.timer, level is info
id 5: pmd, level is info
...
id 32: lib.bbdev, level is notice
id 33: lib.bpf, level is info
id 34: bus.dpaa, level is notice
id 35: bus.fslmc, level is notice
id 36: bus.ifpga, level is notice
id 37: bus.vdev, level is notice
id 38: bus.vmbus, level is notice
id 39: lib.cfgfile, level is info
id 40: pmd.common.dpaax, level is error
...

I enable the logs for a component, set it to debug, I get my logs.
If now I want to reset the logs to the initial state, err... well
unless I took note of it, I don't know what the default level is.
But I should not have to care.

Restarting dpdk sure is something you can do with testpmd but not with
real systems.
Jerin Jacob June 26, 2020, 12:06 p.m. UTC | #14
On Fri, Jun 26, 2020 at 5:13 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Fri, Jun 26, 2020 at 1:16 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > 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.
>
> - Having a macro that does more than what its name tells is inconvenient.

I agree. What could be that name if we want to declare and register?
RTE_LOG_DECLARE_AND_REGISTER_EXTERN?

> The trace framework differentiates declaration and registration.
> Why merge things in the log framework?

Not merging both at the functional level. it has just helper macro to
avoid duplicating the code usage pattern.

>
> Saving one line is not worth it.

It is one line per log registration.

Please check the patch change status, at
,http://mails.dpdk.org/archives/dev/2020-June/170531.html
It is:
 122 files changed, 229 insertions(+), 1322 deletions(-)

Why to add yet another 229 lines, if we can change the name.

>
>
> - Having components set log levels at init time in the macro is a bug to me.
> This has been worked around with
> rte_log_register/rte_log_register_and_pick_level but the initial
> problem is that rte_log_set_level* should only be called by the user.

I agree with the below stuff, That's is not introduced by this patch.
It is already there.
Be it macro or no macro code.

I think this patch helps to change to new scheme as it takes care of
changing the
registration part to commonplace so that we can set to the same level
in the future if
everyone agrees to it


>
> There should be a default level for all dpdk components (which means a
> common interpretation of each level), then the user chooses which logs
> he wants to see.
>
> At the moment, let's say I am looking at a live system, by default, we have:
>
> id 0: lib.eal, level is info
> id 1: lib.malloc, level is info
> id 2: lib.ring, level is info
> id 3: lib.mempool, level is info
> id 4: lib.timer, level is info
> id 5: pmd, level is info
> ...
> id 32: lib.bbdev, level is notice
> id 33: lib.bpf, level is info
> id 34: bus.dpaa, level is notice
> id 35: bus.fslmc, level is notice
> id 36: bus.ifpga, level is notice
> id 37: bus.vdev, level is notice
> id 38: bus.vmbus, level is notice
> id 39: lib.cfgfile, level is info
> id 40: pmd.common.dpaax, level is error
> ...
>
> I enable the logs for a component, set it to debug, I get my logs.
> If now I want to reset the logs to the initial state, err... well
> unless I took note of it, I don't know what the default level is.
> But I should not have to care.
>
> Restarting dpdk sure is something you can do with testpmd but not with
> real systems.
>
>
> --
> David Marchand
>
David Marchand June 26, 2020, 12:13 p.m. UTC | #15
On Fri, Jun 26, 2020 at 2:06 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Fri, Jun 26, 2020 at 5:13 PM David Marchand
> <david.marchand@redhat.com> wrote:
> >
> > On Fri, Jun 26, 2020 at 1:16 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > > 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.
> >
> > - Having a macro that does more than what its name tells is inconvenient.
>
> I agree. What could be that name if we want to declare and register?
> RTE_LOG_DECLARE_AND_REGISTER_EXTERN?

No declaration in macro.
David Marchand June 26, 2020, 12:16 p.m. UTC | #16
On Fri, Jun 26, 2020 at 2:06 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Fri, Jun 26, 2020 at 5:13 PM David Marchand
> <david.marchand@redhat.com> wrote:
> >
> > On Fri, Jun 26, 2020 at 1:16 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > > 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.
> >
> > - Having a macro that does more than what its name tells is inconvenient.
>
> I agree. What could be that name if we want to declare and register?
> RTE_LOG_DECLARE_AND_REGISTER_EXTERN?

(sorry, gmail ctrl+enter ...)

Or no declaration in macro and leave code as it is.


> > - Having components set log levels at init time in the macro is a bug to me.
> > This has been worked around with
> > rte_log_register/rte_log_register_and_pick_level but the initial
> > problem is that rte_log_set_level* should only be called by the user.
>
> I agree with the below stuff, That's is not introduced by this patch.
> It is already there.
> Be it macro or no macro code.
>
> I think this patch helps to change to new scheme as it takes care of
> changing the
> registration part to commonplace so that we can set to the same level
> in the future if
> everyone agrees to it

We will still expose this macro meaning that we will have an API breakage later.
So if we go with introducing this, let's make it good from the start.
Jerin Jacob June 26, 2020, 12:37 p.m. UTC | #17
On Fri, Jun 26, 2020 at 5:46 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Fri, Jun 26, 2020 at 2:06 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> >
> > On Fri, Jun 26, 2020 at 5:13 PM David Marchand
> > <david.marchand@redhat.com> wrote:
> > >
> > > On Fri, Jun 26, 2020 at 1:16 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > > > 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.
> > >
> > > - Having a macro that does more than what its name tells is inconvenient.
> >
> > I agree. What could be that name if we want to declare and register?
> > RTE_LOG_DECLARE_AND_REGISTER_EXTERN?
>
> (sorry, gmail ctrl+enter ...)
>
> Or no declaration in macro and leave code as it is.

Just to understand, Is it some general coding standard? or Just a preference.
I dont see something like that in the Linux kernel.
In this patch, http://patches.dpdk.org/patch/69681/, You are proposing
the declaration in macro.
See: RTE_TRACE_POINT_REGISTER

I dont see anything wrong with that. Do you have technical rationale
for the above rule?


>
>
> > > - Having components set log levels at init time in the macro is a bug to me.
> > > This has been worked around with
> > > rte_log_register/rte_log_register_and_pick_level but the initial
> > > problem is that rte_log_set_level* should only be called by the user.
> >
> > I agree with the below stuff, That's is not introduced by this patch.
> > It is already there.
> > Be it macro or no macro code.
> >
> > I think this patch helps to change to new scheme as it takes care of
> > changing the
> > registration part to commonplace so that we can set to the same level
> > in the future if
> > everyone agrees to it
>
> We will still expose this macro meaning that we will have an API breakage later.
> So if we go with introducing this, let's make it good from the start.

But Is everyone OK with removing the "level" from the register before
we think about
breaking the API later?. I see PMD uses multiple levels in the same
PMD for different
purposes.


>
>
> --
> David Marchand
>
Jerin Jacob June 29, 2020, 1:39 p.m. UTC | #18
On Fri, Jun 26, 2020 at 6:07 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Fri, Jun 26, 2020 at 5:46 PM David Marchand
> <david.marchand@redhat.com> wrote:
> >
> > On Fri, Jun 26, 2020 at 2:06 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > >
> > > On Fri, Jun 26, 2020 at 5:13 PM David Marchand
> > > <david.marchand@redhat.com> wrote:
> > > >
> > > > On Fri, Jun 26, 2020 at 1:16 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > > > > 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.
> > > >
> > > > - Having a macro that does more than what its name tells is inconvenient.
> > >
> > > I agree. What could be that name if we want to declare and register?
> > > RTE_LOG_DECLARE_AND_REGISTER_EXTERN?
> >
> > (sorry, gmail ctrl+enter ...)
> >
> > Or no declaration in macro and leave code as it is.
>
> Just to understand, Is it some general coding standard? or Just a preference.
> I dont see something like that in the Linux kernel.
> In this patch, http://patches.dpdk.org/patch/69681/, You are proposing
> the declaration in macro.
> See: RTE_TRACE_POINT_REGISTER
>
> I dont see anything wrong with that. Do you have technical rationale
> for the above rule?

David,

Waiting for your feedback on this to send the next version.
If you have a strong preference for keeping declaration out macro then
I will respin the v2 based on that. Let me know.



>
>
> >
> >
> > > > - Having components set log levels at init time in the macro is a bug to me.
> > > > This has been worked around with
> > > > rte_log_register/rte_log_register_and_pick_level but the initial
> > > > problem is that rte_log_set_level* should only be called by the user.
> > >
> > > I agree with the below stuff, That's is not introduced by this patch.
> > > It is already there.
> > > Be it macro or no macro code.
> > >
> > > I think this patch helps to change to new scheme as it takes care of
> > > changing the
> > > registration part to commonplace so that we can set to the same level
> > > in the future if
> > > everyone agrees to it
> >
> > We will still expose this macro meaning that we will have an API breakage later.
> > So if we go with introducing this, let's make it good from the start.
>
> But Is everyone OK with removing the "level" from the register before
> we think about
> breaking the API later?. I see PMD uses multiple levels in the same
> PMD for different
> purposes.
>
>
> >
> >
> > --
> > David Marchand
> >
David Marchand June 30, 2020, 1:39 p.m. UTC | #19
On Fri, Jun 26, 2020 at 2:38 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > Or no declaration in macro and leave code as it is.
>
> I dont see anything wrong with that. Do you have technical rationale
> for the above rule?

Avoid global symbols when unneeded.
Global symbols can be hidden with shared build, but that's not the
case with static build.

This is why we try to maintain a namespace for DPDK api.

For logtypes, a lot of those are already global and nobody complained
about them (lucky ?).

With this patch, the static ones, that we would convert to global
symbols, are the following:
drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c:static int
fpga_5gnr_fec_logtype;
drivers/baseband/fpga_lte_fec/fpga_lte_fec.c:static int fpga_lte_fec_logtype;
drivers/baseband/null/bbdev_null.c:static int bbdev_null_logtype;
drivers/baseband/turbo_sw/bbdev_turbo_software.c:static int
bbdev_turbo_sw_logtype;
drivers/net/af_packet/rte_eth_af_packet.c:static int af_packet_logtype;
drivers/net/af_xdp/rte_eth_af_xdp.c:static int af_xdp_logtype;
drivers/net/kni/rte_eth_kni.c:static int eth_kni_logtype;
drivers/net/null/rte_eth_null.c:static int eth_null_logtype;
drivers/net/pcap/rte_eth_pcap.c:static int eth_pcap_logtype;
drivers/net/ring/rte_eth_ring.c:static int eth_ring_logtype;
drivers/net/softnic/rte_eth_softnic.c:static int pmd_softnic_logtype;
drivers/net/vdev_netvsc/vdev_netvsc.c:static int vdev_netvsc_logtype;
drivers/net/vhost/rte_eth_vhost.c:static int vhost_logtype;
drivers/vdpa/ifc/ifcvf_vdpa.c:static int ifcvf_vdpa_logtype;
lib/librte_bbdev/rte_bbdev.c:static int bbdev_logtype;
lib/librte_cfgfile/rte_cfgfile.c:static int cfgfile_logtype;
lib/librte_eventdev/rte_event_timer_adapter.c:static int evtim_logtype;
lib/librte_eventdev/rte_event_timer_adapter.c:static int evtim_buffer_logtype;
lib/librte_eventdev/rte_event_timer_adapter.c:static int evtim_svc_logtype;
lib/librte_pdump/rte_pdump.c:static int pdump_logtype;

All of them follow some kind of convention of finishing with _logtype.
Is this enough and we won't trigger link issues in existing applications?
Probably not possible to tell.

There was a similar discussion with Gaetan in the pci bus code, and so
far we are still in this status quo for global symbols.

So ok, let's go with this embedded global symbol.
If we hit an issue with static linking, we will need a tree-wide cleanup.


> > > > - Having components set log levels at init time in the macro is a bug to me.
> > > > This has been worked around with
> > > > rte_log_register/rte_log_register_and_pick_level but the initial
> > > > problem is that rte_log_set_level* should only be called by the user.
> > >
> > > I agree with the below stuff, That's is not introduced by this patch.
> > > It is already there.
> > > Be it macro or no macro code.
> > >
> > > I think this patch helps to change to new scheme as it takes care of
> > > changing the
> > > registration part to commonplace so that we can set to the same level
> > > in the future if
> > > everyone agrees to it
> >
> > We will still expose this macro meaning that we will have an API breakage later.
> > So if we go with introducing this, let's make it good from the start.
>
> But Is everyone OK with removing the "level" from the register before
> we think about
> breaking the API later?. I see PMD uses multiple levels in the same
> PMD for different
> purposes.

It is an API fix to me, as there should be no place for "level" interpretation.


Just a note, a v2 would be necessary for the
rte_log_register_type_and_pick_level use in any case.
Jerin Jacob June 30, 2020, 2:42 p.m. UTC | #20
On Tue, Jun 30, 2020 at 7:09 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Fri, Jun 26, 2020 at 2:38 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > Or no declaration in macro and leave code as it is.
> >
> > I dont see anything wrong with that. Do you have technical rationale
> > for the above rule?
>
> Avoid global symbols when unneeded.
> Global symbols can be hidden with shared build, but that's not the
> case with static build.
>
> This is why we try to maintain a namespace for DPDK api.
>
> For logtypes, a lot of those are already global and nobody complained
> about them (lucky ?).
>
> With this patch, the static ones, that we would convert to global
> symbols, are the following:
> drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c:static int
> fpga_5gnr_fec_logtype;
> drivers/baseband/fpga_lte_fec/fpga_lte_fec.c:static int fpga_lte_fec_logtype;
> drivers/baseband/null/bbdev_null.c:static int bbdev_null_logtype;
> drivers/baseband/turbo_sw/bbdev_turbo_software.c:static int
> bbdev_turbo_sw_logtype;
> drivers/net/af_packet/rte_eth_af_packet.c:static int af_packet_logtype;
> drivers/net/af_xdp/rte_eth_af_xdp.c:static int af_xdp_logtype;
> drivers/net/kni/rte_eth_kni.c:static int eth_kni_logtype;
> drivers/net/null/rte_eth_null.c:static int eth_null_logtype;
> drivers/net/pcap/rte_eth_pcap.c:static int eth_pcap_logtype;
> drivers/net/ring/rte_eth_ring.c:static int eth_ring_logtype;
> drivers/net/softnic/rte_eth_softnic.c:static int pmd_softnic_logtype;
> drivers/net/vdev_netvsc/vdev_netvsc.c:static int vdev_netvsc_logtype;
> drivers/net/vhost/rte_eth_vhost.c:static int vhost_logtype;
> drivers/vdpa/ifc/ifcvf_vdpa.c:static int ifcvf_vdpa_logtype;
> lib/librte_bbdev/rte_bbdev.c:static int bbdev_logtype;
> lib/librte_cfgfile/rte_cfgfile.c:static int cfgfile_logtype;
> lib/librte_eventdev/rte_event_timer_adapter.c:static int evtim_logtype;
> lib/librte_eventdev/rte_event_timer_adapter.c:static int evtim_buffer_logtype;
> lib/librte_eventdev/rte_event_timer_adapter.c:static int evtim_svc_logtype;
> lib/librte_pdump/rte_pdump.c:static int pdump_logtype;
>
> All of them follow some kind of convention of finishing with _logtype.
> Is this enough and we won't trigger link issues in existing applications?
> Probably not possible to tell.
>
> There was a similar discussion with Gaetan in the pci bus code, and so
> far we are still in this status quo for global symbols.
>
> So ok, let's go with this embedded global symbol.

OK.

> If we hit an issue with static linking, we will need a tree-wide cleanup.
>
>
> > > > > - Having components set log levels at init time in the macro is a bug to me.
> > > > > This has been worked around with
> > > > > rte_log_register/rte_log_register_and_pick_level but the initial
> > > > > problem is that rte_log_set_level* should only be called by the user.
> > > >
> > > > I agree with the below stuff, That's is not introduced by this patch.
> > > > It is already there.
> > > > Be it macro or no macro code.
> > > >
> > > > I think this patch helps to change to new scheme as it takes care of
> > > > changing the
> > > > registration part to commonplace so that we can set to the same level
> > > > in the future if
> > > > everyone agrees to it
> > >
> > > We will still expose this macro meaning that we will have an API breakage later.
> > > So if we go with introducing this, let's make it good from the start.
> >
> > But Is everyone OK with removing the "level" from the register before
> > we think about
> > breaking the API later?. I see PMD uses multiple levels in the same
> > PMD for different
> > purposes.
>
> It is an API fix to me, as there should be no place for "level" interpretation.
>
>
> Just a note, a v2 would be necessary for the
> rte_log_register_type_and_pick_level use in any case.

I will send v2 with rte_log_register_type_and_pick_level change and
rebase to ToT.

>
> --
> David Marchand
>

Patch
diff mbox series

diff --git a/lib/librte_eal/include/rte_log.h b/lib/librte_eal/include/rte_log.h
index 1789ede56..4dc357074 100644
--- a/lib/librte_eal/include/rte_log.h
+++ b/lib/librte_eal/include/rte_log.h
@@ -376,6 +376,31 @@  int rte_vlog(uint32_t level, uint32_t logtype, const char *format, va_list ap)
 		 RTE_LOGTYPE_ ## t, # t ": " __VA_ARGS__) :	\
 	 0)
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Register a dynamic log type in constructor context with its name and level.
+ *
+ * It is a wrapper macro for declaring the logtype, register the log and sets
+ * it's level in the constructor context.
+ *
+ * @param type
+ *   The log type identifier
+ * @param name
+ *    Name for the log type to be registered
+ * @param level
+ *   Log level. A value between EMERG (1) and DEBUG (8).
+ */
+#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);	\
+}
+
 #ifdef __cplusplus
 }
 #endif