I pushed this to main and branch-26.03. On Thu, Jul 30, 2026 at 1:45 PM Mark Michelson <[email protected]> wrote: > > 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
