diff mbox

[dpdk-dev,01/11] avp: implement dynamic logging

Message ID 20171219063840.18981-2-stephen@networkplumber.org (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Stephen Hemminger Dec. 19, 2017, 6:38 a.m. UTC 
  All PMD should be using dynamic log levels.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 config/common_base           |  1 -
 drivers/net/avp/avp_ethdev.c | 10 ++++++++++
 drivers/net/avp/avp_logs.h   |  9 ++++-----
 3 files changed, 14 insertions(+), 6 deletions(-)

Comments

Ferruh Yigit Dec. 20, 2017, 1:53 a.m. UTC | #1 
On 12/18/2017 10:38 PM, Stephen Hemminger wrote:
> All PMD should be using dynamic log levels.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  config/common_base           |  1 -
>  drivers/net/avp/avp_ethdev.c | 10 ++++++++++
>  drivers/net/avp/avp_logs.h   |  9 ++++-----
>  3 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/config/common_base b/config/common_base
> index e74febef4848..f775ec96184c 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -441,7 +441,6 @@ CONFIG_RTE_LIBRTE_ARK_DEBUG_TRACE=n
>  CONFIG_RTE_LIBRTE_AVP_PMD=n
>  CONFIG_RTE_LIBRTE_AVP_DEBUG_RX=n
>  CONFIG_RTE_LIBRTE_AVP_DEBUG_TX=n
> -CONFIG_RTE_LIBRTE_AVP_DEBUG_DRIVER=y
>  CONFIG_RTE_LIBRTE_AVP_DEBUG_BUFFERS=n
>  
>  #
> diff --git a/drivers/net/avp/avp_ethdev.c b/drivers/net/avp/avp_ethdev.c
> index 9b342bfa3ae8..deb6f355d3ef 100644
> --- a/drivers/net/avp/avp_ethdev.c
> +++ b/drivers/net/avp/avp_ethdev.c
> @@ -60,6 +60,7 @@
>  
>  #include "avp_logs.h"
>  
> +int avp_logtype_driver;
>  
>  static int avp_dev_create(struct rte_pci_device *pci_dev,
>  			  struct rte_eth_dev *eth_dev);
> @@ -2312,3 +2313,12 @@ avp_dev_stats_reset(struct rte_eth_dev *eth_dev)
>  
>  RTE_PMD_REGISTER_PCI(net_avp, rte_avp_pmd);
>  RTE_PMD_REGISTER_PCI_TABLE(net_avp, pci_id_avp_map);
> +
> +RTE_INIT(avp_init_log);
> +static void
> +avp_init_log(void)
> +{
> +	avp_logtype_driver = rte_log_register("pmd.avp.driver");
> +	if (avp_logtype_driver >= 0)
> +		rte_log_set_level(avp_logtype_driver, RTE_LOG_NOTICE);
> +}

This can be done later as well but what do you think creating a macro [1], so
that driver part can become just:

PMD_INIT_LOG(avp, init, NOTICE);



[1] something like:
 #define INIT_LOG_VAR_NAME(pmd, type)   logtype_ ## pmd ## _ ## type
 #define INIT_LOG_FUNC_NAME(pmd, type)  log_ ## pmd ## _ ## type

 #define PMD_INIT_LOG(pmd, type, level)                                \
        int INIT_LOG_VAR_NAME(pmd, type);                       \
        RTE_INIT(INIT_LOG_FUNC_NAME(pmd, type));                \
        static void INIT_LOG_FUNC_NAME(pmd, type)(void)                 \
        {                                                       \
                INIT_LOG_VAR_NAME(pmd, type) = rte_log_register("pmd."
RTE_STR(pmd) "." RTE_STR(type)); \
                if (INIT_LOG_VAR_NAME(pmd, type) > 0)           \
                        rte_log_set_level(INIT_LOG_VAR_NAME(pmd, type),
RTE_LOG_##level); \
        }
Stephen Hemminger Dec. 20, 2017, 6:58 p.m. UTC | #2 
On Tue, 19 Dec 2017 17:53:50 -0800
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 12/18/2017 10:38 PM, Stephen Hemminger wrote:
> > All PMD should be using dynamic log levels.
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> >  config/common_base           |  1 -
> >  drivers/net/avp/avp_ethdev.c | 10 ++++++++++
> >  drivers/net/avp/avp_logs.h   |  9 ++++-----
> >  3 files changed, 14 insertions(+), 6 deletions(-)
> > 
> > diff --git a/config/common_base b/config/common_base
> > index e74febef4848..f775ec96184c 100644
> > --- a/config/common_base
> > +++ b/config/common_base
> > @@ -441,7 +441,6 @@ CONFIG_RTE_LIBRTE_ARK_DEBUG_TRACE=n
> >  CONFIG_RTE_LIBRTE_AVP_PMD=n
> >  CONFIG_RTE_LIBRTE_AVP_DEBUG_RX=n
> >  CONFIG_RTE_LIBRTE_AVP_DEBUG_TX=n
> > -CONFIG_RTE_LIBRTE_AVP_DEBUG_DRIVER=y
> >  CONFIG_RTE_LIBRTE_AVP_DEBUG_BUFFERS=n
> >  
> >  #
> > diff --git a/drivers/net/avp/avp_ethdev.c b/drivers/net/avp/avp_ethdev.c
> > index 9b342bfa3ae8..deb6f355d3ef 100644
> > --- a/drivers/net/avp/avp_ethdev.c
> > +++ b/drivers/net/avp/avp_ethdev.c
> > @@ -60,6 +60,7 @@
> >  
> >  #include "avp_logs.h"
> >  
> > +int avp_logtype_driver;
> >  
> >  static int avp_dev_create(struct rte_pci_device *pci_dev,
> >  			  struct rte_eth_dev *eth_dev);
> > @@ -2312,3 +2313,12 @@ avp_dev_stats_reset(struct rte_eth_dev *eth_dev)
> >  
> >  RTE_PMD_REGISTER_PCI(net_avp, rte_avp_pmd);
> >  RTE_PMD_REGISTER_PCI_TABLE(net_avp, pci_id_avp_map);
> > +
> > +RTE_INIT(avp_init_log);
> > +static void
> > +avp_init_log(void)
> > +{
> > +	avp_logtype_driver = rte_log_register("pmd.avp.driver");
> > +	if (avp_logtype_driver >= 0)
> > +		rte_log_set_level(avp_logtype_driver, RTE_LOG_NOTICE);
> > +}  
> 
> This can be done later as well but what do you think creating a macro [1], so
> that driver part can become just:
> 
> PMD_INIT_LOG(avp, init, NOTICE);
> 
> 
> 
> [1] something like:
>  #define INIT_LOG_VAR_NAME(pmd, type)   logtype_ ## pmd ## _ ## type
>  #define INIT_LOG_FUNC_NAME(pmd, type)  log_ ## pmd ## _ ## type
> 
>  #define PMD_INIT_LOG(pmd, type, level)                                \
>         int INIT_LOG_VAR_NAME(pmd, type);                       \
>         RTE_INIT(INIT_LOG_FUNC_NAME(pmd, type));                \
>         static void INIT_LOG_FUNC_NAME(pmd, type)(void)                 \
>         {                                                       \
>                 INIT_LOG_VAR_NAME(pmd, type) = rte_log_register("pmd."
> RTE_STR(pmd) "." RTE_STR(type)); \
>                 if (INIT_LOG_VAR_NAME(pmd, type) > 0)           \
>                         rte_log_set_level(INIT_LOG_VAR_NAME(pmd, type),
> RTE_LOG_##level); \
>         }

That macro is a little complex.  Also, for better or worse, the current
logging is done on a per driver basis. If we want to do something fancier
it should be in common EAL core.
Ferruh Yigit Dec. 21, 2017, 6:02 p.m. UTC | #3 
On 12/20/2017 10:58 AM, Stephen Hemminger wrote:
> On Tue, 19 Dec 2017 17:53:50 -0800
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
>> On 12/18/2017 10:38 PM, Stephen Hemminger wrote:
>>> All PMD should be using dynamic log levels.
>>>
>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

<...>

>>> @@ -2312,3 +2313,12 @@ avp_dev_stats_reset(struct rte_eth_dev *eth_dev)
>>>  
>>>  RTE_PMD_REGISTER_PCI(net_avp, rte_avp_pmd);
>>>  RTE_PMD_REGISTER_PCI_TABLE(net_avp, pci_id_avp_map);
>>> +
>>> +RTE_INIT(avp_init_log);
>>> +static void
>>> +avp_init_log(void)
>>> +{
>>> +	avp_logtype_driver = rte_log_register("pmd.avp.driver");
>>> +	if (avp_logtype_driver >= 0)
>>> +		rte_log_set_level(avp_logtype_driver, RTE_LOG_NOTICE);
>>> +}  
>>
>> This can be done later as well but what do you think creating a macro [1], so
>> that driver part can become just:
>>
>> PMD_INIT_LOG(avp, init, NOTICE);
>>
>>
>>
>> [1] something like:
>>  #define INIT_LOG_VAR_NAME(pmd, type)   logtype_ ## pmd ## _ ## type
>>  #define INIT_LOG_FUNC_NAME(pmd, type)  log_ ## pmd ## _ ## type
>>
>>  #define PMD_INIT_LOG(pmd, type, level)                                \
>>         int INIT_LOG_VAR_NAME(pmd, type);                       \
>>         RTE_INIT(INIT_LOG_FUNC_NAME(pmd, type));                \
>>         static void INIT_LOG_FUNC_NAME(pmd, type)(void)                 \
>>         {                                                       \
>>                 INIT_LOG_VAR_NAME(pmd, type) = rte_log_register("pmd."
>> RTE_STR(pmd) "." RTE_STR(type)); \
>>                 if (INIT_LOG_VAR_NAME(pmd, type) > 0)           \
>>                         rte_log_set_level(INIT_LOG_VAR_NAME(pmd, type),
>> RTE_LOG_##level); \
>>         }
> 
> That macro is a little complex.  Also, for better or worse, the current
> logging is done on a per driver basis. If we want to do something fancier
> it should be in common EAL core.

Of course, my intention was putting it into rte_log.h so updates in each driver
will be minimal. But this can be done better to cover library updates as well.
Olivier Matz Dec. 22, 2017, 1:45 p.m. UTC | #4 
On Thu, Dec 21, 2017 at 10:02:14AM -0800, Ferruh Yigit wrote:
> On 12/20/2017 10:58 AM, Stephen Hemminger wrote:
> >> [1] something like:
> >>  #define INIT_LOG_VAR_NAME(pmd, type)   logtype_ ## pmd ## _ ## type
> >>  #define INIT_LOG_FUNC_NAME(pmd, type)  log_ ## pmd ## _ ## type
> >>
> >>  #define PMD_INIT_LOG(pmd, type, level)                                \
> >>         int INIT_LOG_VAR_NAME(pmd, type);                       \
> >>         RTE_INIT(INIT_LOG_FUNC_NAME(pmd, type));                \
> >>         static void INIT_LOG_FUNC_NAME(pmd, type)(void)                 \
> >>         {                                                       \
> >>                 INIT_LOG_VAR_NAME(pmd, type) = rte_log_register("pmd."
> >> RTE_STR(pmd) "." RTE_STR(type)); \
> >>                 if (INIT_LOG_VAR_NAME(pmd, type) > 0)           \
> >>                         rte_log_set_level(INIT_LOG_VAR_NAME(pmd, type),
> >> RTE_LOG_##level); \
> >>         }
> > 
> > That macro is a little complex.  Also, for better or worse, the current
> > logging is done on a per driver basis. If we want to do something fancier
> > it should be in common EAL core.
> 
> Of course, my intention was putting it into rte_log.h so updates in each driver
> will be minimal. But this can be done better to cover library updates as well.

It's a good idea.

Below is another proposition (untested) that panics if
rte_log_register() fails, and that defines a static variable with a
predefined name.

 #define RTE_LOG_TYPE_REGISTER(name, level)                              \
         static int name##_log_type;                                     \
         __attribute__((constructor, used))                              \
         static void rte_log_register_##name(void)                       \
         {                                                               \
                 name##_log_type = rte_log_register(#name);              \
                 RTE_VERIFY(name##_log_type >= 0);                       \
                 rte_log_set_level(name##_log_type, level);              \
         }
diff mbox

Patch

diff --git a/config/common_base b/config/common_base
index e74febef4848..f775ec96184c 100644
--- a/config/common_base
+++ b/config/common_base
@@ -441,7 +441,6 @@  CONFIG_RTE_LIBRTE_ARK_DEBUG_TRACE=n
 CONFIG_RTE_LIBRTE_AVP_PMD=n
 CONFIG_RTE_LIBRTE_AVP_DEBUG_RX=n
 CONFIG_RTE_LIBRTE_AVP_DEBUG_TX=n
-CONFIG_RTE_LIBRTE_AVP_DEBUG_DRIVER=y
 CONFIG_RTE_LIBRTE_AVP_DEBUG_BUFFERS=n
 
 #
diff --git a/drivers/net/avp/avp_ethdev.c b/drivers/net/avp/avp_ethdev.c
index 9b342bfa3ae8..deb6f355d3ef 100644
--- a/drivers/net/avp/avp_ethdev.c
+++ b/drivers/net/avp/avp_ethdev.c
@@ -60,6 +60,7 @@ 
 
 #include "avp_logs.h"
 
+int avp_logtype_driver;
 
 static int avp_dev_create(struct rte_pci_device *pci_dev,
 			  struct rte_eth_dev *eth_dev);
@@ -2312,3 +2313,12 @@  avp_dev_stats_reset(struct rte_eth_dev *eth_dev)
 
 RTE_PMD_REGISTER_PCI(net_avp, rte_avp_pmd);
 RTE_PMD_REGISTER_PCI_TABLE(net_avp, pci_id_avp_map);
+
+RTE_INIT(avp_init_log);
+static void
+avp_init_log(void)
+{
+	avp_logtype_driver = rte_log_register("pmd.avp.driver");
+	if (avp_logtype_driver >= 0)
+		rte_log_set_level(avp_logtype_driver, RTE_LOG_NOTICE);
+}
diff --git a/drivers/net/avp/avp_logs.h b/drivers/net/avp/avp_logs.h
index 252cab7da2cf..e29394d5857f 100644
--- a/drivers/net/avp/avp_logs.h
+++ b/drivers/net/avp/avp_logs.h
@@ -49,11 +49,10 @@ 
 #define PMD_TX_LOG(level, fmt, args...) do { } while (0)
 #endif
 
-#ifdef RTE_LIBRTE_AVP_DEBUG_DRIVER
+extern int avp_logtype_driver;
+
 #define PMD_DRV_LOG(level, fmt, args...) \
-	RTE_LOG(level, PMD, "%s(): " fmt, __func__, ## args)
-#else
-#define PMD_DRV_LOG(level, fmt, args...) do { } while (0)
-#endif
+	rte_log(RTE_LOG_ ## level, avp_logtype_driver, \
+		"%s(): " fmt, __func__, ## args)
 
 #endif /* _AVP_LOGS_H_ */