On Tue, Nov 22, 2022 at 5:31 PM Lorenzo Bianconi <[email protected]> wrote: > > > Thanks Lorenzo! > > > > Acked-by: Mark Michelson <[email protected]> > > > > The only question I have is why you converted to a shash instead of a smap? > > Hi Mark, > > Thx for the review. Do you mean shash instead of sset? > > Regards, > Lorenzo >
Thanks for the patch. Please see below for a few comments. This patch needs a rebase too. > > > > On 11/4/22 09:08, Lorenzo Bianconi 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. > > > > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129742 > > > Signed-off-by: Lorenzo Bianconi <[email protected]> > > > --- > > > controller/binding.c | 78 +++++++++++++++++++++++++++---------- > > > controller/binding.h | 2 +- > > > controller/ovn-controller.c | 9 ++--- > > > tests/system-ovn.at | 27 +++++++++++++ > > > 4 files changed, 89 insertions(+), 27 deletions(-) > > > > > > diff --git a/controller/binding.c b/controller/binding.c > > > index c3d2b2e42..6e596e6ca 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,6 +148,8 @@ 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 void > > > get_qos_params(const struct sbrec_port_binding *pb, struct hmap > > > *queue_map) > > > { > > > @@ -166,6 +169,7 @@ get_qos_params(const struct sbrec_port_binding *pb, > > > struct hmap *queue_map) > > > node->max_rate = max_rate; > > > node->burst = burst; > > > node->queue_id = queue_id; > > > + node->port_name = xstrdup(pb->logical_port); > > > } > > > static const struct ovsrec_qos * > > > @@ -191,7 +195,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 +210,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; > > > } > > > } > > > @@ -229,9 +233,10 @@ set_qos_type(struct netdev *netdev, const char *type) > > > } > > > static void > > > -setup_qos(const char *egress_iface, struct hmap *queue_map) > > > +setup_qos(struct shash_node *entry, struct hmap *queue_map) Instead of using shash_node as input parameter, I'd suggest changing it to 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); > > > + const char *egress_iface = entry->name; > > > struct netdev *netdev_phy; > > > if (!egress_iface) { > > > @@ -331,6 +336,10 @@ setup_qos(const char *egress_iface, struct hmap > > > *queue_map) > > > continue; > > > } > > > + if (strcmp(sb_info->port_name, entry->data)) { > > > + 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); > > > @@ -352,6 +361,7 @@ 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); > > > } > > > @@ -397,7 +407,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) { > > > @@ -422,7 +432,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); > > > } > > > } > > > } > > > @@ -467,7 +478,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. */ > > > @@ -1512,6 +1523,28 @@ consider_vif_lport(const struct sbrec_port_binding > > > *pb, > > > b_lport = local_binding_add_lport(binding_lports, lbinding, > > > pb, > > > LP_VIF); > > > } > > > + > > > + for (size_t i = 0; i < b_ctx_in->br_int->n_ports; i++) { > > > + const struct ovsrec_port *port_rec = > > > b_ctx_in->br_int->ports[i]; > > > + if (!strcmp(port_rec->name, b_ctx_in->br_int->name)) { > > > + continue; > > > + } > > > + > > > + for (size_t j = 0; j < port_rec->n_interfaces; j++) { > > > + const struct ovsrec_interface *iface_rec > > > + = port_rec->interfaces[j]; > > > + if (!smap_get(&iface_rec->external_ids, > > > "ovn-egress-iface")) { > > > + continue; > > > + } > > > + > > > + const char *iface_id = > > > + smap_get(&iface_rec->external_ids, "iface-id"); > > > + if (iface_id) { > > > + shash_add(b_ctx_out->egress_ifaces, iface_rec->name, > > > + iface_id); > > > + } > > > + } > > > + } > > > } I don't understand the above logic. The above for loop, loops through all the ovs ports of br-int, every time consider_vif_lport() is called. And this seems inefficient to me. consider_vif_lport() is called for each VIF/container/virutal port in binding_run() (when a full recompute happens) and when a port binding or an ovs interface change handler is called. Instead of the above loop, why can't this function just check if the lbinding->iface->external_ids has "ovn-egress-iface" set and if so add it to the b_ctx_out->egress_ifaces shash ? > > > return consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out, > > > @@ -1854,14 +1887,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); > > > @@ -1887,8 +1920,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,7 +1946,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct > > > binding_ctx_out *b_ctx_out) > > > } > > > struct hmap *qos_map_ptr = > > > - !sset_is_empty(b_ctx_out->egress_ifaces) ? &qos_map : NULL; > > > + !shash_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); > > > @@ -2043,11 +2079,11 @@ 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) { > > > + struct shash_node *entry; > > > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > > setup_qos(entry, &qos_map); > > > } > > > } > > > @@ -2405,7 +2441,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; > > > } > > > @@ -2453,7 +2489,7 @@ binding_handle_ovs_interface_changes(struct > > > binding_ctx_in *b_ctx_in, > > > struct hmap qos_map = HMAP_INITIALIZER(&qos_map); > > > struct hmap *qos_map_ptr = > > > - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; > > > + shash_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; > > > /* > > > * We consider an OVS interface for claiming if the following > > > @@ -2494,8 +2530,8 @@ binding_handle_ovs_interface_changes(struct > > > binding_ctx_in *b_ctx_in, > > > 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) { > > > + struct shash_node *entry; > > > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > > setup_qos(entry, &qos_map); > > > } > > > } > > > @@ -2937,7 +2973,7 @@ delete_done: > > > struct hmap qos_map = HMAP_INITIALIZER(&qos_map); > > > struct hmap *qos_map_ptr = > > > - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; > > > + shash_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; > > > SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, > > > > > > b_ctx_in->port_binding_table) { > > > @@ -3017,8 +3053,8 @@ delete_done: > > > 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) { > > > + struct shash_node *entry; > > > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > > setup_qos(entry, &qos_map); > > > } > > > } > > > diff --git a/controller/binding.h b/controller/binding.h > > > index ad959a9e6..9a348b5fb 100644 > > > --- a/controller/binding.h > > > +++ b/controller/binding.h > > > @@ -91,7 +91,7 @@ struct binding_ctx_out { > > > */ > > > bool non_vif_ports_changed; > > > - struct sset *egress_ifaces; > > > + struct shash *egress_ifaces; > > > /* smap of OVS interface name as key and > > > * OVS interface external_ids:iface-id as value. */ > > > struct smap *local_iface_ids; > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > > index 8895c7a2b..14cfeb480 100644 > > > --- a/controller/ovn-controller.c > > > +++ b/controller/ovn-controller.c > > > @@ -1237,7 +1237,7 @@ struct ed_type_runtime_data { > > > struct sset active_tunnels; > > > /* runtime data engine private data. */ > > > - struct sset egress_ifaces; > > > + struct shash egress_ifaces; > > > struct smap local_iface_ids; > > > /* Tracked data. See below for more details and comments. */ > > > @@ -1333,7 +1333,7 @@ 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); > > > smap_init(&data->local_iface_ids); > > > local_binding_data_init(&data->lbinding_data); > > > shash_init(&data->local_active_ports_ipv6_pd); > > > @@ -1353,7 +1353,7 @@ 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); > > > smap_destroy(&rt_data->local_iface_ids); > > > local_datapaths_destroy(&rt_data->local_datapaths); > > > shash_destroy(&rt_data->local_active_ports_ipv6_pd); > > > @@ -1471,13 +1471,12 @@ 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); > > > 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 20c058415..4a0ae73c3 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") > > > @@ -6353,11 +6357,34 @@ OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: > > > dev ovs-public']) > > > OVS_WAIT_UNTIL([tc class show dev ovs-public | \ > > > grep -q 'class htb .* rate 200Kbit ceil 300Kbit burst > > > 375000b cburst 375000b']) > > > +AT_CHECK([ovn-nbctl 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-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']) > > > + Can you please enhance the test case to remove external-ids:ovn-egress-iface=true from ovs-sw01/ovs-sw02 and make sure that ovn-controller clears the qdisc and other qos related stuff. Thanks Numan > > > kill $(pidof ovn-controller) > > > as ovn-sb > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
