On 8/22/24 16:30, Mike Pattrick wrote: > 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.
My point was that the fact the we're having an upcall depends on the packet content. If we use a TCP packet with 192.168.123.1 destination, we'll not have an upcall. > If you'll accept the solution, I can resubmit with sending the options > packet twice, which also resolves the issue. Double sending seems reasonable. As a separate change we may also want to enhance the test on the main branch by sending two packets as well (and checking both of them), so we can cover both the upcall and non-upcall path. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
