From: Nikolay Aleksandrov <razor@blackwall.org> Date: 2021-02-09 10:37:02
From: Nikolay Aleksandrov <redacted>
Hi,
We'd like to have proper 200G and 400G support with 3ad bond mode, so we
need to add new definitions for them in order to have separate oper keys,
aggregated bandwidth and proper operation (patches 01 and 02). In
patch 03 Ido changes the code to use WARN_ONCE() instead of
pr_warn_once which would help future detection of unsupported speeds.
These warnings are usually detected by automated tools and regression
tests.
Thanks,
Nik
Ido Schimmel (1):
bonding: 3ad: Use a more verbose warning for unknown speeds
Nikolay Aleksandrov (2):
bonding: 3ad: add support for 200G speed
bonding: 3ad: add support for 400G speed
drivers/net/bonding/bond_3ad.c | 27 ++++++++++++++++++++++-----
1 file changed, 22 insertions(+), 5 deletions(-)
--
2.29.2
From: Nikolay Aleksandrov <razor@blackwall.org> Date: 2021-02-09 10:38:56
From: Nikolay Aleksandrov <redacted>
In order to be able to use 3ad mode with 200G devices we need to extend
the supported speeds.
Signed-off-by: Nikolay Aleksandrov <redacted>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
drivers/net/bonding/bond_3ad.c | 9 +++++++++
1 file changed, 9 insertions(+)
@@ -245,6 +246,7 @@ static inline int __check_agg_selection_timer(struct port *port)*%AD_LINK_SPEED_50000MBPS*%AD_LINK_SPEED_56000MBPS*%AD_LINK_SPEED_100000MBPS+*%AD_LINK_SPEED_200000MBPS*/staticu16__get_link_speed(structport*port){
@@ -312,6 +314,10 @@ static u16 __get_link_speed(struct port *port)speed=AD_LINK_SPEED_100000MBPS;break;+caseSPEED_200000:+speed=AD_LINK_SPEED_200000MBPS;+break;+default:/* unknown speed value from ethtool. shouldn't happen */if(slave->speed!=SPEED_UNKNOWN)
@@ -733,6 +739,9 @@ static u32 __get_agg_bandwidth(struct aggregator *aggregator)caseAD_LINK_SPEED_100000MBPS:bandwidth=nports*100000;break;+caseAD_LINK_SPEED_200000MBPS:+bandwidth=nports*200000;+break;default:bandwidth=0;/* to silence the compiler */}
From: Nikolay Aleksandrov <razor@blackwall.org> Date: 2021-02-09 10:38:56
From: Nikolay Aleksandrov <redacted>
In order to be able to use 3ad mode with 400G devices we need to extend
the supported speeds.
Signed-off-by: Nikolay Aleksandrov <redacted>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
drivers/net/bonding/bond_3ad.c | 9 +++++++++
1 file changed, 9 insertions(+)
@@ -247,6 +248,7 @@ static inline int __check_agg_selection_timer(struct port *port)*%AD_LINK_SPEED_56000MBPS*%AD_LINK_SPEED_100000MBPS*%AD_LINK_SPEED_200000MBPS+*%AD_LINK_SPEED_400000MBPS*/staticu16__get_link_speed(structport*port){
@@ -318,6 +320,10 @@ static u16 __get_link_speed(struct port *port)speed=AD_LINK_SPEED_200000MBPS;break;+caseSPEED_400000:+speed=AD_LINK_SPEED_400000MBPS;+break;+default:/* unknown speed value from ethtool. shouldn't happen */if(slave->speed!=SPEED_UNKNOWN)
@@ -742,6 +748,9 @@ static u32 __get_agg_bandwidth(struct aggregator *aggregator)caseAD_LINK_SPEED_200000MBPS:bandwidth=nports*200000;break;+caseAD_LINK_SPEED_400000MBPS:+bandwidth=nports*400000;+break;default:bandwidth=0;/* to silence the compiler */}
@@ -326,11 +326,10 @@ static u16 __get_link_speed(struct port *port)default:/* unknown speed value from ethtool. shouldn't happen */-if(slave->speed!=SPEED_UNKNOWN)-pr_warn_once("%s: (slave %s): unknown ethtool speed (%d) for port %d (set it to 0)\n",-slave->bond->dev->name,-slave->dev->name,slave->speed,-port->actor_port_number);+WARN_ONCE(slave->speed!=SPEED_UNKNOWN,+"%s: (slave %s): unknown ethtool speed (%d) for port %d (set it to 0)\n",+slave->bond->dev->name,slave->dev->name,+slave->speed,port->actor_port_number);speed=0;break;}
From: Alexander Duyck <hidden> Date: 2021-02-10 19:45:55
On Tue, Feb 9, 2021 at 2:42 AM Nikolay Aleksandrov [off-list ref] wrote:
From: Ido Schimmel <idosch@nvidia.com>
The bond driver needs to be patched to support new ethtool speeds.
Currently it emits a single warning [1] when it encounters an unknown
speed. As evident by the two previous patches, this is not explicit
enough. Instead, use WARN_ONCE() to get a more verbose warning [2].
[1]
bond10: (slave swp1): unknown ethtool speed (200000) for port 1 (set it to 0)
[2]
bond20: (slave swp2): unknown ethtool speed (400000) for port 1 (set it to 0)
WARNING: CPU: 5 PID: 96 at drivers/net/bonding/bond_3ad.c:317 __get_link_speed.isra.0+0x110/0x120
Modules linked in:
CPU: 5 PID: 96 Comm: kworker/u16:5 Not tainted 5.11.0-rc6-custom-02818-g69a767ec7302 #3243
Hardware name: Mellanox Technologies Ltd. MSN4700/VMOD0010, BIOS 5.11 01/06/2019
Workqueue: bond20 bond_mii_monitor
RIP: 0010:__get_link_speed.isra.0+0x110/0x120
Code: 5b ff ff ff 52 4c 8b 4e 08 44 0f b7 c7 48 c7 c7 18 46 4a b8 48 8b 16 c6 05 d9 76 41 01 01 49 8b 31 89 44 24 04 e8 a2 8a 3f 00 <0f> 0b 8b 44 24 04 59 c3 0
f 1f 84 00 00 00 00 00 48 85 ff 74 3b 53
RSP: 0018:ffffb683c03afde0 EFLAGS: 00010282
RAX: 0000000000000000 RBX: ffff96bd3f2a9a38 RCX: 0000000000000000
RDX: ffff96c06fd67560 RSI: ffff96c06fd57850 RDI: ffff96c06fd57850
RBP: 0000000000000000 R08: ffffffffb8b49888 R09: 0000000000009ffb
R10: 00000000ffffe000 R11: 3fffffffffffffff R12: 0000000000000000
R13: ffff96bd3f2a9a38 R14: ffff96bd49c56400 R15: ffff96bd49c564f0
FS: 0000000000000000(0000) GS:ffff96c06fd40000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f327ad804b0 CR3: 0000000142ad5006 CR4: 00000000003706e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
ad_update_actor_keys+0x36/0xc0
bond_3ad_handle_link_change+0x5d/0xf0
bond_mii_monitor.cold+0x1c2/0x1e8
process_one_work+0x1c9/0x360
worker_thread+0x48/0x3c0
kthread+0x113/0x130
ret_from_fork+0x1f/0x30
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
I'm not really sure making the warning consume more text is really
going to solve the problem. I was actually much happier with just the
first error as I don't need a stack trace. Just having the line is
enough information for me to search and find the cause for the issue.
Adding a backtrace is just overkill.
If we really think this is something that is important maybe we should
move this up to an error instead of a warning. For example why not
make this use pr_err_once, instead of pr_warn_once? It should make it
more likely to be highlighted in the system log.
On Wed, Feb 10, 2021 at 11:44:31AM -0800, Alexander Duyck wrote:
I'm not really sure making the warning consume more text is really
going to solve the problem. I was actually much happier with just the
first error as I don't need a stack trace. Just having the line is
enough information for me to search and find the cause for the issue.
Adding a backtrace is just overkill.
If we really think this is something that is important maybe we should
move this up to an error instead of a warning. For example why not
make this use pr_err_once, instead of pr_warn_once? It should make it
more likely to be highlighted in the system log.
Yea, I expected this comment.
We are currently looking for patterns such as 'BUG', 'WARNING', 'BUG
kmalloc', 'UBSAN' etc in regression. Mostly based on what syzkaller is
doing [1] (which we are also running). We can instead promote this
warning to pr_err_once() and start looking at errors as well. It might
uncover more issues / false positives.
[1] https://github.com/google/syzkaller/blob/42b90a7c596c2b7d8f8d034dff7d8c635631de5a/pkg/report/linux.go#L952