[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