On Thu, Sep 17, 2020 at 1:14 PM Dumitru Ceara <[email protected]> wrote:
> On 9/16/20 8:01 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 existence of same reference > before > > 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]> > > --- > > Hi Han, > > Acked-by: Dumitru Ceara <[email protected]> > If possible it would be great if you could also add the detailed > comments you shared [1] about how lflow_handle_changed_ref() works > before merging it. > > Otherwise we can add them as a follow up patch, whatever works best for > you. > +1 for adding the comment either in this patch or as a separate patch. Thanks Han for fixing this issue. Acked-by: Numan Siddique <[email protected]> Thanks Numan > > Thanks, > Dumitru > > [1] > https://mail.openvswitch.org/pipermail/ovs-dev/2020-September/375166.html > > > v2 - > v3: Fix indentation problems pointed out by Dumitru. > > > > 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..db078d2 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); > > +static struct lflow_ref_node *lflow_ref_lookup(struct hmap > *lflow_ref_table, > > + const struct uuid > *lflow_uuid); > > +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. */ > > + struct lflow_ref_list_node *n; > > + HMAP_FOR_EACH_WITH_HASH (n, hmap_node, uuid_hash(lflow_uuid), > > + &rlfn->lflow_uuids) { > > + 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); > > } > > > > - 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); > > @@ -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); > > +} > > + > > +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); > > 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); > > > > 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 */ > > - 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 > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
