On Tue, Aug 20, 2024 at 6:09 PM Ilya Maximets <[email protected]> wrote: > > On 8/19/24 09:23, Mike Pattrick wrote: > > On Fri, Aug 16, 2024 at 10:04 AM Ilya Maximets <[email protected]> wrote: > >> > >> On 8/15/24 22:03, Mike Pattrick wrote: > >>> During the transition towards checksum offloading, the function to > >>> handle software fallback of IPv4 checksums didn't account for the > >>> options field. > >>> > >>> Fixes: 5d11c47d3ebe ("userspace: Enable IP checksum offloading by > >>> default.") > >>> Reported-by: Jun Wang <[email protected]> > >>> Signed-off-by: Mike Pattrick <[email protected]> > >>> --- > >>> NB: In 3.2 the early checksum offloading patches caused packets from > >>> the netdev-dummy/receive appctl command to resolve checksums prior to > >>> odp-execute. This fixed in 3.3, but that can't easily be backported > >>> without also backporting unrelated code. > >>> > >>> To enable this patch to be backported, I gave one of the packets a > >>> correct checksum and changed the comment so it was correct but also the > >>> same number of lines as in the previous patch. > >>> > >> > >> Hi, Mike. Thanks for looking into this, but I'm still a bit confused > >> about what is going on. The packets in the test do not trigger the > >> dp_packet_ip_set_header_csum() call, i.e. these tests do not actually > >> test the code change in this patch. However, the previous ${bad_frame} > >> does trigger the function and the checksum is re-calculated for it. > >> > >> What's the difference between these two? Are IP options affecting the > >> logic somehow? > > > > This is unrelated to the content of the packet, and is instead related > > to how dummy/receive packets are handled in offloading scenarios. This > > was fixed in: 084c8087292c ("userspace: Support VXLAN and GENEVE > > TSO.") > > So, I traced the packets through the code. And it is, in fact, related to > the content of the packet. All the previous packets in the test have the > same signature - IPv4 / TCP with 192.168.123.1 destination. So, after the > first good frame we have a datapath flow installed that looks like this: > > in_port(1),eth_type(0x0800),ipv4(dst=192.168.123.1,proto=6,frag=no), > actions:set(ipv4(dst=192.168.1.1)),2 > > We have a match on destination IP, because our OF rule is changing it. > All the incoming packets have the same offlad flags set. They are all > marked for Tx IPv4 checksum offload and all marked as having good csum. > > However, the newly added packets in this patch are ICMP packets and have > a different destination address, so they are going through upcall. > And on 3.2 branch all the packets sent through upcall have their checksums > explicitly fixed up beforehand: > > dp_netdev_upcall() -> dp_packet_ol_send_prepare(packet_, 0); > > Since we have csum_good flag set, the preparation removes the Tx offload > flag. Later, the IP address modification no longer sees the Tx offload flag > and modifies checksum incrementally. > > If the packet will not go through upcall, then the Tx offload flag will be > preserved up until dp_packet_ip_set_header_csum() and the checksum will be > fully re-calculated triggering the issue fixed in this patch. > > So, we either need to send the same TCP packet, but with IP options > to not trigger an upcall, or we could just send the same packet twice > and only check the second pass that will go through the datapath without > triggering an upcall.
I said that the behaviour wasn't content dependent because the call to send_prepare happened in the upcall - regardless of packet content. If you'll accept the solution, I can resubmit with sending the options packet twice, which also resolves the issue. Cheers, M > > Best regards, Ilya Maximets. > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
