From 02be2c318c8ef3255b62541ec3de53bd6f325c7a Mon Sep 17 00:00:00 2001 From: Ilya Maximets Date: Mon, 31 Oct 2022 17:17:59 +0100 Subject: netdev-linux: Fix inability to apply QoS on ports with custom qdiscs. tc_del_qdisc() function only removes qdiscs with handle '1:0'. If for some reason the interface has a qdisc with non-zero handle attached, tc_del_qdisc() will not delete it and subsequent tc_install() will fail to install a new qdisc. The problem is that Libvirt by default is setting noqueue qdisc for all tap interfaces it creates. This is done for performance reasons to ensure lockless xmit. The issue is causing non-working QoS in OpenStack setups since new versions of Libvirt started to use OVS to configure it. In the past, Libvirt configured TC on its own, bypassing OVS. Removing the handle value from the deletion request, so any qdisc can be removed. Changing the error checking to also pass ENOENT, since that is the error reported if only default qdisc is present. Alternative solution might be to use NLM_F_REPLACE, but that will be a larger change with a potential need of refactoring. Potential side effect of the change is that OVS may start removing qdiscs that it didn't remove before. Though it's not a new issue and 'linux-noop' QoS type should be used for ports that OVS should not touch. Otherwise, OVS owns qdiscs on all interfaces attached to it. While at it, adding more logs as errors are not logged in any way at the moment making the issue hard to debug. Reported-at: https://bugzilla.redhat.com/2138339 Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2022-October/052088.html Reported-at: https://github.com/openvswitch/ovs-issues/issues/268 Suggested-by: Slawek Kaplonski Acked-by: Eelco Chaudron Signed-off-by: Ilya Maximets --- lib/netdev-linux.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) (limited to 'lib') diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 7ea4070c2..59e8dc0ae 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -2984,12 +2984,18 @@ netdev_linux_set_qos(struct netdev *netdev_, /* Delete existing qdisc. */ error = tc_del_qdisc(netdev_); if (error) { + VLOG_WARN_RL(&rl, "%s: Failed to delete existing qdisc: %s", + netdev_get_name(netdev_), ovs_strerror(error)); goto exit; } ovs_assert(netdev->tc == NULL); /* Install new qdisc. */ error = new_ops->tc_install(netdev_, details); + if (error) { + VLOG_WARN_RL(&rl, "%s: Failed to install new qdisc: %s", + netdev_get_name(netdev_), ovs_strerror(error)); + } ovs_assert((error == 0) == (netdev->tc != NULL)); } @@ -6143,13 +6149,12 @@ tc_del_qdisc(struct netdev *netdev_) if (!tcmsg) { return ENODEV; } - tcmsg->tcm_handle = tc_make_handle(1, 0); tcmsg->tcm_parent = TC_H_ROOT; error = tc_transact(&request, NULL); - if (error == EINVAL) { - /* EINVAL probably means that the default qdisc was in use, in which - * case we've accomplished our purpose. */ + if (error == EINVAL || error == ENOENT) { + /* EINVAL or ENOENT probably means that the default qdisc was in use, + * in which case we've accomplished our purpose. */ error = 0; } if (!error && netdev->tc) { -- cgit v1.2.1