| Message ID | 533fdfd63b607cf92f262cefa49dbd8df441468e.1732879710.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Dumitru Ceara |
| Headers | show |
| Series | [ovs-dev,v2] 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 11/29/24 12:36, 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. > However, rely on default value for veth and tap interfaces since the > reported link speed is not accurate in this case. > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- > Changes since v1: > - use default max-rate value for veth and tap interfaces > --- > controller/binding.c | 20 ++++++++++++++++---- > controller/ovn-controller.c | 1 + > 2 files changed, 17 insertions(+), 4 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 117d0ff5d..7371ac433 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,18 @@ 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); I'd add an empty line here. May probably be added on commit. > + const char *drv_name = smap_get_def(&iface->status, "driver_name", ""); > + /* Link speed for virtual interfaces (e.g. veth or tap is inaccurate > + * so use default value for them while rely on link speed for real > + * NICs. */ The ')' and a comma before 'so' are missing. I assume, comments above can be fixed on commit. With that: Acked-by: Ilya Maximets <i.maximets@ovn.org> > + if (!strcmp(drv_name, "veth") || !strcmp(drv_name, "tap") || > + !iface->n_link_speed) { > + smap_add_format(&other_config, "max-rate", "%lld", > + OVN_QOS_MAX_RATE); > + } else { > + smap_add_format(&other_config, "max-rate", "%lld", > + (long long int) iface->link_speed[0]); > + } > ovsrec_qos_set_other_config(qos, &other_config); > smap_clear(&other_config); > > @@ -391,9 +403,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 187f600b1..f988b233e 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -830,6 +830,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);
On 1/17/25 1:45 PM, Ilya Maximets wrote: > On 11/29/24 12:36, 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. >> However, rely on default value for veth and tap interfaces since the >> reported link speed is not accurate in this case. >> >> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> >> --- >> Changes since v1: >> - use default max-rate value for veth and tap interfaces >> --- >> controller/binding.c | 20 ++++++++++++++++---- >> controller/ovn-controller.c | 1 + >> 2 files changed, 17 insertions(+), 4 deletions(-) >> >> diff --git a/controller/binding.c b/controller/binding.c >> index 117d0ff5d..7371ac433 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,18 @@ 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); > > I'd add an empty line here. May probably be added on commit. > > > >> + const char *drv_name = smap_get_def(&iface->status, "driver_name", ""); >> + /* Link speed for virtual interfaces (e.g. veth or tap is inaccurate >> + * so use default value for them while rely on link speed for real >> + * NICs. */ > > The ')' and a comma before 'so' are missing. > > I assume, comments above can be fixed on commit. > > With that: > Acked-by: Ilya Maximets <i.maximets@ovn.org> > Thanks, Lorenzo and Ilya! I took care of fixing up the small issues Ilya pointed out above and then I applied the patch to main, 24.09 and 24.03. Regards, Dumitru
diff --git a/controller/binding.c b/controller/binding.c index 117d0ff5d..7371ac433 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,18 @@ 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); + const char *drv_name = smap_get_def(&iface->status, "driver_name", ""); + /* Link speed for virtual interfaces (e.g. veth or tap is inaccurate + * so use default value for them while rely on link speed for real + * NICs. */ + if (!strcmp(drv_name, "veth") || !strcmp(drv_name, "tap") || + !iface->n_link_speed) { + smap_add_format(&other_config, "max-rate", "%lld", + OVN_QOS_MAX_RATE); + } else { + smap_add_format(&other_config, "max-rate", "%lld", + (long long int) iface->link_speed[0]); + } ovsrec_qos_set_other_config(qos, &other_config); smap_clear(&other_config); @@ -391,9 +403,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 187f600b1..f988b233e 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -830,6 +830,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);
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. However, rely on default value for veth and tap interfaces since the reported link speed is not accurate in this case. Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- Changes since v1: - use default max-rate value for veth and tap interfaces --- controller/binding.c | 20 ++++++++++++++++---- controller/ovn-controller.c | 1 + 2 files changed, 17 insertions(+), 4 deletions(-)