On 9/23/22 01:07, Han Zhou wrote:
> On Wed, Sep 14, 2022 at 6:10 AM Dumitru Ceara <dce...@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 <dce...@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
>> 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
