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

[Chaoyong He <chaoyong.he@corigine.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.