Re: [ovs-dev] [PATCH 2/2] ovn-controller: Configure interface QoS only if it would actually be used.

[Ben Pfaff]

On Thu, Feb 02, 2017 at 01:33:36AM -0500, Russell Bryant wrote:
> On Wed, Feb 1, 2017 at 12:22 PM, Ben Pfaff  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.

Does this just come down to adding a set_queue() in the right place in
the logical or physical flows somewhere?  It would be nice to make this
work properly.

Thank you for finding the leak.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] ovn-controller: Configure interface QoS only if it would actually be used.

[Russell Bryant]

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  wrote:

>
>
> On Wed, Feb 1, 2017 at 12:22 PM, Ben Pfaff  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 
>> Reported-by: Hexin Wang 
>> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-Febr
>> uary/043564.html
>> Signed-off-by: Ben Pfaff 
>> ---
>>  ovn/controller/binding.c | 41 +++--
>>  1 file changed, 35 insertions(+), 6 deletions(-)
>>
>
> Acked-by: Russell Bryant 
>
> but note the comment inline ...
>
>
>> @@ -257,12 +268,30 @@ setup_qos(const char *egress_iface, struct hmap
>> *queue_map)
>>  }
>>  smap_destroy(_details);
>>
>> -if (strcmp(qdisc_type, OVN_QOS_TYPE)) {
>> -error = netdev_set_qos(netdev_phy, OVN_QOS_TYPE, NULL);
>> -if (error) {
>> -VLOG_WARN_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