> On 17/02/2021 19:33, Lorenzo Bianconi wrote: > > Should the subject have an underscore? 'coverage_counters'
ops, sorry..I forgot to fix it. > > > In order to help understanding system behaviour for debugging purpose, > > introduce coverage counters for run handlers of ovn-controller > > I-P engine nodes. Moreover add a global counter for engine abort. > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1890902 > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com> > > --- > > 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 | 39 +++++++++++++++++++++++++++---------- > > lib/inc-proc-eng.c | 5 +++++ > > lib/inc-proc-eng.h | 12 +++++++++++- > > 3 files changed, 45 insertions(+), 11 deletions(-) > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 4343650fc..42eed9ebd 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -69,6 +69,7 @@ > > #include "stopwatch.h" > > #include "lib/inc-proc-eng.h" > > #include "hmapx.h" > > +#include "coverage.h" > > > > VLOG_DEFINE_THIS_MODULE(main); > > > > @@ -85,6 +86,18 @@ static unixctl_cb_func lflow_cache_flush_cmd; > > static unixctl_cb_func lflow_cache_show_stats_cmd; > > static unixctl_cb_func debug_delay_nb_cfg_report; > > > > +/* Coverage counters for run handlers of OVN controller > > + * incremental processing nodes > > + */ > > +ENGINE_RUN_COVERAGE_DEFINE(flow_output); > > +ENGINE_RUN_COVERAGE_DEFINE(runtime_data); > > +ENGINE_RUN_COVERAGE_DEFINE(addr_sets); > > +ENGINE_RUN_COVERAGE_DEFINE(port_groups); > > +ENGINE_RUN_COVERAGE_DEFINE(ct_zones); > > +ENGINE_RUN_COVERAGE_DEFINE(mff_ovn_geneve); > > +ENGINE_RUN_COVERAGE_DEFINE(ofctrl_is_connected); > > +ENGINE_RUN_COVERAGE_DEFINE(physical_flow_changes); > > > I would like these counters to be part of the framework (i.e. > inc-proc-eng.c/h) rather than being exposed out. The reasoning is that > we shouldn't rely on a developer to modify them correctly when updating > the _handler() and the _run() functions as it is too error-prone IMHO. > For example, if a _run() function exits early and also sets a node to > updated or aborts a node, will the developer/reviewer remember to update > the corresponding counter? Also, if we need to make changes to the > counters, it would be preferable to make the changes in one location. > > I realize that this causes issues for per-node counters as currently > implemented: inc-proc-eng.c has no idea about which nodes are which at > compilation time and COVERAGE_INC() is a macro which requires templating > the name of the node at compilation time. > > For this reason, I suggest the following: > > * Add counters to 'struct engine_node' and/or 'struct engine_context' > * Create an ovs-appctl command, something like `ovs-appctl-ctl -t > ovs-controller inc-proc-eng/stats-show` and `ovs-appctl-ctl -t > ovs-controller inc-proc-eng/stats-clear` that will show/clear these > stats to the user > > This follows a common pattern in OVS/OVN code. e.g. > https://github.com/openvswitch/ovs/blob/master/Documentation/topics/dpdk/pmd.rst#pmd-thread-statistics. > > The next question is what counters to expose. As I think it should be > easier to expose counters using the `appctl` above, I would suggest > exposing as much as you can. However, you could also get feedback from > those in the community who have more operational experience with OVN. > > Here are some suggestions: > > * Total number of engine runs > * Total number of engine aborts > * Total number of engine recomputes > * Number of runs for each node > * Number of computes for each node > * Number of recomputes for each node > * Number of transitions to ABORT for each node > * Number of transitions to UNCHANGED for each node > * Number of transitions to UPDATED for each node > > What do you think? yes, I like the idea...avoiding COVERAGE_* macros will allow a more manageable code. I will work on a PoC, thx :) Regards, Lorenzo > > > + > > #define DEFAULT_BRIDGE_NAME "br-int" > > #define DEFAULT_PROBE_INTERVAL_MSEC 5000 > > #define OFCTRL_DEFAULT_PROBE_INTERVAL_SEC 0 > > @@ -955,6 +968,10 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl) > > SB_NODE(dns, "dns") \ > > SB_NODE(load_balancer, "load_balancer") > > > > +#define SB_NODE(NAME, NAME_STR) ENGINE_RUN_COVERAGE_DEFINE(sb_##NAME); > > + SB_NODES > > +#undef SB_NODE > > + > > enum sb_engine_node { > > #define SB_NODE(NAME, NAME_STR) SB_##NAME, > > SB_NODES > > @@ -972,6 +989,10 @@ enum sb_engine_node { > > OVS_NODE(interface, "interface") \ > > OVS_NODE(qos, "qos") > > > > +#define OVS_NODE(NAME, NAME_STR) ENGINE_RUN_COVERAGE_DEFINE(ovs_##NAME); > > + OVS_NODES > > +#undef OVS_NODE > > + > > enum ovs_engine_node { > > #define OVS_NODE(NAME, NAME_STR) OVS_##NAME, > > OVS_NODES > > @@ -1011,7 +1032,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_node_updated(node, ofctrl_is_connected); > > return; > > } > > engine_set_node_state(node, EN_UNCHANGED); > > @@ -1067,7 +1088,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_node_updated(node, addr_sets); > > } > > > > static bool > > @@ -1147,7 +1168,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_node_updated(node, port_groups); > > } > > > > static bool > > @@ -1482,7 +1503,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_node_updated(node, runtime_data); > > } > > > > static bool > > @@ -1604,8 +1625,7 @@ en_ct_zones_run(struct engine_node *node, void *data) > > &ct_zones_data->current, ct_zones_data->bitmap, > > &ct_zones_data->pending, > > &rt_data->ct_updated_datapaths); > > > > - > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_node_updated(node, ct_zones); > > } > > > > /* The data in the ct_zones node is always valid (i.e., no stale > > pointers). */ > > @@ -1639,7 +1659,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_node_updated(node, mff_ovn_geneve); > > return; > > } > > engine_set_node_state(node, EN_UNCHANGED); > > @@ -1714,7 +1734,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_node_updated(node, physical_flow_changes); > > } > > > > /* ct_zone changes are not handled incrementally but a handler is required > > @@ -2034,8 +2054,7 @@ en_flow_output_run(struct engine_node *node, void > > *data) > > init_physical_ctx(node, rt_data, &p_ctx); > > > > physical_run(&p_ctx, &fo->flow_table); > > - > > - engine_set_node_state(node, EN_UPDATED); > > + engine_set_node_updated(node, flow_output); > > } > > > > static bool > > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c > > index 916dbbe39..a19ad68f8 100644 > > --- a/lib/inc-proc-eng.c > > +++ b/lib/inc-proc-eng.c > > @@ -27,6 +27,7 @@ > > #include "openvswitch/hmap.h" > > #include "openvswitch/vlog.h" > > #include "inc-proc-eng.h" > > +#include "coverage.h" > > > > VLOG_DEFINE_THIS_MODULE(inc_proc_eng); > > > > @@ -44,6 +45,9 @@ static const char *engine_node_state_name[EN_STATE_MAX] = > > { > > [EN_ABORTED] = "Aborted", > > }; > > > > +/* global counter for engine abort */ > > +COVERAGE_DEFINE(engine_abort); > > + > > void > > engine_set_force_recompute(bool val) > > { > > @@ -282,6 +286,7 @@ engine_recompute(struct engine_node *node, bool forced, > > bool allowed) > > > > if (!allowed) { > > VLOG_DBG("node: %s, recompute aborted", node->name); > > + COVERAGE_INC(engine_abort); > > engine_set_node_state(node, EN_ABORTED); > > return; > > } > > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h > > index 857234677..6067a88c1 100644 > > --- a/lib/inc-proc-eng.h > > +++ b/lib/inc-proc-eng.h > > @@ -247,6 +247,10 @@ 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_node_updated(node, NAME) \ > > + engine_set_node_state(node, EN_UPDATED); \ > > + ENGINE_RUN_COVERAGE_INC(NAME); > > + > > struct ed_ovsdb_index { > > const char *name; > > struct ovsdb_idl_index *index; > > @@ -311,7 +315,7 @@ en_##DB_NAME##_##TBL_NAME##_run(struct engine_node > > *node, \ > > const struct DB_NAME##rec_##TBL_NAME##_table *table = \ > > EN_OVSDB_GET(node); \ > > if (DB_NAME##rec_##TBL_NAME##_table_track_get_first(table)) { \ > > - engine_set_node_state(node, EN_UPDATED); \ > > + engine_set_node_updated(node, DB_NAME##_##TBL_NAME); \ > > return; \ > > } \ > > engine_set_node_state(node, EN_UNCHANGED); \ > > @@ -352,4 +356,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); > > > > +#define ENGINE_RUN_COVERAGE_DEFINE(NAME) \ > > + COVERAGE_DEFINE(en_##NAME##_run); > > + > > +#define ENGINE_RUN_COVERAGE_INC(NAME) \ > > + COVERAGE_INC(en_##NAME##_run); > > + > > #endif /* lib/inc-proc-eng.h */ > > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
