[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