On Thu, Apr 10, 2025 at 4:25 AM Mark Michelson via dev <
ovs-dev@openvswitch.org> wrote:
> On 4/9/25 22:12, Mark Michelson via dev wrote:
> > The previous two commits removed all instances where
> > engine_set_node_state() was called outside of inc-proc-eng.c. State
> > changes are all handled by the incremental engine now, so there is no
> > need to expose the state change function in its header.
> >
> > There is a downside to this, which is that we lose visibility into where
> > exactly the state change was made. Debug logs previously pinpointed the
> > line and file where the state change was being made. This is not as
> > useful since state changes now only happen on a few different lines in
> > inc-proc-eng.c.
> >
> > This change introduces a "reason" parameter for engine_set_node_state()
> > so that additional details can be passed in. This way, the debug message
> > can still contain useful information about the "why" behind the state
> > change, even if it can't be traced to a specific line within a node's
> > run() or an input's change_handler() callbacks.
> >
> > Signed-off-by: Mark Michelson <mmich...@redhat.com>
> > ---
>
Hi Mark,
I have one small comment down below.
> lib/inc-proc-eng.c | 41 ++++++++++++++++++++++++++---------------
> > lib/inc-proc-eng.h | 8 --------
> > 2 files changed, 26 insertions(+), 23 deletions(-)
> >
> > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c
> > index 953023bb1..c301f6a69 100644
> > --- a/lib/inc-proc-eng.c
> > +++ b/lib/inc-proc-eng.c
> > @@ -308,19 +308,25 @@ engine_ovsdb_node_add_index(struct engine_node
> *node, const char *name,
> > ed->n_indexes ++;
> > }
> >
> > -void
> > -engine_set_node_state_at(struct engine_node *node,
> > - enum engine_node_state state,
> > - const char *where)
> > +static void
> > +engine_set_node_state(struct engine_node *node,
> > + enum engine_node_state state,
> > + const char *reason_fmt, ...)
> > {
> > if (node->state == state) {
> > return;
> > }
> >
> > - VLOG_DBG("%s: node: %s, old_state %s, new_state %s",
> > - where, node->name,
> > - engine_node_state_name[node->state],
> > - engine_node_state_name[state]);
> > + if (VLOG_IS_DBG_ENABLED()) {
> > + va_list args;
> > + va_start(args, reason_fmt);
> > + char *reason = xvasprintf(reason_fmt, args);
> > + VLOG_DBG("node: %s, old_state %s, new_state %s, reason: %s",
> > + reason, node->name,
> > + engine_node_state_name[node->state],
> > + engine_node_state_name[state]);
> > + va_end(args);
>
> I just spotted that we're leaking "reason" here. I can correct this in
> the next version, but I will wait for additional comments before posting
> it.
>
> > + }
> >
> > node->state = state;
> > }
> > @@ -392,7 +398,7 @@ engine_init_run(void)
> > {
> > VLOG_DBG("Initializing new run");
> > for (size_t i = 0; i < engine_n_nodes; i++) {
> > - engine_set_node_state(engine_nodes[i], EN_STALE);
> > + engine_set_node_state(engine_nodes[i], EN_STALE,
> "engine_init_run");
>
The "format" of reasons should be unified. Most of them start with lower
case and
are without a dot, I don't have a strong preference we should just stick to
one style.
>
> > if (engine_nodes[i]->clear_tracked_data) {
> > engine_nodes[i]->clear_tracked_data(engine_nodes[i]->data);
> > @@ -416,7 +422,7 @@ engine_recompute(struct engine_node *node, bool
> allowed,
> >
> > if (!allowed) {
> > VLOG_DBG("node: %s, recompute (%s) canceled", node->name,
> reason);
> > - engine_set_node_state(node, EN_CANCELED);
> > + engine_set_node_state(node, EN_CANCELED, "recompute not
> allowed");
> > goto done;
> > }
> >
> > @@ -428,7 +434,8 @@ engine_recompute(struct engine_node *node, bool
> allowed,
> >
> > /* Run the node handler which might change state. */
> > long long int now = time_msec();
> > - engine_set_node_state(node, node->run(node, node->data));
> > + engine_set_node_state(node, node->run(node, node->data),
> > + "recompute run() result");
> > node->stats.recompute++;
> > long long int delta_time = time_msec() - now;
> > if (delta_time > engine_compute_log_timeout_msec) {
> > @@ -477,7 +484,9 @@ engine_compute(struct engine_node *node, bool
> recompute_allowed)
> > * Otherwise, handlers might change the state from
> EN_UPDATED
> > * back to EN_UNCHANGED.
> > */
> > - engine_set_node_state(node, (enum engine_node_state)
> handled);
> > + engine_set_node_state(node, (enum engine_node_state)
> handled,
> > + "input %s updated",
> > + node->inputs[i].node->name);
> > }
> > }
> > }
> > @@ -491,7 +500,8 @@ engine_run_node(struct engine_node *node, bool
> recompute_allowed)
> > {
> > if (!node->n_inputs) {
> > /* Run the node handler which might change state. */
> > - engine_set_node_state(node, node->run(node, node->data));
> > + engine_set_node_state(node, node->run(node, node->data),
> > + "run() result due to having no inputs");
> > node->stats.recompute++;
> > return;
> > }
> > @@ -532,7 +542,7 @@ engine_run_node(struct engine_node *node, bool
> recompute_allowed)
> > * still valid.
> > */
> > if (!engine_node_changed(node)) {
> > - engine_set_node_state(node, EN_UNCHANGED);
> > + engine_set_node_state(node, EN_UNCHANGED, "No change detected");
>
Here is the capital letter in reason.
> }
> > }
> >
> > @@ -569,7 +579,8 @@ engine_need_run(void)
> >
> > engine_set_node_state(engine_nodes[i],
> > engine_nodes[i]->run(engine_nodes[i],
> > -
> engine_nodes[i]->data));
> > +
> engine_nodes[i]->data),
> > + "checking if engine needs to be run");
> > engine_nodes[i]->stats.recompute++;
> > VLOG_DBG("input node: %s, state: %s", engine_nodes[i]->name,
> > engine_node_state_name[engine_nodes[i]->state]);
> > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h
> > index 8d9348122..39679ea9c 100644
> > --- a/lib/inc-proc-eng.h
> > +++ b/lib/inc-proc-eng.h
> > @@ -336,10 +336,6 @@ const struct engine_context
> *engine_get_context(void);
> >
> > void engine_set_context(const struct engine_context *);
> >
> > -void engine_set_node_state_at(struct engine_node *node,
> > - enum engine_node_state state,
> > - const char *where);
> > -
> > /* Return true if during the last iteration the node's data was
> updated. */
> > bool engine_node_changed(struct engine_node *node);
> >
> > @@ -372,10 +368,6 @@ void *engine_get_data(struct engine_node *node);
> > */
> > void *engine_get_internal_data(struct engine_node *node);
> >
> > -/* Set the state of the node and log changes. */
> > -#define engine_set_node_state(node, state) \
> > - engine_set_node_state_at(node, state, OVS_SOURCE_LOCATOR)
> > -
> > /* Trigger a full recompute. */
> > void engine_trigger_recompute(void);
> >
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
With the memory leak and the reason addressed:
Acked-by: Ales Musil <amu...@redhat.com>
Thanks,
Ales
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
