On 6/23/23 15:16, David Marchand wrote:
> On Wed, Jun 21, 2023 at 12:13 AM Ilya Maximets <i.maxim...@ovn.org> 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 <f...@sysclose.org>
>>> Co-authored-by: Flavio Leitner <f...@sysclose.org>
>>> Signed-off-by: Mike Pattrick <m...@redhat.com>
>>> ---
>>> 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.
Yeah, that is no good.
> It is a bug in vhost-user/ovs that do not invalidate ECN.
It might have been better if vhost-user library didn't enable any
features that require support on the application side, unless
application asked for them. Currently the application needs to
know all the features that the library "supports" and disable them.
Though I'm not sure if that can actually be changed at this point.
>
> 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:
Do you know which check in QEMU code triggers a disconnection?
>
> 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 :-(.
But we can't really do that from the OVS side, can we?
DPDK will clear the bit even if we enable it...
> 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?
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
