Re: [PATCH 2/3] net/nfp: get rid of the usage of RTE log level type

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 2/20/2023 1:36 AM, Chaoyong He wrote:
>> On 2/17/2023 2:45 AM, Chaoyong He wrote:
>>> Register the own RX/TX debug log level type, and get rid of the usage
>>> of RTE_LOGTYPE_*. Then we can control the log by a independent switch.
>>>
>>> Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
>>> ---
>>>  drivers/net/nfp/nfp_logs.c | 10 ++++++++++
>>> drivers/net/nfp/nfp_logs.h |  8 ++++++--
>>>  2 files changed, 16 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/nfp/nfp_logs.c b/drivers/net/nfp/nfp_logs.c
>>> index 48c42fe53f..cd58bcee43 100644
>>> --- a/drivers/net/nfp/nfp_logs.c
>>> +++ b/drivers/net/nfp/nfp_logs.c
>>> @@ -5,6 +5,16 @@
>>>
>>>  #include "nfp_logs.h"
>>>
>>> +#include <rte_ethdev.h>
>>> +
>>>  RTE_LOG_REGISTER_SUFFIX(nfp_logtype_init, init, NOTICE);
>>> RTE_LOG_REGISTER_SUFFIX(nfp_logtype_driver, driver, NOTICE);
>>> RTE_LOG_REGISTER_SUFFIX(nfp_logtype_cpp, cpp, NOTICE);
>>> +
>>> +#ifdef RTE_ETHDEV_DEBUG_RX
>>> +RTE_LOG_REGISTER_SUFFIX(nfp_logtype_rx, rx, DEBUG) #endif
>>> +
>>> +#ifdef RTE_ETHDEV_DEBUG_TX
>>> +RTE_LOG_REGISTER_SUFFIX(nfp_logtype_tx, tx, DEBUG) #endif
>>> diff --git a/drivers/net/nfp/nfp_logs.h b/drivers/net/nfp/nfp_logs.h
>>> index b7632ee72c..315a57811c 100644
>>> --- a/drivers/net/nfp/nfp_logs.h
>>> +++ b/drivers/net/nfp/nfp_logs.h
>>> @@ -15,15 +15,19 @@ extern int nfp_logtype_init;  #define
>>> PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>")
>>>
>>>  #ifdef RTE_ETHDEV_DEBUG_RX
>>> +extern int nfp_logtype_rx;
>>>  #define PMD_RX_LOG(level, fmt, args...) \
>>> -	RTE_LOG(level, PMD, "%s() rx: " fmt "\n", __func__, ## args)
>>> +	rte_log(RTE_LOG_ ## level, nfp_logtype_rx, \
>>> +		"%s(): " fmt "\n", __func__, ## args)
>>>  #else
>>>  #define PMD_RX_LOG(level, fmt, args...) do { } while (0)  #endif
>>>
>>>  #ifdef RTE_ETHDEV_DEBUG_TX
>>> +extern int nfp_logtype_tx;
>>>  #define PMD_TX_LOG(level, fmt, args...) \
>>> -	RTE_LOG(level, PMD, "%s() tx: " fmt "\n", __func__, ## args)
>>> +	rte_log(RTE_LOG_ ## level, nfp_logtype_tx, \
>>> +		"%s(): " fmt "\n", __func__, ## args)
>>>  #else
>>>  #define PMD_TX_LOG(level, fmt, args...) do { } while (0)  #endif
>>
>> Intention is to replace 'RTE_LOG_DP' with 'PMD_RX_LOG'/'PMD_TX_LOG',
>> but these are not exactly same (although difference is minor).
>>
>> When 'RTE_ETHDEV_DEBUG_RX' is set, ethdev layer also adds some
>> additional load, although I believe that will small comparing to logging in
>> driver.
>> If 'RTE_LOG_DP' used, the ethdev layer cost can be removed.
>>
>> With 'RTE_LOG_DP', log level more verbose than requested won't cause any
>> performance impact. Like if ERR level requested, INFO, DEBUG etc logs will
>> be compiled out and won't cause any performance impact.
>> But with 'RTE_ETHDEV_DEBUG_RX', even log level only request ERR, all
>> logging will add cost of at least an if branch (checking log level).
>>
>>
>> For many cases I am not sure these differences matters, and already many
>> drivers directly uses 'RTE_ETHDEV_DEBUG_RX' as done here. So you may
>> prefer to keep as it is.
>>
>> But if there is a desire for this fine grain approach, it is possible to add a
>> version of 'RTE_LOG_DP' macro that accepts dynamic log type (instead of
>> static RTE_LOGTYPE_# type), what do you think?
>>
> 
> Thanks for the suggestion.
> For now, we prefer to keep as it is.
> If we does need the more refined design in the future, we would follow your advice here, thanks again.

ack, I just wanted to double check. I will proceed as it is.