RE: [PATCH v4 2/2] bnx2x: disable GSO where gso_size is too big for hardware
From: Chopra, Manish <hidden>
Date: 2018-01-31 09:00:26
quoted hunk ↗ jump to hunk
-----Original Message----- From: Daniel Axtens [mailto:dja@axtens.net] Sent: Wednesday, January 31, 2018 8:46 AM To: netdev@vger.kernel.org Cc: Daniel Axtens <redacted>; Eric Dumazet <redacted>; Chopra, Manish [off-list ref]; Jason Wang [off-list ref]; Pravin Shelar [off-list ref]; Marcelo Ricardo Leitner [off-list ref] Subject: [PATCH v4 2/2] bnx2x: disable GSO where gso_size is too big for hardware If a bnx2x card is passed a GSO packet with a gso_size larger than ~9700 bytes, it will cause a firmware error that will bring the card down: bnx2x: [bnx2x_attn_int_deasserted3:4323(enP24p1s0f0)]MC assert! bnx2x: [bnx2x_mc_assert:720(enP24p1s0f0)]XSTORM_ASSERT_LIST_INDEX 0x2 bnx2x: [bnx2x_mc_assert:736(enP24p1s0f0)]XSTORM_ASSERT_INDEX 0x0 = 0x00000000 0x25e43e47 0x00463e01 0x00010052 bnx2x: [bnx2x_mc_assert:750(enP24p1s0f0)]Chip Revision: everest3, FW Version: 7_13_1 ... (dump of values continues) ... Detect when the mac length of a GSO packet is greater than the maximum packet size (9700 bytes) and disable GSO. Signed-off-by: Daniel Axtens <redacted> --- v4: Only call the slow check if the gso_size is large. Eric - I think this is what you had in mind? Manish - do you think this is an acceptable performance trade-off? GSO will work for any packet size, and only jumbo frames will have to do the slower test. Again, only build-tested. --- drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.cb/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c index 7b08323e3f3d..74fc9af4aadb 100644--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c@@ -12934,6 +12934,24 @@ static netdev_features_tbnx2x_features_check(struct sk_buff *skb, struct net_device *dev, netdev_features_t features) { + /* + * A skb with gso_size + header length > 9700 will cause a + * firmware panic. Drop GSO support. + * + * Eventually the upper layer should not pass these packets down. + * + * For speed, if the gso_size is <= 9000, assume there will + * not be 700 bytes of headers and pass it through. Only do a + * full (slow) validation if the gso_size is > 9000. + * + * (Due to the way SKB_BY_FRAGS works this will also do a full + * validation in that case.) + */ + if (unlikely(skb_is_gso(skb) && + (skb_shinfo(skb)->gso_size > 9000) && + !skb_gso_validate_mac_len(skb, 9700))) + features &= ~NETIF_F_GSO_MASK;
Hi Daniel, Obviously, it could be bad from performance perspective since every gso packet has to do these check. When running iperf/netperf performance benchmark, where GSO is likely to occur. Why do you have to put two checks for skb_is_gso() and gso_size ? Isn't gso_size > anything means GSO skb ? I assume it won't cause disabling the offload if "headers [L2 + L3 + L4] + gso_size" is still <= 9700. ? Thanks, Manish