On 6/19/23 08:06, Mike Pattrick wrote: > On Thu, Jun 15, 2023 at 3:30 PM Ilya Maximets <[email protected]> wrote: >> >> On 6/14/23 21:03, Mike Pattrick wrote: >>> This patch set is a stripped down subset of the initial 17 patchset >>> introduced >>> by Flavio Leitner in 2021. >>> >>> The initial omnibus patchset was very complex and included a refactor, which >>> stymied review and would have made backporting more complex. It also didn't >>> resolve an ongoing issue with the DPDK netdev where we are currently >>> incorrectly setting vhost flags, resulting in connectivity inturruptions >>> when >>> upgrading OVS to the full TSO patchset without restarting the attached >>> virtual >>> machines. >>> >>> The current 4-patch set is stripped down to include the following: >>> >>> 1. Public facing documentation on the phylosophy of how OVS handles >>> checksums >>> 2. A method for the user to easily check which checksums are offloaded per >>> interface >>> 3 & 4. Most checksuming activity delayed until a packet is about to be: >>> - sent, or >>> - transformed in such a way that checksumming wouldn't be offloadable >>> regardless, such as encapsulation >>> >>> The main benefit of this set in its current state is an improved handling of >>> checksums when encapsulating packets. But the set lays a groundwork for >>> future >>> work including improvements to how dpdk negotiates virtio vhost flags and >>> more >>> efficent handling of checksums in userspace with tso. >>> >>> This 4-patch reduced set has gone through a lot of revisions so far, >>> including: >>> >>> v5 >>> - Refactor was mostly removed, except for valid->good >>> - Reset unsupported offload flags in send_prepare >>> - Moved send_prepare from process_upcall to netdev_upcall >>> >>> v6 >>> - Re-added tests that were incorrectly excluded from v5 >>> >>> v7 >>> - David Marchand found an issue while upgrading OVS and not rebooting >>> attached >>> vhost VMs where we can't change flags post negotiation or check if they >>> have >>> already been set. >>> - This was temporarily resolved by not setting the offending flags >>> - This issue will be addressed in a more robust fasion if this patchset is >>> applied. >>> >>> v8 >>> - v7 patch 3 failed intel ci, moved some code from patch 4 to patch 3 >>> >>> v9 >>> - Resolved a 10% performance hit in a DPDK-PVP workload that David Marchand >>> found >>> >>> v10 >>> - Large amount of formatting and grammar issues >>> - Ported change to AVX512 code >>> - ovs-appctl command removed >>> - new fields added to netdev status including the information removed from >>> ovs-appctl >>> >>> v11 >>> - AVX512 change introduced a checksum bug due to an incorrectly sized >>> datatype, caught by intel-ci and required a recent Xeon to reproduce. >>> >>> v12 >>> - Updated documentation >>> - Changed tso status field name >>> - Excluded the combination of rte_flow offload, userspace tso, and >>> RAW_ENCAP >>> from simultaniously ocuring >>> - Extended AVX512 support to IPv6 >>> >>> v13 >>> - Re-added erroniously missing mutex annocations from v12 >>> >>> v14 >>> - Rewrote a section of the documentation >>> - Removed exclusion of the combination of rte_flow offload, userspace tso, >>> and >>> RAW_ENCAP >>> - Added a TUNGETFEATURES check in netdev-linux.c for better support of >>> early >>> 2.6 era kernels >>> >>> v15 >>> - Moved some new code outside of mutex protection >>> - Only check TUNGETFEATURES once >>> - Respect FLOW_TNL_F_CSUM flag >>> >>> Mike Pattrick (4): >>> Documentation: Document netdev offload. >>> dpif-netdev: Show netdev offloading flags. >>> userspace: Enable IP checksum offloading by default. >>> userspace: Enable L4 checksum offloading by default. >>> >>> Documentation/automake.mk | 1 + >>> Documentation/topics/index.rst | 1 + >>> .../topics/userspace-checksum-offloading.rst | 96 +++++++ >>> lib/conntrack.c | 30 +- >>> lib/dp-packet.c | 40 +++ >>> lib/dp-packet.h | 140 +++++++++- >>> lib/dpif-netdev-extract-avx512.c | 57 ++++ >>> lib/dpif-netdev.c | 2 + >>> lib/flow.c | 38 ++- >>> lib/ipf.c | 11 +- >>> lib/netdev-dpdk.c | 230 +++++++++++----- >>> lib/netdev-dummy.c | 22 ++ >>> lib/netdev-linux.c | 258 +++++++++++++----- >>> lib/netdev-native-tnl.c | 53 ++-- >>> lib/netdev.c | 81 +++--- >>> lib/odp-execute-avx512.c | 108 +++++--- >>> lib/odp-execute.c | 21 +- >>> lib/packets.c | 209 +++++++++++--- >>> lib/packets.h | 3 + >>> tests/dpif-netdev.at | 96 +++++++ >>> 20 files changed, 1176 insertions(+), 321 deletions(-) >>> create mode 100644 Documentation/topics/userspace-checksum-offloading.rst >>> >> >> Thanks, Mike. This version looks good to me in general. >> >> I did some performance testing on a simple V2V setup with 2 vhost-user ports >> and bidirectional traffic. And I see 1.8 - 2.5 % performance decrease with >> this patch set applied and no offloads enabled. Is it expected? >> >> I have a following change that avoids two unnecessary writes per packet in >> case of no offloading: >> >> diff --git a/lib/dp-packet.c b/lib/dp-packet.c >> index dfedd0e9b..27114a9a9 100644 >> --- a/lib/dp-packet.c >> +++ b/lib/dp-packet.c >> @@ -540,15 +540,21 @@ dp_packet_compare_offsets(struct dp_packet *b1, struct >> dp_packet *b2, >> void >> dp_packet_ol_send_prepare(struct dp_packet *p, uint64_t flags) >> { >> - if (dp_packet_ip_checksum_good(p) || !dp_packet_hwol_tx_ip_csum(p)) { >> - dp_packet_hwol_reset_tx_ip_csum(p); >> - } else if (!(flags & NETDEV_TX_OFFLOAD_IPV4_CKSUM)) { >> - dp_packet_ip_set_header_csum(p); >> - dp_packet_ol_set_ip_csum_good(p); >> - dp_packet_hwol_reset_tx_ip_csum(p); >> + if (dp_packet_hwol_tx_ip_csum(p)) { >> + if (dp_packet_ip_checksum_good(p)) { >> + dp_packet_hwol_reset_tx_ip_csum(p); >> + } else if (!(flags & NETDEV_TX_OFFLOAD_IPV4_CKSUM)) { >> + dp_packet_ip_set_header_csum(p); >> + dp_packet_ol_set_ip_csum_good(p); >> + dp_packet_hwol_reset_tx_ip_csum(p); >> + } >> + } >> + >> + if (!dp_packet_hwol_tx_l4_checksum(p)) { >> + return; >> } >> >> - if (dp_packet_l4_checksum_good(p) || !dp_packet_hwol_tx_l4_checksum(p)) >> { >> + if (dp_packet_l4_checksum_good(p)) { >> dp_packet_hwol_reset_tx_l4_csum(p); >> return; >> } >> --- >> >> This allows to get back about 0.5 - 0.7 % in my testing. If the change looks >> good to you, I can fold it in before applying. >> >> I suppose, we can live with that, because we had a 3.5 - 4.5 % increase >> earlier >> in this release cycle. So, it will still be a net positive for users >> upgrading >> from OVS 3.1. >> >> Thoughts? > > The patch looks ok, but with the caveat that I haven't tested it.
Thanks! I folded this in and added a NEWS record. With that, applied. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
