On Fri, Sep 23, 2022 at 1:42 AM Dumitru Ceara <[email protected]> wrote:
>
> On 9/23/22 01:07, Han Zhou wrote:
> > On Wed, Sep 14, 2022 at 6:10 AM Dumitru Ceara <[email protected]> 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 <[email protected]>
> >
> > 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
> >> [email protected]
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
