| Message ID | 8554da1828dd12b9975158b5322993240b3c75ce.1670191403.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v4] binding: add the capability to apply QoS for lsp | 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 | success | github build: passed |
On Sun, Dec 4, 2022 at 5:06 PM Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote: > > Introduce the capability to apply QoS rules for logical switch ports > claimed by ovn-controller. Rely on shash instead of sset for > egress_ifaces. > > Acked-by: Mark Michelson <mmichels@redhat.com> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129742 > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> Hi Lorenzo, Thanks for addressing the comments. I tested this version and compared with the present main. I see a few differences. With the main version, when I configure qos params to a localnet logical port, ovn-controller creates a qdisc on the tunnel interface which I guess is expected. But I'm still not sure why we need to configure qdiscs on the tunnel interface. But that's a story. Perhaps I need to see the original QoS commits and understand why it was added. With your patch, I don't see that happening. Perhaps there is a bug in setup_qos() function now that 'shash' is used to store the egress ifaces with the logical port and there is no logical port for the tunnel interfaces. Regarding the option - "ovn-egress-iface". I suppose the expectation from the CMS is to set the qos parameters in the logical port and also set this option in the ovs interface right ? I don't really see a need for this. I think if CMS configures the qos parameters in the logical port options, ovn-controller should configure the qos. I think otherwise this would complicate the CMS implementation. Thoughts ? Please see a few comments below > --- > Changes since v3: > - fix typo in new system-ovn test > Changes since v2: > - fix qos configuration restarting ovn-controller > Changes since v1: > - improve ovs interface lookup > - improve system-tests > --- > controller/binding.c | 155 ++++++++++++++++++++++-------------- > controller/binding.h | 5 +- > controller/ovn-controller.c | 15 ++-- > tests/system-ovn.at | 48 +++++++++++ > 4 files changed, 156 insertions(+), 67 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 5df62baef..53520263c 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -115,6 +115,7 @@ struct qos_queue { > uint32_t min_rate; > uint32_t max_rate; > uint32_t burst; > + char *port_name; > }; > > void > @@ -147,25 +148,50 @@ static void update_lport_tracking(const struct sbrec_port_binding *pb, > struct hmap *tracked_dp_bindings, > bool claimed); > > +static bool is_lport_vif(const struct sbrec_port_binding *pb); > + > +static struct qos_queue * > +get_qos_map_entry(struct hmap *queue_map, const char *name) > +{ > + struct qos_queue *qos_node; > + HMAP_FOR_EACH (qos_node, node, queue_map) { > + if (!strcmp(qos_node->port_name, name)) { > + return qos_node; > + } > + } > + > + return NULL; > +} > + > static void > -get_qos_params(const struct sbrec_port_binding *pb, struct hmap *queue_map) > +update_qos_params(const struct sbrec_port_binding *pb, struct hmap *queue_map) > { > uint32_t min_rate = smap_get_int(&pb->options, "qos_min_rate", 0); > uint32_t max_rate = smap_get_int(&pb->options, "qos_max_rate", 0); > uint32_t burst = smap_get_int(&pb->options, "qos_burst", 0); > uint32_t queue_id = smap_get_int(&pb->options, "qdisc_queue_id", 0); > > + struct qos_queue *node = get_qos_map_entry(queue_map, pb->logical_port); > + > if ((!min_rate && !max_rate && !burst) || !queue_id) { > /* Qos is not configured for this port. */ > + if (node) { > + hmap_remove(queue_map, &node->node); > + free(node->port_name); > + free(node); > + } > return; > } > > - struct qos_queue *node = xzalloc(sizeof *node); > - hmap_insert(queue_map, &node->node, hash_int(queue_id, 0)); > + if (!node) { > + node = xzalloc(sizeof *node); > + hmap_insert(queue_map, &node->node, hash_int(queue_id, 0)); > + node->port_name = xstrdup(pb->logical_port); > + } > + node->queue_id = queue_id; > node->min_rate = min_rate; > node->max_rate = max_rate; > node->burst = burst; > - node->queue_id = queue_id; > } > > static const struct ovsrec_qos * > @@ -191,7 +217,7 @@ static bool > set_noop_qos(struct ovsdb_idl_txn *ovs_idl_txn, > const struct ovsrec_port_table *port_table, > const struct ovsrec_qos_table *qos_table, > - struct sset *egress_ifaces) > + struct shash *egress_ifaces) > { > if (!ovs_idl_txn) { > return false; > @@ -206,11 +232,11 @@ set_noop_qos(struct ovsdb_idl_txn *ovs_idl_txn, > size_t count = 0; > > OVSREC_PORT_TABLE_FOR_EACH (port, port_table) { > - if (sset_contains(egress_ifaces, port->name)) { > + if (shash_find(egress_ifaces, port->name)) { > ovsrec_port_set_qos(port, noop_qos); > count++; > } > - if (sset_count(egress_ifaces) == count) { > + if (shash_count(egress_ifaces) == count) { > break; > } > } > @@ -236,7 +262,8 @@ set_qos_type(struct netdev *netdev, const char *type) > } > > static void > -setup_qos(const char *egress_iface, struct hmap *queue_map) > +setup_qos(const char *egress_iface, const char *logical_port, > + struct hmap *queue_map) > { > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); > struct netdev *netdev_phy; > @@ -281,7 +308,7 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) > * a configuration setting. > * > * - Otherwise leave the qdisc alone. */ > - if (hmap_is_empty(queue_map)) { > + if (!get_qos_map_entry(queue_map, logical_port)) { > if (!strcmp(qdisc_type, OVN_QOS_TYPE)) { > set_qos_type(netdev_phy, ""); > } > @@ -338,6 +365,10 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) > continue; > } > > + if (strcmp(sb_info->port_name, logical_port)) { > + continue; > + } > + > smap_clear(&queue_details); > smap_add_format(&queue_details, "min-rate", "%d", sb_info->min_rate); > smap_add_format(&queue_details, "max-rate", "%d", sb_info->max_rate); > @@ -354,11 +385,12 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) > netdev_close(netdev_phy); > } > > -static void > +void > destroy_qos_map(struct hmap *qos_map) > { > struct qos_queue *qos_queue; > HMAP_FOR_EACH_POP (qos_queue, node, qos_map) { > + free(qos_queue->port_name); > free(qos_queue); > } > > @@ -404,7 +436,7 @@ sbrec_get_port_encap(const struct sbrec_chassis *chassis_rec, > static void > add_localnet_egress_interface_mappings( > const struct sbrec_port_binding *port_binding, > - struct shash *bridge_mappings, struct sset *egress_ifaces) > + struct shash *bridge_mappings, struct shash *egress_ifaces) > { > const char *network = smap_get(&port_binding->options, "network_name"); > if (!network) { > @@ -429,7 +461,8 @@ add_localnet_egress_interface_mappings( > if (!is_egress_iface) { > continue; > } > - sset_add(egress_ifaces, iface_rec->name); > + shash_add(egress_ifaces, iface_rec->name, > + port_binding->logical_port); > } > } > } > @@ -474,7 +507,7 @@ update_ld_multichassis_ports(const struct sbrec_port_binding *binding_rec, > static void > update_ld_localnet_port(const struct sbrec_port_binding *binding_rec, > struct shash *bridge_mappings, > - struct sset *egress_ifaces, > + struct shash *egress_ifaces, > struct hmap *local_datapaths) > { > /* Ignore localnet ports for unplugged networks. */ > @@ -1456,7 +1489,7 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, > b_ctx_out->tracked_dp_bindings); > } > if (b_lport->lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) { > - get_qos_params(pb, qos_map); > + update_qos_params(pb, qos_map); > } > } else { > /* We could, but can't claim the lport. */ > @@ -1519,6 +1552,16 @@ consider_vif_lport(const struct sbrec_port_binding *pb, > b_lport = local_binding_add_lport(binding_lports, lbinding, pb, > LP_VIF); > } > + > + if (lbinding->iface && > + smap_get(&lbinding->iface->external_ids, "ovn-egress-iface")) { > + const char *iface_id = smap_get(&lbinding->iface->external_ids, > + "iface-id"); > + if (iface_id) { > + shash_add(b_ctx_out->egress_ifaces, lbinding->iface->name, > + iface_id); When a qos is configured on a logical port, this patch is adding 2 entries in the 'egress_ifaces' shash for the same logical port. I think it's because of the above 'shash_add'. I think you should use shash_replace instead. When a full recompute happens, the function build_local_bindings() also adds the qos configured logical ports to the 'egress_ifaces' and later if there are any updates to the logical port, this function - consider_vif_lport() also adds it to the shash. IMO the qos support in OVN needs a different approach. Instead of configuring the qos using netdev() perhaps we should rely on the OVS meters. Maybe this was brought up earlier too. I'm of the opinion that instead of supporting Qos for logical ports using netdev, we should use OVS meters (not just for logical ports, even for localnet ports). Thoughts ? @Dumitru Ceara @Han Zhou @Mark Michelson @Ilya Maximets Do you have any comments or suggestions here ? Thanks Numan > + } > + } > } > > return consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out, > @@ -1785,7 +1828,7 @@ consider_localnet_lport(const struct sbrec_port_binding *pb, > update_local_lports(pb->logical_port, b_ctx_out); > > if (qos_map && b_ctx_in->ovs_idl_txn) { > - get_qos_params(pb, qos_map); > + update_qos_params(pb, qos_map); > } > > update_related_lport(pb, b_ctx_out); > @@ -1861,14 +1904,14 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in, > &b_ctx_out->lbinding_data->bindings; > for (j = 0; j < port_rec->n_interfaces; j++) { > const struct ovsrec_interface *iface_rec; > + struct local_binding *lbinding = NULL; > > iface_rec = port_rec->interfaces[j]; > iface_id = smap_get(&iface_rec->external_ids, "iface-id"); > int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; > > if (iface_id && ofport > 0) { > - struct local_binding *lbinding = > - local_binding_find(local_bindings, iface_id); > + lbinding = local_binding_find(local_bindings, iface_id); > if (!lbinding) { > lbinding = local_binding_create(iface_id, iface_rec); > local_binding_add(local_bindings, lbinding); > @@ -1895,8 +1938,11 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in, > const char *tunnel_iface > = smap_get(&iface_rec->status, "tunnel_egress_iface"); > if (tunnel_iface) { > - sset_add(b_ctx_out->egress_ifaces, tunnel_iface); > + shash_add(b_ctx_out->egress_ifaces, tunnel_iface, ""); > } > + } else if (lbinding && smap_get(&iface_rec->external_ids, > + "ovn-egress-iface")) { > + shash_add(b_ctx_out->egress_ifaces, iface_rec->name, iface_id); > } > } > } > @@ -1910,16 +1956,11 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > } > > struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings); > - struct hmap qos_map; > > - hmap_init(&qos_map); > if (b_ctx_in->br_int) { > build_local_bindings(b_ctx_in, b_ctx_out); > } > > - struct hmap *qos_map_ptr = > - !sset_is_empty(b_ctx_out->egress_ifaces) ? &qos_map : NULL; > - > struct ovs_list localnet_lports = OVS_LIST_INITIALIZER(&localnet_lports); > struct ovs_list external_lports = OVS_LIST_INITIALIZER(&external_lports); > struct ovs_list multichassis_ports = OVS_LIST_INITIALIZER( > @@ -1956,7 +1997,8 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > break; > > case LP_VIF: > - consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL, qos_map_ptr); > + consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL, > + b_ctx_out->qos_map); > if (pb->additional_chassis) { > struct lport *multichassis_lport = xmalloc( > sizeof *multichassis_lport); > @@ -1967,11 +2009,13 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > break; > > case LP_CONTAINER: > - consider_container_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); > + consider_container_lport(pb, b_ctx_in, b_ctx_out, > + b_ctx_out->qos_map); > break; > > case LP_VIRTUAL: > - consider_virtual_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); > + consider_virtual_lport(pb, b_ctx_in, b_ctx_out, > + b_ctx_out->qos_map); > break; > > case LP_L2GATEWAY: > @@ -1994,7 +2038,8 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > break; > > case LP_LOCALNET: { > - consider_localnet_lport(pb, b_ctx_in, b_ctx_out, &qos_map); > + consider_localnet_lport(pb, b_ctx_in, b_ctx_out, > + b_ctx_out->qos_map); > struct lport *lnet_lport = xmalloc(sizeof *lnet_lport); > lnet_lport->pb = pb; > ovs_list_push_back(&localnet_lports, &lnet_lport->list_node); > @@ -2051,17 +2096,15 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > > shash_destroy(&bridge_mappings); > > - if (!sset_is_empty(b_ctx_out->egress_ifaces) > + if (!shash_is_empty(b_ctx_out->egress_ifaces) > && set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, > b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) { > - const char *entry; > - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > - setup_qos(entry, &qos_map); > + struct shash_node *entry; > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > + setup_qos(entry->name, entry->data, b_ctx_out->qos_map); > } > } > > - destroy_qos_map(&qos_map); > - > cleanup_claimed_port_timestamps(); > } > > @@ -2447,7 +2490,7 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, > } > > if (smap_get(&iface_rec->external_ids, "ovn-egress-iface") || > - sset_contains(b_ctx_out->egress_ifaces, iface_rec->name)) { > + shash_find(b_ctx_out->egress_ifaces, iface_rec->name)) { > handled = false; > break; > } > @@ -2493,10 +2536,6 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, > return false; > } > > - struct hmap qos_map = HMAP_INITIALIZER(&qos_map); > - struct hmap *qos_map_ptr = > - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; > - > /* > * We consider an OVS interface for claiming if the following > * 2 conditions are met: > @@ -2525,24 +2564,22 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, > if (iface_id && ofport > 0 && > is_iface_in_int_bridge(iface_rec, b_ctx_in->br_int)) { > handled = consider_iface_claim(iface_rec, iface_id, b_ctx_in, > - b_ctx_out, qos_map_ptr); > + b_ctx_out, b_ctx_out->qos_map); > if (!handled) { > break; > } > } > } > > - if (handled && qos_map_ptr && set_noop_qos(b_ctx_in->ovs_idl_txn, > - b_ctx_in->port_table, > - b_ctx_in->qos_table, > - b_ctx_out->egress_ifaces)) { > - const char *entry; > - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > - setup_qos(entry, &qos_map); > + if (handled && > + set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, > + b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) { > + struct shash_node *entry; > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > + setup_qos(entry->name, entry->data, b_ctx_out->qos_map); > } > } > > - destroy_qos_map(&qos_map); > return handled; > } > > @@ -2977,10 +3014,6 @@ delete_done: > return false; > } > > - struct hmap qos_map = HMAP_INITIALIZER(&qos_map); > - struct hmap *qos_map_ptr = > - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; > - > SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, > b_ctx_in->port_binding_table) { > /* Loop to handle create and update changes only. */ > @@ -2992,7 +3025,8 @@ delete_done: > update_ld_peers(pb, b_ctx_out->local_datapaths); > } > > - handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, qos_map_ptr); > + handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, > + b_ctx_out->qos_map); > if (!handled) { > break; > } > @@ -3009,7 +3043,8 @@ delete_done: > sset_find_and_delete(b_ctx_out->postponed_ports, port_name); > continue; > } > - handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, qos_map_ptr); > + handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, > + b_ctx_out->qos_map); > if (!handled) { > break; > } > @@ -3055,17 +3090,15 @@ delete_done: > shash_destroy(&bridge_mappings); > } > > - if (handled && qos_map_ptr && set_noop_qos(b_ctx_in->ovs_idl_txn, > - b_ctx_in->port_table, > - b_ctx_in->qos_table, > - b_ctx_out->egress_ifaces)) { > - const char *entry; > - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > - setup_qos(entry, &qos_map); > + if (handled && > + set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, > + b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) { > + struct shash_node *entry; > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > + setup_qos(entry->name, entry->data, b_ctx_out->qos_map); > } > } > > - destroy_qos_map(&qos_map); > return handled; > } > > diff --git a/controller/binding.h b/controller/binding.h > index 6c3a98b02..8be33eddb 100644 > --- a/controller/binding.h > +++ b/controller/binding.h > @@ -91,7 +91,8 @@ struct binding_ctx_out { > */ > bool non_vif_ports_changed; > > - struct sset *egress_ifaces; > + struct shash *egress_ifaces; > + struct hmap *qos_map; > /* smap of OVS interface name as key and > * OVS interface external_ids:iface-id as value. */ > struct smap *local_iface_ids; > @@ -195,6 +196,8 @@ void set_pb_chassis_in_sbrec(const struct sbrec_port_binding *pb, > const struct sbrec_chassis *chassis_rec, > bool is_set); > > +void destroy_qos_map(struct hmap *qos_map); > + > /* Corresponds to each Port_Binding.type. */ > enum en_lport_type { > LP_UNKNOWN, > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index d6251afb8..066d76869 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -1276,7 +1276,8 @@ struct ed_type_runtime_data { > struct sset active_tunnels; > > /* runtime data engine private data. */ > - struct sset egress_ifaces; > + struct shash egress_ifaces; > + struct hmap qos_map; > struct smap local_iface_ids; > > /* Tracked data. See below for more details and comments. */ > @@ -1372,7 +1373,8 @@ en_runtime_data_init(struct engine_node *node OVS_UNUSED, > sset_init(&data->local_lports); > related_lports_init(&data->related_lports); > sset_init(&data->active_tunnels); > - sset_init(&data->egress_ifaces); > + shash_init(&data->egress_ifaces); > + hmap_init(&data->qos_map); > smap_init(&data->local_iface_ids); > local_binding_data_init(&data->lbinding_data); > shash_init(&data->local_active_ports_ipv6_pd); > @@ -1392,7 +1394,8 @@ en_runtime_data_cleanup(void *data) > sset_destroy(&rt_data->local_lports); > related_lports_destroy(&rt_data->related_lports); > sset_destroy(&rt_data->active_tunnels); > - sset_destroy(&rt_data->egress_ifaces); > + shash_destroy(&rt_data->egress_ifaces); > + destroy_qos_map(&rt_data->qos_map); > smap_destroy(&rt_data->local_iface_ids); > local_datapaths_destroy(&rt_data->local_datapaths); > shash_destroy(&rt_data->local_active_ports_ipv6_pd); > @@ -1481,6 +1484,7 @@ init_binding_ctx(struct engine_node *node, > b_ctx_out->related_lports_changed = false; > b_ctx_out->non_vif_ports_changed = false; > b_ctx_out->egress_ifaces = &rt_data->egress_ifaces; > + b_ctx_out->qos_map = &rt_data->qos_map; > b_ctx_out->lbinding_data = &rt_data->lbinding_data; > b_ctx_out->local_iface_ids = &rt_data->local_iface_ids; > b_ctx_out->postponed_ports = rt_data->postponed_ports; > @@ -1510,13 +1514,14 @@ en_runtime_data_run(struct engine_node *node, void *data) > sset_destroy(local_lports); > related_lports_destroy(&rt_data->related_lports); > sset_destroy(active_tunnels); > - sset_destroy(&rt_data->egress_ifaces); > + shash_clear(&rt_data->egress_ifaces); > + destroy_qos_map(&rt_data->qos_map); > + hmap_init(&rt_data->qos_map); > smap_destroy(&rt_data->local_iface_ids); > hmap_init(local_datapaths); > sset_init(local_lports); > related_lports_init(&rt_data->related_lports); > sset_init(active_tunnels); > - sset_init(&rt_data->egress_ifaces); > smap_init(&rt_data->local_iface_ids); > local_binding_data_init(&rt_data->lbinding_data); > } > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index 3e904c9dc..c986198a2 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -6335,6 +6335,10 @@ ADD_NAMESPACES(sw01) > ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03") > ovn-nbctl lsp-add sw0 sw01 \ > -- lsp-set-addresses sw01 "f0:00:00:01:02:03 192.168.1.2" > +ADD_NAMESPACES(sw02) > +ADD_VETH(sw02, sw02, br-int, "192.168.1.3/24", "f2:00:00:01:02:03") > +ovn-nbctl lsp-add sw0 sw02 \ > + -- lsp-set-addresses sw02 "f2:00:00:01:02:03 192.168.1.3" > > ADD_NAMESPACES(public) > ADD_VETH(public, public, br-ext, "192.168.2.2/24", "f0:00:00:01:02:05") > @@ -6345,6 +6349,7 @@ ovn-nbctl lsp-add sw0 public \ > -- lsp-set-type public localnet \ > -- lsp-set-options public network_name=phynet > > +# Setup QoS on a localnet port > AT_CHECK([ovn-nbctl set Logical_Switch_Port public options:qos_min_rate=200000]) > AT_CHECK([ovn-nbctl set Logical_Switch_Port public options:qos_max_rate=300000]) > AT_CHECK([ovn-nbctl set Logical_Switch_Port public options:qos_burst=3000000]) > @@ -6353,15 +6358,58 @@ 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']) > > +# Setup QoS on a logical switch ports > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_min_rate=400000]) > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_max_rate=800000]) > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_burst=5000000]) > +AT_CHECK([ovs-vsctl set interface ovs-sw01 external-ids:ovn-egress-iface=true]) > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01']) > +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \ > + grep -q 'class htb .* rate 400Kbit ceil 800Kbit burst 625000b cburst 625000b']) > + > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qos_min_rate=600000]) > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qos_max_rate=6000000]) > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qos_burst=6000000]) > +AT_CHECK([ovs-vsctl set interface ovs-sw02 external-ids:ovn-egress-iface=true]) > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw02']) > +OVS_WAIT_UNTIL([tc class show dev ovs-sw02 | \ > + grep -q 'class htb .* rate 600Kbit ceil 6Mbit burst 750000b cburst 750000b']) > + > +AT_CHECK([ovn-appctl -t ovn-controller exit --restart]) > +OVS_WAIT_UNTIL([test "$(pidof ovn-controller)" = ""]) > +start_daemon ovn-controller > +OVS_WAIT_UNTIL([test "$(pidof ovn-controller)" != ""]) > + > +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']) > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01']) > +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \ > + grep -q 'class htb .* rate 400Kbit ceil 800Kbit burst 625000b cburst 625000b']) > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw02']) > +OVS_WAIT_UNTIL([tc class show dev ovs-sw02 | \ > + grep -q 'class htb .* rate 600Kbit ceil 6Mbit burst 750000b cburst 750000b']) > > 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_max_rate=300000]) > 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')" = ""]) > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_min_rate=200000]) > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01']) > +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \ > + grep -q 'class htb .* rate 200Kbit ceil 800Kbit burst 625000b cburst 625000b']) > + > +# Disable QoS rules from logical switch ports > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qdisc_queue_id=0]) > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qdisc_queue_id=0]) > +OVS_WAIT_UNTIL([test "$(tc qdisc show | grep 'htb 1: dev ovs-sw01')" = ""]) > +OVS_WAIT_UNTIL([test "$(tc qdisc show | grep 'htb 1: dev ovs-sw02')" = ""]) > + > kill $(pidof ovn-controller) > > as ovn-sb > -- > 2.38.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 12/9/22 04:18, Numan Siddique wrote: > On Sun, Dec 4, 2022 at 5:06 PM Lorenzo Bianconi > <lorenzo.bianconi@redhat.com> wrote: >> >> Introduce the capability to apply QoS rules for logical switch ports >> claimed by ovn-controller. Rely on shash instead of sset for >> egress_ifaces. >> >> Acked-by: Mark Michelson <mmichels@redhat.com> >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129742 >> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > Hi Lorenzo, > > Thanks for addressing the comments. > > I tested this version and compared with the present main. > > I see a few differences. > > With the main version, when I configure qos params to a localnet > logical port, ovn-controller creates a qdisc on the tunnel interface > which I guess is expected. > But I'm still not sure why we need to configure qdiscs on the tunnel > interface. But that's a story. Perhaps I need to see the original > QoS commits and understand why it was added. I'm not sure how that is supposed to work, but adding qdiscs to tunnel interfaces sounds wrong. It will affect all the traffic between nodes, not only particular ports, right? > > With your patch, I don't see that happening. Perhaps there is a bug > in setup_qos() function now that 'shash' is used to store the egress > ifaces with the logical port and > there is no logical port for the tunnel interfaces. > > Regarding the option - "ovn-egress-iface". I suppose the expectation > from the CMS is to set the qos parameters in the logical port and also > set this option in the > ovs interface right ? I don't really see a need for this. I think if > CMS configures the qos parameters in the logical port options, > ovn-controller should configure the qos. > I think otherwise this would complicate the CMS implementation. Thoughts ? > > > Please see a few comments below > >> --- >> Changes since v3: >> - fix typo in new system-ovn test >> Changes since v2: >> - fix qos configuration restarting ovn-controller >> Changes since v1: >> - improve ovs interface lookup >> - improve system-tests >> --- >> controller/binding.c | 155 ++++++++++++++++++++++-------------- >> controller/binding.h | 5 +- >> controller/ovn-controller.c | 15 ++-- >> tests/system-ovn.at | 48 +++++++++++ >> 4 files changed, 156 insertions(+), 67 deletions(-) >> >> diff --git a/controller/binding.c b/controller/binding.c >> index 5df62baef..53520263c 100644 >> --- a/controller/binding.c >> +++ b/controller/binding.c >> @@ -115,6 +115,7 @@ struct qos_queue { >> uint32_t min_rate; >> uint32_t max_rate; >> uint32_t burst; >> + char *port_name; >> }; >> >> void >> @@ -147,25 +148,50 @@ static void update_lport_tracking(const struct sbrec_port_binding *pb, >> struct hmap *tracked_dp_bindings, >> bool claimed); >> >> +static bool is_lport_vif(const struct sbrec_port_binding *pb); >> + >> +static struct qos_queue * >> +get_qos_map_entry(struct hmap *queue_map, const char *name) >> +{ >> + struct qos_queue *qos_node; >> + HMAP_FOR_EACH (qos_node, node, queue_map) { >> + if (!strcmp(qos_node->port_name, name)) { >> + return qos_node; >> + } >> + } >> + >> + return NULL; >> +} >> + >> static void >> -get_qos_params(const struct sbrec_port_binding *pb, struct hmap *queue_map) >> +update_qos_params(const struct sbrec_port_binding *pb, struct hmap *queue_map) >> { >> uint32_t min_rate = smap_get_int(&pb->options, "qos_min_rate", 0); >> uint32_t max_rate = smap_get_int(&pb->options, "qos_max_rate", 0); >> uint32_t burst = smap_get_int(&pb->options, "qos_burst", 0); >> uint32_t queue_id = smap_get_int(&pb->options, "qdisc_queue_id", 0); >> >> + struct qos_queue *node = get_qos_map_entry(queue_map, pb->logical_port); >> + >> if ((!min_rate && !max_rate && !burst) || !queue_id) { >> /* Qos is not configured for this port. */ >> + if (node) { >> + hmap_remove(queue_map, &node->node); >> + free(node->port_name); >> + free(node); >> + } >> return; >> } >> >> - struct qos_queue *node = xzalloc(sizeof *node); >> - hmap_insert(queue_map, &node->node, hash_int(queue_id, 0)); >> + if (!node) { >> + node = xzalloc(sizeof *node); >> + hmap_insert(queue_map, &node->node, hash_int(queue_id, 0)); >> + node->port_name = xstrdup(pb->logical_port); >> + } >> + node->queue_id = queue_id; >> node->min_rate = min_rate; >> node->max_rate = max_rate; >> node->burst = burst; >> - node->queue_id = queue_id; >> } >> >> static const struct ovsrec_qos * >> @@ -191,7 +217,7 @@ static bool >> set_noop_qos(struct ovsdb_idl_txn *ovs_idl_txn, >> const struct ovsrec_port_table *port_table, >> const struct ovsrec_qos_table *qos_table, >> - struct sset *egress_ifaces) >> + struct shash *egress_ifaces) >> { >> if (!ovs_idl_txn) { >> return false; >> @@ -206,11 +232,11 @@ set_noop_qos(struct ovsdb_idl_txn *ovs_idl_txn, >> size_t count = 0; >> >> OVSREC_PORT_TABLE_FOR_EACH (port, port_table) { >> - if (sset_contains(egress_ifaces, port->name)) { >> + if (shash_find(egress_ifaces, port->name)) { >> ovsrec_port_set_qos(port, noop_qos); >> count++; >> } >> - if (sset_count(egress_ifaces) == count) { >> + if (shash_count(egress_ifaces) == count) { >> break; >> } >> } >> @@ -236,7 +262,8 @@ set_qos_type(struct netdev *netdev, const char *type) >> } >> >> static void >> -setup_qos(const char *egress_iface, struct hmap *queue_map) >> +setup_qos(const char *egress_iface, const char *logical_port, >> + struct hmap *queue_map) >> { >> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); >> struct netdev *netdev_phy; >> @@ -281,7 +308,7 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) >> * a configuration setting. >> * >> * - Otherwise leave the qdisc alone. */ >> - if (hmap_is_empty(queue_map)) { >> + if (!get_qos_map_entry(queue_map, logical_port)) { >> if (!strcmp(qdisc_type, OVN_QOS_TYPE)) { >> set_qos_type(netdev_phy, ""); >> } >> @@ -338,6 +365,10 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) >> continue; >> } >> >> + if (strcmp(sb_info->port_name, logical_port)) { >> + continue; >> + } >> + >> smap_clear(&queue_details); >> smap_add_format(&queue_details, "min-rate", "%d", sb_info->min_rate); >> smap_add_format(&queue_details, "max-rate", "%d", sb_info->max_rate); >> @@ -354,11 +385,12 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) >> netdev_close(netdev_phy); >> } >> >> -static void >> +void >> destroy_qos_map(struct hmap *qos_map) >> { >> struct qos_queue *qos_queue; >> HMAP_FOR_EACH_POP (qos_queue, node, qos_map) { >> + free(qos_queue->port_name); >> free(qos_queue); >> } >> >> @@ -404,7 +436,7 @@ sbrec_get_port_encap(const struct sbrec_chassis *chassis_rec, >> static void >> add_localnet_egress_interface_mappings( >> const struct sbrec_port_binding *port_binding, >> - struct shash *bridge_mappings, struct sset *egress_ifaces) >> + struct shash *bridge_mappings, struct shash *egress_ifaces) >> { >> const char *network = smap_get(&port_binding->options, "network_name"); >> if (!network) { >> @@ -429,7 +461,8 @@ add_localnet_egress_interface_mappings( >> if (!is_egress_iface) { >> continue; >> } >> - sset_add(egress_ifaces, iface_rec->name); >> + shash_add(egress_ifaces, iface_rec->name, >> + port_binding->logical_port); >> } >> } >> } >> @@ -474,7 +507,7 @@ update_ld_multichassis_ports(const struct sbrec_port_binding *binding_rec, >> static void >> update_ld_localnet_port(const struct sbrec_port_binding *binding_rec, >> struct shash *bridge_mappings, >> - struct sset *egress_ifaces, >> + struct shash *egress_ifaces, >> struct hmap *local_datapaths) >> { >> /* Ignore localnet ports for unplugged networks. */ >> @@ -1456,7 +1489,7 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, >> b_ctx_out->tracked_dp_bindings); >> } >> if (b_lport->lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) { >> - get_qos_params(pb, qos_map); >> + update_qos_params(pb, qos_map); >> } >> } else { >> /* We could, but can't claim the lport. */ >> @@ -1519,6 +1552,16 @@ consider_vif_lport(const struct sbrec_port_binding *pb, >> b_lport = local_binding_add_lport(binding_lports, lbinding, pb, >> LP_VIF); >> } >> + >> + if (lbinding->iface && >> + smap_get(&lbinding->iface->external_ids, "ovn-egress-iface")) { >> + const char *iface_id = smap_get(&lbinding->iface->external_ids, >> + "iface-id"); >> + if (iface_id) { >> + shash_add(b_ctx_out->egress_ifaces, lbinding->iface->name, >> + iface_id); > > When a qos is configured on a logical port, this patch is adding 2 > entries in the 'egress_ifaces' shash for the same logical port. > I think it's because of the above 'shash_add'. I think you should use > shash_replace instead. > > When a full recompute happens, the function build_local_bindings() > also adds the qos configured logical ports to the 'egress_ifaces' > and later if there are any updates to the logical port, this function > - consider_vif_lport() also adds it to the shash. > > IMO the qos support in OVN needs a different approach. Instead of > configuring the qos using netdev() perhaps we should rely on the OVS > meters. > Maybe this was brought up earlier too. > > I'm of the opinion that instead of supporting Qos for logical ports > using netdev, we should use OVS meters (not just for logical ports, > even for localnet ports). Thoughts ? > > @Dumitru Ceara @Han Zhou @Mark Michelson @Ilya Maximets Do you have > any comments or suggestions here ? I think, meters and what we call QoS in OVS/OVN world are a bit different things. Meters only provide policing, while QoS provides egress traffic shaping ('egress' from the point of view of OVS the switch). One can not be a direct replacemnt of the other, but it depends on a use-case of course. OVS also allows to configure ingress policing separately from meters or egress shaping (QoS). What I definitely agree with is that OVN should not call netdev_* API. Instead, it should configure QoS table in the local OVS database and let ovs-vswitchd to create and control qdiscs on actual interfaces. Besides being a more robust solution, that will also allow using OVN QoS with OVS userspace datapath. Best regards, Ilya Maximets.
> On Sun, Dec 4, 2022 at 5:06 PM Lorenzo Bianconi > <lorenzo.bianconi@redhat.com> wrote: > > > > Introduce the capability to apply QoS rules for logical switch ports > > claimed by ovn-controller. Rely on shash instead of sset for > > egress_ifaces. > > > > Acked-by: Mark Michelson <mmichels@redhat.com> > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129742 > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > Hi Lorenzo, Hi Numan, thx for the review :) > > Thanks for addressing the comments. > > I tested this version and compared with the present main. > > I see a few differences. > > With the main version, when I configure qos params to a localnet > logical port, ovn-controller creates a qdisc on the tunnel interface > which I guess is expected. > But I'm still not sure why we need to configure qdiscs on the tunnel > interface. But that's a story. Perhaps I need to see the original > QoS commits and understand why it was added. > > With your patch, I don't see that happening. Perhaps there is a bug > in setup_qos() function now that 'shash' is used to store the egress > ifaces with the logical port and > there is no logical port for the tunnel interfaces. uhm, I can look into this. Do you think we should keep this behaviour? > > Regarding the option - "ovn-egress-iface". I suppose the expectation > from the CMS is to set the qos parameters in the logical port and also > set this option in the > ovs interface right ? I don't really see a need for this. I think if > CMS configures the qos parameters in the logical port options, > ovn-controller should configure the qos. > I think otherwise this would complicate the CMS implementation. Thoughts ? ovn-egress-iface is mandatory since it is used in setup_qos() to look for the proper netdevice pointer to configure ovs qdisc (please note it is used to identify ovs interfaces and not ovn ones). I think it can be doable (not sure about the complexity) to avoid it for logical switch ports (since we have iface-id in them external_ids column of ovs interface table) but I do not think we can avoid to set "ovn-egress-iface" for localnet port since afaik there is no direct link between ovn localnet port and ovs interface used to connect the underlay network, right? If so I guess it is better to keep it for both localnet and lsp ports. Do you agree? > > > Please see a few comments below > > > --- > > Changes since v3: > > - fix typo in new system-ovn test > > Changes since v2: > > - fix qos configuration restarting ovn-controller > > Changes since v1: > > - improve ovs interface lookup > > - improve system-tests > > --- > > controller/binding.c | 155 ++++++++++++++++++++++-------------- > > controller/binding.h | 5 +- > > controller/ovn-controller.c | 15 ++-- > > tests/system-ovn.at | 48 +++++++++++ > > 4 files changed, 156 insertions(+), 67 deletions(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index 5df62baef..53520263c 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -115,6 +115,7 @@ struct qos_queue { > > uint32_t min_rate; > > uint32_t max_rate; > > uint32_t burst; > > + char *port_name; > > }; > > > > void > > @@ -147,25 +148,50 @@ static void update_lport_tracking(const struct sbrec_port_binding *pb, > > struct hmap *tracked_dp_bindings, > > bool claimed); > > > > +static bool is_lport_vif(const struct sbrec_port_binding *pb); > > + > > +static struct qos_queue * > > +get_qos_map_entry(struct hmap *queue_map, const char *name) > > +{ > > + struct qos_queue *qos_node; > > + HMAP_FOR_EACH (qos_node, node, queue_map) { > > + if (!strcmp(qos_node->port_name, name)) { > > + return qos_node; > > + } > > + } > > + > > + return NULL; > > +} > > + > > static void > > -get_qos_params(const struct sbrec_port_binding *pb, struct hmap *queue_map) > > +update_qos_params(const struct sbrec_port_binding *pb, struct hmap *queue_map) > > { > > uint32_t min_rate = smap_get_int(&pb->options, "qos_min_rate", 0); > > uint32_t max_rate = smap_get_int(&pb->options, "qos_max_rate", 0); > > uint32_t burst = smap_get_int(&pb->options, "qos_burst", 0); > > uint32_t queue_id = smap_get_int(&pb->options, "qdisc_queue_id", 0); > > > > + struct qos_queue *node = get_qos_map_entry(queue_map, pb->logical_port); > > + > > if ((!min_rate && !max_rate && !burst) || !queue_id) { > > /* Qos is not configured for this port. */ > > + if (node) { > > + hmap_remove(queue_map, &node->node); > > + free(node->port_name); > > + free(node); > > + } > > return; > > } > > > > - struct qos_queue *node = xzalloc(sizeof *node); > > - hmap_insert(queue_map, &node->node, hash_int(queue_id, 0)); > > + if (!node) { > > + node = xzalloc(sizeof *node); > > + hmap_insert(queue_map, &node->node, hash_int(queue_id, 0)); > > + node->port_name = xstrdup(pb->logical_port); > > + } > > + node->queue_id = queue_id; > > node->min_rate = min_rate; > > node->max_rate = max_rate; > > node->burst = burst; > > - node->queue_id = queue_id; > > } > > > > static const struct ovsrec_qos * > > @@ -191,7 +217,7 @@ static bool > > set_noop_qos(struct ovsdb_idl_txn *ovs_idl_txn, > > const struct ovsrec_port_table *port_table, > > const struct ovsrec_qos_table *qos_table, > > - struct sset *egress_ifaces) > > + struct shash *egress_ifaces) > > { > > if (!ovs_idl_txn) { > > return false; > > @@ -206,11 +232,11 @@ set_noop_qos(struct ovsdb_idl_txn *ovs_idl_txn, > > size_t count = 0; > > > > OVSREC_PORT_TABLE_FOR_EACH (port, port_table) { > > - if (sset_contains(egress_ifaces, port->name)) { > > + if (shash_find(egress_ifaces, port->name)) { > > ovsrec_port_set_qos(port, noop_qos); > > count++; > > } > > - if (sset_count(egress_ifaces) == count) { > > + if (shash_count(egress_ifaces) == count) { > > break; > > } > > } > > @@ -236,7 +262,8 @@ set_qos_type(struct netdev *netdev, const char *type) > > } > > > > static void > > -setup_qos(const char *egress_iface, struct hmap *queue_map) > > +setup_qos(const char *egress_iface, const char *logical_port, > > + struct hmap *queue_map) > > { > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); > > struct netdev *netdev_phy; > > @@ -281,7 +308,7 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) > > * a configuration setting. > > * > > * - Otherwise leave the qdisc alone. */ > > - if (hmap_is_empty(queue_map)) { > > + if (!get_qos_map_entry(queue_map, logical_port)) { > > if (!strcmp(qdisc_type, OVN_QOS_TYPE)) { > > set_qos_type(netdev_phy, ""); > > } > > @@ -338,6 +365,10 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) > > continue; > > } > > > > + if (strcmp(sb_info->port_name, logical_port)) { > > + continue; > > + } > > + > > smap_clear(&queue_details); > > smap_add_format(&queue_details, "min-rate", "%d", sb_info->min_rate); > > smap_add_format(&queue_details, "max-rate", "%d", sb_info->max_rate); > > @@ -354,11 +385,12 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) > > netdev_close(netdev_phy); > > } > > > > -static void > > +void > > destroy_qos_map(struct hmap *qos_map) > > { > > struct qos_queue *qos_queue; > > HMAP_FOR_EACH_POP (qos_queue, node, qos_map) { > > + free(qos_queue->port_name); > > free(qos_queue); > > } > > > > @@ -404,7 +436,7 @@ sbrec_get_port_encap(const struct sbrec_chassis *chassis_rec, > > static void > > add_localnet_egress_interface_mappings( > > const struct sbrec_port_binding *port_binding, > > - struct shash *bridge_mappings, struct sset *egress_ifaces) > > + struct shash *bridge_mappings, struct shash *egress_ifaces) > > { > > const char *network = smap_get(&port_binding->options, "network_name"); > > if (!network) { > > @@ -429,7 +461,8 @@ add_localnet_egress_interface_mappings( > > if (!is_egress_iface) { > > continue; > > } > > - sset_add(egress_ifaces, iface_rec->name); > > + shash_add(egress_ifaces, iface_rec->name, > > + port_binding->logical_port); > > } > > } > > } > > @@ -474,7 +507,7 @@ update_ld_multichassis_ports(const struct sbrec_port_binding *binding_rec, > > static void > > update_ld_localnet_port(const struct sbrec_port_binding *binding_rec, > > struct shash *bridge_mappings, > > - struct sset *egress_ifaces, > > + struct shash *egress_ifaces, > > struct hmap *local_datapaths) > > { > > /* Ignore localnet ports for unplugged networks. */ > > @@ -1456,7 +1489,7 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, > > b_ctx_out->tracked_dp_bindings); > > } > > if (b_lport->lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) { > > - get_qos_params(pb, qos_map); > > + update_qos_params(pb, qos_map); > > } > > } else { > > /* We could, but can't claim the lport. */ > > @@ -1519,6 +1552,16 @@ consider_vif_lport(const struct sbrec_port_binding *pb, > > b_lport = local_binding_add_lport(binding_lports, lbinding, pb, > > LP_VIF); > > } > > + > > + if (lbinding->iface && > > + smap_get(&lbinding->iface->external_ids, "ovn-egress-iface")) { > > + const char *iface_id = smap_get(&lbinding->iface->external_ids, > > + "iface-id"); > > + if (iface_id) { > > + shash_add(b_ctx_out->egress_ifaces, lbinding->iface->name, > > + iface_id); > > When a qos is configured on a logical port, this patch is adding 2 > entries in the 'egress_ifaces' shash for the same logical port. > I think it's because of the above 'shash_add'. I think you should use > shash_replace instead. ack, I will fix it. > > When a full recompute happens, the function build_local_bindings() > also adds the qos configured logical ports to the 'egress_ifaces' > and later if there are any updates to the logical port, this function > - consider_vif_lport() also adds it to the shash. > > IMO the qos support in OVN needs a different approach. Instead of > configuring the qos using netdev() perhaps we should rely on the OVS > meters. > Maybe this was brought up earlier too. > > I'm of the opinion that instead of supporting Qos for logical ports > using netdev, we should use OVS meters (not just for logical ports, > even for localnet ports). Thoughts ? ovn qos is used for egress traffic (shaping) while ovn meters are used for incoming one (policing). What we can do is to avoid configuring directly netdev qdisc and rely on ovs to perform this configuration but this would be quite a massive rework. Do you think we should do it now before adding this feature or it is ok to do it after we added this feature? I do not have a strong opinion about it. Regards, Lorenzo > > @Dumitru Ceara @Han Zhou @Mark Michelson @Ilya Maximets Do you have > any comments or suggestions here ? > > Thanks > Numan > > > > > + } > > + } > > } > > > > return consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out, > > @@ -1785,7 +1828,7 @@ consider_localnet_lport(const struct sbrec_port_binding *pb, > > update_local_lports(pb->logical_port, b_ctx_out); > > > > if (qos_map && b_ctx_in->ovs_idl_txn) { > > - get_qos_params(pb, qos_map); > > + update_qos_params(pb, qos_map); > > } > > > > update_related_lport(pb, b_ctx_out); > > @@ -1861,14 +1904,14 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in, > > &b_ctx_out->lbinding_data->bindings; > > for (j = 0; j < port_rec->n_interfaces; j++) { > > const struct ovsrec_interface *iface_rec; > > + struct local_binding *lbinding = NULL; > > > > iface_rec = port_rec->interfaces[j]; > > iface_id = smap_get(&iface_rec->external_ids, "iface-id"); > > int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; > > > > if (iface_id && ofport > 0) { > > - struct local_binding *lbinding = > > - local_binding_find(local_bindings, iface_id); > > + lbinding = local_binding_find(local_bindings, iface_id); > > if (!lbinding) { > > lbinding = local_binding_create(iface_id, iface_rec); > > local_binding_add(local_bindings, lbinding); > > @@ -1895,8 +1938,11 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in, > > const char *tunnel_iface > > = smap_get(&iface_rec->status, "tunnel_egress_iface"); > > if (tunnel_iface) { > > - sset_add(b_ctx_out->egress_ifaces, tunnel_iface); > > + shash_add(b_ctx_out->egress_ifaces, tunnel_iface, ""); > > } > > + } else if (lbinding && smap_get(&iface_rec->external_ids, > > + "ovn-egress-iface")) { > > + shash_add(b_ctx_out->egress_ifaces, iface_rec->name, iface_id); > > } > > } > > } > > @@ -1910,16 +1956,11 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > > } > > > > struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings); > > - struct hmap qos_map; > > > > - hmap_init(&qos_map); > > if (b_ctx_in->br_int) { > > build_local_bindings(b_ctx_in, b_ctx_out); > > } > > > > - struct hmap *qos_map_ptr = > > - !sset_is_empty(b_ctx_out->egress_ifaces) ? &qos_map : NULL; > > - > > struct ovs_list localnet_lports = OVS_LIST_INITIALIZER(&localnet_lports); > > struct ovs_list external_lports = OVS_LIST_INITIALIZER(&external_lports); > > struct ovs_list multichassis_ports = OVS_LIST_INITIALIZER( > > @@ -1956,7 +1997,8 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > > break; > > > > case LP_VIF: > > - consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL, qos_map_ptr); > > + consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL, > > + b_ctx_out->qos_map); > > if (pb->additional_chassis) { > > struct lport *multichassis_lport = xmalloc( > > sizeof *multichassis_lport); > > @@ -1967,11 +2009,13 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > > break; > > > > case LP_CONTAINER: > > - consider_container_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); > > + consider_container_lport(pb, b_ctx_in, b_ctx_out, > > + b_ctx_out->qos_map); > > break; > > > > case LP_VIRTUAL: > > - consider_virtual_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); > > + consider_virtual_lport(pb, b_ctx_in, b_ctx_out, > > + b_ctx_out->qos_map); > > break; > > > > case LP_L2GATEWAY: > > @@ -1994,7 +2038,8 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > > break; > > > > case LP_LOCALNET: { > > - consider_localnet_lport(pb, b_ctx_in, b_ctx_out, &qos_map); > > + consider_localnet_lport(pb, b_ctx_in, b_ctx_out, > > + b_ctx_out->qos_map); > > struct lport *lnet_lport = xmalloc(sizeof *lnet_lport); > > lnet_lport->pb = pb; > > ovs_list_push_back(&localnet_lports, &lnet_lport->list_node); > > @@ -2051,17 +2096,15 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > > > > shash_destroy(&bridge_mappings); > > > > - if (!sset_is_empty(b_ctx_out->egress_ifaces) > > + if (!shash_is_empty(b_ctx_out->egress_ifaces) > > && set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, > > b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) { > > - const char *entry; > > - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > - setup_qos(entry, &qos_map); > > + struct shash_node *entry; > > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > + setup_qos(entry->name, entry->data, b_ctx_out->qos_map); > > } > > } > > > > - destroy_qos_map(&qos_map); > > - > > cleanup_claimed_port_timestamps(); > > } > > > > @@ -2447,7 +2490,7 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, > > } > > > > if (smap_get(&iface_rec->external_ids, "ovn-egress-iface") || > > - sset_contains(b_ctx_out->egress_ifaces, iface_rec->name)) { > > + shash_find(b_ctx_out->egress_ifaces, iface_rec->name)) { > > handled = false; > > break; > > } > > @@ -2493,10 +2536,6 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, > > return false; > > } > > > > - struct hmap qos_map = HMAP_INITIALIZER(&qos_map); > > - struct hmap *qos_map_ptr = > > - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; > > - > > /* > > * We consider an OVS interface for claiming if the following > > * 2 conditions are met: > > @@ -2525,24 +2564,22 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, > > if (iface_id && ofport > 0 && > > is_iface_in_int_bridge(iface_rec, b_ctx_in->br_int)) { > > handled = consider_iface_claim(iface_rec, iface_id, b_ctx_in, > > - b_ctx_out, qos_map_ptr); > > + b_ctx_out, b_ctx_out->qos_map); > > if (!handled) { > > break; > > } > > } > > } > > > > - if (handled && qos_map_ptr && set_noop_qos(b_ctx_in->ovs_idl_txn, > > - b_ctx_in->port_table, > > - b_ctx_in->qos_table, > > - b_ctx_out->egress_ifaces)) { > > - const char *entry; > > - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > - setup_qos(entry, &qos_map); > > + if (handled && > > + set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, > > + b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) { > > + struct shash_node *entry; > > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > + setup_qos(entry->name, entry->data, b_ctx_out->qos_map); > > } > > } > > > > - destroy_qos_map(&qos_map); > > return handled; > > } > > > > @@ -2977,10 +3014,6 @@ delete_done: > > return false; > > } > > > > - struct hmap qos_map = HMAP_INITIALIZER(&qos_map); > > - struct hmap *qos_map_ptr = > > - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; > > - > > SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, > > b_ctx_in->port_binding_table) { > > /* Loop to handle create and update changes only. */ > > @@ -2992,7 +3025,8 @@ delete_done: > > update_ld_peers(pb, b_ctx_out->local_datapaths); > > } > > > > - handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, qos_map_ptr); > > + handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, > > + b_ctx_out->qos_map); > > if (!handled) { > > break; > > } > > @@ -3009,7 +3043,8 @@ delete_done: > > sset_find_and_delete(b_ctx_out->postponed_ports, port_name); > > continue; > > } > > - handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, qos_map_ptr); > > + handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, > > + b_ctx_out->qos_map); > > if (!handled) { > > break; > > } > > @@ -3055,17 +3090,15 @@ delete_done: > > shash_destroy(&bridge_mappings); > > } > > > > - if (handled && qos_map_ptr && set_noop_qos(b_ctx_in->ovs_idl_txn, > > - b_ctx_in->port_table, > > - b_ctx_in->qos_table, > > - b_ctx_out->egress_ifaces)) { > > - const char *entry; > > - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > - setup_qos(entry, &qos_map); > > + if (handled && > > + set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, > > + b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) { > > + struct shash_node *entry; > > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > + setup_qos(entry->name, entry->data, b_ctx_out->qos_map); > > } > > } > > > > - destroy_qos_map(&qos_map); > > return handled; > > } > > > > diff --git a/controller/binding.h b/controller/binding.h > > index 6c3a98b02..8be33eddb 100644 > > --- a/controller/binding.h > > +++ b/controller/binding.h > > @@ -91,7 +91,8 @@ struct binding_ctx_out { > > */ > > bool non_vif_ports_changed; > > > > - struct sset *egress_ifaces; > > + struct shash *egress_ifaces; > > + struct hmap *qos_map; > > /* smap of OVS interface name as key and > > * OVS interface external_ids:iface-id as value. */ > > struct smap *local_iface_ids; > > @@ -195,6 +196,8 @@ void set_pb_chassis_in_sbrec(const struct sbrec_port_binding *pb, > > const struct sbrec_chassis *chassis_rec, > > bool is_set); > > > > +void destroy_qos_map(struct hmap *qos_map); > > + > > /* Corresponds to each Port_Binding.type. */ > > enum en_lport_type { > > LP_UNKNOWN, > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index d6251afb8..066d76869 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -1276,7 +1276,8 @@ struct ed_type_runtime_data { > > struct sset active_tunnels; > > > > /* runtime data engine private data. */ > > - struct sset egress_ifaces; > > + struct shash egress_ifaces; > > + struct hmap qos_map; > > struct smap local_iface_ids; > > > > /* Tracked data. See below for more details and comments. */ > > @@ -1372,7 +1373,8 @@ en_runtime_data_init(struct engine_node *node OVS_UNUSED, > > sset_init(&data->local_lports); > > related_lports_init(&data->related_lports); > > sset_init(&data->active_tunnels); > > - sset_init(&data->egress_ifaces); > > + shash_init(&data->egress_ifaces); > > + hmap_init(&data->qos_map); > > smap_init(&data->local_iface_ids); > > local_binding_data_init(&data->lbinding_data); > > shash_init(&data->local_active_ports_ipv6_pd); > > @@ -1392,7 +1394,8 @@ en_runtime_data_cleanup(void *data) > > sset_destroy(&rt_data->local_lports); > > related_lports_destroy(&rt_data->related_lports); > > sset_destroy(&rt_data->active_tunnels); > > - sset_destroy(&rt_data->egress_ifaces); > > + shash_destroy(&rt_data->egress_ifaces); > > + destroy_qos_map(&rt_data->qos_map); > > smap_destroy(&rt_data->local_iface_ids); > > local_datapaths_destroy(&rt_data->local_datapaths); > > shash_destroy(&rt_data->local_active_ports_ipv6_pd); > > @@ -1481,6 +1484,7 @@ init_binding_ctx(struct engine_node *node, > > b_ctx_out->related_lports_changed = false; > > b_ctx_out->non_vif_ports_changed = false; > > b_ctx_out->egress_ifaces = &rt_data->egress_ifaces; > > + b_ctx_out->qos_map = &rt_data->qos_map; > > b_ctx_out->lbinding_data = &rt_data->lbinding_data; > > b_ctx_out->local_iface_ids = &rt_data->local_iface_ids; > > b_ctx_out->postponed_ports = rt_data->postponed_ports; > > @@ -1510,13 +1514,14 @@ en_runtime_data_run(struct engine_node *node, void *data) > > sset_destroy(local_lports); > > related_lports_destroy(&rt_data->related_lports); > > sset_destroy(active_tunnels); > > - sset_destroy(&rt_data->egress_ifaces); > > + shash_clear(&rt_data->egress_ifaces); > > + destroy_qos_map(&rt_data->qos_map); > > + hmap_init(&rt_data->qos_map); > > smap_destroy(&rt_data->local_iface_ids); > > hmap_init(local_datapaths); > > sset_init(local_lports); > > related_lports_init(&rt_data->related_lports); > > sset_init(active_tunnels); > > - sset_init(&rt_data->egress_ifaces); > > smap_init(&rt_data->local_iface_ids); > > local_binding_data_init(&rt_data->lbinding_data); > > } > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > index 3e904c9dc..c986198a2 100644 > > --- a/tests/system-ovn.at > > +++ b/tests/system-ovn.at > > @@ -6335,6 +6335,10 @@ ADD_NAMESPACES(sw01) > > ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03") > > ovn-nbctl lsp-add sw0 sw01 \ > > -- lsp-set-addresses sw01 "f0:00:00:01:02:03 192.168.1.2" > > +ADD_NAMESPACES(sw02) > > +ADD_VETH(sw02, sw02, br-int, "192.168.1.3/24", "f2:00:00:01:02:03") > > +ovn-nbctl lsp-add sw0 sw02 \ > > + -- lsp-set-addresses sw02 "f2:00:00:01:02:03 192.168.1.3" > > > > ADD_NAMESPACES(public) > > ADD_VETH(public, public, br-ext, "192.168.2.2/24", "f0:00:00:01:02:05") > > @@ -6345,6 +6349,7 @@ ovn-nbctl lsp-add sw0 public \ > > -- lsp-set-type public localnet \ > > -- lsp-set-options public network_name=phynet > > > > +# Setup QoS on a localnet port > > AT_CHECK([ovn-nbctl set Logical_Switch_Port public options:qos_min_rate=200000]) > > AT_CHECK([ovn-nbctl set Logical_Switch_Port public options:qos_max_rate=300000]) > > AT_CHECK([ovn-nbctl set Logical_Switch_Port public options:qos_burst=3000000]) > > @@ -6353,15 +6358,58 @@ 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']) > > > > +# Setup QoS on a logical switch ports > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_min_rate=400000]) > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_max_rate=800000]) > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_burst=5000000]) > > +AT_CHECK([ovs-vsctl set interface ovs-sw01 external-ids:ovn-egress-iface=true]) > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01']) > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \ > > + grep -q 'class htb .* rate 400Kbit ceil 800Kbit burst 625000b cburst 625000b']) > > + > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qos_min_rate=600000]) > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qos_max_rate=6000000]) > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qos_burst=6000000]) > > +AT_CHECK([ovs-vsctl set interface ovs-sw02 external-ids:ovn-egress-iface=true]) > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw02']) > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw02 | \ > > + grep -q 'class htb .* rate 600Kbit ceil 6Mbit burst 750000b cburst 750000b']) > > + > > +AT_CHECK([ovn-appctl -t ovn-controller exit --restart]) > > +OVS_WAIT_UNTIL([test "$(pidof ovn-controller)" = ""]) > > +start_daemon ovn-controller > > +OVS_WAIT_UNTIL([test "$(pidof ovn-controller)" != ""]) > > + > > +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']) > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01']) > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \ > > + grep -q 'class htb .* rate 400Kbit ceil 800Kbit burst 625000b cburst 625000b']) > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw02']) > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw02 | \ > > + grep -q 'class htb .* rate 600Kbit ceil 6Mbit burst 750000b cburst 750000b']) > > > > 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_max_rate=300000]) > > 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')" = ""]) > > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_min_rate=200000]) > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01']) > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \ > > + grep -q 'class htb .* rate 200Kbit ceil 800Kbit burst 625000b cburst 625000b']) > > + > > +# Disable QoS rules from logical switch ports > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qdisc_queue_id=0]) > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qdisc_queue_id=0]) > > +OVS_WAIT_UNTIL([test "$(tc qdisc show | grep 'htb 1: dev ovs-sw01')" = ""]) > > +OVS_WAIT_UNTIL([test "$(tc qdisc show | grep 'htb 1: dev ovs-sw02')" = ""]) > > + > > kill $(pidof ovn-controller) > > > > as ovn-sb > > -- > > 2.38.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
On Fri, Dec 9, 2022 at 9:43 AM Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote: > > > On Sun, Dec 4, 2022 at 5:06 PM Lorenzo Bianconi > > <lorenzo.bianconi@redhat.com> wrote: > > > > > > Introduce the capability to apply QoS rules for logical switch ports > > > claimed by ovn-controller. Rely on shash instead of sset for > > > egress_ifaces. > > > > > > Acked-by: Mark Michelson <mmichels@redhat.com> > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129742 > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > > > Hi Lorenzo, > > Hi Numan, > > thx for the review :) > > > > > Thanks for addressing the comments. > > > > I tested this version and compared with the present main. > > > > I see a few differences. > > > > With the main version, when I configure qos params to a localnet > > logical port, ovn-controller creates a qdisc on the tunnel interface > > which I guess is expected. > > But I'm still not sure why we need to configure qdiscs on the tunnel > > interface. But that's a story. Perhaps I need to see the original > > QoS commits and understand why it was added. > > > > With your patch, I don't see that happening. Perhaps there is a bug > > in setup_qos() function now that 'shash' is used to store the egress > > ifaces with the logical port and > > there is no logical port for the tunnel interfaces. > > uhm, I can look into this. Do you think we should keep this behaviour? Frankly I don't know. We need to find out why qos was configured in the first place. I do agree with Ilya's comment that it is odd to configure qos on the tunnel interface and that could impact the other traffic. > > > > > Regarding the option - "ovn-egress-iface". I suppose the expectation > > from the CMS is to set the qos parameters in the logical port and also > > set this option in the > > ovs interface right ? I don't really see a need for this. I think if > > CMS configures the qos parameters in the logical port options, > > ovn-controller should configure the qos. > > I think otherwise this would complicate the CMS implementation. Thoughts ? > > ovn-egress-iface is mandatory since it is used in setup_qos() to look for the > proper netdevice pointer to configure ovs qdisc (please note it is used to > identify ovs interfaces and not ovn ones). > I think it can be doable (not sure about the complexity) to avoid it for > logical switch ports (since we have iface-id in them external_ids column of > ovs interface table) but I do not think we can avoid to set "ovn-egress-iface" > for localnet port since afaik there is no direct link between ovn localnet port > and ovs interface used to connect the underlay network, right? If so I guess > it is better to keep it for both localnet and lsp ports. Do you agree? I think it's ok to expect CMS to set "ovn-egress-iface" for the physical nic connected to the provider bridge. But I don't think we should do the same for the normal logical port VIFs. > > > > > > > Please see a few comments below > > > > > --- > > > Changes since v3: > > > - fix typo in new system-ovn test > > > Changes since v2: > > > - fix qos configuration restarting ovn-controller > > > Changes since v1: > > > - improve ovs interface lookup > > > - improve system-tests > > > --- > > > controller/binding.c | 155 ++++++++++++++++++++++-------------- > > > controller/binding.h | 5 +- > > > controller/ovn-controller.c | 15 ++-- > > > tests/system-ovn.at | 48 +++++++++++ > > > 4 files changed, 156 insertions(+), 67 deletions(-) > > > > > > diff --git a/controller/binding.c b/controller/binding.c > > > index 5df62baef..53520263c 100644 > > > --- a/controller/binding.c > > > +++ b/controller/binding.c > > > @@ -115,6 +115,7 @@ struct qos_queue { > > > uint32_t min_rate; > > > uint32_t max_rate; > > > uint32_t burst; > > > + char *port_name; > > > }; > > > > > > void > > > @@ -147,25 +148,50 @@ static void update_lport_tracking(const struct sbrec_port_binding *pb, > > > struct hmap *tracked_dp_bindings, > > > bool claimed); > > > > > > +static bool is_lport_vif(const struct sbrec_port_binding *pb); > > > + > > > +static struct qos_queue * > > > +get_qos_map_entry(struct hmap *queue_map, const char *name) > > > +{ > > > + struct qos_queue *qos_node; > > > + HMAP_FOR_EACH (qos_node, node, queue_map) { > > > + if (!strcmp(qos_node->port_name, name)) { > > > + return qos_node; > > > + } > > > + } > > > + > > > + return NULL; > > > +} > > > + > > > static void > > > -get_qos_params(const struct sbrec_port_binding *pb, struct hmap *queue_map) > > > +update_qos_params(const struct sbrec_port_binding *pb, struct hmap *queue_map) > > > { > > > uint32_t min_rate = smap_get_int(&pb->options, "qos_min_rate", 0); > > > uint32_t max_rate = smap_get_int(&pb->options, "qos_max_rate", 0); > > > uint32_t burst = smap_get_int(&pb->options, "qos_burst", 0); > > > uint32_t queue_id = smap_get_int(&pb->options, "qdisc_queue_id", 0); > > > > > > + struct qos_queue *node = get_qos_map_entry(queue_map, pb->logical_port); > > > + > > > if ((!min_rate && !max_rate && !burst) || !queue_id) { > > > /* Qos is not configured for this port. */ > > > + if (node) { > > > + hmap_remove(queue_map, &node->node); > > > + free(node->port_name); > > > + free(node); > > > + } > > > return; > > > } > > > > > > - struct qos_queue *node = xzalloc(sizeof *node); > > > - hmap_insert(queue_map, &node->node, hash_int(queue_id, 0)); > > > + if (!node) { > > > + node = xzalloc(sizeof *node); > > > + hmap_insert(queue_map, &node->node, hash_int(queue_id, 0)); > > > + node->port_name = xstrdup(pb->logical_port); > > > + } > > > + node->queue_id = queue_id; > > > node->min_rate = min_rate; > > > node->max_rate = max_rate; > > > node->burst = burst; > > > - node->queue_id = queue_id; > > > } > > > > > > static const struct ovsrec_qos * > > > @@ -191,7 +217,7 @@ static bool > > > set_noop_qos(struct ovsdb_idl_txn *ovs_idl_txn, > > > const struct ovsrec_port_table *port_table, > > > const struct ovsrec_qos_table *qos_table, > > > - struct sset *egress_ifaces) > > > + struct shash *egress_ifaces) > > > { > > > if (!ovs_idl_txn) { > > > return false; > > > @@ -206,11 +232,11 @@ set_noop_qos(struct ovsdb_idl_txn *ovs_idl_txn, > > > size_t count = 0; > > > > > > OVSREC_PORT_TABLE_FOR_EACH (port, port_table) { > > > - if (sset_contains(egress_ifaces, port->name)) { > > > + if (shash_find(egress_ifaces, port->name)) { > > > ovsrec_port_set_qos(port, noop_qos); > > > count++; > > > } > > > - if (sset_count(egress_ifaces) == count) { > > > + if (shash_count(egress_ifaces) == count) { > > > break; > > > } > > > } > > > @@ -236,7 +262,8 @@ set_qos_type(struct netdev *netdev, const char *type) > > > } > > > > > > static void > > > -setup_qos(const char *egress_iface, struct hmap *queue_map) > > > +setup_qos(const char *egress_iface, const char *logical_port, > > > + struct hmap *queue_map) > > > { > > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); > > > struct netdev *netdev_phy; > > > @@ -281,7 +308,7 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) > > > * a configuration setting. > > > * > > > * - Otherwise leave the qdisc alone. */ > > > - if (hmap_is_empty(queue_map)) { > > > + if (!get_qos_map_entry(queue_map, logical_port)) { > > > if (!strcmp(qdisc_type, OVN_QOS_TYPE)) { > > > set_qos_type(netdev_phy, ""); > > > } > > > @@ -338,6 +365,10 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) > > > continue; > > > } > > > > > > + if (strcmp(sb_info->port_name, logical_port)) { > > > + continue; > > > + } > > > + > > > smap_clear(&queue_details); > > > smap_add_format(&queue_details, "min-rate", "%d", sb_info->min_rate); > > > smap_add_format(&queue_details, "max-rate", "%d", sb_info->max_rate); > > > @@ -354,11 +385,12 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) > > > netdev_close(netdev_phy); > > > } > > > > > > -static void > > > +void > > > destroy_qos_map(struct hmap *qos_map) > > > { > > > struct qos_queue *qos_queue; > > > HMAP_FOR_EACH_POP (qos_queue, node, qos_map) { > > > + free(qos_queue->port_name); > > > free(qos_queue); > > > } > > > > > > @@ -404,7 +436,7 @@ sbrec_get_port_encap(const struct sbrec_chassis *chassis_rec, > > > static void > > > add_localnet_egress_interface_mappings( > > > const struct sbrec_port_binding *port_binding, > > > - struct shash *bridge_mappings, struct sset *egress_ifaces) > > > + struct shash *bridge_mappings, struct shash *egress_ifaces) > > > { > > > const char *network = smap_get(&port_binding->options, "network_name"); > > > if (!network) { > > > @@ -429,7 +461,8 @@ add_localnet_egress_interface_mappings( > > > if (!is_egress_iface) { > > > continue; > > > } > > > - sset_add(egress_ifaces, iface_rec->name); > > > + shash_add(egress_ifaces, iface_rec->name, > > > + port_binding->logical_port); > > > } > > > } > > > } > > > @@ -474,7 +507,7 @@ update_ld_multichassis_ports(const struct sbrec_port_binding *binding_rec, > > > static void > > > update_ld_localnet_port(const struct sbrec_port_binding *binding_rec, > > > struct shash *bridge_mappings, > > > - struct sset *egress_ifaces, > > > + struct shash *egress_ifaces, > > > struct hmap *local_datapaths) > > > { > > > /* Ignore localnet ports for unplugged networks. */ > > > @@ -1456,7 +1489,7 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, > > > b_ctx_out->tracked_dp_bindings); > > > } > > > if (b_lport->lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) { > > > - get_qos_params(pb, qos_map); > > > + update_qos_params(pb, qos_map); > > > } > > > } else { > > > /* We could, but can't claim the lport. */ > > > @@ -1519,6 +1552,16 @@ consider_vif_lport(const struct sbrec_port_binding *pb, > > > b_lport = local_binding_add_lport(binding_lports, lbinding, pb, > > > LP_VIF); > > > } > > > + > > > + if (lbinding->iface && > > > + smap_get(&lbinding->iface->external_ids, "ovn-egress-iface")) { > > > + const char *iface_id = smap_get(&lbinding->iface->external_ids, > > > + "iface-id"); > > > + if (iface_id) { > > > + shash_add(b_ctx_out->egress_ifaces, lbinding->iface->name, > > > + iface_id); > > > > When a qos is configured on a logical port, this patch is adding 2 > > entries in the 'egress_ifaces' shash for the same logical port. > > I think it's because of the above 'shash_add'. I think you should use > > shash_replace instead. > > ack, I will fix it. > > > > > When a full recompute happens, the function build_local_bindings() > > also adds the qos configured logical ports to the 'egress_ifaces' > > and later if there are any updates to the logical port, this function > > - consider_vif_lport() also adds it to the shash. > > > > IMO the qos support in OVN needs a different approach. Instead of > > configuring the qos using netdev() perhaps we should rely on the OVS > > meters. > > Maybe this was brought up earlier too. > > > > > I'm of the opinion that instead of supporting Qos for logical ports > > using netdev, we should use OVS meters (not just for logical ports, > > even for localnet ports). Thoughts ? > > ovn qos is used for egress traffic (shaping) while ovn meters are used > for incoming one (policing). > What we can do is to avoid configuring directly netdev qdisc and rely on > ovs to perform this configuration but this would be quite a massive rework. > Do you think we should do it now before adding this feature or it is ok > to do it after we added this feature? I do not have a strong opinion about it. IMO its better to add this new feature using the native OVS QoS support. And along with that we can remove the netdev implementation from OVN. This is what I think. Anyway this feature is out of window for 22.12. We can perhaps target this feature with the new approach in 23.03. Although I will not object if other maintainers are fine getting this feature first and then reimplement using OVS native QoS support. Thanks Numan > > Regards, > Lorenzo > > > > > @Dumitru Ceara @Han Zhou @Mark Michelson @Ilya Maximets Do you have > > any comments or suggestions here ? > > > > Thanks > > Numan > > > > > > > > > + } > > > + } > > > } > > > > > > return consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out, > > > @@ -1785,7 +1828,7 @@ consider_localnet_lport(const struct sbrec_port_binding *pb, > > > update_local_lports(pb->logical_port, b_ctx_out); > > > > > > if (qos_map && b_ctx_in->ovs_idl_txn) { > > > - get_qos_params(pb, qos_map); > > > + update_qos_params(pb, qos_map); > > > } > > > > > > update_related_lport(pb, b_ctx_out); > > > @@ -1861,14 +1904,14 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in, > > > &b_ctx_out->lbinding_data->bindings; > > > for (j = 0; j < port_rec->n_interfaces; j++) { > > > const struct ovsrec_interface *iface_rec; > > > + struct local_binding *lbinding = NULL; > > > > > > iface_rec = port_rec->interfaces[j]; > > > iface_id = smap_get(&iface_rec->external_ids, "iface-id"); > > > int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; > > > > > > if (iface_id && ofport > 0) { > > > - struct local_binding *lbinding = > > > - local_binding_find(local_bindings, iface_id); > > > + lbinding = local_binding_find(local_bindings, iface_id); > > > if (!lbinding) { > > > lbinding = local_binding_create(iface_id, iface_rec); > > > local_binding_add(local_bindings, lbinding); > > > @@ -1895,8 +1938,11 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in, > > > const char *tunnel_iface > > > = smap_get(&iface_rec->status, "tunnel_egress_iface"); > > > if (tunnel_iface) { > > > - sset_add(b_ctx_out->egress_ifaces, tunnel_iface); > > > + shash_add(b_ctx_out->egress_ifaces, tunnel_iface, ""); > > > } > > > + } else if (lbinding && smap_get(&iface_rec->external_ids, > > > + "ovn-egress-iface")) { > > > + shash_add(b_ctx_out->egress_ifaces, iface_rec->name, iface_id); > > > } > > > } > > > } > > > @@ -1910,16 +1956,11 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > > > } > > > > > > struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings); > > > - struct hmap qos_map; > > > > > > - hmap_init(&qos_map); > > > if (b_ctx_in->br_int) { > > > build_local_bindings(b_ctx_in, b_ctx_out); > > > } > > > > > > - struct hmap *qos_map_ptr = > > > - !sset_is_empty(b_ctx_out->egress_ifaces) ? &qos_map : NULL; > > > - > > > struct ovs_list localnet_lports = OVS_LIST_INITIALIZER(&localnet_lports); > > > struct ovs_list external_lports = OVS_LIST_INITIALIZER(&external_lports); > > > struct ovs_list multichassis_ports = OVS_LIST_INITIALIZER( > > > @@ -1956,7 +1997,8 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > > > break; > > > > > > case LP_VIF: > > > - consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL, qos_map_ptr); > > > + consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL, > > > + b_ctx_out->qos_map); > > > if (pb->additional_chassis) { > > > struct lport *multichassis_lport = xmalloc( > > > sizeof *multichassis_lport); > > > @@ -1967,11 +2009,13 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > > > break; > > > > > > case LP_CONTAINER: > > > - consider_container_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); > > > + consider_container_lport(pb, b_ctx_in, b_ctx_out, > > > + b_ctx_out->qos_map); > > > break; > > > > > > case LP_VIRTUAL: > > > - consider_virtual_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); > > > + consider_virtual_lport(pb, b_ctx_in, b_ctx_out, > > > + b_ctx_out->qos_map); > > > break; > > > > > > case LP_L2GATEWAY: > > > @@ -1994,7 +2038,8 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > > > break; > > > > > > case LP_LOCALNET: { > > > - consider_localnet_lport(pb, b_ctx_in, b_ctx_out, &qos_map); > > > + consider_localnet_lport(pb, b_ctx_in, b_ctx_out, > > > + b_ctx_out->qos_map); > > > struct lport *lnet_lport = xmalloc(sizeof *lnet_lport); > > > lnet_lport->pb = pb; > > > ovs_list_push_back(&localnet_lports, &lnet_lport->list_node); > > > @@ -2051,17 +2096,15 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > > > > > > shash_destroy(&bridge_mappings); > > > > > > - if (!sset_is_empty(b_ctx_out->egress_ifaces) > > > + if (!shash_is_empty(b_ctx_out->egress_ifaces) > > > && set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, > > > b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) { > > > - const char *entry; > > > - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > > - setup_qos(entry, &qos_map); > > > + struct shash_node *entry; > > > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > > + setup_qos(entry->name, entry->data, b_ctx_out->qos_map); > > > } > > > } > > > > > > - destroy_qos_map(&qos_map); > > > - > > > cleanup_claimed_port_timestamps(); > > > } > > > > > > @@ -2447,7 +2490,7 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, > > > } > > > > > > if (smap_get(&iface_rec->external_ids, "ovn-egress-iface") || > > > - sset_contains(b_ctx_out->egress_ifaces, iface_rec->name)) { > > > + shash_find(b_ctx_out->egress_ifaces, iface_rec->name)) { > > > handled = false; > > > break; > > > } > > > @@ -2493,10 +2536,6 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, > > > return false; > > > } > > > > > > - struct hmap qos_map = HMAP_INITIALIZER(&qos_map); > > > - struct hmap *qos_map_ptr = > > > - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; > > > - > > > /* > > > * We consider an OVS interface for claiming if the following > > > * 2 conditions are met: > > > @@ -2525,24 +2564,22 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, > > > if (iface_id && ofport > 0 && > > > is_iface_in_int_bridge(iface_rec, b_ctx_in->br_int)) { > > > handled = consider_iface_claim(iface_rec, iface_id, b_ctx_in, > > > - b_ctx_out, qos_map_ptr); > > > + b_ctx_out, b_ctx_out->qos_map); > > > if (!handled) { > > > break; > > > } > > > } > > > } > > > > > > - if (handled && qos_map_ptr && set_noop_qos(b_ctx_in->ovs_idl_txn, > > > - b_ctx_in->port_table, > > > - b_ctx_in->qos_table, > > > - b_ctx_out->egress_ifaces)) { > > > - const char *entry; > > > - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > > - setup_qos(entry, &qos_map); > > > + if (handled && > > > + set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, > > > + b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) { > > > + struct shash_node *entry; > > > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > > + setup_qos(entry->name, entry->data, b_ctx_out->qos_map); > > > } > > > } > > > > > > - destroy_qos_map(&qos_map); > > > return handled; > > > } > > > > > > @@ -2977,10 +3014,6 @@ delete_done: > > > return false; > > > } > > > > > > - struct hmap qos_map = HMAP_INITIALIZER(&qos_map); > > > - struct hmap *qos_map_ptr = > > > - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; > > > - > > > SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, > > > b_ctx_in->port_binding_table) { > > > /* Loop to handle create and update changes only. */ > > > @@ -2992,7 +3025,8 @@ delete_done: > > > update_ld_peers(pb, b_ctx_out->local_datapaths); > > > } > > > > > > - handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, qos_map_ptr); > > > + handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, > > > + b_ctx_out->qos_map); > > > if (!handled) { > > > break; > > > } > > > @@ -3009,7 +3043,8 @@ delete_done: > > > sset_find_and_delete(b_ctx_out->postponed_ports, port_name); > > > continue; > > > } > > > - handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, qos_map_ptr); > > > + handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, > > > + b_ctx_out->qos_map); > > > if (!handled) { > > > break; > > > } > > > @@ -3055,17 +3090,15 @@ delete_done: > > > shash_destroy(&bridge_mappings); > > > } > > > > > > - if (handled && qos_map_ptr && set_noop_qos(b_ctx_in->ovs_idl_txn, > > > - b_ctx_in->port_table, > > > - b_ctx_in->qos_table, > > > - b_ctx_out->egress_ifaces)) { > > > - const char *entry; > > > - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > > - setup_qos(entry, &qos_map); > > > + if (handled && > > > + set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, > > > + b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) { > > > + struct shash_node *entry; > > > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > > + setup_qos(entry->name, entry->data, b_ctx_out->qos_map); > > > } > > > } > > > > > > - destroy_qos_map(&qos_map); > > > return handled; > > > } > > > > > > diff --git a/controller/binding.h b/controller/binding.h > > > index 6c3a98b02..8be33eddb 100644 > > > --- a/controller/binding.h > > > +++ b/controller/binding.h > > > @@ -91,7 +91,8 @@ struct binding_ctx_out { > > > */ > > > bool non_vif_ports_changed; > > > > > > - struct sset *egress_ifaces; > > > + struct shash *egress_ifaces; > > > + struct hmap *qos_map; > > > /* smap of OVS interface name as key and > > > * OVS interface external_ids:iface-id as value. */ > > > struct smap *local_iface_ids; > > > @@ -195,6 +196,8 @@ void set_pb_chassis_in_sbrec(const struct sbrec_port_binding *pb, > > > const struct sbrec_chassis *chassis_rec, > > > bool is_set); > > > > > > +void destroy_qos_map(struct hmap *qos_map); > > > + > > > /* Corresponds to each Port_Binding.type. */ > > > enum en_lport_type { > > > LP_UNKNOWN, > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > > index d6251afb8..066d76869 100644 > > > --- a/controller/ovn-controller.c > > > +++ b/controller/ovn-controller.c > > > @@ -1276,7 +1276,8 @@ struct ed_type_runtime_data { > > > struct sset active_tunnels; > > > > > > /* runtime data engine private data. */ > > > - struct sset egress_ifaces; > > > + struct shash egress_ifaces; > > > + struct hmap qos_map; > > > struct smap local_iface_ids; > > > > > > /* Tracked data. See below for more details and comments. */ > > > @@ -1372,7 +1373,8 @@ en_runtime_data_init(struct engine_node *node OVS_UNUSED, > > > sset_init(&data->local_lports); > > > related_lports_init(&data->related_lports); > > > sset_init(&data->active_tunnels); > > > - sset_init(&data->egress_ifaces); > > > + shash_init(&data->egress_ifaces); > > > + hmap_init(&data->qos_map); > > > smap_init(&data->local_iface_ids); > > > local_binding_data_init(&data->lbinding_data); > > > shash_init(&data->local_active_ports_ipv6_pd); > > > @@ -1392,7 +1394,8 @@ en_runtime_data_cleanup(void *data) > > > sset_destroy(&rt_data->local_lports); > > > related_lports_destroy(&rt_data->related_lports); > > > sset_destroy(&rt_data->active_tunnels); > > > - sset_destroy(&rt_data->egress_ifaces); > > > + shash_destroy(&rt_data->egress_ifaces); > > > + destroy_qos_map(&rt_data->qos_map); > > > smap_destroy(&rt_data->local_iface_ids); > > > local_datapaths_destroy(&rt_data->local_datapaths); > > > shash_destroy(&rt_data->local_active_ports_ipv6_pd); > > > @@ -1481,6 +1484,7 @@ init_binding_ctx(struct engine_node *node, > > > b_ctx_out->related_lports_changed = false; > > > b_ctx_out->non_vif_ports_changed = false; > > > b_ctx_out->egress_ifaces = &rt_data->egress_ifaces; > > > + b_ctx_out->qos_map = &rt_data->qos_map; > > > b_ctx_out->lbinding_data = &rt_data->lbinding_data; > > > b_ctx_out->local_iface_ids = &rt_data->local_iface_ids; > > > b_ctx_out->postponed_ports = rt_data->postponed_ports; > > > @@ -1510,13 +1514,14 @@ en_runtime_data_run(struct engine_node *node, void *data) > > > sset_destroy(local_lports); > > > related_lports_destroy(&rt_data->related_lports); > > > sset_destroy(active_tunnels); > > > - sset_destroy(&rt_data->egress_ifaces); > > > + shash_clear(&rt_data->egress_ifaces); > > > + destroy_qos_map(&rt_data->qos_map); > > > + hmap_init(&rt_data->qos_map); > > > smap_destroy(&rt_data->local_iface_ids); > > > hmap_init(local_datapaths); > > > sset_init(local_lports); > > > related_lports_init(&rt_data->related_lports); > > > sset_init(active_tunnels); > > > - sset_init(&rt_data->egress_ifaces); > > > smap_init(&rt_data->local_iface_ids); > > > local_binding_data_init(&rt_data->lbinding_data); > > > } > > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > > index 3e904c9dc..c986198a2 100644 > > > --- a/tests/system-ovn.at > > > +++ b/tests/system-ovn.at > > > @@ -6335,6 +6335,10 @@ ADD_NAMESPACES(sw01) > > > ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03") > > > ovn-nbctl lsp-add sw0 sw01 \ > > > -- lsp-set-addresses sw01 "f0:00:00:01:02:03 192.168.1.2" > > > +ADD_NAMESPACES(sw02) > > > +ADD_VETH(sw02, sw02, br-int, "192.168.1.3/24", "f2:00:00:01:02:03") > > > +ovn-nbctl lsp-add sw0 sw02 \ > > > + -- lsp-set-addresses sw02 "f2:00:00:01:02:03 192.168.1.3" > > > > > > ADD_NAMESPACES(public) > > > ADD_VETH(public, public, br-ext, "192.168.2.2/24", "f0:00:00:01:02:05") > > > @@ -6345,6 +6349,7 @@ ovn-nbctl lsp-add sw0 public \ > > > -- lsp-set-type public localnet \ > > > -- lsp-set-options public network_name=phynet > > > > > > +# Setup QoS on a localnet port > > > AT_CHECK([ovn-nbctl set Logical_Switch_Port public options:qos_min_rate=200000]) > > > AT_CHECK([ovn-nbctl set Logical_Switch_Port public options:qos_max_rate=300000]) > > > AT_CHECK([ovn-nbctl set Logical_Switch_Port public options:qos_burst=3000000]) > > > @@ -6353,15 +6358,58 @@ 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']) > > > > > > +# Setup QoS on a logical switch ports > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_min_rate=400000]) > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_max_rate=800000]) > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_burst=5000000]) > > > +AT_CHECK([ovs-vsctl set interface ovs-sw01 external-ids:ovn-egress-iface=true]) > > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01']) > > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \ > > > + grep -q 'class htb .* rate 400Kbit ceil 800Kbit burst 625000b cburst 625000b']) > > > + > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qos_min_rate=600000]) > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qos_max_rate=6000000]) > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qos_burst=6000000]) > > > +AT_CHECK([ovs-vsctl set interface ovs-sw02 external-ids:ovn-egress-iface=true]) > > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw02']) > > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw02 | \ > > > + grep -q 'class htb .* rate 600Kbit ceil 6Mbit burst 750000b cburst 750000b']) > > > + > > > +AT_CHECK([ovn-appctl -t ovn-controller exit --restart]) > > > +OVS_WAIT_UNTIL([test "$(pidof ovn-controller)" = ""]) > > > +start_daemon ovn-controller > > > +OVS_WAIT_UNTIL([test "$(pidof ovn-controller)" != ""]) > > > + > > > +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']) > > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01']) > > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \ > > > + grep -q 'class htb .* rate 400Kbit ceil 800Kbit burst 625000b cburst 625000b']) > > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw02']) > > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw02 | \ > > > + grep -q 'class htb .* rate 600Kbit ceil 6Mbit burst 750000b cburst 750000b']) > > > > > > 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_max_rate=300000]) > > > 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')" = ""]) > > > > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_min_rate=200000]) > > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01']) > > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \ > > > + grep -q 'class htb .* rate 200Kbit ceil 800Kbit burst 625000b cburst 625000b']) > > > + > > > +# Disable QoS rules from logical switch ports > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qdisc_queue_id=0]) > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qdisc_queue_id=0]) > > > +OVS_WAIT_UNTIL([test "$(tc qdisc show | grep 'htb 1: dev ovs-sw01')" = ""]) > > > +OVS_WAIT_UNTIL([test "$(tc qdisc show | grep 'htb 1: dev ovs-sw02')" = ""]) > > > + > > > kill $(pidof ovn-controller) > > > > > > as ovn-sb > > > -- > > > 2.38.1 > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
> On Fri, Dec 9, 2022 at 9:43 AM Lorenzo Bianconi > <lorenzo.bianconi@redhat.com> wrote: > > > > > On Sun, Dec 4, 2022 at 5:06 PM Lorenzo Bianconi > > > <lorenzo.bianconi@redhat.com> wrote: > > > > > > > > Introduce the capability to apply QoS rules for logical switch ports > > > > claimed by ovn-controller. Rely on shash instead of sset for > > > > egress_ifaces. > > > > > > > > Acked-by: Mark Michelson <mmichels@redhat.com> > > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129742 > > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > > > > > Hi Lorenzo, > > > > Hi Numan, > > > > thx for the review :) > > > > > > > > Thanks for addressing the comments. > > > > > > I tested this version and compared with the present main. > > > > > > I see a few differences. > > > > > > With the main version, when I configure qos params to a localnet > > > logical port, ovn-controller creates a qdisc on the tunnel interface > > > which I guess is expected. > > > But I'm still not sure why we need to configure qdiscs on the tunnel > > > interface. But that's a story. Perhaps I need to see the original > > > QoS commits and understand why it was added. > > > > > > With your patch, I don't see that happening. Perhaps there is a bug > > > in setup_qos() function now that 'shash' is used to store the egress > > > ifaces with the logical port and > > > there is no logical port for the tunnel interfaces. > > > > uhm, I can look into this. Do you think we should keep this behaviour? > > Frankly I don't know. We need to find out why qos was configured in > the first place. > > I do agree with Ilya's comment that it is odd to configure qos on the > tunnel interface > and that could impact the other traffic. > > > > > > > > > > Regarding the option - "ovn-egress-iface". I suppose the expectation > > > from the CMS is to set the qos parameters in the logical port and also > > > set this option in the > > > ovs interface right ? I don't really see a need for this. I think if > > > CMS configures the qos parameters in the logical port options, > > > ovn-controller should configure the qos. > > > I think otherwise this would complicate the CMS implementation. Thoughts ? > > > > ovn-egress-iface is mandatory since it is used in setup_qos() to look for the > > proper netdevice pointer to configure ovs qdisc (please note it is used to > > identify ovs interfaces and not ovn ones). > > I think it can be doable (not sure about the complexity) to avoid it for > > logical switch ports (since we have iface-id in them external_ids column of > > ovs interface table) but I do not think we can avoid to set "ovn-egress-iface" > > for localnet port since afaik there is no direct link between ovn localnet port > > and ovs interface used to connect the underlay network, right? If so I guess > > it is better to keep it for both localnet and lsp ports. Do you agree? > > I think it's ok to expect CMS to set "ovn-egress-iface" for the > physical nic connected to the > provider bridge. But I don't think we should do the same for the > normal logical port VIFs. does it worth to introduce a difference here? > > > > > > > > > > > > > > Please see a few comments below > > > > > > > --- > > > > Changes since v3: > > > > - fix typo in new system-ovn test > > > > Changes since v2: > > > > - fix qos configuration restarting ovn-controller > > > > Changes since v1: > > > > - improve ovs interface lookup > > > > - improve system-tests > > > > --- > > > > controller/binding.c | 155 ++++++++++++++++++++++-------------- > > > > controller/binding.h | 5 +- > > > > controller/ovn-controller.c | 15 ++-- > > > > tests/system-ovn.at | 48 +++++++++++ > > > > 4 files changed, 156 insertions(+), 67 deletions(-) > > > > > > > > diff --git a/controller/binding.c b/controller/binding.c > > > > index 5df62baef..53520263c 100644 > > > > --- a/controller/binding.c > > > > +++ b/controller/binding.c > > > > @@ -115,6 +115,7 @@ struct qos_queue { > > > > uint32_t min_rate; > > > > uint32_t max_rate; > > > > uint32_t burst; > > > > + char *port_name; > > > > }; > > > > > > > > void > > > > @@ -147,25 +148,50 @@ static void update_lport_tracking(const struct sbrec_port_binding *pb, > > > > struct hmap *tracked_dp_bindings, > > > > bool claimed); > > > > > > > > +static bool is_lport_vif(const struct sbrec_port_binding *pb); > > > > + > > > > +static struct qos_queue * > > > > +get_qos_map_entry(struct hmap *queue_map, const char *name) > > > > +{ > > > > + struct qos_queue *qos_node; > > > > + HMAP_FOR_EACH (qos_node, node, queue_map) { > > > > + if (!strcmp(qos_node->port_name, name)) { > > > > + return qos_node; > > > > + } > > > > + } > > > > + > > > > + return NULL; > > > > +} > > > > + > > > > static void > > > > -get_qos_params(const struct sbrec_port_binding *pb, struct hmap *queue_map) > > > > +update_qos_params(const struct sbrec_port_binding *pb, struct hmap *queue_map) > > > > { > > > > uint32_t min_rate = smap_get_int(&pb->options, "qos_min_rate", 0); > > > > uint32_t max_rate = smap_get_int(&pb->options, "qos_max_rate", 0); > > > > uint32_t burst = smap_get_int(&pb->options, "qos_burst", 0); > > > > uint32_t queue_id = smap_get_int(&pb->options, "qdisc_queue_id", 0); > > > > > > > > + struct qos_queue *node = get_qos_map_entry(queue_map, pb->logical_port); > > > > + > > > > if ((!min_rate && !max_rate && !burst) || !queue_id) { > > > > /* Qos is not configured for this port. */ > > > > + if (node) { > > > > + hmap_remove(queue_map, &node->node); > > > > + free(node->port_name); > > > > + free(node); > > > > + } > > > > return; > > > > } > > > > > > > > - struct qos_queue *node = xzalloc(sizeof *node); > > > > - hmap_insert(queue_map, &node->node, hash_int(queue_id, 0)); > > > > + if (!node) { > > > > + node = xzalloc(sizeof *node); > > > > + hmap_insert(queue_map, &node->node, hash_int(queue_id, 0)); > > > > + node->port_name = xstrdup(pb->logical_port); > > > > + } > > > > + node->queue_id = queue_id; > > > > node->min_rate = min_rate; > > > > node->max_rate = max_rate; > > > > node->burst = burst; > > > > - node->queue_id = queue_id; > > > > } > > > > > > > > static const struct ovsrec_qos * > > > > @@ -191,7 +217,7 @@ static bool > > > > set_noop_qos(struct ovsdb_idl_txn *ovs_idl_txn, > > > > const struct ovsrec_port_table *port_table, > > > > const struct ovsrec_qos_table *qos_table, > > > > - struct sset *egress_ifaces) > > > > + struct shash *egress_ifaces) > > > > { > > > > if (!ovs_idl_txn) { > > > > return false; > > > > @@ -206,11 +232,11 @@ set_noop_qos(struct ovsdb_idl_txn *ovs_idl_txn, > > > > size_t count = 0; > > > > > > > > OVSREC_PORT_TABLE_FOR_EACH (port, port_table) { > > > > - if (sset_contains(egress_ifaces, port->name)) { > > > > + if (shash_find(egress_ifaces, port->name)) { > > > > ovsrec_port_set_qos(port, noop_qos); > > > > count++; > > > > } > > > > - if (sset_count(egress_ifaces) == count) { > > > > + if (shash_count(egress_ifaces) == count) { > > > > break; > > > > } > > > > } > > > > @@ -236,7 +262,8 @@ set_qos_type(struct netdev *netdev, const char *type) > > > > } > > > > > > > > static void > > > > -setup_qos(const char *egress_iface, struct hmap *queue_map) > > > > +setup_qos(const char *egress_iface, const char *logical_port, > > > > + struct hmap *queue_map) > > > > { > > > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); > > > > struct netdev *netdev_phy; > > > > @@ -281,7 +308,7 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) > > > > * a configuration setting. > > > > * > > > > * - Otherwise leave the qdisc alone. */ > > > > - if (hmap_is_empty(queue_map)) { > > > > + if (!get_qos_map_entry(queue_map, logical_port)) { > > > > if (!strcmp(qdisc_type, OVN_QOS_TYPE)) { > > > > set_qos_type(netdev_phy, ""); > > > > } > > > > @@ -338,6 +365,10 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) > > > > continue; > > > > } > > > > > > > > + if (strcmp(sb_info->port_name, logical_port)) { > > > > + continue; > > > > + } > > > > + > > > > smap_clear(&queue_details); > > > > smap_add_format(&queue_details, "min-rate", "%d", sb_info->min_rate); > > > > smap_add_format(&queue_details, "max-rate", "%d", sb_info->max_rate); > > > > @@ -354,11 +385,12 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) > > > > netdev_close(netdev_phy); > > > > } > > > > > > > > -static void > > > > +void > > > > destroy_qos_map(struct hmap *qos_map) > > > > { > > > > struct qos_queue *qos_queue; > > > > HMAP_FOR_EACH_POP (qos_queue, node, qos_map) { > > > > + free(qos_queue->port_name); > > > > free(qos_queue); > > > > } > > > > > > > > @@ -404,7 +436,7 @@ sbrec_get_port_encap(const struct sbrec_chassis *chassis_rec, > > > > static void > > > > add_localnet_egress_interface_mappings( > > > > const struct sbrec_port_binding *port_binding, > > > > - struct shash *bridge_mappings, struct sset *egress_ifaces) > > > > + struct shash *bridge_mappings, struct shash *egress_ifaces) > > > > { > > > > const char *network = smap_get(&port_binding->options, "network_name"); > > > > if (!network) { > > > > @@ -429,7 +461,8 @@ add_localnet_egress_interface_mappings( > > > > if (!is_egress_iface) { > > > > continue; > > > > } > > > > - sset_add(egress_ifaces, iface_rec->name); > > > > + shash_add(egress_ifaces, iface_rec->name, > > > > + port_binding->logical_port); > > > > } > > > > } > > > > } > > > > @@ -474,7 +507,7 @@ update_ld_multichassis_ports(const struct sbrec_port_binding *binding_rec, > > > > static void > > > > update_ld_localnet_port(const struct sbrec_port_binding *binding_rec, > > > > struct shash *bridge_mappings, > > > > - struct sset *egress_ifaces, > > > > + struct shash *egress_ifaces, > > > > struct hmap *local_datapaths) > > > > { > > > > /* Ignore localnet ports for unplugged networks. */ > > > > @@ -1456,7 +1489,7 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, > > > > b_ctx_out->tracked_dp_bindings); > > > > } > > > > if (b_lport->lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) { > > > > - get_qos_params(pb, qos_map); > > > > + update_qos_params(pb, qos_map); > > > > } > > > > } else { > > > > /* We could, but can't claim the lport. */ > > > > @@ -1519,6 +1552,16 @@ consider_vif_lport(const struct sbrec_port_binding *pb, > > > > b_lport = local_binding_add_lport(binding_lports, lbinding, pb, > > > > LP_VIF); > > > > } > > > > + > > > > + if (lbinding->iface && > > > > + smap_get(&lbinding->iface->external_ids, "ovn-egress-iface")) { > > > > + const char *iface_id = smap_get(&lbinding->iface->external_ids, > > > > + "iface-id"); > > > > + if (iface_id) { > > > > + shash_add(b_ctx_out->egress_ifaces, lbinding->iface->name, > > > > + iface_id); > > > > > > When a qos is configured on a logical port, this patch is adding 2 > > > entries in the 'egress_ifaces' shash for the same logical port. > > > I think it's because of the above 'shash_add'. I think you should use > > > shash_replace instead. > > > > ack, I will fix it. > > > > > > > > When a full recompute happens, the function build_local_bindings() > > > also adds the qos configured logical ports to the 'egress_ifaces' > > > and later if there are any updates to the logical port, this function > > > - consider_vif_lport() also adds it to the shash. > > > > > > IMO the qos support in OVN needs a different approach. Instead of > > > configuring the qos using netdev() perhaps we should rely on the OVS > > > meters. > > > Maybe this was brought up earlier too. > > > > > > > > I'm of the opinion that instead of supporting Qos for logical ports > > > using netdev, we should use OVS meters (not just for logical ports, > > > even for localnet ports). Thoughts ? > > > > ovn qos is used for egress traffic (shaping) while ovn meters are used > > for incoming one (policing). > > What we can do is to avoid configuring directly netdev qdisc and rely on > > ovs to perform this configuration but this would be quite a massive rework. > > Do you think we should do it now before adding this feature or it is ok > > to do it after we added this feature? I do not have a strong opinion about it. > > IMO its better to add this new feature using the native OVS QoS support. > And along with that we can remove the netdev implementation from OVN. > > This is what I think. Anyway this feature is out of window for 22.12. > We can perhaps target this feature with the new approach in 23.03. > > Although I will not object if other maintainers are fine getting this > feature first > and then reimplement using OVS native QoS support. I am fine to work on a proper solution for lsp and then move localnet port too. Regards, Lorenzo > > Thanks > Numan > > > > > Regards, > > Lorenzo > > > > > > > > @Dumitru Ceara @Han Zhou @Mark Michelson @Ilya Maximets Do you have > > > any comments or suggestions here ? > > > > > > Thanks > > > Numan > > > > > > > > > > > > > + } > > > > + } > > > > } > > > > > > > > return consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out, > > > > @@ -1785,7 +1828,7 @@ consider_localnet_lport(const struct sbrec_port_binding *pb, > > > > update_local_lports(pb->logical_port, b_ctx_out); > > > > > > > > if (qos_map && b_ctx_in->ovs_idl_txn) { > > > > - get_qos_params(pb, qos_map); > > > > + update_qos_params(pb, qos_map); > > > > } > > > > > > > > update_related_lport(pb, b_ctx_out); > > > > @@ -1861,14 +1904,14 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in, > > > > &b_ctx_out->lbinding_data->bindings; > > > > for (j = 0; j < port_rec->n_interfaces; j++) { > > > > const struct ovsrec_interface *iface_rec; > > > > + struct local_binding *lbinding = NULL; > > > > > > > > iface_rec = port_rec->interfaces[j]; > > > > iface_id = smap_get(&iface_rec->external_ids, "iface-id"); > > > > int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; > > > > > > > > if (iface_id && ofport > 0) { > > > > - struct local_binding *lbinding = > > > > - local_binding_find(local_bindings, iface_id); > > > > + lbinding = local_binding_find(local_bindings, iface_id); > > > > if (!lbinding) { > > > > lbinding = local_binding_create(iface_id, iface_rec); > > > > local_binding_add(local_bindings, lbinding); > > > > @@ -1895,8 +1938,11 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in, > > > > const char *tunnel_iface > > > > = smap_get(&iface_rec->status, "tunnel_egress_iface"); > > > > if (tunnel_iface) { > > > > - sset_add(b_ctx_out->egress_ifaces, tunnel_iface); > > > > + shash_add(b_ctx_out->egress_ifaces, tunnel_iface, ""); > > > > } > > > > + } else if (lbinding && smap_get(&iface_rec->external_ids, > > > > + "ovn-egress-iface")) { > > > > + shash_add(b_ctx_out->egress_ifaces, iface_rec->name, iface_id); > > > > } > > > > } > > > > } > > > > @@ -1910,16 +1956,11 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > > > > } > > > > > > > > struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings); > > > > - struct hmap qos_map; > > > > > > > > - hmap_init(&qos_map); > > > > if (b_ctx_in->br_int) { > > > > build_local_bindings(b_ctx_in, b_ctx_out); > > > > } > > > > > > > > - struct hmap *qos_map_ptr = > > > > - !sset_is_empty(b_ctx_out->egress_ifaces) ? &qos_map : NULL; > > > > - > > > > struct ovs_list localnet_lports = OVS_LIST_INITIALIZER(&localnet_lports); > > > > struct ovs_list external_lports = OVS_LIST_INITIALIZER(&external_lports); > > > > struct ovs_list multichassis_ports = OVS_LIST_INITIALIZER( > > > > @@ -1956,7 +1997,8 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > > > > break; > > > > > > > > case LP_VIF: > > > > - consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL, qos_map_ptr); > > > > + consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL, > > > > + b_ctx_out->qos_map); > > > > if (pb->additional_chassis) { > > > > struct lport *multichassis_lport = xmalloc( > > > > sizeof *multichassis_lport); > > > > @@ -1967,11 +2009,13 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > > > > break; > > > > > > > > case LP_CONTAINER: > > > > - consider_container_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); > > > > + consider_container_lport(pb, b_ctx_in, b_ctx_out, > > > > + b_ctx_out->qos_map); > > > > break; > > > > > > > > case LP_VIRTUAL: > > > > - consider_virtual_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); > > > > + consider_virtual_lport(pb, b_ctx_in, b_ctx_out, > > > > + b_ctx_out->qos_map); > > > > break; > > > > > > > > case LP_L2GATEWAY: > > > > @@ -1994,7 +2038,8 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > > > > break; > > > > > > > > case LP_LOCALNET: { > > > > - consider_localnet_lport(pb, b_ctx_in, b_ctx_out, &qos_map); > > > > + consider_localnet_lport(pb, b_ctx_in, b_ctx_out, > > > > + b_ctx_out->qos_map); > > > > struct lport *lnet_lport = xmalloc(sizeof *lnet_lport); > > > > lnet_lport->pb = pb; > > > > ovs_list_push_back(&localnet_lports, &lnet_lport->list_node); > > > > @@ -2051,17 +2096,15 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > > > > > > > > shash_destroy(&bridge_mappings); > > > > > > > > - if (!sset_is_empty(b_ctx_out->egress_ifaces) > > > > + if (!shash_is_empty(b_ctx_out->egress_ifaces) > > > > && set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, > > > > b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) { > > > > - const char *entry; > > > > - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > > > - setup_qos(entry, &qos_map); > > > > + struct shash_node *entry; > > > > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > > > + setup_qos(entry->name, entry->data, b_ctx_out->qos_map); > > > > } > > > > } > > > > > > > > - destroy_qos_map(&qos_map); > > > > - > > > > cleanup_claimed_port_timestamps(); > > > > } > > > > > > > > @@ -2447,7 +2490,7 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, > > > > } > > > > > > > > if (smap_get(&iface_rec->external_ids, "ovn-egress-iface") || > > > > - sset_contains(b_ctx_out->egress_ifaces, iface_rec->name)) { > > > > + shash_find(b_ctx_out->egress_ifaces, iface_rec->name)) { > > > > handled = false; > > > > break; > > > > } > > > > @@ -2493,10 +2536,6 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, > > > > return false; > > > > } > > > > > > > > - struct hmap qos_map = HMAP_INITIALIZER(&qos_map); > > > > - struct hmap *qos_map_ptr = > > > > - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; > > > > - > > > > /* > > > > * We consider an OVS interface for claiming if the following > > > > * 2 conditions are met: > > > > @@ -2525,24 +2564,22 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, > > > > if (iface_id && ofport > 0 && > > > > is_iface_in_int_bridge(iface_rec, b_ctx_in->br_int)) { > > > > handled = consider_iface_claim(iface_rec, iface_id, b_ctx_in, > > > > - b_ctx_out, qos_map_ptr); > > > > + b_ctx_out, b_ctx_out->qos_map); > > > > if (!handled) { > > > > break; > > > > } > > > > } > > > > } > > > > > > > > - if (handled && qos_map_ptr && set_noop_qos(b_ctx_in->ovs_idl_txn, > > > > - b_ctx_in->port_table, > > > > - b_ctx_in->qos_table, > > > > - b_ctx_out->egress_ifaces)) { > > > > - const char *entry; > > > > - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > > > - setup_qos(entry, &qos_map); > > > > + if (handled && > > > > + set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, > > > > + b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) { > > > > + struct shash_node *entry; > > > > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > > > + setup_qos(entry->name, entry->data, b_ctx_out->qos_map); > > > > } > > > > } > > > > > > > > - destroy_qos_map(&qos_map); > > > > return handled; > > > > } > > > > > > > > @@ -2977,10 +3014,6 @@ delete_done: > > > > return false; > > > > } > > > > > > > > - struct hmap qos_map = HMAP_INITIALIZER(&qos_map); > > > > - struct hmap *qos_map_ptr = > > > > - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; > > > > - > > > > SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, > > > > b_ctx_in->port_binding_table) { > > > > /* Loop to handle create and update changes only. */ > > > > @@ -2992,7 +3025,8 @@ delete_done: > > > > update_ld_peers(pb, b_ctx_out->local_datapaths); > > > > } > > > > > > > > - handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, qos_map_ptr); > > > > + handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, > > > > + b_ctx_out->qos_map); > > > > if (!handled) { > > > > break; > > > > } > > > > @@ -3009,7 +3043,8 @@ delete_done: > > > > sset_find_and_delete(b_ctx_out->postponed_ports, port_name); > > > > continue; > > > > } > > > > - handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, qos_map_ptr); > > > > + handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, > > > > + b_ctx_out->qos_map); > > > > if (!handled) { > > > > break; > > > > } > > > > @@ -3055,17 +3090,15 @@ delete_done: > > > > shash_destroy(&bridge_mappings); > > > > } > > > > > > > > - if (handled && qos_map_ptr && set_noop_qos(b_ctx_in->ovs_idl_txn, > > > > - b_ctx_in->port_table, > > > > - b_ctx_in->qos_table, > > > > - b_ctx_out->egress_ifaces)) { > > > > - const char *entry; > > > > - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > > > - setup_qos(entry, &qos_map); > > > > + if (handled && > > > > + set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, > > > > + b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) { > > > > + struct shash_node *entry; > > > > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > > > + setup_qos(entry->name, entry->data, b_ctx_out->qos_map); > > > > } > > > > } > > > > > > > > - destroy_qos_map(&qos_map); > > > > return handled; > > > > } > > > > > > > > diff --git a/controller/binding.h b/controller/binding.h > > > > index 6c3a98b02..8be33eddb 100644 > > > > --- a/controller/binding.h > > > > +++ b/controller/binding.h > > > > @@ -91,7 +91,8 @@ struct binding_ctx_out { > > > > */ > > > > bool non_vif_ports_changed; > > > > > > > > - struct sset *egress_ifaces; > > > > + struct shash *egress_ifaces; > > > > + struct hmap *qos_map; > > > > /* smap of OVS interface name as key and > > > > * OVS interface external_ids:iface-id as value. */ > > > > struct smap *local_iface_ids; > > > > @@ -195,6 +196,8 @@ void set_pb_chassis_in_sbrec(const struct sbrec_port_binding *pb, > > > > const struct sbrec_chassis *chassis_rec, > > > > bool is_set); > > > > > > > > +void destroy_qos_map(struct hmap *qos_map); > > > > + > > > > /* Corresponds to each Port_Binding.type. */ > > > > enum en_lport_type { > > > > LP_UNKNOWN, > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > > > index d6251afb8..066d76869 100644 > > > > --- a/controller/ovn-controller.c > > > > +++ b/controller/ovn-controller.c > > > > @@ -1276,7 +1276,8 @@ struct ed_type_runtime_data { > > > > struct sset active_tunnels; > > > > > > > > /* runtime data engine private data. */ > > > > - struct sset egress_ifaces; > > > > + struct shash egress_ifaces; > > > > + struct hmap qos_map; > > > > struct smap local_iface_ids; > > > > > > > > /* Tracked data. See below for more details and comments. */ > > > > @@ -1372,7 +1373,8 @@ en_runtime_data_init(struct engine_node *node OVS_UNUSED, > > > > sset_init(&data->local_lports); > > > > related_lports_init(&data->related_lports); > > > > sset_init(&data->active_tunnels); > > > > - sset_init(&data->egress_ifaces); > > > > + shash_init(&data->egress_ifaces); > > > > + hmap_init(&data->qos_map); > > > > smap_init(&data->local_iface_ids); > > > > local_binding_data_init(&data->lbinding_data); > > > > shash_init(&data->local_active_ports_ipv6_pd); > > > > @@ -1392,7 +1394,8 @@ en_runtime_data_cleanup(void *data) > > > > sset_destroy(&rt_data->local_lports); > > > > related_lports_destroy(&rt_data->related_lports); > > > > sset_destroy(&rt_data->active_tunnels); > > > > - sset_destroy(&rt_data->egress_ifaces); > > > > + shash_destroy(&rt_data->egress_ifaces); > > > > + destroy_qos_map(&rt_data->qos_map); > > > > smap_destroy(&rt_data->local_iface_ids); > > > > local_datapaths_destroy(&rt_data->local_datapaths); > > > > shash_destroy(&rt_data->local_active_ports_ipv6_pd); > > > > @@ -1481,6 +1484,7 @@ init_binding_ctx(struct engine_node *node, > > > > b_ctx_out->related_lports_changed = false; > > > > b_ctx_out->non_vif_ports_changed = false; > > > > b_ctx_out->egress_ifaces = &rt_data->egress_ifaces; > > > > + b_ctx_out->qos_map = &rt_data->qos_map; > > > > b_ctx_out->lbinding_data = &rt_data->lbinding_data; > > > > b_ctx_out->local_iface_ids = &rt_data->local_iface_ids; > > > > b_ctx_out->postponed_ports = rt_data->postponed_ports; > > > > @@ -1510,13 +1514,14 @@ en_runtime_data_run(struct engine_node *node, void *data) > > > > sset_destroy(local_lports); > > > > related_lports_destroy(&rt_data->related_lports); > > > > sset_destroy(active_tunnels); > > > > - sset_destroy(&rt_data->egress_ifaces); > > > > + shash_clear(&rt_data->egress_ifaces); > > > > + destroy_qos_map(&rt_data->qos_map); > > > > + hmap_init(&rt_data->qos_map); > > > > smap_destroy(&rt_data->local_iface_ids); > > > > hmap_init(local_datapaths); > > > > sset_init(local_lports); > > > > related_lports_init(&rt_data->related_lports); > > > > sset_init(active_tunnels); > > > > - sset_init(&rt_data->egress_ifaces); > > > > smap_init(&rt_data->local_iface_ids); > > > > local_binding_data_init(&rt_data->lbinding_data); > > > > } > > > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > > > index 3e904c9dc..c986198a2 100644 > > > > --- a/tests/system-ovn.at > > > > +++ b/tests/system-ovn.at > > > > @@ -6335,6 +6335,10 @@ ADD_NAMESPACES(sw01) > > > > ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03") > > > > ovn-nbctl lsp-add sw0 sw01 \ > > > > -- lsp-set-addresses sw01 "f0:00:00:01:02:03 192.168.1.2" > > > > +ADD_NAMESPACES(sw02) > > > > +ADD_VETH(sw02, sw02, br-int, "192.168.1.3/24", "f2:00:00:01:02:03") > > > > +ovn-nbctl lsp-add sw0 sw02 \ > > > > + -- lsp-set-addresses sw02 "f2:00:00:01:02:03 192.168.1.3" > > > > > > > > ADD_NAMESPACES(public) > > > > ADD_VETH(public, public, br-ext, "192.168.2.2/24", "f0:00:00:01:02:05") > > > > @@ -6345,6 +6349,7 @@ ovn-nbctl lsp-add sw0 public \ > > > > -- lsp-set-type public localnet \ > > > > -- lsp-set-options public network_name=phynet > > > > > > > > +# Setup QoS on a localnet port > > > > AT_CHECK([ovn-nbctl set Logical_Switch_Port public options:qos_min_rate=200000]) > > > > AT_CHECK([ovn-nbctl set Logical_Switch_Port public options:qos_max_rate=300000]) > > > > AT_CHECK([ovn-nbctl set Logical_Switch_Port public options:qos_burst=3000000]) > > > > @@ -6353,15 +6358,58 @@ 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']) > > > > > > > > +# Setup QoS on a logical switch ports > > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_min_rate=400000]) > > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_max_rate=800000]) > > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_burst=5000000]) > > > > +AT_CHECK([ovs-vsctl set interface ovs-sw01 external-ids:ovn-egress-iface=true]) > > > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01']) > > > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \ > > > > + grep -q 'class htb .* rate 400Kbit ceil 800Kbit burst 625000b cburst 625000b']) > > > > + > > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qos_min_rate=600000]) > > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qos_max_rate=6000000]) > > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qos_burst=6000000]) > > > > +AT_CHECK([ovs-vsctl set interface ovs-sw02 external-ids:ovn-egress-iface=true]) > > > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw02']) > > > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw02 | \ > > > > + grep -q 'class htb .* rate 600Kbit ceil 6Mbit burst 750000b cburst 750000b']) > > > > + > > > > +AT_CHECK([ovn-appctl -t ovn-controller exit --restart]) > > > > +OVS_WAIT_UNTIL([test "$(pidof ovn-controller)" = ""]) > > > > +start_daemon ovn-controller > > > > +OVS_WAIT_UNTIL([test "$(pidof ovn-controller)" != ""]) > > > > + > > > > +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']) > > > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01']) > > > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \ > > > > + grep -q 'class htb .* rate 400Kbit ceil 800Kbit burst 625000b cburst 625000b']) > > > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw02']) > > > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw02 | \ > > > > + grep -q 'class htb .* rate 600Kbit ceil 6Mbit burst 750000b cburst 750000b']) > > > > > > > > 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_max_rate=300000]) > > > > 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')" = ""]) > > > > > > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_min_rate=200000]) > > > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01']) > > > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \ > > > > + grep -q 'class htb .* rate 200Kbit ceil 800Kbit burst 625000b cburst 625000b']) > > > > + > > > > +# Disable QoS rules from logical switch ports > > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qdisc_queue_id=0]) > > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qdisc_queue_id=0]) > > > > +OVS_WAIT_UNTIL([test "$(tc qdisc show | grep 'htb 1: dev ovs-sw01')" = ""]) > > > > +OVS_WAIT_UNTIL([test "$(tc qdisc show | grep 'htb 1: dev ovs-sw02')" = ""]) > > > > + > > > > kill $(pidof ovn-controller) > > > > > > > > as ovn-sb > > > > -- > > > > 2.38.1 > > > > > > > > _______________________________________________ > > > > dev mailing list > > > > dev@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
On Sun, Dec 11, 2022 at 8:54 AM Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote: > > > On Fri, Dec 9, 2022 at 9:43 AM Lorenzo Bianconi > > <lorenzo.bianconi@redhat.com> wrote: > > > > > > > On Sun, Dec 4, 2022 at 5:06 PM Lorenzo Bianconi > > > > <lorenzo.bianconi@redhat.com> wrote: > > > > > > > > > > Introduce the capability to apply QoS rules for logical switch ports > > > > > claimed by ovn-controller. Rely on shash instead of sset for > > > > > egress_ifaces. > > > > > > > > > > Acked-by: Mark Michelson <mmichels@redhat.com> > > > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129742 > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > > > > > > > Hi Lorenzo, > > > > > > Hi Numan, > > > > > > thx for the review :) > > > > > > > > > > > Thanks for addressing the comments. > > > > > > > > I tested this version and compared with the present main. > > > > > > > > I see a few differences. > > > > > > > > With the main version, when I configure qos params to a localnet > > > > logical port, ovn-controller creates a qdisc on the tunnel interface > > > > which I guess is expected. > > > > But I'm still not sure why we need to configure qdiscs on the tunnel > > > > interface. But that's a story. Perhaps I need to see the original > > > > QoS commits and understand why it was added. > > > > > > > > With your patch, I don't see that happening. Perhaps there is a bug > > > > in setup_qos() function now that 'shash' is used to store the egress > > > > ifaces with the logical port and > > > > there is no logical port for the tunnel interfaces. > > > > > > uhm, I can look into this. Do you think we should keep this behaviour? > > > > Frankly I don't know. We need to find out why qos was configured in > > the first place. > > > > I do agree with Ilya's comment that it is odd to configure qos on the > > tunnel interface > > and that could impact the other traffic. > > > > > > > > > > > > > > > Regarding the option - "ovn-egress-iface". I suppose the expectation > > > > from the CMS is to set the qos parameters in the logical port and also > > > > set this option in the > > > > ovs interface right ? I don't really see a need for this. I think if > > > > CMS configures the qos parameters in the logical port options, > > > > ovn-controller should configure the qos. > > > > I think otherwise this would complicate the CMS implementation. Thoughts ? > > > > > > ovn-egress-iface is mandatory since it is used in setup_qos() to look for the > > > proper netdevice pointer to configure ovs qdisc (please note it is used to > > > identify ovs interfaces and not ovn ones). > > > I think it can be doable (not sure about the complexity) to avoid it for > > > logical switch ports (since we have iface-id in them external_ids column of > > > ovs interface table) but I do not think we can avoid to set "ovn-egress-iface" > > > for localnet port since afaik there is no direct link between ovn localnet port > > > and ovs interface used to connect the underlay network, right? If so I guess > > > it is better to keep it for both localnet and lsp ports. Do you agree? > > > > I think it's ok to expect CMS to set "ovn-egress-iface" for the > > physical nic connected to the > > provider bridge. But I don't think we should do the same for the > > normal logical port VIFs. > > does it worth to introduce a difference here? Yes. Configuring ovn-egress-iface=true for a phsyical interface connected to the provider bridge is a one time activity which CMS can easily do during installation time. I think openstack installer - tripleo does that during deployment. But to set ovn-egress-iface=true on every ovs interface would require some additional changes in CMS. If we take openstack as an example, if qos has to be enabled on a VM, then first neutron has to set the QoS options in the logical switch port and the "nova" compute component also has to set ovn-egress-iface=true when it creates a libvirt VM. This seems unnecessary to me. Thanks Numan > > > > > > > > > > > > > > > > > > > > > > Please see a few comments below > > > > > > > > > --- > > > > > Changes since v3: > > > > > - fix typo in new system-ovn test > > > > > Changes since v2: > > > > > - fix qos configuration restarting ovn-controller > > > > > Changes since v1: > > > > > - improve ovs interface lookup > > > > > - improve system-tests > > > > > --- > > > > > controller/binding.c | 155 ++++++++++++++++++++++-------------- > > > > > controller/binding.h | 5 +- > > > > > controller/ovn-controller.c | 15 ++-- > > > > > tests/system-ovn.at | 48 +++++++++++ > > > > > 4 files changed, 156 insertions(+), 67 deletions(-) > > > > > > > > > > diff --git a/controller/binding.c b/controller/binding.c > > > > > index 5df62baef..53520263c 100644 > > > > > --- a/controller/binding.c > > > > > +++ b/controller/binding.c > > > > > @@ -115,6 +115,7 @@ struct qos_queue { > > > > > uint32_t min_rate; > > > > > uint32_t max_rate; > > > > > uint32_t burst; > > > > > + char *port_name; > > > > > }; > > > > > > > > > > void > > > > > @@ -147,25 +148,50 @@ static void update_lport_tracking(const struct sbrec_port_binding *pb, > > > > > struct hmap *tracked_dp_bindings, > > > > > bool claimed); > > > > > > > > > > +static bool is_lport_vif(const struct sbrec_port_binding *pb); > > > > > + > > > > > +static struct qos_queue * > > > > > +get_qos_map_entry(struct hmap *queue_map, const char *name) > > > > > +{ > > > > > + struct qos_queue *qos_node; > > > > > + HMAP_FOR_EACH (qos_node, node, queue_map) { > > > > > + if (!strcmp(qos_node->port_name, name)) { > > > > > + return qos_node; > > > > > + } > > > > > + } > > > > > + > > > > > + return NULL; > > > > > +} > > > > > + > > > > > static void > > > > > -get_qos_params(const struct sbrec_port_binding *pb, struct hmap *queue_map) > > > > > +update_qos_params(const struct sbrec_port_binding *pb, struct hmap *queue_map) > > > > > { > > > > > uint32_t min_rate = smap_get_int(&pb->options, "qos_min_rate", 0); > > > > > uint32_t max_rate = smap_get_int(&pb->options, "qos_max_rate", 0); > > > > > uint32_t burst = smap_get_int(&pb->options, "qos_burst", 0); > > > > > uint32_t queue_id = smap_get_int(&pb->options, "qdisc_queue_id", 0); > > > > > > > > > > + struct qos_queue *node = get_qos_map_entry(queue_map, pb->logical_port); > > > > > + > > > > > if ((!min_rate && !max_rate && !burst) || !queue_id) { > > > > > /* Qos is not configured for this port. */ > > > > > + if (node) { > > > > > + hmap_remove(queue_map, &node->node); > > > > > + free(node->port_name); > > > > > + free(node); > > > > > + } > > > > > return; > > > > > } > > > > > > > > > > - struct qos_queue *node = xzalloc(sizeof *node); > > > > > - hmap_insert(queue_map, &node->node, hash_int(queue_id, 0)); > > > > > + if (!node) { > > > > > + node = xzalloc(sizeof *node); > > > > > + hmap_insert(queue_map, &node->node, hash_int(queue_id, 0)); > > > > > + node->port_name = xstrdup(pb->logical_port); > > > > > + } > > > > > + node->queue_id = queue_id; > > > > > node->min_rate = min_rate; > > > > > node->max_rate = max_rate; > > > > > node->burst = burst; > > > > > - node->queue_id = queue_id; > > > > > } > > > > > > > > > > static const struct ovsrec_qos * > > > > > @@ -191,7 +217,7 @@ static bool > > > > > set_noop_qos(struct ovsdb_idl_txn *ovs_idl_txn, > > > > > const struct ovsrec_port_table *port_table, > > > > > const struct ovsrec_qos_table *qos_table, > > > > > - struct sset *egress_ifaces) > > > > > + struct shash *egress_ifaces) > > > > > { > > > > > if (!ovs_idl_txn) { > > > > > return false; > > > > > @@ -206,11 +232,11 @@ set_noop_qos(struct ovsdb_idl_txn *ovs_idl_txn, > > > > > size_t count = 0; > > > > > > > > > > OVSREC_PORT_TABLE_FOR_EACH (port, port_table) { > > > > > - if (sset_contains(egress_ifaces, port->name)) { > > > > > + if (shash_find(egress_ifaces, port->name)) { > > > > > ovsrec_port_set_qos(port, noop_qos); > > > > > count++; > > > > > } > > > > > - if (sset_count(egress_ifaces) == count) { > > > > > + if (shash_count(egress_ifaces) == count) { > > > > > break; > > > > > } > > > > > } > > > > > @@ -236,7 +262,8 @@ set_qos_type(struct netdev *netdev, const char *type) > > > > > } > > > > > > > > > > static void > > > > > -setup_qos(const char *egress_iface, struct hmap *queue_map) > > > > > +setup_qos(const char *egress_iface, const char *logical_port, > > > > > + struct hmap *queue_map) > > > > > { > > > > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); > > > > > struct netdev *netdev_phy; > > > > > @@ -281,7 +308,7 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) > > > > > * a configuration setting. > > > > > * > > > > > * - Otherwise leave the qdisc alone. */ > > > > > - if (hmap_is_empty(queue_map)) { > > > > > + if (!get_qos_map_entry(queue_map, logical_port)) { > > > > > if (!strcmp(qdisc_type, OVN_QOS_TYPE)) { > > > > > set_qos_type(netdev_phy, ""); > > > > > } > > > > > @@ -338,6 +365,10 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) > > > > > continue; > > > > > } > > > > > > > > > > + if (strcmp(sb_info->port_name, logical_port)) { > > > > > + continue; > > > > > + } > > > > > + > > > > > smap_clear(&queue_details); > > > > > smap_add_format(&queue_details, "min-rate", "%d", sb_info->min_rate); > > > > > smap_add_format(&queue_details, "max-rate", "%d", sb_info->max_rate); > > > > > @@ -354,11 +385,12 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) > > > > > netdev_close(netdev_phy); > > > > > } > > > > > > > > > > -static void > > > > > +void > > > > > destroy_qos_map(struct hmap *qos_map) > > > > > { > > > > > struct qos_queue *qos_queue; > > > > > HMAP_FOR_EACH_POP (qos_queue, node, qos_map) { > > > > > + free(qos_queue->port_name); > > > > > free(qos_queue); > > > > > } > > > > > > > > > > @@ -404,7 +436,7 @@ sbrec_get_port_encap(const struct sbrec_chassis *chassis_rec, > > > > > static void > > > > > add_localnet_egress_interface_mappings( > > > > > const struct sbrec_port_binding *port_binding, > > > > > - struct shash *bridge_mappings, struct sset *egress_ifaces) > > > > > + struct shash *bridge_mappings, struct shash *egress_ifaces) > > > > > { > > > > > const char *network = smap_get(&port_binding->options, "network_name"); > > > > > if (!network) { > > > > > @@ -429,7 +461,8 @@ add_localnet_egress_interface_mappings( > > > > > if (!is_egress_iface) { > > > > > continue; > > > > > } > > > > > - sset_add(egress_ifaces, iface_rec->name); > > > > > + shash_add(egress_ifaces, iface_rec->name, > > > > > + port_binding->logical_port); > > > > > } > > > > > } > > > > > } > > > > > @@ -474,7 +507,7 @@ update_ld_multichassis_ports(const struct sbrec_port_binding *binding_rec, > > > > > static void > > > > > update_ld_localnet_port(const struct sbrec_port_binding *binding_rec, > > > > > struct shash *bridge_mappings, > > > > > - struct sset *egress_ifaces, > > > > > + struct shash *egress_ifaces, > > > > > struct hmap *local_datapaths) > > > > > { > > > > > /* Ignore localnet ports for unplugged networks. */ > > > > > @@ -1456,7 +1489,7 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, > > > > > b_ctx_out->tracked_dp_bindings); > > > > > } > > > > > if (b_lport->lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) { > > > > > - get_qos_params(pb, qos_map); > > > > > + update_qos_params(pb, qos_map); > > > > > } > > > > > } else { > > > > > /* We could, but can't claim the lport. */ > > > > > @@ -1519,6 +1552,16 @@ consider_vif_lport(const struct sbrec_port_binding *pb, > > > > > b_lport = local_binding_add_lport(binding_lports, lbinding, pb, > > > > > LP_VIF); > > > > > } > > > > > + > > > > > + if (lbinding->iface && > > > > > + smap_get(&lbinding->iface->external_ids, "ovn-egress-iface")) { > > > > > + const char *iface_id = smap_get(&lbinding->iface->external_ids, > > > > > + "iface-id"); > > > > > + if (iface_id) { > > > > > + shash_add(b_ctx_out->egress_ifaces, lbinding->iface->name, > > > > > + iface_id); > > > > > > > > When a qos is configured on a logical port, this patch is adding 2 > > > > entries in the 'egress_ifaces' shash for the same logical port. > > > > I think it's because of the above 'shash_add'. I think you should use > > > > shash_replace instead. > > > > > > ack, I will fix it. > > > > > > > > > > > When a full recompute happens, the function build_local_bindings() > > > > also adds the qos configured logical ports to the 'egress_ifaces' > > > > and later if there are any updates to the logical port, this function > > > > - consider_vif_lport() also adds it to the shash. > > > > > > > > IMO the qos support in OVN needs a different approach. Instead of > > > > configuring the qos using netdev() perhaps we should rely on the OVS > > > > meters. > > > > Maybe this was brought up earlier too. > > > > > > > > > > > I'm of the opinion that instead of supporting Qos for logical ports > > > > using netdev, we should use OVS meters (not just for logical ports, > > > > even for localnet ports). Thoughts ? > > > > > > ovn qos is used for egress traffic (shaping) while ovn meters are used > > > for incoming one (policing). > > > What we can do is to avoid configuring directly netdev qdisc and rely on > > > ovs to perform this configuration but this would be quite a massive rework. > > > Do you think we should do it now before adding this feature or it is ok > > > to do it after we added this feature? I do not have a strong opinion about it. > > > > IMO its better to add this new feature using the native OVS QoS support. > > And along with that we can remove the netdev implementation from OVN. > > > > This is what I think. Anyway this feature is out of window for 22.12. > > We can perhaps target this feature with the new approach in 23.03. > > > > Although I will not object if other maintainers are fine getting this > > feature first > > and then reimplement using OVS native QoS support. > > I am fine to work on a proper solution for lsp and then move localnet port too. > > Regards, > Lorenzo > > > > > Thanks > > Numan > > > > > > > > Regards, > > > Lorenzo > > > > > > > > > > > @Dumitru Ceara @Han Zhou @Mark Michelson @Ilya Maximets Do you have > > > > any comments or suggestions here ? > > > > > > > > Thanks > > > > Numan > > > > > > > > > > > > > > > > > + } > > > > > + } > > > > > } > > > > > > > > > > return consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out, > > > > > @@ -1785,7 +1828,7 @@ consider_localnet_lport(const struct sbrec_port_binding *pb, > > > > > update_local_lports(pb->logical_port, b_ctx_out); > > > > > > > > > > if (qos_map && b_ctx_in->ovs_idl_txn) { > > > > > - get_qos_params(pb, qos_map); > > > > > + update_qos_params(pb, qos_map); > > > > > } > > > > > > > > > > update_related_lport(pb, b_ctx_out); > > > > > @@ -1861,14 +1904,14 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in, > > > > > &b_ctx_out->lbinding_data->bindings; > > > > > for (j = 0; j < port_rec->n_interfaces; j++) { > > > > > const struct ovsrec_interface *iface_rec; > > > > > + struct local_binding *lbinding = NULL; > > > > > > > > > > iface_rec = port_rec->interfaces[j]; > > > > > iface_id = smap_get(&iface_rec->external_ids, "iface-id"); > > > > > int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; > > > > > > > > > > if (iface_id && ofport > 0) { > > > > > - struct local_binding *lbinding = > > > > > - local_binding_find(local_bindings, iface_id); > > > > > + lbinding = local_binding_find(local_bindings, iface_id); > > > > > if (!lbinding) { > > > > > lbinding = local_binding_create(iface_id, iface_rec); > > > > > local_binding_add(local_bindings, lbinding); > > > > > @@ -1895,8 +1938,11 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in, > > > > > const char *tunnel_iface > > > > > = smap_get(&iface_rec->status, "tunnel_egress_iface"); > > > > > if (tunnel_iface) { > > > > > - sset_add(b_ctx_out->egress_ifaces, tunnel_iface); > > > > > + shash_add(b_ctx_out->egress_ifaces, tunnel_iface, ""); > > > > > } > > > > > + } else if (lbinding && smap_get(&iface_rec->external_ids, > > > > > + "ovn-egress-iface")) { > > > > > + shash_add(b_ctx_out->egress_ifaces, iface_rec->name, iface_id); > > > > > } > > > > > } > > > > > } > > > > > @@ -1910,16 +1956,11 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > > > > > } > > > > > > > > > > struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings); > > > > > - struct hmap qos_map; > > > > > > > > > > - hmap_init(&qos_map); > > > > > if (b_ctx_in->br_int) { > > > > > build_local_bindings(b_ctx_in, b_ctx_out); > > > > > } > > > > > > > > > > - struct hmap *qos_map_ptr = > > > > > - !sset_is_empty(b_ctx_out->egress_ifaces) ? &qos_map : NULL; > > > > > - > > > > > struct ovs_list localnet_lports = OVS_LIST_INITIALIZER(&localnet_lports); > > > > > struct ovs_list external_lports = OVS_LIST_INITIALIZER(&external_lports); > > > > > struct ovs_list multichassis_ports = OVS_LIST_INITIALIZER( > > > > > @@ -1956,7 +1997,8 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > > > > > break; > > > > > > > > > > case LP_VIF: > > > > > - consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL, qos_map_ptr); > > > > > + consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL, > > > > > + b_ctx_out->qos_map); > > > > > if (pb->additional_chassis) { > > > > > struct lport *multichassis_lport = xmalloc( > > > > > sizeof *multichassis_lport); > > > > > @@ -1967,11 +2009,13 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > > > > > break; > > > > > > > > > > case LP_CONTAINER: > > > > > - consider_container_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); > > > > > + consider_container_lport(pb, b_ctx_in, b_ctx_out, > > > > > + b_ctx_out->qos_map); > > > > > break; > > > > > > > > > > case LP_VIRTUAL: > > > > > - consider_virtual_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); > > > > > + consider_virtual_lport(pb, b_ctx_in, b_ctx_out, > > > > > + b_ctx_out->qos_map); > > > > > break; > > > > > > > > > > case LP_L2GATEWAY: > > > > > @@ -1994,7 +2038,8 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > > > > > break; > > > > > > > > > > case LP_LOCALNET: { > > > > > - consider_localnet_lport(pb, b_ctx_in, b_ctx_out, &qos_map); > > > > > + consider_localnet_lport(pb, b_ctx_in, b_ctx_out, > > > > > + b_ctx_out->qos_map); > > > > > struct lport *lnet_lport = xmalloc(sizeof *lnet_lport); > > > > > lnet_lport->pb = pb; > > > > > ovs_list_push_back(&localnet_lports, &lnet_lport->list_node); > > > > > @@ -2051,17 +2096,15 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > > > > > > > > > > shash_destroy(&bridge_mappings); > > > > > > > > > > - if (!sset_is_empty(b_ctx_out->egress_ifaces) > > > > > + if (!shash_is_empty(b_ctx_out->egress_ifaces) > > > > > && set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, > > > > > b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) { > > > > > - const char *entry; > > > > > - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > > > > - setup_qos(entry, &qos_map); > > > > > + struct shash_node *entry; > > > > > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > > > > + setup_qos(entry->name, entry->data, b_ctx_out->qos_map); > > > > > } > > > > > } > > > > > > > > > > - destroy_qos_map(&qos_map); > > > > > - > > > > > cleanup_claimed_port_timestamps(); > > > > > } > > > > > > > > > > @@ -2447,7 +2490,7 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, > > > > > } > > > > > > > > > > if (smap_get(&iface_rec->external_ids, "ovn-egress-iface") || > > > > > - sset_contains(b_ctx_out->egress_ifaces, iface_rec->name)) { > > > > > + shash_find(b_ctx_out->egress_ifaces, iface_rec->name)) { > > > > > handled = false; > > > > > break; > > > > > } > > > > > @@ -2493,10 +2536,6 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, > > > > > return false; > > > > > } > > > > > > > > > > - struct hmap qos_map = HMAP_INITIALIZER(&qos_map); > > > > > - struct hmap *qos_map_ptr = > > > > > - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; > > > > > - > > > > > /* > > > > > * We consider an OVS interface for claiming if the following > > > > > * 2 conditions are met: > > > > > @@ -2525,24 +2564,22 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, > > > > > if (iface_id && ofport > 0 && > > > > > is_iface_in_int_bridge(iface_rec, b_ctx_in->br_int)) { > > > > > handled = consider_iface_claim(iface_rec, iface_id, b_ctx_in, > > > > > - b_ctx_out, qos_map_ptr); > > > > > + b_ctx_out, b_ctx_out->qos_map); > > > > > if (!handled) { > > > > > break; > > > > > } > > > > > } > > > > > } > > > > > > > > > > - if (handled && qos_map_ptr && set_noop_qos(b_ctx_in->ovs_idl_txn, > > > > > - b_ctx_in->port_table, > > > > > - b_ctx_in->qos_table, > > > > > - b_ctx_out->egress_ifaces)) { > > > > > - const char *entry; > > > > > - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > > > > - setup_qos(entry, &qos_map); > > > > > + if (handled && > > > > > + set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, > > > > > + b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) { > > > > > + struct shash_node *entry; > > > > > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > > > > + setup_qos(entry->name, entry->data, b_ctx_out->qos_map); > > > > > } > > > > > } > > > > > > > > > > - destroy_qos_map(&qos_map); > > > > > return handled; > > > > > } > > > > > > > > > > @@ -2977,10 +3014,6 @@ delete_done: > > > > > return false; > > > > > } > > > > > > > > > > - struct hmap qos_map = HMAP_INITIALIZER(&qos_map); > > > > > - struct hmap *qos_map_ptr = > > > > > - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; > > > > > - > > > > > SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, > > > > > b_ctx_in->port_binding_table) { > > > > > /* Loop to handle create and update changes only. */ > > > > > @@ -2992,7 +3025,8 @@ delete_done: > > > > > update_ld_peers(pb, b_ctx_out->local_datapaths); > > > > > } > > > > > > > > > > - handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, qos_map_ptr); > > > > > + handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, > > > > > + b_ctx_out->qos_map); > > > > > if (!handled) { > > > > > break; > > > > > } > > > > > @@ -3009,7 +3043,8 @@ delete_done: > > > > > sset_find_and_delete(b_ctx_out->postponed_ports, port_name); > > > > > continue; > > > > > } > > > > > - handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, qos_map_ptr); > > > > > + handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, > > > > > + b_ctx_out->qos_map); > > > > > if (!handled) { > > > > > break; > > > > > } > > > > > @@ -3055,17 +3090,15 @@ delete_done: > > > > > shash_destroy(&bridge_mappings); > > > > > } > > > > > > > > > > - if (handled && qos_map_ptr && set_noop_qos(b_ctx_in->ovs_idl_txn, > > > > > - b_ctx_in->port_table, > > > > > - b_ctx_in->qos_table, > > > > > - b_ctx_out->egress_ifaces)) { > > > > > - const char *entry; > > > > > - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > > > > - setup_qos(entry, &qos_map); > > > > > + if (handled && > > > > > + set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, > > > > > + b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) { > > > > > + struct shash_node *entry; > > > > > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > > > > + setup_qos(entry->name, entry->data, b_ctx_out->qos_map); > > > > > } > > > > > } > > > > > > > > > > - destroy_qos_map(&qos_map); > > > > > return handled; > > > > > } > > > > > > > > > > diff --git a/controller/binding.h b/controller/binding.h > > > > > index 6c3a98b02..8be33eddb 100644 > > > > > --- a/controller/binding.h > > > > > +++ b/controller/binding.h > > > > > @@ -91,7 +91,8 @@ struct binding_ctx_out { > > > > > */ > > > > > bool non_vif_ports_changed; > > > > > > > > > > - struct sset *egress_ifaces; > > > > > + struct shash *egress_ifaces; > > > > > + struct hmap *qos_map; > > > > > /* smap of OVS interface name as key and > > > > > * OVS interface external_ids:iface-id as value. */ > > > > > struct smap *local_iface_ids; > > > > > @@ -195,6 +196,8 @@ void set_pb_chassis_in_sbrec(const struct sbrec_port_binding *pb, > > > > > const struct sbrec_chassis *chassis_rec, > > > > > bool is_set); > > > > > > > > > > +void destroy_qos_map(struct hmap *qos_map); > > > > > + > > > > > /* Corresponds to each Port_Binding.type. */ > > > > > enum en_lport_type { > > > > > LP_UNKNOWN, > > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > > > > index d6251afb8..066d76869 100644 > > > > > --- a/controller/ovn-controller.c > > > > > +++ b/controller/ovn-controller.c > > > > > @@ -1276,7 +1276,8 @@ struct ed_type_runtime_data { > > > > > struct sset active_tunnels; > > > > > > > > > > /* runtime data engine private data. */ > > > > > - struct sset egress_ifaces; > > > > > + struct shash egress_ifaces; > > > > > + struct hmap qos_map; > > > > > struct smap local_iface_ids; > > > > > > > > > > /* Tracked data. See below for more details and comments. */ > > > > > @@ -1372,7 +1373,8 @@ en_runtime_data_init(struct engine_node *node OVS_UNUSED, > > > > > sset_init(&data->local_lports); > > > > > related_lports_init(&data->related_lports); > > > > > sset_init(&data->active_tunnels); > > > > > - sset_init(&data->egress_ifaces); > > > > > + shash_init(&data->egress_ifaces); > > > > > + hmap_init(&data->qos_map); > > > > > smap_init(&data->local_iface_ids); > > > > > local_binding_data_init(&data->lbinding_data); > > > > > shash_init(&data->local_active_ports_ipv6_pd); > > > > > @@ -1392,7 +1394,8 @@ en_runtime_data_cleanup(void *data) > > > > > sset_destroy(&rt_data->local_lports); > > > > > related_lports_destroy(&rt_data->related_lports); > > > > > sset_destroy(&rt_data->active_tunnels); > > > > > - sset_destroy(&rt_data->egress_ifaces); > > > > > + shash_destroy(&rt_data->egress_ifaces); > > > > > + destroy_qos_map(&rt_data->qos_map); > > > > > smap_destroy(&rt_data->local_iface_ids); > > > > > local_datapaths_destroy(&rt_data->local_datapaths); > > > > > shash_destroy(&rt_data->local_active_ports_ipv6_pd); > > > > > @@ -1481,6 +1484,7 @@ init_binding_ctx(struct engine_node *node, > > > > > b_ctx_out->related_lports_changed = false; > > > > > b_ctx_out->non_vif_ports_changed = false; > > > > > b_ctx_out->egress_ifaces = &rt_data->egress_ifaces; > > > > > + b_ctx_out->qos_map = &rt_data->qos_map; > > > > > b_ctx_out->lbinding_data = &rt_data->lbinding_data; > > > > > b_ctx_out->local_iface_ids = &rt_data->local_iface_ids; > > > > > b_ctx_out->postponed_ports = rt_data->postponed_ports; > > > > > @@ -1510,13 +1514,14 @@ en_runtime_data_run(struct engine_node *node, void *data) > > > > > sset_destroy(local_lports); > > > > > related_lports_destroy(&rt_data->related_lports); > > > > > sset_destroy(active_tunnels); > > > > > - sset_destroy(&rt_data->egress_ifaces); > > > > > + shash_clear(&rt_data->egress_ifaces); > > > > > + destroy_qos_map(&rt_data->qos_map); > > > > > + hmap_init(&rt_data->qos_map); > > > > > smap_destroy(&rt_data->local_iface_ids); > > > > > hmap_init(local_datapaths); > > > > > sset_init(local_lports); > > > > > related_lports_init(&rt_data->related_lports); > > > > > sset_init(active_tunnels); > > > > > - sset_init(&rt_data->egress_ifaces); > > > > > smap_init(&rt_data->local_iface_ids); > > > > > local_binding_data_init(&rt_data->lbinding_data); > > > > > } > > > > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > > > > index 3e904c9dc..c986198a2 100644 > > > > > --- a/tests/system-ovn.at > > > > > +++ b/tests/system-ovn.at > > > > > @@ -6335,6 +6335,10 @@ ADD_NAMESPACES(sw01) > > > > > ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03") > > > > > ovn-nbctl lsp-add sw0 sw01 \ > > > > > -- lsp-set-addresses sw01 "f0:00:00:01:02:03 192.168.1.2" > > > > > +ADD_NAMESPACES(sw02) > > > > > +ADD_VETH(sw02, sw02, br-int, "192.168.1.3/24", "f2:00:00:01:02:03") > > > > > +ovn-nbctl lsp-add sw0 sw02 \ > > > > > + -- lsp-set-addresses sw02 "f2:00:00:01:02:03 192.168.1.3" > > > > > > > > > > ADD_NAMESPACES(public) > > > > > ADD_VETH(public, public, br-ext, "192.168.2.2/24", "f0:00:00:01:02:05") > > > > > @@ -6345,6 +6349,7 @@ ovn-nbctl lsp-add sw0 public \ > > > > > -- lsp-set-type public localnet \ > > > > > -- lsp-set-options public network_name=phynet > > > > > > > > > > +# Setup QoS on a localnet port > > > > > AT_CHECK([ovn-nbctl set Logical_Switch_Port public options:qos_min_rate=200000]) > > > > > AT_CHECK([ovn-nbctl set Logical_Switch_Port public options:qos_max_rate=300000]) > > > > > AT_CHECK([ovn-nbctl set Logical_Switch_Port public options:qos_burst=3000000]) > > > > > @@ -6353,15 +6358,58 @@ 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']) > > > > > > > > > > +# Setup QoS on a logical switch ports > > > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_min_rate=400000]) > > > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_max_rate=800000]) > > > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_burst=5000000]) > > > > > +AT_CHECK([ovs-vsctl set interface ovs-sw01 external-ids:ovn-egress-iface=true]) > > > > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01']) > > > > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \ > > > > > + grep -q 'class htb .* rate 400Kbit ceil 800Kbit burst 625000b cburst 625000b']) > > > > > + > > > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qos_min_rate=600000]) > > > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qos_max_rate=6000000]) > > > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qos_burst=6000000]) > > > > > +AT_CHECK([ovs-vsctl set interface ovs-sw02 external-ids:ovn-egress-iface=true]) > > > > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw02']) > > > > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw02 | \ > > > > > + grep -q 'class htb .* rate 600Kbit ceil 6Mbit burst 750000b cburst 750000b']) > > > > > + > > > > > +AT_CHECK([ovn-appctl -t ovn-controller exit --restart]) > > > > > +OVS_WAIT_UNTIL([test "$(pidof ovn-controller)" = ""]) > > > > > +start_daemon ovn-controller > > > > > +OVS_WAIT_UNTIL([test "$(pidof ovn-controller)" != ""]) > > > > > + > > > > > +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']) > > > > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01']) > > > > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \ > > > > > + grep -q 'class htb .* rate 400Kbit ceil 800Kbit burst 625000b cburst 625000b']) > > > > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw02']) > > > > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw02 | \ > > > > > + grep -q 'class htb .* rate 600Kbit ceil 6Mbit burst 750000b cburst 750000b']) > > > > > > > > > > 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_max_rate=300000]) > > > > > 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')" = ""]) > > > > > > > > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_min_rate=200000]) > > > > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01']) > > > > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \ > > > > > + grep -q 'class htb .* rate 200Kbit ceil 800Kbit burst 625000b cburst 625000b']) > > > > > + > > > > > +# Disable QoS rules from logical switch ports > > > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qdisc_queue_id=0]) > > > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qdisc_queue_id=0]) > > > > > +OVS_WAIT_UNTIL([test "$(tc qdisc show | grep 'htb 1: dev ovs-sw01')" = ""]) > > > > > +OVS_WAIT_UNTIL([test "$(tc qdisc show | grep 'htb 1: dev ovs-sw02')" = ""]) > > > > > + > > > > > kill $(pidof ovn-controller) > > > > > > > > > > as ovn-sb > > > > > -- > > > > > 2.38.1 > > > > > > > > > > _______________________________________________ > > > > > dev mailing list > > > > > dev@openvswitch.org > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Fri, Dec 9, 2022 at 8:37 AM Numan Siddique <numans@ovn.org> wrote: > > On Fri, Dec 9, 2022 at 9:43 AM Lorenzo Bianconi > <lorenzo.bianconi@redhat.com> wrote: > > > > > On Sun, Dec 4, 2022 at 5:06 PM Lorenzo Bianconi > > > <lorenzo.bianconi@redhat.com> wrote: > > > > > > > > Introduce the capability to apply QoS rules for logical switch ports > > > > claimed by ovn-controller. Rely on shash instead of sset for > > > > egress_ifaces. > > > > > > > > Acked-by: Mark Michelson <mmichels@redhat.com> > > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129742 > > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > > > > > Hi Lorenzo, > > > > Hi Numan, > > > > thx for the review :) > > > > > > > > Thanks for addressing the comments. > > > > > > I tested this version and compared with the present main. > > > > > > I see a few differences. > > > > > > With the main version, when I configure qos params to a localnet > > > logical port, ovn-controller creates a qdisc on the tunnel interface > > > which I guess is expected. > > > But I'm still not sure why we need to configure qdiscs on the tunnel > > > interface. But that's a story. Perhaps I need to see the original > > > QoS commits and understand why it was added. > > > > > > With your patch, I don't see that happening. Perhaps there is a bug > > > in setup_qos() function now that 'shash' is used to store the egress > > > ifaces with the logical port and > > > there is no logical port for the tunnel interfaces. > > > > uhm, I can look into this. Do you think we should keep this behaviour? > > Frankly I don't know. We need to find out why qos was configured in > the first place. > > I do agree with Ilya's comment that it is odd to configure qos on the > tunnel interface > and that could impact the other traffic. > > > > > > > > > > Regarding the option - "ovn-egress-iface". I suppose the expectation > > > from the CMS is to set the qos parameters in the logical port and also > > > set this option in the > > > ovs interface right ? I don't really see a need for this. I think if > > > CMS configures the qos parameters in the logical port options, > > > ovn-controller should configure the qos. > > > I think otherwise this would complicate the CMS implementation. Thoughts ? > > > > ovn-egress-iface is mandatory since it is used in setup_qos() to look for the > > proper netdevice pointer to configure ovs qdisc (please note it is used to > > identify ovs interfaces and not ovn ones). > > I think it can be doable (not sure about the complexity) to avoid it for > > logical switch ports (since we have iface-id in them external_ids column of > > ovs interface table) but I do not think we can avoid to set "ovn-egress-iface" > > for localnet port since afaik there is no direct link between ovn localnet port > > and ovs interface used to connect the underlay network, right? If so I guess > > it is better to keep it for both localnet and lsp ports. Do you agree? > > I think it's ok to expect CMS to set "ovn-egress-iface" for the > physical nic connected to the > provider bridge. But I don't think we should do the same for the > normal logical port VIFs. > > > > > > > > > > > > > > Please see a few comments below > > > > > > > --- > > > > Changes since v3: > > > > - fix typo in new system-ovn test > > > > Changes since v2: > > > > - fix qos configuration restarting ovn-controller > > > > Changes since v1: > > > > - improve ovs interface lookup > > > > - improve system-tests > > > > --- > > > > controller/binding.c | 155 ++++++++++++++++++++++-------------- > > > > controller/binding.h | 5 +- > > > > controller/ovn-controller.c | 15 ++-- > > > > tests/system-ovn.at | 48 +++++++++++ > > > > 4 files changed, 156 insertions(+), 67 deletions(-) > > > > > > > > diff --git a/controller/binding.c b/controller/binding.c > > > > index 5df62baef..53520263c 100644 > > > > --- a/controller/binding.c > > > > +++ b/controller/binding.c > > > > @@ -115,6 +115,7 @@ struct qos_queue { > > > > uint32_t min_rate; > > > > uint32_t max_rate; > > > > uint32_t burst; > > > > + char *port_name; > > > > }; > > > > > > > > void > > > > @@ -147,25 +148,50 @@ static void update_lport_tracking(const struct sbrec_port_binding *pb, > > > > struct hmap *tracked_dp_bindings, > > > > bool claimed); > > > > > > > > +static bool is_lport_vif(const struct sbrec_port_binding *pb); > > > > + > > > > +static struct qos_queue * > > > > +get_qos_map_entry(struct hmap *queue_map, const char *name) > > > > +{ > > > > + struct qos_queue *qos_node; > > > > + HMAP_FOR_EACH (qos_node, node, queue_map) { > > > > + if (!strcmp(qos_node->port_name, name)) { > > > > + return qos_node; > > > > + } > > > > + } > > > > + > > > > + return NULL; > > > > +} > > > > + > > > > static void > > > > -get_qos_params(const struct sbrec_port_binding *pb, struct hmap *queue_map) > > > > +update_qos_params(const struct sbrec_port_binding *pb, struct hmap *queue_map) > > > > { > > > > uint32_t min_rate = smap_get_int(&pb->options, "qos_min_rate", 0); > > > > uint32_t max_rate = smap_get_int(&pb->options, "qos_max_rate", 0); > > > > uint32_t burst = smap_get_int(&pb->options, "qos_burst", 0); > > > > uint32_t queue_id = smap_get_int(&pb->options, "qdisc_queue_id", 0); > > > > > > > > + struct qos_queue *node = get_qos_map_entry(queue_map, pb->logical_port); > > > > + > > > > if ((!min_rate && !max_rate && !burst) || !queue_id) { > > > > /* Qos is not configured for this port. */ > > > > + if (node) { > > > > + hmap_remove(queue_map, &node->node); > > > > + free(node->port_name); > > > > + free(node); > > > > + } > > > > return; > > > > } > > > > > > > > - struct qos_queue *node = xzalloc(sizeof *node); > > > > - hmap_insert(queue_map, &node->node, hash_int(queue_id, 0)); > > > > + if (!node) { > > > > + node = xzalloc(sizeof *node); > > > > + hmap_insert(queue_map, &node->node, hash_int(queue_id, 0)); > > > > + node->port_name = xstrdup(pb->logical_port); > > > > + } > > > > + node->queue_id = queue_id; > > > > node->min_rate = min_rate; > > > > node->max_rate = max_rate; > > > > node->burst = burst; > > > > - node->queue_id = queue_id; > > > > } > > > > > > > > static const struct ovsrec_qos * > > > > @@ -191,7 +217,7 @@ static bool > > > > set_noop_qos(struct ovsdb_idl_txn *ovs_idl_txn, > > > > const struct ovsrec_port_table *port_table, > > > > const struct ovsrec_qos_table *qos_table, > > > > - struct sset *egress_ifaces) > > > > + struct shash *egress_ifaces) > > > > { > > > > if (!ovs_idl_txn) { > > > > return false; > > > > @@ -206,11 +232,11 @@ set_noop_qos(struct ovsdb_idl_txn *ovs_idl_txn, > > > > size_t count = 0; > > > > > > > > OVSREC_PORT_TABLE_FOR_EACH (port, port_table) { > > > > - if (sset_contains(egress_ifaces, port->name)) { > > > > + if (shash_find(egress_ifaces, port->name)) { > > > > ovsrec_port_set_qos(port, noop_qos); > > > > count++; > > > > } > > > > - if (sset_count(egress_ifaces) == count) { > > > > + if (shash_count(egress_ifaces) == count) { > > > > break; > > > > } > > > > } > > > > @@ -236,7 +262,8 @@ set_qos_type(struct netdev *netdev, const char *type) > > > > } > > > > > > > > static void > > > > -setup_qos(const char *egress_iface, struct hmap *queue_map) > > > > +setup_qos(const char *egress_iface, const char *logical_port, > > > > + struct hmap *queue_map) > > > > { > > > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); > > > > struct netdev *netdev_phy; > > > > @@ -281,7 +308,7 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) > > > > * a configuration setting. > > > > * > > > > * - Otherwise leave the qdisc alone. */ > > > > - if (hmap_is_empty(queue_map)) { > > > > + if (!get_qos_map_entry(queue_map, logical_port)) { > > > > if (!strcmp(qdisc_type, OVN_QOS_TYPE)) { > > > > set_qos_type(netdev_phy, ""); > > > > } > > > > @@ -338,6 +365,10 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) > > > > continue; > > > > } > > > > > > > > + if (strcmp(sb_info->port_name, logical_port)) { > > > > + continue; > > > > + } > > > > + > > > > smap_clear(&queue_details); > > > > smap_add_format(&queue_details, "min-rate", "%d", sb_info->min_rate); > > > > smap_add_format(&queue_details, "max-rate", "%d", sb_info->max_rate); > > > > @@ -354,11 +385,12 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) > > > > netdev_close(netdev_phy); > > > > } > > > > > > > > -static void > > > > +void > > > > destroy_qos_map(struct hmap *qos_map) > > > > { > > > > struct qos_queue *qos_queue; > > > > HMAP_FOR_EACH_POP (qos_queue, node, qos_map) { > > > > + free(qos_queue->port_name); > > > > free(qos_queue); > > > > } > > > > > > > > @@ -404,7 +436,7 @@ sbrec_get_port_encap(const struct sbrec_chassis *chassis_rec, > > > > static void > > > > add_localnet_egress_interface_mappings( > > > > const struct sbrec_port_binding *port_binding, > > > > - struct shash *bridge_mappings, struct sset *egress_ifaces) > > > > + struct shash *bridge_mappings, struct shash *egress_ifaces) > > > > { > > > > const char *network = smap_get(&port_binding->options, "network_name"); > > > > if (!network) { > > > > @@ -429,7 +461,8 @@ add_localnet_egress_interface_mappings( > > > > if (!is_egress_iface) { > > > > continue; > > > > } > > > > - sset_add(egress_ifaces, iface_rec->name); > > > > + shash_add(egress_ifaces, iface_rec->name, > > > > + port_binding->logical_port); > > > > } > > > > } > > > > } > > > > @@ -474,7 +507,7 @@ update_ld_multichassis_ports(const struct sbrec_port_binding *binding_rec, > > > > static void > > > > update_ld_localnet_port(const struct sbrec_port_binding *binding_rec, > > > > struct shash *bridge_mappings, > > > > - struct sset *egress_ifaces, > > > > + struct shash *egress_ifaces, > > > > struct hmap *local_datapaths) > > > > { > > > > /* Ignore localnet ports for unplugged networks. */ > > > > @@ -1456,7 +1489,7 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, > > > > b_ctx_out->tracked_dp_bindings); > > > > } > > > > if (b_lport->lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) { > > > > - get_qos_params(pb, qos_map); > > > > + update_qos_params(pb, qos_map); > > > > } > > > > } else { > > > > /* We could, but can't claim the lport. */ > > > > @@ -1519,6 +1552,16 @@ consider_vif_lport(const struct sbrec_port_binding *pb, > > > > b_lport = local_binding_add_lport(binding_lports, lbinding, pb, > > > > LP_VIF); > > > > } > > > > + > > > > + if (lbinding->iface && > > > > + smap_get(&lbinding->iface->external_ids, "ovn-egress-iface")) { > > > > + const char *iface_id = smap_get(&lbinding->iface->external_ids, > > > > + "iface-id"); > > > > + if (iface_id) { > > > > + shash_add(b_ctx_out->egress_ifaces, lbinding->iface->name, > > > > + iface_id); > > > > > > When a qos is configured on a logical port, this patch is adding 2 > > > entries in the 'egress_ifaces' shash for the same logical port. > > > I think it's because of the above 'shash_add'. I think you should use > > > shash_replace instead. > > > > ack, I will fix it. > > > > > > > > When a full recompute happens, the function build_local_bindings() > > > also adds the qos configured logical ports to the 'egress_ifaces' > > > and later if there are any updates to the logical port, this function > > > - consider_vif_lport() also adds it to the shash. > > > > > > IMO the qos support in OVN needs a different approach. Instead of > > > configuring the qos using netdev() perhaps we should rely on the OVS > > > meters. > > > Maybe this was brought up earlier too. > > > > > > > > I'm of the opinion that instead of supporting Qos for logical ports > > > using netdev, we should use OVS meters (not just for logical ports, > > > even for localnet ports). Thoughts ? > > > > ovn qos is used for egress traffic (shaping) while ovn meters are used > > for incoming one (policing). > > What we can do is to avoid configuring directly netdev qdisc and rely on > > ovs to perform this configuration but this would be quite a massive rework. > > Do you think we should do it now before adding this feature or it is ok > > to do it after we added this feature? I do not have a strong opinion about it. > > IMO its better to add this new feature using the native OVS QoS support. > And along with that we can remove the netdev implementation from OVN. > > This is what I think. Anyway this feature is out of window for 22.12. > We can perhaps target this feature with the new approach in 23.03. > > Although I will not object if other maintainers are fine getting this > feature first > and then reimplement using OVS native QoS support. > I also agree with Ilya that OVN should configure OVS instead of netdev directly, but I am not familiar with the OVS native QoS support and not sure if there is a feature gap. The bugzilla was reported against VF representor port, but I think OVN should follow the same implementation if possible (i.e. configure OVS conf), and let the OVS offloading logic to figure out what to do. (In case there is any VF specific information that needs to be supplied for the VF representor to work, the VIF-plug mechanism may be utilized, which doesn't require any change in OVN code). I am also not familiar with how the current LSP qos options are implemented in OVN. I traced back to the below patch that added the qdisc related logic in 2016: a6095f815ed9 Check and allocate free qdisc queue id for ports with qos parameters And recently the below patch improved it by adding qos_min_rate support. dbf12e5fe1f7 qos: add support for port minimum bandwidth guarantee Thanks, Han
diff --git a/controller/binding.c b/controller/binding.c index 5df62baef..53520263c 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -115,6 +115,7 @@ struct qos_queue { uint32_t min_rate; uint32_t max_rate; uint32_t burst; + char *port_name; }; void @@ -147,25 +148,50 @@ static void update_lport_tracking(const struct sbrec_port_binding *pb, struct hmap *tracked_dp_bindings, bool claimed); +static bool is_lport_vif(const struct sbrec_port_binding *pb); + +static struct qos_queue * +get_qos_map_entry(struct hmap *queue_map, const char *name) +{ + struct qos_queue *qos_node; + HMAP_FOR_EACH (qos_node, node, queue_map) { + if (!strcmp(qos_node->port_name, name)) { + return qos_node; + } + } + + return NULL; +} + static void -get_qos_params(const struct sbrec_port_binding *pb, struct hmap *queue_map) +update_qos_params(const struct sbrec_port_binding *pb, struct hmap *queue_map) { uint32_t min_rate = smap_get_int(&pb->options, "qos_min_rate", 0); uint32_t max_rate = smap_get_int(&pb->options, "qos_max_rate", 0); uint32_t burst = smap_get_int(&pb->options, "qos_burst", 0); uint32_t queue_id = smap_get_int(&pb->options, "qdisc_queue_id", 0); + struct qos_queue *node = get_qos_map_entry(queue_map, pb->logical_port); + if ((!min_rate && !max_rate && !burst) || !queue_id) { /* Qos is not configured for this port. */ + if (node) { + hmap_remove(queue_map, &node->node); + free(node->port_name); + free(node); + } return; } - struct qos_queue *node = xzalloc(sizeof *node); - hmap_insert(queue_map, &node->node, hash_int(queue_id, 0)); + if (!node) { + node = xzalloc(sizeof *node); + hmap_insert(queue_map, &node->node, hash_int(queue_id, 0)); + node->port_name = xstrdup(pb->logical_port); + } + node->queue_id = queue_id; node->min_rate = min_rate; node->max_rate = max_rate; node->burst = burst; - node->queue_id = queue_id; } static const struct ovsrec_qos * @@ -191,7 +217,7 @@ static bool set_noop_qos(struct ovsdb_idl_txn *ovs_idl_txn, const struct ovsrec_port_table *port_table, const struct ovsrec_qos_table *qos_table, - struct sset *egress_ifaces) + struct shash *egress_ifaces) { if (!ovs_idl_txn) { return false; @@ -206,11 +232,11 @@ set_noop_qos(struct ovsdb_idl_txn *ovs_idl_txn, size_t count = 0; OVSREC_PORT_TABLE_FOR_EACH (port, port_table) { - if (sset_contains(egress_ifaces, port->name)) { + if (shash_find(egress_ifaces, port->name)) { ovsrec_port_set_qos(port, noop_qos); count++; } - if (sset_count(egress_ifaces) == count) { + if (shash_count(egress_ifaces) == count) { break; } } @@ -236,7 +262,8 @@ set_qos_type(struct netdev *netdev, const char *type) } static void -setup_qos(const char *egress_iface, struct hmap *queue_map) +setup_qos(const char *egress_iface, const char *logical_port, + struct hmap *queue_map) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); struct netdev *netdev_phy; @@ -281,7 +308,7 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) * a configuration setting. * * - Otherwise leave the qdisc alone. */ - if (hmap_is_empty(queue_map)) { + if (!get_qos_map_entry(queue_map, logical_port)) { if (!strcmp(qdisc_type, OVN_QOS_TYPE)) { set_qos_type(netdev_phy, ""); } @@ -338,6 +365,10 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) continue; } + if (strcmp(sb_info->port_name, logical_port)) { + continue; + } + smap_clear(&queue_details); smap_add_format(&queue_details, "min-rate", "%d", sb_info->min_rate); smap_add_format(&queue_details, "max-rate", "%d", sb_info->max_rate); @@ -354,11 +385,12 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) netdev_close(netdev_phy); } -static void +void destroy_qos_map(struct hmap *qos_map) { struct qos_queue *qos_queue; HMAP_FOR_EACH_POP (qos_queue, node, qos_map) { + free(qos_queue->port_name); free(qos_queue); } @@ -404,7 +436,7 @@ sbrec_get_port_encap(const struct sbrec_chassis *chassis_rec, static void add_localnet_egress_interface_mappings( const struct sbrec_port_binding *port_binding, - struct shash *bridge_mappings, struct sset *egress_ifaces) + struct shash *bridge_mappings, struct shash *egress_ifaces) { const char *network = smap_get(&port_binding->options, "network_name"); if (!network) { @@ -429,7 +461,8 @@ add_localnet_egress_interface_mappings( if (!is_egress_iface) { continue; } - sset_add(egress_ifaces, iface_rec->name); + shash_add(egress_ifaces, iface_rec->name, + port_binding->logical_port); } } } @@ -474,7 +507,7 @@ update_ld_multichassis_ports(const struct sbrec_port_binding *binding_rec, static void update_ld_localnet_port(const struct sbrec_port_binding *binding_rec, struct shash *bridge_mappings, - struct sset *egress_ifaces, + struct shash *egress_ifaces, struct hmap *local_datapaths) { /* Ignore localnet ports for unplugged networks. */ @@ -1456,7 +1489,7 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, b_ctx_out->tracked_dp_bindings); } if (b_lport->lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) { - get_qos_params(pb, qos_map); + update_qos_params(pb, qos_map); } } else { /* We could, but can't claim the lport. */ @@ -1519,6 +1552,16 @@ consider_vif_lport(const struct sbrec_port_binding *pb, b_lport = local_binding_add_lport(binding_lports, lbinding, pb, LP_VIF); } + + if (lbinding->iface && + smap_get(&lbinding->iface->external_ids, "ovn-egress-iface")) { + const char *iface_id = smap_get(&lbinding->iface->external_ids, + "iface-id"); + if (iface_id) { + shash_add(b_ctx_out->egress_ifaces, lbinding->iface->name, + iface_id); + } + } } return consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out, @@ -1785,7 +1828,7 @@ consider_localnet_lport(const struct sbrec_port_binding *pb, update_local_lports(pb->logical_port, b_ctx_out); if (qos_map && b_ctx_in->ovs_idl_txn) { - get_qos_params(pb, qos_map); + update_qos_params(pb, qos_map); } update_related_lport(pb, b_ctx_out); @@ -1861,14 +1904,14 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in, &b_ctx_out->lbinding_data->bindings; for (j = 0; j < port_rec->n_interfaces; j++) { const struct ovsrec_interface *iface_rec; + struct local_binding *lbinding = NULL; iface_rec = port_rec->interfaces[j]; iface_id = smap_get(&iface_rec->external_ids, "iface-id"); int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; if (iface_id && ofport > 0) { - struct local_binding *lbinding = - local_binding_find(local_bindings, iface_id); + lbinding = local_binding_find(local_bindings, iface_id); if (!lbinding) { lbinding = local_binding_create(iface_id, iface_rec); local_binding_add(local_bindings, lbinding); @@ -1895,8 +1938,11 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in, const char *tunnel_iface = smap_get(&iface_rec->status, "tunnel_egress_iface"); if (tunnel_iface) { - sset_add(b_ctx_out->egress_ifaces, tunnel_iface); + shash_add(b_ctx_out->egress_ifaces, tunnel_iface, ""); } + } else if (lbinding && smap_get(&iface_rec->external_ids, + "ovn-egress-iface")) { + shash_add(b_ctx_out->egress_ifaces, iface_rec->name, iface_id); } } } @@ -1910,16 +1956,11 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) } struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings); - struct hmap qos_map; - hmap_init(&qos_map); if (b_ctx_in->br_int) { build_local_bindings(b_ctx_in, b_ctx_out); } - struct hmap *qos_map_ptr = - !sset_is_empty(b_ctx_out->egress_ifaces) ? &qos_map : NULL; - struct ovs_list localnet_lports = OVS_LIST_INITIALIZER(&localnet_lports); struct ovs_list external_lports = OVS_LIST_INITIALIZER(&external_lports); struct ovs_list multichassis_ports = OVS_LIST_INITIALIZER( @@ -1956,7 +1997,8 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) break; case LP_VIF: - consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL, qos_map_ptr); + consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL, + b_ctx_out->qos_map); if (pb->additional_chassis) { struct lport *multichassis_lport = xmalloc( sizeof *multichassis_lport); @@ -1967,11 +2009,13 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) break; case LP_CONTAINER: - consider_container_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); + consider_container_lport(pb, b_ctx_in, b_ctx_out, + b_ctx_out->qos_map); break; case LP_VIRTUAL: - consider_virtual_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); + consider_virtual_lport(pb, b_ctx_in, b_ctx_out, + b_ctx_out->qos_map); break; case LP_L2GATEWAY: @@ -1994,7 +2038,8 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) break; case LP_LOCALNET: { - consider_localnet_lport(pb, b_ctx_in, b_ctx_out, &qos_map); + consider_localnet_lport(pb, b_ctx_in, b_ctx_out, + b_ctx_out->qos_map); struct lport *lnet_lport = xmalloc(sizeof *lnet_lport); lnet_lport->pb = pb; ovs_list_push_back(&localnet_lports, &lnet_lport->list_node); @@ -2051,17 +2096,15 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) shash_destroy(&bridge_mappings); - if (!sset_is_empty(b_ctx_out->egress_ifaces) + if (!shash_is_empty(b_ctx_out->egress_ifaces) && set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) { - const char *entry; - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { - setup_qos(entry, &qos_map); + struct shash_node *entry; + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) { + setup_qos(entry->name, entry->data, b_ctx_out->qos_map); } } - destroy_qos_map(&qos_map); - cleanup_claimed_port_timestamps(); } @@ -2447,7 +2490,7 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, } if (smap_get(&iface_rec->external_ids, "ovn-egress-iface") || - sset_contains(b_ctx_out->egress_ifaces, iface_rec->name)) { + shash_find(b_ctx_out->egress_ifaces, iface_rec->name)) { handled = false; break; } @@ -2493,10 +2536,6 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, return false; } - struct hmap qos_map = HMAP_INITIALIZER(&qos_map); - struct hmap *qos_map_ptr = - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; - /* * We consider an OVS interface for claiming if the following * 2 conditions are met: @@ -2525,24 +2564,22 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, if (iface_id && ofport > 0 && is_iface_in_int_bridge(iface_rec, b_ctx_in->br_int)) { handled = consider_iface_claim(iface_rec, iface_id, b_ctx_in, - b_ctx_out, qos_map_ptr); + b_ctx_out, b_ctx_out->qos_map); if (!handled) { break; } } } - if (handled && qos_map_ptr && set_noop_qos(b_ctx_in->ovs_idl_txn, - b_ctx_in->port_table, - b_ctx_in->qos_table, - b_ctx_out->egress_ifaces)) { - const char *entry; - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { - setup_qos(entry, &qos_map); + if (handled && + set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, + b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) { + struct shash_node *entry; + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) { + setup_qos(entry->name, entry->data, b_ctx_out->qos_map); } } - destroy_qos_map(&qos_map); return handled; } @@ -2977,10 +3014,6 @@ delete_done: return false; } - struct hmap qos_map = HMAP_INITIALIZER(&qos_map); - struct hmap *qos_map_ptr = - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; - SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, b_ctx_in->port_binding_table) { /* Loop to handle create and update changes only. */ @@ -2992,7 +3025,8 @@ delete_done: update_ld_peers(pb, b_ctx_out->local_datapaths); } - handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, qos_map_ptr); + handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, + b_ctx_out->qos_map); if (!handled) { break; } @@ -3009,7 +3043,8 @@ delete_done: sset_find_and_delete(b_ctx_out->postponed_ports, port_name); continue; } - handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, qos_map_ptr); + handled = handle_updated_port(b_ctx_in, b_ctx_out, pb, + b_ctx_out->qos_map); if (!handled) { break; } @@ -3055,17 +3090,15 @@ delete_done: shash_destroy(&bridge_mappings); } - if (handled && qos_map_ptr && set_noop_qos(b_ctx_in->ovs_idl_txn, - b_ctx_in->port_table, - b_ctx_in->qos_table, - b_ctx_out->egress_ifaces)) { - const char *entry; - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { - setup_qos(entry, &qos_map); + if (handled && + set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, + b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) { + struct shash_node *entry; + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) { + setup_qos(entry->name, entry->data, b_ctx_out->qos_map); } } - destroy_qos_map(&qos_map); return handled; } diff --git a/controller/binding.h b/controller/binding.h index 6c3a98b02..8be33eddb 100644 --- a/controller/binding.h +++ b/controller/binding.h @@ -91,7 +91,8 @@ struct binding_ctx_out { */ bool non_vif_ports_changed; - struct sset *egress_ifaces; + struct shash *egress_ifaces; + struct hmap *qos_map; /* smap of OVS interface name as key and * OVS interface external_ids:iface-id as value. */ struct smap *local_iface_ids; @@ -195,6 +196,8 @@ void set_pb_chassis_in_sbrec(const struct sbrec_port_binding *pb, const struct sbrec_chassis *chassis_rec, bool is_set); +void destroy_qos_map(struct hmap *qos_map); + /* Corresponds to each Port_Binding.type. */ enum en_lport_type { LP_UNKNOWN, diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index d6251afb8..066d76869 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1276,7 +1276,8 @@ struct ed_type_runtime_data { struct sset active_tunnels; /* runtime data engine private data. */ - struct sset egress_ifaces; + struct shash egress_ifaces; + struct hmap qos_map; struct smap local_iface_ids; /* Tracked data. See below for more details and comments. */ @@ -1372,7 +1373,8 @@ en_runtime_data_init(struct engine_node *node OVS_UNUSED, sset_init(&data->local_lports); related_lports_init(&data->related_lports); sset_init(&data->active_tunnels); - sset_init(&data->egress_ifaces); + shash_init(&data->egress_ifaces); + hmap_init(&data->qos_map); smap_init(&data->local_iface_ids); local_binding_data_init(&data->lbinding_data); shash_init(&data->local_active_ports_ipv6_pd); @@ -1392,7 +1394,8 @@ en_runtime_data_cleanup(void *data) sset_destroy(&rt_data->local_lports); related_lports_destroy(&rt_data->related_lports); sset_destroy(&rt_data->active_tunnels); - sset_destroy(&rt_data->egress_ifaces); + shash_destroy(&rt_data->egress_ifaces); + destroy_qos_map(&rt_data->qos_map); smap_destroy(&rt_data->local_iface_ids); local_datapaths_destroy(&rt_data->local_datapaths); shash_destroy(&rt_data->local_active_ports_ipv6_pd); @@ -1481,6 +1484,7 @@ init_binding_ctx(struct engine_node *node, b_ctx_out->related_lports_changed = false; b_ctx_out->non_vif_ports_changed = false; b_ctx_out->egress_ifaces = &rt_data->egress_ifaces; + b_ctx_out->qos_map = &rt_data->qos_map; b_ctx_out->lbinding_data = &rt_data->lbinding_data; b_ctx_out->local_iface_ids = &rt_data->local_iface_ids; b_ctx_out->postponed_ports = rt_data->postponed_ports; @@ -1510,13 +1514,14 @@ en_runtime_data_run(struct engine_node *node, void *data) sset_destroy(local_lports); related_lports_destroy(&rt_data->related_lports); sset_destroy(active_tunnels); - sset_destroy(&rt_data->egress_ifaces); + shash_clear(&rt_data->egress_ifaces); + destroy_qos_map(&rt_data->qos_map); + hmap_init(&rt_data->qos_map); smap_destroy(&rt_data->local_iface_ids); hmap_init(local_datapaths); sset_init(local_lports); related_lports_init(&rt_data->related_lports); sset_init(active_tunnels); - sset_init(&rt_data->egress_ifaces); smap_init(&rt_data->local_iface_ids); local_binding_data_init(&rt_data->lbinding_data); } diff --git a/tests/system-ovn.at b/tests/system-ovn.at index 3e904c9dc..c986198a2 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -6335,6 +6335,10 @@ ADD_NAMESPACES(sw01) ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03") ovn-nbctl lsp-add sw0 sw01 \ -- lsp-set-addresses sw01 "f0:00:00:01:02:03 192.168.1.2" +ADD_NAMESPACES(sw02) +ADD_VETH(sw02, sw02, br-int, "192.168.1.3/24", "f2:00:00:01:02:03") +ovn-nbctl lsp-add sw0 sw02 \ + -- lsp-set-addresses sw02 "f2:00:00:01:02:03 192.168.1.3" ADD_NAMESPACES(public) ADD_VETH(public, public, br-ext, "192.168.2.2/24", "f0:00:00:01:02:05") @@ -6345,6 +6349,7 @@ ovn-nbctl lsp-add sw0 public \ -- lsp-set-type public localnet \ -- lsp-set-options public network_name=phynet +# Setup QoS on a localnet port AT_CHECK([ovn-nbctl set Logical_Switch_Port public options:qos_min_rate=200000]) AT_CHECK([ovn-nbctl set Logical_Switch_Port public options:qos_max_rate=300000]) AT_CHECK([ovn-nbctl set Logical_Switch_Port public options:qos_burst=3000000]) @@ -6353,15 +6358,58 @@ 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']) +# Setup QoS on a logical switch ports +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_min_rate=400000]) +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_max_rate=800000]) +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_burst=5000000]) +AT_CHECK([ovs-vsctl set interface ovs-sw01 external-ids:ovn-egress-iface=true]) +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01']) +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \ + grep -q 'class htb .* rate 400Kbit ceil 800Kbit burst 625000b cburst 625000b']) + +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qos_min_rate=600000]) +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qos_max_rate=6000000]) +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qos_burst=6000000]) +AT_CHECK([ovs-vsctl set interface ovs-sw02 external-ids:ovn-egress-iface=true]) +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw02']) +OVS_WAIT_UNTIL([tc class show dev ovs-sw02 | \ + grep -q 'class htb .* rate 600Kbit ceil 6Mbit burst 750000b cburst 750000b']) + +AT_CHECK([ovn-appctl -t ovn-controller exit --restart]) +OVS_WAIT_UNTIL([test "$(pidof ovn-controller)" = ""]) +start_daemon ovn-controller +OVS_WAIT_UNTIL([test "$(pidof ovn-controller)" != ""]) + +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']) +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01']) +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \ + grep -q 'class htb .* rate 400Kbit ceil 800Kbit burst 625000b cburst 625000b']) +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw02']) +OVS_WAIT_UNTIL([tc class show dev ovs-sw02 | \ + grep -q 'class htb .* rate 600Kbit ceil 6Mbit burst 750000b cburst 750000b']) 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_max_rate=300000]) 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')" = ""]) +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qos_min_rate=200000]) +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01']) +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \ + grep -q 'class htb .* rate 200Kbit ceil 800Kbit burst 625000b cburst 625000b']) + +# Disable QoS rules from logical switch ports +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 options:qdisc_queue_id=0]) +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qdisc_queue_id=0]) +OVS_WAIT_UNTIL([test "$(tc qdisc show | grep 'htb 1: dev ovs-sw01')" = ""]) +OVS_WAIT_UNTIL([test "$(tc qdisc show | grep 'htb 1: dev ovs-sw02')" = ""]) + kill $(pidof ovn-controller) as ovn-sb