| Message ID | 9d188390f58ec458e71abae030763be6780d3b4a.1613590188.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] controller: introduce coverage_counters for ovn-controller incremental processing | expand |
On 17/02/2021 19:33, Lorenzo Bianconi wrote: Should the subject have an underscore? 'coverage_counters' > 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.bianconi@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? > + > #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 */ >
> 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.bianconi@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 */ > > >
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); + #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 */
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.bianconi@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(-)