[ovs-dev] [PATCH v3 ovn] controller: introduce coverage counters for ovn-controller incremental processing
Lorenzo Bianconi
lorenzo.bianconi at redhat.com
Mon Feb 22 12:03:20 UTC 2021
> 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 at 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 */
> >
>
More information about the dev
mailing list