Message ID | 1600279283-7597-1-git-send-email-hzhou@ovn.org |
---|---|
State | Accepted |
Headers | show |
Series | [ovs-dev,v3,1/2] lflow.c: Avoid adding redundant resource refs for port-bindings. | expand |
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 <dceara@redhat.com> > 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 <hzhou@ovn.org> > --- Hi Han, Acked-by: Dumitru Ceara <dceara@redhat.com> 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. 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 >
On Thu, Sep 17, 2020 at 1:14 PM Dumitru Ceara <dceara@redhat.com> 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 <dceara@redhat.com> > > 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 <hzhou@ovn.org> > > --- > > Hi Han, > > Acked-by: Dumitru Ceara <dceara@redhat.com> > 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 <numans@ovn.org> 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Thu, Sep 17, 2020 at 5:44 AM Numan Siddique <numans@ovn.org> wrote: > > > > On Thu, Sep 17, 2020 at 1:14 PM Dumitru Ceara <dceara@redhat.com> 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 <dceara@redhat.com> >> > 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 <hzhou@ovn.org> >> > --- >> >> Hi Han, >> >> Acked-by: Dumitru Ceara <dceara@redhat.com> >> >> >> 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 <numans@ovn.org> > > Thanks > Numan > Thanks Dumitru and Numan. I applied this to master and backported to branch 20.09 and 20.06. I didn't add the comments in this patch because it is not related to the fix itself. I (or anyone) can add it in a separate commit, together with any refactors if needed.
Bleep bloop. Greetings Han Zhou, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: error: Failed to merge in the changes. hint: Use 'git am --show-current-patch' to see the failed patch Patch failed at 0001 lflow.c: Avoid adding redundant resource refs for port-bindings. When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
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
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 <dceara@redhat.com> 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 <hzhou@ovn.org> --- 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(-)