ovn-controller: Configure interface QoS only if it would actually be …

…used.

Until now, ovn-controller has unconditionally configured linux-htb on
physical interfaces.  QoS is pretty much always trouble, but it's even more
trouble if we set it up for no good reason.  We received a bug report, in
particular, that doing this disrupts connectivity in Docker.

This commit attempts to make that less likely, by making ovn-controller
only configure a qdisc if QoS support has in turn been configured in OVN.
The same problems as before will recur if QoS support is actually
configured, but at least now there's some purpose, and possibly a symptom
that the user can better diagnose ("I turned on QoS and OVN stopped
working" is at least a cause-and-effect chain that makes some sense).

Reported-by: Ritesh Rekhi <ritesh.rekhi@nutanix.com>
Reported-by: Hexin Wang <hexin.wang@nutanix.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-February/043564.html
Tested-by: Hexin Wang <hexin.wang@nutanix.com>
Tested-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-February/043575.html
Signed-off-by: Ben Pfaff <blp@ovn.org>

ovn/controller/binding.c

@@ -226,6 +226,17 @@ set_noop_qos(struct controller_ctx *ctx, struct sset *egress_ifaces)
     return true;
 }
 
+static void
+set_qos_type(struct netdev *netdev, const char *type)
+{
+    int error = netdev_set_qos(netdev, type, NULL);
+    if (error) {
+        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
+        VLOG_WARN_RL(&rl, "%s: could not set qdisc type \"%s\" (%s)",
+                     netdev_get_name(netdev), type, ovs_strerror(error));
+    }
+}
+
 static void
 setup_qos(const char *egress_iface, struct hmap *queue_map)
 {
@@ -244,7 +255,7 @@ setup_qos(const char *egress_iface, struct hmap *queue_map)
         return;
     }
 
-    /* Check and configure qdisc. */
+    /* Check current qdisc. */
     const char *qdisc_type;
     struct smap qdisc_details;
 
@@ -257,12 +268,30 @@ setup_qos(const char *egress_iface, struct hmap *queue_map)
     }
     smap_destroy(&qdisc_details);
 
-    if (strcmp(qdisc_type, OVN_QOS_TYPE)) {
-        error = netdev_set_qos(netdev_phy, OVN_QOS_TYPE, NULL);
-        if (error) {
-            VLOG_WARN_RL(&rl, "%s: could not configure QoS (%s)",
-                         egress_iface, ovs_strerror(error));
+    /* If we're not actually being requested to do any QoS:
+     *
+     *     - If the current qdisc type is OVN_QOS_TYPE, then we clear the qdisc
+     *       type to "".  Otherwise, it's possible that our own leftover qdisc
+     *       settings could cause strange behavior on egress.  Also, QoS is
+     *       expensive and may waste CPU time even if it's not really in use.
+     *
+     *       OVN isn't the only software that can configure qdiscs, and
+     *       physical interfaces are shared resources, so there is some risk in
+     *       this strategy: we could disrupt some other program's QoS.
+     *       Probably, to entirely avoid this possibility we would need to add
+     *       a configuration setting.
+     *
+     *     - Otherwise leave the qdisc alone. */
+    if (hmap_is_empty(queue_map)) {
+        if (!strcmp(qdisc_type, OVN_QOS_TYPE)) {
+            set_qos_type(netdev_phy, "");
         }
+        return;
+    }
+
+    /* Configure qdisc. */
+    if (strcmp(qdisc_type, OVN_QOS_TYPE)) {
+        set_qos_type(netdev_phy, OVN_QOS_TYPE);
     }
 
     /* Check and delete if needed. */