On Fri, Jun 2, 2023 at 7:56 AM Numan Siddique <num...@ovn.org> wrote:
>
> On Fri, Jun 2, 2023 at 12:12 AM Han Zhou <hz...@ovn.org> wrote:
> >
> > This patch ensures logical flows remain persistent between engine runs,
> > given there are no changes. In case of any change, it will deconstruct
> > and reconstruct the hmap during recompute. This functionality is needed
> > for future incremental processing, particularly when logical flows need
> > to be removed.
> >
> > Signed-off-by: Han Zhou <hz...@ovn.org>
> > Reviewed-by: Ales Musil <amu...@redhat.com>
>
> Acked-by: Numan Siddique <num...@ovn.org>
>
> Numan
Thanks Numan. I applied to main.
Han
>
> > ---
> > northd/en-lflow.c | 16 ++++++++++----
> > northd/northd.c | 55 +++++++++++++++++++++++++++++++----------------
> > northd/northd.h | 13 +++++++++--
> > ovs | 2 +-
> > 4 files changed, 61 insertions(+), 25 deletions(-)
> >
> > diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> > index bed7bb001e20..081ec7c353ed 100644
> > --- a/northd/en-lflow.c
> > +++ b/northd/en-lflow.c
> > @@ -30,7 +30,7 @@
> >
> > VLOG_DEFINE_THIS_MODULE(en_lflow);
> >
> > -void en_lflow_run(struct engine_node *node, void *data OVS_UNUSED)
> > +void en_lflow_run(struct engine_node *node, void *data)
> > {
> > const struct engine_context *eng_ctx = engine_get_context();
> >
> > @@ -68,13 +68,17 @@ void en_lflow_run(struct engine_node *node, void
*data OVS_UNUSED)
> > lflow_input.ovn_internal_version_changed =
> > northd_data->ovn_internal_version_changed;
> >
> > + struct lflow_data *lflow_data = data;
> > + lflow_data_destroy(lflow_data);
> > + lflow_data_init(lflow_data);
> > +
> > stopwatch_start(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
> > build_bfd_table(eng_ctx->ovnsb_idl_txn,
> > lflow_input.nbrec_bfd_table,
> > lflow_input.sbrec_bfd_table,
> > &bfd_connections,
> > &northd_data->lr_ports);
> > - build_lflows(&lflow_input, eng_ctx->ovnsb_idl_txn);
> > + build_lflows(eng_ctx->ovnsb_idl_txn, &lflow_input,
&lflow_data->lflows);
> > bfd_cleanup_connections(lflow_input.nbrec_bfd_table,
> > &bfd_connections);
> > hmap_destroy(&bfd_connections);
> > @@ -82,12 +86,16 @@ void en_lflow_run(struct engine_node *node, void
*data OVS_UNUSED)
> >
> > engine_set_node_state(node, EN_UPDATED);
> > }
> > +
> > void *en_lflow_init(struct engine_node *node OVS_UNUSED,
> > struct engine_arg *arg OVS_UNUSED)
> > {
> > - return NULL;
> > + struct lflow_data *data = xmalloc(sizeof *data);
> > + lflow_data_init(data);
> > + return data;
> > }
> >
> > -void en_lflow_cleanup(void *data OVS_UNUSED)
> > +void en_lflow_cleanup(void *data)
> > {
> > + lflow_data_destroy(data);
> > }
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 277f4780bd20..93f126aa32b4 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -15250,6 +15250,22 @@ build_lswitch_and_lrouter_flows(const struct
ovn_datapaths *ls_datapaths,
> >
> > static ssize_t max_seen_lflow_size = 128;
> >
> > +void
> > +lflow_data_init(struct lflow_data *data)
> > +{
> > + fast_hmap_size_for(&data->lflows, max_seen_lflow_size);
> > +}
> > +
> > +void
> > +lflow_data_destroy(struct lflow_data *data)
> > +{
> > + struct ovn_lflow *lflow;
> > + HMAP_FOR_EACH_SAFE (lflow, hmap_node, &data->lflows) {
> > + ovn_lflow_destroy(&data->lflows, lflow);
> > + }
> > + hmap_destroy(&data->lflows);
> > +}
> > +
> > void run_update_worker_pool(int n_threads)
> > {
> > /* If number of threads has been updated (or initially set),
> > @@ -15279,10 +15295,10 @@ build_mcast_groups(const struct
sbrec_igmp_group_table *sbrec_igmp_group_table,
> >
> > /* Updates the Logical_Flow and Multicast_Group tables in the OVN_SB
database,
> > * constructing their contents based on the OVN_NB database. */
> > -void build_lflows(struct lflow_input *input_data,
> > - struct ovsdb_idl_txn *ovnsb_txn)
> > +void build_lflows(struct ovsdb_idl_txn *ovnsb_txn,
> > + struct lflow_input *input_data,
> > + struct hmap *lflows)
> > {
> > - struct hmap lflows;
> > struct hmap mcast_groups;
> > struct hmap igmp_groups;
> >
> > @@ -15292,13 +15308,11 @@ void build_lflows(struct lflow_input
*input_data,
> > input_data->ls_ports, input_data->lr_ports,
> > &mcast_groups, &igmp_groups);
> >
> > - fast_hmap_size_for(&lflows, max_seen_lflow_size);
> > -
> > build_lswitch_and_lrouter_flows(input_data->ls_datapaths,
> > input_data->lr_datapaths,
> > input_data->ls_ports,
> > input_data->lr_ports,
> > - input_data->port_groups, &lflows,
> > + input_data->port_groups, lflows,
> > &mcast_groups, &igmp_groups,
> > input_data->meter_groups,
input_data->lbs,
> > input_data->bfd_connections,
> > @@ -15311,10 +15325,10 @@ void build_lflows(struct lflow_input
*input_data,
> > /* Parallel build may result in a suboptimal hash. Resize the
> > * hash to a correct size before doing lookups */
> >
> > - hmap_expand(&lflows);
> > + hmap_expand(lflows);
> >
> > - if (hmap_count(&lflows) > max_seen_lflow_size) {
> > - max_seen_lflow_size = hmap_count(&lflows);
> > + if (hmap_count(lflows) > max_seen_lflow_size) {
> > + max_seen_lflow_size = hmap_count(lflows);
> > }
> >
> > stopwatch_start(LFLOWS_DP_GROUPS_STOPWATCH_NAME, time_msec());
> > @@ -15332,7 +15346,7 @@ void build_lflows(struct lflow_input
*input_data,
> > fast_hmap_size_for(&single_dp_lflows, max_seen_lflow_size);
> >
> > struct ovn_lflow *lflow;
> > - HMAP_FOR_EACH_SAFE (lflow, hmap_node, &lflows) {
> > + HMAP_FOR_EACH_SAFE (lflow, hmap_node, lflows) {
> > struct ovn_datapath **datapaths_array;
> > size_t n_datapaths;
> >
> > @@ -15360,7 +15374,7 @@ void build_lflows(struct lflow_input
*input_data,
> > /* Logical flow should be re-hashed to allow lookups. */
> > uint32_t hash = hmap_node_hash(&lflow->hmap_node);
> > /* Remove from lflows. */
> > - hmap_remove(&lflows, &lflow->hmap_node);
> > + hmap_remove(lflows, &lflow->hmap_node);
> > hash =
ovn_logical_flow_hash_datapath(&lflow->od->sb->header_.uuid,
> > hash);
> > /* Add to single_dp_lflows. */
> > @@ -15370,13 +15384,14 @@ void build_lflows(struct lflow_input
*input_data,
> >
> > /* Merge multiple and single dp hashes. */
> >
> > - fast_hmap_merge(&lflows, &single_dp_lflows);
> > + fast_hmap_merge(lflows, &single_dp_lflows);
> >
> > hmap_destroy(&single_dp_lflows);
> >
> > stopwatch_stop(LFLOWS_DP_GROUPS_STOPWATCH_NAME, time_msec());
> > stopwatch_start(LFLOWS_TO_SB_STOPWATCH_NAME, time_msec());
> >
> > + struct hmap lflows_temp = HMAP_INITIALIZER(&lflows_temp);
> > /* Push changes to the Logical_Flow table to database. */
> > const struct sbrec_logical_flow *sbflow;
> > SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_SAFE (sbflow,
> > @@ -15419,7 +15434,7 @@ void build_lflows(struct lflow_input
*input_data,
> > = !strcmp(sbflow->pipeline, "ingress") ? P_IN : P_OUT;
> >
> > lflow = ovn_lflow_find(
> > - &lflows, dp_group ? NULL : logical_datapath_od,
> > + lflows, dp_group ? NULL : logical_datapath_od,
> > ovn_stage_build(ovn_datapath_get_type(logical_datapath_od),
> > pipeline, sbflow->table_id),
> > sbflow->priority, sbflow->match, sbflow->actions,
> > @@ -15480,13 +15495,15 @@ void build_lflows(struct lflow_input
*input_data,
> > }
> >
> > /* This lflow updated. Not needed anymore. */
> > - ovn_lflow_destroy(&lflows, lflow);
> > + hmap_remove(lflows, &lflow->hmap_node);
> > + hmap_insert(&lflows_temp, &lflow->hmap_node,
> > + hmap_node_hash(&lflow->hmap_node));
> > } else {
> > sbrec_logical_flow_delete(sbflow);
> > }
> > }
> >
> > - HMAP_FOR_EACH_SAFE (lflow, hmap_node, &lflows) {
> > + HMAP_FOR_EACH_SAFE (lflow, hmap_node, lflows) {
> > const char *pipeline =
ovn_stage_get_pipeline_name(lflow->stage);
> > uint8_t table = ovn_stage_get_table(lflow->stage);
> > struct hmap *dp_groups;
> > @@ -15550,10 +15567,12 @@ void build_lflows(struct lflow_input
*input_data,
> > }
> > sbrec_logical_flow_set_external_ids(sbflow, &ids);
> > smap_destroy(&ids);
> > -
> > - ovn_lflow_destroy(&lflows, lflow);
> > + hmap_remove(lflows, &lflow->hmap_node);
> > + hmap_insert(&lflows_temp, &lflow->hmap_node,
> > + hmap_node_hash(&lflow->hmap_node));
> > }
> > - hmap_destroy(&lflows);
> > + hmap_swap(lflows, &lflows_temp);
> > + hmap_destroy(&lflows_temp);
> >
> > stopwatch_stop(LFLOWS_TO_SB_STOPWATCH_NAME, time_msec());
> > struct ovn_dp_group *dpg;
> > diff --git a/northd/northd.h b/northd/northd.h
> > index f073ceb6d9c2..edccb0ca89c0 100644
> > --- a/northd/northd.h
> > +++ b/northd/northd.h
> > @@ -99,6 +99,13 @@ struct northd_data {
> > struct chassis_features features;
> > };
> >
> > +struct lflow_data {
> > + struct hmap lflows;
> > +};
> > +
> > +void lflow_data_init(struct lflow_data *);
> > +void lflow_data_destroy(struct lflow_data *);
> > +
> > struct lflow_input {
> > /* Northbound table references */
> > const struct nbrec_bfd_table *nbrec_bfd_table;
> > @@ -295,8 +302,10 @@ void northd_destroy(struct northd_data *data);
> > void northd_init(struct northd_data *data);
> > void northd_indices_create(struct northd_data *data,
> > struct ovsdb_idl *ovnsb_idl);
> > -void build_lflows(struct lflow_input *input_data,
> > - struct ovsdb_idl_txn *ovnsb_txn);
> > +void build_lflows(struct ovsdb_idl_txn *ovnsb_txn,
> > + struct lflow_input *input_data,
> > + struct hmap *lflows);
> > +
> > void build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
> > const struct nbrec_bfd_table *,
> > const struct sbrec_bfd_table *,
> > diff --git a/ovs b/ovs
> > index 0187eadfce45..b72a7f92573a 160000
> > --- a/ovs
> > +++ b/ovs
> > @@ -1 +1 @@
> > -Subproject commit 0187eadfce4505d502e57c0e688b830f0a1ec728
> > +Subproject commit b72a7f92573aa4e6205e57cb978532b4c04702e1
> > --
> > 2.30.2
> >
> > _______________________________________________
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
