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>
Acked-by: Ales Musil <amu...@redhat.com>
---
v1 -> v2:
* Fixed memory leak of "reason" parameter when changing state.
* Made capitalization and punctuation consistent for state change
reasons.
* Added Ales's ack.
---
lib/inc-proc-eng.c | 42 +++++++++++++++++++++++++++---------------
lib/inc-proc-eng.h | 8 --------
2 files changed, 27 insertions(+), 23 deletions(-)
diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c
index 953023bb1..a2ed61b69 100644
--- a/lib/inc-proc-eng.c
+++ b/lib/inc-proc-eng.c
@@ -308,19 +308,26 @@ 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);
+ free(reason);
+ }
node->state = state;
}
@@ -392,7 +399,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");
if (engine_nodes[i]->clear_tracked_data) {
engine_nodes[i]->clear_tracked_data(engine_nodes[i]->data);
@@ -416,7 +423,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 +435,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 +485,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 +501,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 +543,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");
}
}
@@ -569,7 +580,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);
--
2.47.0
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
