Ah, I noticed after reviewing that the patches were already merged. Sorry. I'll submit my suggestions as a follow-up patch.
On Thu, Feb 2, 2017 at 1:33 AM, Russell Bryant <russ...@ovn.org> wrote: > > > On Wed, Feb 1, 2017 at 12:22 PM, Ben Pfaff <b...@ovn.org> wrote: > >> 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 isn't related to this commit, but last time I was looking at this I > noticed that QoS also doesn't work when traffic leaves the host on a > physical network (via a logical switch with a localnet port) instead of via > a tunnel. It also doesn't work between ports on the same host, but I'm not > sure if that really matters. > > >> 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.re...@nutanix.com> >> Reported-by: Hexin Wang <hexin.w...@nutanix.com> >> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-Febr >> uary/043564.html >> Signed-off-by: Ben Pfaff <b...@ovn.org> >> --- >> ovn/controller/binding.c | 41 +++++++++++++++++++++++++++++++++++------ >> 1 file changed, 35 insertions(+), 6 deletions(-) >> > > Acked-by: Russell Bryant <russ...@ovn.org> > > but note the comment inline ... > > >> @@ -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, ""); >> } >> > > I think we need a netdev_close(netdev_phy); here. > > There's also another "return;" where netdev_close was missed in the code > just above these changes in the same function. > > Otherwise, lgtm. > > >> + return; >> + } >> + >> + /* Configure qdisc. */ >> + if (strcmp(qdisc_type, OVN_QOS_TYPE)) { >> + set_qos_type(netdev_phy, OVN_QOS_TYPE); >> } >> >> /* Check and delete if needed. */ >> -- >> 2.10.2 >> >> _______________________________________________ >> dev mailing list >> d...@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > > > > -- > Russell Bryant > -- Russell Bryant _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev