| Message ID | 0c2921a51056a18495bbaeb21687d5657eb371d1.1730717990.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] controller: binding: Set HTB root max-rate according to the link speed. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Mon, Nov 4, 2024 at 12:17 PM Lorenzo Bianconi < lorenzo.bianconi@redhat.com> wrote: > Set HTB Qdisc scheduler root max-rate according to the reported > link-speed if it is available (use default value if the link_speed is > not reported by OvS). This patch allows more precise shaping when the > NIC speed is lower than 34Gbps (current default value) and even take into > account cases where NIC capacity is greater than 34 Gbps. > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > Hi Lorenzo, there is missing fixes tag: Fixes: 7d1d111ff213 ("controller: configure qos through ovs qos table and do not run tc directly") --- > controller/binding.c | 11 +++++++---- > controller/ovn-controller.c | 1 + > tests/system-ovn.at | 2 +- > 3 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 16d64e534..b453e55ad 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -244,6 +244,7 @@ add_or_del_qos_port(const char *ovn_port, bool add) > static bool > add_ovs_qos_table_entry(struct ovsdb_idl_txn *ovs_idl_txn, > const struct ovsrec_port *port, > + const struct ovsrec_interface *iface, > unsigned long long min_rate, > unsigned long long max_rate, > unsigned long long burst, > @@ -262,7 +263,9 @@ add_ovs_qos_table_entry(struct ovsdb_idl_txn > *ovs_idl_txn, > qos = ovsrec_qos_insert(ovs_idl_txn); > ovsrec_qos_set_type(qos, OVN_QOS_TYPE); > ovsrec_port_set_qos(port, qos); > - smap_add_format(&other_config, "max-rate", "%lld", > OVN_QOS_MAX_RATE); > + unsigned long long link_speed = > + iface->n_link_speed ? iface->link_speed[0] : OVN_QOS_MAX_RATE; > + smap_add_format(&other_config, "max-rate", "%lld", link_speed); > ovsrec_qos_set_other_config(qos, &other_config); > smap_clear(&other_config); > > @@ -391,9 +394,9 @@ configure_qos(const struct sbrec_port_binding *pb, > } > if (iface) { > /* Add new QoS entries. */ > - if (add_ovs_qos_table_entry(ovs_idl_txn, port, min_rate, > - max_rate, burst, queue_id, > - pb->logical_port)) { > + if (add_ovs_qos_table_entry(ovs_idl_txn, port, iface, > + min_rate, max_rate, burst, > + queue_id, pb->logical_port)) { > if (!q) { > q = xzalloc(sizeof *q); > hmap_insert(qos_map, &q->node, hash); > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index e87274121..38287dbf2 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -827,6 +827,7 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl) > ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_options); > ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_ofport); > ovsdb_idl_track_add_column(ovs_idl, > &ovsrec_interface_col_external_ids); > + ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_link_speed); > ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_name); > ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_interfaces); > ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_external_ids); > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index 6dfc3055a..c9d7a7a75 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -6756,7 +6756,7 @@ AT_CHECK([ovn-nbctl remove Logical_Switch_Port > public options qos_min_rate=20000 > 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 12Kbit ceil 34359Mbit burst > 375000b cburst 373662b']) > + grep -q 'class htb .* rate 12Kbit ceil 10Gbit burst > 375000b cburst 365Kb']) > > 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.47.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Other than that it looks good. Acked-by: Ales Musil <amusil@redhat.com>
On 11/4/24 12:09, Lorenzo Bianconi wrote: > Set HTB Qdisc scheduler root max-rate according to the reported > link-speed if it is available (use default value if the link_speed is > not reported by OvS). This patch allows more precise shaping when the > NIC speed is lower than 34Gbps (current default value) and even take into > account cases where NIC capacity is greater than 34 Gbps. Could you, please, describe the problem in more details, i.e. why the shaping is not precise with the current code? This information is essential in the commit message. Also, there is a large comment above add_ovs_qos_table_entry() function about link speed not being reliable at least for virtual ports. This comment will need an adjustment, but also this patch will unnecessarily limit the speed of all the veth and tap interfaces in OVN. We need a way to avoid that. Best regards, Ilya Maximets.
diff --git a/controller/binding.c b/controller/binding.c index 16d64e534..b453e55ad 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -244,6 +244,7 @@ add_or_del_qos_port(const char *ovn_port, bool add) static bool add_ovs_qos_table_entry(struct ovsdb_idl_txn *ovs_idl_txn, const struct ovsrec_port *port, + const struct ovsrec_interface *iface, unsigned long long min_rate, unsigned long long max_rate, unsigned long long burst, @@ -262,7 +263,9 @@ add_ovs_qos_table_entry(struct ovsdb_idl_txn *ovs_idl_txn, qos = ovsrec_qos_insert(ovs_idl_txn); ovsrec_qos_set_type(qos, OVN_QOS_TYPE); ovsrec_port_set_qos(port, qos); - smap_add_format(&other_config, "max-rate", "%lld", OVN_QOS_MAX_RATE); + unsigned long long link_speed = + iface->n_link_speed ? iface->link_speed[0] : OVN_QOS_MAX_RATE; + smap_add_format(&other_config, "max-rate", "%lld", link_speed); ovsrec_qos_set_other_config(qos, &other_config); smap_clear(&other_config); @@ -391,9 +394,9 @@ configure_qos(const struct sbrec_port_binding *pb, } if (iface) { /* Add new QoS entries. */ - if (add_ovs_qos_table_entry(ovs_idl_txn, port, min_rate, - max_rate, burst, queue_id, - pb->logical_port)) { + if (add_ovs_qos_table_entry(ovs_idl_txn, port, iface, + min_rate, max_rate, burst, + queue_id, pb->logical_port)) { if (!q) { q = xzalloc(sizeof *q); hmap_insert(qos_map, &q->node, hash); diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index e87274121..38287dbf2 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -827,6 +827,7 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl) ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_options); ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_ofport); ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_external_ids); + ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_link_speed); ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_name); ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_interfaces); ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_external_ids); diff --git a/tests/system-ovn.at b/tests/system-ovn.at index 6dfc3055a..c9d7a7a75 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -6756,7 +6756,7 @@ AT_CHECK([ovn-nbctl remove Logical_Switch_Port public options qos_min_rate=20000 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 12Kbit ceil 34359Mbit burst 375000b cburst 373662b']) + grep -q 'class htb .* rate 12Kbit ceil 10Gbit burst 375000b cburst 365Kb']) 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')" = ""])
Set HTB Qdisc scheduler root max-rate according to the reported link-speed if it is available (use default value if the link_speed is not reported by OvS). This patch allows more precise shaping when the NIC speed is lower than 34Gbps (current default value) and even take into account cases where NIC capacity is greater than 34 Gbps. Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- controller/binding.c | 11 +++++++---- controller/ovn-controller.c | 1 + tests/system-ovn.at | 2 +- 3 files changed, 9 insertions(+), 5 deletions(-)