Re: net: tso: add UDP segmentation support: adds regression for ax200 upload

[Ben Greear <greearb@candelatech.com>]

On 12/21/20 12:01 PM, Rainer Suhm wrote:
> Am 21.12.20 um 20:14 schrieb Eric Dumazet:
>> On Mon, Dec 21, 2020 at 8:04 PM Eric Dumazet <edumazet@google.com> wrote:
>>>
>>> On Mon, Dec 21, 2020 at 7:46 PM Eric Dumazet <edumazet@google.com> wrote:
>>>>
>>>> On Sat, Dec 19, 2020 at 5:55 PM Ben Greear <greearb@candelatech.com> wrote:
>>>>>
>>>>> On 12/19/20 7:18 AM, Johannes Berg wrote:
>>>>>> On Fri, 2020-12-18 at 12:16 -0800, Jakub Kicinski wrote:
>>>>>>> On Thu, 17 Dec 2020 12:40:26 -0800 Ben Greear wrote:
>>>>>>>> On 12/17/20 10:20 AM, Eric Dumazet wrote:
>>>>>>>>> On Thu, Dec 17, 2020 at 7:13 PM Ben Greear <greearb@candelatech.com> wrote:
>>>>>>>>>> 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.
>>>>>>
>>>>>>> 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.
>>>>>
>>>>>> 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 <greearb@candelatech.com>
>>>>> Candela Technologies Inc  http://www.candelatech.com
>>>>
>>>> It 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.c
>>> b/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.c
>> b/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;
>>
> 
> 
> This looks good to me.
> Transmission rate is in the expected range. iperf3 shows no retries anymore.
> 
> Here is my kernel log with the above changes applied, and the debug patches from Eric.

I tested this successfully as well.

Eric:  Thanks for the patch!

--Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com