On 11/15/22 07:45, Ales Musil wrote: > On Tue, Nov 1, 2022 at 3:01 PM Ilya Maximets <[email protected]> wrote: > >> If netdev_set_qos() is called without specifying max-rate, it will >> use reported link speed of that interface instead as a ceil. >> >> If OVS libraries are not able to detect the link speed, the 100 Mbps >> will be used. Also, TAP interfaces do report 10 Mbps as their link >> speed by default. >> >> In both cases all the traffic will be unnecessary limited to just >> 100 or 10 Mpbs regardless of the 'qos_max_rate' configured for that >> port in OVN databases, because maximum rate of a single queue can not >> be higher than a total maximum rate of a QoS type. >> >> Fix that by always specifying the max-rate as a maximum supported >> value - (2^32 - 1) * 8 bits per second. This way netdev_set_qos() >> will not need to check the link speed or guess a value. >> >> Fixes: 8dda482b8059 ("Check and allocate free qdisc queue id for ports >> with qos parameters") >> Reported-at: https://bugzilla.redhat.com/2136716 >> Signed-off-by: Ilya Maximets <[email protected]> >> --- >> controller/binding.c | 9 ++++++++- >> tests/system-ovn.at | 6 +++++- >> 2 files changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/controller/binding.c b/controller/binding.c >> index c3d2b2e42..a50379895 100644 >> --- a/controller/binding.c >> +++ b/controller/binding.c >> @@ -220,7 +220,14 @@ set_noop_qos(struct ovsdb_idl_txn *ovs_idl_txn, >> static void >> set_qos_type(struct netdev *netdev, const char *type) >> { >> - int error = netdev_set_qos(netdev, type, NULL); >> + /* 34359738360 == (2^32 - 1) * 8. netdev_set_qos() doesn't support >> + * 64-bit rate netlink attributes, so the maximum value is 2^32 - 1 >> bytes. >> + * The 'max-rate' config option is in bits, so multiplying by 8. >> + * Without setting max-rate the reported link speed will be used, >> which >> + * can be unrecognized for certain NICs or reported too low for >> virtual >> + * interfaces. */ >> + const struct smap conf = SMAP_CONST1(&conf, "max-rate", >> "34359738360"); >> + int error = netdev_set_qos(netdev, type, &conf); >> if (error) { >> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); >> VLOG_WARN_RL(&rl, "%s: could not set qdisc type \"%s\" (%s)", >> diff --git a/tests/system-ovn.at b/tests/system-ovn.at >> index 20c058415..cc267ba25 100644 >> --- a/tests/system-ovn.at >> +++ b/tests/system-ovn.at >> @@ -6353,8 +6353,12 @@ OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev >> ovs-public']) >> OVS_WAIT_UNTIL([tc class show dev ovs-public | \ >> grep -q 'class htb .* rate 200Kbit ceil 300Kbit burst >> 375000b cburst 375000b']) >> >> -AT_CHECK([ovn-nbctl remove Logical_Switch_Port public options >> qos_min_rate=200000]) >> + >> AT_CHECK([ovn-nbctl remove Logical_Switch_Port public options >> qos_max_rate=300000]) >> +OVS_WAIT_UNTIL([tc class show dev ovs-public | \ >> + grep -q 'class htb .* rate 200Kbit ceil 34359Mbit burst >> 375000b .*']) >> + >> +AT_CHECK([ovn-nbctl remove Logical_Switch_Port public options >> qos_min_rate=200000]) >> AT_CHECK([ovn-nbctl remove Logical_Switch_Port public options >> qos_burst=3000000]) >> OVS_WAIT_UNTIL([test "$(tc qdisc show | grep 'htb 1: dev ovs-public')" = >> ""]) >> >> -- >> 2.37.3 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > Looks good to me, thanks. > > Acked-by: Ales Musil <[email protected]>
Thanks, Ales and Ilya! I pushed this to the main branch and backported it to all stable branches down to 22.03. Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
