[ovs-dev,v3] controller: introduce coverage counters for ovn-controller incremental processing

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(-)

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 */
> > 
>