diff mbox series

[1/6] net/e1000: use dynamic log type for tx/rx debug

Message ID 20190716154013.6974-2-stephen@networkplumber.org (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series replace usage of LOGTYPE_PMD in Intel drivers |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS
ci/Intel-compilation success Compilation OK

Commit Message

Stephen Hemminger July 16, 2019, 3:40 p.m. UTC 
  The generic RTE_LOGTYPE_PMD is a historical relic and should
not be used. Every driver should register the logtypes
for itself.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/e1000/e1000_logs.c | 48 +++++++++++++++++++++++++++-------
 drivers/net/e1000/e1000_logs.h | 25 +++++++++++-------
 2 files changed, 55 insertions(+), 18 deletions(-)

Comments

Ferruh Yigit Aug. 27, 2019, 8:21 a.m. UTC | #1 
On 7/16/2019 4:40 PM, Stephen Hemminger wrote:
> The generic RTE_LOGTYPE_PMD is a historical relic and should
> not be used. Every driver should register the logtypes
> for itself.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

<...>

> +#ifdef RTE_LIBRTE_E1000_DEBUG_RX
> +	e1000_logtype_rx = rte_log_register("pmd.net.e1000.rx");
> +	if (e1000_logtype_rx >= 0)
> +		rte_log_set_level(e1000_logtype_rx, RTE_LOG_NOTICE);

What do you think setting default level for data path log level to
'RTE_LOG_DEBUG' since they are already controlled by a config option, and to
keep the behavior consistent with previous usage, because almost all macros are
called with DEBUG level. Same for all drivers in this patchset.
Xiaolong Ye Sept. 30, 2019, 3:28 p.m. UTC | #2 
On 08/27, Ferruh Yigit wrote:
>On 7/16/2019 4:40 PM, Stephen Hemminger wrote:
>> The generic RTE_LOGTYPE_PMD is a historical relic and should
>> not be used. Every driver should register the logtypes
>> for itself.
>> 
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>
><...>
>
>> +#ifdef RTE_LIBRTE_E1000_DEBUG_RX
>> +	e1000_logtype_rx = rte_log_register("pmd.net.e1000.rx");
>> +	if (e1000_logtype_rx >= 0)
>> +		rte_log_set_level(e1000_logtype_rx, RTE_LOG_NOTICE);
>
>What do you think setting default level for data path log level to
>'RTE_LOG_DEBUG' since they are already controlled by a config option, and to
>keep the behavior consistent with previous usage, because almost all macros are
>called with DEBUG level. Same for all drivers in this patchset.

+1

Thanks,
Xiaolong
Stephen Hemminger Sept. 30, 2019, 3:50 p.m. UTC | #3 
On Mon, 30 Sep 2019 23:28:38 +0800
Ye Xiaolong <xiaolong.ye@intel.com> wrote:

> On 08/27, Ferruh Yigit wrote:
> >On 7/16/2019 4:40 PM, Stephen Hemminger wrote:  
> >> The generic RTE_LOGTYPE_PMD is a historical relic and should
> >> not be used. Every driver should register the logtypes
> >> for itself.
> >> 
> >> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>  
> >
> ><...>
> >  
> >> +#ifdef RTE_LIBRTE_E1000_DEBUG_RX
> >> +	e1000_logtype_rx = rte_log_register("pmd.net.e1000.rx");
> >> +	if (e1000_logtype_rx >= 0)
> >> +		rte_log_set_level(e1000_logtype_rx, RTE_LOG_NOTICE);  
> >
> >What do you think setting default level for data path log level to
> >'RTE_LOG_DEBUG' since they are already controlled by a config option, and to
> >keep the behavior consistent with previous usage, because almost all macros are
> >called with DEBUG level. Same for all drivers in this patchset.  
> 
> +1
> 
> Thanks,
> Xiaolong

Or drop DEBUG_RX and DEBUG_TX  completely since they are not useful anymore.
The code works, and distributions can not enable this.
Ferruh Yigit Sept. 30, 2019, 3:56 p.m. UTC | #4 
On 9/30/2019 4:50 PM, Stephen Hemminger wrote:
> On Mon, 30 Sep 2019 23:28:38 +0800
> Ye Xiaolong <xiaolong.ye@intel.com> wrote:
> 
>> On 08/27, Ferruh Yigit wrote:
>>> On 7/16/2019 4:40 PM, Stephen Hemminger wrote:  
>>>> The generic RTE_LOGTYPE_PMD is a historical relic and should
>>>> not be used. Every driver should register the logtypes
>>>> for itself.
>>>>
>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>  
>>>
>>> <...>
>>>  
>>>> +#ifdef RTE_LIBRTE_E1000_DEBUG_RX
>>>> +	e1000_logtype_rx = rte_log_register("pmd.net.e1000.rx");
>>>> +	if (e1000_logtype_rx >= 0)
>>>> +		rte_log_set_level(e1000_logtype_rx, RTE_LOG_NOTICE);  
>>>
>>> What do you think setting default level for data path log level to
>>> 'RTE_LOG_DEBUG' since they are already controlled by a config option, and to
>>> keep the behavior consistent with previous usage, because almost all macros are
>>> called with DEBUG level. Same for all drivers in this patchset.  
>>
>> +1
>>
>> Thanks,
>> Xiaolong
> 
> Or drop DEBUG_RX and DEBUG_TX  completely since they are not useful anymore.
> The code works, and distributions can not enable this.
> 

Distributions can't enable this, but still can be useful for the debug. I am
planning to update the default log level to DEBUG and merge it, any objection?
Ferruh Yigit Sept. 30, 2019, 4:10 p.m. UTC | #5 
On 8/27/2019 9:21 AM, Ferruh Yigit wrote:
> On 7/16/2019 4:40 PM, Stephen Hemminger wrote:
>> The generic RTE_LOGTYPE_PMD is a historical relic and should
>> not be used. Every driver should register the logtypes
>> for itself.
>>
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> <...>
> 
>> +#ifdef RTE_LIBRTE_E1000_DEBUG_RX
>> +	e1000_logtype_rx = rte_log_register("pmd.net.e1000.rx");
>> +	if (e1000_logtype_rx >= 0)
>> +		rte_log_set_level(e1000_logtype_rx, RTE_LOG_NOTICE);
> 
> What do you think setting default level for data path log level to
> 'RTE_LOG_DEBUG' since they are already controlled by a config option, and to
> keep the behavior consistent with previous usage, because almost all macros are
> called with DEBUG level. Same for all drivers in this patchset.
> 

For series,
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Series applied to dpdk-next-net/master, thanks.

(Default log level for datapath change to RTE_LOG_DEBUG while merging.)
diff mbox series

Patch

diff --git a/drivers/net/e1000/e1000_logs.c b/drivers/net/e1000/e1000_logs.c
index 22173939f5e5..05f883dd201b 100644
--- a/drivers/net/e1000/e1000_logs.c
+++ b/drivers/net/e1000/e1000_logs.c
@@ -8,19 +8,49 @@ 
 int e1000_logtype_init;
 int e1000_logtype_driver;
 
+#ifdef RTE_LIBRTE_E1000_DEBUG_RX
+int e1000_logtype_rx;
+#endif
+#ifdef RTE_LIBRTE_E1000_DEBUG_TX
+int e1000_logtype_tx;
+#endif
+#ifdef RTE_LIBRTE_E1000_DEBUG_TX_FREE
+int e1000_logtype_tx_free;
+#endif
+
 /* avoids double registering of logs if EM and IGB drivers are in use */
 static int e1000_log_initialized;
 
 void
 e1000_igb_init_log(void)
 {
-	if (!e1000_log_initialized) {
-		e1000_logtype_init = rte_log_register("pmd.net.e1000.init");
-		if (e1000_logtype_init >= 0)
-			rte_log_set_level(e1000_logtype_init, RTE_LOG_NOTICE);
-		e1000_logtype_driver = rte_log_register("pmd.net.e1000.driver");
-		if (e1000_logtype_driver >= 0)
-			rte_log_set_level(e1000_logtype_driver, RTE_LOG_NOTICE);
-		e1000_log_initialized = 1;
-	}
+	if (e1000_log_initialized)
+		return;
+
+	e1000_logtype_init = rte_log_register("pmd.net.e1000.init");
+	if (e1000_logtype_init >= 0)
+		rte_log_set_level(e1000_logtype_init, RTE_LOG_NOTICE);
+	e1000_logtype_driver = rte_log_register("pmd.net.e1000.driver");
+	if (e1000_logtype_driver >= 0)
+		rte_log_set_level(e1000_logtype_driver, RTE_LOG_NOTICE);
+
+#ifdef RTE_LIBRTE_E1000_DEBUG_RX
+	e1000_logtype_rx = rte_log_register("pmd.net.e1000.rx");
+	if (e1000_logtype_rx >= 0)
+		rte_log_set_level(e1000_logtype_rx, RTE_LOG_NOTICE);
+#endif
+
+#ifdef RTE_LIBRTE_E1000_DEBUG_TX
+	e1000_logtype_tx = rte_log_register("pmd.net.e1000.tx");
+	if (e1000_logtype_tx >= 0)
+		rte_log_set_level(e1000_logtype_tx, RTE_LOG_NOTICE);
+#endif
+
+#ifdef RTE_LIBRTE_E1000_DEBUG_TX_FREE
+	e1000_logtype_tx_free = rte_log_register("pmd.net.e1000.tx_free");
+	if (e1000_logtype_tx_free >= 0)
+		rte_log_set_level(e1000_logtype_tx_free, RTE_LOG_NOTICE);
+#endif
+
+	e1000_log_initialized = 1;
 }
diff --git a/drivers/net/e1000/e1000_logs.h b/drivers/net/e1000/e1000_logs.h
index 69d3d31186e0..2612134f38a0 100644
--- a/drivers/net/e1000/e1000_logs.h
+++ b/drivers/net/e1000/e1000_logs.h
@@ -8,6 +8,7 @@ 
 #include <rte_log.h>
 
 extern int e1000_logtype_init;
+
 #define PMD_INIT_LOG(level, fmt, args...) \
 	rte_log(RTE_LOG_ ## level, e1000_logtype_init, \
 		"%s(): " fmt "\n", __func__, ##args)
@@ -15,24 +16,30 @@  extern int e1000_logtype_init;
 #define PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>")
 
 #ifdef RTE_LIBRTE_E1000_DEBUG_RX
-#define PMD_RX_LOG(level, fmt, args...) \
-	RTE_LOG(level, PMD, "%s(): " fmt "\n", __func__, ## args)
+extern int e1000_logtype_rx;
+#define PMD_RX_LOG(level, fmt, args...)			\
+	rte_log(RTE_LOG_ ## level, e1000_logtype_rx,	\
+		"%s(): " fmt "\n", __func__, ## args)
 #else
-#define PMD_RX_LOG(level, fmt, args...) do { } while(0)
+#define PMD_RX_LOG(level, fmt, args...) do { } while (0)
 #endif
 
 #ifdef RTE_LIBRTE_E1000_DEBUG_TX
-#define PMD_TX_LOG(level, fmt, args...) \
-	RTE_LOG(level, PMD, "%s(): " fmt "\n", __func__, ## args)
+extern int e1000_logtype_tx;
+#define PMD_TX_LOG(level, fmt, args...)			\
+	rte_log(RTE_LOG_ ## level, e1000_logtype_tx,	\
+		"%s(): " fmt "\n", __func__, ## args)
 #else
-#define PMD_TX_LOG(level, fmt, args...) do { } while(0)
+#define PMD_TX_LOG(level, fmt, args...) do { } while (0)
 #endif
 
 #ifdef RTE_LIBRTE_E1000_DEBUG_TX_FREE
-#define PMD_TX_FREE_LOG(level, fmt, args...) \
-	RTE_LOG(level, PMD, "%s(): " fmt "\n", __func__, ## args)
+extern int e1000_logtype_tx_free;
+#define PMD_TX_FREE_LOG(level, fmt, args...)			\
+	rte_log(RTE_LOG_ ## level, e1000_logtype_tx_free,	\
+		"%s(): " fmt "\n", __func__, ## args)
 #else
-#define PMD_TX_FREE_LOG(level, fmt, args...) do { } while(0)
+#define PMD_TX_FREE_LOG(level, fmt, args...) do { } while (0)
 #endif
 
 extern int e1000_logtype_driver;