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 <skaplons@redhat.com>
Acked-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

2 files changed, 45 insertions, 4 deletions

diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 911f55fe7..f82208ff2 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -2963,12 +2963,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));
     }
 
@@ -5959,13 +5965,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) {
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 0f09d3713..bb7b77ebb 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -1558,6 +1558,42 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=4" | ofctl_strip], [0], [dnl
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_BANNER([QoS])
+
+AT_SETUP([QoS - basic configuration])
+AT_SKIP_IF([test $HAVE_TC = no])
+OVS_TRAFFIC_VSWITCHD_START()
+
+ADD_NAMESPACES(at_ns0, at_ns1)
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
+ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
+
+dnl Adding a custom qdisc to ovs-p1, ovs-p0 will have the default qdisc.
+AT_CHECK([tc qdisc add dev ovs-p1 root noqueue])
+AT_CHECK([tc qdisc show dev ovs-p1 | grep -q noqueue])
+
+dnl Configure the same QoS for both ports.
+AT_CHECK([ovs-vsctl set port ovs-p0 qos=@qos -- set port ovs-p1 qos=@qos dnl
+            -- --id=@qos create qos dnl
+               type=linux-htb other-config:max-rate=3000000 queues:0=@queue dnl
+            -- --id=@queue create queue dnl
+               other_config:min-rate=2000000 other_config:max-rate=3000000 dnl
+               other_config:burst=3000000],
+         [ignore], [ignore])
+
+dnl Wait for qdiscs to be applied.
+OVS_WAIT_UNTIL([tc qdisc show dev ovs-p0 | grep -q htb])
+OVS_WAIT_UNTIL([tc qdisc show dev ovs-p1 | grep -q htb])
+
+dnl Check the configuration.
+m4_define([HTB_CONF], [rate 2Mbit ceil 3Mbit burst 375000b cburst 375000b])
+AT_CHECK([tc class show dev ovs-p0 | grep -q 'class htb .* HTB_CONF'])
+AT_CHECK([tc class show dev ovs-p1 | grep -q 'class htb .* HTB_CONF'])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_BANNER([conntrack])
 
 AT_SETUP([conntrack - controller])