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-
> February/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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
