[ovs-dev] [PATCH ovn 4/9] ovn-controller: Fix conjunction handling with incremental processing.

Mark Michelson mmichels at redhat.com
Wed Sep 2 15:52:44 UTC 2020


I think this can be simplified some.

Specifically, I don't think every ovn_flow needs to have a corresponding 
sb_flow_ref created. The only time that sb_flow_ref logic is needed is 
when OF flows get appended to an existing flow. In every other case, either

a) A southbound logical flow UUID corresponds to exactly one OF flow, or
b) A southbound object UUID corresponds to multiple OF flows, but they 
are unrelated to each other.

So why not only create sb_flow_refs in the ofctrl_add_or_append_flow() 
case? This way, you save a lot of unnecessary memory and processing 
overhead during "normal" cases.

On 8/21/20 3:16 PM, Han Zhou wrote:
> When translating lflows to OVS flows, different lflows can refer to same OVS
> flow as a result of calling ofctrl_add_or_append_flow(), particularly for
> conjunction combinding. However, the implementation doesn't work with
> incremental processing, because when any of the lflows are removed, we rely on
> the lflow's uuid to remove the OVS flow in the desired flow table. Currently
> only one single lflow uuid is maintained in the desired flow, so removing one
> of the lflows that references to the same desired flow resulted in wrong
> behavior: either removing flows that are used by other lflows, or the existing
> flows are not updated (part of the conjunction actions should be removed from
> the flow).
> 
> To solve the problem, this patch maintains the cross reference (M:N) between
> lflows (and other sb objects) and desired flows, and handles the flow removal
> and updates with a flood-removal and re-add approach.
> 
> Fixes: e659bab31a9 ("Combine conjunctions with identical matches into one flow.")
> Cc: Mark Michelson <mmichels at redhat.com>
> Signed-off-by: Han Zhou <hzhou at ovn.org>
> ---
>   controller/lflow.c  | 103 +++++++++++------
>   controller/ofctrl.c | 319 +++++++++++++++++++++++++++++++++++++++++-----------
>   controller/ofctrl.h |  31 ++++-
>   tests/ovn.at        |  90 +++++++++++++++
>   4 files changed, 438 insertions(+), 105 deletions(-)
> 
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 0c35b7d..6fbd36f 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -342,41 +342,56 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in,
>       struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts);
>       nd_ra_opts_init(&nd_ra_opts);
>   
> -    /* Handle removed flows first, and then other flows, so that when
> -     * the flows being added and removed have same match conditions
> -     * can be processed in the proper order */
> +    struct controller_event_options controller_event_opts;
> +    controller_event_opts_init(&controller_event_opts);
> +
> +    /* Handle flow removing first (for deleted or updated lflows), and then
> +     * handle reprocessing or adding flows, so that when the flows being
> +     * removed and added with same match conditions can be processed in the
> +     * proper order */
> +
> +    struct hmap flood_remove_nodes = HMAP_INITIALIZER(&flood_remove_nodes);
> +    struct ofctrl_flood_remove_node *ofrn, *next;
>       SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow,
>                                                  l_ctx_in->logical_flow_table) {
> -        /* Remove any flows that should be removed. */
> -        if (sbrec_logical_flow_is_deleted(lflow)) {
> -            VLOG_DBG("handle deleted lflow "UUID_FMT,
> +        if (!sbrec_logical_flow_is_new(lflow)) {
> +            VLOG_DBG("delete lflow "UUID_FMT,
>                        UUID_ARGS(&lflow->header_.uuid));
> -            ofctrl_remove_flows(l_ctx_out->flow_table, &lflow->header_.uuid);
> -            /* Delete entries from lflow resource reference. */
> -            lflow_resource_destroy_lflow(l_ctx_out->lfrr,
> -                                         &lflow->header_.uuid);
> +            ofrn = xmalloc(sizeof *ofrn);
> +            ofrn->sb_uuid = lflow->header_.uuid;
> +            hmap_insert(&flood_remove_nodes, &ofrn->hmap_node,
> +                        uuid_hash(&ofrn->sb_uuid));
>           }
>       }
> +    ofctrl_flood_remove_flows(l_ctx_out->flow_table, &flood_remove_nodes);
> +    HMAP_FOR_EACH (ofrn, hmap_node, &flood_remove_nodes) {
> +        /* Delete entries from lflow resource reference. */
> +        lflow_resource_destroy_lflow(l_ctx_out->lfrr, &ofrn->sb_uuid);
> +        /* Reprocessing the lflow if the sb record is not deleted. */
> +        lflow = sbrec_logical_flow_table_get_for_uuid(
> +            l_ctx_in->logical_flow_table, &ofrn->sb_uuid);
> +        if (lflow) {
> +            VLOG_DBG("re-add lflow "UUID_FMT,
> +                     UUID_ARGS(&lflow->header_.uuid));
> +            if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> +                                       &nd_ra_opts, &controller_event_opts,
> +                                       l_ctx_in, l_ctx_out)) {
> +                ret = false;
> +                break;
> +            }
> +        }
> +    }
> +    HMAP_FOR_EACH_SAFE (ofrn, next, hmap_node, &flood_remove_nodes) {
> +        hmap_remove(&flood_remove_nodes, &ofrn->hmap_node);
> +        free(ofrn);
> +    }
> +    hmap_destroy(&flood_remove_nodes);
>   
> -    struct controller_event_options controller_event_opts;
> -    controller_event_opts_init(&controller_event_opts);
> -
> +    /* Now handle new lflows only. */
>       SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow,
>                                                  l_ctx_in->logical_flow_table) {
> -        if (!sbrec_logical_flow_is_deleted(lflow)) {
> -            /* Now, add/modify existing flows. If the logical
> -             * flow is a modification, just remove the flows
> -             * for this row, and then add new flows. */
> -            if (!sbrec_logical_flow_is_new(lflow)) {
> -                VLOG_DBG("handle updated lflow "UUID_FMT,
> -                         UUID_ARGS(&lflow->header_.uuid));
> -                ofctrl_remove_flows(l_ctx_out->flow_table,
> -                                    &lflow->header_.uuid);
> -                /* Delete entries from lflow resource reference. */
> -                lflow_resource_destroy_lflow(l_ctx_out->lfrr,
> -                                             &lflow->header_.uuid);
> -            }
> -            VLOG_DBG("handle new lflow "UUID_FMT,
> +        if (sbrec_logical_flow_is_new(lflow)) {
> +            VLOG_DBG("add lflow "UUID_FMT,
>                        UUID_ARGS(&lflow->header_.uuid));
>               if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
>                                          &nd_ra_opts, &controller_event_opts,
> @@ -420,7 +435,6 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name,
>        * when reparsing the lflows. */
>       LIST_FOR_EACH (lrln, ref_list, &rlfn->ref_lflow_head) {
>           ovs_list_remove(&lrln->lflow_list);
> -        lflow_resource_destroy_lflow(l_ctx_out->lfrr, &lrln->lflow_uuid);
>       }
>   
>       struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
> @@ -446,22 +460,32 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name,
>       controller_event_opts_init(&controller_event_opts);
>   
>       /* Re-parse the related lflows. */
> +    /* 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) {
> +        VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type: %d,"
> +                 " name: %s.",
> +                 UUID_ARGS(&lrln->lflow_uuid),
> +                 ref_type, ref_name);
> +        ofctrl_flood_remove_add_node(&flood_remove_nodes, &lrln->lflow_uuid);
> +    }
> +    ofctrl_flood_remove_flows(l_ctx_out->flow_table, &flood_remove_nodes);
> +
> +    /* Secondly, for each lflow that is actually removed, reprocessing it. */
> +    HMAP_FOR_EACH (ofrn, hmap_node, &flood_remove_nodes) {
> +        lflow_resource_destroy_lflow(l_ctx_out->lfrr, &ofrn->sb_uuid);
> +
>           const struct sbrec_logical_flow *lflow =
>               sbrec_logical_flow_table_get_for_uuid(l_ctx_in->logical_flow_table,
> -                                                  &lrln->lflow_uuid);
> +                                                  &ofrn->sb_uuid);
>           if (!lflow) {
> -            VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type: %d,"
> -                     " name: %s - not found.",
> -                     UUID_ARGS(&lrln->lflow_uuid),
> +            VLOG_DBG("lflow "UUID_FMT" not found while reprocessing for"
> +                     " resource type: %d, name: %s.",
> +                     UUID_ARGS(&ofrn->sb_uuid),
>                        ref_type, ref_name);
>               continue;
>           }
> -        VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type: %d,"
> -                 " name: %s.",
> -                 UUID_ARGS(&lrln->lflow_uuid),
> -                 ref_type, ref_name);
> -        ofctrl_remove_flows(l_ctx_out->flow_table, &lrln->lflow_uuid);
>   
>           if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
>                                      &nd_ra_opts, &controller_event_opts,
> @@ -471,6 +495,11 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name,
>           }
>           *changed = true;
>       }
> +    HMAP_FOR_EACH_SAFE (ofrn, ofrn_next, hmap_node, &flood_remove_nodes) {
> +        hmap_remove(&flood_remove_nodes, &ofrn->hmap_node);
> +        free(ofrn);
> +    }
> +    hmap_destroy(&flood_remove_nodes);
>   
>       LIST_FOR_EACH_SAFE (lrln, next, ref_list, &rlfn->ref_lflow_head) {
>           ovs_list_remove(&lrln->ref_list);
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 919db6d..c500f52 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -54,8 +54,12 @@ VLOG_DEFINE_THIS_MODULE(ofctrl);
>   /* An OpenFlow flow. */
>   struct ovn_flow {
>       struct hmap_node match_hmap_node; /* For match based hashing. */
> -    struct hindex_node uuid_hindex_node; /* For uuid based hashing. */
>       struct ovs_list list_node; /* For handling lists of flows. */
> +    struct ovs_list references; /* A list of struct sb_flow_ref nodes, which
> +                                   references this flow. (There are cases
> +                                   that multiple SB entities share the same
> +                                   desired OpenFlow flow, e.g. when
> +                                   conjunction is used.) */
>   
>       /* Key. */
>       uint8_t table_id;
> @@ -63,21 +67,34 @@ struct ovn_flow {
>       struct minimatch match;
>   
>       /* Data. */
> -    struct uuid sb_uuid;
>       struct ofpact *ofpacts;
>       size_t ofpacts_len;
>       uint64_t cookie;
>   };
>   
> +struct sb_to_flow {
> +    struct hmap_node hmap_node; /* Node in
> +                                   ovn_desired_flow_table.uuid_flow_table. */
> +    struct uuid sb_uuid;
> +    struct ovs_list flows; /* A list of struct sb_flow_ref nodes that are
> +                              referenced by the sb_uuid. */
> +};
> +
> +struct sb_flow_ref {
> +    struct ovs_list sb_list; /* List node in ovn_flow.references. */
> +    struct ovs_list flow_list; /* List node in sb_to_flow.ovn_flows. */
> +    struct ovn_flow *flow;
> +    struct uuid sb_uuid;
> +};
> +
>   static struct ovn_flow *ovn_flow_alloc(uint8_t table_id, uint16_t priority,
>                                          uint64_t cookie,
>                                          const struct match *match,
> -                                       const struct ofpbuf *actions,
> -                                       const struct uuid *sb_uuid);
> +                                       const struct ofpbuf *actions);
>   static uint32_t ovn_flow_match_hash(const struct ovn_flow *);
>   static struct ovn_flow *ovn_flow_lookup(struct hmap *flow_table,
>                                           const struct ovn_flow *target,
> -                                        bool cmp_sb_uuid);
> +                                        const struct uuid *sb_uuid);
>   static char *ovn_flow_to_string(const struct ovn_flow *);
>   static void ovn_flow_log(const struct ovn_flow *, const char *action);
>   static void ovn_flow_destroy(struct ovn_flow *);
> @@ -644,13 +661,46 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type)
>       }
>   }
>   

> +static struct sb_to_flow *
> +sb_to_flow_find(struct hmap *uuid_flow_table, const struct uuid *sb_uuid)
> +{
> +    struct sb_to_flow *stf;
> +    HMAP_FOR_EACH_WITH_HASH (stf, hmap_node, uuid_hash(sb_uuid),
> +                            uuid_flow_table) {
> +        if (uuid_equals(sb_uuid, &stf->sb_uuid)) {
> +            return stf;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static void
> +link_flow_to_sb(struct ovn_desired_flow_table *flow_table,
> +                struct ovn_flow *f, const struct uuid *sb_uuid)
> +{
> +    struct sb_flow_ref *sfr = xmalloc(sizeof *sfr);
> +    sfr->flow = f;
> +    sfr->sb_uuid = *sb_uuid;
> +    ovs_list_insert(&f->references, &sfr->sb_list);
> +    struct sb_to_flow *stf = sb_to_flow_find(&flow_table->uuid_flow_table,
> +                                             sb_uuid);
> +    if (!stf) {
> +        stf = xmalloc(sizeof *stf);
> +        stf->sb_uuid = *sb_uuid;
> +        ovs_list_init(&stf->flows);
> +        hmap_insert(&flow_table->uuid_flow_table, &stf->hmap_node,
> +                    uuid_hash(sb_uuid));
> +    }
> +    ovs_list_insert(&stf->flows, &sfr->flow_list);
> +}
> +
>   /* Flow table interfaces to the rest of ovn-controller. */
>   
>   /* Adds a flow to 'desired_flows' with the specified 'match' and 'actions' to
>    * the OpenFlow table numbered 'table_id' with the given 'priority' and
>    * OpenFlow 'cookie'.  The caller retains ownership of 'match' and 'actions'.
>    *
> - * The flow is also added to a hash index based on sb_uuid.
> + * The flow is also linked to the sb_uuid that generates it.
>    *
>    * This just assembles the desired flow table in memory.  Nothing is actually
>    * sent to the switch until a later call to ofctrl_put().
> @@ -665,11 +715,9 @@ ofctrl_check_and_add_flow(struct ovn_desired_flow_table *flow_table,
>                             bool log_duplicate_flow)
>   {
>       struct ovn_flow *f = ovn_flow_alloc(table_id, priority, cookie, match,
> -                                        actions, sb_uuid);
> +                                        actions);
>   
> -    ovn_flow_log(f, "ofctrl_add_flow");
> -
> -    if (ovn_flow_lookup(&flow_table->match_flow_table, f, true)) {
> +    if (ovn_flow_lookup(&flow_table->match_flow_table, f, sb_uuid)) {
>           if (log_duplicate_flow) {
>               static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
>               if (!VLOG_DROP_DBG(&rl)) {
> @@ -684,31 +732,8 @@ ofctrl_check_and_add_flow(struct ovn_desired_flow_table *flow_table,
>   
>       hmap_insert(&flow_table->match_flow_table, &f->match_hmap_node,
>                   f->match_hmap_node.hash);
> -    hindex_insert(&flow_table->uuid_flow_table, &f->uuid_hindex_node,
> -                  f->uuid_hindex_node.hash);
> -}
> -
> -/* Removes a bundles of flows from the flow table. */
> -void
> -ofctrl_remove_flows(struct ovn_desired_flow_table *flow_table,
> -                    const struct uuid *sb_uuid)
> -{
> -    struct ovn_flow *f, *next;
> -    HINDEX_FOR_EACH_WITH_HASH_SAFE (f, next, uuid_hindex_node,
> -                                    uuid_hash(sb_uuid),
> -                                    &flow_table->uuid_flow_table) {
> -        if (uuid_equals(&f->sb_uuid, sb_uuid)) {
> -            ovn_flow_log(f, "ofctrl_remove_flow");
> -            hmap_remove(&flow_table->match_flow_table,
> -                        &f->match_hmap_node);
> -            hindex_remove(&flow_table->uuid_flow_table, &f->uuid_hindex_node);
> -            ovn_flow_destroy(f);
> -        }
> -    }
> -
> -    /* remove any related group and meter info */
> -    ovn_extend_table_remove_desired(groups, sb_uuid);
> -    ovn_extend_table_remove_desired(meters, sb_uuid);
> +    link_flow_to_sb(flow_table, f, sb_uuid);
> +    ovn_flow_log(f, "ofctrl_add_flow");
>   }
>   
>   void
> @@ -721,6 +746,9 @@ ofctrl_add_flow(struct ovn_desired_flow_table *desired_flows,
>                                 match, actions, sb_uuid, true);
>   }
>   
> +/* Either add a new flow, or append actions on an existing flow. If the
> + * flow existed, a new link will also be created between the new sb_uuid
> + * and the existing flow. */
>   void
>   ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows,
>                             uint8_t table_id, uint16_t priority, uint64_t cookie,
> @@ -729,12 +757,10 @@ ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows,
>                             const struct uuid *sb_uuid)
>   {
>       struct ovn_flow *f = ovn_flow_alloc(table_id, priority, cookie, match,
> -                                        actions, sb_uuid);
> -
> -    ovn_flow_log(f, "ofctrl_add_or_append_flow");
> +                                        actions);
>   
>       struct ovn_flow *existing;
> -    existing = ovn_flow_lookup(&desired_flows->match_flow_table, f, false);
> +    existing = ovn_flow_lookup(&desired_flows->match_flow_table, f, NULL);
>       if (existing) {
>           /* There's already a flow with this particular match. Append the
>            * action to that flow rather than adding a new flow
> @@ -751,11 +777,169 @@ ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows,
>   
>           ofpbuf_uninit(&compound);
>           ovn_flow_destroy(f);
> +        f = existing;
>       } else {
>           hmap_insert(&desired_flows->match_flow_table, &f->match_hmap_node,
>                       f->match_hmap_node.hash);
> -        hindex_insert(&desired_flows->uuid_flow_table, &f->uuid_hindex_node,
> -                      f->uuid_hindex_node.hash);
> +    }
> +    link_flow_to_sb(desired_flows, f, sb_uuid);
> +
> +    if (existing) {
> +        ovn_flow_log(f, "ofctrl_add_or_append_flow (append)");
> +    } else {
> +        ovn_flow_log(f, "ofctrl_add_or_append_flow (add)");
> +    }
> +}
> +
> +/* Remove ovn_flows for the given "sb_to_flow" node in the uuid_flow_table.
> + * Optionally log the message for each flow that is acturally removed, if
> + * log_msg is not NULL. */
> +static void
> +remove_flows_from_sb_to_flow(struct ovn_desired_flow_table *flow_table,
> +                             struct sb_to_flow *stf,
> +                             const char *log_msg)
> +{
> +    struct sb_flow_ref *sfr, *next;
> +    LIST_FOR_EACH_SAFE (sfr, next, flow_list, &stf->flows) {
> +        ovs_list_remove(&sfr->sb_list);
> +        ovs_list_remove(&sfr->flow_list);
> +        struct ovn_flow *f = sfr->flow;
> +        free(sfr);
> +
> +        if (ovs_list_is_empty(&f->references)) {
> +            if (log_msg) {
> +                ovn_flow_log(f, log_msg);
> +            }
> +            hmap_remove(&flow_table->match_flow_table,
> +                        &f->match_hmap_node);
> +            ovn_flow_destroy(f);
> +        }
> +    }
> +    hmap_remove(&flow_table->uuid_flow_table, &stf->hmap_node);
> +    free(stf);
> +}
> +
> +void
> +ofctrl_remove_flows(struct ovn_desired_flow_table *flow_table,
> +                    const struct uuid *sb_uuid)
> +{
> +    struct sb_to_flow *stf = sb_to_flow_find(&flow_table->uuid_flow_table,
> +                                             sb_uuid);
> +    if (stf) {
> +        remove_flows_from_sb_to_flow(flow_table, stf, "ofctrl_remove_flow");
> +    }
> +
> +    /* remove any related group and meter info */
> +    ovn_extend_table_remove_desired(groups, sb_uuid);
> +    ovn_extend_table_remove_desired(meters, sb_uuid);
> +}
> +
> +static struct ofctrl_flood_remove_node *
> +flood_remove_find_node(struct hmap *flood_remove_nodes, struct uuid *sb_uuid)
> +{
> +    struct ofctrl_flood_remove_node *ofrn;
> +    HMAP_FOR_EACH_WITH_HASH (ofrn, hmap_node, uuid_hash(sb_uuid),
> +                             flood_remove_nodes) {
> +        if (uuid_equals(&ofrn->sb_uuid, sb_uuid)) {
> +            return ofrn;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +void
> +ofctrl_flood_remove_add_node(struct hmap *flood_remove_nodes,
> +                             const struct uuid *sb_uuid)
> +{
> +    struct ofctrl_flood_remove_node *ofrn = xmalloc(sizeof *ofrn);
> +    ofrn->sb_uuid = *sb_uuid;
> +    hmap_insert(flood_remove_nodes, &ofrn->hmap_node, uuid_hash(sb_uuid));
> +}
> +
> +static void
> +flood_remove_flows_for_sb_uuid(struct ovn_desired_flow_table *flow_table,
> +                               const struct uuid *sb_uuid,
> +                               struct hmap *flood_remove_nodes)
> +{
> +    struct sb_to_flow *stf = sb_to_flow_find(&flow_table->uuid_flow_table,
> +                                             sb_uuid);
> +    if (!stf) {
> +        return;
> +    }
> +
> +    /* ovn_flows that have other references and waiting to be removed. */
> +    struct ovs_list to_be_removed = OVS_LIST_INITIALIZER(&to_be_removed);
> +
> +    /* Traverse all flows for the given sb_uuid. */
> +    struct sb_flow_ref *sfr, *next;
> +    LIST_FOR_EACH_SAFE (sfr, next, flow_list, &stf->flows) {
> +        struct ovn_flow *f = sfr->flow;
> +        ovn_flow_log(f, "flood remove");
> +
> +        ovs_list_remove(&sfr->sb_list);
> +        ovs_list_remove(&sfr->flow_list);
> +        free(sfr);
> +
> +        ovs_assert(ovs_list_is_empty(&f->list_node));
> +        if (ovs_list_is_empty(&f->references)) {
> +            /* This is to optimize the case when most flows have only
> +             * one referencing sb_uuid, so to_be_removed list should
> +             * be empty in most cases. */
> +            hmap_remove(&flow_table->match_flow_table,
> +                        &f->match_hmap_node);
> +            ovn_flow_destroy(f);
> +        } else {
> +            ovs_list_insert(&to_be_removed, &f->list_node);
> +        }
> +    }
> +    hmap_remove(&flow_table->uuid_flow_table, &stf->hmap_node);
> +    free(stf);
> +
> +    /* Traverse other referencing sb_uuids for the flows in the to_be_removed
> +     * list. */
> +
> +    /* Detach the items in f->references from the sfr.flow_list lists,
> +     * so that recursive calls will not mess up the sfr.sb_list list. */
> +    struct ovn_flow *f, *f_next;
> +    LIST_FOR_EACH (f, list_node, &to_be_removed) {
> +        ovs_assert(!ovs_list_is_empty(&f->references));
> +        LIST_FOR_EACH (sfr, sb_list, &f->references) {
> +            ovs_list_remove(&sfr->flow_list);
> +        }
> +    }
> +    LIST_FOR_EACH_SAFE (f, f_next, list_node, &to_be_removed) {
> +        LIST_FOR_EACH_SAFE (sfr, next, sb_list, &f->references) {
> +            if (!flood_remove_find_node(flood_remove_nodes, &sfr->sb_uuid)) {
> +                ofctrl_flood_remove_add_node(flood_remove_nodes,
> +                                             &sfr->sb_uuid);
> +                flood_remove_flows_for_sb_uuid(flow_table, &sfr->sb_uuid,
> +                                               flood_remove_nodes);
> +            }
> +            ovs_list_remove(&sfr->sb_list);
> +            free(sfr);
> +        }
> +        ovs_list_remove(&f->list_node);
> +        hmap_remove(&flow_table->match_flow_table,
> +                    &f->match_hmap_node);
> +        ovn_flow_destroy(f);
> +    }
> +
> +}
> +
> +void
> +ofctrl_flood_remove_flows(struct ovn_desired_flow_table *flow_table,
> +                          struct hmap *flood_remove_nodes)
> +{
> +    struct ofctrl_flood_remove_node *ofrn;
> +    HMAP_FOR_EACH (ofrn, hmap_node, flood_remove_nodes) {
> +        flood_remove_flows_for_sb_uuid(flow_table, &ofrn->sb_uuid,
> +                                       flood_remove_nodes);
> +    }
> +
> +    /* remove any related group and meter info */
> +    HMAP_FOR_EACH (ofrn, hmap_node, flood_remove_nodes) {
> +        ovn_extend_table_remove_desired(groups, &ofrn->sb_uuid);
> +        ovn_extend_table_remove_desired(meters, &ofrn->sb_uuid);
>       }
>   }
>   

> @@ -763,18 +947,17 @@ ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows,
>   
>   static struct ovn_flow *
>   ovn_flow_alloc(uint8_t table_id, uint16_t priority, uint64_t cookie,
> -               const struct match *match, const struct ofpbuf *actions,
> -               const struct uuid *sb_uuid)
> +               const struct match *match, const struct ofpbuf *actions)
>   {
>       struct ovn_flow *f = xmalloc(sizeof *f);
> +    ovs_list_init(&f->references);
> +    ovs_list_init(&f->list_node);
>       f->table_id = table_id;
>       f->priority = priority;
>       minimatch_init(&f->match, match);
>       f->ofpacts = xmemdup(actions->data, actions->size);
>       f->ofpacts_len = actions->size;
> -    f->sb_uuid = *sb_uuid;
>       f->match_hmap_node.hash = ovn_flow_match_hash(f);
> -    f->uuid_hindex_node.hash = uuid_hash(&f->sb_uuid);
>       f->cookie = cookie;
>   
>       return f;
> @@ -793,23 +976,27 @@ static struct ovn_flow *
>   ovn_flow_dup(struct ovn_flow *src)
>   {
>       struct ovn_flow *dst = xmalloc(sizeof *dst);
> +    ovs_list_init(&dst->references);
>       dst->table_id = src->table_id;
>       dst->priority = src->priority;
>       minimatch_clone(&dst->match, &src->match);
>       dst->ofpacts = xmemdup(src->ofpacts, src->ofpacts_len);
>       dst->ofpacts_len = src->ofpacts_len;
> -    dst->sb_uuid = src->sb_uuid;
>       dst->match_hmap_node.hash = src->match_hmap_node.hash;
> -    dst->uuid_hindex_node.hash = uuid_hash(&src->sb_uuid);
>       dst->cookie = src->cookie;
>       return dst;
>   }
>   
>   /* Finds and returns an ovn_flow in 'flow_table' whose key is identical to
> - * 'target''s key, or NULL if there is none. */
> + * 'target''s key, or NULL if there is none.
> + *
> + * If sb_uuid is not NULL, the function will also check if the found flow is
> + * referenced by the sb_uuid.
> + *
> + * NOTE: sb_uuid can only be used for ovn_desired_flow_table lookup. */
>   static struct ovn_flow *
>   ovn_flow_lookup(struct hmap *flow_table, const struct ovn_flow *target,
> -                bool cmp_sb_uuid)
> +                const struct uuid *sb_uuid)
>   {
>       struct ovn_flow *f;
>   
> @@ -818,9 +1005,16 @@ ovn_flow_lookup(struct hmap *flow_table, const struct ovn_flow *target,
>           if (f->table_id == target->table_id
>               && f->priority == target->priority
>               && minimatch_equal(&f->match, &target->match)) {
> -            if (!cmp_sb_uuid || uuid_equals(&target->sb_uuid, &f->sb_uuid)) {
> +            if (!sb_uuid) {
>                   return f;
>               }
> +            ovs_assert(flow_table != &installed_flows);
> +            struct sb_flow_ref *sfr;
> +            LIST_FOR_EACH (sfr, sb_list, &f->references) {
> +                if (uuid_equals(sb_uuid, &sfr->sb_uuid)) {
> +                    return f;
> +                }
> +            }
>           }
>       }
>       return NULL;
> @@ -830,7 +1024,7 @@ static char *
>   ovn_flow_to_string(const struct ovn_flow *f)
>   {
>       struct ds s = DS_EMPTY_INITIALIZER;
> -    ds_put_format(&s, "sb_uuid="UUID_FMT", ", UUID_ARGS(&f->sb_uuid));
> +
>       ds_put_format(&s, "cookie=%"PRIx64", ", f->cookie);
>       ds_put_format(&s, "table_id=%"PRIu8", ", f->table_id);
>       ds_put_format(&s, "priority=%"PRIu16", ", f->priority);
> @@ -855,6 +1049,7 @@ static void
>   ovn_flow_destroy(struct ovn_flow *f)
>   {
>       if (f) {
> +        ovs_assert(ovs_list_is_empty(&f->references));
>           minimatch_destroy(&f->match);
>           free(f->ofpacts);
>           free(f);
> @@ -866,18 +1061,16 @@ void
>   ovn_desired_flow_table_init(struct ovn_desired_flow_table *flow_table)
>   {
>       hmap_init(&flow_table->match_flow_table);
> -    hindex_init(&flow_table->uuid_flow_table);
> +    hmap_init(&flow_table->uuid_flow_table);
>   }
>   
>   void
>   ovn_desired_flow_table_clear(struct ovn_desired_flow_table *flow_table)
>   {
> -    struct ovn_flow *f, *next;
> -    HMAP_FOR_EACH_SAFE (f, next, match_hmap_node,
> -                        &flow_table->match_flow_table) {
> -        hmap_remove(&flow_table->match_flow_table, &f->match_hmap_node);
> -        hindex_remove(&flow_table->uuid_flow_table, &f->uuid_hindex_node);
> -        ovn_flow_destroy(f);
> +    struct sb_to_flow *stf, *next;
> +    HMAP_FOR_EACH_SAFE (stf, next, hmap_node,
> +                        &flow_table->uuid_flow_table) {
> +        remove_flows_from_sb_to_flow(flow_table, stf, NULL);
>       }
>   }
>   
> @@ -886,7 +1079,7 @@ ovn_desired_flow_table_destroy(struct ovn_desired_flow_table *flow_table)
>   {
>       ovn_desired_flow_table_clear(flow_table);
>       hmap_destroy(&flow_table->match_flow_table);
> -    hindex_destroy(&flow_table->uuid_flow_table);
> +    hmap_destroy(&flow_table->uuid_flow_table);
>   }
>   
>   static void
> @@ -1221,7 +1414,7 @@ ofctrl_put(struct ovn_desired_flow_table *flow_table,
>       struct ovn_flow *i, *next;
>       HMAP_FOR_EACH_SAFE (i, next, match_hmap_node, &installed_flows) {
>           struct ovn_flow *d = ovn_flow_lookup(&flow_table->match_flow_table,
> -                                             i, false);
> +                                             i, NULL);
>           if (!d) {
>               /* Installed flow is no longer desirable.  Delete it from the
>                * switch and from installed_flows. */
> @@ -1237,10 +1430,6 @@ ofctrl_put(struct ovn_desired_flow_table *flow_table,
>               hmap_remove(&installed_flows, &i->match_hmap_node);
>               ovn_flow_destroy(i);
>           } else {
> -            if (!uuid_equals(&i->sb_uuid, &d->sb_uuid)) {
> -                /* Update installed flow's UUID. */
> -                i->sb_uuid = d->sb_uuid;
> -            }
>               if (!ofpacts_equal(i->ofpacts, i->ofpacts_len,
>                                  d->ofpacts, d->ofpacts_len) ||
>                   i->cookie != d->cookie) {
> @@ -1277,7 +1466,7 @@ ofctrl_put(struct ovn_desired_flow_table *flow_table,
>        * in the installed flow table. */
>       struct ovn_flow *d;
>       HMAP_FOR_EACH (d, match_hmap_node, &flow_table->match_flow_table) {
> -        i = ovn_flow_lookup(&installed_flows, d, false);
> +        i = ovn_flow_lookup(&installed_flows, d, NULL);
>           if (!i) {
>               /* Send flow_mod to add flow. */
>               struct ofputil_flow_mod fm = {
> diff --git a/controller/ofctrl.h b/controller/ofctrl.h
> index 37f06db..8789ce4 100644
> --- a/controller/ofctrl.h
> +++ b/controller/ofctrl.h
> @@ -35,8 +35,9 @@ struct ovn_desired_flow_table {
>       /* Hash map flow table using flow match conditions as hash key.*/
>       struct hmap match_flow_table;
>   
> -    /* SB uuid index for the nodes in match_flow_table.*/
> -    struct hindex uuid_flow_table;
> +    /* SB uuid index for the cross reference nodes that link to the nodes in
> +     * match_flow_table.*/
> +    struct hmap uuid_flow_table;
>   };
>   
>   /* Interface for OVN main loop. */
> @@ -74,7 +75,30 @@ void ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows,
>                                  const struct ofpbuf *actions,
>                                  const struct uuid *sb_uuid);
>   
> -void ofctrl_remove_flows(struct ovn_desired_flow_table *, const struct uuid *);
> +/* Removes a bundles of flows from the flow table for a specific sb_uuid. The
> + * flows are removed only if they are not referenced by any other sb_uuid(s).
> + * For flood-removing all related flows referenced by other sb_uuid(s), use
> + * ofctrl_flood_remove_flows(). */
> +void ofctrl_remove_flows(struct ovn_desired_flow_table *,
> +                         const struct uuid *sb_uuid);
> +
> +/* The function ofctrl_flood_remove_flows flood-removes flows from the desired
> + * flow table for the sb_uuids provided in the flood_remove_nodes argument.
> + * For each given sb_uuid in flood_remove_nodes, it removes all the flows
> + * generated by the sb_uuid, and if any of the flows are referenced by another
> + * sb_uuid, it continues removing all the flows used by that sb_uuid as well,
> + * and so on, recursively.
> + *
> + * It adds all the sb_uuids that are actually removed in the
> + * flood_remove_nodes. */
> +struct ofctrl_flood_remove_node {
> +    struct hmap_node hmap_node;
> +    struct uuid sb_uuid;
> +};
> +void ofctrl_flood_remove_flows(struct ovn_desired_flow_table *,
> +                               struct hmap *flood_remove_nodes);
> +void ofctrl_flood_remove_add_node(struct hmap *flood_remove_nodes,
> +                                  const struct uuid *sb_uuid);
>   
>   void ovn_desired_flow_table_init(struct ovn_desired_flow_table *);
>   void ovn_desired_flow_table_clear(struct ovn_desired_flow_table *);
> @@ -86,6 +110,7 @@ void ofctrl_check_and_add_flow(struct ovn_desired_flow_table *,
>                                  const struct ofpbuf *ofpacts,
>                                  const struct uuid *, bool log_duplicate_flow);
>   
> +
>   bool ofctrl_is_connected(void);
>   void ofctrl_set_probe_interval(int probe_interval);
>   
> diff --git a/tests/ovn.at b/tests/ovn.at
> index d14f381..14def6e 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -13583,6 +13583,8 @@ AT_CHECK([cat 2.packets], [0], [expout])
>   
>   OVS_WAIT_UNTIL([test 9 = `as hv1 ovs-ofctl dump-flows br-int | \
>   grep conjunction | wc -l`])
> +OVS_WAIT_UNTIL([test 3 = `as hv1 ovs-ofctl dump-flows br-int | \
> +grep conjunction.*conjunction | wc -l`])
>   OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows br-int | \
>   grep conj_id | wc -l`])
>   
> @@ -13600,9 +13602,97 @@ sip=`ip_to_hex 10 0 0 4`
>   dip=`ip_to_hex 10 0 0 7`
>   
>   test_ip 1 f00000000001 f00000000002 $sip $dip
> +
>   $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets
>   AT_CHECK([cat 2.packets], [0], [])
>   
> +# Remove the first ACL, and verify that the conjunction flows are updated
> +# properly.
> +# There should be total of 6 flows present with conjunction action and 1 flow
> +# with conj match. Eg.
> +# table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop
> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(4,2/2)
> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(4,2/2)
> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(4,2/2)
> +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(4,1/2)
> +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(4,1/2)
> +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(4,1/2)
> +
> +ovn-nbctl acl-del ls1 to-lport 1001 \
> +'ip4 && ip4.src == $set1 && ip4.dst == $set1'
> +
> +OVS_WAIT_UNTIL([test 6 = `as hv1 ovs-ofctl dump-flows br-int | \
> +grep conjunction | wc -l`])
> +OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \
> +grep conjunction.*conjunction | wc -l`])
> +OVS_WAIT_UNTIL([test 1 = `as hv1 ovs-ofctl dump-flows br-int | \
> +grep conj_id | wc -l`])
> +
> +# Add the ACL back
> +ovn-nbctl acl-add ls1 to-lport 1001 \
> +'ip4 && ip4.src == $set1 && ip4.dst == $set1' allow
> +# Add one more ACL with more overlapping
> +ovn-nbctl acl-add ls1 to-lport 1001 \
> +'ip4 && ip4.src == $set1 && ip4.dst == {10.0.0.9, 10.0.0.10}' drop
> +
> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(4,1/2)
> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(4,1/2)
> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(5,1/2)
> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5 actions=conjunction(5,1/2)
> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(5,1/2)
> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(4,1/2),conjunction(6,1/2)
> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(6,1/2)
> +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(4,2/2),conjunction(5,2/2),conjunction(6,2/2)
> +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(4,2/2),conjunction(5,2/2),conjunction(6,2/2)
> +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(4,2/2),conjunction(5,2/2),conjunction(6,2/2)
> +
> +OVS_WAIT_UNTIL([test 10 = `as hv1 ovs-ofctl dump-flows br-int | \
> +grep conjunction | wc -l`])
> +OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \
> +grep conjunction.*conjunction | wc -l`])
> +OVS_WAIT_UNTIL([test 3 = `as hv1 ovs-ofctl dump-flows br-int | \
> +grep conjunction.*conjunction.*conjunction | wc -l`])
> +
> +# Remove 10.0.0.7 from address set2. All flows should be updated properly.
> +ovn-nbctl set Address_Set set2 \
> +addresses=\"10.0.0.8\",\"10.0.0.9\"
> +
> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(9,1/2)
> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(7,1/2)
> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(8,1/2)
> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5 actions=conjunction(9,1/2)
> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(7,1/2),conjunction(8,1/2)
> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(9,1/2)
> +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(7,2/2),conjunction(8,2/2),conjunction(9,2/2)
> +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(7,2/2),conjunction(8,2/2),conjunction(9,2/2)
> +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(7,2/2),conjunction(8,2/2),conjunction(9,2/2)
> +
> +OVS_WAIT_UNTIL([test 9 = `as hv1 ovs-ofctl dump-flows br-int | \
> +grep conjunction | wc -l`])
> +OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \
> +grep conjunction.*conjunction | wc -l`])
> +OVS_WAIT_UNTIL([test 3 = `as hv1 ovs-ofctl dump-flows br-int | \
> +grep conjunction.*conjunction.*conjunction | wc -l`])
> +
> +# Remove an ACL again
> +ovn-nbctl acl-del ls1 to-lport 1001 \
> +'ip4 && ip4.src == $set1 && ip4.dst == $set1'
> +
> +ovn-nbctl --wait=hv sync
> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(10,1/2)
> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(11,1/2)
> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(10,1/2),conjunction(11,1/2)
> +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(10,2/2),conjunction(11,2/2)
> +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(10,2/2),conjunction(11,2/2)
> +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(10,2/2),conjunction(11,2/2)
> +
> +OVS_WAIT_UNTIL([test 6 = `as hv1 ovs-ofctl dump-flows br-int | \
> +grep conjunction | wc -l`])
> +OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \
> +grep conjunction.*conjunction | wc -l`])
> +OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \
> +grep conjunction.*conjunction.*conjunction | wc -l`])
> +
>   OVN_CLEANUP([hv1])
>   AT_CLEANUP
>   
> 



More information about the dev mailing list