> On Mon, May 15, 2023 at 4:53 PM Lorenzo Bianconi > <[email protected]> wrote: > > > > > Hi Lorenzo, > > > > Hi Mark, > > > > > > > > This patch and patch 3 both have the same commit message (the top line is > > > different, but the rest is the same). This doesn't do a good job of > > > explaining the individual changes in patches 2 and 3. > > > > ack, I will fix it. > > Hi Lorenzo,
Hi Numan, > > I have a few comments (which applies to the whole series and not just > this patch). > > 1. I tested this patch series and found a crash. Below is the trace. > > [root@ovn-central ~]# /data/ovn-northd > 2023-05-15T23:38:47Z|00001|ovn_northd|INFO|OVN internal version is : > [23.03.90-20.27.2-70.6] > 2023-05-15T23:38:47Z|00002|reconnect|INFO|unix:/usr/local/var/run/ovn/ovnnb_db.sock: > connecting... > 2023-05-15T23:38:47Z|00003|reconnect|INFO|unix:/usr/local/var/run/ovn/ovnnb_db.sock: > connected > 2023-05-15T23:38:47Z|00004|ovn_northd|INFO|OVN NB IDL reconnected, > force recompute. > 2023-05-15T23:38:47Z|00005|reconnect|INFO|unix:/usr/local/var/run/ovn/ovnsb_db.sock: > connecting... > 2023-05-15T23:38:47Z|00006|reconnect|INFO|unix:/usr/local/var/run/ovn/ovnsb_db.sock: > connected > 2023-05-15T23:38:47Z|00007|ovn_northd|INFO|OVN SB IDL reconnected, > force recompute. > 2023-05-15T23:38:47Z|00008|ovn_northd|INFO|ovn-northd lock acquired. > This ovn-northd instance is now active. > AddressSanitizer:DEADLYSIGNAL > ================================================================= > ==288==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 > (pc 0x7f019742303d bp 0x7ffd92aae9a0 sp 0x7ffd92aae148 T0) > ==288==The signal is caused by a READ memory access. > ==288==Hint: address points to the zero page. > #0 0x7f019742303d in __strlen_avx2 (/lib64/libc.so.6+0x15603d) > #1 0x7f0197c1336c in __interceptor_strlen.part.0 > (/lib64/libasan.so.8+0x4c36c) > #2 0x624a6d in xstrdup lib/util.c:205 > #3 0x61a862 in smap_add lib/smap.c:55 > #4 0x4224ed in ovn_port_update_sbrec ../northd/northd.c:3514 > #5 0x42bf8f in build_ports ../northd/northd.c:4697 > #6 0x47878d in ovnnb_db_run ../northd/northd.c:16647 > #7 0x47a609 in northd_run ../northd/northd.c:16962 > #8 0x4805af in en_northd_run ../northd/en-northd.c:119 > #9 0x4b529f in engine_recompute ../lib/inc-proc-eng.c:415 > #10 0x4b5a16 in engine_run_node ../lib/inc-proc-eng.c:477 > #11 0x4b5c76 in engine_run ../lib/inc-proc-eng.c:528 > #12 0x4866c4 in inc_proc_northd_run ../northd/inc-proc-northd.c:283 > #13 0x47e189 in main ../northd/ovn-northd.c:920 > #14 0x7f01972f450f in __libc_start_call_main (/lib64/libc.so.6+0x2750f) > #15 0x7f01972f45c8 in __libc_start_main@GLIBC_2.2.5 > (/lib64/libc.so.6+0x275c8) > #16 0x4089d4 in _start (/data/ovn-northd+0x4089d4) > > I was able to reproduce this by setting the qos params on a logical > port except "network_name". ack, I will fix it. > > I think there will be a similar crash in ovn-controller (because of > patch 3 of this series) if ovn-controllers are updated before > ovn-northd. > > Please make sure to check for NULL ptr before calling xstrdup > > 2. As I had mentioned in the earlier email , ovn-northd needs to > properly allocate the queue id now that we support setting qos params > fo the normal VIF logical ports (with localnet port connected to the > logical switch) > Looks like ovn-northd allocates a queue id starting from 1 for > each chassis. Maybe as a simple fix, it can start from queue id 2. > But I'm not quite familiar with that part of the code. we need to keep queue_id allocation in northd since we need queue_id for the logical flow generation. I will look into it. > > > 3. Lets say a normal VIF logical port is configured with qos params. > When the ovn-controller releases this logical port (due to ovs > interface getting deleted or external_ids:iface-id of the ovs > interface getting cleared) > It doesn't delete the qos queue for this logical port. Triggering > a recompute fixes this issue. I think you need to call > remove_stale_qos_entry() in the function > binding_handle_ovs_interface_changes() or in consider_iface_release() > Please add some unit tests in ovn.at to cover this scenario. ack, I will fix it. > > 4. The test case "ovn-controller incremental processing" passes until > patch 3 of the series but starts failing in patch 4,5 and 6. It > passes again in patch 7 and onwards. > This is consistent in my local testing. Please make sure that it > passes for all the individual patches. ack, I will fix it. Regards, Lorenzo > > "ovn-controller incremental processing FAILED > (ovn-performance.at:562)" > > > Thanks > Numan > > > > > > > > Regards, > > Lorenzo > > > > > > > > On 5/15/23 06:03, Lorenzo Bianconi wrote: > > > > This is a preliminary patch to rework OVN QoS implementation in order to > > > > configure it through OVS QoS table instead of running tc command > > > > directly bypassing OVS. > > > > > > > > Signed-off-by: Lorenzo Bianconi <[email protected]> > > > > --- > > > > controller/binding.c | 201 ++++++++++++++++++------------------ > > > > controller/binding.h | 3 + > > > > controller/ovn-controller.c | 8 +- > > > > 3 files changed, 109 insertions(+), 103 deletions(-) > > > > > > > > diff --git a/controller/binding.c b/controller/binding.c > > > > index a0fbacc97..ad19a4092 100644 > > > > --- a/controller/binding.c > > > > +++ b/controller/binding.c > > > > @@ -109,14 +109,6 @@ binding_wait(void) > > > > } > > > > } > > > > -struct qos_queue { > > > > - struct hmap_node node; > > > > - uint32_t queue_id; > > > > - uint32_t min_rate; > > > > - uint32_t max_rate; > > > > - uint32_t burst; > > > > -}; > > > > - > > > > void > > > > binding_register_ovs_idl(struct ovsdb_idl *ovs_idl) > > > > { > > > > @@ -147,8 +139,48 @@ static void update_lport_tracking(const struct > > > > sbrec_port_binding *pb, > > > > struct hmap *tracked_dp_bindings, > > > > bool claimed); > > > > +struct qos_queue { > > > > + struct hmap_node node; > > > > + > > > > + char *port; > > > > + > > > > + uint32_t queue_id; > > > > + uint32_t min_rate; > > > > + uint32_t max_rate; > > > > + uint32_t burst; > > > > +}; > > > > + > > > > +static struct qos_queue * > > > > +find_qos_queue(struct hmap *queue_map, uint32_t hash, const char *port) > > > > +{ > > > > + struct qos_queue *q; > > > > + HMAP_FOR_EACH_WITH_HASH (q, node, hash, queue_map) { > > > > + if (!strcmp(q->port, port)) { > > > > + return q; > > > > + } > > > > + } > > > > + return NULL; > > > > +} > > > > + > > > > static void > > > > -get_qos_params(const struct sbrec_port_binding *pb, struct hmap > > > > *queue_map) > > > > +qos_queue_erase_entry(struct qos_queue *q) > > > > +{ > > > > + free(q->port); > > > > + free(q); > > > > +} > > > > + > > > > +void > > > > +destroy_qos_map(struct hmap *qos_map) > > > > +{ > > > > + struct qos_queue *q; > > > > + HMAP_FOR_EACH_POP (q, node, qos_map) { > > > > + qos_queue_erase_entry(q); > > > > + } > > > > + hmap_destroy(qos_map); > > > > +} > > > > + > > > > +static void > > > > +get_qos_queue(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); > > > > @@ -160,12 +192,17 @@ get_qos_params(const struct sbrec_port_binding > > > > *pb, struct hmap *queue_map) > > > > return; > > > > } > > > > - struct qos_queue *node = xzalloc(sizeof *node); > > > > - hmap_insert(queue_map, &node->node, hash_int(queue_id, 0)); > > > > - node->min_rate = min_rate; > > > > - node->max_rate = max_rate; > > > > - node->burst = burst; > > > > - node->queue_id = queue_id; > > > > + uint32_t hash = hash_string(pb->logical_port, 0); > > > > + struct qos_queue *q = find_qos_queue(queue_map, hash, > > > > pb->logical_port); > > > > + if (!q) { > > > > + q = xzalloc(sizeof *q); > > > > + hmap_insert(queue_map, &q->node, hash); > > > > + q->port = xstrdup(pb->logical_port); > > > > + q->queue_id = queue_id; > > > > + } > > > > + q->min_rate = min_rate; > > > > + q->max_rate = max_rate; > > > > + q->burst = burst; > > > > } > > > > static const struct ovsrec_qos * > > > > @@ -354,17 +391,6 @@ setup_qos(const char *egress_iface, struct hmap > > > > *queue_map) > > > > netdev_close(netdev_phy); > > > > } > > > > -static 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); > > > > - } > > > > - > > > > - hmap_destroy(qos_map); > > > > -} > > > > - > > > > /* > > > > * Get the encap from the chassis for this port. The interface > > > > * may have an external_ids:encap-ip=<encap-ip> set; if so we > > > > @@ -651,7 +677,7 @@ static struct binding_lport > > > > *local_binding_get_primary_or_localport_lport( > > > > static bool local_binding_handle_stale_binding_lports( > > > > struct local_binding *lbinding, struct binding_ctx_in *b_ctx_in, > > > > - struct binding_ctx_out *b_ctx_out, struct hmap *qos_map); > > > > + struct binding_ctx_out *b_ctx_out); > > > > static struct binding_lport *binding_lport_create( > > > > const struct sbrec_port_binding *, > > > > @@ -1469,8 +1495,7 @@ consider_vif_lport_(const struct > > > > sbrec_port_binding *pb, > > > > bool can_bind, > > > > struct binding_ctx_in *b_ctx_in, > > > > struct binding_ctx_out *b_ctx_out, > > > > - struct binding_lport *b_lport, > > > > - struct hmap *qos_map) > > > > + struct binding_lport *b_lport) > > > > { > > > > bool lbinding_set = b_lport && is_lbinding_set(b_lport->lbinding); > > > > @@ -1502,8 +1527,8 @@ consider_vif_lport_(const struct > > > > sbrec_port_binding *pb, > > > > tracked_datapath_lport_add(pb, > > > > TRACKED_RESOURCE_UPDATED, > > > > > > > > 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); > > > > + if (b_lport->lbinding->iface && b_ctx_in->ovs_idl_txn) { > > > > + get_qos_queue(pb, b_ctx_out->qos_map); > > > > } > > > > } else { > > > > /* We could, but can't claim the lport. */ > > > > @@ -1537,8 +1562,7 @@ static bool > > > > consider_vif_lport(const struct sbrec_port_binding *pb, > > > > struct binding_ctx_in *b_ctx_in, > > > > struct binding_ctx_out *b_ctx_out, > > > > - struct local_binding *lbinding, > > > > - struct hmap *qos_map) > > > > + struct local_binding *lbinding) > > > > { > > > > bool can_bind = > > > > lport_can_bind_on_this_chassis(b_ctx_in->chassis_rec, pb); > > > > @@ -1570,15 +1594,13 @@ consider_vif_lport(const struct > > > > sbrec_port_binding *pb, > > > > } > > > > } > > > > - return consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out, > > > > - b_lport, qos_map); > > > > + return consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out, > > > > b_lport); > > > > } > > > > static bool > > > > consider_container_lport(const struct sbrec_port_binding *pb, > > > > struct binding_ctx_in *b_ctx_in, > > > > - struct binding_ctx_out *b_ctx_out, > > > > - struct hmap *qos_map) > > > > + struct binding_ctx_out *b_ctx_out) > > > > { > > > > struct shash *local_bindings = > > > > &b_ctx_out->lbinding_data->bindings; > > > > struct local_binding *parent_lbinding; > > > > @@ -1627,7 +1649,7 @@ consider_container_lport(const struct > > > > sbrec_port_binding *pb, > > > > /* Its possible that the parent lport is not considered > > > > yet. > > > > * So call consider_vif_lport() to process it first. */ > > > > consider_vif_lport(parent_pb, b_ctx_in, b_ctx_out, > > > > - parent_lbinding, qos_map); > > > > + parent_lbinding); > > > > parent_b_lport = binding_lport_find(binding_lports, > > > > pb->parent_port); > > > > } else { > > > > @@ -1659,14 +1681,13 @@ consider_container_lport(const struct > > > > sbrec_port_binding *pb, > > > > bool can_bind = > > > > lport_can_bind_on_this_chassis(b_ctx_in->chassis_rec, pb); > > > > return consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out, > > > > - container_b_lport, qos_map); > > > > + container_b_lport); > > > > } > > > > static bool > > > > consider_virtual_lport(const struct sbrec_port_binding *pb, > > > > struct binding_ctx_in *b_ctx_in, > > > > - struct binding_ctx_out *b_ctx_out, > > > > - struct hmap *qos_map) > > > > + struct binding_ctx_out *b_ctx_out) > > > > { > > > > struct shash *local_bindings = > > > > &b_ctx_out->lbinding_data->bindings; > > > > struct local_binding *parent_lbinding = > > > > @@ -1694,7 +1715,7 @@ consider_virtual_lport(const struct > > > > sbrec_port_binding *pb, > > > > /* Its possible that the parent lport is not > > > > considered yet. > > > > * So call consider_vif_lport() to process it first. > > > > */ > > > > consider_vif_lport(parent_pb, b_ctx_in, b_ctx_out, > > > > - parent_lbinding, qos_map); > > > > + parent_lbinding); > > > > } > > > > } > > > > @@ -1708,7 +1729,7 @@ consider_virtual_lport(const struct > > > > sbrec_port_binding *pb, > > > > } > > > > if (!consider_vif_lport_(pb, true, b_ctx_in, b_ctx_out, > > > > - virtual_b_lport, qos_map)) { > > > > + virtual_b_lport)) { > > > > return false; > > > > } > > > > @@ -1826,15 +1847,14 @@ consider_l3gw_lport(const struct > > > > sbrec_port_binding *pb, > > > > static void > > > > consider_localnet_lport(const struct sbrec_port_binding *pb, > > > > struct binding_ctx_in *b_ctx_in, > > > > - struct binding_ctx_out *b_ctx_out, > > > > - struct hmap *qos_map) > > > > + struct binding_ctx_out *b_ctx_out) > > > > { > > > > /* Add all localnet ports to local_ifaces so that we allocate ct > > > > zones > > > > * for them. */ > > > > update_local_lports(pb->logical_port, b_ctx_out); > > > > - if (qos_map && b_ctx_in->ovs_idl_txn) { > > > > - get_qos_params(pb, qos_map); > > > > + if (b_ctx_in->ovs_idl_txn) { > > > > + get_qos_queue(pb, b_ctx_out->qos_map); > > > > } > > > > update_related_lport(pb, b_ctx_out); > > > > @@ -1950,16 +1970,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( > > > > @@ -1996,7 +2011,7 @@ 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); > > > > if (pb->additional_chassis) { > > > > struct lport *multichassis_lport = xmalloc( > > > > sizeof *multichassis_lport); > > > > @@ -2007,11 +2022,11 @@ 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); > > > > 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); > > > > break; > > > > case LP_L2GATEWAY: > > > > @@ -2034,7 +2049,7 @@ 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); > > > > struct lport *lnet_lport = xmalloc(sizeof *lnet_lport); > > > > lnet_lport->pb = pb; > > > > ovs_list_push_back(&localnet_lports, > > > > &lnet_lport->list_node); > > > > @@ -2096,12 +2111,10 @@ binding_run(struct binding_ctx_in *b_ctx_in, > > > > struct binding_ctx_out *b_ctx_out) > > > > 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); > > > > + setup_qos(entry, b_ctx_out->qos_map); > > > > } > > > > } > > > > - destroy_qos_map(&qos_map); > > > > - > > > > cleanup_claimed_port_timestamps(); > > > > } > > > > @@ -2190,8 +2203,7 @@ static bool > > > > consider_iface_claim(const struct ovsrec_interface *iface_rec, > > > > const char *iface_id, > > > > struct binding_ctx_in *b_ctx_in, > > > > - struct binding_ctx_out *b_ctx_out, > > > > - struct hmap *qos_map) > > > > + struct binding_ctx_out *b_ctx_out) > > > > { > > > > update_local_lports(iface_id, b_ctx_out); > > > > smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, > > > > iface_id); > > > > @@ -2239,7 +2251,7 @@ consider_iface_claim(const struct > > > > ovsrec_interface *iface_rec, > > > > } > > > > if (lport_type == LP_VIF && > > > > - !consider_vif_lport(pb, b_ctx_in, b_ctx_out, lbinding, > > > > qos_map)) { > > > > + !consider_vif_lport(pb, b_ctx_in, b_ctx_out, lbinding)) { > > > > return false; > > > > } > > > > @@ -2250,8 +2262,7 @@ consider_iface_claim(const struct > > > > ovsrec_interface *iface_rec, > > > > * claim the container lbindings. */ > > > > LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) { > > > > if (b_lport->type == LP_CONTAINER) { > > > > - if (!consider_container_lport(b_lport->pb, b_ctx_in, > > > > b_ctx_out, > > > > - qos_map)) { > > > > + if (!consider_container_lport(b_lport->pb, b_ctx_in, > > > > b_ctx_out)) { > > > > return false; > > > > } > > > > } > > > > @@ -2538,10 +2549,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: > > > > @@ -2570,24 +2577,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); > > > > 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)) { > > > > + 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)) { > > > > const char *entry; > > > > SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > > > - setup_qos(entry, &qos_map); > > > > + setup_qos(entry, b_ctx_out->qos_map); > > > > } > > > > } > > > > - destroy_qos_map(&qos_map); > > > > return handled; > > > > } > > > > @@ -2686,18 +2691,17 @@ static bool > > > > handle_updated_vif_lport(const struct sbrec_port_binding *pb, > > > > enum en_lport_type lport_type, > > > > struct binding_ctx_in *b_ctx_in, > > > > - struct binding_ctx_out *b_ctx_out, > > > > - struct hmap *qos_map) > > > > + struct binding_ctx_out *b_ctx_out) > > > > { > > > > bool claimed = (pb->chassis == b_ctx_in->chassis_rec); > > > > bool handled = true; > > > > if (lport_type == LP_VIRTUAL) { > > > > - handled = consider_virtual_lport(pb, b_ctx_in, b_ctx_out, > > > > qos_map); > > > > + handled = consider_virtual_lport(pb, b_ctx_in, b_ctx_out); > > > > } else if (lport_type == LP_CONTAINER) { > > > > - handled = consider_container_lport(pb, b_ctx_in, b_ctx_out, > > > > qos_map); > > > > + handled = consider_container_lport(pb, b_ctx_in, b_ctx_out); > > > > } else { > > > > - handled = consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL, > > > > qos_map); > > > > + handled = consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL); > > > > } > > > > if (!handled) { > > > > @@ -2727,7 +2731,7 @@ handle_updated_vif_lport(const struct > > > > sbrec_port_binding *pb, > > > > LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) { > > > > if (b_lport->type == LP_CONTAINER) { > > > > handled = consider_container_lport(b_lport->pb, b_ctx_in, > > > > - b_ctx_out, qos_map); > > > > + b_ctx_out); > > > > if (!handled) { > > > > return false; > > > > } > > > > @@ -2792,8 +2796,7 @@ consider_patch_port_for_local_datapaths(const > > > > struct sbrec_port_binding *pb, > > > > static bool > > > > handle_updated_port(struct binding_ctx_in *b_ctx_in, > > > > struct binding_ctx_out *b_ctx_out, > > > > - const struct sbrec_port_binding *pb, > > > > - struct hmap *qos_map_ptr) > > > > + const struct sbrec_port_binding *pb) > > > > { > > > > update_active_pb_ras_pd(pb, b_ctx_out->local_active_ports_ipv6_pd, > > > > "ipv6_prefix_delegation"); > > > > @@ -2815,7 +2818,7 @@ handle_updated_port(struct binding_ctx_in > > > > *b_ctx_in, > > > > if (b_lport->lbinding) { > > > > if (!local_binding_handle_stale_binding_lports( > > > > - b_lport->lbinding, b_ctx_in, b_ctx_out, > > > > qos_map_ptr)) { > > > > + b_lport->lbinding, b_ctx_in, b_ctx_out)) { > > > > return false; > > > > } > > > > } > > > > @@ -2829,7 +2832,7 @@ handle_updated_port(struct binding_ctx_in > > > > *b_ctx_in, > > > > case LP_VIRTUAL: > > > > update_ld_multichassis_ports(pb, b_ctx_out->local_datapaths); > > > > handled = handle_updated_vif_lport(pb, lport_type, b_ctx_in, > > > > - b_ctx_out, qos_map_ptr); > > > > + b_ctx_out); > > > > break; > > > > case LP_LOCALPORT: > > > > @@ -2889,7 +2892,7 @@ handle_updated_port(struct binding_ctx_in > > > > *b_ctx_in, > > > > break; > > > > case LP_LOCALNET: { > > > > - consider_localnet_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); > > > > + consider_localnet_lport(pb, b_ctx_in, b_ctx_out); > > > > struct shash bridge_mappings = > > > > SHASH_INITIALIZER(&bridge_mappings); > > > > @@ -3029,10 +3032,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. */ > > > > @@ -3044,7 +3043,7 @@ 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); > > > > if (!handled) { > > > > break; > > > > } > > > > @@ -3061,7 +3060,7 @@ 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); > > > > if (!handled) { > > > > break; > > > > } > > > > @@ -3107,17 +3106,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)) { > > > > + 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)) { > > > > const char *entry; > > > > SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > > > - setup_qos(entry, &qos_map); > > > > + setup_qos(entry, b_ctx_out->qos_map); > > > > } > > > > } > > > > - destroy_qos_map(&qos_map); > > > > return handled; > > > > } > > > > @@ -3265,8 +3262,7 @@ local_binding_add_lport(struct shash > > > > *binding_lports, > > > > static bool > > > > local_binding_handle_stale_binding_lports(struct local_binding > > > > *lbinding, > > > > struct binding_ctx_in > > > > *b_ctx_in, > > > > - struct binding_ctx_out > > > > *b_ctx_out, > > > > - struct hmap *qos_map) > > > > + struct binding_ctx_out > > > > *b_ctx_out) > > > > { > > > > /* Check if this lbinding has a primary binding_lport or > > > > * localport binding_lport or not. */ > > > > @@ -3288,14 +3284,15 @@ > > > > local_binding_handle_stale_binding_lports(struct local_binding > > > > *lbinding, > > > > pb = b_lport->pb; > > > > binding_lport_delete(&b_ctx_out->lbinding_data->lports, > > > > b_lport); > > > > - handled = consider_virtual_lport(pb, b_ctx_in, b_ctx_out, > > > > qos_map); > > > > + handled = consider_virtual_lport(pb, b_ctx_in, b_ctx_out); > > > > } else if (b_lport->type == LP_CONTAINER && > > > > pb_lport_type == LP_CONTAINER) { > > > > /* For container lport, binding_lport is preserved so > > > > that when > > > > * the parent port is created, it can be considered. > > > > * consider_container_lport() creates the binding_lport > > > > for the parent > > > > * port (with iface set to NULL). */ > > > > - handled = consider_container_lport(b_lport->pb, b_ctx_in, > > > > b_ctx_out, qos_map); > > > > + handled = consider_container_lport(b_lport->pb, b_ctx_in, > > > > + b_ctx_out); > > > > } else { > > > > /* This can happen when the lport type changes from one > > > > type > > > > * to another. Eg. from normal lport to external. > > > > Release the > > > > diff --git a/controller/binding.h b/controller/binding.h > > > > index 5b73c6a4b..87ee7b540 100644 > > > > --- a/controller/binding.h > > > > +++ b/controller/binding.h > > > > @@ -92,6 +92,7 @@ struct binding_ctx_out { > > > > bool non_vif_ports_changed; > > > > struct sset *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; > > > > @@ -260,4 +261,6 @@ void binding_wait(void); > > > > /* Clean up module state. */ > > > > void binding_destroy(void); > > > > +void destroy_qos_map(struct hmap *); > > > > + > > > > #endif /* controller/binding.h */ > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > > > index de90025f0..ee0e495ba 100644 > > > > --- a/controller/ovn-controller.c > > > > +++ b/controller/ovn-controller.c > > > > @@ -1341,6 +1341,7 @@ struct ed_type_runtime_data { > > > > /* runtime data engine private data. */ > > > > struct sset egress_ifaces; > > > > + struct hmap qos_map; > > > > struct smap local_iface_ids; > > > > /* Tracked data. See below for more details and comments. */ > > > > @@ -1400,7 +1401,7 @@ struct ed_type_runtime_data { > > > > * > > > > --------------------------------------------------------------------- > > > > * | local_iface_ids | This is used internally within the runtime > > > > data | > > > > * | egress_ifaces | engine (used only in binding.c) and hence > > > > there | > > > > - * | | there is no need to track. > > > > | > > > > + * | qos_map | there is no need to track. > > > > | > > > > * > > > > --------------------------------------------------------------------- > > > > * | | Active tunnels is built in the > > > > | > > > > * | | bfd_calculate_active_tunnels() for the tunnel > > > > | > > > > @@ -1437,6 +1438,7 @@ en_runtime_data_init(struct engine_node *node > > > > OVS_UNUSED, > > > > related_lports_init(&data->related_lports); > > > > sset_init(&data->active_tunnels); > > > > sset_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); > > > > @@ -1457,6 +1459,7 @@ en_runtime_data_cleanup(void *data) > > > > related_lports_destroy(&rt_data->related_lports); > > > > sset_destroy(&rt_data->active_tunnels); > > > > sset_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); > > > > @@ -1545,6 +1548,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; > > > > @@ -1575,12 +1579,14 @@ en_runtime_data_run(struct engine_node *node, > > > > void *data) > > > > related_lports_destroy(&rt_data->related_lports); > > > > sset_destroy(active_tunnels); > > > > sset_destroy(&rt_data->egress_ifaces); > > > > + destroy_qos_map(&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); > > > > + hmap_init(&rt_data->qos_map); > > > > smap_init(&rt_data->local_iface_ids); > > > > local_binding_data_init(&rt_data->lbinding_data); > > > > } > > > > > _______________________________________________ > > 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
