[ovs-dev] [PATCH ovn v3 1/2] lflow.c: Avoid adding redundant resource refs for port-bindings.
Numan Siddique
numans at ovn.org
Thu Sep 17 12:44:23 UTC 2020
On Thu, Sep 17, 2020 at 1:14 PM Dumitru Ceara <dceara at 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 at 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 at ovn.org>
> > ---
>
> Hi Han,
>
> Acked-by: Dumitru Ceara <dceara at 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 at 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 at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
More information about the dev
mailing list