From: Ferruh Yigit <ferruh.yigit@amd.com>
To: Stephen Hemminger <stephen@networkplumber.org>
Cc: Chaoyong He <chaoyong.he@corigine.com>,
"dev@dpdk.org" <dev@dpdk.org>,
oss-drivers <oss-drivers@corigine.com>,
Niklas Soderlund <niklas.soderlund@corigine.com>
Subject: Re: [PATCH 2/3] net/nfp: get rid of the usage of RTE log level type
Date: Mon, 20 Feb 2023 16:32:37 +0000 [thread overview]
Message-ID: <5076e27d-b356-2e67-0885-daf325d7faf8@amd.com> (raw)
In-Reply-To: <20230220081641.623a2a97@hermes.local>
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.
next prev parent reply other threads:[~2023-02-20 16:32 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-17 2:45 [PATCH 0/3] refactor the nfp log subsystem Chaoyong He
2023-02-17 2:45 ` [PATCH 1/3] net/nfp: add the log source file Chaoyong He
2023-02-17 2:45 ` [PATCH 2/3] net/nfp: get rid of the usage of RTE log level type Chaoyong He
2023-02-17 13:59 ` Ferruh Yigit
2023-02-20 1:36 ` Chaoyong He
2023-02-20 10:09 ` Ferruh Yigit
2023-02-20 16:16 ` Stephen Hemminger
2023-02-20 16:32 ` Ferruh Yigit [this message]
2023-02-17 2:45 ` [PATCH 3/3] net/nfp: get rid of the usage of RTE log macro Chaoyong He
2023-02-20 12:03 ` [PATCH 0/3] refactor the nfp log subsystem Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5076e27d-b356-2e67-0885-daf325d7faf8@amd.com \
--to=ferruh.yigit@amd.com \
--cc=chaoyong.he@corigine.com \
--cc=dev@dpdk.org \
--cc=niklas.soderlund@corigine.com \
--cc=oss-drivers@corigine.com \
--cc=stephen@networkplumber.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).