On Wed, Sep 2, 2020 at 8:53 AM Mark Michelson <mmich...@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?
>
> 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 <mmich...@redhat.com>
> > Signed-off-by: Han Zhou <hz...@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
> >
> >
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
