[ovs-dev,v3,1/2] lflow.c: Avoid adding redundant resource refs for port-bindings.

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(-)

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