[ovs-dev] [PATCH 2/2] ovn-controller: Configure interface QoS only if it would actually be used.
Russell Bryant
russell at ovn.org
Thu Feb 2 06:35:10 UTC 2017
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 <russell at ovn.org> wrote:
>
>
> On Wed, Feb 1, 2017 at 12:22 PM, Ben Pfaff <blp at 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.rekhi at nutanix.com>
>> Reported-by: Hexin Wang <hexin.wang at nutanix.com>
>> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-Febr
>> uary/043564.html
>> Signed-off-by: Ben Pfaff <blp at ovn.org>
>> ---
>> ovn/controller/binding.c | 41 +++++++++++++++++++++++++++++++++++------
>> 1 file changed, 35 insertions(+), 6 deletions(-)
>>
>
> Acked-by: Russell Bryant <russell at 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
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
>
>
> --
> Russell Bryant
>
--
Russell Bryant
More information about the dev
mailing list