| Message ID | 166316096749.13381.6182490210115279372.stgit@dceara.remote.csb |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Refactor and simplify a bit ovn-controller I-P code. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
On Wed, Sep 14, 2022 at 3:10 PM Dumitru Ceara <dceara@redhat.com> wrote: > This is actually more in line with how the callback is used. It's called > every the incremental engine preparese for the next engine run. > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > controller/ovn-controller.c | 41 > ++++++++++++++++++++--------------------- > lib/inc-proc-eng.c | 19 +++++++++++-------- > lib/inc-proc-eng.h | 19 ++++++++++--------- > 3 files changed, 41 insertions(+), 38 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 18a01bbab..f26d6a9e0 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -1058,7 +1058,7 @@ en_ofctrl_is_connected_run(struct engine_node *node, > void *data) > * processing to OVS_interface changes but simply mark the node status > as > * UPDATED (and so the run() and the change handler is the same). > * 2. The iface_table_external_ids_old is computed/updated in the member > - * clear_tracked_data(), because that is when the last round of > processing > + * init_run(), because that is when the last round of processing > * has completed but the new IDL data is yet to refresh, so we replace > the > * old data with the current data. */ > struct ed_type_ovs_interface_shadow { > @@ -1096,7 +1096,8 @@ en_ovs_interface_shadow_cleanup(void *data_) > } > > static void > -en_ovs_interface_shadow_clear_tracked_data(void *data_) > +en_ovs_interface_shadow_init_run(struct engine_node *node OVS_UNUSED, > + void *data_) > { > struct ed_type_ovs_interface_shadow *data = data_; > > iface_table_external_ids_old_destroy(&data->iface_table_external_ids_old); > @@ -1163,7 +1164,7 @@ en_activated_ports_cleanup(void *data_) > } > > static void > -en_activated_ports_clear_tracked_data(void *data) > +en_activated_ports_init_run(struct engine_node *node OVS_UNUSED, void > *data) > { > en_activated_ports_cleanup(data); > } > @@ -1311,7 +1312,7 @@ struct ed_type_runtime_data { > */ > > static void > -en_runtime_data_clear_tracked_data(void *data_) > +en_runtime_data_init_run(struct engine_node *node OVS_UNUSED, void *data_) > { > struct ed_type_runtime_data *data = data_; > > @@ -1669,14 +1670,14 @@ en_addr_sets_init(struct engine_node *node > OVS_UNUSED, > } > > static void > -en_addr_sets_clear_tracked_data(void *data) > +en_addr_sets_init_run(struct engine_node *node OVS_UNUSED, void *data) > { > struct ed_type_addr_sets *as = data; > sset_clear(&as->new); > sset_clear(&as->deleted); > - struct shash_node *node; > - SHASH_FOR_EACH_SAFE (node, &as->updated) { > - struct addr_set_diff *asd = node->data; > + struct shash_node *as_node; > + SHASH_FOR_EACH_SAFE (as_node, &as->updated) { > + struct addr_set_diff *asd = as_node->data; > expr_constant_set_destroy(asd->added); > free(asd->added); > expr_constant_set_destroy(asd->deleted); > @@ -1689,8 +1690,6 @@ en_addr_sets_clear_tracked_data(void *data) > static void > en_addr_sets_cleanup(void *data) > { > - en_addr_sets_clear_tracked_data(data); > - > struct ed_type_addr_sets *as = data; > expr_const_sets_destroy(&as->addr_sets); > shash_destroy(&as->addr_sets); > @@ -1933,7 +1932,7 @@ port_groups_update(const struct > sbrec_port_group_table *port_group_table, > } > > static void > -en_port_groups_clear_tracked_data(void *data_) > +en_port_groups_init_run(struct engine_node *node OVS_UNUSED, void *data_) > { > struct ed_type_port_groups *pg = data_; > sset_clear(&pg->new); > @@ -2078,7 +2077,7 @@ en_ct_zones_init(struct engine_node *node, struct > engine_arg *arg OVS_UNUSED) > } > > static void > -en_ct_zones_clear_tracked_data(void *data_) > +en_ct_zones_init_run(struct engine_node *node OVS_UNUSED, void *data_) > { > struct ed_type_ct_zones *data = data_; > data->recomputed = false; > @@ -2529,7 +2528,7 @@ struct ed_type_lflow_output { > struct conj_ids conj_ids; > > /* lflows processed in the current engine execution. > - * Cleared by en_lflow_output_clear_tracked_data before each engine > + * Cleared by en_lflow_output_init_run before each engine > * execution. */ > struct hmap lflows_processed; > > @@ -2733,7 +2732,7 @@ en_lflow_output_init(struct engine_node *node > OVS_UNUSED, > } > > static void > -en_lflow_output_clear_tracked_data(void *data) > +en_lflow_output_init_run(struct engine_node *node OVS_UNUSED, void *data) > { > struct ed_type_lflow_output *flow_output_data = data; > lflows_processed_destroy(&flow_output_data->lflows_processed); > @@ -3678,20 +3677,20 @@ main(int argc, char *argv[]) > > /* Define inc-proc-engine nodes. */ > ENGINE_NODE(sb_ro, "sb_ro"); > - ENGINE_NODE_WITH_CLEAR_TRACK_DATA_IS_VALID(ct_zones, "ct_zones"); > - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(ovs_interface_shadow, > + ENGINE_NODE_WITH_INIT_RUN_IS_VALID(ct_zones, "ct_zones"); > + ENGINE_NODE_WITH_INIT_RUN(ovs_interface_shadow, > "ovs_interface_shadow"); > - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(runtime_data, "runtime_data"); > + ENGINE_NODE_WITH_INIT_RUN(runtime_data, "runtime_data"); > ENGINE_NODE(non_vif_data, "non_vif_data"); > ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve"); > ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected"); > - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(activated_ports, "activated_ports"); > + ENGINE_NODE_WITH_INIT_RUN(activated_ports, "activated_ports"); > ENGINE_NODE(postponed_ports, "postponed_ports"); > ENGINE_NODE(pflow_output, "physical_flow_output"); > - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lflow_output, > "logical_flow_output"); > + ENGINE_NODE_WITH_INIT_RUN(lflow_output, "logical_flow_output"); > ENGINE_NODE(flow_output, "flow_output"); > - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(addr_sets, "addr_sets"); > - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_groups, "port_groups"); > + ENGINE_NODE_WITH_INIT_RUN(addr_sets, "addr_sets"); > + ENGINE_NODE_WITH_INIT_RUN(port_groups, "port_groups"); > ENGINE_NODE(northd_options, "northd_options"); > > #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR); > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c > index 2e2b31033..30a20bb35 100644 > --- a/lib/inc-proc-eng.c > +++ b/lib/inc-proc-eng.c > @@ -196,10 +196,12 @@ void > engine_cleanup(void) > { > for (size_t i = 0; i < engine_n_nodes; i++) { > - if (engine_nodes[i]->clear_tracked_data) { > - engine_nodes[i]->clear_tracked_data(engine_nodes[i]->data); > + if (engine_nodes[i]->init_run) { > + engine_nodes[i]->init_run(engine_nodes[i], > engine_nodes[i]->data); > } > + } > > + for (size_t i = 0; i < engine_n_nodes; i++) { > if (engine_nodes[i]->cleanup) { > engine_nodes[i]->cleanup(engine_nodes[i]->data); > } > @@ -351,8 +353,8 @@ engine_init_run(void) > for (size_t i = 0; i < engine_n_nodes; i++) { > engine_set_node_state(engine_nodes[i], EN_STALE); > > - if (engine_nodes[i]->clear_tracked_data) { > - engine_nodes[i]->clear_tracked_data(engine_nodes[i]->data); > + if (engine_nodes[i]->init_run) { > + engine_nodes[i]->init_run(engine_nodes[i], > engine_nodes[i]->data); > } > } > } > @@ -377,10 +379,11 @@ engine_recompute(struct engine_node *node, bool > allowed, > goto done; > } > > - /* Clear tracked data before calling run() so that partially tracked > data > - * from some of the change handler executions are cleared. */ > - if (node->clear_tracked_data) { > - node->clear_tracked_data(node->data); > + /* Make sure data is properly initialized before calling run(), e.g., > + * partially tracked data some of the change handler executions must > + * be cleared. */ > + if (node->init_run) { > + node->init_run(node, node->data); > } > > /* Run the node handler which might change state. */ > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h > index dc365dc18..ab5b91b28 100644 > --- a/lib/inc-proc-eng.h > +++ b/lib/inc-proc-eng.h > @@ -150,6 +150,11 @@ struct engine_node { > /* Method to clean up data. It may be NULL. */ > void (*cleanup)(void *data); > > + /* Method to initialize state at every engine run. It can be used for > + * example to clear up tracked data maintained by the engine node in > the > + * engine 'data'. It may be NULL. */ > + void (*init_run)(struct engine_node *node, void *data); > + > /* Fully processes all inputs of this node and regenerates the data > * of this node. The pointer to the node's data is passed as argument. > * 'run' handlers can also call engine_get_context() and the > @@ -165,10 +170,6 @@ struct engine_node { > */ > bool (*is_valid)(struct engine_node *); > > - /* Method to clear up tracked data maintained by the engine node in > the > - * engine 'data'. It may be NULL. */ > - void (*clear_tracked_data)(void *tracked_data); > - > /* Engine stats. */ > struct engine_stats stats; > }; > @@ -311,20 +312,20 @@ void engine_ovsdb_node_add_index(struct engine_node > *, const char *name, > .run = en_##NAME##_run, \ > .cleanup = en_##NAME##_cleanup, \ > .is_valid = NULL, \ > - .clear_tracked_data = NULL, \ > + .init_run = NULL, \ > }; > > -#define ENGINE_NODE_WITH_CLEAR_TRACK_DATA_IS_VALID(NAME, NAME_STR) \ > +#define ENGINE_NODE_WITH_INIT_RUN_IS_VALID(NAME, NAME_STR) \ > ENGINE_NODE(NAME, NAME_STR) \ > - en_##NAME.clear_tracked_data = en_##NAME##_clear_tracked_data; \ > + en_##NAME.init_run = en_##NAME##_init_run; \ > en_##NAME.is_valid = en_##NAME##_is_valid; > > #define ENGINE_NODE(NAME, NAME_STR) \ > ENGINE_NODE_DEF(NAME, NAME_STR) > > -#define ENGINE_NODE_WITH_CLEAR_TRACK_DATA(NAME, NAME_STR) \ > +#define ENGINE_NODE_WITH_INIT_RUN(NAME, NAME_STR) \ > ENGINE_NODE(NAME, NAME_STR) \ > - en_##NAME.clear_tracked_data = en_##NAME##_clear_tracked_data; > + en_##NAME.init_run = en_##NAME##_init_run; > > /* Macro to define member functions of an engine node which represents > * a table of OVSDB */ > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks. Acked-by: Ales Musil <amusil@redhat.com>
On Wed, Sep 14, 2022 at 6:10 AM Dumitru Ceara <dceara@redhat.com> wrote: > > This is actually more in line with how the callback is used. It's called > every the incremental engine preparese for the next engine run. > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> Thanks Dumtru. The name looks good to me, but why does the new function require both the node and node->data as parameters? > --- > controller/ovn-controller.c | 41 ++++++++++++++++++++--------------------- > lib/inc-proc-eng.c | 19 +++++++++++-------- > lib/inc-proc-eng.h | 19 ++++++++++--------- > 3 files changed, 41 insertions(+), 38 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 18a01bbab..f26d6a9e0 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -1058,7 +1058,7 @@ en_ofctrl_is_connected_run(struct engine_node *node, void *data) > * processing to OVS_interface changes but simply mark the node status as > * UPDATED (and so the run() and the change handler is the same). > * 2. The iface_table_external_ids_old is computed/updated in the member > - * clear_tracked_data(), because that is when the last round of processing > + * init_run(), because that is when the last round of processing > * has completed but the new IDL data is yet to refresh, so we replace the > * old data with the current data. */ > struct ed_type_ovs_interface_shadow { > @@ -1096,7 +1096,8 @@ en_ovs_interface_shadow_cleanup(void *data_) > } > > static void > -en_ovs_interface_shadow_clear_tracked_data(void *data_) > +en_ovs_interface_shadow_init_run(struct engine_node *node OVS_UNUSED, > + void *data_) > { > struct ed_type_ovs_interface_shadow *data = data_; > iface_table_external_ids_old_destroy(&data->iface_table_external_ids_old); > @@ -1163,7 +1164,7 @@ en_activated_ports_cleanup(void *data_) > } > > static void > -en_activated_ports_clear_tracked_data(void *data) > +en_activated_ports_init_run(struct engine_node *node OVS_UNUSED, void *data) > { > en_activated_ports_cleanup(data); > } > @@ -1311,7 +1312,7 @@ struct ed_type_runtime_data { > */ > > static void > -en_runtime_data_clear_tracked_data(void *data_) > +en_runtime_data_init_run(struct engine_node *node OVS_UNUSED, void *data_) > { > struct ed_type_runtime_data *data = data_; > > @@ -1669,14 +1670,14 @@ en_addr_sets_init(struct engine_node *node OVS_UNUSED, > } > > static void > -en_addr_sets_clear_tracked_data(void *data) > +en_addr_sets_init_run(struct engine_node *node OVS_UNUSED, void *data) > { > struct ed_type_addr_sets *as = data; > sset_clear(&as->new); > sset_clear(&as->deleted); > - struct shash_node *node; > - SHASH_FOR_EACH_SAFE (node, &as->updated) { > - struct addr_set_diff *asd = node->data; > + struct shash_node *as_node; > + SHASH_FOR_EACH_SAFE (as_node, &as->updated) { > + struct addr_set_diff *asd = as_node->data; > expr_constant_set_destroy(asd->added); > free(asd->added); > expr_constant_set_destroy(asd->deleted); > @@ -1689,8 +1690,6 @@ en_addr_sets_clear_tracked_data(void *data) > static void > en_addr_sets_cleanup(void *data) > { > - en_addr_sets_clear_tracked_data(data); > - > struct ed_type_addr_sets *as = data; > expr_const_sets_destroy(&as->addr_sets); > shash_destroy(&as->addr_sets); > @@ -1933,7 +1932,7 @@ port_groups_update(const struct sbrec_port_group_table *port_group_table, > } > > static void > -en_port_groups_clear_tracked_data(void *data_) > +en_port_groups_init_run(struct engine_node *node OVS_UNUSED, void *data_) > { > struct ed_type_port_groups *pg = data_; > sset_clear(&pg->new); > @@ -2078,7 +2077,7 @@ en_ct_zones_init(struct engine_node *node, struct engine_arg *arg OVS_UNUSED) > } > > static void > -en_ct_zones_clear_tracked_data(void *data_) > +en_ct_zones_init_run(struct engine_node *node OVS_UNUSED, void *data_) > { > struct ed_type_ct_zones *data = data_; > data->recomputed = false; > @@ -2529,7 +2528,7 @@ struct ed_type_lflow_output { > struct conj_ids conj_ids; > > /* lflows processed in the current engine execution. > - * Cleared by en_lflow_output_clear_tracked_data before each engine > + * Cleared by en_lflow_output_init_run before each engine > * execution. */ > struct hmap lflows_processed; > > @@ -2733,7 +2732,7 @@ en_lflow_output_init(struct engine_node *node OVS_UNUSED, > } > > static void > -en_lflow_output_clear_tracked_data(void *data) > +en_lflow_output_init_run(struct engine_node *node OVS_UNUSED, void *data) > { > struct ed_type_lflow_output *flow_output_data = data; > lflows_processed_destroy(&flow_output_data->lflows_processed); > @@ -3678,20 +3677,20 @@ main(int argc, char *argv[]) > > /* Define inc-proc-engine nodes. */ > ENGINE_NODE(sb_ro, "sb_ro"); > - ENGINE_NODE_WITH_CLEAR_TRACK_DATA_IS_VALID(ct_zones, "ct_zones"); > - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(ovs_interface_shadow, > + ENGINE_NODE_WITH_INIT_RUN_IS_VALID(ct_zones, "ct_zones"); > + ENGINE_NODE_WITH_INIT_RUN(ovs_interface_shadow, > "ovs_interface_shadow"); > - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(runtime_data, "runtime_data"); > + ENGINE_NODE_WITH_INIT_RUN(runtime_data, "runtime_data"); > ENGINE_NODE(non_vif_data, "non_vif_data"); > ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve"); > ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected"); > - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(activated_ports, "activated_ports"); > + ENGINE_NODE_WITH_INIT_RUN(activated_ports, "activated_ports"); > ENGINE_NODE(postponed_ports, "postponed_ports"); > ENGINE_NODE(pflow_output, "physical_flow_output"); > - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lflow_output, "logical_flow_output"); > + ENGINE_NODE_WITH_INIT_RUN(lflow_output, "logical_flow_output"); > ENGINE_NODE(flow_output, "flow_output"); > - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(addr_sets, "addr_sets"); > - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_groups, "port_groups"); > + ENGINE_NODE_WITH_INIT_RUN(addr_sets, "addr_sets"); > + ENGINE_NODE_WITH_INIT_RUN(port_groups, "port_groups"); > ENGINE_NODE(northd_options, "northd_options"); > > #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR); > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c > index 2e2b31033..30a20bb35 100644 > --- a/lib/inc-proc-eng.c > +++ b/lib/inc-proc-eng.c > @@ -196,10 +196,12 @@ void > engine_cleanup(void) > { > for (size_t i = 0; i < engine_n_nodes; i++) { > - if (engine_nodes[i]->clear_tracked_data) { > - engine_nodes[i]->clear_tracked_data(engine_nodes[i]->data); > + if (engine_nodes[i]->init_run) { > + engine_nodes[i]->init_run(engine_nodes[i], engine_nodes[i]->data); > } > + } May I ask why breaking this into two loops? It looks correct in both ways, but I am curious if there is any reason behind this change. Thanks, Han > > + for (size_t i = 0; i < engine_n_nodes; i++) { > if (engine_nodes[i]->cleanup) { > engine_nodes[i]->cleanup(engine_nodes[i]->data); > } > @@ -351,8 +353,8 @@ engine_init_run(void) > for (size_t i = 0; i < engine_n_nodes; i++) { > engine_set_node_state(engine_nodes[i], EN_STALE); > > - if (engine_nodes[i]->clear_tracked_data) { > - engine_nodes[i]->clear_tracked_data(engine_nodes[i]->data); > + if (engine_nodes[i]->init_run) { > + engine_nodes[i]->init_run(engine_nodes[i], engine_nodes[i]->data); > } > } > } > @@ -377,10 +379,11 @@ engine_recompute(struct engine_node *node, bool allowed, > goto done; > } > > - /* Clear tracked data before calling run() so that partially tracked data > - * from some of the change handler executions are cleared. */ > - if (node->clear_tracked_data) { > - node->clear_tracked_data(node->data); > + /* Make sure data is properly initialized before calling run(), e.g., > + * partially tracked data some of the change handler executions must > + * be cleared. */ > + if (node->init_run) { > + node->init_run(node, node->data); > } > > /* Run the node handler which might change state. */ > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h > index dc365dc18..ab5b91b28 100644 > --- a/lib/inc-proc-eng.h > +++ b/lib/inc-proc-eng.h > @@ -150,6 +150,11 @@ struct engine_node { > /* Method to clean up data. It may be NULL. */ > void (*cleanup)(void *data); > > + /* Method to initialize state at every engine run. It can be used for > + * example to clear up tracked data maintained by the engine node in the > + * engine 'data'. It may be NULL. */ > + void (*init_run)(struct engine_node *node, void *data); > + > /* Fully processes all inputs of this node and regenerates the data > * of this node. The pointer to the node's data is passed as argument. > * 'run' handlers can also call engine_get_context() and the > @@ -165,10 +170,6 @@ struct engine_node { > */ > bool (*is_valid)(struct engine_node *); > > - /* Method to clear up tracked data maintained by the engine node in the > - * engine 'data'. It may be NULL. */ > - void (*clear_tracked_data)(void *tracked_data); > - > /* Engine stats. */ > struct engine_stats stats; > }; > @@ -311,20 +312,20 @@ void engine_ovsdb_node_add_index(struct engine_node *, const char *name, > .run = en_##NAME##_run, \ > .cleanup = en_##NAME##_cleanup, \ > .is_valid = NULL, \ > - .clear_tracked_data = NULL, \ > + .init_run = NULL, \ > }; > > -#define ENGINE_NODE_WITH_CLEAR_TRACK_DATA_IS_VALID(NAME, NAME_STR) \ > +#define ENGINE_NODE_WITH_INIT_RUN_IS_VALID(NAME, NAME_STR) \ > ENGINE_NODE(NAME, NAME_STR) \ > - en_##NAME.clear_tracked_data = en_##NAME##_clear_tracked_data; \ > + en_##NAME.init_run = en_##NAME##_init_run; \ > en_##NAME.is_valid = en_##NAME##_is_valid; > > #define ENGINE_NODE(NAME, NAME_STR) \ > ENGINE_NODE_DEF(NAME, NAME_STR) > > -#define ENGINE_NODE_WITH_CLEAR_TRACK_DATA(NAME, NAME_STR) \ > +#define ENGINE_NODE_WITH_INIT_RUN(NAME, NAME_STR) \ > ENGINE_NODE(NAME, NAME_STR) \ > - en_##NAME.clear_tracked_data = en_##NAME##_clear_tracked_data; > + en_##NAME.init_run = en_##NAME##_init_run; > > /* Macro to define member functions of an engine node which represents > * a table of OVSDB */ > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 9/23/22 01:07, Han Zhou wrote: > On Wed, Sep 14, 2022 at 6:10 AM Dumitru Ceara <dceara@redhat.com> wrote: >> >> This is actually more in line with how the callback is used. It's called >> every the incremental engine preparese for the next engine run. >> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > Thanks Dumtru. The name looks good to me, but why does the new function > require both the node and node->data as parameters? > Thanks for the review! Considering that this is an initialization function that runs before every engine run for every node, users might find it interesting to do other things too. For example, getting some OVSDB indexes from input nodes. This is an example from the not yet submitted components template code: static void en_template_vars_init_run(struct engine_node *node, void *data) { struct ed_type_template_vars *tv_data = data; tv_data->sbrec_template_var_table = EN_OVSDB_GET(engine_get_input("SB_template_var", node)); tv_data->ovsrec_ovs_table = EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); tv_data->sbrec_port_binding_by_name = engine_ovsdb_node_get_index(engine_get_input("SB_port_binding", node), "name"); tv_data->sbrec_chassis_by_name = engine_ovsdb_node_get_index(engine_get_input("SB_chassis", node), "name"); sset_clear(&tv_data->new); sset_clear(&tv_data->deleted); sset_clear(&tv_data->updated); tv_data->change_tracked = false; } >> --- >> controller/ovn-controller.c | 41 > ++++++++++++++++++++--------------------- >> lib/inc-proc-eng.c | 19 +++++++++++-------- >> lib/inc-proc-eng.h | 19 ++++++++++--------- >> 3 files changed, 41 insertions(+), 38 deletions(-) >> >> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >> index 18a01bbab..f26d6a9e0 100644 >> --- a/controller/ovn-controller.c >> +++ b/controller/ovn-controller.c >> @@ -1058,7 +1058,7 @@ en_ofctrl_is_connected_run(struct engine_node > *node, void *data) >> * processing to OVS_interface changes but simply mark the node > status as >> * UPDATED (and so the run() and the change handler is the same). >> * 2. The iface_table_external_ids_old is computed/updated in the member >> - * clear_tracked_data(), because that is when the last round of > processing >> + * init_run(), because that is when the last round of processing >> * has completed but the new IDL data is yet to refresh, so we > replace the >> * old data with the current data. */ >> struct ed_type_ovs_interface_shadow { >> @@ -1096,7 +1096,8 @@ en_ovs_interface_shadow_cleanup(void *data_) >> } >> >> static void >> -en_ovs_interface_shadow_clear_tracked_data(void *data_) >> +en_ovs_interface_shadow_init_run(struct engine_node *node OVS_UNUSED, >> + void *data_) >> { >> struct ed_type_ovs_interface_shadow *data = data_; >> > iface_table_external_ids_old_destroy(&data->iface_table_external_ids_old); >> @@ -1163,7 +1164,7 @@ en_activated_ports_cleanup(void *data_) >> } >> >> static void >> -en_activated_ports_clear_tracked_data(void *data) >> +en_activated_ports_init_run(struct engine_node *node OVS_UNUSED, void > *data) >> { >> en_activated_ports_cleanup(data); >> } >> @@ -1311,7 +1312,7 @@ struct ed_type_runtime_data { >> */ >> >> static void >> -en_runtime_data_clear_tracked_data(void *data_) >> +en_runtime_data_init_run(struct engine_node *node OVS_UNUSED, void > *data_) >> { >> struct ed_type_runtime_data *data = data_; >> >> @@ -1669,14 +1670,14 @@ en_addr_sets_init(struct engine_node *node > OVS_UNUSED, >> } >> >> static void >> -en_addr_sets_clear_tracked_data(void *data) >> +en_addr_sets_init_run(struct engine_node *node OVS_UNUSED, void *data) >> { >> struct ed_type_addr_sets *as = data; >> sset_clear(&as->new); >> sset_clear(&as->deleted); >> - struct shash_node *node; >> - SHASH_FOR_EACH_SAFE (node, &as->updated) { >> - struct addr_set_diff *asd = node->data; >> + struct shash_node *as_node; >> + SHASH_FOR_EACH_SAFE (as_node, &as->updated) { >> + struct addr_set_diff *asd = as_node->data; >> expr_constant_set_destroy(asd->added); >> free(asd->added); >> expr_constant_set_destroy(asd->deleted); >> @@ -1689,8 +1690,6 @@ en_addr_sets_clear_tracked_data(void *data) >> static void >> en_addr_sets_cleanup(void *data) >> { >> - en_addr_sets_clear_tracked_data(data); >> - >> struct ed_type_addr_sets *as = data; >> expr_const_sets_destroy(&as->addr_sets); >> shash_destroy(&as->addr_sets); >> @@ -1933,7 +1932,7 @@ port_groups_update(const struct > sbrec_port_group_table *port_group_table, >> } >> >> static void >> -en_port_groups_clear_tracked_data(void *data_) >> +en_port_groups_init_run(struct engine_node *node OVS_UNUSED, void *data_) >> { >> struct ed_type_port_groups *pg = data_; >> sset_clear(&pg->new); >> @@ -2078,7 +2077,7 @@ en_ct_zones_init(struct engine_node *node, struct > engine_arg *arg OVS_UNUSED) >> } >> >> static void >> -en_ct_zones_clear_tracked_data(void *data_) >> +en_ct_zones_init_run(struct engine_node *node OVS_UNUSED, void *data_) >> { >> struct ed_type_ct_zones *data = data_; >> data->recomputed = false; >> @@ -2529,7 +2528,7 @@ struct ed_type_lflow_output { >> struct conj_ids conj_ids; >> >> /* lflows processed in the current engine execution. >> - * Cleared by en_lflow_output_clear_tracked_data before each engine >> + * Cleared by en_lflow_output_init_run before each engine >> * execution. */ >> struct hmap lflows_processed; >> >> @@ -2733,7 +2732,7 @@ en_lflow_output_init(struct engine_node *node > OVS_UNUSED, >> } >> >> static void >> -en_lflow_output_clear_tracked_data(void *data) >> +en_lflow_output_init_run(struct engine_node *node OVS_UNUSED, void *data) >> { >> struct ed_type_lflow_output *flow_output_data = data; >> lflows_processed_destroy(&flow_output_data->lflows_processed); >> @@ -3678,20 +3677,20 @@ main(int argc, char *argv[]) >> >> /* Define inc-proc-engine nodes. */ >> ENGINE_NODE(sb_ro, "sb_ro"); >> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA_IS_VALID(ct_zones, "ct_zones"); >> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(ovs_interface_shadow, >> + ENGINE_NODE_WITH_INIT_RUN_IS_VALID(ct_zones, "ct_zones"); >> + ENGINE_NODE_WITH_INIT_RUN(ovs_interface_shadow, >> "ovs_interface_shadow"); >> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(runtime_data, "runtime_data"); >> + ENGINE_NODE_WITH_INIT_RUN(runtime_data, "runtime_data"); >> ENGINE_NODE(non_vif_data, "non_vif_data"); >> ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve"); >> ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected"); >> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(activated_ports, > "activated_ports"); >> + ENGINE_NODE_WITH_INIT_RUN(activated_ports, "activated_ports"); >> ENGINE_NODE(postponed_ports, "postponed_ports"); >> ENGINE_NODE(pflow_output, "physical_flow_output"); >> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lflow_output, > "logical_flow_output"); >> + ENGINE_NODE_WITH_INIT_RUN(lflow_output, "logical_flow_output"); >> ENGINE_NODE(flow_output, "flow_output"); >> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(addr_sets, "addr_sets"); >> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_groups, "port_groups"); >> + ENGINE_NODE_WITH_INIT_RUN(addr_sets, "addr_sets"); >> + ENGINE_NODE_WITH_INIT_RUN(port_groups, "port_groups"); >> ENGINE_NODE(northd_options, "northd_options"); >> >> #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR); >> diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c >> index 2e2b31033..30a20bb35 100644 >> --- a/lib/inc-proc-eng.c >> +++ b/lib/inc-proc-eng.c >> @@ -196,10 +196,12 @@ void >> engine_cleanup(void) >> { >> for (size_t i = 0; i < engine_n_nodes; i++) { >> - if (engine_nodes[i]->clear_tracked_data) { >> - engine_nodes[i]->clear_tracked_data(engine_nodes[i]->data); >> + if (engine_nodes[i]->init_run) { >> + engine_nodes[i]->init_run(engine_nodes[i], > engine_nodes[i]->data); >> } >> + } > > May I ask why breaking this into two loops? It looks correct in both ways, > but I am curious if there is any reason behind this change. > If we want to allow users to access input nodes in the init_run callback (which is what I showed above) we need to ensure that at cleanup the inputs still exist before calling init_run() for the last time. I can add a comment if that makes it more clear. What do you think? > Thanks, > Han > Thanks, Dumitru >> >> + for (size_t i = 0; i < engine_n_nodes; i++) { >> if (engine_nodes[i]->cleanup) { >> engine_nodes[i]->cleanup(engine_nodes[i]->data); >> } >> @@ -351,8 +353,8 @@ engine_init_run(void) >> for (size_t i = 0; i < engine_n_nodes; i++) { >> engine_set_node_state(engine_nodes[i], EN_STALE); >> >> - if (engine_nodes[i]->clear_tracked_data) { >> - engine_nodes[i]->clear_tracked_data(engine_nodes[i]->data); >> + if (engine_nodes[i]->init_run) { >> + engine_nodes[i]->init_run(engine_nodes[i], > engine_nodes[i]->data); >> } >> } >> } >> @@ -377,10 +379,11 @@ engine_recompute(struct engine_node *node, bool > allowed, >> goto done; >> } >> >> - /* Clear tracked data before calling run() so that partially tracked > data >> - * from some of the change handler executions are cleared. */ >> - if (node->clear_tracked_data) { >> - node->clear_tracked_data(node->data); >> + /* Make sure data is properly initialized before calling run(), e.g., >> + * partially tracked data some of the change handler executions must >> + * be cleared. */ >> + if (node->init_run) { >> + node->init_run(node, node->data); >> } >> >> /* Run the node handler which might change state. */ >> diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h >> index dc365dc18..ab5b91b28 100644 >> --- a/lib/inc-proc-eng.h >> +++ b/lib/inc-proc-eng.h >> @@ -150,6 +150,11 @@ struct engine_node { >> /* Method to clean up data. It may be NULL. */ >> void (*cleanup)(void *data); >> >> + /* Method to initialize state at every engine run. It can be used > for >> + * example to clear up tracked data maintained by the engine node in > the >> + * engine 'data'. It may be NULL. */ >> + void (*init_run)(struct engine_node *node, void *data); >> + >> /* Fully processes all inputs of this node and regenerates the data >> * of this node. The pointer to the node's data is passed as > argument. >> * 'run' handlers can also call engine_get_context() and the >> @@ -165,10 +170,6 @@ struct engine_node { >> */ >> bool (*is_valid)(struct engine_node *); >> >> - /* Method to clear up tracked data maintained by the engine node in > the >> - * engine 'data'. It may be NULL. */ >> - void (*clear_tracked_data)(void *tracked_data); >> - >> /* Engine stats. */ >> struct engine_stats stats; >> }; >> @@ -311,20 +312,20 @@ void engine_ovsdb_node_add_index(struct engine_node > *, const char *name, >> .run = en_##NAME##_run, \ >> .cleanup = en_##NAME##_cleanup, \ >> .is_valid = NULL, \ >> - .clear_tracked_data = NULL, \ >> + .init_run = NULL, \ >> }; >> >> -#define ENGINE_NODE_WITH_CLEAR_TRACK_DATA_IS_VALID(NAME, NAME_STR) \ >> +#define ENGINE_NODE_WITH_INIT_RUN_IS_VALID(NAME, NAME_STR) \ >> ENGINE_NODE(NAME, NAME_STR) \ >> - en_##NAME.clear_tracked_data = en_##NAME##_clear_tracked_data; \ >> + en_##NAME.init_run = en_##NAME##_init_run; \ >> en_##NAME.is_valid = en_##NAME##_is_valid; >> >> #define ENGINE_NODE(NAME, NAME_STR) \ >> ENGINE_NODE_DEF(NAME, NAME_STR) >> >> -#define ENGINE_NODE_WITH_CLEAR_TRACK_DATA(NAME, NAME_STR) \ >> +#define ENGINE_NODE_WITH_INIT_RUN(NAME, NAME_STR) \ >> ENGINE_NODE(NAME, NAME_STR) \ >> - en_##NAME.clear_tracked_data = en_##NAME##_clear_tracked_data; >> + en_##NAME.init_run = en_##NAME##_init_run; >> >> /* Macro to define member functions of an engine node which represents >> * a table of OVSDB */ >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Fri, Sep 23, 2022 at 1:42 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 9/23/22 01:07, Han Zhou wrote: > > On Wed, Sep 14, 2022 at 6:10 AM Dumitru Ceara <dceara@redhat.com> wrote: > >> > >> This is actually more in line with how the callback is used. It's called > >> every the incremental engine preparese for the next engine run. > >> > >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > > > Thanks Dumtru. The name looks good to me, but why does the new function > > require both the node and node->data as parameters? > > > > Thanks for the review! Considering that this is an initialization > function that runs before every engine run for every node, users might > find it interesting to do other things too. For example, getting some > OVSDB indexes from input nodes. > > This is an example from the not yet submitted components template code: > > static void > en_template_vars_init_run(struct engine_node *node, void *data) > { > struct ed_type_template_vars *tv_data = data; > > tv_data->sbrec_template_var_table = > EN_OVSDB_GET(engine_get_input("SB_template_var", node)); > tv_data->ovsrec_ovs_table = > EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); > tv_data->sbrec_port_binding_by_name = > engine_ovsdb_node_get_index(engine_get_input("SB_port_binding", node), > "name"); > tv_data->sbrec_chassis_by_name = > engine_ovsdb_node_get_index(engine_get_input("SB_chassis", node), > "name"); > > sset_clear(&tv_data->new); > sset_clear(&tv_data->deleted); > sset_clear(&tv_data->updated); > tv_data->change_tracked = false; > } > I don't quite understand this example. It seems ed_type_template_vars stores some of its input to its own data, but could you explain why? These members should belong to the input nodes, and they can always be accessed in the run() or handler functions. If it requires more code to explain, I'd suggest including this as part of your *template* series so that it is easier to review together. > >> --- > >> controller/ovn-controller.c | 41 > > ++++++++++++++++++++--------------------- > >> lib/inc-proc-eng.c | 19 +++++++++++-------- > >> lib/inc-proc-eng.h | 19 ++++++++++--------- > >> 3 files changed, 41 insertions(+), 38 deletions(-) > >> > >> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > >> index 18a01bbab..f26d6a9e0 100644 > >> --- a/controller/ovn-controller.c > >> +++ b/controller/ovn-controller.c > >> @@ -1058,7 +1058,7 @@ en_ofctrl_is_connected_run(struct engine_node > > *node, void *data) > >> * processing to OVS_interface changes but simply mark the node > > status as > >> * UPDATED (and so the run() and the change handler is the same). > >> * 2. The iface_table_external_ids_old is computed/updated in the member > >> - * clear_tracked_data(), because that is when the last round of > > processing > >> + * init_run(), because that is when the last round of processing > >> * has completed but the new IDL data is yet to refresh, so we > > replace the > >> * old data with the current data. */ > >> struct ed_type_ovs_interface_shadow { > >> @@ -1096,7 +1096,8 @@ en_ovs_interface_shadow_cleanup(void *data_) > >> } > >> > >> static void > >> -en_ovs_interface_shadow_clear_tracked_data(void *data_) > >> +en_ovs_interface_shadow_init_run(struct engine_node *node OVS_UNUSED, > >> + void *data_) > >> { > >> struct ed_type_ovs_interface_shadow *data = data_; > >> > > iface_table_external_ids_old_destroy(&data->iface_table_external_ids_old); > >> @@ -1163,7 +1164,7 @@ en_activated_ports_cleanup(void *data_) > >> } > >> > >> static void > >> -en_activated_ports_clear_tracked_data(void *data) > >> +en_activated_ports_init_run(struct engine_node *node OVS_UNUSED, void > > *data) > >> { > >> en_activated_ports_cleanup(data); > >> } > >> @@ -1311,7 +1312,7 @@ struct ed_type_runtime_data { > >> */ > >> > >> static void > >> -en_runtime_data_clear_tracked_data(void *data_) > >> +en_runtime_data_init_run(struct engine_node *node OVS_UNUSED, void > > *data_) > >> { > >> struct ed_type_runtime_data *data = data_; > >> > >> @@ -1669,14 +1670,14 @@ en_addr_sets_init(struct engine_node *node > > OVS_UNUSED, > >> } > >> > >> static void > >> -en_addr_sets_clear_tracked_data(void *data) > >> +en_addr_sets_init_run(struct engine_node *node OVS_UNUSED, void *data) > >> { > >> struct ed_type_addr_sets *as = data; > >> sset_clear(&as->new); > >> sset_clear(&as->deleted); > >> - struct shash_node *node; > >> - SHASH_FOR_EACH_SAFE (node, &as->updated) { > >> - struct addr_set_diff *asd = node->data; > >> + struct shash_node *as_node; > >> + SHASH_FOR_EACH_SAFE (as_node, &as->updated) { > >> + struct addr_set_diff *asd = as_node->data; > >> expr_constant_set_destroy(asd->added); > >> free(asd->added); > >> expr_constant_set_destroy(asd->deleted); > >> @@ -1689,8 +1690,6 @@ en_addr_sets_clear_tracked_data(void *data) > >> static void > >> en_addr_sets_cleanup(void *data) > >> { > >> - en_addr_sets_clear_tracked_data(data); > >> - > >> struct ed_type_addr_sets *as = data; > >> expr_const_sets_destroy(&as->addr_sets); > >> shash_destroy(&as->addr_sets); > >> @@ -1933,7 +1932,7 @@ port_groups_update(const struct > > sbrec_port_group_table *port_group_table, > >> } > >> > >> static void > >> -en_port_groups_clear_tracked_data(void *data_) > >> +en_port_groups_init_run(struct engine_node *node OVS_UNUSED, void *data_) > >> { > >> struct ed_type_port_groups *pg = data_; > >> sset_clear(&pg->new); > >> @@ -2078,7 +2077,7 @@ en_ct_zones_init(struct engine_node *node, struct > > engine_arg *arg OVS_UNUSED) > >> } > >> > >> static void > >> -en_ct_zones_clear_tracked_data(void *data_) > >> +en_ct_zones_init_run(struct engine_node *node OVS_UNUSED, void *data_) > >> { > >> struct ed_type_ct_zones *data = data_; > >> data->recomputed = false; > >> @@ -2529,7 +2528,7 @@ struct ed_type_lflow_output { > >> struct conj_ids conj_ids; > >> > >> /* lflows processed in the current engine execution. > >> - * Cleared by en_lflow_output_clear_tracked_data before each engine > >> + * Cleared by en_lflow_output_init_run before each engine > >> * execution. */ > >> struct hmap lflows_processed; > >> > >> @@ -2733,7 +2732,7 @@ en_lflow_output_init(struct engine_node *node > > OVS_UNUSED, > >> } > >> > >> static void > >> -en_lflow_output_clear_tracked_data(void *data) > >> +en_lflow_output_init_run(struct engine_node *node OVS_UNUSED, void *data) > >> { > >> struct ed_type_lflow_output *flow_output_data = data; > >> lflows_processed_destroy(&flow_output_data->lflows_processed); > >> @@ -3678,20 +3677,20 @@ main(int argc, char *argv[]) > >> > >> /* Define inc-proc-engine nodes. */ > >> ENGINE_NODE(sb_ro, "sb_ro"); > >> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA_IS_VALID(ct_zones, "ct_zones"); > >> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(ovs_interface_shadow, > >> + ENGINE_NODE_WITH_INIT_RUN_IS_VALID(ct_zones, "ct_zones"); > >> + ENGINE_NODE_WITH_INIT_RUN(ovs_interface_shadow, > >> "ovs_interface_shadow"); > >> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(runtime_data, "runtime_data"); > >> + ENGINE_NODE_WITH_INIT_RUN(runtime_data, "runtime_data"); > >> ENGINE_NODE(non_vif_data, "non_vif_data"); > >> ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve"); > >> ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected"); > >> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(activated_ports, > > "activated_ports"); > >> + ENGINE_NODE_WITH_INIT_RUN(activated_ports, "activated_ports"); > >> ENGINE_NODE(postponed_ports, "postponed_ports"); > >> ENGINE_NODE(pflow_output, "physical_flow_output"); > >> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lflow_output, > > "logical_flow_output"); > >> + ENGINE_NODE_WITH_INIT_RUN(lflow_output, "logical_flow_output"); > >> ENGINE_NODE(flow_output, "flow_output"); > >> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(addr_sets, "addr_sets"); > >> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_groups, "port_groups"); > >> + ENGINE_NODE_WITH_INIT_RUN(addr_sets, "addr_sets"); > >> + ENGINE_NODE_WITH_INIT_RUN(port_groups, "port_groups"); > >> ENGINE_NODE(northd_options, "northd_options"); > >> > >> #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR); > >> diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c > >> index 2e2b31033..30a20bb35 100644 > >> --- a/lib/inc-proc-eng.c > >> +++ b/lib/inc-proc-eng.c > >> @@ -196,10 +196,12 @@ void > >> engine_cleanup(void) > >> { > >> for (size_t i = 0; i < engine_n_nodes; i++) { > >> - if (engine_nodes[i]->clear_tracked_data) { > >> - engine_nodes[i]->clear_tracked_data(engine_nodes[i]->data); > >> + if (engine_nodes[i]->init_run) { > >> + engine_nodes[i]->init_run(engine_nodes[i], > > engine_nodes[i]->data); > >> } > >> + } > > > > May I ask why breaking this into two loops? It looks correct in both ways, > > but I am curious if there is any reason behind this change. > > > > If we want to allow users to access input nodes in the init_run callback > (which is what I showed above) we need to ensure that at cleanup the > inputs still exist before calling init_run() for the last time. > > I can add a comment if that makes it more clear. What do you think? > A comment may be helpful, but I am also wondering shall we just avoid relying on calling init_run() for cleanup? Maybe in some situations we don't really want some of the initialization logic to get called at engine_cleanup. The name init_run suggests that it is normal to perform initializations including possible memory allocations, which is definitely not something we want in engine_cleanup(). We could either: 1. split this into two functions: - init_run(): called at every iteration - clear_tracked_data(): called at every iteration, and also at the engine_cleanup (this may leads to tedious macro definitions) OR, 2. let each node implementation takes care of: - clean up tracked data at init_run(), and - clean up tracked data at the final cleanup() // instead of expecting engine_cleanup to call init_run(). Thanks, Han > > Thanks, > > Han > > > > Thanks, > Dumitru > > >> > >> + for (size_t i = 0; i < engine_n_nodes; i++) { > >> if (engine_nodes[i]->cleanup) { > >> engine_nodes[i]->cleanup(engine_nodes[i]->data); > >> } > >> @@ -351,8 +353,8 @@ engine_init_run(void) > >> for (size_t i = 0; i < engine_n_nodes; i++) { > >> engine_set_node_state(engine_nodes[i], EN_STALE); > >> > >> - if (engine_nodes[i]->clear_tracked_data) { > >> - engine_nodes[i]->clear_tracked_data(engine_nodes[i]->data); > >> + if (engine_nodes[i]->init_run) { > >> + engine_nodes[i]->init_run(engine_nodes[i], > > engine_nodes[i]->data); > >> } > >> } > >> } > >> @@ -377,10 +379,11 @@ engine_recompute(struct engine_node *node, bool > > allowed, > >> goto done; > >> } > >> > >> - /* Clear tracked data before calling run() so that partially tracked > > data > >> - * from some of the change handler executions are cleared. */ > >> - if (node->clear_tracked_data) { > >> - node->clear_tracked_data(node->data); > >> + /* Make sure data is properly initialized before calling run(), e.g., > >> + * partially tracked data some of the change handler executions must > >> + * be cleared. */ > >> + if (node->init_run) { > >> + node->init_run(node, node->data); > >> } > >> > >> /* Run the node handler which might change state. */ > >> diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h > >> index dc365dc18..ab5b91b28 100644 > >> --- a/lib/inc-proc-eng.h > >> +++ b/lib/inc-proc-eng.h > >> @@ -150,6 +150,11 @@ struct engine_node { > >> /* Method to clean up data. It may be NULL. */ > >> void (*cleanup)(void *data); > >> > >> + /* Method to initialize state at every engine run. It can be used > > for > >> + * example to clear up tracked data maintained by the engine node in > > the > >> + * engine 'data'. It may be NULL. */ > >> + void (*init_run)(struct engine_node *node, void *data); > >> + > >> /* Fully processes all inputs of this node and regenerates the data > >> * of this node. The pointer to the node's data is passed as > > argument. > >> * 'run' handlers can also call engine_get_context() and the > >> @@ -165,10 +170,6 @@ struct engine_node { > >> */ > >> bool (*is_valid)(struct engine_node *); > >> > >> - /* Method to clear up tracked data maintained by the engine node in > > the > >> - * engine 'data'. It may be NULL. */ > >> - void (*clear_tracked_data)(void *tracked_data); > >> - > >> /* Engine stats. */ > >> struct engine_stats stats; > >> }; > >> @@ -311,20 +312,20 @@ void engine_ovsdb_node_add_index(struct engine_node > > *, const char *name, > >> .run = en_##NAME##_run, \ > >> .cleanup = en_##NAME##_cleanup, \ > >> .is_valid = NULL, \ > >> - .clear_tracked_data = NULL, \ > >> + .init_run = NULL, \ > >> }; > >> > >> -#define ENGINE_NODE_WITH_CLEAR_TRACK_DATA_IS_VALID(NAME, NAME_STR) \ > >> +#define ENGINE_NODE_WITH_INIT_RUN_IS_VALID(NAME, NAME_STR) \ > >> ENGINE_NODE(NAME, NAME_STR) \ > >> - en_##NAME.clear_tracked_data = en_##NAME##_clear_tracked_data; \ > >> + en_##NAME.init_run = en_##NAME##_init_run; \ > >> en_##NAME.is_valid = en_##NAME##_is_valid; > >> > >> #define ENGINE_NODE(NAME, NAME_STR) \ > >> ENGINE_NODE_DEF(NAME, NAME_STR) > >> > >> -#define ENGINE_NODE_WITH_CLEAR_TRACK_DATA(NAME, NAME_STR) \ > >> +#define ENGINE_NODE_WITH_INIT_RUN(NAME, NAME_STR) \ > >> ENGINE_NODE(NAME, NAME_STR) \ > >> - en_##NAME.clear_tracked_data = en_##NAME##_clear_tracked_data; > >> + en_##NAME.init_run = en_##NAME##_init_run; > >> > >> /* Macro to define member functions of an engine node which represents > >> * a table of OVSDB */ > >> > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
On 9/23/22 17:47, Han Zhou wrote: > On Fri, Sep 23, 2022 at 1:42 AM Dumitru Ceara <dceara@redhat.com> wrote: >> >> On 9/23/22 01:07, Han Zhou wrote: >>> On Wed, Sep 14, 2022 at 6:10 AM Dumitru Ceara <dceara@redhat.com> wrote: >>>> >>>> This is actually more in line with how the callback is used. It's > called >>>> every the incremental engine preparese for the next engine run. >>>> >>>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >>> >>> Thanks Dumtru. The name looks good to me, but why does the new function >>> require both the node and node->data as parameters? >>> >> >> Thanks for the review! Considering that this is an initialization >> function that runs before every engine run for every node, users might >> find it interesting to do other things too. For example, getting some >> OVSDB indexes from input nodes. >> >> This is an example from the not yet submitted components template code: >> >> static void >> en_template_vars_init_run(struct engine_node *node, void *data) >> { >> struct ed_type_template_vars *tv_data = data; >> >> tv_data->sbrec_template_var_table = >> EN_OVSDB_GET(engine_get_input("SB_template_var", node)); >> tv_data->ovsrec_ovs_table = >> EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); >> tv_data->sbrec_port_binding_by_name = >> engine_ovsdb_node_get_index(engine_get_input("SB_port_binding", > node), >> "name"); >> tv_data->sbrec_chassis_by_name = >> engine_ovsdb_node_get_index(engine_get_input("SB_chassis", node), >> "name"); >> >> sset_clear(&tv_data->new); >> sset_clear(&tv_data->deleted); >> sset_clear(&tv_data->updated); >> tv_data->change_tracked = false; >> } >> > > I don't quite understand this example. It seems ed_type_template_vars > stores some of its input to its own data, but could you explain why? These It was just to avoid looking up the indexes every time. But you're right there's no huge benefit to doing that. > members should belong to the input nodes, and they can always be accessed > in the run() or handler functions. If it requires more code to explain, > I'd suggest including this as part of your *template* series so that it is > easier to review together. > Sounds good. >>>> --- >>>> controller/ovn-controller.c | 41 >>> ++++++++++++++++++++--------------------- >>>> lib/inc-proc-eng.c | 19 +++++++++++-------- >>>> lib/inc-proc-eng.h | 19 ++++++++++--------- >>>> 3 files changed, 41 insertions(+), 38 deletions(-) >>>> >>>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >>>> index 18a01bbab..f26d6a9e0 100644 >>>> --- a/controller/ovn-controller.c >>>> +++ b/controller/ovn-controller.c >>>> @@ -1058,7 +1058,7 @@ en_ofctrl_is_connected_run(struct engine_node >>> *node, void *data) >>>> * processing to OVS_interface changes but simply mark the node >>> status as >>>> * UPDATED (and so the run() and the change handler is the same). >>>> * 2. The iface_table_external_ids_old is computed/updated in the > member >>>> - * clear_tracked_data(), because that is when the last round of >>> processing >>>> + * init_run(), because that is when the last round of processing >>>> * has completed but the new IDL data is yet to refresh, so we >>> replace the >>>> * old data with the current data. */ >>>> struct ed_type_ovs_interface_shadow { >>>> @@ -1096,7 +1096,8 @@ en_ovs_interface_shadow_cleanup(void *data_) >>>> } >>>> >>>> static void >>>> -en_ovs_interface_shadow_clear_tracked_data(void *data_) >>>> +en_ovs_interface_shadow_init_run(struct engine_node *node OVS_UNUSED, >>>> + void *data_) >>>> { >>>> struct ed_type_ovs_interface_shadow *data = data_; >>>> >>> > iface_table_external_ids_old_destroy(&data->iface_table_external_ids_old); >>>> @@ -1163,7 +1164,7 @@ en_activated_ports_cleanup(void *data_) >>>> } >>>> >>>> static void >>>> -en_activated_ports_clear_tracked_data(void *data) >>>> +en_activated_ports_init_run(struct engine_node *node OVS_UNUSED, void >>> *data) >>>> { >>>> en_activated_ports_cleanup(data); >>>> } >>>> @@ -1311,7 +1312,7 @@ struct ed_type_runtime_data { >>>> */ >>>> >>>> static void >>>> -en_runtime_data_clear_tracked_data(void *data_) >>>> +en_runtime_data_init_run(struct engine_node *node OVS_UNUSED, void >>> *data_) >>>> { >>>> struct ed_type_runtime_data *data = data_; >>>> >>>> @@ -1669,14 +1670,14 @@ en_addr_sets_init(struct engine_node *node >>> OVS_UNUSED, >>>> } >>>> >>>> static void >>>> -en_addr_sets_clear_tracked_data(void *data) >>>> +en_addr_sets_init_run(struct engine_node *node OVS_UNUSED, void *data) >>>> { >>>> struct ed_type_addr_sets *as = data; >>>> sset_clear(&as->new); >>>> sset_clear(&as->deleted); >>>> - struct shash_node *node; >>>> - SHASH_FOR_EACH_SAFE (node, &as->updated) { >>>> - struct addr_set_diff *asd = node->data; >>>> + struct shash_node *as_node; >>>> + SHASH_FOR_EACH_SAFE (as_node, &as->updated) { >>>> + struct addr_set_diff *asd = as_node->data; >>>> expr_constant_set_destroy(asd->added); >>>> free(asd->added); >>>> expr_constant_set_destroy(asd->deleted); >>>> @@ -1689,8 +1690,6 @@ en_addr_sets_clear_tracked_data(void *data) >>>> static void >>>> en_addr_sets_cleanup(void *data) >>>> { >>>> - en_addr_sets_clear_tracked_data(data); >>>> - >>>> struct ed_type_addr_sets *as = data; >>>> expr_const_sets_destroy(&as->addr_sets); >>>> shash_destroy(&as->addr_sets); >>>> @@ -1933,7 +1932,7 @@ port_groups_update(const struct >>> sbrec_port_group_table *port_group_table, >>>> } >>>> >>>> static void >>>> -en_port_groups_clear_tracked_data(void *data_) >>>> +en_port_groups_init_run(struct engine_node *node OVS_UNUSED, void > *data_) >>>> { >>>> struct ed_type_port_groups *pg = data_; >>>> sset_clear(&pg->new); >>>> @@ -2078,7 +2077,7 @@ en_ct_zones_init(struct engine_node *node, struct >>> engine_arg *arg OVS_UNUSED) >>>> } >>>> >>>> static void >>>> -en_ct_zones_clear_tracked_data(void *data_) >>>> +en_ct_zones_init_run(struct engine_node *node OVS_UNUSED, void *data_) >>>> { >>>> struct ed_type_ct_zones *data = data_; >>>> data->recomputed = false; >>>> @@ -2529,7 +2528,7 @@ struct ed_type_lflow_output { >>>> struct conj_ids conj_ids; >>>> >>>> /* lflows processed in the current engine execution. >>>> - * Cleared by en_lflow_output_clear_tracked_data before each > engine >>>> + * Cleared by en_lflow_output_init_run before each engine >>>> * execution. */ >>>> struct hmap lflows_processed; >>>> >>>> @@ -2733,7 +2732,7 @@ en_lflow_output_init(struct engine_node *node >>> OVS_UNUSED, >>>> } >>>> >>>> static void >>>> -en_lflow_output_clear_tracked_data(void *data) >>>> +en_lflow_output_init_run(struct engine_node *node OVS_UNUSED, void > *data) >>>> { >>>> struct ed_type_lflow_output *flow_output_data = data; >>>> lflows_processed_destroy(&flow_output_data->lflows_processed); >>>> @@ -3678,20 +3677,20 @@ main(int argc, char *argv[]) >>>> >>>> /* Define inc-proc-engine nodes. */ >>>> ENGINE_NODE(sb_ro, "sb_ro"); >>>> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA_IS_VALID(ct_zones, "ct_zones"); >>>> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(ovs_interface_shadow, >>>> + ENGINE_NODE_WITH_INIT_RUN_IS_VALID(ct_zones, "ct_zones"); >>>> + ENGINE_NODE_WITH_INIT_RUN(ovs_interface_shadow, >>>> "ovs_interface_shadow"); >>>> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(runtime_data, "runtime_data"); >>>> + ENGINE_NODE_WITH_INIT_RUN(runtime_data, "runtime_data"); >>>> ENGINE_NODE(non_vif_data, "non_vif_data"); >>>> ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve"); >>>> ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected"); >>>> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(activated_ports, >>> "activated_ports"); >>>> + ENGINE_NODE_WITH_INIT_RUN(activated_ports, "activated_ports"); >>>> ENGINE_NODE(postponed_ports, "postponed_ports"); >>>> ENGINE_NODE(pflow_output, "physical_flow_output"); >>>> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lflow_output, >>> "logical_flow_output"); >>>> + ENGINE_NODE_WITH_INIT_RUN(lflow_output, "logical_flow_output"); >>>> ENGINE_NODE(flow_output, "flow_output"); >>>> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(addr_sets, "addr_sets"); >>>> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_groups, "port_groups"); >>>> + ENGINE_NODE_WITH_INIT_RUN(addr_sets, "addr_sets"); >>>> + ENGINE_NODE_WITH_INIT_RUN(port_groups, "port_groups"); >>>> ENGINE_NODE(northd_options, "northd_options"); >>>> >>>> #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR); >>>> diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c >>>> index 2e2b31033..30a20bb35 100644 >>>> --- a/lib/inc-proc-eng.c >>>> +++ b/lib/inc-proc-eng.c >>>> @@ -196,10 +196,12 @@ void >>>> engine_cleanup(void) >>>> { >>>> for (size_t i = 0; i < engine_n_nodes; i++) { >>>> - if (engine_nodes[i]->clear_tracked_data) { >>>> - > engine_nodes[i]->clear_tracked_data(engine_nodes[i]->data); >>>> + if (engine_nodes[i]->init_run) { >>>> + engine_nodes[i]->init_run(engine_nodes[i], >>> engine_nodes[i]->data); >>>> } >>>> + } >>> >>> May I ask why breaking this into two loops? It looks correct in both > ways, >>> but I am curious if there is any reason behind this change. >>> >> >> If we want to allow users to access input nodes in the init_run callback >> (which is what I showed above) we need to ensure that at cleanup the >> inputs still exist before calling init_run() for the last time. >> >> I can add a comment if that makes it more clear. What do you think? >> > A comment may be helpful, but I am also wondering shall we just avoid > relying on calling init_run() for cleanup? Maybe in some situations we > don't really want some of the initialization logic to get called at > engine_cleanup. The name init_run suggests that it is normal to perform > initializations including possible memory allocations, which is definitely > not something we want in engine_cleanup(). > True, this is the real issue we probably need both things: initialize some data before every run *and* cleanup some data at the end of a run. > We could either: > 1. split this into two functions: > - init_run(): called at every iteration > - clear_tracked_data(): called at every iteration, and also at the > engine_cleanup I would choose this implementation to be honest. > (this may leads to tedious macro definitions) Maybe we should just remove most of the generic engine node definition macros (we can keep the ovsdb ones) and use explicit node definition with designated initializers. I can try it out and if it looks OK I can post something like that. > OR, > 2. let each node implementation takes care of: > - clean up tracked data at init_run(), and > - clean up tracked data at the final cleanup() // instead of expecting > engine_cleanup to call init_run(). This might still be "sub-optimal" because some nodes need to do some sort of cleanup after every run and no explicit initialization. That would mean they'd still need to do that work in the init_run() function. Approach #1 above deals with this in a better way. > > Thanks, > Han > In conclusion, I'll drop this patch from this series and try to rework it and include it in the component template series. Thanks, Dumitru >>> Thanks, >>> Han >>> >> >> Thanks, >> Dumitru >> >>>> >>>> + for (size_t i = 0; i < engine_n_nodes; i++) { >>>> if (engine_nodes[i]->cleanup) { >>>> engine_nodes[i]->cleanup(engine_nodes[i]->data); >>>> } >>>> @@ -351,8 +353,8 @@ engine_init_run(void) >>>> for (size_t i = 0; i < engine_n_nodes; i++) { >>>> engine_set_node_state(engine_nodes[i], EN_STALE); >>>> >>>> - if (engine_nodes[i]->clear_tracked_data) { >>>> - > engine_nodes[i]->clear_tracked_data(engine_nodes[i]->data); >>>> + if (engine_nodes[i]->init_run) { >>>> + engine_nodes[i]->init_run(engine_nodes[i], >>> engine_nodes[i]->data); >>>> } >>>> } >>>> } >>>> @@ -377,10 +379,11 @@ engine_recompute(struct engine_node *node, bool >>> allowed, >>>> goto done; >>>> } >>>> >>>> - /* Clear tracked data before calling run() so that partially > tracked >>> data >>>> - * from some of the change handler executions are cleared. */ >>>> - if (node->clear_tracked_data) { >>>> - node->clear_tracked_data(node->data); >>>> + /* Make sure data is properly initialized before calling run(), > e.g., >>>> + * partially tracked data some of the change handler executions > must >>>> + * be cleared. */ >>>> + if (node->init_run) { >>>> + node->init_run(node, node->data); >>>> } >>>> >>>> /* Run the node handler which might change state. */ >>>> diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h >>>> index dc365dc18..ab5b91b28 100644 >>>> --- a/lib/inc-proc-eng.h >>>> +++ b/lib/inc-proc-eng.h >>>> @@ -150,6 +150,11 @@ struct engine_node { >>>> /* Method to clean up data. It may be NULL. */ >>>> void (*cleanup)(void *data); >>>> >>>> + /* Method to initialize state at every engine run. It can be used >>> for >>>> + * example to clear up tracked data maintained by the engine node > in >>> the >>>> + * engine 'data'. It may be NULL. */ >>>> + void (*init_run)(struct engine_node *node, void *data); >>>> + >>>> /* Fully processes all inputs of this node and regenerates the > data >>>> * of this node. The pointer to the node's data is passed as >>> argument. >>>> * 'run' handlers can also call engine_get_context() and the >>>> @@ -165,10 +170,6 @@ struct engine_node { >>>> */ >>>> bool (*is_valid)(struct engine_node *); >>>> >>>> - /* Method to clear up tracked data maintained by the engine node > in >>> the >>>> - * engine 'data'. It may be NULL. */ >>>> - void (*clear_tracked_data)(void *tracked_data); >>>> - >>>> /* Engine stats. */ >>>> struct engine_stats stats; >>>> }; >>>> @@ -311,20 +312,20 @@ void engine_ovsdb_node_add_index(struct > engine_node >>> *, const char *name, >>>> .run = en_##NAME##_run, \ >>>> .cleanup = en_##NAME##_cleanup, \ >>>> .is_valid = NULL, \ >>>> - .clear_tracked_data = NULL, \ >>>> + .init_run = NULL, \ >>>> }; >>>> >>>> -#define ENGINE_NODE_WITH_CLEAR_TRACK_DATA_IS_VALID(NAME, NAME_STR) \ >>>> +#define ENGINE_NODE_WITH_INIT_RUN_IS_VALID(NAME, NAME_STR) \ >>>> ENGINE_NODE(NAME, NAME_STR) \ >>>> - en_##NAME.clear_tracked_data = en_##NAME##_clear_tracked_data; \ >>>> + en_##NAME.init_run = en_##NAME##_init_run; \ >>>> en_##NAME.is_valid = en_##NAME##_is_valid; >>>> >>>> #define ENGINE_NODE(NAME, NAME_STR) \ >>>> ENGINE_NODE_DEF(NAME, NAME_STR) >>>> >>>> -#define ENGINE_NODE_WITH_CLEAR_TRACK_DATA(NAME, NAME_STR) \ >>>> +#define ENGINE_NODE_WITH_INIT_RUN(NAME, NAME_STR) \ >>>> ENGINE_NODE(NAME, NAME_STR) \ >>>> - en_##NAME.clear_tracked_data = en_##NAME##_clear_tracked_data; >>>> + en_##NAME.init_run = en_##NAME##_init_run; >>>> >>>> /* Macro to define member functions of an engine node which represents >>>> * a table of OVSDB */ >>>> >>>> _______________________________________________ >>>> dev mailing list >>>> dev@openvswitch.org >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >> >
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 18a01bbab..f26d6a9e0 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1058,7 +1058,7 @@ en_ofctrl_is_connected_run(struct engine_node *node, void *data) * processing to OVS_interface changes but simply mark the node status as * UPDATED (and so the run() and the change handler is the same). * 2. The iface_table_external_ids_old is computed/updated in the member - * clear_tracked_data(), because that is when the last round of processing + * init_run(), because that is when the last round of processing * has completed but the new IDL data is yet to refresh, so we replace the * old data with the current data. */ struct ed_type_ovs_interface_shadow { @@ -1096,7 +1096,8 @@ en_ovs_interface_shadow_cleanup(void *data_) } static void -en_ovs_interface_shadow_clear_tracked_data(void *data_) +en_ovs_interface_shadow_init_run(struct engine_node *node OVS_UNUSED, + void *data_) { struct ed_type_ovs_interface_shadow *data = data_; iface_table_external_ids_old_destroy(&data->iface_table_external_ids_old); @@ -1163,7 +1164,7 @@ en_activated_ports_cleanup(void *data_) } static void -en_activated_ports_clear_tracked_data(void *data) +en_activated_ports_init_run(struct engine_node *node OVS_UNUSED, void *data) { en_activated_ports_cleanup(data); } @@ -1311,7 +1312,7 @@ struct ed_type_runtime_data { */ static void -en_runtime_data_clear_tracked_data(void *data_) +en_runtime_data_init_run(struct engine_node *node OVS_UNUSED, void *data_) { struct ed_type_runtime_data *data = data_; @@ -1669,14 +1670,14 @@ en_addr_sets_init(struct engine_node *node OVS_UNUSED, } static void -en_addr_sets_clear_tracked_data(void *data) +en_addr_sets_init_run(struct engine_node *node OVS_UNUSED, void *data) { struct ed_type_addr_sets *as = data; sset_clear(&as->new); sset_clear(&as->deleted); - struct shash_node *node; - SHASH_FOR_EACH_SAFE (node, &as->updated) { - struct addr_set_diff *asd = node->data; + struct shash_node *as_node; + SHASH_FOR_EACH_SAFE (as_node, &as->updated) { + struct addr_set_diff *asd = as_node->data; expr_constant_set_destroy(asd->added); free(asd->added); expr_constant_set_destroy(asd->deleted); @@ -1689,8 +1690,6 @@ en_addr_sets_clear_tracked_data(void *data) static void en_addr_sets_cleanup(void *data) { - en_addr_sets_clear_tracked_data(data); - struct ed_type_addr_sets *as = data; expr_const_sets_destroy(&as->addr_sets); shash_destroy(&as->addr_sets); @@ -1933,7 +1932,7 @@ port_groups_update(const struct sbrec_port_group_table *port_group_table, } static void -en_port_groups_clear_tracked_data(void *data_) +en_port_groups_init_run(struct engine_node *node OVS_UNUSED, void *data_) { struct ed_type_port_groups *pg = data_; sset_clear(&pg->new); @@ -2078,7 +2077,7 @@ en_ct_zones_init(struct engine_node *node, struct engine_arg *arg OVS_UNUSED) } static void -en_ct_zones_clear_tracked_data(void *data_) +en_ct_zones_init_run(struct engine_node *node OVS_UNUSED, void *data_) { struct ed_type_ct_zones *data = data_; data->recomputed = false; @@ -2529,7 +2528,7 @@ struct ed_type_lflow_output { struct conj_ids conj_ids; /* lflows processed in the current engine execution. - * Cleared by en_lflow_output_clear_tracked_data before each engine + * Cleared by en_lflow_output_init_run before each engine * execution. */ struct hmap lflows_processed; @@ -2733,7 +2732,7 @@ en_lflow_output_init(struct engine_node *node OVS_UNUSED, } static void -en_lflow_output_clear_tracked_data(void *data) +en_lflow_output_init_run(struct engine_node *node OVS_UNUSED, void *data) { struct ed_type_lflow_output *flow_output_data = data; lflows_processed_destroy(&flow_output_data->lflows_processed); @@ -3678,20 +3677,20 @@ main(int argc, char *argv[]) /* Define inc-proc-engine nodes. */ ENGINE_NODE(sb_ro, "sb_ro"); - ENGINE_NODE_WITH_CLEAR_TRACK_DATA_IS_VALID(ct_zones, "ct_zones"); - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(ovs_interface_shadow, + ENGINE_NODE_WITH_INIT_RUN_IS_VALID(ct_zones, "ct_zones"); + ENGINE_NODE_WITH_INIT_RUN(ovs_interface_shadow, "ovs_interface_shadow"); - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(runtime_data, "runtime_data"); + ENGINE_NODE_WITH_INIT_RUN(runtime_data, "runtime_data"); ENGINE_NODE(non_vif_data, "non_vif_data"); ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve"); ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected"); - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(activated_ports, "activated_ports"); + ENGINE_NODE_WITH_INIT_RUN(activated_ports, "activated_ports"); ENGINE_NODE(postponed_ports, "postponed_ports"); ENGINE_NODE(pflow_output, "physical_flow_output"); - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lflow_output, "logical_flow_output"); + ENGINE_NODE_WITH_INIT_RUN(lflow_output, "logical_flow_output"); ENGINE_NODE(flow_output, "flow_output"); - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(addr_sets, "addr_sets"); - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_groups, "port_groups"); + ENGINE_NODE_WITH_INIT_RUN(addr_sets, "addr_sets"); + ENGINE_NODE_WITH_INIT_RUN(port_groups, "port_groups"); ENGINE_NODE(northd_options, "northd_options"); #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR); diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c index 2e2b31033..30a20bb35 100644 --- a/lib/inc-proc-eng.c +++ b/lib/inc-proc-eng.c @@ -196,10 +196,12 @@ void engine_cleanup(void) { for (size_t i = 0; i < engine_n_nodes; i++) { - if (engine_nodes[i]->clear_tracked_data) { - engine_nodes[i]->clear_tracked_data(engine_nodes[i]->data); + if (engine_nodes[i]->init_run) { + engine_nodes[i]->init_run(engine_nodes[i], engine_nodes[i]->data); } + } + for (size_t i = 0; i < engine_n_nodes; i++) { if (engine_nodes[i]->cleanup) { engine_nodes[i]->cleanup(engine_nodes[i]->data); } @@ -351,8 +353,8 @@ engine_init_run(void) for (size_t i = 0; i < engine_n_nodes; i++) { engine_set_node_state(engine_nodes[i], EN_STALE); - if (engine_nodes[i]->clear_tracked_data) { - engine_nodes[i]->clear_tracked_data(engine_nodes[i]->data); + if (engine_nodes[i]->init_run) { + engine_nodes[i]->init_run(engine_nodes[i], engine_nodes[i]->data); } } } @@ -377,10 +379,11 @@ engine_recompute(struct engine_node *node, bool allowed, goto done; } - /* Clear tracked data before calling run() so that partially tracked data - * from some of the change handler executions are cleared. */ - if (node->clear_tracked_data) { - node->clear_tracked_data(node->data); + /* Make sure data is properly initialized before calling run(), e.g., + * partially tracked data some of the change handler executions must + * be cleared. */ + if (node->init_run) { + node->init_run(node, node->data); } /* Run the node handler which might change state. */ diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h index dc365dc18..ab5b91b28 100644 --- a/lib/inc-proc-eng.h +++ b/lib/inc-proc-eng.h @@ -150,6 +150,11 @@ struct engine_node { /* Method to clean up data. It may be NULL. */ void (*cleanup)(void *data); + /* Method to initialize state at every engine run. It can be used for + * example to clear up tracked data maintained by the engine node in the + * engine 'data'. It may be NULL. */ + void (*init_run)(struct engine_node *node, void *data); + /* Fully processes all inputs of this node and regenerates the data * of this node. The pointer to the node's data is passed as argument. * 'run' handlers can also call engine_get_context() and the @@ -165,10 +170,6 @@ struct engine_node { */ bool (*is_valid)(struct engine_node *); - /* Method to clear up tracked data maintained by the engine node in the - * engine 'data'. It may be NULL. */ - void (*clear_tracked_data)(void *tracked_data); - /* Engine stats. */ struct engine_stats stats; }; @@ -311,20 +312,20 @@ void engine_ovsdb_node_add_index(struct engine_node *, const char *name, .run = en_##NAME##_run, \ .cleanup = en_##NAME##_cleanup, \ .is_valid = NULL, \ - .clear_tracked_data = NULL, \ + .init_run = NULL, \ }; -#define ENGINE_NODE_WITH_CLEAR_TRACK_DATA_IS_VALID(NAME, NAME_STR) \ +#define ENGINE_NODE_WITH_INIT_RUN_IS_VALID(NAME, NAME_STR) \ ENGINE_NODE(NAME, NAME_STR) \ - en_##NAME.clear_tracked_data = en_##NAME##_clear_tracked_data; \ + en_##NAME.init_run = en_##NAME##_init_run; \ en_##NAME.is_valid = en_##NAME##_is_valid; #define ENGINE_NODE(NAME, NAME_STR) \ ENGINE_NODE_DEF(NAME, NAME_STR) -#define ENGINE_NODE_WITH_CLEAR_TRACK_DATA(NAME, NAME_STR) \ +#define ENGINE_NODE_WITH_INIT_RUN(NAME, NAME_STR) \ ENGINE_NODE(NAME, NAME_STR) \ - en_##NAME.clear_tracked_data = en_##NAME##_clear_tracked_data; + en_##NAME.init_run = en_##NAME##_init_run; /* Macro to define member functions of an engine node which represents * a table of OVSDB */
This is actually more in line with how the callback is used. It's called every the incremental engine preparese for the next engine run. Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- controller/ovn-controller.c | 41 ++++++++++++++++++++--------------------- lib/inc-proc-eng.c | 19 +++++++++++-------- lib/inc-proc-eng.h | 19 ++++++++++--------- 3 files changed, 41 insertions(+), 38 deletions(-)