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. Cheers, M > > Best regards, Ilya Maximets. > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
