From: Chaoyong He <chaoyong.he@corigine.com>
To: Ferruh Yigit <ferruh.yigit@amd.com>, "dev@dpdk.org" <dev@dpdk.org>
Cc: 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 01:36:33 +0000 [thread overview]
Message-ID: <SJ0PR13MB55450BDC118EE0D7D56CB1489EA49@SJ0PR13MB5545.namprd13.prod.outlook.com> (raw)
In-Reply-To: <f8fc36a5-ba36-fd6a-4a08-9f22ea335898@amd.com>
> 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.
next prev parent reply other threads:[~2023-02-20 1:36 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 [this message]
2023-02-20 10:09 ` Ferruh Yigit
2023-02-20 16:16 ` Stephen Hemminger
2023-02-20 16:32 ` Ferruh Yigit
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=SJ0PR13MB55450BDC118EE0D7D56CB1489EA49@SJ0PR13MB5545.namprd13.prod.outlook.com \
--to=chaoyong.he@corigine.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.com \
--cc=niklas.soderlund@corigine.com \
--cc=oss-drivers@corigine.com \
/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).