[ovs-dev] ovn-controller: Avoid reprocessing same lflows in the same I-P run.

For I-P node lflow_output, different change handlers can trigger
reprocessing lflows. However, it is a waste to reprocess the same lflow
multiple times in the same run, because each processing of the lflow is
against the same input data.

For example, if a lflow references an addresset A and a port-group P, it
is possible that both A and P are changed in the same run, the same
lflow reprocess would be triggered by both
lflow_output_addr_sets_handler() and lflow_output_port_groups_handler().
Another example may incur a even bigger waste is that when a NB port-group
include ports across a big number of datapaths, so the lflow is
referencing a big number of SB port-groups with DP group including all
those DPs. When there are multiple port changes in the NB port-group,
there can be multiple small SB port-group changes, and so the lflows can
be reprocessed multiple times.

This patch avoid reprocessing the same lflow in the same I-P run, which
should improve performance in above scenarios.

There is only one exception when a lflow may still be reprocessed more
than once: if a lflow A is processed, which results in a compound
conjunction added to an existed flow generated by an exited lflow B, and
then lflow B needs to be reprocessed in the same run, which would
cause flood-remove and reprocess lflow A. In this case lflow A is
processed twice.

Apart from the performance gains, there is also a potential problem of
DP group fixed by this patch. If there is addrset/pg change and at the
same run there is also a new local datapath monitored, then the existed
code would firstly handle the addrset/pg changes causing a lflow being
processed against the DP group including the new DP, which could have
conjunction flows, but later the same lflow is reprocessed by
lflow_output_runtime_data_handler()->lflow_add_flows_for_datapath() for
the new DP only. Because lflows adding conjunction flows will not be
checked against redundancy but only tries to combine the conjunction
actions, it would result in redundanct conjunction actions added to the
same flow, which is also linked to the same SB lflow twice. The
mechanism of this patch will avoid this problem.

Signed-off-by: Han Zhou <hzhou@ovn.org>
---
 controller/lflow.c          | 143 +++++++++++++++++++++++++++++++-----
 controller/lflow.h          |   7 ++
 controller/ovn-controller.c |  18 ++++-
 3 files changed, 147 insertions(+), 21 deletions(-)

As far as I can tell, this looks correct.

Acked-by: Mark Michelson <mmichels@redhat.com>

On 1/20/22 13:06, Han Zhou wrote:
> For I-P node lflow_output, different change handlers can trigger
> reprocessing lflows. However, it is a waste to reprocess the same lflow
> multiple times in the same run, because each processing of the lflow is
> against the same input data.
> 
> For example, if a lflow references an addresset A and a port-group P, it
> is possible that both A and P are changed in the same run, the same
> lflow reprocess would be triggered by both
> lflow_output_addr_sets_handler() and lflow_output_port_groups_handler().
> Another example may incur a even bigger waste is that when a NB port-group
> include ports across a big number of datapaths, so the lflow is
> referencing a big number of SB port-groups with DP group including all
> those DPs. When there are multiple port changes in the NB port-group,
> there can be multiple small SB port-group changes, and so the lflows can
> be reprocessed multiple times.
> 
> This patch avoid reprocessing the same lflow in the same I-P run, which
> should improve performance in above scenarios.
> 
> There is only one exception when a lflow may still be reprocessed more
> than once: if a lflow A is processed, which results in a compound
> conjunction added to an existed flow generated by an exited lflow B, and
> then lflow B needs to be reprocessed in the same run, which would
> cause flood-remove and reprocess lflow A. In this case lflow A is
> processed twice.
> 
> Apart from the performance gains, there is also a potential problem of
> DP group fixed by this patch. If there is addrset/pg change and at the
> same run there is also a new local datapath monitored, then the existed
> code would firstly handle the addrset/pg changes causing a lflow being
> processed against the DP group including the new DP, which could have
> conjunction flows, but later the same lflow is reprocessed by
> lflow_output_runtime_data_handler()->lflow_add_flows_for_datapath() for
> the new DP only. Because lflows adding conjunction flows will not be
> checked against redundancy but only tries to combine the conjunction
> actions, it would result in redundanct conjunction actions added to the
> same flow, which is also linked to the same SB lflow twice. The
> mechanism of this patch will avoid this problem.
> 
> Signed-off-by: Han Zhou <hzhou@ovn.org>
> ---
>   controller/lflow.c          | 143 +++++++++++++++++++++++++++++++-----
>   controller/lflow.h          |   7 ++
>   controller/ovn-controller.c |  18 ++++-
>   3 files changed, 147 insertions(+), 21 deletions(-)
> 
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 933e2f3cc..b976c7d56 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -78,8 +78,16 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow,
>                         struct hmap *dhcp_opts, struct hmap *dhcpv6_opts,
>                         struct hmap *nd_ra_opts,
>                         struct controller_event_options *controller_event_opts,
> +                      bool is_recompute,
>                         struct lflow_ctx_in *l_ctx_in,
>                         struct lflow_ctx_out *l_ctx_out);
> +static struct lflow_processed_node *
> +lflows_processed_find(struct hmap *lflows_processed,
> +                      const struct uuid *lflow_uuid);
> +static void lflows_processed_add(struct hmap *lflows_processed,
> +                                 const struct uuid *lflow_uuid);
> +static void lflows_processed_remove(struct hmap *lflows_processed,
> +                                    struct lflow_processed_node *node);
>   static void lflow_resource_add(struct lflow_resource_ref *, enum ref_type,
>                                  const char *ref_name, const struct uuid *);
>   static struct ref_lflow_node *ref_lflow_lookup(struct hmap *ref_lflow_table,
> @@ -366,7 +374,7 @@ add_logical_flows(struct lflow_ctx_in *l_ctx_in,
>   
>       SBREC_LOGICAL_FLOW_TABLE_FOR_EACH (lflow, l_ctx_in->logical_flow_table) {
>           consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> -                              &nd_ra_opts, &controller_event_opts,
> +                              &nd_ra_opts, &controller_event_opts, true,
>                                 l_ctx_in, l_ctx_out);
>       }
>   
> @@ -413,6 +421,12 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in,
>       struct ofctrl_flood_remove_node *ofrn, *next;
>       SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow,
>                                                  l_ctx_in->logical_flow_table) {
> +        if (lflows_processed_find(l_ctx_out->lflows_processed,
> +                                  &lflow->header_.uuid)) {
> +            VLOG_DBG("lflow "UUID_FMT"has been processed, skip.",
> +                     UUID_ARGS(&lflow->header_.uuid));
> +            continue;
> +        }
>           VLOG_DBG("delete lflow "UUID_FMT, UUID_ARGS(&lflow->header_.uuid));
>           ofrn = xmalloc(sizeof *ofrn);
>           ofrn->sb_uuid = lflow->header_.uuid;
> @@ -437,8 +451,20 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in,
>           if (lflow) {
>               VLOG_DBG("re-add lflow "UUID_FMT,
>                        UUID_ARGS(&lflow->header_.uuid));
> +
> +            /* For the extra lflows that need to be reprocessed because of the
> +             * flood remove, remove it from lflows_processed. */
> +            struct lflow_processed_node *lfp_node =
> +                lflows_processed_find(l_ctx_out->lflows_processed,
> +                                      &lflow->header_.uuid);
> +            if (lfp_node) {
> +                VLOG_DBG("lflow "UUID_FMT"has been processed, now reprocess.",
> +                         UUID_ARGS(&lflow->header_.uuid));
> +                lflows_processed_remove(l_ctx_out->lflows_processed, lfp_node);
> +            }
> +
>               consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> -                                  &nd_ra_opts, &controller_event_opts,
> +                                  &nd_ra_opts, &controller_event_opts, false,
>                                     l_ctx_in, l_ctx_out);
>           }
>       }
> @@ -473,15 +499,22 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name,
>       *changed = false;
>       bool ret = true;
>   
> -    hmap_remove(&l_ctx_out->lfrr->ref_lflow_table, &rlfn->node);
> +    struct ovs_list lflows_todo = OVS_LIST_INITIALIZER(&lflows_todo);
>   
> -    struct lflow_ref_list_node *lrln, *next;
> -    /* Detach the rlfn->lflow_uuids nodes from the lfrr table and clean
> -     * up all other nodes related to the lflows that uses the resource,
> -     * so that the old nodes won't interfere with updating the lfrr table
> -     * when reparsing the lflows. */
> +    struct lflow_ref_list_node *lrln, *lrln_uuid, *lrln_uuid_next;
>       HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) {
> -        ovs_list_remove(&lrln->list_node);
> +        if (lflows_processed_find(l_ctx_out->lflows_processed,
> +                                  &lrln->lflow_uuid)) {
> +            continue;
> +        }
> +        /* Use lflow_ref_list_node as list node to store the uuid.
> +         * Other fields are not used here. */
> +        lrln_uuid = xmalloc(sizeof *lrln_uuid);
> +        lrln_uuid->lflow_uuid = lrln->lflow_uuid;
> +        ovs_list_push_back(&lflows_todo, &lrln_uuid->list_node);
> +    }
> +    if (ovs_list_is_empty(&lflows_todo)) {
> +        return true;
>       }
>   
>       struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
> @@ -509,17 +542,19 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name,
>       /* 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;
> -    HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) {
> +    LIST_FOR_EACH_SAFE (lrln_uuid, lrln_uuid_next, list_node, &lflows_todo) {
>           VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type: %d,"
>                    " name: %s.",
> -                 UUID_ARGS(&lrln->lflow_uuid),
> +                 UUID_ARGS(&lrln_uuid->lflow_uuid),
>                    ref_type, ref_name);
> -        ofctrl_flood_remove_add_node(&flood_remove_nodes, &lrln->lflow_uuid);
> +        ofctrl_flood_remove_add_node(&flood_remove_nodes,
> +                                     &lrln_uuid->lflow_uuid);
> +        free(lrln_uuid);
>       }
>       ofctrl_flood_remove_flows(l_ctx_out->flow_table, &flood_remove_nodes);
>   
>       /* Secondly, for each lflow that is actually removed, reprocessing it. */
> +    struct ofctrl_flood_remove_node *ofrn, *ofrn_next;
>       HMAP_FOR_EACH (ofrn, hmap_node, &flood_remove_nodes) {
>           lflow_resource_destroy_lflow(l_ctx_out->lfrr, &ofrn->sb_uuid);
>           lflow_conj_ids_free(l_ctx_out->conj_ids, &ofrn->sb_uuid);
> @@ -535,8 +570,19 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name,
>               continue;
>           }
>   
> +        /* For the extra lflows that need to be reprocessed because of the
> +         * flood remove, remove it from lflows_processed. */
> +        struct lflow_processed_node *lfp_node =
> +            lflows_processed_find(l_ctx_out->lflows_processed,
> +                                  &lflow->header_.uuid);
> +        if (lfp_node) {
> +            VLOG_DBG("lflow "UUID_FMT"has been processed, now reprocess.",
> +                     UUID_ARGS(&lflow->header_.uuid));
> +            lflows_processed_remove(l_ctx_out->lflows_processed, lfp_node);
> +        }
> +
>           consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> -                              &nd_ra_opts, &controller_event_opts,
> +                              &nd_ra_opts, &controller_event_opts, false,
>                                 l_ctx_in, l_ctx_out);
>           *changed = true;
>       }
> @@ -546,12 +592,6 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name,
>       }
>       hmap_destroy(&flood_remove_nodes);
>   
> -    HMAP_FOR_EACH_SAFE (lrln, next, hmap_node, &rlfn->lflow_uuids) {
> -        hmap_remove(&rlfn->lflow_uuids, &lrln->hmap_node);
> -        free(lrln);
> -    }
> -    ref_lflow_node_destroy(rlfn);
> -
>       dhcp_opts_destroy(&dhcp_opts);
>       dhcp_opts_destroy(&dhcpv6_opts);
>       nd_ra_opts_destroy(&nd_ra_opts);
> @@ -969,11 +1009,54 @@ done:
>       free(matches);
>   }
>   
> +static struct lflow_processed_node *
> +lflows_processed_find(struct hmap *lflows_processed,
> +                      const struct uuid *lflow_uuid)
> +{
> +    struct lflow_processed_node *node;
> +    HMAP_FOR_EACH_WITH_HASH (node, hmap_node, uuid_hash(lflow_uuid),
> +                             lflows_processed) {
> +        if (uuid_equals(&node->lflow_uuid, lflow_uuid)) {
> +            return node;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static void
> +lflows_processed_add(struct hmap *lflows_processed,
> +                     const struct uuid *lflow_uuid)
> +{
> +    struct lflow_processed_node *node = xmalloc(sizeof *node);
> +    node->lflow_uuid = *lflow_uuid;
> +    hmap_insert(lflows_processed, &node->hmap_node, uuid_hash(lflow_uuid));
> +}
> +
> +static void
> +lflows_processed_remove(struct hmap *lflows_processed,
> +                        struct lflow_processed_node *node)
> +{
> +    hmap_remove(lflows_processed, &node->hmap_node);
> +    free(node);
> +}
> +
> +void
> +lflows_processed_destroy(struct hmap *lflows_processed)
> +{
> +    struct lflow_processed_node *node, *next;
> +    HMAP_FOR_EACH_SAFE (node, next, hmap_node, lflows_processed) {
> +        hmap_remove(lflows_processed, &node->hmap_node);
> +        free(node);
> +    }
> +    hmap_destroy(lflows_processed);
> +}
> +
>   static void
>   consider_logical_flow(const struct sbrec_logical_flow *lflow,
>                         struct hmap *dhcp_opts, struct hmap *dhcpv6_opts,
>                         struct hmap *nd_ra_opts,
>                         struct controller_event_options *controller_event_opts,
> +                      bool is_recompute,
>                         struct lflow_ctx_in *l_ctx_in,
>                         struct lflow_ctx_out *l_ctx_out)
>   {
> @@ -987,6 +1070,13 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow,
>       }
>       ovs_assert(!dp_group || !dp);
>   
> +    if (!is_recompute) {
> +        ovs_assert(!lflows_processed_find(l_ctx_out->lflows_processed,
> +                                          &lflow->header_.uuid));
> +        lflows_processed_add(l_ctx_out->lflows_processed,
> +                             &lflow->header_.uuid);
> +    }
> +
>       if (dp) {
>           consider_logical_flow__(lflow, dp,
>                                   dhcp_opts, dhcpv6_opts, nd_ra_opts,
> @@ -1923,6 +2013,12 @@ lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp,
>       const struct sbrec_logical_flow *lflow;
>       SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL (
>           lflow, lf_row, l_ctx_in->sbrec_logical_flow_by_logical_datapath) {
> +        if (lflows_processed_find(l_ctx_out->lflows_processed,
> +                                  &lflow->header_.uuid)) {
> +            continue;
> +        }
> +        lflows_processed_add(l_ctx_out->lflows_processed,
> +                             &lflow->header_.uuid);
>           consider_logical_flow__(lflow, dp, &dhcp_opts, &dhcpv6_opts,
>                                   &nd_ra_opts, &controller_event_opts,
>                                   l_ctx_in, l_ctx_out);
> @@ -1949,6 +2045,13 @@ lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp,
>           sbrec_logical_flow_index_set_logical_dp_group(lf_row, ldpg);
>           SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL (
>               lflow, lf_row, l_ctx_in->sbrec_logical_flow_by_logical_dp_group) {
> +            if (lflows_processed_find(l_ctx_out->lflows_processed,
> +                                      &lflow->header_.uuid)) {
> +                continue;
> +            }
> +            /* Don't call lflows_processed_add() because here we process the
> +             * lflow only for one of the DPs in the DP group, which may be
> +             * incomplete. */
>               consider_logical_flow__(lflow, dp, &dhcp_opts, &dhcpv6_opts,
>                                       &nd_ra_opts, &controller_event_opts,
>                                       l_ctx_in, l_ctx_out);
> diff --git a/controller/lflow.h b/controller/lflow.h
> index 489dd70fb..d6dad17ec 100644
> --- a/controller/lflow.h
> +++ b/controller/lflow.h
> @@ -159,10 +159,17 @@ struct lflow_ctx_out {
>       struct lflow_resource_ref *lfrr;
>       struct lflow_cache *lflow_cache;
>       struct conj_ids *conj_ids;
> +    struct hmap *lflows_processed;
>       struct simap *hairpin_lb_ids;
>       struct id_pool *hairpin_id_pool;
>   };
>   
> +struct lflow_processed_node {
> +    struct hmap_node hmap_node; /* In ed_type_lflow_output.lflows_processed. */
> +    struct uuid lflow_uuid;
> +};
> +void lflows_processed_destroy(struct hmap *lflows_processed);
> +
>   void lflow_init(void);
>   void lflow_run(struct lflow_ctx_in *, struct lflow_ctx_out *);
>   void lflow_handle_cached_flows(struct lflow_cache *,
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 5069aedfc..8631bccbc 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -2176,6 +2176,11 @@ struct ed_type_lflow_output {
>       /* conjunciton ID usage information of lflows */
>       struct conj_ids conj_ids;
>   
> +    /* lflows processed in the current engine execution.
> +     * Cleared by en_lflow_output_clear_tracked_data before each engine
> +     * execution. */
> +    struct hmap lflows_processed;
> +
>       /* Data which is persistent and not cleared during
>        * full recompute. */
>       struct lflow_output_persistent_data pd;
> @@ -2307,6 +2312,7 @@ init_lflow_ctx(struct engine_node *node,
>       l_ctx_out->meter_table = &fo->meter_table;
>       l_ctx_out->lfrr = &fo->lflow_resource_ref;
>       l_ctx_out->conj_ids = &fo->conj_ids;
> +    l_ctx_out->lflows_processed = &fo->lflows_processed;
>       l_ctx_out->lflow_cache = fo->pd.lflow_cache;
>       l_ctx_out->hairpin_id_pool = fo->hd.pool;
>       l_ctx_out->hairpin_lb_ids = &fo->hd.ids;
> @@ -2322,11 +2328,20 @@ en_lflow_output_init(struct engine_node *node OVS_UNUSED,
>       ovn_extend_table_init(&data->meter_table);
>       lflow_resource_init(&data->lflow_resource_ref);
>       lflow_conj_ids_init(&data->conj_ids);
> +    hmap_init(&data->lflows_processed);
>       simap_init(&data->hd.ids);
>       data->hd.pool = id_pool_create(1, UINT32_MAX - 1);
>       return data;
>   }
>   
> +static void
> +en_lflow_output_clear_tracked_data(void *data)
> +{
> +    struct ed_type_lflow_output *flow_output_data = data;
> +    lflows_processed_destroy(&flow_output_data->lflows_processed);
> +    hmap_init(&flow_output_data->lflows_processed);
> +}
> +
>   static void
>   en_lflow_output_cleanup(void *data)
>   {
> @@ -2336,6 +2351,7 @@ en_lflow_output_cleanup(void *data)
>       ovn_extend_table_destroy(&flow_output_data->meter_table);
>       lflow_resource_destroy(&flow_output_data->lflow_resource_ref);
>       lflow_conj_ids_destroy(&flow_output_data->conj_ids);
> +    lflows_processed_destroy(&flow_output_data->lflows_processed);
>       lflow_cache_destroy(flow_output_data->pd.lflow_cache);
>       simap_destroy(&flow_output_data->hd.ids);
>       id_pool_destroy(flow_output_data->hd.pool);
> @@ -3213,7 +3229,7 @@ main(int argc, char *argv[])
>       ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve");
>       ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected");
>       ENGINE_NODE(pflow_output, "physical_flow_output");
> -    ENGINE_NODE(lflow_output, "logical_flow_output");
> +    ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lflow_output, "logical_flow_output");
>       ENGINE_NODE(flow_output, "flow_output");
>       ENGINE_NODE(addr_sets, "addr_sets");
>       ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_groups, "port_groups");
>

On Fri, Feb 4, 2022 at 7:58 AM Mark Michelson <mmichels@redhat.com> wrote:
>
> As far as I can tell, this looks correct.
>
> Acked-by: Mark Michelson <mmichels@redhat.com>

Thanks Mark. I applied it to the main branch.
>
> On 1/20/22 13:06, Han Zhou wrote:
> > For I-P node lflow_output, different change handlers can trigger
> > reprocessing lflows. However, it is a waste to reprocess the same lflow
> > multiple times in the same run, because each processing of the lflow is
> > against the same input data.
> >
> > For example, if a lflow references an addresset A and a port-group P, it
> > is possible that both A and P are changed in the same run, the same
> > lflow reprocess would be triggered by both
> > lflow_output_addr_sets_handler() and lflow_output_port_groups_handler().
> > Another example may incur a even bigger waste is that when a NB
port-group
> > include ports across a big number of datapaths, so the lflow is
> > referencing a big number of SB port-groups with DP group including all
> > those DPs. When there are multiple port changes in the NB port-group,
> > there can be multiple small SB port-group changes, and so the lflows can
> > be reprocessed multiple times.
> >
> > This patch avoid reprocessing the same lflow in the same I-P run, which
> > should improve performance in above scenarios.
> >
> > There is only one exception when a lflow may still be reprocessed more
> > than once: if a lflow A is processed, which results in a compound
> > conjunction added to an existed flow generated by an exited lflow B, and
> > then lflow B needs to be reprocessed in the same run, which would
> > cause flood-remove and reprocess lflow A. In this case lflow A is
> > processed twice.
> >
> > Apart from the performance gains, there is also a potential problem of
> > DP group fixed by this patch. If there is addrset/pg change and at the
> > same run there is also a new local datapath monitored, then the existed
> > code would firstly handle the addrset/pg changes causing a lflow being
> > processed against the DP group including the new DP, which could have
> > conjunction flows, but later the same lflow is reprocessed by
> > lflow_output_runtime_data_handler()->lflow_add_flows_for_datapath() for
> > the new DP only. Because lflows adding conjunction flows will not be
> > checked against redundancy but only tries to combine the conjunction
> > actions, it would result in redundanct conjunction actions added to the
> > same flow, which is also linked to the same SB lflow twice. The
> > mechanism of this patch will avoid this problem.
> >
> > Signed-off-by: Han Zhou <hzhou@ovn.org>
> > ---
> >   controller/lflow.c          | 143 +++++++++++++++++++++++++++++++-----
> >   controller/lflow.h          |   7 ++
> >   controller/ovn-controller.c |  18 ++++-
> >   3 files changed, 147 insertions(+), 21 deletions(-)
> >
> > diff --git a/controller/lflow.c b/controller/lflow.c
> > index 933e2f3cc..b976c7d56 100644
> > --- a/controller/lflow.c
> > +++ b/controller/lflow.c
> > @@ -78,8 +78,16 @@ consider_logical_flow(const struct
sbrec_logical_flow *lflow,
> >                         struct hmap *dhcp_opts, struct hmap
*dhcpv6_opts,
> >                         struct hmap *nd_ra_opts,
> >                         struct controller_event_options
*controller_event_opts,
> > +                      bool is_recompute,
> >                         struct lflow_ctx_in *l_ctx_in,
> >                         struct lflow_ctx_out *l_ctx_out);
> > +static struct lflow_processed_node *
> > +lflows_processed_find(struct hmap *lflows_processed,
> > +                      const struct uuid *lflow_uuid);
> > +static void lflows_processed_add(struct hmap *lflows_processed,
> > +                                 const struct uuid *lflow_uuid);
> > +static void lflows_processed_remove(struct hmap *lflows_processed,
> > +                                    struct lflow_processed_node *node);
> >   static void lflow_resource_add(struct lflow_resource_ref *, enum
ref_type,
> >                                  const char *ref_name, const struct
uuid *);
> >   static struct ref_lflow_node *ref_lflow_lookup(struct hmap
*ref_lflow_table,
> > @@ -366,7 +374,7 @@ add_logical_flows(struct lflow_ctx_in *l_ctx_in,
> >
> >       SBREC_LOGICAL_FLOW_TABLE_FOR_EACH (lflow,
l_ctx_in->logical_flow_table) {
> >           consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> > -                              &nd_ra_opts, &controller_event_opts,
> > +                              &nd_ra_opts, &controller_event_opts,
true,
> >                                 l_ctx_in, l_ctx_out);
> >       }
> >
> > @@ -413,6 +421,12 @@ lflow_handle_changed_flows(struct lflow_ctx_in
*l_ctx_in,
> >       struct ofctrl_flood_remove_node *ofrn, *next;
> >       SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow,
> >
 l_ctx_in->logical_flow_table) {
> > +        if (lflows_processed_find(l_ctx_out->lflows_processed,
> > +                                  &lflow->header_.uuid)) {
> > +            VLOG_DBG("lflow "UUID_FMT"has been processed, skip.",
> > +                     UUID_ARGS(&lflow->header_.uuid));
> > +            continue;
> > +        }
> >           VLOG_DBG("delete lflow "UUID_FMT,
UUID_ARGS(&lflow->header_.uuid));
> >           ofrn = xmalloc(sizeof *ofrn);
> >           ofrn->sb_uuid = lflow->header_.uuid;
> > @@ -437,8 +451,20 @@ lflow_handle_changed_flows(struct lflow_ctx_in
*l_ctx_in,
> >           if (lflow) {
> >               VLOG_DBG("re-add lflow "UUID_FMT,
> >                        UUID_ARGS(&lflow->header_.uuid));
> > +
> > +            /* For the extra lflows that need to be reprocessed
because of the
> > +             * flood remove, remove it from lflows_processed. */
> > +            struct lflow_processed_node *lfp_node =
> > +                lflows_processed_find(l_ctx_out->lflows_processed,
> > +                                      &lflow->header_.uuid);
> > +            if (lfp_node) {
> > +                VLOG_DBG("lflow "UUID_FMT"has been processed, now
reprocess.",
> > +                         UUID_ARGS(&lflow->header_.uuid));
> > +                lflows_processed_remove(l_ctx_out->lflows_processed,
lfp_node);
> > +            }
> > +
> >               consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> > -                                  &nd_ra_opts, &controller_event_opts,
> > +                                  &nd_ra_opts, &controller_event_opts,
false,
> >                                     l_ctx_in, l_ctx_out);
> >           }
> >       }
> > @@ -473,15 +499,22 @@ lflow_handle_changed_ref(enum ref_type ref_type,
const char *ref_name,
> >       *changed = false;
> >       bool ret = true;
> >
> > -    hmap_remove(&l_ctx_out->lfrr->ref_lflow_table, &rlfn->node);
> > +    struct ovs_list lflows_todo = OVS_LIST_INITIALIZER(&lflows_todo);
> >
> > -    struct lflow_ref_list_node *lrln, *next;
> > -    /* Detach the rlfn->lflow_uuids nodes from the lfrr table and clean
> > -     * up all other nodes related to the lflows that uses the resource,
> > -     * so that the old nodes won't interfere with updating the lfrr
table
> > -     * when reparsing the lflows. */
> > +    struct lflow_ref_list_node *lrln, *lrln_uuid, *lrln_uuid_next;
> >       HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) {
> > -        ovs_list_remove(&lrln->list_node);
> > +        if (lflows_processed_find(l_ctx_out->lflows_processed,
> > +                                  &lrln->lflow_uuid)) {
> > +            continue;
> > +        }
> > +        /* Use lflow_ref_list_node as list node to store the uuid.
> > +         * Other fields are not used here. */
> > +        lrln_uuid = xmalloc(sizeof *lrln_uuid);
> > +        lrln_uuid->lflow_uuid = lrln->lflow_uuid;
> > +        ovs_list_push_back(&lflows_todo, &lrln_uuid->list_node);
> > +    }
> > +    if (ovs_list_is_empty(&lflows_todo)) {
> > +        return true;
> >       }
> >
> >       struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
> > @@ -509,17 +542,19 @@ lflow_handle_changed_ref(enum ref_type ref_type,
const char *ref_name,
> >       /* 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;
> > -    HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) {
> > +    LIST_FOR_EACH_SAFE (lrln_uuid, lrln_uuid_next, list_node,
&lflows_todo) {
> >           VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type: %d,"
> >                    " name: %s.",
> > -                 UUID_ARGS(&lrln->lflow_uuid),
> > +                 UUID_ARGS(&lrln_uuid->lflow_uuid),
> >                    ref_type, ref_name);
> > -        ofctrl_flood_remove_add_node(&flood_remove_nodes,
&lrln->lflow_uuid);
> > +        ofctrl_flood_remove_add_node(&flood_remove_nodes,
> > +                                     &lrln_uuid->lflow_uuid);
> > +        free(lrln_uuid);
> >       }
> >       ofctrl_flood_remove_flows(l_ctx_out->flow_table,
&flood_remove_nodes);
> >
> >       /* Secondly, for each lflow that is actually removed,
reprocessing it. */
> > +    struct ofctrl_flood_remove_node *ofrn, *ofrn_next;
> >       HMAP_FOR_EACH (ofrn, hmap_node, &flood_remove_nodes) {
> >           lflow_resource_destroy_lflow(l_ctx_out->lfrr, &ofrn->sb_uuid);
> >           lflow_conj_ids_free(l_ctx_out->conj_ids, &ofrn->sb_uuid);
> > @@ -535,8 +570,19 @@ lflow_handle_changed_ref(enum ref_type ref_type,
const char *ref_name,
> >               continue;
> >           }
> >
> > +        /* For the extra lflows that need to be reprocessed because of
the
> > +         * flood remove, remove it from lflows_processed. */
> > +        struct lflow_processed_node *lfp_node =
> > +            lflows_processed_find(l_ctx_out->lflows_processed,
> > +                                  &lflow->header_.uuid);
> > +        if (lfp_node) {
> > +            VLOG_DBG("lflow "UUID_FMT"has been processed, now
reprocess.",
> > +                     UUID_ARGS(&lflow->header_.uuid));
> > +            lflows_processed_remove(l_ctx_out->lflows_processed,
lfp_node);
> > +        }
> > +
> >           consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> > -                              &nd_ra_opts, &controller_event_opts,
> > +                              &nd_ra_opts, &controller_event_opts,
false,
> >                                 l_ctx_in, l_ctx_out);
> >           *changed = true;
> >       }
> > @@ -546,12 +592,6 @@ lflow_handle_changed_ref(enum ref_type ref_type,
const char *ref_name,
> >       }
> >       hmap_destroy(&flood_remove_nodes);
> >
> > -    HMAP_FOR_EACH_SAFE (lrln, next, hmap_node, &rlfn->lflow_uuids) {
> > -        hmap_remove(&rlfn->lflow_uuids, &lrln->hmap_node);
> > -        free(lrln);
> > -    }
> > -    ref_lflow_node_destroy(rlfn);
> > -
> >       dhcp_opts_destroy(&dhcp_opts);
> >       dhcp_opts_destroy(&dhcpv6_opts);
> >       nd_ra_opts_destroy(&nd_ra_opts);
> > @@ -969,11 +1009,54 @@ done:
> >       free(matches);
> >   }
> >
> > +static struct lflow_processed_node *
> > +lflows_processed_find(struct hmap *lflows_processed,
> > +                      const struct uuid *lflow_uuid)
> > +{
> > +    struct lflow_processed_node *node;
> > +    HMAP_FOR_EACH_WITH_HASH (node, hmap_node, uuid_hash(lflow_uuid),
> > +                             lflows_processed) {
> > +        if (uuid_equals(&node->lflow_uuid, lflow_uuid)) {
> > +            return node;
> > +        }
> > +    }
> > +    return NULL;
> > +}
> > +
> > +static void
> > +lflows_processed_add(struct hmap *lflows_processed,
> > +                     const struct uuid *lflow_uuid)
> > +{
> > +    struct lflow_processed_node *node = xmalloc(sizeof *node);
> > +    node->lflow_uuid = *lflow_uuid;
> > +    hmap_insert(lflows_processed, &node->hmap_node,
uuid_hash(lflow_uuid));
> > +}
> > +
> > +static void
> > +lflows_processed_remove(struct hmap *lflows_processed,
> > +                        struct lflow_processed_node *node)
> > +{
> > +    hmap_remove(lflows_processed, &node->hmap_node);
> > +    free(node);
> > +}
> > +
> > +void
> > +lflows_processed_destroy(struct hmap *lflows_processed)
> > +{
> > +    struct lflow_processed_node *node, *next;
> > +    HMAP_FOR_EACH_SAFE (node, next, hmap_node, lflows_processed) {
> > +        hmap_remove(lflows_processed, &node->hmap_node);
> > +        free(node);
> > +    }
> > +    hmap_destroy(lflows_processed);
> > +}
> > +
> >   static void
> >   consider_logical_flow(const struct sbrec_logical_flow *lflow,
> >                         struct hmap *dhcp_opts, struct hmap
*dhcpv6_opts,
> >                         struct hmap *nd_ra_opts,
> >                         struct controller_event_options
*controller_event_opts,
> > +                      bool is_recompute,
> >                         struct lflow_ctx_in *l_ctx_in,
> >                         struct lflow_ctx_out *l_ctx_out)
> >   {
> > @@ -987,6 +1070,13 @@ consider_logical_flow(const struct
sbrec_logical_flow *lflow,
> >       }
> >       ovs_assert(!dp_group || !dp);
> >
> > +    if (!is_recompute) {
> > +        ovs_assert(!lflows_processed_find(l_ctx_out->lflows_processed,
> > +                                          &lflow->header_.uuid));
> > +        lflows_processed_add(l_ctx_out->lflows_processed,
> > +                             &lflow->header_.uuid);
> > +    }
> > +
> >       if (dp) {
> >           consider_logical_flow__(lflow, dp,
> >                                   dhcp_opts, dhcpv6_opts, nd_ra_opts,
> > @@ -1923,6 +2013,12 @@ lflow_add_flows_for_datapath(const struct
sbrec_datapath_binding *dp,
> >       const struct sbrec_logical_flow *lflow;
> >       SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL (
> >           lflow, lf_row,
l_ctx_in->sbrec_logical_flow_by_logical_datapath) {
> > +        if (lflows_processed_find(l_ctx_out->lflows_processed,
> > +                                  &lflow->header_.uuid)) {
> > +            continue;
> > +        }
> > +        lflows_processed_add(l_ctx_out->lflows_processed,
> > +                             &lflow->header_.uuid);
> >           consider_logical_flow__(lflow, dp, &dhcp_opts, &dhcpv6_opts,
> >                                   &nd_ra_opts, &controller_event_opts,
> >                                   l_ctx_in, l_ctx_out);
> > @@ -1949,6 +2045,13 @@ lflow_add_flows_for_datapath(const struct
sbrec_datapath_binding *dp,
> >           sbrec_logical_flow_index_set_logical_dp_group(lf_row, ldpg);
> >           SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL (
> >               lflow, lf_row,
l_ctx_in->sbrec_logical_flow_by_logical_dp_group) {
> > +            if (lflows_processed_find(l_ctx_out->lflows_processed,
> > +                                      &lflow->header_.uuid)) {
> > +                continue;
> > +            }
> > +            /* Don't call lflows_processed_add() because here we
process the
> > +             * lflow only for one of the DPs in the DP group, which
may be
> > +             * incomplete. */
> >               consider_logical_flow__(lflow, dp, &dhcp_opts,
&dhcpv6_opts,
> >                                       &nd_ra_opts,
&controller_event_opts,
> >                                       l_ctx_in, l_ctx_out);
> > diff --git a/controller/lflow.h b/controller/lflow.h
> > index 489dd70fb..d6dad17ec 100644
> > --- a/controller/lflow.h
> > +++ b/controller/lflow.h
> > @@ -159,10 +159,17 @@ struct lflow_ctx_out {
> >       struct lflow_resource_ref *lfrr;
> >       struct lflow_cache *lflow_cache;
> >       struct conj_ids *conj_ids;
> > +    struct hmap *lflows_processed;
> >       struct simap *hairpin_lb_ids;
> >       struct id_pool *hairpin_id_pool;
> >   };
> >
> > +struct lflow_processed_node {
> > +    struct hmap_node hmap_node; /* In
ed_type_lflow_output.lflows_processed. */
> > +    struct uuid lflow_uuid;
> > +};
> > +void lflows_processed_destroy(struct hmap *lflows_processed);
> > +
> >   void lflow_init(void);
> >   void lflow_run(struct lflow_ctx_in *, struct lflow_ctx_out *);
> >   void lflow_handle_cached_flows(struct lflow_cache *,
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 5069aedfc..8631bccbc 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -2176,6 +2176,11 @@ struct ed_type_lflow_output {
> >       /* conjunciton ID usage information of lflows */
> >       struct conj_ids conj_ids;
> >
> > +    /* lflows processed in the current engine execution.
> > +     * Cleared by en_lflow_output_clear_tracked_data before each engine
> > +     * execution. */
> > +    struct hmap lflows_processed;
> > +
> >       /* Data which is persistent and not cleared during
> >        * full recompute. */
> >       struct lflow_output_persistent_data pd;
> > @@ -2307,6 +2312,7 @@ init_lflow_ctx(struct engine_node *node,
> >       l_ctx_out->meter_table = &fo->meter_table;
> >       l_ctx_out->lfrr = &fo->lflow_resource_ref;
> >       l_ctx_out->conj_ids = &fo->conj_ids;
> > +    l_ctx_out->lflows_processed = &fo->lflows_processed;
> >       l_ctx_out->lflow_cache = fo->pd.lflow_cache;
> >       l_ctx_out->hairpin_id_pool = fo->hd.pool;
> >       l_ctx_out->hairpin_lb_ids = &fo->hd.ids;
> > @@ -2322,11 +2328,20 @@ en_lflow_output_init(struct engine_node *node
OVS_UNUSED,
> >       ovn_extend_table_init(&data->meter_table);
> >       lflow_resource_init(&data->lflow_resource_ref);
> >       lflow_conj_ids_init(&data->conj_ids);
> > +    hmap_init(&data->lflows_processed);
> >       simap_init(&data->hd.ids);
> >       data->hd.pool = id_pool_create(1, UINT32_MAX - 1);
> >       return data;
> >   }
> >
> > +static void
> > +en_lflow_output_clear_tracked_data(void *data)
> > +{
> > +    struct ed_type_lflow_output *flow_output_data = data;
> > +    lflows_processed_destroy(&flow_output_data->lflows_processed);
> > +    hmap_init(&flow_output_data->lflows_processed);
> > +}
> > +
> >   static void
> >   en_lflow_output_cleanup(void *data)
> >   {
> > @@ -2336,6 +2351,7 @@ en_lflow_output_cleanup(void *data)
> >       ovn_extend_table_destroy(&flow_output_data->meter_table);
> >       lflow_resource_destroy(&flow_output_data->lflow_resource_ref);
> >       lflow_conj_ids_destroy(&flow_output_data->conj_ids);
> > +    lflows_processed_destroy(&flow_output_data->lflows_processed);
> >       lflow_cache_destroy(flow_output_data->pd.lflow_cache);
> >       simap_destroy(&flow_output_data->hd.ids);
> >       id_pool_destroy(flow_output_data->hd.pool);
> > @@ -3213,7 +3229,7 @@ main(int argc, char *argv[])
> >       ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve");
> >       ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected");
> >       ENGINE_NODE(pflow_output, "physical_flow_output");
> > -    ENGINE_NODE(lflow_output, "logical_flow_output");
> > +    ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lflow_output,
"logical_flow_output");
> >       ENGINE_NODE(flow_output, "flow_output");
> >       ENGINE_NODE(addr_sets, "addr_sets");
> >       ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_groups, "port_groups");
> >
>

On Sat, Feb 5, 2022 at 12:12 AM Han Zhou <hzhou@ovn.org> wrote:
>
> On Fri, Feb 4, 2022 at 7:58 AM Mark Michelson <mmichels@redhat.com> wrote:
> >
> > As far as I can tell, this looks correct.
> >
> > Acked-by: Mark Michelson <mmichels@redhat.com>
>
> Thanks Mark. I applied it to the main branch.


Hi Han,

We have an issue in OVN 21.12 (and OVN 21.09) where some of the
conjunction flows are missing and recompute resolves the issue.

And applying this patch to branch-21.12 fixes the issue.

I need to dig further.  But before that I want to check with you if
you have any comments on this.

The details of the issue can be found here -
https://bugzilla.redhat.com/show_bug.cgi?id=2071272#c12

I'm just wondering if backporting this commit would hide the real issue or not.

Thanks
Numan

> >
> > On 1/20/22 13:06, Han Zhou wrote:
> > > For I-P node lflow_output, different change handlers can trigger
> > > reprocessing lflows. However, it is a waste to reprocess the same lflow
> > > multiple times in the same run, because each processing of the lflow is
> > > against the same input data.
> > >
> > > For example, if a lflow references an addresset A and a port-group P, it
> > > is possible that both A and P are changed in the same run, the same
> > > lflow reprocess would be triggered by both
> > > lflow_output_addr_sets_handler() and lflow_output_port_groups_handler().
> > > Another example may incur a even bigger waste is that when a NB
> port-group
> > > include ports across a big number of datapaths, so the lflow is
> > > referencing a big number of SB port-groups with DP group including all
> > > those DPs. When there are multiple port changes in the NB port-group,
> > > there can be multiple small SB port-group changes, and so the lflows can
> > > be reprocessed multiple times.
> > >
> > > This patch avoid reprocessing the same lflow in the same I-P run, which
> > > should improve performance in above scenarios.
> > >
> > > There is only one exception when a lflow may still be reprocessed more
> > > than once: if a lflow A is processed, which results in a compound
> > > conjunction added to an existed flow generated by an exited lflow B, and
> > > then lflow B needs to be reprocessed in the same run, which would
> > > cause flood-remove and reprocess lflow A. In this case lflow A is
> > > processed twice.
> > >
> > > Apart from the performance gains, there is also a potential problem of
> > > DP group fixed by this patch. If there is addrset/pg change and at the
> > > same run there is also a new local datapath monitored, then the existed
> > > code would firstly handle the addrset/pg changes causing a lflow being
> > > processed against the DP group including the new DP, which could have
> > > conjunction flows, but later the same lflow is reprocessed by
> > > lflow_output_runtime_data_handler()->lflow_add_flows_for_datapath() for
> > > the new DP only. Because lflows adding conjunction flows will not be
> > > checked against redundancy but only tries to combine the conjunction
> > > actions, it would result in redundanct conjunction actions added to the
> > > same flow, which is also linked to the same SB lflow twice. The
> > > mechanism of this patch will avoid this problem.
> > >
> > > Signed-off-by: Han Zhou <hzhou@ovn.org>
> > > ---
> > >   controller/lflow.c          | 143 +++++++++++++++++++++++++++++++-----
> > >   controller/lflow.h          |   7 ++
> > >   controller/ovn-controller.c |  18 ++++-
> > >   3 files changed, 147 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/controller/lflow.c b/controller/lflow.c
> > > index 933e2f3cc..b976c7d56 100644
> > > --- a/controller/lflow.c
> > > +++ b/controller/lflow.c
> > > @@ -78,8 +78,16 @@ consider_logical_flow(const struct
> sbrec_logical_flow *lflow,
> > >                         struct hmap *dhcp_opts, struct hmap
> *dhcpv6_opts,
> > >                         struct hmap *nd_ra_opts,
> > >                         struct controller_event_options
> *controller_event_opts,
> > > +                      bool is_recompute,
> > >                         struct lflow_ctx_in *l_ctx_in,
> > >                         struct lflow_ctx_out *l_ctx_out);
> > > +static struct lflow_processed_node *
> > > +lflows_processed_find(struct hmap *lflows_processed,
> > > +                      const struct uuid *lflow_uuid);
> > > +static void lflows_processed_add(struct hmap *lflows_processed,
> > > +                                 const struct uuid *lflow_uuid);
> > > +static void lflows_processed_remove(struct hmap *lflows_processed,
> > > +                                    struct lflow_processed_node *node);
> > >   static void lflow_resource_add(struct lflow_resource_ref *, enum
> ref_type,
> > >                                  const char *ref_name, const struct
> uuid *);
> > >   static struct ref_lflow_node *ref_lflow_lookup(struct hmap
> *ref_lflow_table,
> > > @@ -366,7 +374,7 @@ add_logical_flows(struct lflow_ctx_in *l_ctx_in,
> > >
> > >       SBREC_LOGICAL_FLOW_TABLE_FOR_EACH (lflow,
> l_ctx_in->logical_flow_table) {
> > >           consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> > > -                              &nd_ra_opts, &controller_event_opts,
> > > +                              &nd_ra_opts, &controller_event_opts,
> true,
> > >                                 l_ctx_in, l_ctx_out);
> > >       }
> > >
> > > @@ -413,6 +421,12 @@ lflow_handle_changed_flows(struct lflow_ctx_in
> *l_ctx_in,
> > >       struct ofctrl_flood_remove_node *ofrn, *next;
> > >       SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow,
> > >
>  l_ctx_in->logical_flow_table) {
> > > +        if (lflows_processed_find(l_ctx_out->lflows_processed,
> > > +                                  &lflow->header_.uuid)) {
> > > +            VLOG_DBG("lflow "UUID_FMT"has been processed, skip.",
> > > +                     UUID_ARGS(&lflow->header_.uuid));
> > > +            continue;
> > > +        }
> > >           VLOG_DBG("delete lflow "UUID_FMT,
> UUID_ARGS(&lflow->header_.uuid));
> > >           ofrn = xmalloc(sizeof *ofrn);
> > >           ofrn->sb_uuid = lflow->header_.uuid;
> > > @@ -437,8 +451,20 @@ lflow_handle_changed_flows(struct lflow_ctx_in
> *l_ctx_in,
> > >           if (lflow) {
> > >               VLOG_DBG("re-add lflow "UUID_FMT,
> > >                        UUID_ARGS(&lflow->header_.uuid));
> > > +
> > > +            /* For the extra lflows that need to be reprocessed
> because of the
> > > +             * flood remove, remove it from lflows_processed. */
> > > +            struct lflow_processed_node *lfp_node =
> > > +                lflows_processed_find(l_ctx_out->lflows_processed,
> > > +                                      &lflow->header_.uuid);
> > > +            if (lfp_node) {
> > > +                VLOG_DBG("lflow "UUID_FMT"has been processed, now
> reprocess.",
> > > +                         UUID_ARGS(&lflow->header_.uuid));
> > > +                lflows_processed_remove(l_ctx_out->lflows_processed,
> lfp_node);
> > > +            }
> > > +
> > >               consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> > > -                                  &nd_ra_opts, &controller_event_opts,
> > > +                                  &nd_ra_opts, &controller_event_opts,
> false,
> > >                                     l_ctx_in, l_ctx_out);
> > >           }
> > >       }
> > > @@ -473,15 +499,22 @@ lflow_handle_changed_ref(enum ref_type ref_type,
> const char *ref_name,
> > >       *changed = false;
> > >       bool ret = true;
> > >
> > > -    hmap_remove(&l_ctx_out->lfrr->ref_lflow_table, &rlfn->node);
> > > +    struct ovs_list lflows_todo = OVS_LIST_INITIALIZER(&lflows_todo);
> > >
> > > -    struct lflow_ref_list_node *lrln, *next;
> > > -    /* Detach the rlfn->lflow_uuids nodes from the lfrr table and clean
> > > -     * up all other nodes related to the lflows that uses the resource,
> > > -     * so that the old nodes won't interfere with updating the lfrr
> table
> > > -     * when reparsing the lflows. */
> > > +    struct lflow_ref_list_node *lrln, *lrln_uuid, *lrln_uuid_next;
> > >       HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) {
> > > -        ovs_list_remove(&lrln->list_node);
> > > +        if (lflows_processed_find(l_ctx_out->lflows_processed,
> > > +                                  &lrln->lflow_uuid)) {
> > > +            continue;
> > > +        }
> > > +        /* Use lflow_ref_list_node as list node to store the uuid.
> > > +         * Other fields are not used here. */
> > > +        lrln_uuid = xmalloc(sizeof *lrln_uuid);
> > > +        lrln_uuid->lflow_uuid = lrln->lflow_uuid;
> > > +        ovs_list_push_back(&lflows_todo, &lrln_uuid->list_node);
> > > +    }
> > > +    if (ovs_list_is_empty(&lflows_todo)) {
> > > +        return true;
> > >       }
> > >
> > >       struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
> > > @@ -509,17 +542,19 @@ lflow_handle_changed_ref(enum ref_type ref_type,
> const char *ref_name,
> > >       /* 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;
> > > -    HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) {
> > > +    LIST_FOR_EACH_SAFE (lrln_uuid, lrln_uuid_next, list_node,
> &lflows_todo) {
> > >           VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type: %d,"
> > >                    " name: %s.",
> > > -                 UUID_ARGS(&lrln->lflow_uuid),
> > > +                 UUID_ARGS(&lrln_uuid->lflow_uuid),
> > >                    ref_type, ref_name);
> > > -        ofctrl_flood_remove_add_node(&flood_remove_nodes,
> &lrln->lflow_uuid);
> > > +        ofctrl_flood_remove_add_node(&flood_remove_nodes,
> > > +                                     &lrln_uuid->lflow_uuid);
> > > +        free(lrln_uuid);
> > >       }
> > >       ofctrl_flood_remove_flows(l_ctx_out->flow_table,
> &flood_remove_nodes);
> > >
> > >       /* Secondly, for each lflow that is actually removed,
> reprocessing it. */
> > > +    struct ofctrl_flood_remove_node *ofrn, *ofrn_next;
> > >       HMAP_FOR_EACH (ofrn, hmap_node, &flood_remove_nodes) {
> > >           lflow_resource_destroy_lflow(l_ctx_out->lfrr, &ofrn->sb_uuid);
> > >           lflow_conj_ids_free(l_ctx_out->conj_ids, &ofrn->sb_uuid);
> > > @@ -535,8 +570,19 @@ lflow_handle_changed_ref(enum ref_type ref_type,
> const char *ref_name,
> > >               continue;
> > >           }
> > >
> > > +        /* For the extra lflows that need to be reprocessed because of
> the
> > > +         * flood remove, remove it from lflows_processed. */
> > > +        struct lflow_processed_node *lfp_node =
> > > +            lflows_processed_find(l_ctx_out->lflows_processed,
> > > +                                  &lflow->header_.uuid);
> > > +        if (lfp_node) {
> > > +            VLOG_DBG("lflow "UUID_FMT"has been processed, now
> reprocess.",
> > > +                     UUID_ARGS(&lflow->header_.uuid));
> > > +            lflows_processed_remove(l_ctx_out->lflows_processed,
> lfp_node);
> > > +        }
> > > +
> > >           consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> > > -                              &nd_ra_opts, &controller_event_opts,
> > > +                              &nd_ra_opts, &controller_event_opts,
> false,
> > >                                 l_ctx_in, l_ctx_out);
> > >           *changed = true;
> > >       }
> > > @@ -546,12 +592,6 @@ lflow_handle_changed_ref(enum ref_type ref_type,
> const char *ref_name,
> > >       }
> > >       hmap_destroy(&flood_remove_nodes);
> > >
> > > -    HMAP_FOR_EACH_SAFE (lrln, next, hmap_node, &rlfn->lflow_uuids) {
> > > -        hmap_remove(&rlfn->lflow_uuids, &lrln->hmap_node);
> > > -        free(lrln);
> > > -    }
> > > -    ref_lflow_node_destroy(rlfn);
> > > -
> > >       dhcp_opts_destroy(&dhcp_opts);
> > >       dhcp_opts_destroy(&dhcpv6_opts);
> > >       nd_ra_opts_destroy(&nd_ra_opts);
> > > @@ -969,11 +1009,54 @@ done:
> > >       free(matches);
> > >   }
> > >
> > > +static struct lflow_processed_node *
> > > +lflows_processed_find(struct hmap *lflows_processed,
> > > +                      const struct uuid *lflow_uuid)
> > > +{
> > > +    struct lflow_processed_node *node;
> > > +    HMAP_FOR_EACH_WITH_HASH (node, hmap_node, uuid_hash(lflow_uuid),
> > > +                             lflows_processed) {
> > > +        if (uuid_equals(&node->lflow_uuid, lflow_uuid)) {
> > > +            return node;
> > > +        }
> > > +    }
> > > +    return NULL;
> > > +}
> > > +
> > > +static void
> > > +lflows_processed_add(struct hmap *lflows_processed,
> > > +                     const struct uuid *lflow_uuid)
> > > +{
> > > +    struct lflow_processed_node *node = xmalloc(sizeof *node);
> > > +    node->lflow_uuid = *lflow_uuid;
> > > +    hmap_insert(lflows_processed, &node->hmap_node,
> uuid_hash(lflow_uuid));
> > > +}
> > > +
> > > +static void
> > > +lflows_processed_remove(struct hmap *lflows_processed,
> > > +                        struct lflow_processed_node *node)
> > > +{
> > > +    hmap_remove(lflows_processed, &node->hmap_node);
> > > +    free(node);
> > > +}
> > > +
> > > +void
> > > +lflows_processed_destroy(struct hmap *lflows_processed)
> > > +{
> > > +    struct lflow_processed_node *node, *next;
> > > +    HMAP_FOR_EACH_SAFE (node, next, hmap_node, lflows_processed) {
> > > +        hmap_remove(lflows_processed, &node->hmap_node);
> > > +        free(node);
> > > +    }
> > > +    hmap_destroy(lflows_processed);
> > > +}
> > > +
> > >   static void
> > >   consider_logical_flow(const struct sbrec_logical_flow *lflow,
> > >                         struct hmap *dhcp_opts, struct hmap
> *dhcpv6_opts,
> > >                         struct hmap *nd_ra_opts,
> > >                         struct controller_event_options
> *controller_event_opts,
> > > +                      bool is_recompute,
> > >                         struct lflow_ctx_in *l_ctx_in,
> > >                         struct lflow_ctx_out *l_ctx_out)
> > >   {
> > > @@ -987,6 +1070,13 @@ consider_logical_flow(const struct
> sbrec_logical_flow *lflow,
> > >       }
> > >       ovs_assert(!dp_group || !dp);
> > >
> > > +    if (!is_recompute) {
> > > +        ovs_assert(!lflows_processed_find(l_ctx_out->lflows_processed,
> > > +                                          &lflow->header_.uuid));
> > > +        lflows_processed_add(l_ctx_out->lflows_processed,
> > > +                             &lflow->header_.uuid);
> > > +    }
> > > +
> > >       if (dp) {
> > >           consider_logical_flow__(lflow, dp,
> > >                                   dhcp_opts, dhcpv6_opts, nd_ra_opts,
> > > @@ -1923,6 +2013,12 @@ lflow_add_flows_for_datapath(const struct
> sbrec_datapath_binding *dp,
> > >       const struct sbrec_logical_flow *lflow;
> > >       SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL (
> > >           lflow, lf_row,
> l_ctx_in->sbrec_logical_flow_by_logical_datapath) {
> > > +        if (lflows_processed_find(l_ctx_out->lflows_processed,
> > > +                                  &lflow->header_.uuid)) {
> > > +            continue;
> > > +        }
> > > +        lflows_processed_add(l_ctx_out->lflows_processed,
> > > +                             &lflow->header_.uuid);
> > >           consider_logical_flow__(lflow, dp, &dhcp_opts, &dhcpv6_opts,
> > >                                   &nd_ra_opts, &controller_event_opts,
> > >                                   l_ctx_in, l_ctx_out);
> > > @@ -1949,6 +2045,13 @@ lflow_add_flows_for_datapath(const struct
> sbrec_datapath_binding *dp,
> > >           sbrec_logical_flow_index_set_logical_dp_group(lf_row, ldpg);
> > >           SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL (
> > >               lflow, lf_row,
> l_ctx_in->sbrec_logical_flow_by_logical_dp_group) {
> > > +            if (lflows_processed_find(l_ctx_out->lflows_processed,
> > > +                                      &lflow->header_.uuid)) {
> > > +                continue;
> > > +            }
> > > +            /* Don't call lflows_processed_add() because here we
> process the
> > > +             * lflow only for one of the DPs in the DP group, which
> may be
> > > +             * incomplete. */
> > >               consider_logical_flow__(lflow, dp, &dhcp_opts,
> &dhcpv6_opts,
> > >                                       &nd_ra_opts,
> &controller_event_opts,
> > >                                       l_ctx_in, l_ctx_out);
> > > diff --git a/controller/lflow.h b/controller/lflow.h
> > > index 489dd70fb..d6dad17ec 100644
> > > --- a/controller/lflow.h
> > > +++ b/controller/lflow.h
> > > @@ -159,10 +159,17 @@ struct lflow_ctx_out {
> > >       struct lflow_resource_ref *lfrr;
> > >       struct lflow_cache *lflow_cache;
> > >       struct conj_ids *conj_ids;
> > > +    struct hmap *lflows_processed;
> > >       struct simap *hairpin_lb_ids;
> > >       struct id_pool *hairpin_id_pool;
> > >   };
> > >
> > > +struct lflow_processed_node {
> > > +    struct hmap_node hmap_node; /* In
> ed_type_lflow_output.lflows_processed. */
> > > +    struct uuid lflow_uuid;
> > > +};
> > > +void lflows_processed_destroy(struct hmap *lflows_processed);
> > > +
> > >   void lflow_init(void);
> > >   void lflow_run(struct lflow_ctx_in *, struct lflow_ctx_out *);
> > >   void lflow_handle_cached_flows(struct lflow_cache *,
> > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > > index 5069aedfc..8631bccbc 100644
> > > --- a/controller/ovn-controller.c
> > > +++ b/controller/ovn-controller.c
> > > @@ -2176,6 +2176,11 @@ struct ed_type_lflow_output {
> > >       /* conjunciton ID usage information of lflows */
> > >       struct conj_ids conj_ids;
> > >
> > > +    /* lflows processed in the current engine execution.
> > > +     * Cleared by en_lflow_output_clear_tracked_data before each engine
> > > +     * execution. */
> > > +    struct hmap lflows_processed;
> > > +
> > >       /* Data which is persistent and not cleared during
> > >        * full recompute. */
> > >       struct lflow_output_persistent_data pd;
> > > @@ -2307,6 +2312,7 @@ init_lflow_ctx(struct engine_node *node,
> > >       l_ctx_out->meter_table = &fo->meter_table;
> > >       l_ctx_out->lfrr = &fo->lflow_resource_ref;
> > >       l_ctx_out->conj_ids = &fo->conj_ids;
> > > +    l_ctx_out->lflows_processed = &fo->lflows_processed;
> > >       l_ctx_out->lflow_cache = fo->pd.lflow_cache;
> > >       l_ctx_out->hairpin_id_pool = fo->hd.pool;
> > >       l_ctx_out->hairpin_lb_ids = &fo->hd.ids;
> > > @@ -2322,11 +2328,20 @@ en_lflow_output_init(struct engine_node *node
> OVS_UNUSED,
> > >       ovn_extend_table_init(&data->meter_table);
> > >       lflow_resource_init(&data->lflow_resource_ref);
> > >       lflow_conj_ids_init(&data->conj_ids);
> > > +    hmap_init(&data->lflows_processed);
> > >       simap_init(&data->hd.ids);
> > >       data->hd.pool = id_pool_create(1, UINT32_MAX - 1);
> > >       return data;
> > >   }
> > >
> > > +static void
> > > +en_lflow_output_clear_tracked_data(void *data)
> > > +{
> > > +    struct ed_type_lflow_output *flow_output_data = data;
> > > +    lflows_processed_destroy(&flow_output_data->lflows_processed);
> > > +    hmap_init(&flow_output_data->lflows_processed);
> > > +}
> > > +
> > >   static void
> > >   en_lflow_output_cleanup(void *data)
> > >   {
> > > @@ -2336,6 +2351,7 @@ en_lflow_output_cleanup(void *data)
> > >       ovn_extend_table_destroy(&flow_output_data->meter_table);
> > >       lflow_resource_destroy(&flow_output_data->lflow_resource_ref);
> > >       lflow_conj_ids_destroy(&flow_output_data->conj_ids);
> > > +    lflows_processed_destroy(&flow_output_data->lflows_processed);
> > >       lflow_cache_destroy(flow_output_data->pd.lflow_cache);
> > >       simap_destroy(&flow_output_data->hd.ids);
> > >       id_pool_destroy(flow_output_data->hd.pool);
> > > @@ -3213,7 +3229,7 @@ main(int argc, char *argv[])
> > >       ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve");
> > >       ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected");
> > >       ENGINE_NODE(pflow_output, "physical_flow_output");
> > > -    ENGINE_NODE(lflow_output, "logical_flow_output");
> > > +    ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lflow_output,
> "logical_flow_output");
> > >       ENGINE_NODE(flow_output, "flow_output");
> > >       ENGINE_NODE(addr_sets, "addr_sets");
> > >       ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_groups, "port_groups");
> > >
> >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On Mon, Apr 11, 2022 at 2:05 PM Numan Siddique <numans@ovn.org> wrote:
>
> On Sat, Feb 5, 2022 at 12:12 AM Han Zhou <hzhou@ovn.org> wrote:
> >
> > On Fri, Feb 4, 2022 at 7:58 AM Mark Michelson <mmichels@redhat.com>
wrote:
> > >
> > > As far as I can tell, this looks correct.
> > >
> > > Acked-by: Mark Michelson <mmichels@redhat.com>
> >
> > Thanks Mark. I applied it to the main branch.
>
>
> Hi Han,
>
> We have an issue in OVN 21.12 (and OVN 21.09) where some of the
> conjunction flows are missing and recompute resolves the issue.
>
> And applying this patch to branch-21.12 fixes the issue.
>
> I need to dig further.  But before that I want to check with you if
> you have any comments on this.
>
> The details of the issue can be found here -
> https://bugzilla.redhat.com/show_bug.cgi?id=2071272#c12
>
Hi Numan,

Thanks a lot for the details of the problem and such great analysis and
reproducing steps. I reproduced the problem following your steps and
figured out the root cause.
In the test, it generates combined conjunctions and repeatedly processes
(flood remove and then adding back) the logical flows that generated those
combined conjunction flows in the same I-P run.

Precondition:
logical flow A has a conjunction flow OF_A
logical flow B has two disjunctions but one of them has only one member
(e.g. PG1 has only one LSP locally), so it doesn't have any conjunction
flow yet

The change:
PG1 has a new LSP binded locally

Execution of the I-P run:
1) port-group change handler recomputes logical flow B, which adds a
combined conjunction to OF_A, because logical flow B has an overlapping
disjunction set with logical flow A (e.g. the udp port range) - so OF_A is
updated
2) port-binding change handler recomputes logical flow B, which
flood-removes OVS flows including OF_A, and then re-add OF_A and then
update it again with the combined conjunction
3) The sequence of change-tracking for OF_A is:
update->delete->add->update. When adding to tracking, it is merged
on-the-fly, to: delete OF_A -> add/update OF_A(new)
4) Before installing the flows incrementally, the function
merge_tracked_flows() compares the *desired-flow* of OF_A(new) and OF_A and
they are exactly the same, so they are both removed from the tracking, and
simply link the OF_A(new) to the installed-flow of OF_A, as if OF_A doesn't
need any change.

So we can see that the problem here is that in 4) the merge_tracked_flows()
assumes the OF_A's desired-flow is the same as the installed-flow, which is
not correct in this scenario, because OF_A has been updated before being
deleted, which means the installed-flow doesn't match the desired flow
being deleted.
The fix is in the merge_tracked_flows(), when finding an exact match
between the added/updated flow and the deleted flows, we not only compare
the desired flows but also the installed flows.
Please take a look:

https://patchwork.ozlabs.org/project/ovn/patch/20220413063412.2972298-1-hzhou@ovn.org/

> I'm just wondering if backporting this commit would hide the real issue
or not.

Backporting this commit would definitely help, because it is less likely to
trigger the "delete and add/update" change sequence for the same OVS flow
in the same run. But I believe it is better to fix the root cause directly,
without making assumptions of the flow change sequences.

Thanks,
Han

>
> Thanks
> Numan
>




> > >
> > > On 1/20/22 13:06, Han Zhou wrote:
> > > > For I-P node lflow_output, different change handlers can trigger
> > > > reprocessing lflows. However, it is a waste to reprocess the same
lflow
> > > > multiple times in the same run, because each processing of the
lflow is
> > > > against the same input data.
> > > >
> > > > For example, if a lflow references an addresset A and a port-group
P, it
> > > > is possible that both A and P are changed in the same run, the same
> > > > lflow reprocess would be triggered by both
> > > > lflow_output_addr_sets_handler() and
lflow_output_port_groups_handler().
> > > > Another example may incur a even bigger waste is that when a NB
> > port-group
> > > > include ports across a big number of datapaths, so the lflow is
> > > > referencing a big number of SB port-groups with DP group including
all
> > > > those DPs. When there are multiple port changes in the NB
port-group,
> > > > there can be multiple small SB port-group changes, and so the
lflows can
> > > > be reprocessed multiple times.
> > > >
> > > > This patch avoid reprocessing the same lflow in the same I-P run,
which
> > > > should improve performance in above scenarios.
> > > >
> > > > There is only one exception when a lflow may still be reprocessed
more
> > > > than once: if a lflow A is processed, which results in a compound
> > > > conjunction added to an existed flow generated by an exited lflow
B, and
> > > > then lflow B needs to be reprocessed in the same run, which would
> > > > cause flood-remove and reprocess lflow A. In this case lflow A is
> > > > processed twice.
> > > >
> > > > Apart from the performance gains, there is also a potential problem
of
> > > > DP group fixed by this patch. If there is addrset/pg change and at
the
> > > > same run there is also a new local datapath monitored, then the
existed
> > > > code would firstly handle the addrset/pg changes causing a lflow
being
> > > > processed against the DP group including the new DP, which could
have
> > > > conjunction flows, but later the same lflow is reprocessed by
> > > > lflow_output_runtime_data_handler()->lflow_add_flows_for_datapath()
for
> > > > the new DP only. Because lflows adding conjunction flows will not be
> > > > checked against redundancy but only tries to combine the conjunction
> > > > actions, it would result in redundanct conjunction actions added to
the
> > > > same flow, which is also linked to the same SB lflow twice. The
> > > > mechanism of this patch will avoid this problem.
> > > >
> > > > Signed-off-by: Han Zhou <hzhou@ovn.org>
> > > > ---
> > > >   controller/lflow.c          | 143
+++++++++++++++++++++++++++++++-----
> > > >   controller/lflow.h          |   7 ++
> > > >   controller/ovn-controller.c |  18 ++++-
> > > >   3 files changed, 147 insertions(+), 21 deletions(-)
> > > >
> > > > diff --git a/controller/lflow.c b/controller/lflow.c
> > > > index 933e2f3cc..b976c7d56 100644
> > > > --- a/controller/lflow.c
> > > > +++ b/controller/lflow.c
> > > > @@ -78,8 +78,16 @@ consider_logical_flow(const struct
> > sbrec_logical_flow *lflow,
> > > >                         struct hmap *dhcp_opts, struct hmap
> > *dhcpv6_opts,
> > > >                         struct hmap *nd_ra_opts,
> > > >                         struct controller_event_options
> > *controller_event_opts,
> > > > +                      bool is_recompute,
> > > >                         struct lflow_ctx_in *l_ctx_in,
> > > >                         struct lflow_ctx_out *l_ctx_out);
> > > > +static struct lflow_processed_node *
> > > > +lflows_processed_find(struct hmap *lflows_processed,
> > > > +                      const struct uuid *lflow_uuid);
> > > > +static void lflows_processed_add(struct hmap *lflows_processed,
> > > > +                                 const struct uuid *lflow_uuid);
> > > > +static void lflows_processed_remove(struct hmap *lflows_processed,
> > > > +                                    struct lflow_processed_node
*node);
> > > >   static void lflow_resource_add(struct lflow_resource_ref *, enum
> > ref_type,
> > > >                                  const char *ref_name, const struct
> > uuid *);
> > > >   static struct ref_lflow_node *ref_lflow_lookup(struct hmap
> > *ref_lflow_table,
> > > > @@ -366,7 +374,7 @@ add_logical_flows(struct lflow_ctx_in *l_ctx_in,
> > > >
> > > >       SBREC_LOGICAL_FLOW_TABLE_FOR_EACH (lflow,
> > l_ctx_in->logical_flow_table) {
> > > >           consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> > > > -                              &nd_ra_opts, &controller_event_opts,
> > > > +                              &nd_ra_opts, &controller_event_opts,
> > true,
> > > >                                 l_ctx_in, l_ctx_out);
> > > >       }
> > > >
> > > > @@ -413,6 +421,12 @@ lflow_handle_changed_flows(struct lflow_ctx_in
> > *l_ctx_in,
> > > >       struct ofctrl_flood_remove_node *ofrn, *next;
> > > >       SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow,
> > > >
> >  l_ctx_in->logical_flow_table) {
> > > > +        if (lflows_processed_find(l_ctx_out->lflows_processed,
> > > > +                                  &lflow->header_.uuid)) {
> > > > +            VLOG_DBG("lflow "UUID_FMT"has been processed, skip.",
> > > > +                     UUID_ARGS(&lflow->header_.uuid));
> > > > +            continue;
> > > > +        }
> > > >           VLOG_DBG("delete lflow "UUID_FMT,
> > UUID_ARGS(&lflow->header_.uuid));
> > > >           ofrn = xmalloc(sizeof *ofrn);
> > > >           ofrn->sb_uuid = lflow->header_.uuid;
> > > > @@ -437,8 +451,20 @@ lflow_handle_changed_flows(struct lflow_ctx_in
> > *l_ctx_in,
> > > >           if (lflow) {
> > > >               VLOG_DBG("re-add lflow "UUID_FMT,
> > > >                        UUID_ARGS(&lflow->header_.uuid));
> > > > +
> > > > +            /* For the extra lflows that need to be reprocessed
> > because of the
> > > > +             * flood remove, remove it from lflows_processed. */
> > > > +            struct lflow_processed_node *lfp_node =
> > > > +                lflows_processed_find(l_ctx_out->lflows_processed,
> > > > +                                      &lflow->header_.uuid);
> > > > +            if (lfp_node) {
> > > > +                VLOG_DBG("lflow "UUID_FMT"has been processed, now
> > reprocess.",
> > > > +                         UUID_ARGS(&lflow->header_.uuid));
> > > > +
 lflows_processed_remove(l_ctx_out->lflows_processed,
> > lfp_node);
> > > > +            }
> > > > +
> > > >               consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> > > > -                                  &nd_ra_opts,
&controller_event_opts,
> > > > +                                  &nd_ra_opts,
&controller_event_opts,
> > false,
> > > >                                     l_ctx_in, l_ctx_out);
> > > >           }
> > > >       }
> > > > @@ -473,15 +499,22 @@ lflow_handle_changed_ref(enum ref_type
ref_type,
> > const char *ref_name,
> > > >       *changed = false;
> > > >       bool ret = true;
> > > >
> > > > -    hmap_remove(&l_ctx_out->lfrr->ref_lflow_table, &rlfn->node);
> > > > +    struct ovs_list lflows_todo =
OVS_LIST_INITIALIZER(&lflows_todo);
> > > >
> > > > -    struct lflow_ref_list_node *lrln, *next;
> > > > -    /* Detach the rlfn->lflow_uuids nodes from the lfrr table and
clean
> > > > -     * up all other nodes related to the lflows that uses the
resource,
> > > > -     * so that the old nodes won't interfere with updating the lfrr
> > table
> > > > -     * when reparsing the lflows. */
> > > > +    struct lflow_ref_list_node *lrln, *lrln_uuid, *lrln_uuid_next;
> > > >       HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) {
> > > > -        ovs_list_remove(&lrln->list_node);
> > > > +        if (lflows_processed_find(l_ctx_out->lflows_processed,
> > > > +                                  &lrln->lflow_uuid)) {
> > > > +            continue;
> > > > +        }
> > > > +        /* Use lflow_ref_list_node as list node to store the uuid.
> > > > +         * Other fields are not used here. */
> > > > +        lrln_uuid = xmalloc(sizeof *lrln_uuid);
> > > > +        lrln_uuid->lflow_uuid = lrln->lflow_uuid;
> > > > +        ovs_list_push_back(&lflows_todo, &lrln_uuid->list_node);
> > > > +    }
> > > > +    if (ovs_list_is_empty(&lflows_todo)) {
> > > > +        return true;
> > > >       }
> > > >
> > > >       struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
> > > > @@ -509,17 +542,19 @@ lflow_handle_changed_ref(enum ref_type
ref_type,
> > const char *ref_name,
> > > >       /* 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;
> > > > -    HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) {
> > > > +    LIST_FOR_EACH_SAFE (lrln_uuid, lrln_uuid_next, list_node,
> > &lflows_todo) {
> > > >           VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type:
%d,"
> > > >                    " name: %s.",
> > > > -                 UUID_ARGS(&lrln->lflow_uuid),
> > > > +                 UUID_ARGS(&lrln_uuid->lflow_uuid),
> > > >                    ref_type, ref_name);
> > > > -        ofctrl_flood_remove_add_node(&flood_remove_nodes,
> > &lrln->lflow_uuid);
> > > > +        ofctrl_flood_remove_add_node(&flood_remove_nodes,
> > > > +                                     &lrln_uuid->lflow_uuid);
> > > > +        free(lrln_uuid);
> > > >       }
> > > >       ofctrl_flood_remove_flows(l_ctx_out->flow_table,
> > &flood_remove_nodes);
> > > >
> > > >       /* Secondly, for each lflow that is actually removed,
> > reprocessing it. */
> > > > +    struct ofctrl_flood_remove_node *ofrn, *ofrn_next;
> > > >       HMAP_FOR_EACH (ofrn, hmap_node, &flood_remove_nodes) {
> > > >           lflow_resource_destroy_lflow(l_ctx_out->lfrr,
&ofrn->sb_uuid);
> > > >           lflow_conj_ids_free(l_ctx_out->conj_ids, &ofrn->sb_uuid);
> > > > @@ -535,8 +570,19 @@ lflow_handle_changed_ref(enum ref_type
ref_type,
> > const char *ref_name,
> > > >               continue;
> > > >           }
> > > >
> > > > +        /* For the extra lflows that need to be reprocessed
because of
> > the
> > > > +         * flood remove, remove it from lflows_processed. */
> > > > +        struct lflow_processed_node *lfp_node =
> > > > +            lflows_processed_find(l_ctx_out->lflows_processed,
> > > > +                                  &lflow->header_.uuid);
> > > > +        if (lfp_node) {
> > > > +            VLOG_DBG("lflow "UUID_FMT"has been processed, now
> > reprocess.",
> > > > +                     UUID_ARGS(&lflow->header_.uuid));
> > > > +            lflows_processed_remove(l_ctx_out->lflows_processed,
> > lfp_node);
> > > > +        }
> > > > +
> > > >           consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> > > > -                              &nd_ra_opts, &controller_event_opts,
> > > > +                              &nd_ra_opts, &controller_event_opts,
> > false,
> > > >                                 l_ctx_in, l_ctx_out);
> > > >           *changed = true;
> > > >       }
> > > > @@ -546,12 +592,6 @@ lflow_handle_changed_ref(enum ref_type
ref_type,
> > const char *ref_name,
> > > >       }
> > > >       hmap_destroy(&flood_remove_nodes);
> > > >
> > > > -    HMAP_FOR_EACH_SAFE (lrln, next, hmap_node, &rlfn->lflow_uuids)
{
> > > > -        hmap_remove(&rlfn->lflow_uuids, &lrln->hmap_node);
> > > > -        free(lrln);
> > > > -    }
> > > > -    ref_lflow_node_destroy(rlfn);
> > > > -
> > > >       dhcp_opts_destroy(&dhcp_opts);
> > > >       dhcp_opts_destroy(&dhcpv6_opts);
> > > >       nd_ra_opts_destroy(&nd_ra_opts);
> > > > @@ -969,11 +1009,54 @@ done:
> > > >       free(matches);
> > > >   }
> > > >
> > > > +static struct lflow_processed_node *
> > > > +lflows_processed_find(struct hmap *lflows_processed,
> > > > +                      const struct uuid *lflow_uuid)
> > > > +{
> > > > +    struct lflow_processed_node *node;
> > > > +    HMAP_FOR_EACH_WITH_HASH (node, hmap_node,
uuid_hash(lflow_uuid),
> > > > +                             lflows_processed) {
> > > > +        if (uuid_equals(&node->lflow_uuid, lflow_uuid)) {
> > > > +            return node;
> > > > +        }
> > > > +    }
> > > > +    return NULL;
> > > > +}
> > > > +
> > > > +static void
> > > > +lflows_processed_add(struct hmap *lflows_processed,
> > > > +                     const struct uuid *lflow_uuid)
> > > > +{
> > > > +    struct lflow_processed_node *node = xmalloc(sizeof *node);
> > > > +    node->lflow_uuid = *lflow_uuid;
> > > > +    hmap_insert(lflows_processed, &node->hmap_node,
> > uuid_hash(lflow_uuid));
> > > > +}
> > > > +
> > > > +static void
> > > > +lflows_processed_remove(struct hmap *lflows_processed,
> > > > +                        struct lflow_processed_node *node)
> > > > +{
> > > > +    hmap_remove(lflows_processed, &node->hmap_node);
> > > > +    free(node);
> > > > +}
> > > > +
> > > > +void
> > > > +lflows_processed_destroy(struct hmap *lflows_processed)
> > > > +{
> > > > +    struct lflow_processed_node *node, *next;
> > > > +    HMAP_FOR_EACH_SAFE (node, next, hmap_node, lflows_processed) {
> > > > +        hmap_remove(lflows_processed, &node->hmap_node);
> > > > +        free(node);
> > > > +    }
> > > > +    hmap_destroy(lflows_processed);
> > > > +}
> > > > +
> > > >   static void
> > > >   consider_logical_flow(const struct sbrec_logical_flow *lflow,
> > > >                         struct hmap *dhcp_opts, struct hmap
> > *dhcpv6_opts,
> > > >                         struct hmap *nd_ra_opts,
> > > >                         struct controller_event_options
> > *controller_event_opts,
> > > > +                      bool is_recompute,
> > > >                         struct lflow_ctx_in *l_ctx_in,
> > > >                         struct lflow_ctx_out *l_ctx_out)
> > > >   {
> > > > @@ -987,6 +1070,13 @@ consider_logical_flow(const struct
> > sbrec_logical_flow *lflow,
> > > >       }
> > > >       ovs_assert(!dp_group || !dp);
> > > >
> > > > +    if (!is_recompute) {
> > > > +
 ovs_assert(!lflows_processed_find(l_ctx_out->lflows_processed,
> > > > +                                          &lflow->header_.uuid));
> > > > +        lflows_processed_add(l_ctx_out->lflows_processed,
> > > > +                             &lflow->header_.uuid);
> > > > +    }
> > > > +
> > > >       if (dp) {
> > > >           consider_logical_flow__(lflow, dp,
> > > >                                   dhcp_opts, dhcpv6_opts,
nd_ra_opts,
> > > > @@ -1923,6 +2013,12 @@ lflow_add_flows_for_datapath(const struct
> > sbrec_datapath_binding *dp,
> > > >       const struct sbrec_logical_flow *lflow;
> > > >       SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL (
> > > >           lflow, lf_row,
> > l_ctx_in->sbrec_logical_flow_by_logical_datapath) {
> > > > +        if (lflows_processed_find(l_ctx_out->lflows_processed,
> > > > +                                  &lflow->header_.uuid)) {
> > > > +            continue;
> > > > +        }
> > > > +        lflows_processed_add(l_ctx_out->lflows_processed,
> > > > +                             &lflow->header_.uuid);
> > > >           consider_logical_flow__(lflow, dp, &dhcp_opts,
&dhcpv6_opts,
> > > >                                   &nd_ra_opts,
&controller_event_opts,
> > > >                                   l_ctx_in, l_ctx_out);
> > > > @@ -1949,6 +2045,13 @@ lflow_add_flows_for_datapath(const struct
> > sbrec_datapath_binding *dp,
> > > >           sbrec_logical_flow_index_set_logical_dp_group(lf_row,
ldpg);
> > > >           SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL (
> > > >               lflow, lf_row,
> > l_ctx_in->sbrec_logical_flow_by_logical_dp_group) {
> > > > +            if (lflows_processed_find(l_ctx_out->lflows_processed,
> > > > +                                      &lflow->header_.uuid)) {
> > > > +                continue;
> > > > +            }
> > > > +            /* Don't call lflows_processed_add() because here we
> > process the
> > > > +             * lflow only for one of the DPs in the DP group, which
> > may be
> > > > +             * incomplete. */
> > > >               consider_logical_flow__(lflow, dp, &dhcp_opts,
> > &dhcpv6_opts,
> > > >                                       &nd_ra_opts,
> > &controller_event_opts,
> > > >                                       l_ctx_in, l_ctx_out);
> > > > diff --git a/controller/lflow.h b/controller/lflow.h
> > > > index 489dd70fb..d6dad17ec 100644
> > > > --- a/controller/lflow.h
> > > > +++ b/controller/lflow.h
> > > > @@ -159,10 +159,17 @@ struct lflow_ctx_out {
> > > >       struct lflow_resource_ref *lfrr;
> > > >       struct lflow_cache *lflow_cache;
> > > >       struct conj_ids *conj_ids;
> > > > +    struct hmap *lflows_processed;
> > > >       struct simap *hairpin_lb_ids;
> > > >       struct id_pool *hairpin_id_pool;
> > > >   };
> > > >
> > > > +struct lflow_processed_node {
> > > > +    struct hmap_node hmap_node; /* In
> > ed_type_lflow_output.lflows_processed. */
> > > > +    struct uuid lflow_uuid;
> > > > +};
> > > > +void lflows_processed_destroy(struct hmap *lflows_processed);
> > > > +
> > > >   void lflow_init(void);
> > > >   void lflow_run(struct lflow_ctx_in *, struct lflow_ctx_out *);
> > > >   void lflow_handle_cached_flows(struct lflow_cache *,
> > > > diff --git a/controller/ovn-controller.c
b/controller/ovn-controller.c
> > > > index 5069aedfc..8631bccbc 100644
> > > > --- a/controller/ovn-controller.c
> > > > +++ b/controller/ovn-controller.c
> > > > @@ -2176,6 +2176,11 @@ struct ed_type_lflow_output {
> > > >       /* conjunciton ID usage information of lflows */
> > > >       struct conj_ids conj_ids;
> > > >
> > > > +    /* lflows processed in the current engine execution.
> > > > +     * Cleared by en_lflow_output_clear_tracked_data before each
engine
> > > > +     * execution. */
> > > > +    struct hmap lflows_processed;
> > > > +
> > > >       /* Data which is persistent and not cleared during
> > > >        * full recompute. */
> > > >       struct lflow_output_persistent_data pd;
> > > > @@ -2307,6 +2312,7 @@ init_lflow_ctx(struct engine_node *node,
> > > >       l_ctx_out->meter_table = &fo->meter_table;
> > > >       l_ctx_out->lfrr = &fo->lflow_resource_ref;
> > > >       l_ctx_out->conj_ids = &fo->conj_ids;
> > > > +    l_ctx_out->lflows_processed = &fo->lflows_processed;
> > > >       l_ctx_out->lflow_cache = fo->pd.lflow_cache;
> > > >       l_ctx_out->hairpin_id_pool = fo->hd.pool;
> > > >       l_ctx_out->hairpin_lb_ids = &fo->hd.ids;
> > > > @@ -2322,11 +2328,20 @@ en_lflow_output_init(struct engine_node
*node
> > OVS_UNUSED,
> > > >       ovn_extend_table_init(&data->meter_table);
> > > >       lflow_resource_init(&data->lflow_resource_ref);
> > > >       lflow_conj_ids_init(&data->conj_ids);
> > > > +    hmap_init(&data->lflows_processed);
> > > >       simap_init(&data->hd.ids);
> > > >       data->hd.pool = id_pool_create(1, UINT32_MAX - 1);
> > > >       return data;
> > > >   }
> > > >
> > > > +static void
> > > > +en_lflow_output_clear_tracked_data(void *data)
> > > > +{
> > > > +    struct ed_type_lflow_output *flow_output_data = data;
> > > > +    lflows_processed_destroy(&flow_output_data->lflows_processed);
> > > > +    hmap_init(&flow_output_data->lflows_processed);
> > > > +}
> > > > +
> > > >   static void
> > > >   en_lflow_output_cleanup(void *data)
> > > >   {
> > > > @@ -2336,6 +2351,7 @@ en_lflow_output_cleanup(void *data)
> > > >       ovn_extend_table_destroy(&flow_output_data->meter_table);
> > > >       lflow_resource_destroy(&flow_output_data->lflow_resource_ref);
> > > >       lflow_conj_ids_destroy(&flow_output_data->conj_ids);
> > > > +    lflows_processed_destroy(&flow_output_data->lflows_processed);
> > > >       lflow_cache_destroy(flow_output_data->pd.lflow_cache);
> > > >       simap_destroy(&flow_output_data->hd.ids);
> > > >       id_pool_destroy(flow_output_data->hd.pool);
> > > > @@ -3213,7 +3229,7 @@ main(int argc, char *argv[])
> > > >       ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve");
> > > >       ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected");
> > > >       ENGINE_NODE(pflow_output, "physical_flow_output");
> > > > -    ENGINE_NODE(lflow_output, "logical_flow_output");
> > > > +    ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lflow_output,
> > "logical_flow_output");
> > > >       ENGINE_NODE(flow_output, "flow_output");
> > > >       ENGINE_NODE(addr_sets, "addr_sets");
> > > >       ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_groups, "port_groups");
> > > >
> > >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >

On Wed, Apr 13, 2022 at 2:36 AM Han Zhou <hzhou@ovn.org> wrote:
>
> On Mon, Apr 11, 2022 at 2:05 PM Numan Siddique <numans@ovn.org> wrote:
> >
> > On Sat, Feb 5, 2022 at 12:12 AM Han Zhou <hzhou@ovn.org> wrote:
> > >
> > > On Fri, Feb 4, 2022 at 7:58 AM Mark Michelson <mmichels@redhat.com>
> wrote:
> > > >
> > > > As far as I can tell, this looks correct.
> > > >
> > > > Acked-by: Mark Michelson <mmichels@redhat.com>
> > >
> > > Thanks Mark. I applied it to the main branch.
> >
> >
> > Hi Han,
> >
> > We have an issue in OVN 21.12 (and OVN 21.09) where some of the
> > conjunction flows are missing and recompute resolves the issue.
> >
> > And applying this patch to branch-21.12 fixes the issue.
> >
> > I need to dig further.  But before that I want to check with you if
> > you have any comments on this.
> >
> > The details of the issue can be found here -
> > https://bugzilla.redhat.com/show_bug.cgi?id=2071272#c12
> >
> Hi Numan,
>
> Thanks a lot for the details of the problem and such great analysis and
> reproducing steps. I reproduced the problem following your steps and
> figured out the root cause.
> In the test, it generates combined conjunctions and repeatedly processes
> (flood remove and then adding back) the logical flows that generated those
> combined conjunction flows in the same I-P run.
>
> Precondition:
> logical flow A has a conjunction flow OF_A
> logical flow B has two disjunctions but one of them has only one member
> (e.g. PG1 has only one LSP locally), so it doesn't have any conjunction
> flow yet
>
> The change:
> PG1 has a new LSP binded locally
>
> Execution of the I-P run:
> 1) port-group change handler recomputes logical flow B, which adds a
> combined conjunction to OF_A, because logical flow B has an overlapping
> disjunction set with logical flow A (e.g. the udp port range) - so OF_A is
> updated
> 2) port-binding change handler recomputes logical flow B, which
> flood-removes OVS flows including OF_A, and then re-add OF_A and then
> update it again with the combined conjunction
> 3) The sequence of change-tracking for OF_A is:
> update->delete->add->update. When adding to tracking, it is merged
> on-the-fly, to: delete OF_A -> add/update OF_A(new)
> 4) Before installing the flows incrementally, the function
> merge_tracked_flows() compares the *desired-flow* of OF_A(new) and OF_A and
> they are exactly the same, so they are both removed from the tracking, and
> simply link the OF_A(new) to the installed-flow of OF_A, as if OF_A doesn't
> need any change.
>
> So we can see that the problem here is that in 4) the merge_tracked_flows()
> assumes the OF_A's desired-flow is the same as the installed-flow, which is
> not correct in this scenario, because OF_A has been updated before being
> deleted, which means the installed-flow doesn't match the desired flow
> being deleted.
> The fix is in the merge_tracked_flows(), when finding an exact match
> between the added/updated flow and the deleted flows, we not only compare
> the desired flows but also the installed flows.
> Please take a look:
>
> https://patchwork.ozlabs.org/project/ovn/patch/20220413063412.2972298-1-hzhou@ovn.org/

Thanks Han for root causing this and fixing it.  I had root caused the
issue yesterday and was planning to look into the fix.
It's great you submitted the fix.  I just need to review the patch now :)


It's all thanks to François Rigault for the analysis and for the
reproduction steps.

Thanks
Numan

>
> > I'm just wondering if backporting this commit would hide the real issue
> or not.
>
> Backporting this commit would definitely help, because it is less likely to
> trigger the "delete and add/update" change sequence for the same OVS flow
> in the same run. But I believe it is better to fix the root cause directly,
> without making assumptions of the flow change sequences.
>
> Thanks,
> Han
>
> >
> > Thanks
> > Numan
> >
>
>
>
>
> > > >
> > > > On 1/20/22 13:06, Han Zhou wrote:
> > > > > For I-P node lflow_output, different change handlers can trigger
> > > > > reprocessing lflows. However, it is a waste to reprocess the same
> lflow
> > > > > multiple times in the same run, because each processing of the
> lflow is
> > > > > against the same input data.
> > > > >
> > > > > For example, if a lflow references an addresset A and a port-group
> P, it
> > > > > is possible that both A and P are changed in the same run, the same
> > > > > lflow reprocess would be triggered by both
> > > > > lflow_output_addr_sets_handler() and
> lflow_output_port_groups_handler().
> > > > > Another example may incur a even bigger waste is that when a NB
> > > port-group
> > > > > include ports across a big number of datapaths, so the lflow is
> > > > > referencing a big number of SB port-groups with DP group including
> all
> > > > > those DPs. When there are multiple port changes in the NB
> port-group,
> > > > > there can be multiple small SB port-group changes, and so the
> lflows can
> > > > > be reprocessed multiple times.
> > > > >
> > > > > This patch avoid reprocessing the same lflow in the same I-P run,
> which
> > > > > should improve performance in above scenarios.
> > > > >
> > > > > There is only one exception when a lflow may still be reprocessed
> more
> > > > > than once: if a lflow A is processed, which results in a compound
> > > > > conjunction added to an existed flow generated by an exited lflow
> B, and
> > > > > then lflow B needs to be reprocessed in the same run, which would
> > > > > cause flood-remove and reprocess lflow A. In this case lflow A is
> > > > > processed twice.
> > > > >
> > > > > Apart from the performance gains, there is also a potential problem
> of
> > > > > DP group fixed by this patch. If there is addrset/pg change and at
> the
> > > > > same run there is also a new local datapath monitored, then the
> existed
> > > > > code would firstly handle the addrset/pg changes causing a lflow
> being
> > > > > processed against the DP group including the new DP, which could
> have
> > > > > conjunction flows, but later the same lflow is reprocessed by
> > > > > lflow_output_runtime_data_handler()->lflow_add_flows_for_datapath()
> for
> > > > > the new DP only. Because lflows adding conjunction flows will not be
> > > > > checked against redundancy but only tries to combine the conjunction
> > > > > actions, it would result in redundanct conjunction actions added to
> the
> > > > > same flow, which is also linked to the same SB lflow twice. The
> > > > > mechanism of this patch will avoid this problem.
> > > > >
> > > > > Signed-off-by: Han Zhou <hzhou@ovn.org>
> > > > > ---
> > > > >   controller/lflow.c          | 143
> +++++++++++++++++++++++++++++++-----
> > > > >   controller/lflow.h          |   7 ++
> > > > >   controller/ovn-controller.c |  18 ++++-
> > > > >   3 files changed, 147 insertions(+), 21 deletions(-)
> > > > >
> > > > > diff --git a/controller/lflow.c b/controller/lflow.c
> > > > > index 933e2f3cc..b976c7d56 100644
> > > > > --- a/controller/lflow.c
> > > > > +++ b/controller/lflow.c
> > > > > @@ -78,8 +78,16 @@ consider_logical_flow(const struct
> > > sbrec_logical_flow *lflow,
> > > > >                         struct hmap *dhcp_opts, struct hmap
> > > *dhcpv6_opts,
> > > > >                         struct hmap *nd_ra_opts,
> > > > >                         struct controller_event_options
> > > *controller_event_opts,
> > > > > +                      bool is_recompute,
> > > > >                         struct lflow_ctx_in *l_ctx_in,
> > > > >                         struct lflow_ctx_out *l_ctx_out);
> > > > > +static struct lflow_processed_node *
> > > > > +lflows_processed_find(struct hmap *lflows_processed,
> > > > > +                      const struct uuid *lflow_uuid);
> > > > > +static void lflows_processed_add(struct hmap *lflows_processed,
> > > > > +                                 const struct uuid *lflow_uuid);
> > > > > +static void lflows_processed_remove(struct hmap *lflows_processed,
> > > > > +                                    struct lflow_processed_node
> *node);
> > > > >   static void lflow_resource_add(struct lflow_resource_ref *, enum
> > > ref_type,
> > > > >                                  const char *ref_name, const struct
> > > uuid *);
> > > > >   static struct ref_lflow_node *ref_lflow_lookup(struct hmap
> > > *ref_lflow_table,
> > > > > @@ -366,7 +374,7 @@ add_logical_flows(struct lflow_ctx_in *l_ctx_in,
> > > > >
> > > > >       SBREC_LOGICAL_FLOW_TABLE_FOR_EACH (lflow,
> > > l_ctx_in->logical_flow_table) {
> > > > >           consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> > > > > -                              &nd_ra_opts, &controller_event_opts,
> > > > > +                              &nd_ra_opts, &controller_event_opts,
> > > true,
> > > > >                                 l_ctx_in, l_ctx_out);
> > > > >       }
> > > > >
> > > > > @@ -413,6 +421,12 @@ lflow_handle_changed_flows(struct lflow_ctx_in
> > > *l_ctx_in,
> > > > >       struct ofctrl_flood_remove_node *ofrn, *next;
> > > > >       SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow,
> > > > >
> > >  l_ctx_in->logical_flow_table) {
> > > > > +        if (lflows_processed_find(l_ctx_out->lflows_processed,
> > > > > +                                  &lflow->header_.uuid)) {
> > > > > +            VLOG_DBG("lflow "UUID_FMT"has been processed, skip.",
> > > > > +                     UUID_ARGS(&lflow->header_.uuid));
> > > > > +            continue;
> > > > > +        }
> > > > >           VLOG_DBG("delete lflow "UUID_FMT,
> > > UUID_ARGS(&lflow->header_.uuid));
> > > > >           ofrn = xmalloc(sizeof *ofrn);
> > > > >           ofrn->sb_uuid = lflow->header_.uuid;
> > > > > @@ -437,8 +451,20 @@ lflow_handle_changed_flows(struct lflow_ctx_in
> > > *l_ctx_in,
> > > > >           if (lflow) {
> > > > >               VLOG_DBG("re-add lflow "UUID_FMT,
> > > > >                        UUID_ARGS(&lflow->header_.uuid));
> > > > > +
> > > > > +            /* For the extra lflows that need to be reprocessed
> > > because of the
> > > > > +             * flood remove, remove it from lflows_processed. */
> > > > > +            struct lflow_processed_node *lfp_node =
> > > > > +                lflows_processed_find(l_ctx_out->lflows_processed,
> > > > > +                                      &lflow->header_.uuid);
> > > > > +            if (lfp_node) {
> > > > > +                VLOG_DBG("lflow "UUID_FMT"has been processed, now
> > > reprocess.",
> > > > > +                         UUID_ARGS(&lflow->header_.uuid));
> > > > > +
>  lflows_processed_remove(l_ctx_out->lflows_processed,
> > > lfp_node);
> > > > > +            }
> > > > > +
> > > > >               consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> > > > > -                                  &nd_ra_opts,
> &controller_event_opts,
> > > > > +                                  &nd_ra_opts,
> &controller_event_opts,
> > > false,
> > > > >                                     l_ctx_in, l_ctx_out);
> > > > >           }
> > > > >       }
> > > > > @@ -473,15 +499,22 @@ lflow_handle_changed_ref(enum ref_type
> ref_type,
> > > const char *ref_name,
> > > > >       *changed = false;
> > > > >       bool ret = true;
> > > > >
> > > > > -    hmap_remove(&l_ctx_out->lfrr->ref_lflow_table, &rlfn->node);
> > > > > +    struct ovs_list lflows_todo =
> OVS_LIST_INITIALIZER(&lflows_todo);
> > > > >
> > > > > -    struct lflow_ref_list_node *lrln, *next;
> > > > > -    /* Detach the rlfn->lflow_uuids nodes from the lfrr table and
> clean
> > > > > -     * up all other nodes related to the lflows that uses the
> resource,
> > > > > -     * so that the old nodes won't interfere with updating the lfrr
> > > table
> > > > > -     * when reparsing the lflows. */
> > > > > +    struct lflow_ref_list_node *lrln, *lrln_uuid, *lrln_uuid_next;
> > > > >       HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) {
> > > > > -        ovs_list_remove(&lrln->list_node);
> > > > > +        if (lflows_processed_find(l_ctx_out->lflows_processed,
> > > > > +                                  &lrln->lflow_uuid)) {
> > > > > +            continue;
> > > > > +        }
> > > > > +        /* Use lflow_ref_list_node as list node to store the uuid.
> > > > > +         * Other fields are not used here. */
> > > > > +        lrln_uuid = xmalloc(sizeof *lrln_uuid);
> > > > > +        lrln_uuid->lflow_uuid = lrln->lflow_uuid;
> > > > > +        ovs_list_push_back(&lflows_todo, &lrln_uuid->list_node);
> > > > > +    }
> > > > > +    if (ovs_list_is_empty(&lflows_todo)) {
> > > > > +        return true;
> > > > >       }
> > > > >
> > > > >       struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
> > > > > @@ -509,17 +542,19 @@ lflow_handle_changed_ref(enum ref_type
> ref_type,
> > > const char *ref_name,
> > > > >       /* 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;
> > > > > -    HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) {
> > > > > +    LIST_FOR_EACH_SAFE (lrln_uuid, lrln_uuid_next, list_node,
> > > &lflows_todo) {
> > > > >           VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type:
> %d,"
> > > > >                    " name: %s.",
> > > > > -                 UUID_ARGS(&lrln->lflow_uuid),
> > > > > +                 UUID_ARGS(&lrln_uuid->lflow_uuid),
> > > > >                    ref_type, ref_name);
> > > > > -        ofctrl_flood_remove_add_node(&flood_remove_nodes,
> > > &lrln->lflow_uuid);
> > > > > +        ofctrl_flood_remove_add_node(&flood_remove_nodes,
> > > > > +                                     &lrln_uuid->lflow_uuid);
> > > > > +        free(lrln_uuid);
> > > > >       }
> > > > >       ofctrl_flood_remove_flows(l_ctx_out->flow_table,
> > > &flood_remove_nodes);
> > > > >
> > > > >       /* Secondly, for each lflow that is actually removed,
> > > reprocessing it. */
> > > > > +    struct ofctrl_flood_remove_node *ofrn, *ofrn_next;
> > > > >       HMAP_FOR_EACH (ofrn, hmap_node, &flood_remove_nodes) {
> > > > >           lflow_resource_destroy_lflow(l_ctx_out->lfrr,
> &ofrn->sb_uuid);
> > > > >           lflow_conj_ids_free(l_ctx_out->conj_ids, &ofrn->sb_uuid);
> > > > > @@ -535,8 +570,19 @@ lflow_handle_changed_ref(enum ref_type
> ref_type,
> > > const char *ref_name,
> > > > >               continue;
> > > > >           }
> > > > >
> > > > > +        /* For the extra lflows that need to be reprocessed
> because of
> > > the
> > > > > +         * flood remove, remove it from lflows_processed. */
> > > > > +        struct lflow_processed_node *lfp_node =
> > > > > +            lflows_processed_find(l_ctx_out->lflows_processed,
> > > > > +                                  &lflow->header_.uuid);
> > > > > +        if (lfp_node) {
> > > > > +            VLOG_DBG("lflow "UUID_FMT"has been processed, now
> > > reprocess.",
> > > > > +                     UUID_ARGS(&lflow->header_.uuid));
> > > > > +            lflows_processed_remove(l_ctx_out->lflows_processed,
> > > lfp_node);
> > > > > +        }
> > > > > +
> > > > >           consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> > > > > -                              &nd_ra_opts, &controller_event_opts,
> > > > > +                              &nd_ra_opts, &controller_event_opts,
> > > false,
> > > > >                                 l_ctx_in, l_ctx_out);
> > > > >           *changed = true;
> > > > >       }
> > > > > @@ -546,12 +592,6 @@ lflow_handle_changed_ref(enum ref_type
> ref_type,
> > > const char *ref_name,
> > > > >       }
> > > > >       hmap_destroy(&flood_remove_nodes);
> > > > >
> > > > > -    HMAP_FOR_EACH_SAFE (lrln, next, hmap_node, &rlfn->lflow_uuids)
> {
> > > > > -        hmap_remove(&rlfn->lflow_uuids, &lrln->hmap_node);
> > > > > -        free(lrln);
> > > > > -    }
> > > > > -    ref_lflow_node_destroy(rlfn);
> > > > > -
> > > > >       dhcp_opts_destroy(&dhcp_opts);
> > > > >       dhcp_opts_destroy(&dhcpv6_opts);
> > > > >       nd_ra_opts_destroy(&nd_ra_opts);
> > > > > @@ -969,11 +1009,54 @@ done:
> > > > >       free(matches);
> > > > >   }
> > > > >
> > > > > +static struct lflow_processed_node *
> > > > > +lflows_processed_find(struct hmap *lflows_processed,
> > > > > +                      const struct uuid *lflow_uuid)
> > > > > +{
> > > > > +    struct lflow_processed_node *node;
> > > > > +    HMAP_FOR_EACH_WITH_HASH (node, hmap_node,
> uuid_hash(lflow_uuid),
> > > > > +                             lflows_processed) {
> > > > > +        if (uuid_equals(&node->lflow_uuid, lflow_uuid)) {
> > > > > +            return node;
> > > > > +        }
> > > > > +    }
> > > > > +    return NULL;
> > > > > +}
> > > > > +
> > > > > +static void
> > > > > +lflows_processed_add(struct hmap *lflows_processed,
> > > > > +                     const struct uuid *lflow_uuid)
> > > > > +{
> > > > > +    struct lflow_processed_node *node = xmalloc(sizeof *node);
> > > > > +    node->lflow_uuid = *lflow_uuid;
> > > > > +    hmap_insert(lflows_processed, &node->hmap_node,
> > > uuid_hash(lflow_uuid));
> > > > > +}
> > > > > +
> > > > > +static void
> > > > > +lflows_processed_remove(struct hmap *lflows_processed,
> > > > > +                        struct lflow_processed_node *node)
> > > > > +{
> > > > > +    hmap_remove(lflows_processed, &node->hmap_node);
> > > > > +    free(node);
> > > > > +}
> > > > > +
> > > > > +void
> > > > > +lflows_processed_destroy(struct hmap *lflows_processed)
> > > > > +{
> > > > > +    struct lflow_processed_node *node, *next;
> > > > > +    HMAP_FOR_EACH_SAFE (node, next, hmap_node, lflows_processed) {
> > > > > +        hmap_remove(lflows_processed, &node->hmap_node);
> > > > > +        free(node);
> > > > > +    }
> > > > > +    hmap_destroy(lflows_processed);
> > > > > +}
> > > > > +
> > > > >   static void
> > > > >   consider_logical_flow(const struct sbrec_logical_flow *lflow,
> > > > >                         struct hmap *dhcp_opts, struct hmap
> > > *dhcpv6_opts,
> > > > >                         struct hmap *nd_ra_opts,
> > > > >                         struct controller_event_options
> > > *controller_event_opts,
> > > > > +                      bool is_recompute,
> > > > >                         struct lflow_ctx_in *l_ctx_in,
> > > > >                         struct lflow_ctx_out *l_ctx_out)
> > > > >   {
> > > > > @@ -987,6 +1070,13 @@ consider_logical_flow(const struct
> > > sbrec_logical_flow *lflow,
> > > > >       }
> > > > >       ovs_assert(!dp_group || !dp);
> > > > >
> > > > > +    if (!is_recompute) {
> > > > > +
>  ovs_assert(!lflows_processed_find(l_ctx_out->lflows_processed,
> > > > > +                                          &lflow->header_.uuid));
> > > > > +        lflows_processed_add(l_ctx_out->lflows_processed,
> > > > > +                             &lflow->header_.uuid);
> > > > > +    }
> > > > > +
> > > > >       if (dp) {
> > > > >           consider_logical_flow__(lflow, dp,
> > > > >                                   dhcp_opts, dhcpv6_opts,
> nd_ra_opts,
> > > > > @@ -1923,6 +2013,12 @@ lflow_add_flows_for_datapath(const struct
> > > sbrec_datapath_binding *dp,
> > > > >       const struct sbrec_logical_flow *lflow;
> > > > >       SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL (
> > > > >           lflow, lf_row,
> > > l_ctx_in->sbrec_logical_flow_by_logical_datapath) {
> > > > > +        if (lflows_processed_find(l_ctx_out->lflows_processed,
> > > > > +                                  &lflow->header_.uuid)) {
> > > > > +            continue;
> > > > > +        }
> > > > > +        lflows_processed_add(l_ctx_out->lflows_processed,
> > > > > +                             &lflow->header_.uuid);
> > > > >           consider_logical_flow__(lflow, dp, &dhcp_opts,
> &dhcpv6_opts,
> > > > >                                   &nd_ra_opts,
> &controller_event_opts,
> > > > >                                   l_ctx_in, l_ctx_out);
> > > > > @@ -1949,6 +2045,13 @@ lflow_add_flows_for_datapath(const struct
> > > sbrec_datapath_binding *dp,
> > > > >           sbrec_logical_flow_index_set_logical_dp_group(lf_row,
> ldpg);
> > > > >           SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL (
> > > > >               lflow, lf_row,
> > > l_ctx_in->sbrec_logical_flow_by_logical_dp_group) {
> > > > > +            if (lflows_processed_find(l_ctx_out->lflows_processed,
> > > > > +                                      &lflow->header_.uuid)) {
> > > > > +                continue;
> > > > > +            }
> > > > > +            /* Don't call lflows_processed_add() because here we
> > > process the
> > > > > +             * lflow only for one of the DPs in the DP group, which
> > > may be
> > > > > +             * incomplete. */
> > > > >               consider_logical_flow__(lflow, dp, &dhcp_opts,
> > > &dhcpv6_opts,
> > > > >                                       &nd_ra_opts,
> > > &controller_event_opts,
> > > > >                                       l_ctx_in, l_ctx_out);
> > > > > diff --git a/controller/lflow.h b/controller/lflow.h
> > > > > index 489dd70fb..d6dad17ec 100644
> > > > > --- a/controller/lflow.h
> > > > > +++ b/controller/lflow.h
> > > > > @@ -159,10 +159,17 @@ struct lflow_ctx_out {
> > > > >       struct lflow_resource_ref *lfrr;
> > > > >       struct lflow_cache *lflow_cache;
> > > > >       struct conj_ids *conj_ids;
> > > > > +    struct hmap *lflows_processed;
> > > > >       struct simap *hairpin_lb_ids;
> > > > >       struct id_pool *hairpin_id_pool;
> > > > >   };
> > > > >
> > > > > +struct lflow_processed_node {
> > > > > +    struct hmap_node hmap_node; /* In
> > > ed_type_lflow_output.lflows_processed. */
> > > > > +    struct uuid lflow_uuid;
> > > > > +};
> > > > > +void lflows_processed_destroy(struct hmap *lflows_processed);
> > > > > +
> > > > >   void lflow_init(void);
> > > > >   void lflow_run(struct lflow_ctx_in *, struct lflow_ctx_out *);
> > > > >   void lflow_handle_cached_flows(struct lflow_cache *,
> > > > > diff --git a/controller/ovn-controller.c
> b/controller/ovn-controller.c
> > > > > index 5069aedfc..8631bccbc 100644
> > > > > --- a/controller/ovn-controller.c
> > > > > +++ b/controller/ovn-controller.c
> > > > > @@ -2176,6 +2176,11 @@ struct ed_type_lflow_output {
> > > > >       /* conjunciton ID usage information of lflows */
> > > > >       struct conj_ids conj_ids;
> > > > >
> > > > > +    /* lflows processed in the current engine execution.
> > > > > +     * Cleared by en_lflow_output_clear_tracked_data before each
> engine
> > > > > +     * execution. */
> > > > > +    struct hmap lflows_processed;
> > > > > +
> > > > >       /* Data which is persistent and not cleared during
> > > > >        * full recompute. */
> > > > >       struct lflow_output_persistent_data pd;
> > > > > @@ -2307,6 +2312,7 @@ init_lflow_ctx(struct engine_node *node,
> > > > >       l_ctx_out->meter_table = &fo->meter_table;
> > > > >       l_ctx_out->lfrr = &fo->lflow_resource_ref;
> > > > >       l_ctx_out->conj_ids = &fo->conj_ids;
> > > > > +    l_ctx_out->lflows_processed = &fo->lflows_processed;
> > > > >       l_ctx_out->lflow_cache = fo->pd.lflow_cache;
> > > > >       l_ctx_out->hairpin_id_pool = fo->hd.pool;
> > > > >       l_ctx_out->hairpin_lb_ids = &fo->hd.ids;
> > > > > @@ -2322,11 +2328,20 @@ en_lflow_output_init(struct engine_node
> *node
> > > OVS_UNUSED,
> > > > >       ovn_extend_table_init(&data->meter_table);
> > > > >       lflow_resource_init(&data->lflow_resource_ref);
> > > > >       lflow_conj_ids_init(&data->conj_ids);
> > > > > +    hmap_init(&data->lflows_processed);
> > > > >       simap_init(&data->hd.ids);
> > > > >       data->hd.pool = id_pool_create(1, UINT32_MAX - 1);
> > > > >       return data;
> > > > >   }
> > > > >
> > > > > +static void
> > > > > +en_lflow_output_clear_tracked_data(void *data)
> > > > > +{
> > > > > +    struct ed_type_lflow_output *flow_output_data = data;
> > > > > +    lflows_processed_destroy(&flow_output_data->lflows_processed);
> > > > > +    hmap_init(&flow_output_data->lflows_processed);
> > > > > +}
> > > > > +
> > > > >   static void
> > > > >   en_lflow_output_cleanup(void *data)
> > > > >   {
> > > > > @@ -2336,6 +2351,7 @@ en_lflow_output_cleanup(void *data)
> > > > >       ovn_extend_table_destroy(&flow_output_data->meter_table);
> > > > >       lflow_resource_destroy(&flow_output_data->lflow_resource_ref);
> > > > >       lflow_conj_ids_destroy(&flow_output_data->conj_ids);
> > > > > +    lflows_processed_destroy(&flow_output_data->lflows_processed);
> > > > >       lflow_cache_destroy(flow_output_data->pd.lflow_cache);
> > > > >       simap_destroy(&flow_output_data->hd.ids);
> > > > >       id_pool_destroy(flow_output_data->hd.pool);
> > > > > @@ -3213,7 +3229,7 @@ main(int argc, char *argv[])
> > > > >       ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve");
> > > > >       ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected");
> > > > >       ENGINE_NODE(pflow_output, "physical_flow_output");
> > > > > -    ENGINE_NODE(lflow_output, "logical_flow_output");
> > > > > +    ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lflow_output,
> > > "logical_flow_output");
> > > > >       ENGINE_NODE(flow_output, "flow_output");
> > > > >       ENGINE_NODE(addr_sets, "addr_sets");
> > > > >       ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_groups, "port_groups");
> > > > >
> > > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>