Message ID | a77b4f24c70588028b5b91aae105f451572293b3.1613744681.git.lorenzo.bianconi@redhat.com |
---|---|
State | Changes Requested |
Headers | show |
Series | [ovs-dev,v3] controller: introduce coverage counters for ovn-controller incremental processing | expand |
On 2/19/21 3:26 PM, Lorenzo Bianconi wrote: > Introduce inc-engine/stats ovs-applctl command in order to dump > ovn-controller incremental processing engine statistics. So far for each > node a counter for run, abort and engine_handler have been added. > Counters are incremented when the node move to "updated" state. > In order to dump I-P stats we can can use the following commands: > > $ovs-appctl -t ovn-controller inc-engine/stats > SB_address_set > run 1 abort 0 handler 0 > addr_sets > run 2 abort 1 handler 0 > SB_port_group > run 0 abort 0 handler 0 > port_groups > run 2 abort 0 handler 0 > ofctrl_is_connected > run 1 abort 0 handler 0 > OVS_open_vswitch > run 0 abort 0 handler 0 > OVS_bridge > run 0 abort 0 handler 0 > OVS_qos > run 0 abort 0 handler 0 > SB_chassis > run 1 abort 0 handler 0 > .... > > flow_output > run 2 abort 0 handler 34 > > Morover introduce the inc-engine/stats-clear command to reset engine > statistics > > $ovs-appctl -t ovn-controller inc-engine/stats-clear > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- Hi Lorenzo, Thanks for working on this! The commit title needs to be updated as we're not adding coverage counters anymore. > Changes since v2: > - introduce inc-engine/stats and inc-engine/stats-clear commands and drop > COVERAGE_* dependency > > Changes since v1: > - drop handler counters and add global abort counter > - improve documentation and naming scheme > - introduce engine_set_node_updated utility macro > --- > controller/ovn-controller.c | 53 ++++++++++++++++++++++--------------- > lib/inc-proc-eng.c | 48 +++++++++++++++++++++++++++++++++ > lib/inc-proc-eng.h | 26 ++++++++++++++++++ > 3 files changed, 106 insertions(+), 21 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 4343650fc..a937803c8 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -1011,7 +1011,7 @@ en_ofctrl_is_connected_run(struct engine_node *node, void *data) > ofctrl_seqno_flush(); > binding_seqno_flush(); > } > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_run(node); > return; > } > engine_set_node_state(node, EN_UNCHANGED); > @@ -1067,7 +1067,7 @@ en_addr_sets_run(struct engine_node *node, void *data) > addr_sets_init(as_table, &as->addr_sets); > > as->change_tracked = false; > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_run(node); This seems error prone to me, what if a new engine node is added and the wrong macro (or no macro) is used to set the state? It's probably simpler to increment the stats in the inc-proc engine implementation directly: - change handlers are only called in one place, engine_compute(): if a change handler sets the node state to EN_UPDATED we can increment the node's change handler stats. - run handlers are only called in one place, engine_recompute(): if a node handler sets the node state to EN_UPDATED we can increment the node's run handler stats. > } > > static bool > @@ -1088,7 +1088,7 @@ addr_sets_sb_address_set_handler(struct engine_node *node, void *data) > > if (!sset_is_empty(&as->new) || !sset_is_empty(&as->deleted) || > !sset_is_empty(&as->updated)) { > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > } else { > engine_set_node_state(node, EN_UNCHANGED); > } > @@ -1147,7 +1147,7 @@ en_port_groups_run(struct engine_node *node, void *data) > port_groups_init(pg_table, &pg->port_groups); > > pg->change_tracked = false; > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_run(node); > } > > static bool > @@ -1168,7 +1168,7 @@ port_groups_sb_port_group_handler(struct engine_node *node, void *data) > > if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) || > !sset_is_empty(&pg->updated)) { > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > } else { > engine_set_node_state(node, EN_UNCHANGED); > } > @@ -1482,7 +1482,7 @@ en_runtime_data_run(struct engine_node *node, void *data) > > binding_run(&b_ctx_in, &b_ctx_out); > > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_run(node); > } > > static bool > @@ -1500,7 +1500,7 @@ runtime_data_ovs_interface_handler(struct engine_node *node, void *data) > } > > if (b_ctx_out.local_lports_changed) { > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > rt_data->local_lports_changed = b_ctx_out.local_lports_changed; > } > > @@ -1528,7 +1528,7 @@ runtime_data_sb_port_binding_handler(struct engine_node *node, void *data) > if (b_ctx_out.local_lport_ids_changed || > b_ctx_out.non_vif_ports_changed || > !hmap_is_empty(b_ctx_out.tracked_dp_bindings)) { > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > } > > return true; > @@ -1605,7 +1605,7 @@ en_ct_zones_run(struct engine_node *node, void *data) > &ct_zones_data->pending, &rt_data->ct_updated_datapaths); > > > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_run(node); > } > > /* The data in the ct_zones node is always valid (i.e., no stale pointers). */ > @@ -1639,7 +1639,7 @@ en_mff_ovn_geneve_run(struct engine_node *node, void *data) > enum mf_field_id mff_ovn_geneve = ofctrl_get_mf_field_id(); > if (ed_mff_ovn_geneve->mff_ovn_geneve != mff_ovn_geneve) { > ed_mff_ovn_geneve->mff_ovn_geneve = mff_ovn_geneve; > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_run(node); > return; > } > engine_set_node_state(node, EN_UNCHANGED); > @@ -1714,7 +1714,7 @@ en_physical_flow_changes_run(struct engine_node *node, void *data) > { > struct ed_type_pfc_data *pfc_tdata = data; > pfc_tdata->recompute_physical_flows = true; > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_run(node); > } > > /* ct_zone changes are not handled incrementally but a handler is required > @@ -1734,7 +1734,7 @@ physical_flow_changes_ovs_iface_handler(struct engine_node *node, void *data) > { > struct ed_type_pfc_data *pfc_tdata = data; > pfc_tdata->ovs_ifaces_changed = true; > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > return true; > } > > @@ -2035,7 +2035,7 @@ en_flow_output_run(struct engine_node *node, void *data) > > physical_run(&p_ctx, &fo->flow_table); > > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_run(node); > } > > static bool > @@ -2059,7 +2059,7 @@ flow_output_sb_logical_flow_handler(struct engine_node *node, void *data) > > bool handled = lflow_handle_changed_flows(&l_ctx_in, &l_ctx_out); > > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > return handled; > } > > @@ -2085,7 +2085,7 @@ flow_output_sb_mac_binding_handler(struct engine_node *node, void *data) > lflow_handle_changed_neighbors(sbrec_port_binding_by_name, > mac_binding_table, local_datapaths, flow_table); > > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > return true; > } > > @@ -2108,7 +2108,7 @@ flow_output_sb_port_binding_handler(struct engine_node *node, > */ > physical_handle_port_binding_changes(&p_ctx, flow_table); > > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > return true; > } > > @@ -2126,7 +2126,7 @@ flow_output_sb_multicast_group_handler(struct engine_node *node, void *data) > > physical_handle_mc_group_changes(&p_ctx, flow_table); > > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > return true; > > } > @@ -2135,6 +2135,7 @@ static bool > _flow_output_resource_ref_handler(struct engine_node *node, void *data, > enum ref_type ref_type) > { > + bool update_counter = false; We don't need this if we move the stats handling inside the inc-proc engine. > struct ed_type_runtime_data *rt_data = > engine_get_input_data("runtime_data", node); > > @@ -2208,6 +2209,7 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, > } > if (changed) { > engine_set_node_state(node, EN_UPDATED); > + update_counter = true; > } > } > SSET_FOR_EACH (ref_name, updated) { > @@ -2217,6 +2219,7 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, > } > if (changed) { > engine_set_node_state(node, EN_UPDATED); > + update_counter = true; > } > } > SSET_FOR_EACH (ref_name, new) { > @@ -2226,8 +2229,12 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, > } > if (changed) { > engine_set_node_state(node, EN_UPDATED); > + update_counter = true; > } > } > + if (update_counter) { > + node->stats.handler++; > + } > > return true; > } > @@ -2254,7 +2261,7 @@ flow_output_physical_flow_changes_handler(struct engine_node *node, void *data) > struct physical_ctx p_ctx; > init_physical_ctx(node, rt_data, &p_ctx); > > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > struct ed_type_pfc_data *pfc_data = > engine_get_input_data("physical_flow_changes", node); > > @@ -2293,7 +2300,7 @@ flow_output_runtime_data_handler(struct engine_node *node, > struct hmap *tracked_dp_bindings = &rt_data->tracked_dp_bindings; > if (hmap_is_empty(tracked_dp_bindings)) { > if (rt_data->local_lports_changed) { > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > } > return true; > } > @@ -2325,7 +2332,7 @@ flow_output_runtime_data_handler(struct engine_node *node, > } > } > > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > return true; > } > > @@ -2342,7 +2349,7 @@ flow_output_sb_load_balancer_handler(struct engine_node *node, void *data) > > bool handled = lflow_handle_changed_lbs(&l_ctx_in, &l_ctx_out); > > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > return handled; > } > > @@ -2681,6 +2688,10 @@ main(int argc, char *argv[]) > > unixctl_command_register("recompute", "", 0, 0, engine_recompute_cmd, > NULL); > + unixctl_command_register("inc-engine/stats", "", 0, 0, engine_dump_stats, > + NULL); > + unixctl_command_register("inc-engine/stats-clear", "", 0, 0, > + engine_clear_stats, NULL); > unixctl_command_register("lflow-cache/flush", "", 0, 0, > lflow_cache_flush_cmd, > &flow_output_data->pd); > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c > index 916dbbe39..ce3fed562 100644 > --- a/lib/inc-proc-eng.c > +++ b/lib/inc-proc-eng.c > @@ -32,6 +32,7 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_eng); > > static bool engine_force_recompute = false; > static bool engine_run_aborted = false; > +static bool engine_stats_reset = false; > static const struct engine_context *engine_context; > > static struct engine_node **engine_nodes; > @@ -283,6 +284,7 @@ engine_recompute(struct engine_node *node, bool forced, bool allowed) > if (!allowed) { > VLOG_DBG("node: %s, recompute aborted", node->name); > engine_set_node_state(node, EN_ABORTED); > + node->stats.abort++; > return; > } > > @@ -383,9 +385,26 @@ engine_run(bool recompute_allowed) > } > } > > +static void > +engine_reset_stats(void) > +{ > + if (!engine_stats_reset) { > + return; > + } > + > + engine_stats_reset = false; > + for (size_t i = 0; i < engine_n_nodes; i++) { > + struct engine_node *node = engine_nodes[i]; > + > + memset(&node->stats, 0, sizeof(node->stats)); > + } > +} > + > bool > engine_need_run(void) > { > + engine_reset_stats(); > + > for (size_t i = 0; i < engine_n_nodes; i++) { > /* Check only leaf nodes for updates. */ > if (engine_nodes[i]->n_inputs) { > @@ -401,3 +420,32 @@ engine_need_run(void) > } > return false; > } > + > +void > +engine_clear_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, > + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED) > +{ > + engine_stats_reset = true; Why don't we just reset the stats directly here? > + unixctl_command_reply(conn, NULL); > +} > + > +void > +engine_dump_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, > + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED) > +{ > + struct ds dump = DS_EMPTY_INITIALIZER; > + > + engine_reset_stats(); > + > + for (size_t i = 0; i < engine_n_nodes; i++) { > + struct engine_node *node = engine_nodes[i]; > + > + ds_put_format(&dump, "%s\n", node->name); > + ds_put_format(&dump, "\trun\t%lu", node->stats.run); > + ds_put_format(&dump, "\tabort\t%lu", node->stats.abort); > + ds_put_format(&dump, "\thandler\t%lu\n", node->stats.handler); > + } > + unixctl_command_reply(conn, ds_cstr(&dump)); > + > + ds_destroy(&dump); > +} > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h > index 857234677..0eeca7ced 100644 > --- a/lib/inc-proc-eng.h > +++ b/lib/inc-proc-eng.h > @@ -60,6 +60,8 @@ > * against all its inputs. > */ > > +#include "unixctl.h" > + > #define ENGINE_MAX_INPUT 256 > #define ENGINE_MAX_OVSDB_INDEX 256 > > @@ -107,6 +109,12 @@ enum engine_node_state { > EN_STATE_MAX, > }; > > +struct engine_stats { > + unsigned long run; > + unsigned long abort; > + unsigned long handler; Maybe it's more accurate if we use 'change_handler' instead of 'handler'? > +}; > + > struct engine_node { > /* A unique name for each node. */ > char *name; > @@ -154,6 +162,9 @@ 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; > }; > > /* Initialize the data for the engine nodes. It calls each node's > @@ -247,6 +258,14 @@ void *engine_get_internal_data(struct engine_node *node); > #define engine_set_node_state(node, state) \ > engine_set_node_state_at(node, state, OVS_SOURCE_LOCATOR) > > +#define engine_set_note_update_from_run(node) \ s/note/node Also, if we keep these macros: - multi line macros should be wrapped in do { ... } while(0). - macro args should be wrapped in () to avoid unexpected expression evaluations. > + engine_set_node_state(node, EN_UPDATED); \ > + node->stats.run++; > + > +#define engine_set_note_update_from_handler(node) \ > + engine_set_node_state(node, EN_UPDATED); \ > + node->stats.handler++; Same as above. Thanks, Dumitru > + > struct ed_ovsdb_index { > const char *name; > struct ovsdb_idl_index *index; > @@ -312,6 +331,7 @@ en_##DB_NAME##_##TBL_NAME##_run(struct engine_node *node, \ > EN_OVSDB_GET(node); \ > if (DB_NAME##rec_##TBL_NAME##_table_track_get_first(table)) { \ > engine_set_node_state(node, EN_UPDATED); \ > + node->stats.run++; \ > return; \ > } \ > engine_set_node_state(node, EN_UNCHANGED); \ > @@ -352,4 +372,10 @@ static void en_##DB_NAME##_##TBL_NAME##_cleanup(void *data OVS_UNUSED) \ > #define ENGINE_NODE_OVS(TBL_NAME, TBL_NAME_STR) \ > ENGINE_NODE_OVSDB(ovs, "OVS", TBL_NAME, TBL_NAME_STR); > > + > +void engine_dump_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, > + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED); > +void engine_clear_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, > + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED); > + > #endif /* lib/inc-proc-eng.h */ >
> On 2/19/21 3:26 PM, Lorenzo Bianconi wrote: > > Introduce inc-engine/stats ovs-applctl command in order to dump > > ovn-controller incremental processing engine statistics. So far for each > > node a counter for run, abort and engine_handler have been added. > > Counters are incremented when the node move to "updated" state. > > In order to dump I-P stats we can can use the following commands: > > > > $ovs-appctl -t ovn-controller inc-engine/stats > > SB_address_set > > run 1 abort 0 handler 0 > > addr_sets > > run 2 abort 1 handler 0 > > SB_port_group > > run 0 abort 0 handler 0 > > port_groups > > run 2 abort 0 handler 0 > > ofctrl_is_connected > > run 1 abort 0 handler 0 > > OVS_open_vswitch > > run 0 abort 0 handler 0 > > OVS_bridge > > run 0 abort 0 handler 0 > > OVS_qos > > run 0 abort 0 handler 0 > > SB_chassis > > run 1 abort 0 handler 0 > > .... > > > > flow_output > > run 2 abort 0 handler 34 > > > > Morover introduce the inc-engine/stats-clear command to reset engine > > statistics > > > > $ovs-appctl -t ovn-controller inc-engine/stats-clear > > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > --- > > Hi Lorenzo, > > Thanks for working on this! > Hi Dumitru, thx for the review. > The commit title needs to be updated as we're not adding coverage counters > anymore. > ack, I will fix it in v4. > > Changes since v2: > > - introduce inc-engine/stats and inc-engine/stats-clear commands and drop > > COVERAGE_* dependency > > > > Changes since v1: > > - drop handler counters and add global abort counter > > - improve documentation and naming scheme > > - introduce engine_set_node_updated utility macro > > --- > > controller/ovn-controller.c | 53 ++++++++++++++++++++++--------------- > > lib/inc-proc-eng.c | 48 +++++++++++++++++++++++++++++++++ > > lib/inc-proc-eng.h | 26 ++++++++++++++++++ > > 3 files changed, 106 insertions(+), 21 deletions(-) > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 4343650fc..a937803c8 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -1011,7 +1011,7 @@ en_ofctrl_is_connected_run(struct engine_node *node, void *data) > > ofctrl_seqno_flush(); > > binding_seqno_flush(); > > } > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_run(node); > > return; > > } > > engine_set_node_state(node, EN_UNCHANGED); > > @@ -1067,7 +1067,7 @@ en_addr_sets_run(struct engine_node *node, void *data) > > addr_sets_init(as_table, &as->addr_sets); > > as->change_tracked = false; > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_run(node); > > This seems error prone to me, what if a new engine node is added and the > wrong macro (or no macro) is used to set the state? > > It's probably simpler to increment the stats in the inc-proc engine > implementation directly: > > - change handlers are only called in one place, engine_compute(): if a > change handler sets the node state to EN_UPDATED we can increment the node's > change handler stats. > - run handlers are only called in one place, engine_recompute(): if a node > handler sets the node state to EN_UPDATED we can increment the node's run > handler stats. ack, the code is much simpler doing so, thx :) I will fix it in v4. > > > } > > static bool > > @@ -1088,7 +1088,7 @@ addr_sets_sb_address_set_handler(struct engine_node *node, void *data) > > if (!sset_is_empty(&as->new) || !sset_is_empty(&as->deleted) || > > !sset_is_empty(&as->updated)) { > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > } else { > > engine_set_node_state(node, EN_UNCHANGED); > > } > > @@ -1147,7 +1147,7 @@ en_port_groups_run(struct engine_node *node, void *data) > > port_groups_init(pg_table, &pg->port_groups); > > pg->change_tracked = false; > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_run(node); > > } > > static bool > > @@ -1168,7 +1168,7 @@ port_groups_sb_port_group_handler(struct engine_node *node, void *data) > > if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) || > > !sset_is_empty(&pg->updated)) { > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > } else { > > engine_set_node_state(node, EN_UNCHANGED); > > } > > @@ -1482,7 +1482,7 @@ en_runtime_data_run(struct engine_node *node, void *data) > > binding_run(&b_ctx_in, &b_ctx_out); > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_run(node); > > } > > static bool > > @@ -1500,7 +1500,7 @@ runtime_data_ovs_interface_handler(struct engine_node *node, void *data) > > } > > if (b_ctx_out.local_lports_changed) { > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > rt_data->local_lports_changed = b_ctx_out.local_lports_changed; > > } > > @@ -1528,7 +1528,7 @@ runtime_data_sb_port_binding_handler(struct engine_node *node, void *data) > > if (b_ctx_out.local_lport_ids_changed || > > b_ctx_out.non_vif_ports_changed || > > !hmap_is_empty(b_ctx_out.tracked_dp_bindings)) { > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > } > > return true; > > @@ -1605,7 +1605,7 @@ en_ct_zones_run(struct engine_node *node, void *data) > > &ct_zones_data->pending, &rt_data->ct_updated_datapaths); > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_run(node); > > } > > /* The data in the ct_zones node is always valid (i.e., no stale pointers). */ > > @@ -1639,7 +1639,7 @@ en_mff_ovn_geneve_run(struct engine_node *node, void *data) > > enum mf_field_id mff_ovn_geneve = ofctrl_get_mf_field_id(); > > if (ed_mff_ovn_geneve->mff_ovn_geneve != mff_ovn_geneve) { > > ed_mff_ovn_geneve->mff_ovn_geneve = mff_ovn_geneve; > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_run(node); > > return; > > } > > engine_set_node_state(node, EN_UNCHANGED); > > @@ -1714,7 +1714,7 @@ en_physical_flow_changes_run(struct engine_node *node, void *data) > > { > > struct ed_type_pfc_data *pfc_tdata = data; > > pfc_tdata->recompute_physical_flows = true; > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_run(node); > > } > > /* ct_zone changes are not handled incrementally but a handler is required > > @@ -1734,7 +1734,7 @@ physical_flow_changes_ovs_iface_handler(struct engine_node *node, void *data) > > { > > struct ed_type_pfc_data *pfc_tdata = data; > > pfc_tdata->ovs_ifaces_changed = true; > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > return true; > > } > > @@ -2035,7 +2035,7 @@ en_flow_output_run(struct engine_node *node, void *data) > > physical_run(&p_ctx, &fo->flow_table); > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_run(node); > > } > > static bool > > @@ -2059,7 +2059,7 @@ flow_output_sb_logical_flow_handler(struct engine_node *node, void *data) > > bool handled = lflow_handle_changed_flows(&l_ctx_in, &l_ctx_out); > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > return handled; > > } > > @@ -2085,7 +2085,7 @@ flow_output_sb_mac_binding_handler(struct engine_node *node, void *data) > > lflow_handle_changed_neighbors(sbrec_port_binding_by_name, > > mac_binding_table, local_datapaths, flow_table); > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > return true; > > } > > @@ -2108,7 +2108,7 @@ flow_output_sb_port_binding_handler(struct engine_node *node, > > */ > > physical_handle_port_binding_changes(&p_ctx, flow_table); > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > return true; > > } > > @@ -2126,7 +2126,7 @@ flow_output_sb_multicast_group_handler(struct engine_node *node, void *data) > > physical_handle_mc_group_changes(&p_ctx, flow_table); > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > return true; > > } > > @@ -2135,6 +2135,7 @@ static bool > > _flow_output_resource_ref_handler(struct engine_node *node, void *data, > > enum ref_type ref_type) > > { > > + bool update_counter = false; > > We don't need this if we move the stats handling inside the inc-proc engine. ack, I will fix it in v4. > > > struct ed_type_runtime_data *rt_data = > > engine_get_input_data("runtime_data", node); > > @@ -2208,6 +2209,7 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, > > } > > if (changed) { > > engine_set_node_state(node, EN_UPDATED); > > + update_counter = true; > > } > > } > > SSET_FOR_EACH (ref_name, updated) { > > @@ -2217,6 +2219,7 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, > > } > > if (changed) { > > engine_set_node_state(node, EN_UPDATED); > > + update_counter = true; > > } > > } > > SSET_FOR_EACH (ref_name, new) { > > @@ -2226,8 +2229,12 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, > > } > > if (changed) { > > engine_set_node_state(node, EN_UPDATED); > > + update_counter = true; > > } > > } > > + if (update_counter) { > > + node->stats.handler++; > > + } > > return true; > > } > > @@ -2254,7 +2261,7 @@ flow_output_physical_flow_changes_handler(struct engine_node *node, void *data) > > struct physical_ctx p_ctx; > > init_physical_ctx(node, rt_data, &p_ctx); > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > struct ed_type_pfc_data *pfc_data = > > engine_get_input_data("physical_flow_changes", node); > > @@ -2293,7 +2300,7 @@ flow_output_runtime_data_handler(struct engine_node *node, > > struct hmap *tracked_dp_bindings = &rt_data->tracked_dp_bindings; > > if (hmap_is_empty(tracked_dp_bindings)) { > > if (rt_data->local_lports_changed) { > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > } > > return true; > > } > > @@ -2325,7 +2332,7 @@ flow_output_runtime_data_handler(struct engine_node *node, > > } > > } > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > return true; > > } > > @@ -2342,7 +2349,7 @@ flow_output_sb_load_balancer_handler(struct engine_node *node, void *data) > > bool handled = lflow_handle_changed_lbs(&l_ctx_in, &l_ctx_out); > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > return handled; > > } > > @@ -2681,6 +2688,10 @@ main(int argc, char *argv[]) > > unixctl_command_register("recompute", "", 0, 0, engine_recompute_cmd, > > NULL); > > + unixctl_command_register("inc-engine/stats", "", 0, 0, engine_dump_stats, > > + NULL); > > + unixctl_command_register("inc-engine/stats-clear", "", 0, 0, > > + engine_clear_stats, NULL); > > unixctl_command_register("lflow-cache/flush", "", 0, 0, > > lflow_cache_flush_cmd, > > &flow_output_data->pd); > > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c > > index 916dbbe39..ce3fed562 100644 > > --- a/lib/inc-proc-eng.c > > +++ b/lib/inc-proc-eng.c > > @@ -32,6 +32,7 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_eng); > > static bool engine_force_recompute = false; > > static bool engine_run_aborted = false; > > +static bool engine_stats_reset = false; > > static const struct engine_context *engine_context; > > static struct engine_node **engine_nodes; > > @@ -283,6 +284,7 @@ engine_recompute(struct engine_node *node, bool forced, bool allowed) > > if (!allowed) { > > VLOG_DBG("node: %s, recompute aborted", node->name); > > engine_set_node_state(node, EN_ABORTED); > > + node->stats.abort++; > > return; > > } > > @@ -383,9 +385,26 @@ engine_run(bool recompute_allowed) > > } > > } > > +static void > > +engine_reset_stats(void) > > +{ > > + if (!engine_stats_reset) { > > + return; > > + } > > + > > + engine_stats_reset = false; > > + for (size_t i = 0; i < engine_n_nodes; i++) { > > + struct engine_node *node = engine_nodes[i]; > > + > > + memset(&node->stats, 0, sizeof(node->stats)); > > + } > > +} > > + > > bool > > engine_need_run(void) > > { > > + engine_reset_stats(); > > + > > for (size_t i = 0; i < engine_n_nodes; i++) { > > /* Check only leaf nodes for updates. */ > > if (engine_nodes[i]->n_inputs) { > > @@ -401,3 +420,32 @@ engine_need_run(void) > > } > > return false; > > } > > + > > +void > > +engine_clear_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, > > + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED) > > +{ > > + engine_stats_reset = true; > > Why don't we just reset the stats directly here? ack, I will fix it in v4. > > > + unixctl_command_reply(conn, NULL); > > +} > > + > > +void > > +engine_dump_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, > > + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED) > > +{ > > + struct ds dump = DS_EMPTY_INITIALIZER; > > + > > + engine_reset_stats(); > > + > > + for (size_t i = 0; i < engine_n_nodes; i++) { > > + struct engine_node *node = engine_nodes[i]; > > + > > + ds_put_format(&dump, "%s\n", node->name); > > + ds_put_format(&dump, "\trun\t%lu", node->stats.run); > > + ds_put_format(&dump, "\tabort\t%lu", node->stats.abort); > > + ds_put_format(&dump, "\thandler\t%lu\n", node->stats.handler); > > + } > > + unixctl_command_reply(conn, ds_cstr(&dump)); > > + > > + ds_destroy(&dump); > > +} > > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h > > index 857234677..0eeca7ced 100644 > > --- a/lib/inc-proc-eng.h > > +++ b/lib/inc-proc-eng.h > > @@ -60,6 +60,8 @@ > > * against all its inputs. > > */ > > +#include "unixctl.h" > > + > > #define ENGINE_MAX_INPUT 256 > > #define ENGINE_MAX_OVSDB_INDEX 256 > > @@ -107,6 +109,12 @@ enum engine_node_state { > > EN_STATE_MAX, > > }; > > +struct engine_stats { > > + unsigned long run; > > + unsigned long abort; > > + unsigned long handler; > > Maybe it's more accurate if we use 'change_handler' instead of 'handler'? ack, I will fix it in v4. > > > +}; > > + > > struct engine_node { > > /* A unique name for each node. */ > > char *name; > > @@ -154,6 +162,9 @@ 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; > > }; > > /* Initialize the data for the engine nodes. It calls each node's > > @@ -247,6 +258,14 @@ void *engine_get_internal_data(struct engine_node *node); > > #define engine_set_node_state(node, state) \ > > engine_set_node_state_at(node, state, OVS_SOURCE_LOCATOR) > > +#define engine_set_note_update_from_run(node) \ > > s/note/node > > Also, if we keep these macros: > - multi line macros should be wrapped in do { ... } while(0). > - macro args should be wrapped in () to avoid unexpected expression > evaluations. addressing comments above we can drop these macros Regards, Lorenzo > > > + engine_set_node_state(node, EN_UPDATED); \ > > + node->stats.run++; > > + > > +#define engine_set_note_update_from_handler(node) \ > > + engine_set_node_state(node, EN_UPDATED); \ > > + node->stats.handler++; > > Same as above. > > Thanks, > Dumitru > > > + > > struct ed_ovsdb_index { > > const char *name; > > struct ovsdb_idl_index *index; > > @@ -312,6 +331,7 @@ en_##DB_NAME##_##TBL_NAME##_run(struct engine_node *node, \ > > EN_OVSDB_GET(node); \ > > if (DB_NAME##rec_##TBL_NAME##_table_track_get_first(table)) { \ > > engine_set_node_state(node, EN_UPDATED); \ > > + node->stats.run++; \ > > return; \ > > } \ > > engine_set_node_state(node, EN_UNCHANGED); \ > > @@ -352,4 +372,10 @@ static void en_##DB_NAME##_##TBL_NAME##_cleanup(void *data OVS_UNUSED) \ > > #define ENGINE_NODE_OVS(TBL_NAME, TBL_NAME_STR) \ > > ENGINE_NODE_OVSDB(ovs, "OVS", TBL_NAME, TBL_NAME_STR); > > + > > +void engine_dump_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, > > + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED); > > +void engine_clear_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, > > + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED); > > + > > #endif /* lib/inc-proc-eng.h */ > > >
On 22/02/2021 12:03, Lorenzo Bianconi wrote: >> On 2/19/21 3:26 PM, Lorenzo Bianconi wrote: >>> Introduce inc-engine/stats ovs-applctl command in order to dump >>> ovn-controller incremental processing engine statistics. So far for each >>> node a counter for run, abort and engine_handler have been added. >>> Counters are incremented when the node move to "updated" state. >>> In order to dump I-P stats we can can use the following commands: >>> >>> $ovs-appctl -t ovn-controller inc-engine/stats >>> SB_address_set >>> run 1 abort 0 handler 0 >>> addr_sets >>> run 2 abort 1 handler 0 >>> SB_port_group >>> run 0 abort 0 handler 0 >>> port_groups >>> run 2 abort 0 handler 0 >>> ofctrl_is_connected >>> run 1 abort 0 handler 0 >>> OVS_open_vswitch >>> run 0 abort 0 handler 0 >>> OVS_bridge >>> run 0 abort 0 handler 0 >>> OVS_qos >>> run 0 abort 0 handler 0 >>> SB_chassis >>> run 1 abort 0 handler 0 >>> .... >>> >>> flow_output >>> run 2 abort 0 handler 34 >>> >>> Morover introduce the inc-engine/stats-clear command to reset engine >>> statistics >>> >>> $ovs-appctl -t ovn-controller inc-engine/stats-clear >>> >>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> >>> --- >> >> Hi Lorenzo, >> >> Thanks for working on this! >> > > Hi Dumitru, > > thx for the review. > >> The commit title needs to be updated as we're not adding coverage counters >> anymore. >> > > ack, I will fix it in v4. > >>> Changes since v2: >>> - introduce inc-engine/stats and inc-engine/stats-clear commands and drop >>> COVERAGE_* dependency >>> >>> Changes since v1: >>> - drop handler counters and add global abort counter >>> - improve documentation and naming scheme >>> - introduce engine_set_node_updated utility macro >>> --- >>> controller/ovn-controller.c | 53 ++++++++++++++++++++++--------------- >>> lib/inc-proc-eng.c | 48 +++++++++++++++++++++++++++++++++ >>> lib/inc-proc-eng.h | 26 ++++++++++++++++++ >>> 3 files changed, 106 insertions(+), 21 deletions(-) >>> >>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >>> index 4343650fc..a937803c8 100644 >>> --- a/controller/ovn-controller.c >>> +++ b/controller/ovn-controller.c >>> @@ -1011,7 +1011,7 @@ en_ofctrl_is_connected_run(struct engine_node *node, void *data) >>> ofctrl_seqno_flush(); >>> binding_seqno_flush(); >>> } >>> - engine_set_node_state(node, EN_UPDATED); >>> + engine_set_note_update_from_run(node); >>> return; >>> } >>> engine_set_node_state(node, EN_UNCHANGED); >>> @@ -1067,7 +1067,7 @@ en_addr_sets_run(struct engine_node *node, void *data) >>> addr_sets_init(as_table, &as->addr_sets); >>> as->change_tracked = false; >>> - engine_set_node_state(node, EN_UPDATED); >>> + engine_set_note_update_from_run(node); >> >> This seems error prone to me, what if a new engine node is added and the >> wrong macro (or no macro) is used to set the state? >> >> It's probably simpler to increment the stats in the inc-proc engine >> implementation directly: >> >> - change handlers are only called in one place, engine_compute(): if a >> change handler sets the node state to EN_UPDATED we can increment the node's >> change handler stats. >> - run handlers are only called in one place, engine_recompute(): if a node >> handler sets the node state to EN_UPDATED we can increment the node's run >> handler stats. > > ack, the code is much simpler doing so, thx :) I will fix it in v4. I made a suggestion in v4, I think we should count the state at the end of each engine_node_run() invocation (as this will count the final state of each node). I also suggestion counting the number of times we "compute" (i.e. attempt an incremental_compute) and "recompute". Dumitru, what do you think - as this goes against what you are suggestion? >> >>> } >>> static bool >>> @@ -1088,7 +1088,7 @@ addr_sets_sb_address_set_handler(struct engine_node *node, void *data) >>> if (!sset_is_empty(&as->new) || !sset_is_empty(&as->deleted) || >>> !sset_is_empty(&as->updated)) { >>> - engine_set_node_state(node, EN_UPDATED); >>> + engine_set_note_update_from_handler(node); >>> } else { >>> engine_set_node_state(node, EN_UNCHANGED); >>> } >>> @@ -1147,7 +1147,7 @@ en_port_groups_run(struct engine_node *node, void *data) >>> port_groups_init(pg_table, &pg->port_groups); >>> pg->change_tracked = false; >>> - engine_set_node_state(node, EN_UPDATED); >>> + engine_set_note_update_from_run(node); >>> } >>> static bool >>> @@ -1168,7 +1168,7 @@ port_groups_sb_port_group_handler(struct engine_node *node, void *data) >>> if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) || >>> !sset_is_empty(&pg->updated)) { >>> - engine_set_node_state(node, EN_UPDATED); >>> + engine_set_note_update_from_handler(node); >>> } else { >>> engine_set_node_state(node, EN_UNCHANGED); >>> } >>> @@ -1482,7 +1482,7 @@ en_runtime_data_run(struct engine_node *node, void *data) >>> binding_run(&b_ctx_in, &b_ctx_out); >>> - engine_set_node_state(node, EN_UPDATED); >>> + engine_set_note_update_from_run(node); >>> } >>> static bool >>> @@ -1500,7 +1500,7 @@ runtime_data_ovs_interface_handler(struct engine_node *node, void *data) >>> } >>> if (b_ctx_out.local_lports_changed) { >>> - engine_set_node_state(node, EN_UPDATED); >>> + engine_set_note_update_from_handler(node); >>> rt_data->local_lports_changed = b_ctx_out.local_lports_changed; >>> } >>> @@ -1528,7 +1528,7 @@ runtime_data_sb_port_binding_handler(struct engine_node *node, void *data) >>> if (b_ctx_out.local_lport_ids_changed || >>> b_ctx_out.non_vif_ports_changed || >>> !hmap_is_empty(b_ctx_out.tracked_dp_bindings)) { >>> - engine_set_node_state(node, EN_UPDATED); >>> + engine_set_note_update_from_handler(node); >>> } >>> return true; >>> @@ -1605,7 +1605,7 @@ en_ct_zones_run(struct engine_node *node, void *data) >>> &ct_zones_data->pending, &rt_data->ct_updated_datapaths); >>> - engine_set_node_state(node, EN_UPDATED); >>> + engine_set_note_update_from_run(node); >>> } >>> /* The data in the ct_zones node is always valid (i.e., no stale pointers). */ >>> @@ -1639,7 +1639,7 @@ en_mff_ovn_geneve_run(struct engine_node *node, void *data) >>> enum mf_field_id mff_ovn_geneve = ofctrl_get_mf_field_id(); >>> if (ed_mff_ovn_geneve->mff_ovn_geneve != mff_ovn_geneve) { >>> ed_mff_ovn_geneve->mff_ovn_geneve = mff_ovn_geneve; >>> - engine_set_node_state(node, EN_UPDATED); >>> + engine_set_note_update_from_run(node); >>> return; >>> } >>> engine_set_node_state(node, EN_UNCHANGED); >>> @@ -1714,7 +1714,7 @@ en_physical_flow_changes_run(struct engine_node *node, void *data) >>> { >>> struct ed_type_pfc_data *pfc_tdata = data; >>> pfc_tdata->recompute_physical_flows = true; >>> - engine_set_node_state(node, EN_UPDATED); >>> + engine_set_note_update_from_run(node); >>> } >>> /* ct_zone changes are not handled incrementally but a handler is required >>> @@ -1734,7 +1734,7 @@ physical_flow_changes_ovs_iface_handler(struct engine_node *node, void *data) >>> { >>> struct ed_type_pfc_data *pfc_tdata = data; >>> pfc_tdata->ovs_ifaces_changed = true; >>> - engine_set_node_state(node, EN_UPDATED); >>> + engine_set_note_update_from_handler(node); >>> return true; >>> } >>> @@ -2035,7 +2035,7 @@ en_flow_output_run(struct engine_node *node, void *data) >>> physical_run(&p_ctx, &fo->flow_table); >>> - engine_set_node_state(node, EN_UPDATED); >>> + engine_set_note_update_from_run(node); >>> } >>> static bool >>> @@ -2059,7 +2059,7 @@ flow_output_sb_logical_flow_handler(struct engine_node *node, void *data) >>> bool handled = lflow_handle_changed_flows(&l_ctx_in, &l_ctx_out); >>> - engine_set_node_state(node, EN_UPDATED); >>> + engine_set_note_update_from_handler(node); >>> return handled; >>> } >>> @@ -2085,7 +2085,7 @@ flow_output_sb_mac_binding_handler(struct engine_node *node, void *data) >>> lflow_handle_changed_neighbors(sbrec_port_binding_by_name, >>> mac_binding_table, local_datapaths, flow_table); >>> - engine_set_node_state(node, EN_UPDATED); >>> + engine_set_note_update_from_handler(node); >>> return true; >>> } >>> @@ -2108,7 +2108,7 @@ flow_output_sb_port_binding_handler(struct engine_node *node, >>> */ >>> physical_handle_port_binding_changes(&p_ctx, flow_table); >>> - engine_set_node_state(node, EN_UPDATED); >>> + engine_set_note_update_from_handler(node); >>> return true; >>> } >>> @@ -2126,7 +2126,7 @@ flow_output_sb_multicast_group_handler(struct engine_node *node, void *data) >>> physical_handle_mc_group_changes(&p_ctx, flow_table); >>> - engine_set_node_state(node, EN_UPDATED); >>> + engine_set_note_update_from_handler(node); >>> return true; >>> } >>> @@ -2135,6 +2135,7 @@ static bool >>> _flow_output_resource_ref_handler(struct engine_node *node, void *data, >>> enum ref_type ref_type) >>> { >>> + bool update_counter = false; >> >> We don't need this if we move the stats handling inside the inc-proc engine. > > ack, I will fix it in v4. > >> >>> struct ed_type_runtime_data *rt_data = >>> engine_get_input_data("runtime_data", node); >>> @@ -2208,6 +2209,7 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, >>> } >>> if (changed) { >>> engine_set_node_state(node, EN_UPDATED); >>> + update_counter = true; >>> } >>> } >>> SSET_FOR_EACH (ref_name, updated) { >>> @@ -2217,6 +2219,7 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, >>> } >>> if (changed) { >>> engine_set_node_state(node, EN_UPDATED); >>> + update_counter = true; >>> } >>> } >>> SSET_FOR_EACH (ref_name, new) { >>> @@ -2226,8 +2229,12 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, >>> } >>> if (changed) { >>> engine_set_node_state(node, EN_UPDATED); >>> + update_counter = true; >>> } >>> } >>> + if (update_counter) { >>> + node->stats.handler++; >>> + } >>> return true; >>> } >>> @@ -2254,7 +2261,7 @@ flow_output_physical_flow_changes_handler(struct engine_node *node, void *data) >>> struct physical_ctx p_ctx; >>> init_physical_ctx(node, rt_data, &p_ctx); >>> - engine_set_node_state(node, EN_UPDATED); >>> + engine_set_note_update_from_handler(node); >>> struct ed_type_pfc_data *pfc_data = >>> engine_get_input_data("physical_flow_changes", node); >>> @@ -2293,7 +2300,7 @@ flow_output_runtime_data_handler(struct engine_node *node, >>> struct hmap *tracked_dp_bindings = &rt_data->tracked_dp_bindings; >>> if (hmap_is_empty(tracked_dp_bindings)) { >>> if (rt_data->local_lports_changed) { >>> - engine_set_node_state(node, EN_UPDATED); >>> + engine_set_note_update_from_handler(node); >>> } >>> return true; >>> } >>> @@ -2325,7 +2332,7 @@ flow_output_runtime_data_handler(struct engine_node *node, >>> } >>> } >>> - engine_set_node_state(node, EN_UPDATED); >>> + engine_set_note_update_from_handler(node); >>> return true; >>> } >>> @@ -2342,7 +2349,7 @@ flow_output_sb_load_balancer_handler(struct engine_node *node, void *data) >>> bool handled = lflow_handle_changed_lbs(&l_ctx_in, &l_ctx_out); >>> - engine_set_node_state(node, EN_UPDATED); >>> + engine_set_note_update_from_handler(node); >>> return handled; >>> } >>> @@ -2681,6 +2688,10 @@ main(int argc, char *argv[]) >>> unixctl_command_register("recompute", "", 0, 0, engine_recompute_cmd, >>> NULL); >>> + unixctl_command_register("inc-engine/stats", "", 0, 0, engine_dump_stats, >>> + NULL); >>> + unixctl_command_register("inc-engine/stats-clear", "", 0, 0, >>> + engine_clear_stats, NULL); >>> unixctl_command_register("lflow-cache/flush", "", 0, 0, >>> lflow_cache_flush_cmd, >>> &flow_output_data->pd); >>> diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c >>> index 916dbbe39..ce3fed562 100644 >>> --- a/lib/inc-proc-eng.c >>> +++ b/lib/inc-proc-eng.c >>> @@ -32,6 +32,7 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_eng); >>> static bool engine_force_recompute = false; >>> static bool engine_run_aborted = false; >>> +static bool engine_stats_reset = false; >>> static const struct engine_context *engine_context; >>> static struct engine_node **engine_nodes; >>> @@ -283,6 +284,7 @@ engine_recompute(struct engine_node *node, bool forced, bool allowed) >>> if (!allowed) { >>> VLOG_DBG("node: %s, recompute aborted", node->name); >>> engine_set_node_state(node, EN_ABORTED); >>> + node->stats.abort++; >>> return; >>> } >>> @@ -383,9 +385,26 @@ engine_run(bool recompute_allowed) >>> } >>> } >>> +static void >>> +engine_reset_stats(void) >>> +{ >>> + if (!engine_stats_reset) { >>> + return; >>> + } >>> + >>> + engine_stats_reset = false; >>> + for (size_t i = 0; i < engine_n_nodes; i++) { >>> + struct engine_node *node = engine_nodes[i]; >>> + >>> + memset(&node->stats, 0, sizeof(node->stats)); >>> + } >>> +} >>> + >>> bool >>> engine_need_run(void) >>> { >>> + engine_reset_stats(); >>> + >>> for (size_t i = 0; i < engine_n_nodes; i++) { >>> /* Check only leaf nodes for updates. */ >>> if (engine_nodes[i]->n_inputs) { >>> @@ -401,3 +420,32 @@ engine_need_run(void) >>> } >>> return false; >>> } >>> + >>> +void >>> +engine_clear_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, >>> + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED) >>> +{ >>> + engine_stats_reset = true; >> >> Why don't we just reset the stats directly here? > > ack, I will fix it in v4. > >> >>> + unixctl_command_reply(conn, NULL); >>> +} >>> + >>> +void >>> +engine_dump_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, >>> + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED) >>> +{ >>> + struct ds dump = DS_EMPTY_INITIALIZER; >>> + >>> + engine_reset_stats(); >>> + >>> + for (size_t i = 0; i < engine_n_nodes; i++) { >>> + struct engine_node *node = engine_nodes[i]; >>> + >>> + ds_put_format(&dump, "%s\n", node->name); >>> + ds_put_format(&dump, "\trun\t%lu", node->stats.run); >>> + ds_put_format(&dump, "\tabort\t%lu", node->stats.abort); >>> + ds_put_format(&dump, "\thandler\t%lu\n", node->stats.handler); >>> + } >>> + unixctl_command_reply(conn, ds_cstr(&dump)); >>> + >>> + ds_destroy(&dump); >>> +} >>> diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h >>> index 857234677..0eeca7ced 100644 >>> --- a/lib/inc-proc-eng.h >>> +++ b/lib/inc-proc-eng.h >>> @@ -60,6 +60,8 @@ >>> * against all its inputs. >>> */ >>> +#include "unixctl.h" >>> + >>> #define ENGINE_MAX_INPUT 256 >>> #define ENGINE_MAX_OVSDB_INDEX 256 >>> @@ -107,6 +109,12 @@ enum engine_node_state { >>> EN_STATE_MAX, >>> }; >>> +struct engine_stats { >>> + unsigned long run; >>> + unsigned long abort; >>> + unsigned long handler; >> >> Maybe it's more accurate if we use 'change_handler' instead of 'handler'? > > ack, I will fix it in v4. > >> >>> +}; >>> + >>> struct engine_node { >>> /* A unique name for each node. */ >>> char *name; >>> @@ -154,6 +162,9 @@ 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; >>> }; >>> /* Initialize the data for the engine nodes. It calls each node's >>> @@ -247,6 +258,14 @@ void *engine_get_internal_data(struct engine_node *node); >>> #define engine_set_node_state(node, state) \ >>> engine_set_node_state_at(node, state, OVS_SOURCE_LOCATOR) >>> +#define engine_set_note_update_from_run(node) \ >> >> s/note/node >> >> Also, if we keep these macros: >> - multi line macros should be wrapped in do { ... } while(0). >> - macro args should be wrapped in () to avoid unexpected expression >> evaluations. > > addressing comments above we can drop these macros > > Regards, > Lorenzo > >> >>> + engine_set_node_state(node, EN_UPDATED); \ >>> + node->stats.run++; >>> + >>> +#define engine_set_note_update_from_handler(node) \ >>> + engine_set_node_state(node, EN_UPDATED); \ >>> + node->stats.handler++; >> >> Same as above. >> >> Thanks, >> Dumitru >> >>> + >>> struct ed_ovsdb_index { >>> const char *name; >>> struct ovsdb_idl_index *index; >>> @@ -312,6 +331,7 @@ en_##DB_NAME##_##TBL_NAME##_run(struct engine_node *node, \ >>> EN_OVSDB_GET(node); \ >>> if (DB_NAME##rec_##TBL_NAME##_table_track_get_first(table)) { \ >>> engine_set_node_state(node, EN_UPDATED); \ >>> + node->stats.run++; \ >>> return; \ >>> } \ >>> engine_set_node_state(node, EN_UNCHANGED); \ >>> @@ -352,4 +372,10 @@ static void en_##DB_NAME##_##TBL_NAME##_cleanup(void *data OVS_UNUSED) \ >>> #define ENGINE_NODE_OVS(TBL_NAME, TBL_NAME_STR) \ >>> ENGINE_NODE_OVSDB(ovs, "OVS", TBL_NAME, TBL_NAME_STR); >>> + >>> +void engine_dump_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, >>> + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED); >>> +void engine_clear_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, >>> + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED); >>> + >>> #endif /* lib/inc-proc-eng.h */ >>> >>
On 19/02/2021 14:26, Lorenzo Bianconi wrote: > Introduce inc-engine/stats ovs-applctl command in order to dump > ovn-controller incremental processing engine statistics. So far for each > node a counter for run, abort and engine_handler have been added. > Counters are incremented when the node move to "updated" state. > In order to dump I-P stats we can can use the following commands: > > $ovs-appctl -t ovn-controller inc-engine/stats > SB_address_set > run 1 abort 0 handler 0 > addr_sets > run 2 abort 1 handler 0 > SB_port_group > run 0 abort 0 handler 0 > port_groups > run 2 abort 0 handler 0 > ofctrl_is_connected > run 1 abort 0 handler 0 > OVS_open_vswitch > run 0 abort 0 handler 0 > OVS_bridge > run 0 abort 0 handler 0 > OVS_qos > run 0 abort 0 handler 0 > SB_chassis > run 1 abort 0 handler 0 > .... > > flow_output > run 2 abort 0 handler 34 > > Morover introduce the inc-engine/stats-clear command to reset engine > statistics > > $ovs-appctl -t ovn-controller inc-engine/stats-clear > Hi Lorenzo, One disadvantage of a clear/show type stat is you don't have builtin rate information like coverage counters and if you get an offline report, you don't know how long it has been since the counters were cleared, or if they were ever cleared. You could consider to embed duration info into the show command. For example, 'pmd-perf-show' command shows e.g. $ ovs-appctl dpif-netdev/pmd-perf-show Time: 14:07:57.468 Measurement duration: 4.480 s <pmd stats> For the pmd case, I'm not sure that wall clock is really useful, but measurement duration (time since last clear) is. Kevin. > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- > Changes since v2: > - introduce inc-engine/stats and inc-engine/stats-clear commands and drop > COVERAGE_* dependency > > Changes since v1: > - drop handler counters and add global abort counter > - improve documentation and naming scheme > - introduce engine_set_node_updated utility macro > --- > controller/ovn-controller.c | 53 ++++++++++++++++++++++--------------- > lib/inc-proc-eng.c | 48 +++++++++++++++++++++++++++++++++ > lib/inc-proc-eng.h | 26 ++++++++++++++++++ > 3 files changed, 106 insertions(+), 21 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 4343650fc..a937803c8 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -1011,7 +1011,7 @@ en_ofctrl_is_connected_run(struct engine_node *node, void *data) > ofctrl_seqno_flush(); > binding_seqno_flush(); > } > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_run(node); > return; > } > engine_set_node_state(node, EN_UNCHANGED); > @@ -1067,7 +1067,7 @@ en_addr_sets_run(struct engine_node *node, void *data) > addr_sets_init(as_table, &as->addr_sets); > > as->change_tracked = false; > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_run(node); > } > > static bool > @@ -1088,7 +1088,7 @@ addr_sets_sb_address_set_handler(struct engine_node *node, void *data) > > if (!sset_is_empty(&as->new) || !sset_is_empty(&as->deleted) || > !sset_is_empty(&as->updated)) { > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > } else { > engine_set_node_state(node, EN_UNCHANGED); > } > @@ -1147,7 +1147,7 @@ en_port_groups_run(struct engine_node *node, void *data) > port_groups_init(pg_table, &pg->port_groups); > > pg->change_tracked = false; > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_run(node); > } > > static bool > @@ -1168,7 +1168,7 @@ port_groups_sb_port_group_handler(struct engine_node *node, void *data) > > if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) || > !sset_is_empty(&pg->updated)) { > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > } else { > engine_set_node_state(node, EN_UNCHANGED); > } > @@ -1482,7 +1482,7 @@ en_runtime_data_run(struct engine_node *node, void *data) > > binding_run(&b_ctx_in, &b_ctx_out); > > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_run(node); > } > > static bool > @@ -1500,7 +1500,7 @@ runtime_data_ovs_interface_handler(struct engine_node *node, void *data) > } > > if (b_ctx_out.local_lports_changed) { > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > rt_data->local_lports_changed = b_ctx_out.local_lports_changed; > } > > @@ -1528,7 +1528,7 @@ runtime_data_sb_port_binding_handler(struct engine_node *node, void *data) > if (b_ctx_out.local_lport_ids_changed || > b_ctx_out.non_vif_ports_changed || > !hmap_is_empty(b_ctx_out.tracked_dp_bindings)) { > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > } > > return true; > @@ -1605,7 +1605,7 @@ en_ct_zones_run(struct engine_node *node, void *data) > &ct_zones_data->pending, &rt_data->ct_updated_datapaths); > > > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_run(node); > } > > /* The data in the ct_zones node is always valid (i.e., no stale pointers). */ > @@ -1639,7 +1639,7 @@ en_mff_ovn_geneve_run(struct engine_node *node, void *data) > enum mf_field_id mff_ovn_geneve = ofctrl_get_mf_field_id(); > if (ed_mff_ovn_geneve->mff_ovn_geneve != mff_ovn_geneve) { > ed_mff_ovn_geneve->mff_ovn_geneve = mff_ovn_geneve; > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_run(node); > return; > } > engine_set_node_state(node, EN_UNCHANGED); > @@ -1714,7 +1714,7 @@ en_physical_flow_changes_run(struct engine_node *node, void *data) > { > struct ed_type_pfc_data *pfc_tdata = data; > pfc_tdata->recompute_physical_flows = true; > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_run(node); > } > > /* ct_zone changes are not handled incrementally but a handler is required > @@ -1734,7 +1734,7 @@ physical_flow_changes_ovs_iface_handler(struct engine_node *node, void *data) > { > struct ed_type_pfc_data *pfc_tdata = data; > pfc_tdata->ovs_ifaces_changed = true; > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > return true; > } > > @@ -2035,7 +2035,7 @@ en_flow_output_run(struct engine_node *node, void *data) > > physical_run(&p_ctx, &fo->flow_table); > > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_run(node); > } > > static bool > @@ -2059,7 +2059,7 @@ flow_output_sb_logical_flow_handler(struct engine_node *node, void *data) > > bool handled = lflow_handle_changed_flows(&l_ctx_in, &l_ctx_out); > > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > return handled; > } > > @@ -2085,7 +2085,7 @@ flow_output_sb_mac_binding_handler(struct engine_node *node, void *data) > lflow_handle_changed_neighbors(sbrec_port_binding_by_name, > mac_binding_table, local_datapaths, flow_table); > > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > return true; > } > > @@ -2108,7 +2108,7 @@ flow_output_sb_port_binding_handler(struct engine_node *node, > */ > physical_handle_port_binding_changes(&p_ctx, flow_table); > > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > return true; > } > > @@ -2126,7 +2126,7 @@ flow_output_sb_multicast_group_handler(struct engine_node *node, void *data) > > physical_handle_mc_group_changes(&p_ctx, flow_table); > > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > return true; > > } > @@ -2135,6 +2135,7 @@ static bool > _flow_output_resource_ref_handler(struct engine_node *node, void *data, > enum ref_type ref_type) > { > + bool update_counter = false; > struct ed_type_runtime_data *rt_data = > engine_get_input_data("runtime_data", node); > > @@ -2208,6 +2209,7 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, > } > if (changed) { > engine_set_node_state(node, EN_UPDATED); > + update_counter = true; > } > } > SSET_FOR_EACH (ref_name, updated) { > @@ -2217,6 +2219,7 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, > } > if (changed) { > engine_set_node_state(node, EN_UPDATED); > + update_counter = true; > } > } > SSET_FOR_EACH (ref_name, new) { > @@ -2226,8 +2229,12 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, > } > if (changed) { > engine_set_node_state(node, EN_UPDATED); > + update_counter = true; > } > } > + if (update_counter) { > + node->stats.handler++; > + } > > return true; > } > @@ -2254,7 +2261,7 @@ flow_output_physical_flow_changes_handler(struct engine_node *node, void *data) > struct physical_ctx p_ctx; > init_physical_ctx(node, rt_data, &p_ctx); > > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > struct ed_type_pfc_data *pfc_data = > engine_get_input_data("physical_flow_changes", node); > > @@ -2293,7 +2300,7 @@ flow_output_runtime_data_handler(struct engine_node *node, > struct hmap *tracked_dp_bindings = &rt_data->tracked_dp_bindings; > if (hmap_is_empty(tracked_dp_bindings)) { > if (rt_data->local_lports_changed) { > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > } > return true; > } > @@ -2325,7 +2332,7 @@ flow_output_runtime_data_handler(struct engine_node *node, > } > } > > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > return true; > } > > @@ -2342,7 +2349,7 @@ flow_output_sb_load_balancer_handler(struct engine_node *node, void *data) > > bool handled = lflow_handle_changed_lbs(&l_ctx_in, &l_ctx_out); > > - engine_set_node_state(node, EN_UPDATED); > + engine_set_note_update_from_handler(node); > return handled; > } > > @@ -2681,6 +2688,10 @@ main(int argc, char *argv[]) > > unixctl_command_register("recompute", "", 0, 0, engine_recompute_cmd, > NULL); > + unixctl_command_register("inc-engine/stats", "", 0, 0, engine_dump_stats, > + NULL); > + unixctl_command_register("inc-engine/stats-clear", "", 0, 0, > + engine_clear_stats, NULL); > unixctl_command_register("lflow-cache/flush", "", 0, 0, > lflow_cache_flush_cmd, > &flow_output_data->pd); > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c > index 916dbbe39..ce3fed562 100644 > --- a/lib/inc-proc-eng.c > +++ b/lib/inc-proc-eng.c > @@ -32,6 +32,7 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_eng); > > static bool engine_force_recompute = false; > static bool engine_run_aborted = false; > +static bool engine_stats_reset = false; > static const struct engine_context *engine_context; > > static struct engine_node **engine_nodes; > @@ -283,6 +284,7 @@ engine_recompute(struct engine_node *node, bool forced, bool allowed) > if (!allowed) { > VLOG_DBG("node: %s, recompute aborted", node->name); > engine_set_node_state(node, EN_ABORTED); > + node->stats.abort++; > return; > } > > @@ -383,9 +385,26 @@ engine_run(bool recompute_allowed) > } > } > > +static void > +engine_reset_stats(void) > +{ > + if (!engine_stats_reset) { > + return; > + } > + > + engine_stats_reset = false; > + for (size_t i = 0; i < engine_n_nodes; i++) { > + struct engine_node *node = engine_nodes[i]; > + > + memset(&node->stats, 0, sizeof(node->stats)); > + } > +} > + > bool > engine_need_run(void) > { > + engine_reset_stats(); > + > for (size_t i = 0; i < engine_n_nodes; i++) { > /* Check only leaf nodes for updates. */ > if (engine_nodes[i]->n_inputs) { > @@ -401,3 +420,32 @@ engine_need_run(void) > } > return false; > } > + > +void > +engine_clear_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, > + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED) > +{ > + engine_stats_reset = true; > + unixctl_command_reply(conn, NULL); > +} > + > +void > +engine_dump_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, > + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED) > +{ > + struct ds dump = DS_EMPTY_INITIALIZER; > + > + engine_reset_stats(); > + > + for (size_t i = 0; i < engine_n_nodes; i++) { > + struct engine_node *node = engine_nodes[i]; > + > + ds_put_format(&dump, "%s\n", node->name); > + ds_put_format(&dump, "\trun\t%lu", node->stats.run); > + ds_put_format(&dump, "\tabort\t%lu", node->stats.abort); > + ds_put_format(&dump, "\thandler\t%lu\n", node->stats.handler); > + } > + unixctl_command_reply(conn, ds_cstr(&dump)); > + > + ds_destroy(&dump); > +} > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h > index 857234677..0eeca7ced 100644 > --- a/lib/inc-proc-eng.h > +++ b/lib/inc-proc-eng.h > @@ -60,6 +60,8 @@ > * against all its inputs. > */ > > +#include "unixctl.h" > + > #define ENGINE_MAX_INPUT 256 > #define ENGINE_MAX_OVSDB_INDEX 256 > > @@ -107,6 +109,12 @@ enum engine_node_state { > EN_STATE_MAX, > }; > > +struct engine_stats { > + unsigned long run; > + unsigned long abort; > + unsigned long handler; > +}; > + > struct engine_node { > /* A unique name for each node. */ > char *name; > @@ -154,6 +162,9 @@ 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; > }; > > /* Initialize the data for the engine nodes. It calls each node's > @@ -247,6 +258,14 @@ void *engine_get_internal_data(struct engine_node *node); > #define engine_set_node_state(node, state) \ > engine_set_node_state_at(node, state, OVS_SOURCE_LOCATOR) > > +#define engine_set_note_update_from_run(node) \ > + engine_set_node_state(node, EN_UPDATED); \ > + node->stats.run++; > + > +#define engine_set_note_update_from_handler(node) \ > + engine_set_node_state(node, EN_UPDATED); \ > + node->stats.handler++; > + > struct ed_ovsdb_index { > const char *name; > struct ovsdb_idl_index *index; > @@ -312,6 +331,7 @@ en_##DB_NAME##_##TBL_NAME##_run(struct engine_node *node, \ > EN_OVSDB_GET(node); \ > if (DB_NAME##rec_##TBL_NAME##_table_track_get_first(table)) { \ > engine_set_node_state(node, EN_UPDATED); \ > + node->stats.run++; \ > return; \ > } \ > engine_set_node_state(node, EN_UNCHANGED); \ > @@ -352,4 +372,10 @@ static void en_##DB_NAME##_##TBL_NAME##_cleanup(void *data OVS_UNUSED) \ > #define ENGINE_NODE_OVS(TBL_NAME, TBL_NAME_STR) \ > ENGINE_NODE_OVSDB(ovs, "OVS", TBL_NAME, TBL_NAME_STR); > > + > +void engine_dump_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, > + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED); > +void engine_clear_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, > + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED); > + > #endif /* lib/inc-proc-eng.h */ >
> On 19/02/2021 14:26, Lorenzo Bianconi wrote: > > Introduce inc-engine/stats ovs-applctl command in order to dump > > ovn-controller incremental processing engine statistics. So far for each > > node a counter for run, abort and engine_handler have been added. > > Counters are incremented when the node move to "updated" state. > > In order to dump I-P stats we can can use the following commands: > > > > $ovs-appctl -t ovn-controller inc-engine/stats > > SB_address_set > > run 1 abort 0 handler 0 > > addr_sets > > run 2 abort 1 handler 0 > > SB_port_group > > run 0 abort 0 handler 0 > > port_groups > > run 2 abort 0 handler 0 > > ofctrl_is_connected > > run 1 abort 0 handler 0 > > OVS_open_vswitch > > run 0 abort 0 handler 0 > > OVS_bridge > > run 0 abort 0 handler 0 > > OVS_qos > > run 0 abort 0 handler 0 > > SB_chassis > > run 1 abort 0 handler 0 > > .... > > > > flow_output > > run 2 abort 0 handler 34 > > > > Morover introduce the inc-engine/stats-clear command to reset engine > > statistics > > > > $ovs-appctl -t ovn-controller inc-engine/stats-clear > > > > Hi Lorenzo, > Hi Kevin, thx for the review. > One disadvantage of a clear/show type stat is you don't have builtin > rate information like coverage counters and if you get an offline > report, you don't know how long it has been since the counters were > cleared, or if they were ever cleared. > > You could consider to embed duration info into the show command. For > example, 'pmd-perf-show' command shows e.g. > > $ ovs-appctl dpif-netdev/pmd-perf-show > Time: 14:07:57.468 > Measurement duration: 4.480 s > <pmd stats> > > > For the pmd case, I'm not sure that wall clock is really useful, but > measurement duration (time since last clear) is. I agree, I do not think the rate is useful in this case but the time from last "clear" request is a nice to have I guess. I will add it in v5. Regards, Lorenzo > > Kevin. > > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > --- > > Changes since v2: > > - introduce inc-engine/stats and inc-engine/stats-clear commands and drop > > COVERAGE_* dependency > > > > Changes since v1: > > - drop handler counters and add global abort counter > > - improve documentation and naming scheme > > - introduce engine_set_node_updated utility macro > > --- > > controller/ovn-controller.c | 53 ++++++++++++++++++++++--------------- > > lib/inc-proc-eng.c | 48 +++++++++++++++++++++++++++++++++ > > lib/inc-proc-eng.h | 26 ++++++++++++++++++ > > 3 files changed, 106 insertions(+), 21 deletions(-) > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 4343650fc..a937803c8 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -1011,7 +1011,7 @@ en_ofctrl_is_connected_run(struct engine_node *node, void *data) > > ofctrl_seqno_flush(); > > binding_seqno_flush(); > > } > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_run(node); > > return; > > } > > engine_set_node_state(node, EN_UNCHANGED); > > @@ -1067,7 +1067,7 @@ en_addr_sets_run(struct engine_node *node, void *data) > > addr_sets_init(as_table, &as->addr_sets); > > > > as->change_tracked = false; > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_run(node); > > } > > > > static bool > > @@ -1088,7 +1088,7 @@ addr_sets_sb_address_set_handler(struct engine_node *node, void *data) > > > > if (!sset_is_empty(&as->new) || !sset_is_empty(&as->deleted) || > > !sset_is_empty(&as->updated)) { > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > } else { > > engine_set_node_state(node, EN_UNCHANGED); > > } > > @@ -1147,7 +1147,7 @@ en_port_groups_run(struct engine_node *node, void *data) > > port_groups_init(pg_table, &pg->port_groups); > > > > pg->change_tracked = false; > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_run(node); > > } > > > > static bool > > @@ -1168,7 +1168,7 @@ port_groups_sb_port_group_handler(struct engine_node *node, void *data) > > > > if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) || > > !sset_is_empty(&pg->updated)) { > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > } else { > > engine_set_node_state(node, EN_UNCHANGED); > > } > > @@ -1482,7 +1482,7 @@ en_runtime_data_run(struct engine_node *node, void *data) > > > > binding_run(&b_ctx_in, &b_ctx_out); > > > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_run(node); > > } > > > > static bool > > @@ -1500,7 +1500,7 @@ runtime_data_ovs_interface_handler(struct engine_node *node, void *data) > > } > > > > if (b_ctx_out.local_lports_changed) { > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > rt_data->local_lports_changed = b_ctx_out.local_lports_changed; > > } > > > > @@ -1528,7 +1528,7 @@ runtime_data_sb_port_binding_handler(struct engine_node *node, void *data) > > if (b_ctx_out.local_lport_ids_changed || > > b_ctx_out.non_vif_ports_changed || > > !hmap_is_empty(b_ctx_out.tracked_dp_bindings)) { > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > } > > > > return true; > > @@ -1605,7 +1605,7 @@ en_ct_zones_run(struct engine_node *node, void *data) > > &ct_zones_data->pending, &rt_data->ct_updated_datapaths); > > > > > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_run(node); > > } > > > > /* The data in the ct_zones node is always valid (i.e., no stale pointers). */ > > @@ -1639,7 +1639,7 @@ en_mff_ovn_geneve_run(struct engine_node *node, void *data) > > enum mf_field_id mff_ovn_geneve = ofctrl_get_mf_field_id(); > > if (ed_mff_ovn_geneve->mff_ovn_geneve != mff_ovn_geneve) { > > ed_mff_ovn_geneve->mff_ovn_geneve = mff_ovn_geneve; > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_run(node); > > return; > > } > > engine_set_node_state(node, EN_UNCHANGED); > > @@ -1714,7 +1714,7 @@ en_physical_flow_changes_run(struct engine_node *node, void *data) > > { > > struct ed_type_pfc_data *pfc_tdata = data; > > pfc_tdata->recompute_physical_flows = true; > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_run(node); > > } > > > > /* ct_zone changes are not handled incrementally but a handler is required > > @@ -1734,7 +1734,7 @@ physical_flow_changes_ovs_iface_handler(struct engine_node *node, void *data) > > { > > struct ed_type_pfc_data *pfc_tdata = data; > > pfc_tdata->ovs_ifaces_changed = true; > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > return true; > > } > > > > @@ -2035,7 +2035,7 @@ en_flow_output_run(struct engine_node *node, void *data) > > > > physical_run(&p_ctx, &fo->flow_table); > > > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_run(node); > > } > > > > static bool > > @@ -2059,7 +2059,7 @@ flow_output_sb_logical_flow_handler(struct engine_node *node, void *data) > > > > bool handled = lflow_handle_changed_flows(&l_ctx_in, &l_ctx_out); > > > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > return handled; > > } > > > > @@ -2085,7 +2085,7 @@ flow_output_sb_mac_binding_handler(struct engine_node *node, void *data) > > lflow_handle_changed_neighbors(sbrec_port_binding_by_name, > > mac_binding_table, local_datapaths, flow_table); > > > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > return true; > > } > > > > @@ -2108,7 +2108,7 @@ flow_output_sb_port_binding_handler(struct engine_node *node, > > */ > > physical_handle_port_binding_changes(&p_ctx, flow_table); > > > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > return true; > > } > > > > @@ -2126,7 +2126,7 @@ flow_output_sb_multicast_group_handler(struct engine_node *node, void *data) > > > > physical_handle_mc_group_changes(&p_ctx, flow_table); > > > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > return true; > > > > } > > @@ -2135,6 +2135,7 @@ static bool > > _flow_output_resource_ref_handler(struct engine_node *node, void *data, > > enum ref_type ref_type) > > { > > + bool update_counter = false; > > struct ed_type_runtime_data *rt_data = > > engine_get_input_data("runtime_data", node); > > > > @@ -2208,6 +2209,7 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, > > } > > if (changed) { > > engine_set_node_state(node, EN_UPDATED); > > + update_counter = true; > > } > > } > > SSET_FOR_EACH (ref_name, updated) { > > @@ -2217,6 +2219,7 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, > > } > > if (changed) { > > engine_set_node_state(node, EN_UPDATED); > > + update_counter = true; > > } > > } > > SSET_FOR_EACH (ref_name, new) { > > @@ -2226,8 +2229,12 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, > > } > > if (changed) { > > engine_set_node_state(node, EN_UPDATED); > > + update_counter = true; > > } > > } > > + if (update_counter) { > > + node->stats.handler++; > > + } > > > > return true; > > } > > @@ -2254,7 +2261,7 @@ flow_output_physical_flow_changes_handler(struct engine_node *node, void *data) > > struct physical_ctx p_ctx; > > init_physical_ctx(node, rt_data, &p_ctx); > > > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > struct ed_type_pfc_data *pfc_data = > > engine_get_input_data("physical_flow_changes", node); > > > > @@ -2293,7 +2300,7 @@ flow_output_runtime_data_handler(struct engine_node *node, > > struct hmap *tracked_dp_bindings = &rt_data->tracked_dp_bindings; > > if (hmap_is_empty(tracked_dp_bindings)) { > > if (rt_data->local_lports_changed) { > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > } > > return true; > > } > > @@ -2325,7 +2332,7 @@ flow_output_runtime_data_handler(struct engine_node *node, > > } > > } > > > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > return true; > > } > > > > @@ -2342,7 +2349,7 @@ flow_output_sb_load_balancer_handler(struct engine_node *node, void *data) > > > > bool handled = lflow_handle_changed_lbs(&l_ctx_in, &l_ctx_out); > > > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_note_update_from_handler(node); > > return handled; > > } > > > > @@ -2681,6 +2688,10 @@ main(int argc, char *argv[]) > > > > unixctl_command_register("recompute", "", 0, 0, engine_recompute_cmd, > > NULL); > > + unixctl_command_register("inc-engine/stats", "", 0, 0, engine_dump_stats, > > + NULL); > > + unixctl_command_register("inc-engine/stats-clear", "", 0, 0, > > + engine_clear_stats, NULL); > > unixctl_command_register("lflow-cache/flush", "", 0, 0, > > lflow_cache_flush_cmd, > > &flow_output_data->pd); > > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c > > index 916dbbe39..ce3fed562 100644 > > --- a/lib/inc-proc-eng.c > > +++ b/lib/inc-proc-eng.c > > @@ -32,6 +32,7 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_eng); > > > > static bool engine_force_recompute = false; > > static bool engine_run_aborted = false; > > +static bool engine_stats_reset = false; > > static const struct engine_context *engine_context; > > > > static struct engine_node **engine_nodes; > > @@ -283,6 +284,7 @@ engine_recompute(struct engine_node *node, bool forced, bool allowed) > > if (!allowed) { > > VLOG_DBG("node: %s, recompute aborted", node->name); > > engine_set_node_state(node, EN_ABORTED); > > + node->stats.abort++; > > return; > > } > > > > @@ -383,9 +385,26 @@ engine_run(bool recompute_allowed) > > } > > } > > > > +static void > > +engine_reset_stats(void) > > +{ > > + if (!engine_stats_reset) { > > + return; > > + } > > + > > + engine_stats_reset = false; > > + for (size_t i = 0; i < engine_n_nodes; i++) { > > + struct engine_node *node = engine_nodes[i]; > > + > > + memset(&node->stats, 0, sizeof(node->stats)); > > + } > > +} > > + > > bool > > engine_need_run(void) > > { > > + engine_reset_stats(); > > + > > for (size_t i = 0; i < engine_n_nodes; i++) { > > /* Check only leaf nodes for updates. */ > > if (engine_nodes[i]->n_inputs) { > > @@ -401,3 +420,32 @@ engine_need_run(void) > > } > > return false; > > } > > + > > +void > > +engine_clear_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, > > + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED) > > +{ > > + engine_stats_reset = true; > > + unixctl_command_reply(conn, NULL); > > +} > > + > > +void > > +engine_dump_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, > > + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED) > > +{ > > + struct ds dump = DS_EMPTY_INITIALIZER; > > + > > + engine_reset_stats(); > > + > > + for (size_t i = 0; i < engine_n_nodes; i++) { > > + struct engine_node *node = engine_nodes[i]; > > + > > + ds_put_format(&dump, "%s\n", node->name); > > + ds_put_format(&dump, "\trun\t%lu", node->stats.run); > > + ds_put_format(&dump, "\tabort\t%lu", node->stats.abort); > > + ds_put_format(&dump, "\thandler\t%lu\n", node->stats.handler); > > + } > > + unixctl_command_reply(conn, ds_cstr(&dump)); > > + > > + ds_destroy(&dump); > > +} > > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h > > index 857234677..0eeca7ced 100644 > > --- a/lib/inc-proc-eng.h > > +++ b/lib/inc-proc-eng.h > > @@ -60,6 +60,8 @@ > > * against all its inputs. > > */ > > > > +#include "unixctl.h" > > + > > #define ENGINE_MAX_INPUT 256 > > #define ENGINE_MAX_OVSDB_INDEX 256 > > > > @@ -107,6 +109,12 @@ enum engine_node_state { > > EN_STATE_MAX, > > }; > > > > +struct engine_stats { > > + unsigned long run; > > + unsigned long abort; > > + unsigned long handler; > > +}; > > + > > struct engine_node { > > /* A unique name for each node. */ > > char *name; > > @@ -154,6 +162,9 @@ 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; > > }; > > > > /* Initialize the data for the engine nodes. It calls each node's > > @@ -247,6 +258,14 @@ void *engine_get_internal_data(struct engine_node *node); > > #define engine_set_node_state(node, state) \ > > engine_set_node_state_at(node, state, OVS_SOURCE_LOCATOR) > > > > +#define engine_set_note_update_from_run(node) \ > > + engine_set_node_state(node, EN_UPDATED); \ > > + node->stats.run++; > > + > > +#define engine_set_note_update_from_handler(node) \ > > + engine_set_node_state(node, EN_UPDATED); \ > > + node->stats.handler++; > > + > > struct ed_ovsdb_index { > > const char *name; > > struct ovsdb_idl_index *index; > > @@ -312,6 +331,7 @@ en_##DB_NAME##_##TBL_NAME##_run(struct engine_node *node, \ > > EN_OVSDB_GET(node); \ > > if (DB_NAME##rec_##TBL_NAME##_table_track_get_first(table)) { \ > > engine_set_node_state(node, EN_UPDATED); \ > > + node->stats.run++; \ > > return; \ > > } \ > > engine_set_node_state(node, EN_UNCHANGED); \ > > @@ -352,4 +372,10 @@ static void en_##DB_NAME##_##TBL_NAME##_cleanup(void *data OVS_UNUSED) \ > > #define ENGINE_NODE_OVS(TBL_NAME, TBL_NAME_STR) \ > > ENGINE_NODE_OVSDB(ovs, "OVS", TBL_NAME, TBL_NAME_STR); > > > > + > > +void engine_dump_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, > > + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED); > > +void engine_clear_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, > > + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED); > > + > > #endif /* lib/inc-proc-eng.h */ > > >
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 4343650fc..a937803c8 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1011,7 +1011,7 @@ en_ofctrl_is_connected_run(struct engine_node *node, void *data) ofctrl_seqno_flush(); binding_seqno_flush(); } - engine_set_node_state(node, EN_UPDATED); + engine_set_note_update_from_run(node); return; } engine_set_node_state(node, EN_UNCHANGED); @@ -1067,7 +1067,7 @@ en_addr_sets_run(struct engine_node *node, void *data) addr_sets_init(as_table, &as->addr_sets); as->change_tracked = false; - engine_set_node_state(node, EN_UPDATED); + engine_set_note_update_from_run(node); } static bool @@ -1088,7 +1088,7 @@ addr_sets_sb_address_set_handler(struct engine_node *node, void *data) if (!sset_is_empty(&as->new) || !sset_is_empty(&as->deleted) || !sset_is_empty(&as->updated)) { - engine_set_node_state(node, EN_UPDATED); + engine_set_note_update_from_handler(node); } else { engine_set_node_state(node, EN_UNCHANGED); } @@ -1147,7 +1147,7 @@ en_port_groups_run(struct engine_node *node, void *data) port_groups_init(pg_table, &pg->port_groups); pg->change_tracked = false; - engine_set_node_state(node, EN_UPDATED); + engine_set_note_update_from_run(node); } static bool @@ -1168,7 +1168,7 @@ port_groups_sb_port_group_handler(struct engine_node *node, void *data) if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) || !sset_is_empty(&pg->updated)) { - engine_set_node_state(node, EN_UPDATED); + engine_set_note_update_from_handler(node); } else { engine_set_node_state(node, EN_UNCHANGED); } @@ -1482,7 +1482,7 @@ en_runtime_data_run(struct engine_node *node, void *data) binding_run(&b_ctx_in, &b_ctx_out); - engine_set_node_state(node, EN_UPDATED); + engine_set_note_update_from_run(node); } static bool @@ -1500,7 +1500,7 @@ runtime_data_ovs_interface_handler(struct engine_node *node, void *data) } if (b_ctx_out.local_lports_changed) { - engine_set_node_state(node, EN_UPDATED); + engine_set_note_update_from_handler(node); rt_data->local_lports_changed = b_ctx_out.local_lports_changed; } @@ -1528,7 +1528,7 @@ runtime_data_sb_port_binding_handler(struct engine_node *node, void *data) if (b_ctx_out.local_lport_ids_changed || b_ctx_out.non_vif_ports_changed || !hmap_is_empty(b_ctx_out.tracked_dp_bindings)) { - engine_set_node_state(node, EN_UPDATED); + engine_set_note_update_from_handler(node); } return true; @@ -1605,7 +1605,7 @@ en_ct_zones_run(struct engine_node *node, void *data) &ct_zones_data->pending, &rt_data->ct_updated_datapaths); - engine_set_node_state(node, EN_UPDATED); + engine_set_note_update_from_run(node); } /* The data in the ct_zones node is always valid (i.e., no stale pointers). */ @@ -1639,7 +1639,7 @@ en_mff_ovn_geneve_run(struct engine_node *node, void *data) enum mf_field_id mff_ovn_geneve = ofctrl_get_mf_field_id(); if (ed_mff_ovn_geneve->mff_ovn_geneve != mff_ovn_geneve) { ed_mff_ovn_geneve->mff_ovn_geneve = mff_ovn_geneve; - engine_set_node_state(node, EN_UPDATED); + engine_set_note_update_from_run(node); return; } engine_set_node_state(node, EN_UNCHANGED); @@ -1714,7 +1714,7 @@ en_physical_flow_changes_run(struct engine_node *node, void *data) { struct ed_type_pfc_data *pfc_tdata = data; pfc_tdata->recompute_physical_flows = true; - engine_set_node_state(node, EN_UPDATED); + engine_set_note_update_from_run(node); } /* ct_zone changes are not handled incrementally but a handler is required @@ -1734,7 +1734,7 @@ physical_flow_changes_ovs_iface_handler(struct engine_node *node, void *data) { struct ed_type_pfc_data *pfc_tdata = data; pfc_tdata->ovs_ifaces_changed = true; - engine_set_node_state(node, EN_UPDATED); + engine_set_note_update_from_handler(node); return true; } @@ -2035,7 +2035,7 @@ en_flow_output_run(struct engine_node *node, void *data) physical_run(&p_ctx, &fo->flow_table); - engine_set_node_state(node, EN_UPDATED); + engine_set_note_update_from_run(node); } static bool @@ -2059,7 +2059,7 @@ flow_output_sb_logical_flow_handler(struct engine_node *node, void *data) bool handled = lflow_handle_changed_flows(&l_ctx_in, &l_ctx_out); - engine_set_node_state(node, EN_UPDATED); + engine_set_note_update_from_handler(node); return handled; } @@ -2085,7 +2085,7 @@ flow_output_sb_mac_binding_handler(struct engine_node *node, void *data) lflow_handle_changed_neighbors(sbrec_port_binding_by_name, mac_binding_table, local_datapaths, flow_table); - engine_set_node_state(node, EN_UPDATED); + engine_set_note_update_from_handler(node); return true; } @@ -2108,7 +2108,7 @@ flow_output_sb_port_binding_handler(struct engine_node *node, */ physical_handle_port_binding_changes(&p_ctx, flow_table); - engine_set_node_state(node, EN_UPDATED); + engine_set_note_update_from_handler(node); return true; } @@ -2126,7 +2126,7 @@ flow_output_sb_multicast_group_handler(struct engine_node *node, void *data) physical_handle_mc_group_changes(&p_ctx, flow_table); - engine_set_node_state(node, EN_UPDATED); + engine_set_note_update_from_handler(node); return true; } @@ -2135,6 +2135,7 @@ static bool _flow_output_resource_ref_handler(struct engine_node *node, void *data, enum ref_type ref_type) { + bool update_counter = false; struct ed_type_runtime_data *rt_data = engine_get_input_data("runtime_data", node); @@ -2208,6 +2209,7 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, } if (changed) { engine_set_node_state(node, EN_UPDATED); + update_counter = true; } } SSET_FOR_EACH (ref_name, updated) { @@ -2217,6 +2219,7 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, } if (changed) { engine_set_node_state(node, EN_UPDATED); + update_counter = true; } } SSET_FOR_EACH (ref_name, new) { @@ -2226,8 +2229,12 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, } if (changed) { engine_set_node_state(node, EN_UPDATED); + update_counter = true; } } + if (update_counter) { + node->stats.handler++; + } return true; } @@ -2254,7 +2261,7 @@ flow_output_physical_flow_changes_handler(struct engine_node *node, void *data) struct physical_ctx p_ctx; init_physical_ctx(node, rt_data, &p_ctx); - engine_set_node_state(node, EN_UPDATED); + engine_set_note_update_from_handler(node); struct ed_type_pfc_data *pfc_data = engine_get_input_data("physical_flow_changes", node); @@ -2293,7 +2300,7 @@ flow_output_runtime_data_handler(struct engine_node *node, struct hmap *tracked_dp_bindings = &rt_data->tracked_dp_bindings; if (hmap_is_empty(tracked_dp_bindings)) { if (rt_data->local_lports_changed) { - engine_set_node_state(node, EN_UPDATED); + engine_set_note_update_from_handler(node); } return true; } @@ -2325,7 +2332,7 @@ flow_output_runtime_data_handler(struct engine_node *node, } } - engine_set_node_state(node, EN_UPDATED); + engine_set_note_update_from_handler(node); return true; } @@ -2342,7 +2349,7 @@ flow_output_sb_load_balancer_handler(struct engine_node *node, void *data) bool handled = lflow_handle_changed_lbs(&l_ctx_in, &l_ctx_out); - engine_set_node_state(node, EN_UPDATED); + engine_set_note_update_from_handler(node); return handled; } @@ -2681,6 +2688,10 @@ main(int argc, char *argv[]) unixctl_command_register("recompute", "", 0, 0, engine_recompute_cmd, NULL); + unixctl_command_register("inc-engine/stats", "", 0, 0, engine_dump_stats, + NULL); + unixctl_command_register("inc-engine/stats-clear", "", 0, 0, + engine_clear_stats, NULL); unixctl_command_register("lflow-cache/flush", "", 0, 0, lflow_cache_flush_cmd, &flow_output_data->pd); diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c index 916dbbe39..ce3fed562 100644 --- a/lib/inc-proc-eng.c +++ b/lib/inc-proc-eng.c @@ -32,6 +32,7 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_eng); static bool engine_force_recompute = false; static bool engine_run_aborted = false; +static bool engine_stats_reset = false; static const struct engine_context *engine_context; static struct engine_node **engine_nodes; @@ -283,6 +284,7 @@ engine_recompute(struct engine_node *node, bool forced, bool allowed) if (!allowed) { VLOG_DBG("node: %s, recompute aborted", node->name); engine_set_node_state(node, EN_ABORTED); + node->stats.abort++; return; } @@ -383,9 +385,26 @@ engine_run(bool recompute_allowed) } } +static void +engine_reset_stats(void) +{ + if (!engine_stats_reset) { + return; + } + + engine_stats_reset = false; + for (size_t i = 0; i < engine_n_nodes; i++) { + struct engine_node *node = engine_nodes[i]; + + memset(&node->stats, 0, sizeof(node->stats)); + } +} + bool engine_need_run(void) { + engine_reset_stats(); + for (size_t i = 0; i < engine_n_nodes; i++) { /* Check only leaf nodes for updates. */ if (engine_nodes[i]->n_inputs) { @@ -401,3 +420,32 @@ engine_need_run(void) } return false; } + +void +engine_clear_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED) +{ + engine_stats_reset = true; + unixctl_command_reply(conn, NULL); +} + +void +engine_dump_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED) +{ + struct ds dump = DS_EMPTY_INITIALIZER; + + engine_reset_stats(); + + for (size_t i = 0; i < engine_n_nodes; i++) { + struct engine_node *node = engine_nodes[i]; + + ds_put_format(&dump, "%s\n", node->name); + ds_put_format(&dump, "\trun\t%lu", node->stats.run); + ds_put_format(&dump, "\tabort\t%lu", node->stats.abort); + ds_put_format(&dump, "\thandler\t%lu\n", node->stats.handler); + } + unixctl_command_reply(conn, ds_cstr(&dump)); + + ds_destroy(&dump); +} diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h index 857234677..0eeca7ced 100644 --- a/lib/inc-proc-eng.h +++ b/lib/inc-proc-eng.h @@ -60,6 +60,8 @@ * against all its inputs. */ +#include "unixctl.h" + #define ENGINE_MAX_INPUT 256 #define ENGINE_MAX_OVSDB_INDEX 256 @@ -107,6 +109,12 @@ enum engine_node_state { EN_STATE_MAX, }; +struct engine_stats { + unsigned long run; + unsigned long abort; + unsigned long handler; +}; + struct engine_node { /* A unique name for each node. */ char *name; @@ -154,6 +162,9 @@ 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; }; /* Initialize the data for the engine nodes. It calls each node's @@ -247,6 +258,14 @@ void *engine_get_internal_data(struct engine_node *node); #define engine_set_node_state(node, state) \ engine_set_node_state_at(node, state, OVS_SOURCE_LOCATOR) +#define engine_set_note_update_from_run(node) \ + engine_set_node_state(node, EN_UPDATED); \ + node->stats.run++; + +#define engine_set_note_update_from_handler(node) \ + engine_set_node_state(node, EN_UPDATED); \ + node->stats.handler++; + struct ed_ovsdb_index { const char *name; struct ovsdb_idl_index *index; @@ -312,6 +331,7 @@ en_##DB_NAME##_##TBL_NAME##_run(struct engine_node *node, \ EN_OVSDB_GET(node); \ if (DB_NAME##rec_##TBL_NAME##_table_track_get_first(table)) { \ engine_set_node_state(node, EN_UPDATED); \ + node->stats.run++; \ return; \ } \ engine_set_node_state(node, EN_UNCHANGED); \ @@ -352,4 +372,10 @@ static void en_##DB_NAME##_##TBL_NAME##_cleanup(void *data OVS_UNUSED) \ #define ENGINE_NODE_OVS(TBL_NAME, TBL_NAME_STR) \ ENGINE_NODE_OVSDB(ovs, "OVS", TBL_NAME, TBL_NAME_STR); + +void engine_dump_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED); +void engine_clear_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED); + #endif /* lib/inc-proc-eng.h */
Introduce inc-engine/stats ovs-applctl command in order to dump ovn-controller incremental processing engine statistics. So far for each node a counter for run, abort and engine_handler have been added. Counters are incremented when the node move to "updated" state. In order to dump I-P stats we can can use the following commands: $ovs-appctl -t ovn-controller inc-engine/stats SB_address_set run 1 abort 0 handler 0 addr_sets run 2 abort 1 handler 0 SB_port_group run 0 abort 0 handler 0 port_groups run 2 abort 0 handler 0 ofctrl_is_connected run 1 abort 0 handler 0 OVS_open_vswitch run 0 abort 0 handler 0 OVS_bridge run 0 abort 0 handler 0 OVS_qos run 0 abort 0 handler 0 SB_chassis run 1 abort 0 handler 0 .... flow_output run 2 abort 0 handler 34 Morover introduce the inc-engine/stats-clear command to reset engine statistics $ovs-appctl -t ovn-controller inc-engine/stats-clear Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- Changes since v2: - introduce inc-engine/stats and inc-engine/stats-clear commands and drop COVERAGE_* dependency Changes since v1: - drop handler counters and add global abort counter - improve documentation and naming scheme - introduce engine_set_node_updated utility macro --- controller/ovn-controller.c | 53 ++++++++++++++++++++++--------------- lib/inc-proc-eng.c | 48 +++++++++++++++++++++++++++++++++ lib/inc-proc-eng.h | 26 ++++++++++++++++++ 3 files changed, 106 insertions(+), 21 deletions(-)