Received-SPF: pass (google.com: domain of linux-kernel+bounces-157007-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <eb0e1eb6-306c-4cf7-922d-203db09d8d29@nbd.name>
Date: Wed, 24 Apr 2024 15:50:32 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC] net: add TCP fraglist GRO support
To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
 Paolo Abeni <pabeni@redhat.com>, Eric Dumazet <edumazet@google.com>
Cc: netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
 Jakub Kicinski <kuba@kernel.org>, David Ahern <dsahern@kernel.org>,
 linux-kernel@vger.kernel.org
References: <20240423094117.93206-1-nbd@nbd.name>
 <CANn89i+6xRe4V6aDmD-9EM0uD7A87f6rzg3S7Xq6-NaB_Mb4nw@mail.gmail.com>
 <63abfa26-d990-46c3-8982-3eaf7b8f8ee5@nbd.name>
 <CANn89iJZvoKVB+AK1_44gki2pHyigyMLXFkyevSQpH3iDbnCvw@mail.gmail.com>
 <7476374f-cf0c-45d0-8100-1b2cd2f290d5@nbd.name>
 <CANn89iLddm704LHPDnnoF2RbCfvrivAz0e6HTeiBARmvzoUBjA@mail.gmail.com>
 <ebe85dca-e0e9-4c55-a15d-20d340f66848@nbd.name>
 <97f10c8b5b615eac8f65d67ef10928d97b6b760d.camel@redhat.com>
 <328ab7b3-4890-4e0d-8b9a-fed7700f1a6a@nbd.name>
 <66285f637bdec_1924cc2941b@willemb.c.googlers.com.notmuch>
From: Felix Fietkau <nbd@nbd.name>
Content-Language: en-US
Autocrypt: addr=nbd@nbd.name; keydata=
 xsDiBEah5CcRBADIY7pu4LIv3jBlyQ/2u87iIZGe6f0f8pyB4UjzfJNXhJb8JylYYRzIOSxh
 ExKsdLCnJqsG1PY1mqTtoG8sONpwsHr2oJ4itjcGHfn5NJSUGTbtbbxLro13tHkGFCoCr4Z5
 Pv+XRgiANSpYlIigiMbOkide6wbggQK32tC20QxUIwCg4k6dtV/4kwEeiOUfErq00TVqIiEE
 AKcUi4taOuh/PQWx/Ujjl/P1LfJXqLKRPa8PwD4j2yjoc9l+7LptSxJThL9KSu6gtXQjcoR2
 vCK0OeYJhgO4kYMI78h1TSaxmtImEAnjFPYJYVsxrhay92jisYc7z5R/76AaELfF6RCjjGeP
 wdalulG+erWju710Bif7E1yjYVWeA/9Wd1lsOmx6uwwYgNqoFtcAunDaMKi9xVQW18FsUusM
 TdRvTZLBpoUAy+MajAL+R73TwLq3LnKpIcCwftyQXK5pEDKq57OhxJVv1Q8XkA9Dn1SBOjNB
 l25vJDFAT9ntp9THeDD2fv15yk4EKpWhu4H00/YX8KkhFsrtUs69+vZQwc0cRmVsaXggRmll
 dGthdSA8bmJkQG5iZC5uYW1lPsJgBBMRAgAgBQJGoeQnAhsjBgsJCAcDAgQVAggDBBYCAwEC
 HgECF4AACgkQ130UHQKnbvXsvgCgjsAIIOsY7xZ8VcSm7NABpi91yTMAniMMmH7FRenEAYMa
 VrwYTIThkTlQzsFNBEah5FQQCACMIep/hTzgPZ9HbCTKm9xN4bZX0JjrqjFem1Nxf3MBM5vN
 CYGBn8F4sGIzPmLhl4xFeq3k5irVg/YvxSDbQN6NJv8o+tP6zsMeWX2JjtV0P4aDIN1pK2/w
 VxcicArw0VYdv2ZCarccFBgH2a6GjswqlCqVM3gNIMI8ikzenKcso8YErGGiKYeMEZLwHaxE
 Y7mTPuOTrWL8uWWRL5mVjhZEVvDez6em/OYvzBwbkhImrryF29e3Po2cfY2n7EKjjr3/141K
 DHBBdgXlPNfDwROnA5ugjjEBjwkwBQqPpDA7AYPvpHh5vLbZnVGu5CwG7NAsrb2isRmjYoqk
 wu++3117AAMFB/9S0Sj7qFFQcD4laADVsabTpNNpaV4wAgVTRHKV/kC9luItzwDnUcsZUPdQ
 f3MueRJ3jIHU0UmRBG3uQftqbZJj3ikhnfvyLmkCNe+/hXhPu9sGvXyi2D4vszICvc1KL4RD
 aLSrOsROx22eZ26KqcW4ny7+va2FnvjsZgI8h4sDmaLzKczVRIiLITiMpLFEU/VoSv0m1F4B
 FtRgoiyjFzigWG0MsTdAN6FJzGh4mWWGIlE7o5JraNhnTd+yTUIPtw3ym6l8P+gbvfoZida0
 TspgwBWLnXQvP5EDvlZnNaKa/3oBes6z0QdaSOwZCRA3QSLHBwtgUsrT6RxRSweLrcabwkkE
 GBECAAkFAkah5FQCGwwACgkQ130UHQKnbvW2GgCeMncXpbbWNT2AtoAYICrKyX5R3iMAoMhw
 cL98efvrjdstUfTCP2pfetyN
In-Reply-To: <66285f637bdec_1924cc2941b@willemb.c.googlers.com.notmuch>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

On 24.04.24 03:24, Willem de Bruijn wrote:
> Felix Fietkau wrote:
>> On 23.04.24 16:34, Paolo Abeni wrote:
>> > On Tue, 2024-04-23 at 14:23 +0200, Felix Fietkau wrote:
>> >> On 23.04.24 14:11, Eric Dumazet wrote:
>> >> > On Tue, Apr 23, 2024 at 1:55 PM Felix Fietkau <nbd@nbd.name> wrote:
>> >> > > 
>> >> > > In the world of consumer-grade WiFi devices, there are a lot of chipsets
>> >> > > with limited or nonexistent SG support, and very limited checksum
>> >> > > offload capabilities on Ethernet. The WiFi side of these devices is
>> >> > > often even worse. I think fraglist GRO is a decent fallback for the
>> >> > > inevitable corner cases.
>> >> > 
>> >> > What about netfilter and NAT ? Are they okay with NETIF_F_FRAGLIST_GRO already ?
>> >> > 
>> >> > Many of these devices are probably using NAT.
>> >> 
>> >> In my tests, nftables NAT works just fine, both with and without 
>> >> flowtable offloading. I didn't see anything in netfilter that would have 
>> >> a problem with this.
>> > 
>> > I see you handle explicitly NAT changes in __tcpv4_gso_segment_csum(),
>> > like the current UDP code.
>> > 
>> > The TCP header has many other fields that could be updated affecting
>> > the TCP csum.
>> > Handling every possible mutation looks cumbersome and will likely
>> > reduce the performance benefits.
>> > 
>> > What is your plan WRT other TCP header fields update?
>> 
>> I think that should be easy enough to handle. My patch already only 
>> combines packets where tcp_flag_word(th) is identical. So when 
>> segmenting, I could handle all flags changes with a single 
>> inet_proto_csum_replace4 call.
>> 
>> > Strictly WRT the patch, I guess it deserves to be split in series,
>> > moving UDP helpers in common code and possibly factoring out more
>> > helpers with separate patches.
>> Will do.
> 
> A significant chunk of the complexity is in the
> tcp[46]_check_fraglist_gro sk match. Is this heuristic worth the
> complexity?
> 
> It seems that the platforms that will enable NETIF_F_FRAGLIST will
> be mainly forwarding planes.

There are people using their devices as file servers and routers at the 
same time. The heuristic helps for those cases.

> If keeping, this refinement can probably a separate follow-on patch in
> the series too:
> 
> - refactor existing udp code
> - add segmentation support to handle such packets on tx
> - add coalescing support that starts building such packets on rx
> - refine coalescing choice
I don't really understand what you're suggesting. With my patch, the GRO 
code handles coalescing of packets. Segmentation on output is also 
supported. The next version of my patch will fix the cases that were too 
similar to the UDP code, so I guess refactoring to share code doesn't 
really make sense there.
Am I missing something?

Thanks,

- Felix