| Message ID | 20221101140032.734440-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Delegated to: | Dumitru Ceara |
| Headers | show |
| Series | [ovs-dev] controller: Fix QoS for ports with undetected or too low link speed. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On Tue, Nov 1, 2022 at 3:01 PM Ilya Maximets <i.maximets@ovn.org> 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 <i.maximets@ovn.org> > --- > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks. Acked-by: Ales Musil <amusil@redhat.com>
On 11/15/22 07:45, Ales Musil wrote: > On Tue, Nov 1, 2022 at 3:01 PM Ilya Maximets <i.maximets@ovn.org> 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 <i.maximets@ovn.org> >> --- >> 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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > Looks good to me, thanks. > > Acked-by: Ales Musil <amusil@redhat.com> Thanks, Ales and Ilya! I pushed this to the main branch and backported it to all stable branches down to 22.03. Regards, Dumitru
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')" = ""])
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 <i.maximets@ovn.org> --- controller/binding.c | 9 ++++++++- tests/system-ovn.at | 6 +++++- 2 files changed, 13 insertions(+), 2 deletions(-)