Hi Han, On 9/15/20 8:40 PM, Han Zhou wrote: > When a lport is referenced by a logical flow where port-binding refs > needs to be added, currently it can add the same reference pair multiple > times in below situations (introduced in commit ade4e77): > > 1) In add_matches_to_flow_table(), different matches from same lflow > can have same inport/outport. > > 2) In is_chassis_resident_cb(), a lflow can have multiple is_chassis_resident > check for same lport (although not very common), and at the same time > the lport used in is_chassis_resident can overlap with the inport/ > outport of the same flow. > > Now because of the redundant entries added, it results in unexpected behavior > such as same lflow being processed multiple times as a waste of processing. > More severely, after commit 580aea72e it can result in orphaned pointer > leading > to crash, as reported in [0]. > > This patch fixes the problems by checking existance of same reference before
Nit: s/existance/existence > adding in lflow_resource_add(). To do this check efficiently, hmap is used to > replace the list struct for the resource-to-lflow index. > > [0] https://mail.openvswitch.org/pipermail/ovs-dev/2020-September/374991.html > > Reported-by: Dumitru Ceara <[email protected]> > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-dev/2020-September/374991.html > Fixes: ade4e779d3fb ("ovn-controller: Use the tracked runtime data changes > for flow calculation.") > Fixes: 580aea72e26f ("ovn-controller: Fix conjunction handling with > incremental processing.") > Signed-off-by: Han Zhou <[email protected]> > --- > v1 -> v2: > - Addressed Dumitru's comments. > - Moved the unrelated change to a separate patch in the series: > "lflow.c: Release ref_lflow_node as soon as it is not needed." > > controller/lflow.c | 69 > +++++++++++++++++++++++++++++++++++++----------------- > controller/lflow.h | 27 +++++++++++---------- > tests/ovn.at | 49 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 112 insertions(+), 33 deletions(-) > > diff --git a/controller/lflow.c b/controller/lflow.c > index e785866..86a5b76 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c > @@ -74,6 +74,15 @@ consider_logical_flow(const struct sbrec_logical_flow > *lflow, > struct lflow_ctx_out *l_ctx_out); > static void lflow_resource_add(struct lflow_resource_ref *, enum ref_type, > const char *ref_name, const struct uuid *); > +static struct ref_lflow_node * ref_lflow_lookup(struct hmap *ref_lflow_table, > + enum ref_type, > + const char *ref_name); I don't see an explicit rule for pointer return types for functions in our coding guidelines but from what I see most of the time we prefer to attach the '*' to the function name. Or: Indent needs one more space. > +static struct lflow_ref_node * lflow_ref_lookup(struct hmap *lflow_ref_table, > + const struct uuid > *lflow_uuid); Same here. > +static void ref_lflow_node_destroy(struct ref_lflow_node *); > +static void lflow_resource_destroy_lflow(struct lflow_resource_ref *, > + const struct uuid *lflow_uuid); > + > > static bool > lookup_port_cb(const void *aux_, const char *port_name, unsigned int *portp) > @@ -161,15 +170,14 @@ lflow_resource_destroy(struct lflow_resource_ref *lfrr) > { > struct ref_lflow_node *rlfn, *rlfn_next; > HMAP_FOR_EACH_SAFE (rlfn, rlfn_next, node, &lfrr->ref_lflow_table) { > - free(rlfn->ref_name); > struct lflow_ref_list_node *lrln, *next; > - LIST_FOR_EACH_SAFE (lrln, next, ref_list, &rlfn->ref_lflow_head) { > - ovs_list_remove(&lrln->ref_list); > - ovs_list_remove(&lrln->lflow_list); > + HMAP_FOR_EACH_SAFE (lrln, next, hmap_node, &rlfn->lflow_uuids) { > + ovs_list_remove(&lrln->list_node); > + hmap_remove(&rlfn->lflow_uuids, &lrln->hmap_node); > free(lrln); > } > hmap_remove(&lfrr->ref_lflow_table, &rlfn->node); > - free(rlfn); > + ref_lflow_node_destroy(rlfn); > } > hmap_destroy(&lfrr->ref_lflow_table); > > @@ -224,17 +232,28 @@ lflow_resource_add(struct lflow_resource_ref *lfrr, > enum ref_type type, > { > struct ref_lflow_node *rlfn = ref_lflow_lookup(&lfrr->ref_lflow_table, > type, ref_name); > + struct lflow_ref_node *lfrn = lflow_ref_lookup(&lfrr->lflow_ref_table, > + lflow_uuid); > + if (rlfn && lfrn) { > + /* Check if the mapping already existed before adding a new one. */ I would move this to a separate function: static bool ref_lflow_node_contains_uuid(const struct ref_lflow_node *, const struct uuid *); > + struct lflow_ref_list_node *n; > + HMAP_FOR_EACH_WITH_HASH (n, hmap_node, uuid_hash(lflow_uuid), > + &rlfn->lflow_uuids) { Indent needs an extra space. > + if (uuid_equals(&n->lflow_uuid, lflow_uuid)) { > + return; > + } > + } > + } > + > if (!rlfn) { > rlfn = xzalloc(sizeof *rlfn); > rlfn->node.hash = hash_string(ref_name, type); > rlfn->type = type; > rlfn->ref_name = xstrdup(ref_name); > - ovs_list_init(&rlfn->ref_lflow_head); > + hmap_init(&rlfn->lflow_uuids); > hmap_insert(&lfrr->ref_lflow_table, &rlfn->node, rlfn->node.hash); As mentioned on the v1 thread, we have lookup functions, it would make sense to have *_create() functions. > } > > - struct lflow_ref_node *lfrn = lflow_ref_lookup(&lfrr->lflow_ref_table, > - lflow_uuid); > if (!lfrn) { > lfrn = xzalloc(sizeof *lfrn); > lfrn->node.hash = uuid_hash(lflow_uuid); Same here. > @@ -245,8 +264,17 @@ lflow_resource_add(struct lflow_resource_ref *lfrr, enum > ref_type type, > > struct lflow_ref_list_node *lrln = xzalloc(sizeof *lrln); > lrln->lflow_uuid = *lflow_uuid; > - ovs_list_push_back(&rlfn->ref_lflow_head, &lrln->ref_list); > - ovs_list_push_back(&lfrn->lflow_ref_head, &lrln->lflow_list); > + lrln->rlfn = rlfn; > + hmap_insert(&rlfn->lflow_uuids, &lrln->hmap_node, uuid_hash(lflow_uuid)); > + ovs_list_push_back(&lfrn->lflow_ref_head, &lrln->list_node); Here too. > +} > + > +static void > +ref_lflow_node_destroy(struct ref_lflow_node *rlfn) > +{ > + free(rlfn->ref_name); > + hmap_destroy(&rlfn->lflow_uuids); > + free(rlfn); > } > > static void > @@ -261,9 +289,9 @@ lflow_resource_destroy_lflow(struct lflow_resource_ref > *lfrr, > > hmap_remove(&lfrr->lflow_ref_table, &lfrn->node); > struct lflow_ref_list_node *lrln, *next; > - LIST_FOR_EACH_SAFE (lrln, next, lflow_list, &lfrn->lflow_ref_head) { > - ovs_list_remove(&lrln->ref_list); > - ovs_list_remove(&lrln->lflow_list); > + LIST_FOR_EACH_SAFE (lrln, next, list_node, &lfrn->lflow_ref_head) { > + ovs_list_remove(&lrln->list_node); > + hmap_remove(&lrln->rlfn->lflow_uuids, &lrln->hmap_node); Here we remove every lflow_ref_list_node from lflow_ref_head and also remove it from the lflow_uuids hashmap but in lflow_handle_changed_ref() we split this operation in two stages: detach and later destroy. We have a comment in lflow_handle_changed_ref() but I think it would be cleaner if we had a "lflow_ref_list_node_detach()" and "lflow_ref_list_node_destroy()" API. We can even add "ovs_list_remove(&lrln->list_node)" if we're a bit careful and check that the list_node is used before removing it. My first impression when reading the code in lflow_handle_changed_ref() was that we forgot to remove the node from the list. > free(lrln); > } > free(lfrn); > @@ -531,12 +559,12 @@ lflow_handle_changed_ref(enum ref_type ref_type, const > char *ref_name, > hmap_remove(&l_ctx_out->lfrr->ref_lflow_table, &rlfn->node); > > struct lflow_ref_list_node *lrln, *next; > - /* Detach the rlfn->ref_lflow_head nodes from the lfrr table and clean > + /* Detach the rlfn->lflow_uuids nodes from the lfrr table and clean > * up all other nodes related to the lflows that uses the resource, > * so that the old nodes won't interfere with updating the lfrr table > * when reparsing the lflows. */ > - LIST_FOR_EACH (lrln, ref_list, &rlfn->ref_lflow_head) { > - ovs_list_remove(&lrln->lflow_list); > + HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) { > + ovs_list_remove(&lrln->list_node); > } > > struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); > @@ -565,7 +593,7 @@ lflow_handle_changed_ref(enum ref_type ref_type, const > char *ref_name, > /* Firstly, flood remove the flows from desired flow table. */ > struct hmap flood_remove_nodes = HMAP_INITIALIZER(&flood_remove_nodes); > struct ofctrl_flood_remove_node *ofrn, *ofrn_next; > - LIST_FOR_EACH (lrln, ref_list, &rlfn->ref_lflow_head) { > + HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) { > VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type: %d," > " name: %s.", > UUID_ARGS(&lrln->lflow_uuid), > @@ -604,12 +632,11 @@ lflow_handle_changed_ref(enum ref_type ref_type, const > char *ref_name, > } > hmap_destroy(&flood_remove_nodes); > > - LIST_FOR_EACH_SAFE (lrln, next, ref_list, &rlfn->ref_lflow_head) { > - ovs_list_remove(&lrln->ref_list); > + HMAP_FOR_EACH_SAFE (lrln, next, hmap_node, &rlfn->lflow_uuids) { > + hmap_remove(&rlfn->lflow_uuids, &lrln->hmap_node); > free(lrln); > } > - free(rlfn->ref_name); > - free(rlfn); > + ref_lflow_node_destroy(rlfn); I think detaching the "lflow_ref_list_node" entries from ref_lflow_node and freeing them can be part of ref_lflow_node_destroy(). We duplicate this code already in lflow_resource_destroy() and here, lflow_handle_changed_ref(). > > dhcp_opts_destroy(&dhcp_opts); > dhcp_opts_destroy(&dhcpv6_opts); > diff --git a/controller/lflow.h b/controller/lflow.h > index c66b318..1251fb0 100644 > --- a/controller/lflow.h > +++ b/controller/lflow.h > @@ -78,25 +78,28 @@ enum ref_type { > REF_TYPE_PORTBINDING > }; > > -/* Maintains the relationship for a pair of named resource and > - * a lflow, indexed by both ref_lflow_table and lflow_ref_table. */ > -struct lflow_ref_list_node { > - struct ovs_list lflow_list; /* list for same lflow */ > - struct ovs_list ref_list; /* list for same ref */ > - struct uuid lflow_uuid; > -}; > - > struct ref_lflow_node { > - struct hmap_node node; > + struct hmap_node node; /* node in lflow_resource_ref.ref_lflow_table. */ > enum ref_type type; /* key */ > char *ref_name; /* key */ Nit: I would align the comments with the one for the 'node' field. Regards, Dumitru > - struct ovs_list ref_lflow_head; > + struct hmap lflow_uuids; /* Contains lflow_ref_list_node. Use hmap > instead > + of list so that lflow_resource_add() can > check > + and avoid adding redundant entires in O(1). > */ > }; > > struct lflow_ref_node { > - struct hmap_node node; > + struct hmap_node node; /* node in lflow_resource_ref.lflow_ref_table. */ > struct uuid lflow_uuid; /* key */ > - struct ovs_list lflow_ref_head; > + struct ovs_list lflow_ref_head; /* Contains lflow_ref_list_node. */ > +}; > + > +/* Maintains the relationship for a pair of named resource and > + * a lflow, indexed by both ref_lflow_table and lflow_ref_table. */ > +struct lflow_ref_list_node { > + struct ovs_list list_node; /* node in lflow_ref_node.lflow_ref_head. */ > + struct hmap_node hmap_node; /* node in ref_lflow_node.lflow_uuids. */ > + struct uuid lflow_uuid; > + struct ref_lflow_node *rlfn; > }; > > struct lflow_resource_ref { > diff --git a/tests/ovn.at b/tests/ovn.at > index 41fe577..d368fb9 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -22117,3 +22117,52 @@ OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [1.expected]) > > OVN_CLEANUP([hv1]) > AT_CLEANUP > + > +AT_SETUP([ovn -- port bind/unbind change handling with conj flows - IPv6]) > +ovn_start > + > +ovn-nbctl ls-add ls1 > + > +ovn-nbctl lsp-add ls1 lsp1 \ > + -- lsp-set-addresses lsp1 "f0:00:00:00:00:01 2001::1" \ > + -- lsp-set-port-security lsp1 "f0:00:00:00:00:01 2001::1" > + > +net_add n1 > +sim_add hv1 > + > +as hv1 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > +ovs-vsctl -- add-port br-int hv1-vif1 -- \ > + set interface hv1-vif1 external-ids:iface-id=lsp1 \ > + options:tx_pcap=hv1/vif1-tx.pcap \ > + options:rxq_pcap=hv1/vif1-rx.pcap \ > + ofport-request=1 > + > +ovn-nbctl --wait=hv sync > + > +# Expected conjunction flows: > +# ... nd_tll=00:00:00:00:00:00 actions=conjunction(2,2/2) > +# ... nd_tll=f0:00:00:00:00:01 actions=conjunction(2,2/2) > +# ... nd_target=fe80::f200:ff:fe00:1 actions=conjunction(2,1/2) > +# ... nd_target=2001::1 actions=conjunction(2,1/2) > +OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \ > +grep conjunction | wc -l`]) > + > +# unbind the port > +ovs-vsctl set interface hv1-vif1 external_ids:iface-id=foo > +OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \ > +grep conjunction | wc -l`]) > + > +# bind the port again > +ovs-vsctl set interface hv1-vif1 external_ids:iface-id=lsp1 > +OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \ > +grep conjunction | wc -l`]) > + > +# unbind the port again > +ovs-vsctl set interface hv1-vif1 external_ids:iface-id=foo > +OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \ > +grep conjunction | wc -l`]) > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
