Re: [dpdk-dev] [PATCH] log: do not drop debug logs at compile time

["Wiles, Keith" <keith.wiles@intel.com>]

Regards,
Keith

> On Oct 3, 2016, at 10:37 AM, Olivier Matz <olivier.matz@6wind.com> wrote:
> 
> 
> 
> On 10/03/2016 05:27 PM, Wiles, Keith wrote:
>> 
>> Regards,
>> Keith
>> 
>>> On Oct 3, 2016, at 10:02 AM, Olivier Matz <olivier.matz@6wind.com> wrote:
>>> 
>>> Hi Keith,
>>> 
>>> On 09/30/2016 05:48 PM, Wiles, Keith wrote:
>>>>> On Sep 30, 2016, at 4:33 AM, Thomas Monjalon <thomas.monjalon@6wind.com> wrote:
>>>>> 
>>>>> 2016-09-16 09:43, Olivier Matz:
>>>>>> Today, all logs whose level is lower than INFO are dropped at
>>>>>> compile-time. This prevents from enabling debug logs at runtime using
>>>>>> --log-level=8.
>>>>>> 
>>>>>> The rationale was to remove debug logs from the data path at
>>>>>> compile-time, avoiding a test at run-time.
>>>>>> 
>>>>>> This patch changes the behavior of RTE_LOG() to avoid the compile-time
>>>>>> optimization, and introduces the RTE_LOG_DP() macro that has the same
>>>>>> behavior than the previous RTE_LOG(), for the rare cases where debug
>>>>>> logs are in the data path.
>>>>>> 
>>>>>> So it is now possible to enable debug logs at run-time by just
>>>>>> specifying --log-level=8. Some drivers still have special compile-time
>>>>>> options to enable more debug log. Maintainers may consider to
>>>>>> remove/reduce them.
>>>>>> 
>>>>>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>>>>> 
>>>>> I think it is a good change.
>>>>> However I'm not sure we should take it for 16.11 as it was sent late and
>>>>> there is no review comment.
>>>>> It is neither really a fix nor really a feature.
>>>>> If there are some +1, and no opinions against, it will go in 16.11.
>>>>> Note that some drivers would need some changes to fully benefit of
>>>>> debug logs enabled at run-time.
>>>> 
>>>> Would this be easier to add a new LOG level instead say DEBUG_DATAPATH and then change the RTE_LOG to exclude the new log level?
>>>> 
>>>> 
>>> 
>>> The log levels are quite standard, I don't feel it would be very clear
>>> to have a new level for that. It would also prevent to have different
>>> log level inside data path.
>> 
>> I am not following you here. Having one more log level for DEBUG in the data path is not a big change and you can still have any other log level in the data or anyplace else for that matter.
> 
> Adding a new log level is not a big change, you are right.
> But to me it looks confusing to have DEBUG, INFO, ..., WARNING, ERROR,
> plus a DEBUG_DATAPATH. For instance, how do you compare levels? Or if
> your log stream forwards logs to syslog, you cannot do a 1:1 mapping
> with standard syslog levels.

Doing 1:1 mapping is not a big problem unless you are trying to compare to old logs, which maybe the case the first time and after that it should not be a problem.

> 
> What makes you feel it's easier to add a log level instead of adding a
> new RTE_LOG_DP() function?

It seems to me the log levels are for displaying logs at different levels adding a new macro to not log is just a hack because we do not have a log level for data path. This is why I would like to see a log level added and not a new macro.

It also appears the new RTE_LOG() will always be in the code as you moved the test to the RTE_LOG_DP() macro. This would mean all RTE_LOG() in the code will always call rte_log(), correct?

If using a new DEBUG_DP (maybe DATAPATH is a better log level name) level we can use the same macro as before and modify the level only. This way we can remove via the compiler any log that is below the default RTE_LOG_LEVEL. I see keeping the rte_log() could be a performance problem or code blot when you really want to remove them all.

The DATAPATH log level would be above (smaller number) then DEBUG in the enum list. To remove all debug logs just set the RTE_LOG_LEVEL to RTE_LOG_DATAPATH.


> 
> 
> Regards,
> Olivier