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]>
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
[email protected] IM: amusil
<https://red.ht/sig>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
