>
> 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 <[email protected] <mailto:[email protected]>>
> > Signed-off-by: Han Zhou <[email protected] <mailto:[email protected]>>
> > ---
> > 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
> >
> >
>
