On 7/6/23 21:42, David Marchand wrote:
> At some point in OVS history, some virtio features were announced as
> supported (ECN and UFO virtio features).
>
> The userspace TSO code, which has been added later, does not support
> those features and tries to disable them.
>
> This breaks OVS upgrades: if an existing VM already negotiated such
> features, their lack on reconnection to an upgraded OVS triggers a
> vhost socket disconnection by Qemu.
> This results in an endless loop because Qemu then retries with the same
> set of virtio features.
>
> This patch proposes to try and detect those vhost socket disconnection
> and fallback restoring the old virtio features (and disabling TSO for this
> vhost port).
>
> Acked-by: Mike Pattrick <[email protected]>
> Acked-by: Simon Horman <[email protected]>
> Signed-off-by: David Marchand <[email protected]>
> ---
> Changelog since v4:
> - I kept acks as the logic behind the state machine did not change much,
> - fixed indent of enumeration in documentation,
> - used status:<field> in documentation instead of grep -o,
> - renamed "disabled_tso" as "userspace-tso",
> - switched to a state machine with flags,
> - removed note on byte padding in netdev_dpdk struct,
Thanks, David! A few nits inline.
Best regards, Ilya Maximets.
>
> Changelog since v3:
> - updated documentation now that the interface offloads status is reported
> in ovsdb,
> - fixed one coding style issue,
>
> Changelog since v2:
> - reported workaround presence in the ovsdb port status field and
> updated documentation accordingly,
> - tried to use "better" names, to distinguish ECN virtio feature from
> TSO OVS netdev feature,
>
> Changelog since v1:
> - added a note in the documentation,
> - fixed vhost unregister trigger (so that both disabling and re-enabling
> TSO is handled),
> - cleared netdev features when disabling TSO,
> - changed level and ratelimited log message on vhost socket disconnect,
>
> ---
> Documentation/topics/userspace-tso.rst | 26 ++++++-
> lib/netdev-dpdk.c | 98 ++++++++++++++++++++++++--
> 2 files changed, 118 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/topics/userspace-tso.rst
> b/Documentation/topics/userspace-tso.rst
> index 5a43c2e86b..c4b15f2604 100644
> --- a/Documentation/topics/userspace-tso.rst
> +++ b/Documentation/topics/userspace-tso.rst
> @@ -68,7 +68,7 @@ as follows.
> connection is established, `TSO` is thus advertised to the guest as an
> available feature:
>
> -QEMU Command Line Parameter::
> +1. QEMU Command Line Parameter::
>
> $ sudo $QEMU_DIR/x86_64-softmmu/qemu-system-x86_64 \
> ...
> @@ -77,12 +77,34 @@ QEMU Command Line Parameter::
> ...
>
> 2. Ethtool. Assuming that the guest's OS also supports `TSO`, ethtool can be
> -used to enable same::
> + used to enable same::
>
> $ ethtool -K eth0 sg on # scatter-gather is a prerequisite for TSO
> $ ethtool -K eth0 tso on
> $ ethtool -k eth0
>
> +**Note:** Enabling this feature impacts the virtio features exposed by the
> DPDK
> +vHost User backend to a guest. If a guest was already connected to OvS before
> +enabling TSO and restarting OvS, this guest ports won't have TSO available::
> +
> + $ ovs-vsctl get interface vhost0 status:tx_tcp_seg_offload
> + "false"
> +
> +To help diagnose the issue, those ports have some additional information in
> +their status field in ovsdb::
> +
> + $ ovs-vsctl get interface vhost0 status:userspace-tso
> + disabled
> +
> +To restore TSO for this guest ports, this guest QEMU process must be stopped,
> +then started again. OvS will then report::
> +
> + $ ovs-vsctl get interface vhost0 status:tx_tcp_seg_offload
> + "true"
> +
> + $ ovs-vsctl get interface vhost0 status:userspace-tso
> + ovs-vsctl: no key "userspace-tso" in Interface record "vhost0" column
> status
> +
> ~~~~~~~~~~~
> Limitations
> ~~~~~~~~~~~
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 63dac689e3..ae28eaa6b6 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -418,6 +418,18 @@ enum dpdk_hw_ol_features {
> NETDEV_TX_TSO_OFFLOAD = 1 << 7,
> };
>
> +/* Flags for the netdev_dpdk virtio_features_state field.
> + * This is used for the virtio features recovery mechanism linked to TSO
> + * support. */
> +#define OVS_VIRTIO_F_CLEAN (UINT8_C(1) << 0)
> +#define OVS_VIRTIO_F_WORKAROUND (UINT8_C(1) << 1)
> +#define OVS_VIRTIO_F_NEGOTIATED (UINT8_C(1) << 2)
> +#define OVS_VIRTIO_F_RECONF_PENDING (UINT8_C(1) << 3)
> +#define OVS_VIRTIO_F_CLEAN_NEGOTIATED (OVS_VIRTIO_F_CLEAN \
> + | OVS_VIRTIO_F_NEGOTIATED)
> +#define OVS_VIRTIO_F_WORKAROUND_NEGOTIATED (OVS_VIRTIO_F_WORKAROUND \
> + | OVS_VIRTIO_F_NEGOTIATED)
Maybe break the lines before the '(' ? Might be easier to read:
#define OVS_VIRTIO_F_CLEAN_NEGOTIATED \
(OVS_VIRTIO_F_CLEAN | OVS_VIRTIO_F_NEGOTIATED)
#define OVS_VIRTIO_F_WORKAROUND_NEGOTIATED \
(OVS_VIRTIO_F_WORKAROUND | OVS_VIRTIO_F_NEGOTIATED)
(not sure about extra spaces)
> +
> /*
> * In order to avoid confusion in variables names, following naming
> convention
> * should be used, if possible:
> @@ -474,7 +486,11 @@ struct netdev_dpdk {
> bool vhost_reconfigured;
>
> atomic_uint8_t vhost_tx_retries_max;
> - /* 2 pad bytes here. */
> +
> + /* Flags for virtio features recovery mechanism. */
> + uint8_t virtio_features_state;
> +
> + /* 1 pad byte here. */
> );
>
> PADDED_MEMBERS(CACHE_LINE_SIZE,
> @@ -1359,6 +1375,7 @@ common_construct(struct netdev *netdev, dpdk_port_t
> port_no,
> dev->requested_lsc_interrupt_mode = 0;
> ovsrcu_index_init(&dev->vid, -1);
> dev->vhost_reconfigured = false;
> + dev->virtio_features_state = OVS_VIRTIO_F_CLEAN;
> dev->attached = false;
> dev->started = false;
> dev->reset_needed = false;
> @@ -3883,6 +3900,12 @@ netdev_dpdk_vhost_user_get_status(const struct netdev
> *netdev,
> xasprintf("%d", vring.size));
> }
>
> + if (userspace_tso_enabled()
> + && dev->virtio_features_state & OVS_VIRTIO_F_WORKAROUND) {
> +
> + smap_add_format(args, "userspace-tso", "disabled");
> + }
> +
> ovs_mutex_unlock(&dev->mutex);
> return 0;
> }
> @@ -4245,6 +4268,10 @@ new_device(int vid)
> newnode = dev->socket_id;
> }
>
> + if (!(dev->virtio_features_state & OVS_VIRTIO_F_NEGOTIATED)) {
> + dev->virtio_features_state |= OVS_VIRTIO_F_NEGOTIATED;
> + }
Can we do this unconditionally?
> +
> if (dev->requested_n_txq < qp_num
> || dev->requested_n_rxq < qp_num
> || dev->requested_socket_id != newnode
> @@ -4268,7 +4295,9 @@ new_device(int vid)
> dev->hw_ol_features |= NETDEV_TX_SCTP_CKSUM_OFFLOAD;
> }
>
> - if (userspace_tso_enabled()) {
> + if (userspace_tso_enabled()
> + && dev->virtio_features_state & OVS_VIRTIO_F_CLEAN) {
If this needed? There is no harm in using these features on Tx if negotiated.
> +
> if (features & (1ULL << VIRTIO_NET_F_GUEST_TSO4)
> && features & (1ULL << VIRTIO_NET_F_GUEST_TSO6)) {
>
> @@ -4524,6 +4553,41 @@ destroy_connection(int vid)
> dev->requested_n_txq = qp_num;
> netdev_request_reconfigure(&dev->up);
> }
> +
> + if (!(dev->virtio_features_state & OVS_VIRTIO_F_NEGOTIATED)) {
> + /* The socket disconnected before reaching new_device. It
> + * likely means that the guest did not agree with the virtio
> + * features. */
> + VLOG_WARN_RL(&rl, "Connection on socket '%s' closed during "
> + "initialization.", dev->vhost_id);
> + }
> + if (!(dev->virtio_features_state & OVS_VIRTIO_F_RECONF_PENDING))
> {
> + switch (dev->virtio_features_state) {
> + case OVS_VIRTIO_F_CLEAN:
> + dev->virtio_features_state = OVS_VIRTIO_F_WORKAROUND;
> + break;
An empty line between cases.
> + case OVS_VIRTIO_F_WORKAROUND:
> + dev->virtio_features_state = OVS_VIRTIO_F_CLEAN;
> + break;
> + case OVS_VIRTIO_F_CLEAN_NEGOTIATED:
> + /* The virtio features were clean and got accepted by the
> + * guest. We expect it will be the case in the future and
> + * change nothing. */
> + break;
> + case OVS_VIRTIO_F_WORKAROUND_NEGOTIATED:
> + /* Let's try to go with clean virtio features on the next
> + * connection. */
> + dev->virtio_features_state = OVS_VIRTIO_F_CLEAN;
> + break;
> + default:
> + OVS_NOT_REACHED();
> + }
> + if (!(dev->virtio_features_state & OVS_VIRTIO_F_NEGOTIATED))
> {
> + dev->virtio_features_state |=
> OVS_VIRTIO_F_RECONF_PENDING;
> + netdev_request_reconfigure(&dev->up);
> + }
> + }
> +
> ovs_mutex_unlock(&dev->mutex);
> exists = true;
> break;
> @@ -5454,10 +5518,34 @@ static int
> netdev_dpdk_vhost_client_reconfigure(struct netdev *netdev)
> {
> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> + bool unregister = false;
> + bool enable_tso;
> + char *vhost_id;
> int err;
>
> ovs_mutex_lock(&dev->mutex);
>
> + /* This vhost-user port was registered to the vhost library already, but
> + * a socket disconnection happened and configuration must be re-evaluated
> + * wrt dev->virtio_features_state. */
This comment is related to the 'if' statement below and not to the 'enable_tso'
variable. Also, it's written as if the condition is true, which is a bit
misleading when a comment is outside.
> + enable_tso = userspace_tso_enabled()
> + && dev->virtio_features_state & OVS_VIRTIO_F_CLEAN;
Should this be moved under the 'Configure vHost client mode' condition?
> + if (dev->vhost_driver_flags & RTE_VHOST_USER_CLIENT && dev->vhost_id
> + && dev->virtio_features_state & OVS_VIRTIO_F_RECONF_PENDING) {
> +
> + dev->vhost_driver_flags &= ~RTE_VHOST_USER_CLIENT;
> + vhost_id = dev->vhost_id;
> + unregister = true;
> + }
> +
> + ovs_mutex_unlock(&dev->mutex);
> +
> + if (unregister) {
> + dpdk_vhost_driver_unregister(dev, vhost_id);
> + }
> +
> + ovs_mutex_lock(&dev->mutex);
> +
> /* Configure vHost client mode if requested and if the following criteria
> * are met:
> * 1. Device hasn't been registered yet.
> @@ -5467,6 +5555,8 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev
> *netdev)
> uint64_t virtio_unsup_features = 0;
> uint64_t vhost_flags = 0;
>
> + dev->virtio_features_state &= ~OVS_VIRTIO_F_RECONF_PENDING;
> +
> /* Register client-mode device. */
> vhost_flags |= RTE_VHOST_USER_CLIENT;
>
> @@ -5487,7 +5577,7 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev
> *netdev)
> }
>
> /* Enable External Buffers if TCP Segmentation Offload is enabled. */
> - if (userspace_tso_enabled()) {
> + if (enable_tso) {
> vhost_flags |= RTE_VHOST_USER_EXTBUF_SUPPORT;
> }
>
> @@ -5512,7 +5602,7 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev
> *netdev)
> goto unlock;
> }
>
> - if (userspace_tso_enabled()) {
> + if (enable_tso) {
> virtio_unsup_features = 1ULL << VIRTIO_NET_F_HOST_ECN
> | 1ULL << VIRTIO_NET_F_HOST_UFO;
> VLOG_DBG("%s: TSO enabled on vhost port",
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
