ethdev: add function name to log message

PATCH In review Stephen Hemminger 4 participants 8 messages Oct 11, 2018 – Oct 17, 2018 Network drivers

Share & tools

Stephen Hemminger

author

2018-10-11 17:59 UTC #0 lore raw

PATCH 1/1 ethdev: add function name to log message

While debugging some virtio driver issues, saw:
Invalid port_id=3
but it was not clear which function was logging this because the same log is being done in multiple places in ethdev.

Trivial fix is to prefix with function name as is done in other drivers.

Signed-off-by Stephen Hemminger <[email protected]>

---
 drivers/net/failsafe/failsafe.c | 1 -
 lib/librte_ethdev/rte_ethdev.h  | 5 +++--
 2 files changed, 3 insertions(+), 3 deletions(-)

drivers/net/failsafe/failsafe.c +0 -1

index c3999f0265b2..3830ed33dba2 100644
@@ -2,7 +2,6 @@
  * Copyright 2017 6WIND S.A.
  * Copyright 2017 Mellanox Technologies, Ltd
  */
-
 #include <rte_alarm.h>
 #include <rte_malloc.h>
 #include <rte_ethdev_driver.h>

lib/librte_ethdev/rte_ethdev.h +3 -2

index 012577b0ab22..9632c67b929b 100644
@@ -161,8 +161,9 @@ extern "C" {
 
 extern int rte_eth_dev_logtype;
 
-#define RTE_ETHDEV_LOG(level, ...) \
-	rte_log(RTE_LOG_ ## level, rte_eth_dev_logtype, "" __VA_ARGS__)
+#define RTE_ETHDEV_LOG(level, fmt, ...)		\
+	rte_log(RTE_LOG_ ## level, rte_eth_dev_logtype, \
+		"%s():" fmt, __func__, ## __VA_ARGS__)
 
 struct rte_mbuf;

Signature · 1 line

2.19.1

Ferruh Yigit

2018-10-12 10:42 UTC #1 lore raw

lib/librte_ethdev/rte_ethdev.h extern "C" { · line 166

Quoted 9 lines · Stephen Hemminger

lib/librte_ethdev/rte_ethdev.h +3 -2

@@ -161,8 +161,9 @@ extern "C" {
 
 extern int rte_eth_dev_logtype;
 
-#define RTE_ETHDEV_LOG(level, ...) \
-	rte_log(RTE_LOG_ ## level, rte_eth_dev_logtype, "" __VA_ARGS__)
+#define RTE_ETHDEV_LOG(level, fmt, ...)		\
+	rte_log(RTE_LOG_ ## level, rte_eth_dev_logtype, \
+		"%s():" fmt, __func__, ## __VA_ARGS__)

+1 to adding function name, but

failsafe is giving build error [1] with clang because of ## usage [2], that is why I add this as "" __VA_ARGS__ at first place but you can't do this trick if __VA_ARGS__ used after fmt.

I am not aware of a solution for this, __VA_OPT__(,) also didn't worked with clang.

[1]
.../build/include/rte_ethdev.h:166:26: error: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Werror,-Wgnu-zero-variadic-macro-arguments]

                "%s():" fmt, __func__, ## __VA_ARGS__)
                                       ^

[2]
This seems because of "-pedantic" argument driver uses, and other PMDs using "-pedantic", like mlx, will have same error although they are disable by default and error not observed in default build.

Ferruh Yigit

2018-10-12 10:45 UTC #2 lore raw

Quoted 13 lines · Ferruh Yigit

lib/librte_ethdev/rte_ethdev.h extern "C" { · line 166
Quoted 9 lines · Stephen Hemminger
lib/librte_ethdev/rte_ethdev.h +3 -2
@@ -161,8 +161,9 @@ extern "C" {
 
 extern int rte_eth_dev_logtype;
 
-#define RTE_ETHDEV_LOG(level, ...) \
-	rte_log(RTE_LOG_ ## level, rte_eth_dev_logtype, "" __VA_ARGS__)
+#define RTE_ETHDEV_LOG(level, fmt, ...)		\
+	rte_log(RTE_LOG_ ## level, rte_eth_dev_logtype, \
+		"%s():" fmt, __func__, ## __VA_ARGS__)
+1 to adding function name, but

Quoted 5 lines

failsafe is giving build error [1] with clang because of ## usage [2], that is why I add this as "" __VA_ARGS__ at first place but you can't do this trick if __VA_ARGS__ used after fmt.
I am not aware of a solution for this, __VA_OPT__(,) also didn't worked with clang.

+cc Adrien & Gaetan,

I saw Adrien put some "workaround" to this for mlx5

Quoted 13 lines

[1]
.../build/include/rte_ethdev.h:166:26: error: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Werror,-Wgnu-zero-variadic-macro-arguments]
                "%s():" fmt, __func__, ## __VA_ARGS__)
                                       ^
[2]
This seems because of "-pedantic" argument driver uses, and other PMDs using "-pedantic", like mlx, will have same error although they are disable by default and error not observed in default build.

Adrien Mazarguil

2018-10-12 12:44 UTC #3 lore raw

Quoted 18 lines · Ferruh Yigit

Quoted 17 lines · Ferruh Yigit
lib/librte_ethdev/rte_ethdev.h extern "C" { · line 166
Quoted 9 lines · Stephen Hemminger
lib/librte_ethdev/rte_ethdev.h +3 -2
@@ -161,8 +161,9 @@ extern "C" {
 
 extern int rte_eth_dev_logtype;
 
-#define RTE_ETHDEV_LOG(level, ...) \
-	rte_log(RTE_LOG_ ## level, rte_eth_dev_logtype, "" __VA_ARGS__)
+#define RTE_ETHDEV_LOG(level, fmt, ...)		\
+	rte_log(RTE_LOG_ ## level, rte_eth_dev_logtype, \
+		"%s():" fmt, __func__, ## __VA_ARGS__)
+1 to adding function name, but
failsafe is giving build error [1] with clang because of ## usage [2], that is why I add this as "" __VA_ARGS__ at first place but you can't do this trick if __VA_ARGS__ used after fmt.

Quoted 5 lines

Quoted 1 line
I am not aware of a solution for this, __VA_OPT__(,) also didn't worked with clang.
+cc Adrien & Gaetan,
I saw Adrien put some "workaround" to this for mlx5

Yes, through RTE_FMT() (rte_common.h). Something like this:

 #define RTE_ETHDEV_LOG(level, fmt, ...) \
     rte_log(RTE_LOG_ ## level, \
             rte_eth_dev_logtype, \
             "%s():" fmt, \
             __func__, \
             ## __VA_ARGS__)

Can be rewritten like that:

 #define RTE_ETHDEV_LOG(level, ...) \
     rte_log(RTE_LOG_ ## level, \
             rte_eth_dev_logtype, \
             RTE_FMT("%s():" RTE_FMT_HEAD(__VA_ARGS__,), \
                     __func__, \
                     RTE_FMT_TAIL(__VA_ARGS__,)))

Although not too pretty and convenient, it does the job. In short:

- Remove "fmt" argument from prototype.
- Enclose format string and its arguments in RTE_FMT().
- Replace "fmt" with RTE_FMT_HEAD(__VA_ARGS__,).
- Replace "## __VA_ARGS__" with RTE_FMT_TAIL(__VA_ARGS__,).
- Yes, trailing commas are mandatory in RTE_FMT_(HEAD|TAIL)().
- Note it quietly appends a dummy "%.0s" argument to the format string.

Quoted 12 lines

Quoted 11 lines
[1]
.../build/include/rte_ethdev.h:166:26: error: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Werror,-Wgnu-zero-variadic-macro-arguments]
                "%s():" fmt, __func__, ## __VA_ARGS__)
                                       ^
[2]
This seems because of "-pedantic" argument driver uses, and other PMDs using "-pedantic", like mlx, will have same error although they are disable by default and error not observed in default build.

Signature · 2 lines

Adrien Mazarguil
6WIND

Stephen Hemminger

author

2018-10-12 14:54 UTC #4 lore raw

Quoted 43 lines · On Fri, 12 Oct 2018 14:43:57 +0200 Adrien Mazarguil…

Quoted 18 lines · Ferruh Yigit
Quoted 17 lines · Ferruh Yigit
lib/librte_ethdev/rte_ethdev.h extern "C" { · line 166
Quoted 9 lines · Stephen Hemminger
lib/librte_ethdev/rte_ethdev.h +3 -2
@@ -161,8 +161,9 @@ extern "C" {
 
 extern int rte_eth_dev_logtype;
 
-#define RTE_ETHDEV_LOG(level, ...) \
-	rte_log(RTE_LOG_ ## level, rte_eth_dev_logtype, "" __VA_ARGS__)
+#define RTE_ETHDEV_LOG(level, fmt, ...)		\
+	rte_log(RTE_LOG_ ## level, rte_eth_dev_logtype, \
+		"%s():" fmt, __func__, ## __VA_ARGS__)  
+1 to adding function name, but
failsafe is giving build error [1] with clang because of ## usage [2], that is why I add this as "" __VA_ARGS__ at first place but you can't do this trick if __VA_ARGS__ used after fmt.
Quoted 5 lines
Quoted 1 line
I am not aware of a solution for this, __VA_OPT__(,) also didn't worked with clang.
+cc Adrien & Gaetan,
I saw Adrien put some "workaround" to this for mlx5
Yes, through RTE_FMT() (rte_common.h). Something like this:
 #define RTE_ETHDEV_LOG(level, fmt, ...) \
     rte_log(RTE_LOG_ ## level, \
             rte_eth_dev_logtype, \
             "%s():" fmt, \
             __func__, \
             ## __VA_ARGS__)
Can be rewritten like that:
 #define RTE_ETHDEV_LOG(level, ...) \
     rte_log(RTE_LOG_ ## level, \
             rte_eth_dev_logtype, \
             RTE_FMT("%s():" RTE_FMT_HEAD(__VA_ARGS__,), \
                     __func__, \
                     RTE_FMT_TAIL(__VA_ARGS__,)))

Quoted 22 lines

Although not too pretty and convenient, it does the job. In short:
- Remove "fmt" argument from prototype.
- Enclose format string and its arguments in RTE_FMT().
- Replace "fmt" with RTE_FMT_HEAD(__VA_ARGS__,).
- Replace "## __VA_ARGS__" with RTE_FMT_TAIL(__VA_ARGS__,).
- Yes, trailing commas are mandatory in RTE_FMT_(HEAD|TAIL)().
- Note it quietly appends a dummy "%.0s" argument to the format string.
Quoted 12 lines
Quoted 11 lines
[1]
.../build/include/rte_ethdev.h:166:26: error: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Werror,-Wgnu-zero-variadic-macro-arguments]
                "%s():" fmt, __func__, ## __VA_ARGS__)
                                       ^
[2]
This seems because of "-pedantic" argument driver uses, and other PMDs using "-pedantic", like mlx, will have same error although they are disable by default and error not observed in default build.

Since zero varadic macros is a GNU extension, maybe just adding GNU source?

Ferruh Yigit

2018-10-16 15:55 UTC #5 lore raw

Quoted 69 lines · Stephen Hemminger

Quoted 43 lines · On Fri, 12 Oct 2018 14:43:57 +0200 Adrien Mazarguil…
Quoted 18 lines · Ferruh Yigit
Quoted 17 lines · Ferruh Yigit
lib/librte_ethdev/rte_ethdev.h extern "C" { · line 166
Quoted 9 lines · Stephen Hemminger
lib/librte_ethdev/rte_ethdev.h +3 -2
@@ -161,8 +161,9 @@ extern "C" {
 
 extern int rte_eth_dev_logtype;
 
-#define RTE_ETHDEV_LOG(level, ...) \
-	rte_log(RTE_LOG_ ## level, rte_eth_dev_logtype, "" __VA_ARGS__)
+#define RTE_ETHDEV_LOG(level, fmt, ...)		\
+	rte_log(RTE_LOG_ ## level, rte_eth_dev_logtype, \
+		"%s():" fmt, __func__, ## __VA_ARGS__)  
+1 to adding function name, but
failsafe is giving build error [1] with clang because of ## usage [2], that is why I add this as "" __VA_ARGS__ at first place but you can't do this trick if __VA_ARGS__ used after fmt.
Quoted 5 lines
Quoted 1 line
I am not aware of a solution for this, __VA_OPT__(,) also didn't worked with clang.
+cc Adrien & Gaetan,
I saw Adrien put some "workaround" to this for mlx5
Yes, through RTE_FMT() (rte_common.h). Something like this:
 #define RTE_ETHDEV_LOG(level, fmt, ...) \
     rte_log(RTE_LOG_ ## level, \
             rte_eth_dev_logtype, \
             "%s():" fmt, \
             __func__, \
             ## __VA_ARGS__)
Can be rewritten like that:
 #define RTE_ETHDEV_LOG(level, ...) \
     rte_log(RTE_LOG_ ## level, \
             rte_eth_dev_logtype, \
             RTE_FMT("%s():" RTE_FMT_HEAD(__VA_ARGS__,), \
                     __func__, \
                     RTE_FMT_TAIL(__VA_ARGS__,)))
Quoted 22 lines
Although not too pretty and convenient, it does the job. In short:
- Remove "fmt" argument from prototype.
- Enclose format string and its arguments in RTE_FMT().
- Replace "fmt" with RTE_FMT_HEAD(__VA_ARGS__,).
- Replace "## __VA_ARGS__" with RTE_FMT_TAIL(__VA_ARGS__,).
- Yes, trailing commas are mandatory in RTE_FMT_(HEAD|TAIL)().
- Note it quietly appends a dummy "%.0s" argument to the format string.
Quoted 12 lines
Quoted 11 lines
[1]
.../build/include/rte_ethdev.h:166:26: error: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Werror,-Wgnu-zero-variadic-macro-arguments]
                "%s():" fmt, __func__, ## __VA_ARGS__)
                                       ^
[2]
This seems because of "-pedantic" argument driver uses, and other PMDs using "-pedantic", like mlx, will have same error although they are disable by default and error not observed in default build.

Quoted 1 line

Since zero varadic macros is a GNU extension, maybe just adding GNU source?

I think -pedantic is preventing using extension whether we have _GNU_SOURCE or not.

Gaetan,

Why we have -pedantic option enabled for failsafe?

Gaëtan Rivet

2018-10-17 09:26 UTC #6 lore raw

Hi Ferruh,

Quoted 70 lines · Ferruh Yigit

Quoted 69 lines · Stephen Hemminger
Quoted 65 lines · On Fri, 12 Oct 2018 14:43:57 +0200 Adrien Mazarguil…
Quoted 18 lines · Ferruh Yigit
Quoted 17 lines · Ferruh Yigit
lib/librte_ethdev/rte_ethdev.h extern "C" { · line 166
Quoted 9 lines · Stephen Hemminger
lib/librte_ethdev/rte_ethdev.h +3 -2
@@ -161,8 +161,9 @@ extern "C" {
 
 extern int rte_eth_dev_logtype;
 
-#define RTE_ETHDEV_LOG(level, ...) \
-	rte_log(RTE_LOG_ ## level, rte_eth_dev_logtype, "" __VA_ARGS__)
+#define RTE_ETHDEV_LOG(level, fmt, ...)		\
+	rte_log(RTE_LOG_ ## level, rte_eth_dev_logtype, \
+		"%s():" fmt, __func__, ## __VA_ARGS__)  
+1 to adding function name, but
failsafe is giving build error [1] with clang because of ## usage [2], that is why I add this as "" __VA_ARGS__ at first place but you can't do this trick if __VA_ARGS__ used after fmt.
Quoted 5 lines
Quoted 1 line
I am not aware of a solution for this, __VA_OPT__(,) also didn't worked with clang.
+cc Adrien & Gaetan,
I saw Adrien put some "workaround" to this for mlx5
Yes, through RTE_FMT() (rte_common.h). Something like this:
 #define RTE_ETHDEV_LOG(level, fmt, ...) \
     rte_log(RTE_LOG_ ## level, \
             rte_eth_dev_logtype, \
             "%s():" fmt, \
             __func__, \
             ## __VA_ARGS__)
Can be rewritten like that:
 #define RTE_ETHDEV_LOG(level, ...) \
     rte_log(RTE_LOG_ ## level, \
             rte_eth_dev_logtype, \
             RTE_FMT("%s():" RTE_FMT_HEAD(__VA_ARGS__,), \
                     __func__, \
                     RTE_FMT_TAIL(__VA_ARGS__,)))
Although not too pretty and convenient, it does the job. In short:
- Remove "fmt" argument from prototype.
- Enclose format string and its arguments in RTE_FMT().
- Replace "fmt" with RTE_FMT_HEAD(__VA_ARGS__,).
- Replace "## __VA_ARGS__" with RTE_FMT_TAIL(__VA_ARGS__,).
- Yes, trailing commas are mandatory in RTE_FMT_(HEAD|TAIL)().
- Note it quietly appends a dummy "%.0s" argument to the format string.
Quoted 12 lines
Quoted 11 lines
[1]
.../build/include/rte_ethdev.h:166:26: error: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Werror,-Wgnu-zero-variadic-macro-arguments]
                "%s():" fmt, __func__, ## __VA_ARGS__)
                                       ^
[2]
This seems because of "-pedantic" argument driver uses, and other PMDs using "-pedantic", like mlx, will have same error although they are disable by default and error not observed in default build.

Quoted 8 lines

Quoted 1 line
Since zero varadic macros is a GNU extension, maybe just adding GNU source?
I think -pedantic is preventing using extension whether we have _GNU_SOURCE or not.
Gaetan,
Why we have -pedantic option enabled for failsafe?

Because I wanted to enforce it in failsafe, and core DPDK headers were compatible so it wasn't too much of a hassle.

If rte_ethdev.h is meant to lose this compatibility, then pedantic will be disabled upon including it. But I guess if it was kept compatible until now, then it was a deliberate effort?

Signature · 2 lines

Gaëtan Rivet
6WIND

Ferruh Yigit

2018-10-17 17:08 UTC #7 lore raw

Quoted 80 lines · Gaëtan Rivet

Hi Ferruh,
Quoted 77 lines · Ferruh Yigit
Quoted 69 lines · Stephen Hemminger
Quoted 43 lines · On Fri, 12 Oct 2018 14:43:57 +0200 Adrien Mazarguil…
Quoted 18 lines · Ferruh Yigit
Quoted 17 lines · Ferruh Yigit
Review extern "C" { · line 166
Quoted 9 lines · Stephen Hemminger
(quoted diff) +3 -2
@@ -161,8 +161,9 @@ extern "C" {
 
 extern int rte_eth_dev_logtype;
 
-#define RTE_ETHDEV_LOG(level, ...) \
-	rte_log(RTE_LOG_ ## level, rte_eth_dev_logtype, "" __VA_ARGS__)
+#define RTE_ETHDEV_LOG(level, fmt, ...)		\
+	rte_log(RTE_LOG_ ## level, rte_eth_dev_logtype, \
+		"%s():" fmt, __func__, ## __VA_ARGS__)  
+1 to adding function name, but
failsafe is giving build error [1] with clang because of ## usage [2], that is why I add this as "" __VA_ARGS__ at first place but you can't do this trick if __VA_ARGS__ used after fmt.
Quoted 5 lines
Quoted 1 line
I am not aware of a solution for this, __VA_OPT__(,) also didn't worked with clang.
+cc Adrien & Gaetan,
I saw Adrien put some "workaround" to this for mlx5
Yes, through RTE_FMT() (rte_common.h). Something like this:
 #define RTE_ETHDEV_LOG(level, fmt, ...) \
     rte_log(RTE_LOG_ ## level, \
             rte_eth_dev_logtype, \
             "%s():" fmt, \
             __func__, \
             ## __VA_ARGS__)
Can be rewritten like that:
 #define RTE_ETHDEV_LOG(level, ...) \
     rte_log(RTE_LOG_ ## level, \
             rte_eth_dev_logtype, \
             RTE_FMT("%s():" RTE_FMT_HEAD(__VA_ARGS__,), \
                     __func__, \
                     RTE_FMT_TAIL(__VA_ARGS__,)))
Quoted 22 lines
Although not too pretty and convenient, it does the job. In short:
- Remove "fmt" argument from prototype.
- Enclose format string and its arguments in RTE_FMT().
- Replace "fmt" with RTE_FMT_HEAD(__VA_ARGS__,).
- Replace "## __VA_ARGS__" with RTE_FMT_TAIL(__VA_ARGS__,).
- Yes, trailing commas are mandatory in RTE_FMT_(HEAD|TAIL)().
- Note it quietly appends a dummy "%.0s" argument to the format string.
Quoted 12 lines
Quoted 11 lines
[1]
.../build/include/rte_ethdev.h:166:26: error: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Werror,-Wgnu-zero-variadic-macro-arguments]
                "%s():" fmt, __func__, ## __VA_ARGS__)
                                       ^
[2]
This seems because of "-pedantic" argument driver uses, and other PMDs using "-pedantic", like mlx, will have same error although they are disable by default and error not observed in default build.
Quoted 1 line
Since zero varadic macros is a GNU extension, maybe just adding GNU source?
I think -pedantic is preventing using extension whether we have _GNU_SOURCE or not.
Gaetan,

Quoted 8 lines

Quoted 1 line
Why we have -pedantic option enabled for failsafe?
Because I wanted to enforce it in failsafe, and core DPDK headers were compatible so it wasn't too much of a hassle.
If rte_ethdev.h is meant to lose this compatibility, then pedantic will be disabled upon including it. But I guess if it was kept compatible until now, then it was a deliberate effort?

I think it is deliberate effort, I remember Adrien put some patches in the past to make DPDK headers pedantic compatible by adding __extension__ keywords.

I tried to use __extension__ for ## but not able to make it work, is there a way to do this?

And I see the reasoning of making DPDK headers pedantic compatible, I think we should keep them compatible.