On Wed, Nov 27, 2019 at 5:16 AM Dumitru Ceara <dce...@redhat.com> 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 <hzh...@ebay.com>
> Fixes: ca278d98a4f5 ("ovn-controller: Initial use of incremental engine -
quiet mode.")
> Signed-off-by: Dumitru Ceara <dce...@redhat.com>
> ---
> 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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
