On 2/20/2023 4:16 PM, Stephen Hemminger wrote:
> On Mon, 20 Feb 2023 10:09:51 +0000
> Ferruh Yigit <ferruh.yi...@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...@corigine.com>
>>>>> Reviewed-by: Niklas Söderlund <niklas.soderl...@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.
