On Tue, Dec 3, 2019 at 9:28 AM Han Zhou <[email protected]> wrote: > > > > On Wed, Nov 27, 2019 at 5:16 AM Dumitru Ceara <[email protected]> wrote: > > > > The incremental processing engine might stop a run before the > > en_runtime_data node is processed. In such cases the ed_runtime_data > > fields might contain pointers to already deleted SB records. For > > example, if a port binding corresponding to a patch port is removed from > > the SB database and the incremental processing engine aborts before the > > en_runtime_data node is processed then the corresponding local_datapath > > hashtable entry in ed_runtime_data is stale and will store a pointer to > > the already freed sbrec_port_binding record. > > > > This will cause invalid memory accesses in various places (e.g., > > pinctrl_run() -> prepare_ipv6_ras()). > > > > To fix the issue we introduce the concept of "internal_data" vs "data" in > > engine nodes. The first field, "internal_data", is data that can be accessed > > by the incremental engine nodes handlers (data from other nodes must be > > considered read-only and data from other nodes must not be accessed if the > > nodes haven't been refreshed in the current iteration). The second field, > > "data" is a pointer reset at engine_run() and if non-NULL indicates to > > users outside the incremental engine that the data is safe to use. > > > > This commit also adds an "is_valid()" method to engine nodes to allow > > users to override the default behavior of determining if data is valid in a > > node (e.g., for the ct-zones node the data is always safe to access). > > > > CC: Han Zhou <[email protected]> > > Fixes: ca278d98a4f5 ("ovn-controller: Initial use of incremental engine - > > quiet mode.") > > Signed-off-by: Dumitru Ceara <[email protected]> > > --- > > controller/ovn-controller.c | 230 > > ++++++++++++++++++++++--------------------- > > lib/inc-proc-eng.c | 36 ++++++- > > lib/inc-proc-eng.h | 28 +++++ > > 3 files changed, 174 insertions(+), 120 deletions(-) > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index f5a83f9..18cc5fe 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -741,8 +741,7 @@ struct ed_type_ofctrl_is_connected { > > static void > > en_ofctrl_is_connected_init(struct engine_node *node) > > { > > - struct ed_type_ofctrl_is_connected *data = > > - (struct ed_type_ofctrl_is_connected *)node->data; > > + struct ed_type_ofctrl_is_connected *data = node->internal_data; > > data->connected = false; > > } > > > > @@ -754,8 +753,7 @@ en_ofctrl_is_connected_cleanup(struct engine_node *node > > OVS_UNUSED) > > static void > > en_ofctrl_is_connected_run(struct engine_node *node) > > { > > - struct ed_type_ofctrl_is_connected *data = > > - (struct ed_type_ofctrl_is_connected *)node->data; > > + struct ed_type_ofctrl_is_connected *data = node->internal_data; > > if (data->connected != ofctrl_is_connected()) { > > data->connected = !data->connected; > > engine_set_node_state(node, EN_UPDATED); > > @@ -775,7 +773,7 @@ struct ed_type_addr_sets { > > static void > > en_addr_sets_init(struct engine_node *node) > > { > > - struct ed_type_addr_sets *as = (struct ed_type_addr_sets *)node->data; > > + struct ed_type_addr_sets *as = node->internal_data; > > shash_init(&as->addr_sets); > > as->change_tracked = false; > > sset_init(&as->new); > > @@ -786,7 +784,7 @@ en_addr_sets_init(struct engine_node *node) > > static void > > en_addr_sets_cleanup(struct engine_node *node) > > { > > - struct ed_type_addr_sets *as = (struct ed_type_addr_sets *)node->data; > > + struct ed_type_addr_sets *as = node->internal_data; > > expr_const_sets_destroy(&as->addr_sets); > > shash_destroy(&as->addr_sets); > > sset_destroy(&as->new); > > @@ -797,7 +795,7 @@ en_addr_sets_cleanup(struct engine_node *node) > > static void > > en_addr_sets_run(struct engine_node *node) > > { > > - struct ed_type_addr_sets *as = (struct ed_type_addr_sets *)node->data; > > + struct ed_type_addr_sets *as = node->internal_data; > > > > sset_clear(&as->new); > > sset_clear(&as->deleted); > > @@ -817,7 +815,7 @@ en_addr_sets_run(struct engine_node *node) > > static bool > > addr_sets_sb_address_set_handler(struct engine_node *node) > > { > > - struct ed_type_addr_sets *as = (struct ed_type_addr_sets *)node->data; > > + struct ed_type_addr_sets *as = node->internal_data; > > > > sset_clear(&as->new); > > sset_clear(&as->deleted); > > @@ -852,7 +850,7 @@ struct ed_type_port_groups{ > > static void > > en_port_groups_init(struct engine_node *node) > > { > > - struct ed_type_port_groups *pg = (struct ed_type_port_groups > > *)node->data; > > + struct ed_type_port_groups *pg = node->internal_data; > > shash_init(&pg->port_groups); > > pg->change_tracked = false; > > sset_init(&pg->new); > > @@ -863,7 +861,7 @@ en_port_groups_init(struct engine_node *node) > > static void > > en_port_groups_cleanup(struct engine_node *node) > > { > > - struct ed_type_port_groups *pg = (struct ed_type_port_groups > > *)node->data; > > + struct ed_type_port_groups *pg = node->internal_data; > > expr_const_sets_destroy(&pg->port_groups); > > shash_destroy(&pg->port_groups); > > sset_destroy(&pg->new); > > @@ -874,7 +872,7 @@ en_port_groups_cleanup(struct engine_node *node) > > static void > > en_port_groups_run(struct engine_node *node) > > { > > - struct ed_type_port_groups *pg = (struct ed_type_port_groups > > *)node->data; > > + struct ed_type_port_groups *pg = node->internal_data; > > > > sset_clear(&pg->new); > > sset_clear(&pg->deleted); > > @@ -894,7 +892,7 @@ en_port_groups_run(struct engine_node *node) > > static bool > > port_groups_sb_port_group_handler(struct engine_node *node) > > { > > - struct ed_type_port_groups *pg = (struct ed_type_port_groups > > *)node->data; > > + struct ed_type_port_groups *pg = node->internal_data; > > > > sset_clear(&pg->new); > > sset_clear(&pg->deleted); > > @@ -938,8 +936,7 @@ struct ed_type_runtime_data { > > static void > > en_runtime_data_init(struct engine_node *node) > > { > > - struct ed_type_runtime_data *data = > > - (struct ed_type_runtime_data *)node->data; > > + struct ed_type_runtime_data *data = node->internal_data; > > > > hmap_init(&data->local_datapaths); > > sset_init(&data->local_lports); > > @@ -950,8 +947,7 @@ en_runtime_data_init(struct engine_node *node) > > static void > > en_runtime_data_cleanup(struct engine_node *node) > > { > > - struct ed_type_runtime_data *data = > > - (struct ed_type_runtime_data *)node->data; > > + struct ed_type_runtime_data *data = node->internal_data; > > > > sset_destroy(&data->local_lports); > > sset_destroy(&data->local_lport_ids); > > @@ -970,8 +966,7 @@ en_runtime_data_cleanup(struct engine_node *node) > > static void > > en_runtime_data_run(struct engine_node *node) > > { > > - struct ed_type_runtime_data *data = > > - (struct ed_type_runtime_data *)node->data; > > + struct ed_type_runtime_data *data = node->internal_data; > > struct hmap *local_datapaths = &data->local_datapaths; > > struct sset *local_lports = &data->local_lports; > > struct sset *local_lport_ids = &data->local_lport_ids; > > @@ -1019,8 +1014,7 @@ en_runtime_data_run(struct engine_node *node) > > ovs_assert(chassis); > > > > struct ed_type_ofctrl_is_connected *ed_ofctrl_is_connected = > > - (struct ed_type_ofctrl_is_connected *)engine_get_input( > > - "ofctrl_is_connected", node)->data; > > + engine_get_input("ofctrl_is_connected", node)->internal_data; > > if (ed_ofctrl_is_connected->connected) { > > /* Calculate the active tunnels only if have an an active > > * OpenFlow connection to br-int. > > @@ -1076,8 +1070,7 @@ en_runtime_data_run(struct engine_node *node) > > static bool > > runtime_data_sb_port_binding_handler(struct engine_node *node) > > { > > - struct ed_type_runtime_data *data = > > - (struct ed_type_runtime_data *)node->data; > > + struct ed_type_runtime_data *data = node->internal_data; > > struct sset *local_lports = &data->local_lports; > > struct sset *active_tunnels = &data->active_tunnels; > > > > @@ -1121,7 +1114,7 @@ struct ed_type_ct_zones { > > static void > > en_ct_zones_init(struct engine_node *node) > > { > > - struct ed_type_ct_zones *data = node->data; > > + struct ed_type_ct_zones *data = node->internal_data; > > struct ovsrec_open_vswitch_table *ovs_table = > > (struct ovsrec_open_vswitch_table *)EN_OVSDB_GET( > > engine_get_input("OVS_open_vswitch", node)); > > @@ -1140,7 +1133,7 @@ en_ct_zones_init(struct engine_node *node) > > static void > > en_ct_zones_cleanup(struct engine_node *node) > > { > > - struct ed_type_ct_zones *data = node->data; > > + struct ed_type_ct_zones *data = node->internal_data; > > > > simap_destroy(&data->current); > > shash_destroy(&data->pending); > > @@ -1149,10 +1142,9 @@ en_ct_zones_cleanup(struct engine_node *node) > > static void > > en_ct_zones_run(struct engine_node *node) > > { > > - struct ed_type_ct_zones *data = node->data; > > + struct ed_type_ct_zones *data = node->internal_data; > > struct ed_type_runtime_data *rt_data = > > - (struct ed_type_runtime_data *)engine_get_input( > > - "runtime_data", node)->data; > > + engine_get_input("runtime_data", node)->internal_data; > > > > update_ct_zones(&rt_data->local_lports, &rt_data->local_datapaths, > > &data->current, data->bitmap, &data->pending); > > @@ -1160,6 +1152,13 @@ en_ct_zones_run(struct engine_node *node) > > engine_set_node_state(node, EN_UPDATED); > > } > > > > +/* The data in the ct_zones node is always valid (i.e., no stale > > pointers). */ > > +static bool > > +en_ct_zones_is_valid(struct engine_node *node OVS_UNUSED) > > +{ > > + return true; > > +} > > + > > struct ed_type_mff_ovn_geneve { > > enum mf_field_id mff_ovn_geneve; > > }; > > @@ -1167,8 +1166,7 @@ struct ed_type_mff_ovn_geneve { > > static void > > en_mff_ovn_geneve_init(struct engine_node *node) > > { > > - struct ed_type_mff_ovn_geneve *data = > > - (struct ed_type_mff_ovn_geneve *)node->data; > > + struct ed_type_mff_ovn_geneve *data = node->internal_data; > > data->mff_ovn_geneve = 0; > > } > > > > @@ -1180,8 +1178,7 @@ en_mff_ovn_geneve_cleanup(struct engine_node *node > > OVS_UNUSED) > > static void > > en_mff_ovn_geneve_run(struct engine_node *node) > > { > > - struct ed_type_mff_ovn_geneve *data = > > - (struct ed_type_mff_ovn_geneve *)node->data; > > + struct ed_type_mff_ovn_geneve *data = node->internal_data; > > enum mf_field_id mff_ovn_geneve = ofctrl_get_mf_field_id(); > > if (data->mff_ovn_geneve != mff_ovn_geneve) { > > data->mff_ovn_geneve = mff_ovn_geneve; > > @@ -1207,8 +1204,7 @@ struct ed_type_flow_output { > > static void > > en_flow_output_init(struct engine_node *node) > > { > > - struct ed_type_flow_output *data = > > - (struct ed_type_flow_output *)node->data; > > + struct ed_type_flow_output *data = node->internal_data; > > ovn_desired_flow_table_init(&data->flow_table); > > ovn_extend_table_init(&data->group_table); > > ovn_extend_table_init(&data->meter_table); > > @@ -1219,8 +1215,7 @@ en_flow_output_init(struct engine_node *node) > > static void > > en_flow_output_cleanup(struct engine_node *node) > > { > > - struct ed_type_flow_output *data = > > - (struct ed_type_flow_output *)node->data; > > + struct ed_type_flow_output *data = node->internal_data; > > ovn_desired_flow_table_destroy(&data->flow_table); > > ovn_extend_table_destroy(&data->group_table); > > ovn_extend_table_destroy(&data->meter_table); > > @@ -1231,21 +1226,18 @@ static void > > en_flow_output_run(struct engine_node *node) > > { > > struct ed_type_runtime_data *rt_data = > > - (struct ed_type_runtime_data *)engine_get_input( > > - "runtime_data", node)->data; > > + engine_get_input("runtime_data", node)->internal_data; > > struct hmap *local_datapaths = &rt_data->local_datapaths; > > struct sset *local_lports = &rt_data->local_lports; > > struct sset *local_lport_ids = &rt_data->local_lport_ids; > > struct sset *active_tunnels = &rt_data->active_tunnels; > > > > struct ed_type_ct_zones *ct_zones_data = > > - (struct ed_type_ct_zones *)engine_get_input( > > - "ct_zones", node)->data; > > + engine_get_input("ct_zones", node)->internal_data; > > struct simap *ct_zones = &ct_zones_data->current; > > > > struct ed_type_mff_ovn_geneve *ed_mff_ovn_geneve = > > - (struct ed_type_mff_ovn_geneve *)engine_get_input( > > - "mff_ovn_geneve", node)->data; > > + engine_get_input("mff_ovn_geneve", node)->internal_data; > > enum mf_field_id mff_ovn_geneve = ed_mff_ovn_geneve->mff_ovn_geneve; > > > > struct ovsrec_open_vswitch_table *ovs_table = > > @@ -1262,12 +1254,11 @@ en_flow_output_run(struct engine_node *node) > > engine_get_input("SB_chassis", node), > > "name"); > > struct ed_type_addr_sets *as_data = > > - (struct ed_type_addr_sets *)engine_get_input("addr_sets", > > node)->data; > > + engine_get_input("addr_sets", node)->internal_data; > > struct shash *addr_sets = &as_data->addr_sets; > > > > struct ed_type_port_groups *pg_data = > > - (struct ed_type_port_groups *)engine_get_input( > > - "port_groups", node)->data; > > + engine_get_input("port_groups", node)->internal_data; > > struct shash *port_groups = &pg_data->port_groups; > > > > const struct sbrec_chassis *chassis = NULL; > > @@ -1277,8 +1268,7 @@ en_flow_output_run(struct engine_node *node) > > > > ovs_assert(br_int && chassis); > > > > - struct ed_type_flow_output *fo = > > - (struct ed_type_flow_output *)node->data; > > + struct ed_type_flow_output *fo = node->internal_data; > > struct ovn_desired_flow_table *flow_table = &fo->flow_table; > > struct ovn_extend_table *group_table = &fo->group_table; > > struct ovn_extend_table *meter_table = &fo->meter_table; > > @@ -1361,18 +1351,16 @@ static bool > > flow_output_sb_logical_flow_handler(struct engine_node *node) > > { > > struct ed_type_runtime_data *data = > > - (struct ed_type_runtime_data *)engine_get_input( > > - "runtime_data", node)->data; > > + engine_get_input("runtime_data", node)->internal_data; > > struct hmap *local_datapaths = &data->local_datapaths; > > struct sset *local_lport_ids = &data->local_lport_ids; > > struct sset *active_tunnels = &data->active_tunnels; > > struct ed_type_addr_sets *as_data = > > - (struct ed_type_addr_sets *)engine_get_input("addr_sets", > > node)->data; > > + engine_get_input("addr_sets", node)->internal_data; > > struct shash *addr_sets = &as_data->addr_sets; > > > > struct ed_type_port_groups *pg_data = > > - (struct ed_type_port_groups *)engine_get_input( > > - "port_groups", node)->data; > > + engine_get_input("port_groups", node)->internal_data; > > struct shash *port_groups = &pg_data->port_groups; > > > > struct ovsrec_open_vswitch_table *ovs_table = > > @@ -1396,8 +1384,7 @@ flow_output_sb_logical_flow_handler(struct > > engine_node *node) > > > > ovs_assert(br_int && chassis); > > > > - struct ed_type_flow_output *fo = > > - (struct ed_type_flow_output *)node->data; > > + struct ed_type_flow_output *fo = node->internal_data; > > struct ovn_desired_flow_table *flow_table = &fo->flow_table; > > struct ovn_extend_table *group_table = &fo->group_table; > > struct ovn_extend_table *meter_table = &fo->meter_table; > > @@ -1452,8 +1439,7 @@ flow_output_sb_mac_binding_handler(struct engine_node > > *node) > > (struct sbrec_mac_binding_table *)EN_OVSDB_GET( > > engine_get_input("SB_mac_binding", node)); > > > > - struct ed_type_flow_output *fo = > > - (struct ed_type_flow_output *)node->data; > > + struct ed_type_flow_output *fo = node->internal_data; > > struct ovn_desired_flow_table *flow_table = &fo->flow_table; > > > > lflow_handle_changed_neighbors(sbrec_port_binding_by_name, > > @@ -1467,19 +1453,16 @@ static bool > > flow_output_sb_port_binding_handler(struct engine_node *node) > > { > > struct ed_type_runtime_data *data = > > - (struct ed_type_runtime_data *)engine_get_input( > > - "runtime_data", node)->data; > > + engine_get_input("runtime_data", node)->internal_data; > > struct hmap *local_datapaths = &data->local_datapaths; > > struct sset *active_tunnels = &data->active_tunnels; > > > > struct ed_type_ct_zones *ct_zones_data = > > - (struct ed_type_ct_zones *)engine_get_input( > > - "ct_zones", node)->data; > > + engine_get_input("ct_zones", node)->internal_data; > > struct simap *ct_zones = &ct_zones_data->current; > > > > struct ed_type_mff_ovn_geneve *ed_mff_ovn_geneve = > > - (struct ed_type_mff_ovn_geneve *)engine_get_input( > > - "mff_ovn_geneve", node)->data; > > + engine_get_input("mff_ovn_geneve", node)->internal_data; > > enum mf_field_id mff_ovn_geneve = ed_mff_ovn_geneve->mff_ovn_geneve; > > > > struct ovsrec_open_vswitch_table *ovs_table = > > @@ -1501,8 +1484,7 @@ flow_output_sb_port_binding_handler(struct > > engine_node *node) > > } > > ovs_assert(br_int && chassis); > > > > - struct ed_type_flow_output *fo = > > - (struct ed_type_flow_output *)node->data; > > + struct ed_type_flow_output *fo = node->internal_data; > > struct ovn_desired_flow_table *flow_table = &fo->flow_table; > > > > struct ovsdb_idl_index *sbrec_port_binding_by_name = > > @@ -1575,18 +1557,15 @@ static bool > > flow_output_sb_multicast_group_handler(struct engine_node *node) > > { > > struct ed_type_runtime_data *data = > > - (struct ed_type_runtime_data *)engine_get_input( > > - "runtime_data", node)->data; > > + engine_get_input("runtime_data", node)->internal_data; > > struct hmap *local_datapaths = &data->local_datapaths; > > > > struct ed_type_ct_zones *ct_zones_data = > > - (struct ed_type_ct_zones *)engine_get_input( > > - "ct_zones", node)->data; > > + engine_get_input("ct_zones", node)->internal_data; > > struct simap *ct_zones = &ct_zones_data->current; > > > > struct ed_type_mff_ovn_geneve *ed_mff_ovn_geneve = > > - (struct ed_type_mff_ovn_geneve *)engine_get_input( > > - "mff_ovn_geneve", node)->data; > > + engine_get_input("mff_ovn_geneve", node)->internal_data; > > enum mf_field_id mff_ovn_geneve = ed_mff_ovn_geneve->mff_ovn_geneve; > > > > struct ovsrec_open_vswitch_table *ovs_table = > > @@ -1608,8 +1587,7 @@ flow_output_sb_multicast_group_handler(struct > > engine_node *node) > > } > > ovs_assert(br_int && chassis); > > > > - struct ed_type_flow_output *fo = > > - (struct ed_type_flow_output *)node->data; > > + struct ed_type_flow_output *fo = node->internal_data; > > struct ovn_desired_flow_table *flow_table = &fo->flow_table; > > > > struct sbrec_multicast_group_table *multicast_group_table = > > @@ -1630,19 +1608,17 @@ _flow_output_resource_ref_handler(struct > > engine_node *node, > > enum ref_type ref_type) > > { > > struct ed_type_runtime_data *data = > > - (struct ed_type_runtime_data *)engine_get_input( > > - "runtime_data", node)->data; > > + engine_get_input("runtime_data", node)->internal_data; > > struct hmap *local_datapaths = &data->local_datapaths; > > struct sset *local_lport_ids = &data->local_lport_ids; > > struct sset *active_tunnels = &data->active_tunnels; > > > > struct ed_type_addr_sets *as_data = > > - (struct ed_type_addr_sets *)engine_get_input("addr_sets", > > node)->data; > > + engine_get_input("addr_sets", node)->internal_data; > > struct shash *addr_sets = &as_data->addr_sets; > > > > struct ed_type_port_groups *pg_data = > > - (struct ed_type_port_groups *)engine_get_input( > > - "port_groups", node)->data; > > + engine_get_input("port_groups", node)->internal_data; > > struct shash *port_groups = &pg_data->port_groups; > > > > struct ovsrec_open_vswitch_table *ovs_table = > > @@ -1666,7 +1642,7 @@ _flow_output_resource_ref_handler(struct engine_node > > *node, > > ovs_assert(br_int && chassis); > > > > struct ed_type_flow_output *fo = > > - (struct ed_type_flow_output *)node->data; > > + node->internal_data; > > struct ovn_desired_flow_table *flow_table = &fo->flow_table; > > struct ovn_extend_table *group_table = &fo->group_table; > > struct ovn_extend_table *meter_table = &fo->meter_table; > > @@ -1899,7 +1875,7 @@ main(int argc, char *argv[]) > > struct ed_type_addr_sets ed_addr_sets; > > struct ed_type_port_groups ed_port_groups; > > > > - ENGINE_NODE(ct_zones, "ct_zones"); > > + ENGINE_NODE_CUSTOM_DATA(ct_zones, "ct_zones"); > > ENGINE_NODE(runtime_data, "runtime_data"); > > ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve"); > > ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected"); > > @@ -2061,7 +2037,10 @@ main(int argc, char *argv[]) > > } > > > > if (br_int) { > > - ofctrl_run(br_int, &ed_ct_zones.pending); > > + if (en_ct_zones.data) { > > + struct ed_type_ct_zones *ct_zones = en_ct_zones.data; > > + ofctrl_run(br_int, &ct_zones->pending); > > + } > > > > if (chassis) { > > patch_run(ovs_idl_txn, > > @@ -2102,40 +2081,53 @@ main(int argc, char *argv[]) > > stopwatch_stop(CONTROLLER_LOOP_STOPWATCH_NAME, > > time_msec()); > > if (ovs_idl_txn) { > > - commit_ct_zones(br_int, &ed_ct_zones.pending); > > + if (en_ct_zones.data) { > > + struct ed_type_ct_zones *ct_zones = > > + en_ct_zones.data; > > + commit_ct_zones(br_int, &ct_zones->pending); > > + } > > > > bfd_run(ovsrec_interface_table_get(ovs_idl_loop.idl), > > br_int, chassis, > > sbrec_ha_chassis_group_table_get( > > ovnsb_idl_loop.idl), > > > > sbrec_sb_global_table_get(ovnsb_idl_loop.idl)); > > } > > - ofctrl_put(&ed_flow_output.flow_table, > > - &ed_ct_zones.pending, > > - sbrec_meter_table_get(ovnsb_idl_loop.idl), > > - get_nb_cfg(sbrec_sb_global_table_get( > > - ovnsb_idl_loop.idl)), > > - engine_node_changed(&en_flow_output)); > > - pinctrl_run(ovnsb_idl_txn, > > - sbrec_datapath_binding_by_key, > > - sbrec_port_binding_by_datapath, > > - sbrec_port_binding_by_key, > > - sbrec_port_binding_by_name, > > - sbrec_mac_binding_by_lport_ip, > > - sbrec_igmp_group, > > - sbrec_ip_multicast, > > - sbrec_dns_table_get(ovnsb_idl_loop.idl), > > - sbrec_controller_event_table_get( > > - ovnsb_idl_loop.idl), > > - sbrec_service_monitor_table_get( > > - ovnsb_idl_loop.idl), > > - br_int, chassis, > > - &ed_runtime_data.local_datapaths, > > - &ed_runtime_data.active_tunnels); > > - > > - if (engine_node_changed(&en_runtime_data)) { > > - update_sb_monitors(ovnsb_idl_loop.idl, chassis, > > - &ed_runtime_data.local_lports, > > - > > &ed_runtime_data.local_datapaths); > > + if (en_flow_output.data && en_ct_zones.data) { > > + struct ed_type_ct_zones *ct_zones = > > + en_ct_zones.data; > > + struct ed_type_flow_output *flow_output = > > + en_flow_output.data; > > + ofctrl_put(&flow_output->flow_table, > > + &ct_zones->pending, > > + > > sbrec_meter_table_get(ovnsb_idl_loop.idl), > > + get_nb_cfg(sbrec_sb_global_table_get( > > + ovnsb_idl_loop.idl)), > > + engine_node_changed(&en_flow_output)); > > + } > > + if (en_runtime_data.data) { > > + struct ed_type_runtime_data *rt_data = > > + en_runtime_data.data; > > + pinctrl_run(ovnsb_idl_txn, > > + sbrec_datapath_binding_by_key, > > + sbrec_port_binding_by_datapath, > > + sbrec_port_binding_by_key, > > + sbrec_port_binding_by_name, > > + sbrec_mac_binding_by_lport_ip, > > + sbrec_igmp_group, > > + sbrec_ip_multicast, > > + > > sbrec_dns_table_get(ovnsb_idl_loop.idl), > > + sbrec_controller_event_table_get( > > + ovnsb_idl_loop.idl), > > + sbrec_service_monitor_table_get( > > + ovnsb_idl_loop.idl), > > + br_int, chassis, > > + &rt_data->local_datapaths, > > + &rt_data->active_tunnels); > > + if (engine_node_changed(&en_runtime_data)) { > > + update_sb_monitors(ovnsb_idl_loop.idl, chassis, > > + &rt_data->local_lports, > > + &rt_data->local_datapaths); > > + } > > } > > } > > > > @@ -2170,9 +2162,14 @@ main(int argc, char *argv[]) > > > > > > if (pending_pkt.conn) { > > - if (br_int && chassis) { > > + if (br_int && chassis && en_addr_sets.data && > > + en_port_groups.data) { > > + struct ed_type_addr_sets *as_data = > > + en_addr_sets.data; > > + struct ed_type_port_groups *pg_data = > > + en_port_groups.data; > > char *error = ofctrl_inject_pkt(br_int, > > pending_pkt.flow_s, > > - &ed_addr_sets.addr_sets, > > &ed_port_groups.port_groups); > > + &as_data->addr_sets, &pg_data->port_groups); > > if (error) { > > unixctl_command_reply_error(pending_pkt.conn, > > error); > > free(error); > > @@ -2210,12 +2207,15 @@ main(int argc, char *argv[]) > > } > > > > if (ovsdb_idl_loop_commit_and_wait(&ovs_idl_loop) == 1) { > > - struct shash_node *iter, *iter_next; > > - SHASH_FOR_EACH_SAFE (iter, iter_next, &ed_ct_zones.pending) { > > - struct ct_zone_pending_entry *ctzpe = iter->data; > > - if (ctzpe->state == CT_ZONE_DB_SENT) { > > - shash_delete(&ed_ct_zones.pending, iter); > > - free(ctzpe); > > + if (en_ct_zones.data) { > > + struct ed_type_ct_zones *ct_zones = en_ct_zones.data; > > + struct shash_node *iter, *iter_next; > > + SHASH_FOR_EACH_SAFE (iter, iter_next, &ct_zones->pending) { > > + struct ct_zone_pending_entry *ctzpe = iter->data; > > + if (ctzpe->state == CT_ZONE_DB_SENT) { > > + shash_delete(&ct_zones->pending, iter); > > + free(ctzpe); > > + } > > } > > } > > } > > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c > > index 59b5cac..569c93d 100644 > > --- a/lib/inc-proc-eng.c > > +++ b/lib/inc-proc-eng.c > > @@ -153,7 +153,7 @@ engine_add_input(struct engine_node *node, struct > > engine_node *input, > > struct ovsdb_idl_index * > > engine_ovsdb_node_get_index(struct engine_node *node, const char *name) > > { > > - struct ed_type_ovsdb_table *ed = (struct ed_type_ovsdb_table > > *)node->data; > > + struct ed_type_ovsdb_table *ed = node->internal_data; > > for (size_t i = 0; i < ed->n_indexes; i++) { > > if (!strcmp(ed->indexes[i].name, name)) { > > return ed->indexes[i].index; > > @@ -167,7 +167,7 @@ void > > engine_ovsdb_node_add_index(struct engine_node *node, const char *name, > > struct ovsdb_idl_index *index) > > { > > - struct ed_type_ovsdb_table *ed = (struct ed_type_ovsdb_table > > *)node->data; > > + struct ed_type_ovsdb_table *ed = node->internal_data; > > ovs_assert(ed->n_indexes < ENGINE_MAX_OVSDB_INDEX); > > > > ed->indexes[ed->n_indexes].name = name; > > @@ -192,6 +192,19 @@ engine_set_node_state_at(struct engine_node *node, > > node->state = state; > > } > > > > +static bool > > +engine_node_valid(struct engine_node *node) > > +{ > > + if (node->state == EN_UPDATED || node->state == EN_VALID) { > > + return true; > > + } > > + > > + if (node->is_valid) { > > + return node->is_valid(node); > > + } > > + return false; > > +} > > + > > bool > > engine_node_changed(struct engine_node *node) > > { > > @@ -215,12 +228,26 @@ engine_aborted(void) > > return engine_run_aborted; > > } > > > > +static void * > > +engine_get_data(struct engine_node *node) > > +{ > > + if (engine_node_valid(node)) { > > + return node->internal_data; > > + } > > + return NULL; > > +} > > + > > void > > engine_init_run(void) > > { > > VLOG_DBG("Initializing new run"); > > for (size_t i = 0; i < engine_n_nodes; i++) { > > engine_set_node_state(engine_nodes[i], EN_STALE); > > + > > + /* Make sure we reset the data pointer for outside users. > > + * For nodes that always store valid data the value will be > > non-NULL. > > + */ > > + engine_nodes[i]->data = engine_get_data(engine_nodes[i]); > > } > > } > > > > @@ -333,6 +360,11 @@ engine_run(bool recompute_allowed) > > engine_run_aborted = true; > > return; > > } > > + > > + /* Make sure we reset the data pointer for outside users as the > > + * node's state might have changed. > > + */ > > + engine_nodes[i]->data = engine_get_data(engine_nodes[i]); > > } > > } > > > > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h > > index bb8df07..e0d26ea 100644 > > --- a/lib/inc-proc-eng.h > > +++ b/lib/inc-proc-eng.h > > @@ -111,6 +111,12 @@ struct engine_node { > > * and run() function of the current node. Users should ensure that the > > * data is read-only in change-handlers of the nodes that depends on > > this > > * node. */ > > + void *internal_data; > > + > > + /* A pointer to node data accessible for users outside the processing > > + * engine. The value of the pointer is updated by the engine itself and > > + * users should ensure that the data is only read. > > + */ > > void *data; > > > > /* State of the node after the last engine run. */ > > @@ -125,6 +131,13 @@ struct engine_node { > > /* Fully processes all inputs of this node and regenerates the data > > * of this node */ > > void (*run)(struct engine_node *); > > + > > + /* Method to validate if the 'internal_data' is valid. This allows > > users > > + * to customize when 'internal_data' can be used (e.g., even if the > > node > > + * hasn't been refreshed in the last iteration, if 'internal_data' > > + * doesn't store pointers to DB records it's still safe to use). > > + */ > > + bool (*is_valid)(struct engine_node *); > > }; > > > > /* Initialize the data for the engine nodes. It calls each node's > > @@ -201,7 +214,7 @@ struct ed_type_ovsdb_table { > > }; > > > > #define EN_OVSDB_GET(NODE) \ > > - (((struct ed_type_ovsdb_table *)NODE->data)->table) > > + (((struct ed_type_ovsdb_table *)NODE->internal_data)->table) > > > > struct ovsdb_idl_index * engine_ovsdb_node_get_index(struct engine_node *, > > const char *name); > > @@ -210,16 +223,25 @@ void engine_ovsdb_node_add_index(struct engine_node > > *, const char *name, > > struct ovsdb_idl_index *); > > > > /* Macro to define an engine node. */ > > -#define ENGINE_NODE(NAME, NAME_STR) \ > > +#define ENGINE_NODE_DEF(NAME, NAME_STR) \ > > struct engine_node en_##NAME = { \ > > .name = NAME_STR, \ > > - .data = &ed_##NAME, \ > > + .internal_data = &ed_##NAME, \ > > + .data = NULL, \ > > .state = EN_STALE, \ > > .init = en_##NAME##_init, \ > > .run = en_##NAME##_run, \ > > .cleanup = en_##NAME##_cleanup, \ > > + .is_valid = en_##NAME##_is_valid, \ > > }; > > > > +#define ENGINE_NODE_CUSTOM_DATA(NAME, NAME_STR) \ > > + ENGINE_NODE_DEF(NAME, NAME_STR) > > + > > +#define ENGINE_NODE(NAME, NAME_STR) \ > > + static bool (*en_##NAME##_is_valid)(struct engine_node *node) = NULL; \ > > + ENGINE_NODE_DEF(NAME, NAME_STR) > > + > > /* Macro to define member functions of an engine node which represents > > * a table of OVSDB */ > > #define ENGINE_FUNC_OVSDB(DB_NAME, TBL_NAME) \ > > > > Thanks Dumitru. The approach looks valid. However, I think it could be > improved a little, to avoid introducing the internal_data and make it easier > to ensure there is no direct access to the node's data without validation. > I think we can expose the engine_get_data() interface and force it to be used > before accessing the data. The rules can be simply: always use this > engine_get_data() to get engine node's data before accessing, except for > accessing its own data in an engine node's handler (because it needs to be > computed to become valid/updated). > We can remove the ed_xxx variables such as: "struct ed_type_flow_output > ed_flow_output;" so that we can avoid accessing data directly such as > "&ed_flow_output.flow_table". Otherwise, it is hard to ensure the correctness > if someone just update the code to bypass the if (...data) check. Code review > would help, but it is still hard to guarantee. > To remove the ed_xxx variable, we just need to move the data memory > allocation in each node's init() function (and free it in cleanup()). I think > I proposed this in an earlier version but I totally understand it is easy to > get lost because of too much information exchanged. > I really appreciate your effort for the fixes and improvements. I think I > have no problem of merging this last patch with current version, but it could > be even better. Please let me know if you are ok with the above suggestion, > or I can merge it and let's address it with a separate patch.
Thanks Han for the reviews and for applying the previous patches in the series. You're right, it's cleaner to remove direct access to internal node data. I thought the resulting code would be less verbose if we add the "internal_data" field but it does add the need for extra care when reviewing new code. I'll rework the last patch and send a v8. Thanks, Dumitru > > Thanks, > Han > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
