Re: net: tso: add UDP segmentation support: adds regression for ax200 upload
From: Eric Dumazet <edumazet@google.com>
Date: 2020-12-21 19:14:56
Also in:
linux-wireless
On Mon, Dec 21, 2020 at 8:04 PM Eric Dumazet [off-list ref] wrote:
quoted hunk ↗ jump to hunk
On Mon, Dec 21, 2020 at 7:46 PM Eric Dumazet [off-list ref] wrote:quoted
On Sat, Dec 19, 2020 at 5:55 PM Ben Greear [off-list ref] wrote:quoted
On 12/19/20 7:18 AM, Johannes Berg wrote:quoted
On Fri, 2020-12-18 at 12:16 -0800, Jakub Kicinski wrote:quoted
On Thu, 17 Dec 2020 12:40:26 -0800 Ben Greear wrote:quoted
On 12/17/20 10:20 AM, Eric Dumazet wrote:quoted
On Thu, Dec 17, 2020 at 7:13 PM Ben Greear [off-list ref] wrote:quoted
It is the iwlwifi/mvm logic that supports ax200.Let me ask again : I see two different potential call points : drivers/net/wireless/intel/iwlwifi/pcie/tx.c:1529: tso_build_hdr(skb, hdr_page->pos, &tso, data_left, !total_len); drivers/net/wireless/intel/iwlwifi/queue/tx.c:427: tso_build_hdr(skb, hdr_page->pos, &tso, data_left, !total_len); To the best of your knowledge, which one would be used in your case ? Both are horribly complex, I do not want to spend time studying two implementations.It is the queue/tx.c code that executes on my system, verified with printk.Not sure why Intel's not on CC here.Heh :) Let's also add linux-wireless.quoted
Luca, is the ax200 TSO performance regression with recent kernel on your radar?It wasn't on mine for sure, so far. But it's supposed to be Christmas vacation, so haven't checked our bug tracker etc. I see Emmanuel was at least looking at the bug report, but not sure what else happened yet.Not to bitch and moan too much, but even the most basic of testing would have shown this, how can testing be so poor on the ax200 driver? It even shows up with the out-of-tree ax200 driver.quoted
Off the top of my head, I don't really see the issue. Does anyone have the ability to capture the frames over the air (e.g. with another AX200 in monitor mode, load the driver with amsdu_size=3 module parameter to properly capture A-MSDUs)?I can do that at some point, and likely it could be reproduced with an /n or /ac AP and those are a lot easier to sniff. Thanks, Ben -- Ben Greear [off-list ref] Candela Technologies Inc http://www.candelatech.comIt seems the problem comes from some skbs reaching the driver with gso_type == 0, meaning skb_is_gso_tcp() is fuzzy. (net/core/tso.c is only one of the skb_is_gso_tcp() users) Local TCP stack should provide either SKB_GSO_TCPV4 or SKB_GSO_TCPV6 for GSO packets. So maybe the issue is coming from traffic coming from a VM through a tun device or something, and our handling of GSO_ROBUST / DODGY never cared about setting SKB_GSO_TCPV4 or SKB_GSO_TCPV6 if not already given by user space ? Or a plain bug somewhere, possibly overwriting gso_type with 0 or garbage...Oh well, iwl_mvm_tx_tso_segment() 'builds' a fake gso packet. I suspect this will fix the issue :diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tx.cb/drivers/net/wireless/intel/iwlwifi/mvm/tx.c index a983c215df310776ffe67f3b3ffa203eab609bfc..e7ad6367c88de4aff700c630d850760d1d3bf011 100644--- a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c +++ b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c@@ -773,6 +773,7 @@ iwl_mvm_tx_tso_segment(struct sk_buff *skb,unsigned int num_subframes, next = skb_gso_segment(skb, netdev_flags); skb_shinfo(skb)->gso_size = mss; + skb_shinfo(skb)->gso_type = ipv4 ? SKB_GSO_TCPV4 : SKB_GSO_TCPV6; if (WARN_ON_ONCE(IS_ERR(next))) return -EINVAL; else if (next)
Or more precisely :
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tx.cb/drivers/net/wireless/intel/iwlwifi/mvm/tx.c index a983c215df310776ffe67f3b3ffa203eab609bfc..11145bf29f3cbeefcce1a05cc81fd90978f2cbfe 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c@@ -773,6 +773,7 @@ iwl_mvm_tx_tso_segment(struct sk_buff *skb,unsigned int num_subframes,
next = skb_gso_segment(skb, netdev_flags);
skb_shinfo(skb)->gso_size = mss;
+ skb_shinfo(skb)->gso_type = ipv4 ? SKB_GSO_TCPV4 : SKB_GSO_TCPV6;
if (WARN_ON_ONCE(IS_ERR(next)))
return -EINVAL;
else if (next)@@ -795,6 +796,7 @@ iwl_mvm_tx_tso_segment(struct sk_buff *skb,unsigned int num_subframes,
if (tcp_payload_len > mss) {
skb_shinfo(tmp)->gso_size = mss;
+ skb_shinfo(tmp)->gso_type = ipv4 ?
SKB_GSO_TCPV4 : SKB_GSO_TCPV6;
} else {
if (qos) {
u8 *qc;