I did not finish reviewing the rest, but I have an additional comment
on the virtio/vhost features negotiations.
On Wed, Jul 13, 2022 at 8:45 PM Mike Pattrick <m...@redhat.com> wrote:
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 811c62a87..a69bab829 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
[snip]
> @@ -3938,6 +3980,37 @@ new_device(int vid)
> dev->vhost_reconfigured = true;
> }
>
> + uint64_t features;
> + if (rte_vhost_get_negotiated_features(vid, &features)) {
> + VLOG_INFO("Error checking guest features for "
> + "vHost Device '%s'", dev->vhost_id);
> + } else {
> + if (features & (1ULL << VIRTIO_NET_F_GUEST_CSUM)) {
> + dev->hw_ol_features |= NETDEV_TX_TCP_CKSUM_OFFLOAD;
> + dev->hw_ol_features |= NETDEV_TX_UDP_CKSUM_OFFLOAD;
> + dev->hw_ol_features |= NETDEV_TX_SCTP_CKSUM_OFFLOAD;
> + }
> +
> + if (userspace_tso_enabled()) {
> + if (features & (1ULL << VIRTIO_NET_F_GUEST_TSO4)
> + && features & (1ULL << VIRTIO_NET_F_GUEST_TSO6)) {
> +
> + dev->hw_ol_features |= NETDEV_TX_TSO_OFFLOAD;
> + VLOG_DBG("%s: TSO enabled on vhost port",
> + netdev_get_name(&dev->up));
> + } else {
> + VLOG_WARN("%s: Tx TSO offload is not supported.",
> + netdev_get_name(&dev->up));
> + }
> + }
> + }
> +
> + /* There is no support in virtio net to offload IPv4 csum,
> + * but the vhost library handles IPv4 csum offloading fine. */
> + dev->hw_ol_features |= NETDEV_TX_IPV4_CKSUM_OFFLOAD;
> +
> + netdev_dpdk_update_netdev_flags(dev);
> +
> ovsrcu_index_set(&dev->vid, vid);
> exists = true;
>
> @@ -4001,6 +4074,14 @@ destroy_device(int vid)
> dev->up.n_rxq * sizeof *dev->vhost_rxq_enabled);
> netdev_dpdk_txq_map_clear(dev);
>
> + /* Clear offload capabilities before next new_device. */
> + dev->hw_ol_features &= ~NETDEV_TX_IPV4_CKSUM_OFFLOAD;
> + dev->hw_ol_features &= ~NETDEV_TX_TCP_CKSUM_OFFLOAD;
> + dev->hw_ol_features &= ~NETDEV_TX_UDP_CKSUM_OFFLOAD;
> + dev->hw_ol_features &= ~NETDEV_TX_SCTP_CKSUM_OFFLOAD;
> + dev->hw_ol_features &= ~NETDEV_TX_TSO_OFFLOAD;
> + netdev_dpdk_update_netdev_flags(dev);
> +
> netdev_change_seq_changed(&dev->up);
> ovs_mutex_unlock(&dev->mutex);
> exists = true;
> @@ -4938,22 +5019,7 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
> }
>
> err = dpdk_eth_dev_init(dev);
> -
> - if (dev->hw_ol_features & NETDEV_TX_IPV4_CKSUM_OFFLOAD) {
> - netdev->ol_flags |= NETDEV_TX_OFFLOAD_IPV4_CKSUM;
> - } else {
> - netdev->ol_flags &= ~NETDEV_TX_OFFLOAD_IPV4_CKSUM;
> - }
> -
> - if (dev->hw_ol_features & NETDEV_TX_TSO_OFFLOAD) {
> - 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_IPV4_CKSUM;
> - if (dev->hw_ol_features & NETDEV_TX_SCTP_CHECKSUM_OFFLOAD) {
> - netdev->ol_flags |= NETDEV_TX_OFFLOAD_SCTP_CKSUM;
> - }
> - }
> + netdev_dpdk_update_netdev_flags(dev);
>
> /* If both requested and actual hwaddr were previously
> * unset (initialized to 0), then first device init above
> @@ -4995,11 +5061,6 @@ dpdk_vhost_reconfigure_helper(struct netdev_dpdk *dev)
> dev->tx_q[0].map = 0;
> }
>
> - if (userspace_tso_enabled()) {
> - dev->hw_ol_features |= NETDEV_TX_TSO_OFFLOAD;
> - VLOG_DBG("%s: TSO enabled on vhost port", netdev_get_name(&dev->up));
> - }
> -
> netdev_dpdk_remap_txqs(dev);
>
> if (netdev_dpdk_get_vid(dev) >= 0) {
> @@ -5020,6 +5081,8 @@ dpdk_vhost_reconfigure_helper(struct netdev_dpdk *dev)
> }
> }
>
> + netdev_dpdk_update_netdev_flags(dev);
> +
> return 0;
> }
>
> @@ -5042,7 +5105,7 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev
> *netdev)
> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> int err;
> uint64_t vhost_flags = 0;
> - uint64_t vhost_unsup_flags;
> + uint64_t vhost_unsup_flags = 0;
>
> ovs_mutex_lock(&dev->mutex);
>
> @@ -5094,19 +5157,17 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev
> *netdev)
> goto unlock;
> }
>
> + 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;
We don't need ol_flags updates here.
Before new_device, we don't know what the guest supports, so no need
to report some tx capability to OVS upper layers.
ol_flags are set once and for all during new_device.
> +
> if (userspace_tso_enabled()) {
> netdev->ol_flags |= NETDEV_TX_OFFLOAD_TCP_TSO;
Idem.
> - 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;
> - } 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;
So far, VIRTIO_NET_F_CSUM served as a barrier, preventing a guest from
negotiating TSO virtio features bits (HOST_TSO4, HOST_TSO6 or
HOST_UFO).
*In theory*, if we go with this patch and just remove
VIRTIO_NET_F_CSUM in the non tso case, I would have expected a (newly
started) guest see those HOST_TSO* features as available, since OVS is
not disabling them via rte_vhost_driver_disable_features().
*But in practice*, on my RHEL8, a guest only negotiates 0x17020a783
features, iow:
VIRTIO_NET_F_CSUM
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
After scratching my head for some time, I understand that this
behavior comes from the vhost library filtering features based on
vhost-user flags passed at register:
https://git.dpdk.org/dpdk-stable/tree/lib/vhost/socket.c?h=21.11#n917
Since OVS did not enable extbuf support (which is under tso check),
the vhost library implictly disables those HOST_TSO* virtio features.
This is a bit hard to understand unless you know the internals of the
vhost library.
I suggest updating this patch to help future people looking into this code.
One solution would be to add comments in the code explaining why a
guest can't see HOST_TSO* features.
But I recommend OVS explicitly disables those HOST_TSO* and HOST_UFO
features (something like diff below).
> + VLOG_DBG("%s: TSO enabled on vhost port",
> + netdev_get_name(&dev->up));
> }
>
> err = rte_vhost_driver_disable_features(dev->vhost_id,
I have been running with the diff below on top of this patch (inline
patch might get broken because of gmail.. sorry).
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 069107d48..d3c9be045 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -4007,6 +4007,7 @@ new_device(int vid)
ovs_mutex_lock(&dev->mutex);
if (nullable_string_is_equal(ifname, dev->vhost_id)) {
uint32_t qp_num = rte_vhost_get_vring_num(vid) / VIRTIO_QNUM;
+ uint64_t features;
/* Get NUMA information */
newnode = rte_vhost_get_numa_node(vid);
@@ -4031,7 +4032,6 @@ new_device(int vid)
dev->vhost_reconfigured = true;
}
- uint64_t features;
if (rte_vhost_get_negotiated_features(vid, &features)) {
VLOG_INFO("Error checking guest features for "
"vHost Device '%s'", dev->vhost_id);
@@ -5155,8 +5155,6 @@ netdev_dpdk_vhost_client_reconfigure(struct
netdev *netdev)
{
struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
int err;
- uint64_t vhost_flags = 0;
- uint64_t vhost_unsup_flags = 0;
ovs_mutex_lock(&dev->mutex);
@@ -5166,6 +5164,9 @@ netdev_dpdk_vhost_client_reconfigure(struct
netdev *netdev)
* 2. A path has been specified.
*/
if (!(dev->vhost_driver_flags & RTE_VHOST_USER_CLIENT) && dev->vhost_id) {
+ uint64_t virtio_unsup_features;
+ uint64_t vhost_flags = 0;
+
/* Register client-mode device. */
vhost_flags |= RTE_VHOST_USER_CLIENT;
@@ -5208,21 +5209,21 @@ netdev_dpdk_vhost_client_reconfigure(struct
netdev *netdev)
goto unlock;
}
- 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;
-
if (userspace_tso_enabled()) {
- netdev->ol_flags |= NETDEV_TX_OFFLOAD_TCP_TSO;
- vhost_unsup_flags = 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));
+ virtio_unsup_features = 1ULL << VIRTIO_NET_F_HOST_ECN
+ | 1ULL << VIRTIO_NET_F_HOST_UFO;
+ } else {
+ /* 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;
}
err = rte_vhost_driver_disable_features(dev->vhost_id,
- vhost_unsup_flags);
+ virtio_unsup_features);
if (err) {
VLOG_ERR("rte_vhost_driver_disable_features failed for "
"vhost user client port: %s\n", dev->up.name);
--
David Marchand
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
