On 2/23/21 3:52 PM, Lorenzo Bianconi wrote: > Introduce inc-engine/show-stats ovs-appctl 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:
Hi. Thanks for working on this! Few style/doc comments inline. Best regards, Ilya Maximets. > > $ovs-appctl -t ovn-controller inc-engine/show-stats > SB_address_set > recompute 1 abort 0 compute 0 > addr_sets > recompute 2 abort 1 compute 0 > SB_port_group > recompute 0 abort 0 compute 0 > port_groups > recompute 2 abort 0 compute 0 > ofctrl_is_connected > recompute 1 abort 0 compute 0 > OVS_open_vswitch > recompute 0 abort 0 compute 0 > OVS_bridge > recompute 0 abort 0 compute 0 > OVS_qos > recompute 0 abort 0 compute 0 > SB_chassis > recompute 1 abort 0 compute 0 > .... > > flow_output > recompute 2 abort 0 compute 34 > > Moreover introduce the inc-engine/clear-stats command to reset engine > statistics > > $ovs-appctl -t ovn-controller inc-engine/clear-stats These commends should be documented in controller/ovn-controller.8.xml and, probably, mentioned in a NEWS file. > > Signed-off-by: Lorenzo Bianconi <[email protected]> > --- > Changes since v4: > - rename handlers in inc-engine/show-stats and inc-engine/clear-stats > - move all the code in lib/inc-proc-eng.{c,h} > - instad of run and change_handler, track node recompute and node compute > Changes since v3: > - drop engine_set_note_update_from_run/engine_set_note_update_from_handler > macros and move stats code in lib/inc-proc-eng.c > - fix commit log > 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 > --- > lib/inc-proc-eng.c | 41 +++++++++++++++++++++++++++++++++++++++++ > lib/inc-proc-eng.h | 10 ++++++++++ > 2 files changed, 51 insertions(+) > > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c > index 916dbbe39..6afe692bb 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 "unixctl.h" > > VLOG_DEFINE_THIS_MODULE(inc_proc_eng); > > @@ -102,6 +103,37 @@ engine_get_nodes(struct engine_node *node, size_t > *n_count) > return engine_topo_sort(node, NULL, n_count, &n_size); > } > > +static void > +engine_clear_stats(struct unixctl_conn *conn, int argc OVS_UNUSED, > + const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED) > +{ > + for (size_t i = 0; i < engine_n_nodes; i++) { > + struct engine_node *node = engine_nodes[i]; > + > + memset(&node->stats, 0, sizeof(node->stats)); Please, don't parenthesize the argument of a sizeof. > + } > + unixctl_command_reply(conn, NULL); > +} > + > +static 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; > + > + 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, "\trecompute\t%lu", node->stats.recompute); > + ds_put_format(&dump, "\tcompute\t%lu", node->stats.compute); > + ds_put_format(&dump, "\tabort\t%lu\n", node->stats.abort); Please, don't use tabs in the output. OVS and OVN (as derived from OVS) uses spaces for indentation including command outputs. For the numbers there will be fixed-size fields to make them aligned. > + } > + unixctl_command_reply(conn, ds_cstr(&dump)); > + > + ds_destroy(&dump); > +} > + > void > engine_init(struct engine_node *node, struct engine_arg *arg) > { > @@ -115,6 +147,11 @@ engine_init(struct engine_node *node, struct engine_arg > *arg) > engine_nodes[i]->data = NULL; > } > } > + > + unixctl_command_register("inc-engine/show-stats", "", 0, 0, > + engine_dump_stats, NULL); > + unixctl_command_register("inc-engine/clear-stats", "", 0, 0, > + engine_clear_stats, NULL); > } > > void > @@ -283,11 +320,13 @@ 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; > } > > /* Run the node handler which might change state. */ > node->run(node, node->data); > + node->stats.recompute++; > } > > /* Return true if the node could be computed, false otherwise. */ > @@ -312,6 +351,8 @@ engine_compute(struct engine_node *node, bool > recompute_allowed) > } > } > } > + node->stats.compute++; > + > return true; > } > > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h > index 857234677..31456e435 100644 > --- a/lib/inc-proc-eng.h > +++ b/lib/inc-proc-eng.h > @@ -107,6 +107,12 @@ enum engine_node_state { > EN_STATE_MAX, > }; > > +struct engine_stats { > + unsigned long recompute; > + unsigned long compute; > + unsigned long abort; Is 'unsigned long' enough? Just as shy as 200 updates per second will overflow it in less than a year on a 32bit system. :) unit64_t is usually a better option for various counters and more portable since the size is platform independent. > +}; > + > struct engine_node { > /* A unique name for each node. */ > char *name; > @@ -154,6 +160,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 */ Period in the end of a comment. > + struct engine_stats stats; > }; > > /* Initialize the data for the engine nodes. It calls each node's > @@ -352,4 +361,5 @@ 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); > > + This looks unrelated. > #endif /* lib/inc-proc-eng.h */ > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
