Thanks Dumitru! Acked-by: Mark Michelson <[email protected]>
On Thu, Jul 30, 2026 at 12:34 PM Dumitru Ceara <[email protected]> wrote: > > Commit 7d5513012b4e added stopwatch_start()/stopwatch_stop() calls > around every engine node recompute and change handler invocation. > With ~320 engine stopwatches, each iteration incurs hundreds of > heap allocations, mutex operations, and syscalls (clock_gettime > and write on a pipe fd). This causes significant CPU increase in > scaled environments. > > Guard the stopwatch_start()/stopwatch_stop() calls behind a new > per-node flag that defaults to false. Add two appctl commands, > "inc-engine/enable-stopwatch" and "inc-engine/disable-stopwatch", > to toggle the flag at runtime. Both commands accept an optional > node name argument for per-node granularity; without an argument > all nodes are affected. The one-time stopwatch_create() calls > in engine_init() and engine_add_input_impl() are left unchanged > because they have zero per-iteration cost. > > Fixes: 7d5513012b4e ("inc-proc-eng: Build stopwatches into every incremental > node.") > Reported-at: https://redhat.atlassian.net/browse/FDP-4170 > Assisted-by: Claude Opus 4.6, Claude Code > Signed-off-by: Dumitru Ceara <[email protected]> > --- > controller/ovn-controller.8.xml | 20 +++++++ > lib/inc-proc-eng.c | 60 +++++++++++++++++++-- > lib/inc-proc-eng.h | 3 ++ > northd/ovn-northd.8.xml | 20 +++++++ > tests/ovn-northd.at | 96 +++++++++++++++++++++++++++++++++ > 5 files changed, 195 insertions(+), 4 deletions(-) > > diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml > index dc4d106e59..e355046901 100644 > --- a/controller/ovn-controller.8.xml > +++ b/controller/ovn-controller.8.xml > @@ -896,6 +896,26 @@ > <dd> > Reset <code>ovn-controller</code> engine counters. > </dd> > + > + <dt><code>inc-engine/enable-stopwatch</code> [<var>node</var>]</dt> > + <dd> > + Enables the per-node and per-handler stopwatches in the > + incremental processing engine. If <var>node</var> is specified, > + only that node's stopwatches are enabled; otherwise all nodes > + are affected. While enabled, every engine node recompute and > + change handler invocation is timed and the results can be > + viewed with the <code>stopwatch/show</code> command. > + Stopwatches are disabled by default because they add measurable > + CPU overhead. > + </dd> > + > + <dt><code>inc-engine/disable-stopwatch</code> [<var>node</var>]</dt> > + <dd> > + Disables the per-node and per-handler stopwatches in the > + incremental processing engine. If <var>node</var> is specified, > + only that node's stopwatches are disabled; otherwise all nodes > + are affected. This is the default state. > + </dd> > </dl> > </p> > > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c > index a4b6c8cde3..bcb0848c3b 100644 > --- a/lib/inc-proc-eng.c > +++ b/lib/inc-proc-eng.c > @@ -220,6 +220,46 @@ engine_list_stopwatch_cmd(struct unixctl_conn *conn, int > argc OVS_UNUSED, > ds_destroy(&output_str); > } > > +static void > +engine_set_stopwatch_cmd(struct unixctl_conn *conn, int argc, > + const char *argv[], bool enabled) > +{ > + const char *node_name = argc > 1 ? argv[1] : NULL; > + > + struct engine_node *node; > + bool found = false; > + VECTOR_FOR_EACH (&engine_nodes, node) { > + if (node_name && strcmp(node->name, node_name)) { > + continue; > + } > + node->stopwatch_enabled = enabled; > + found = true; > + if (node_name) { > + break; > + } > + } > + > + if (node_name && !found) { > + unixctl_command_reply_error(conn, "node not found"); > + return; > + } > + unixctl_command_reply(conn, NULL); > +} > + > +static void > +engine_enable_stopwatches_cmd(struct unixctl_conn *conn, int argc, > + const char *argv[], void *arg OVS_UNUSED) > +{ > + engine_set_stopwatch_cmd(conn, argc, argv, true); > +} > + > +static void > +engine_disable_stopwatches_cmd(struct unixctl_conn *conn, int argc, > + const char *argv[], void *arg OVS_UNUSED) > +{ > + engine_set_stopwatch_cmd(conn, argc, argv, false); > +} > + > static void > engine_get_compute_failure_info(struct engine_node *node) > { > @@ -256,6 +296,10 @@ engine_init(struct engine_node *node, struct engine_arg > *arg) > engine_set_log_timeout_cmd, NULL); > unixctl_command_register("inc-engine/list-stopwatches", "", 0, 1, > engine_list_stopwatch_cmd, NULL); > + unixctl_command_register("inc-engine/enable-stopwatch", "[node]", 0, 1, > + engine_enable_stopwatches_cmd, NULL); > + unixctl_command_register("inc-engine/disable-stopwatch", "[node]", 0, 1, > + engine_disable_stopwatches_cmd, NULL); > } > > void > @@ -455,9 +499,13 @@ static enum engine_node_state > run_recompute_callback(struct engine_node *node) > { > enum engine_node_state ret; > - stopwatch_start(node->name, time_msec()); > + if (node->stopwatch_enabled) { > + stopwatch_start(node->name, time_msec()); > + } > ret = node->run(node, node->data); > - stopwatch_stop(node->name, time_msec()); > + if (node->stopwatch_enabled) { > + stopwatch_stop(node->name, time_msec()); > + } > return ret; > } > > @@ -465,9 +513,13 @@ static enum engine_input_handler_result > run_change_handler(struct engine_node *node, struct engine_node_input *input) > { > enum engine_input_handler_result ret; > - stopwatch_start(input->change_handler_name, time_msec()); > + if (node->stopwatch_enabled) { > + stopwatch_start(input->change_handler_name, time_msec()); > + } > ret = input->change_handler(node, node->data); > - stopwatch_stop(input->change_handler_name, time_msec()); > + if (node->stopwatch_enabled) { > + stopwatch_stop(input->change_handler_name, time_msec()); > + } > return ret; > } > > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h > index 1cb2466b23..ece33e1beb 100644 > --- a/lib/inc-proc-eng.h > +++ b/lib/inc-proc-eng.h > @@ -282,6 +282,9 @@ struct engine_node { > > /* Indication if the node writes to SB DB. */ > bool sb_write; > + > + /* Whether stopwatches are enabled for this node. */ > + bool stopwatch_enabled; > }; > > /* Initialize the data for the engine nodes. It calls each node's > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > index a8db49f659..c2670601a7 100644 > --- a/northd/ovn-northd.8.xml > +++ b/northd/ovn-northd.8.xml > @@ -257,6 +257,26 @@ > node are listed. > </dd> > > + <dt><code>inc-engine/enable-stopwatch</code> [<var>node</var>]</dt> > + <dd> > + Enables the per-node and per-handler stopwatches in the > + incremental processing engine. If <var>node</var> is specified, > + only that node's stopwatches are enabled; otherwise all nodes > + are affected. While enabled, every engine node recompute and > + change handler invocation is timed and the results can be > + viewed with the <code>stopwatch/show</code> command. > + Stopwatches are disabled by default because they add measurable > + CPU overhead. > + </dd> > + > + <dt><code>inc-engine/disable-stopwatch</code> [<var>node</var>]</dt> > + <dd> > + Disables the per-node and per-handler stopwatches in the > + incremental processing engine. If <var>node</var> is specified, > + only that node's stopwatches are disabled; otherwise all nodes > + are affected. This is the default state. > + </dd> > + > </dl> > </p> > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 484642b7a6..c58f731e90 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -21760,3 +21760,99 @@ ct_next(ct_state=new|trk) { > OVN_CLEANUP_NORTHD > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD_NO_HV([ > +AT_SETUP([ovn-northd - engine stopwatches enable/disable]) > +AT_KEYWORDS([ovn]) > +ovn_start > + > +dnl By default, engine stopwatches are disabled. Trigger a recompute to > +dnl ensure the engine runs, then verify that no samples were collected. > +check as northd ovn-appctl -t ovn-northd inc-engine/recompute > +check ovn-nbctl --wait=sb sync > +AT_CHECK([as northd ovn-appctl -t ovn-northd stopwatch/show northd], [0], > [dnl > +Statistics for 'northd' > + Total samples: 0 > + Maximum: 0 msec > + Minimum: 0 msec > + 95th percentile: 0.000000 msec > + Short term average: 0.000000 msec > + Long term average: 0.000000 msec > +]) > + > +dnl Enable stopwatches only for the "northd" node. > +check as northd ovn-appctl -t ovn-northd inc-engine/enable-stopwatch northd > +check as northd ovn-appctl -t ovn-northd inc-engine/recompute > +check ovn-nbctl --wait=sb sync > + > +dnl The "northd" stopwatch should have collected samples. > +OVS_WAIT_UNTIL([ > + stats=$(as northd ovn-appctl -t ovn-northd stopwatch/show northd) > + echo "$stats" | grep -q 'Total samples: [[1-9]]' > +]) > + > +dnl The "lr_nat" stopwatch should still have no samples. > +AT_CHECK([as northd ovn-appctl -t ovn-northd stopwatch/show lr_nat], [0], > [dnl > +Statistics for 'lr_nat' > + Total samples: 0 > + Maximum: 0 msec > + Minimum: 0 msec > + 95th percentile: 0.000000 msec > + Short term average: 0.000000 msec > + Long term average: 0.000000 msec > +]) > + > +dnl Enable stopwatches globally (no argument). > +check as northd ovn-appctl -t ovn-northd inc-engine/enable-stopwatch > +check as northd ovn-appctl -t ovn-northd stopwatch/reset > +check as northd ovn-appctl -t ovn-northd inc-engine/recompute > +check ovn-nbctl --wait=sb sync > + > +dnl Now "lr_nat" should also have collected samples. > +OVS_WAIT_UNTIL([ > + stats=$(as northd ovn-appctl -t ovn-northd stopwatch/show lr_nat) > + echo "$stats" | grep -q 'Total samples: [[1-9]]' > +]) > + > +dnl Disable stopwatches only for the "northd" node. > +check as northd ovn-appctl -t ovn-northd inc-engine/disable-stopwatch northd > +check as northd ovn-appctl -t ovn-northd stopwatch/reset > +check as northd ovn-appctl -t ovn-northd inc-engine/recompute > +check ovn-nbctl --wait=sb sync > + > +dnl Verify "northd" has no new samples but "lr_nat" still collects. > +AT_CHECK([as northd ovn-appctl -t ovn-northd stopwatch/show northd], [0], > [dnl > +Statistics for 'northd' > + Total samples: 0 > + Maximum: 0 msec > + Minimum: 0 msec > + 95th percentile: 0.000000 msec > + Short term average: 0.000000 msec > + Long term average: 0.000000 msec > +]) > +OVS_WAIT_UNTIL([ > + stats=$(as northd ovn-appctl -t ovn-northd stopwatch/show lr_nat) > + echo "$stats" | grep -q 'Total samples: [[1-9]]' > +]) > + > +dnl Disable stopwatches globally and verify no samples are collected. > +check as northd ovn-appctl -t ovn-northd inc-engine/disable-stopwatch > +check as northd ovn-appctl -t ovn-northd stopwatch/reset > +check as northd ovn-appctl -t ovn-northd inc-engine/recompute > +check ovn-nbctl --wait=sb sync > +AT_CHECK([as northd ovn-appctl -t ovn-northd stopwatch/show northd], [0], > [dnl > +Statistics for 'northd' > + Total samples: 0 > + Maximum: 0 msec > + Minimum: 0 msec > + 95th percentile: 0.000000 msec > + Short term average: 0.000000 msec > + Long term average: 0.000000 msec > +]) > + > +dnl Error case: non-existent node name. > +AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/enable-stopwatch > nonexistent_node], [2], [], [ignore]) > + > +OVN_CLEANUP_NORTHD > +AT_CLEANUP > +]) > -- > 2.54.0 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
