> On 23/02/2021 14:52, 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:
> >
> > $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
> >
> > 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));
> > + }
> > + 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);
> > + }
> > + 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;
>
> node->run() itself could cause an abort. You should move this counter
> into engine_run() and check the state of each node after
> engine_node_run() has been invoked. In this way, you ensure you catch
> all cases.
I can see the point now, the approach you suggested is more robust for future
changes. I will fix it.
> > }
> >
> > /* Run the node handler which might change state. */
> > node->run(node, node->data);
> > + node->stats.recompute++;
>
> There are two other places that we node->run() in this file. One is a
> the state of engine_run_node(). This is for root nodes. I think we
> should increment there as well. The other is engine_need_run() - I am
> not sure about this but to me, it looks like it is doing a recompute.
>
> We will need to increment this there as well.
ack, I will fix them.
Regards,
Lorenzo
>
> > }
> >
> > /* 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;
> > +};
> > +
> > 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 */
> > + 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);
> >
> > +
> > #endif /* lib/inc-proc-eng.h */
> >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
