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

Han Zhou hzhou at ovn.org
Thu Sep 3 21:30:12 UTC 2020


The previous email was blocked by ML because of the embedded picture, so I
replaced it with a link and resend.

On Thu, Sep 3, 2020 at 2:17 PM Han Zhou <hzhou at ovn.org> wrote:
>
>
>
> On Thu, Sep 3, 2020 at 6:00 AM Mark Michelson <mmichels at redhat.com> wrote:
>>
>> On 9/2/20 2:55 PM, Han Zhou wrote:
>> >
>> >
>> > On Wed, Sep 2, 2020 at 8:53 AM Mark Michelson <mmichels at redhat.com
>> > <mailto:mmichels at redhat.com>> wrote:
>> >  >
>> >  > 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.
>> >
>> > Thanks for the review. Sorry that I didn't get your point completely.
>> > You are right that currently there is only one scenario, the
>> > ofctrl_add_or_append_flow(), that will lead to one openflow being
>> > referenced by multiple SB lflows. However, to handle such case the data
>> > structure will need to be able to represent it. The new data structure
>> > with the sb_flow_ref that cross references between openflow rules and
SB
>> > objects is for this purpose. Due to the data structure change, the
>> > add/remove operations implementation are updated accordingly. It is
>> > general enough to handle both the *normal* situation and the *special*
>> > situation. Are you saying that we should have different data structures
>> > and different logic between *normal* and *special*? But that would only
>> > make the code more complex rather than simple, unnecessarily. Or did
you
>> > mean something else?
>>
>> My main concern was with the amount of overhead required for simple
>> cases. However, as you pointed out, if you special-case the conjunction
>> situation by using a different structure only in that case, you risk
>> having unmaintainable code.
>>
>> I have a secondary concern with the odd way that an sb_flow_ref could be
>> part of multiple lists, and how there is a circular reference between
>> sb_flow_ref and its contained ovn_flow. It is hard to visualize and
>> seems easy for someone to mess up later. I did think of an alternative
>> method for doing this. Here's how it would work:
>>
>> The desired flow table has a hashmap of sb_to_flow structures. The
>> sb_to_flow struct would look exactly as you have it now, only instead of
>> the "flows" field referring to a list of sb_flow_ref structures, they
>> would instead refer to a list of ovn_flow structures.
>>
>
> Thanks for clarifying. Unfortunately this wouldn't work, because a single
ovn_flow can be referenced by multiple SB flows, so each ovn_flow may need
to be in multiple such lists, which is impossible without a separate list
node structure (i.e. we can't link a ovn_flow struct in multiple lists with
just an embedded ovs_list struct in ovn_flow struct). This is why the
"flows" field in sb_to_flow struct needs to refer to a separate struct
"sb_flow_ref" that references ovn_flow struct, indirectly.
>
> The sb_flow_ref is in fact easy to visualize if you think of it as a link
between a pair of ovn_flow and SB uuid. Please see the below picture
(embedded in email) which is how I visualized it when designing this. It is
the most effective way I could think of so far to represent the M:N
relationship. If the picture helps, I can draw an ASCII version and
document it in the code.
>


https://docs.google.com/presentation/d/12sfhFvvPS3jDSe_9B6Sd5zSnICqje4YFvzQGLLEp9eE/edit?usp=sharing

>
>>
>> Then ovn_flow would be updated to have
>>      struct ovs_list sb_uuids;
>> in it.
>>
>> This would be a list of UUIDs:
>>
>> struct uuid_list {
>>     struct uuid uuid;
>>     struct ovs_list list_node;
>> };
>>
>
> Initially I was tying to do the same - creating separate nodes for uuid
list, but now that we already have sb_flow_ref nodes that reference
ovn_flows, instead of allocating extra nodes for uuid list, this
information is combined in sb_flow_ref node. This is more efficient because
only one node is required to link between a pair of ovn_flow and sb_uuid,
i.e. the lists of sb_uuids in each ovn_flow and the lists of ovn_flow
references in each sb_to_flow are merged, as dipicted in the above picture.
This model represents the links in a more straightforward way and also
saves 50% of the dynamic memory allocations. Does this address your
concerns?
>
> Thanks,
> Han
>
>>
>> This list of UUIDs on the ovn_flow allows for you to maintain a list of
>> all SB UUIDs that refer to this particular flow.
>>
>> So now the algorithm becomes:
>>
>> Add new ovn_flow:
>>     Create sb_to_flow for this SB UUID and add to hashmap.
>>     Create ovn_flow
>>     Add SB UUID to ovn_flow's sb_uuids list.
>>     Add ovn_flow to sb_to_flow list.
>>
>> Append to an ovn_flow:
>>     Create sb_to_flow for this SB UUID and add to hashmap.
>>     Find existing ovn_flow.
>>     Add SB UUID to existing ovn_flow's sb_uuids list.
>>
>> When lflow is modified or removed:
>>     Look up sb_to_flow structure for this lflow's UUID
>>     Iterate over the ovn_flows in the sb_to_flow.
>>     In the ovn_flow, remove the lflow's UUID from the list.
>>     Iterate over ovn_flow's sb_uuids, and recurse.
>>     Remove sb_to_flow from hashmap.
>>
>>
>> This approach gets rid of sb_flow_ref and the odd list structures. For
>> the nominal case where an ovn_flow has only one UUID in its sb_uuids
>> list, it should be just as efficient as what you've implemented. The
>> case of removing flows for conjunctive matches may be slightly slower
>> though. What do you think?
>>
>> >
>> >  >
>> >  > 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 <mailto:
mmichels at redhat.com>>
>> >  > > Signed-off-by: Han Zhou <hzhou at ovn.org <mailto:hzhou at ovn.org>>
>> >  > > ---
>> >  > >   controller/lflow.c  | 103 +++++++++++------
>> >  > >   controller/ofctrl.c | 319
>> > +++++++++++++++++++++++++++++++++++++++++-----------
>> >  > >   controller/ofctrl.h |  31 ++++-
>> >  > >   tests/ovn.at <http://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 <http://ovn.at> b/tests/ovn.at
>> > <http://ovn.at>
>> >  > > index d14f381..14def6e 100644
>> >  > > --- a/tests/ovn.at <http://ovn.at>
>> >  > > +++ b/tests/ovn.at <http://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 <http://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