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 4:16 PM, Stephen Hemminger wrote:
> On Mon, 20 Feb 2023 10:09:51 +0000
> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> 
>> 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.
> 
> As part of my patch series (work in progress) to get rid of RTE_LOGTYPE_PMD
> needed to add a helper now for RTE_DP_LOG like this.
> 
> diff --git a/lib/eal/include/rte_log.h b/lib/eal/include/rte_log.h
> index 6d2b0856a565..f377bc6db79b 100644
> --- a/lib/eal/include/rte_log.h
> +++ b/lib/eal/include/rte_log.h
> @@ -336,6 +336,37 @@ int rte_vlog(uint32_t level, uint32_t logtype, const char *format, va_list ap)
>  	 rte_log(RTE_LOG_ ## l,					\
>  		 RTE_LOGTYPE_ ## t, # t ": " __VA_ARGS__)
>  
> +/**
> + * Generates a log message for data path.
> + *
> + * Similar to rte_log(), except that it is an inline function that
> + * can be eliminated at compilation time if RTE_LOG_DP_LEVEL configuration
> + * option is lower than the log level argument.
> + *
> + * @param level
> + *   Log level. A value between RTE_LOG_EMERG (1) and RTE_LOG_DEBUG (8).
> + * @param logtype
> + *   The log type, for example, RTE_LOGTYPE_EAL.
> + * @param format
> + *   The format string, as in printf(3), followed by the variable arguments
> + *   required by the format.
> + * @return
> + *   - 0: Success.
> + *   - Negative on error.
> + */
> +static inline __rte_format_printf(3, 4)
> +void rte_log_dp(uint32_t level, uint32_t logtype, const char *format, ...)
> +
> +{
> +	va_list ap;
> +
> +	if (level <= RTE_LOG_DP_LEVEL) {
> +		va_start(ap, format);
> +		rte_vlog(level, logtype, format, ap);
> +		va_end(ap);
> +	}
> +}
> +
>  /**
>   * Generates a log message for data path.
>   *
> @@ -357,10 +388,8 @@ int rte_vlog(uint32_t level, uint32_t logtype, const char *format, va_list ap)
>   *   - Negative on error.
>   */
>  #define RTE_LOG_DP(l, t, ...)					\
> -	(void)((RTE_LOG_ ## l <= RTE_LOG_DP_LEVEL) ?		\
> -	 rte_log(RTE_LOG_ ## l,					\
> -		 RTE_LOGTYPE_ ## t, # t ": " __VA_ARGS__) :	\
> -	 0)
> +	rte_log_dp(RTE_LOG_ ## l,				\
> +		 RTE_LOGTYPE_ ## t, # t ": " __VA_ARGS__)
>  


+1 to have 'rte_log_dp()' as above, this way data path logs can be added
with dynamic log types.