On Wed, Jun 21, 2023 at 12:13 AM Ilya Maximets <[email protected]> wrote: > > On 6/14/23 21:03, Mike Pattrick wrote: > > The netdev receiving packets is supposed to provide the flags > > indicating if the L4 checksum was verified and it is OK or BAD, > > otherwise the stack will check when appropriate by software. > > > > If the packet comes with good checksum, then postpone the > > checksum calculation to the egress device if needed. > > > > When encapsulate a packet with that flag, set the checksum > > of the inner L4 header since that is not yet supported. > > > > Calculate the L4 checksum when the packet is going to be sent > > over a device that doesn't support the feature. > > > > Linux tap devices allows enabling L3 and L4 offload, so this > > patch enables the feature. However, Linux socket interface > > remains disabled because the API doesn't allow enabling > > those two features without enabling TSO too. > > > > Signed-off-by: Flavio Leitner <[email protected]> > > Co-authored-by: Flavio Leitner <[email protected]> > > Signed-off-by: Mike Pattrick <[email protected]> > > --- > > Since v9: > > - Extended miniflow_extract changes into avx512 code > > - Formatting changes > > - Note that we cannot currently enable checksum offloading in > > CONFIGURE_VETH_OFFLOADS for check-system-userspace as > > netdev-linux.c currently only parses the vnet header if TSO > > is enabled. > > Since v10: > > - No change > > Since v11: > > - Added AVX512 IPv6 checksum offload support. > > - Improved error messages and logging. > > Since v12: > > - Added missing mutex annotations > > Since v13: > > - Added TUNGETFEATURES check in netdev-linux > > Since v14: > > - Only check TUNGETFEATURES once > > - Respect FLOW_TNL_F_CSUM flag > > --- > > lib/conntrack.c | 15 +- > > lib/dp-packet.c | 25 +++ > > lib/dp-packet.h | 78 +++++++++- > > lib/dpif-netdev-extract-avx512.c | 62 +++++++- > > lib/flow.c | 23 +++ > > lib/netdev-dpdk.c | 172 +++++++++++++++------ > > lib/netdev-linux.c | 258 ++++++++++++++++++++++--------- > > lib/netdev-native-tnl.c | 32 +--- > > lib/netdev.c | 46 ++---- > > lib/odp-execute-avx512.c | 88 +++++++---- > > lib/packets.c | 175 ++++++++++++++++----- > > lib/packets.h | 3 + > > 12 files changed, 710 insertions(+), 267 deletions(-) > > <snip> > > > @@ -5443,22 +5513,22 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev > > *netdev) > > } > > > > if (userspace_tso_enabled()) { > > - netdev->ol_flags |= NETDEV_TX_OFFLOAD_TCP_TSO; > > - netdev->ol_flags |= NETDEV_TX_OFFLOAD_TCP_CKSUM; > > - netdev->ol_flags |= NETDEV_TX_OFFLOAD_UDP_CKSUM; > > - netdev->ol_flags |= NETDEV_TX_OFFLOAD_SCTP_CKSUM; > > - netdev->ol_flags |= NETDEV_TX_OFFLOAD_IPV4_CKSUM; > > - vhost_unsup_flags = 1ULL << VIRTIO_NET_F_HOST_ECN > > - | 1ULL << VIRTIO_NET_F_HOST_UFO; > > + virtio_unsup_features = 1ULL << VIRTIO_NET_F_HOST_ECN > > + | 1ULL << VIRTIO_NET_F_HOST_UFO; > > + VLOG_DBG("%s: TSO enabled on vhost port", > > + netdev_get_name(&dev->up)); > > } else { > > - /* This disables checksum offloading and all the features > > - * that depends on it (TSO, UFO, ECN) according to virtio > > - * specification. */ > > - vhost_unsup_flags = 1ULL << VIRTIO_NET_F_CSUM; > > + /* Advertise checksum offloading to the guest, but explicitly > > + * disable TSO and friends. > > + * NOTE: we can't disable HOST_ECN which may have been wrongly > > + * negotiated by a running guest. */ > > + virtio_unsup_features = 1ULL << VIRTIO_NET_F_HOST_TSO4 > > + | 1ULL << VIRTIO_NET_F_HOST_TSO6 > > + | 1ULL << VIRTIO_NET_F_HOST_UFO; > > Hold on a second... Why exactly we can disable UFO, but can't disable ECN ? > > Previously, this branch of code was disabling VIRTIO_NET_F_CSUM, so neither > ECN or UFO should be negotiated, right? > > Or is it possible to have ECN negotiated without VIRTIO_NET_F_HOST_CSUM > enabled? > In that case, why the same is not true for UFO as well? > > What do I miss here?
Sorry, long mail in hope I am not missing anything: Pasting the v1.2 virtio spec https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-2210001: VIRTIO_NET_F_HOST_TSO4 Requires VIRTIO_NET_F_CSUM. VIRTIO_NET_F_HOST_TSO6 Requires VIRTIO_NET_F_CSUM. VIRTIO_NET_F_HOST_ECN Requires VIRTIO_NET_F_HOST_TSO4 or VIRTIO_NET_F_HOST_TSO6. VIRTIO_NET_F_HOST_UFO Requires VIRTIO_NET_F_CSUM. So I agree, one would expect that none of the HOST_TSO*/ECN/UFO features are successfully negotiated in the absence of VIRTIO_NET_F_CSUM. But, in practice for some released OVS versions, ECN does get negotiated without VIRTIO_NET_F_CSUM. It is a bug in vhost-user/ovs that do not invalidate ECN. I started ovs with tso disabled (before the recent changes), then looking at a vm first boot up: # git ll | head -1 22df63c38 - (HEAD) Documentation: Document netdev offload. (8 days ago) <Mike Pattrick> Qemu requests virtio features supported by vhost-user: Thread 17 "vhost-events" hit Breakpoint 1, vhost_user_get_features (pdev=0x7f6263dfc638, msg=0x7f6263dfc640, main_fd=78) at ../lib/vhost/vhost_user.c:325 325 { Missing separate debuginfos, use: yum debuginfo-install libevent-2.1.8-5.el8.x86_64 libibverbs-44.0-2.el8.1.x86_64 unbound-libs-1.16.2-2.el8.x86_64 (gdb) n 327 uint64_t features = 0; (gdb) n 329 if (validate_msg_fds(msg, 0) != 0) (gdb) n 332 rte_vhost_driver_get_features(dev->ifname, &features); (gdb) n 334 msg->payload.u64 = features; (gdb) p features $1 = 57921677258 Which translates to: ./features.sh 57921677258 VIRTIO_NET_F_GUEST_CSUM VIRTIO_NET_F_MTU VIRTIO_NET_F_GSO VIRTIO_NET_F_GUEST_TSO4 VIRTIO_NET_F_GUEST_TSO6 VIRTIO_NET_F_GUEST_ECN VIRTIO_NET_F_GUEST_UFO VIRTIO_NET_F_HOST_ECN VIRTIO_NET_F_MRG_RXBUF VIRTIO_NET_F_CTRL_VQ VIRTIO_NET_F_CTRL_RX VIRTIO_NET_F_GUEST_ANNOUNCE VIRTIO_NET_F_MQ Unknown feature bit: 26 VIRTIO_F_ANY_LAYOUT VIRTIO_RING_F_INDIRECT_DESC VIRTIO_RING_F_EVENT_IDX ! VHOST_USER_F_PROTOCOL_FEATURES VIRTIO_F_VERSION_1 VIRTIO_F_RING_PACKED VIRTIO_F_IN_ORDER ECN is incorrectly exposed. qemu could filter this incorrect feature, but unfortunately, later: Thread 17 "vhost-events" hit Breakpoint 2, vhost_user_set_features (pdev=0x7f6263dfc638, msg=0x7f6263dfc640, main_fd=78) at ../lib/vhost/vhost_user.c:369 369 { (gdb) n 372 uint64_t vhost_features = 0; (gdb) p features $2 = 6176155522 Which translates to: $ ./features.sh 6176155522 VIRTIO_NET_F_GUEST_CSUM VIRTIO_NET_F_GUEST_TSO4 VIRTIO_NET_F_GUEST_TSO6 VIRTIO_NET_F_GUEST_ECN VIRTIO_NET_F_GUEST_UFO VIRTIO_NET_F_HOST_ECN VIRTIO_NET_F_MRG_RXBUF VIRTIO_NET_F_GUEST_ANNOUNCE VIRTIO_RING_F_INDIRECT_DESC VIRTIO_RING_F_EVENT_IDX ! VHOST_USER_F_PROTOCOL_FEATURES VIRTIO_F_VERSION_1 Now... why is UFO different? This may be due to a partial clearing when disabling linear buffer feature in the vhost library. https://git.dpdk.org/dpdk-stable/tree/lib/vhost/socket.c?h=22.11#n930 If this is the case, there may be a potential issue with really old VM that were booted prior to v19.11 commit: 70c774768908 ("vhost: disable host TSO for linear buffers without extbuf") and have lived until now. So I went and downgraded to ovs 2.11 (dpdk 18.11). I can see both ECN and UFO on VHOST_USER_GET_FEATURES: $ ./features.sh 57921693642 VIRTIO_NET_F_GUEST_CSUM VIRTIO_NET_F_MTU VIRTIO_NET_F_GSO VIRTIO_NET_F_GUEST_TSO4 VIRTIO_NET_F_GUEST_TSO6 VIRTIO_NET_F_GUEST_ECN VIRTIO_NET_F_GUEST_UFO VIRTIO_NET_F_HOST_ECN VIRTIO_NET_F_HOST_UFO VIRTIO_NET_F_MRG_RXBUF VIRTIO_NET_F_CTRL_VQ VIRTIO_NET_F_CTRL_RX VIRTIO_NET_F_GUEST_ANNOUNCE VIRTIO_NET_F_MQ Unknown feature bit: 26 VIRTIO_F_ANY_LAYOUT VIRTIO_RING_F_INDIRECT_DESC VIRTIO_RING_F_EVENT_IDX ! VHOST_USER_F_PROTOCOL_FEATURES VIRTIO_F_VERSION_1 VIRTIO_F_RING_PACKED VIRTIO_F_IN_ORDER And.. so qemu sets VHOST_USER_SET_FEATURES: $ ./features.sh 6176171906 VIRTIO_NET_F_GUEST_CSUM VIRTIO_NET_F_GUEST_TSO4 VIRTIO_NET_F_GUEST_TSO6 VIRTIO_NET_F_GUEST_ECN VIRTIO_NET_F_GUEST_UFO VIRTIO_NET_F_HOST_ECN VIRTIO_NET_F_HOST_UFO VIRTIO_NET_F_MRG_RXBUF VIRTIO_NET_F_GUEST_ANNOUNCE VIRTIO_RING_F_INDIRECT_DESC VIRTIO_RING_F_EVENT_IDX ! VHOST_USER_F_PROTOCOL_FEATURES VIRTIO_F_VERSION_1 As a conclusion, we may have to handle both incorrect setting of UFO and ECN :-(. We probably missed this issue during validation as people are not testing upgrades from 2.11 to current master. Did I miss any other concern of yours? -- David Marchand _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
