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