Hi David,
On 9/20/22 17:50, David Marchand wrote:
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 <[email protected]> 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.
Yes, it is not straightforward to understand, I thought the existing
logging in rte_vhost_driver_register() would be enough:
VHOST_LOG_CONFIG(path, INFO, "Linear buffers requested without external
buffers,\n");
VHOST_LOG_CONFIG(path, INFO, "disabling host segmentation offloading
support\n");
But having an explicit comment here would help to get it without having
to run it.
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);
I think above patch makes sense.
Thanks,
Maxime
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
