On Fri, May 16, 2025 at 6:31 PM Lorenzo Bianconi < [email protected]> wrote:
> Introduce the dump_compute_failure_info callback used to report info about > the input node that triggers the parent node recompute. > This patch sets the callback just for DB nodes dumping the table name and > the row UUID that triggers the recompute when the parent node does not > support incremental processing for this input. Following patches will > specify dump_compute_failure_info callback even for non-DB nodes. > > Reported-at: https://issues.redhat.com/browse/FDP-1396 > Signed-off-by: Lorenzo Bianconi <[email protected]> > --- > Hi Lorenzo, thank you for the patch. There might have been some misunderstanding about the goal of this, see down below. lib/inc-proc-eng.c | 8 ++++++-- > lib/inc-proc-eng.h | 43 +++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 49 insertions(+), 2 deletions(-) > > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c > index d19a6ca55..705358949 100644 > --- a/lib/inc-proc-eng.c > +++ b/lib/inc-proc-eng.c > @@ -501,14 +501,18 @@ engine_run_node(struct engine_node *node, bool > recompute_allowed) > */ > bool need_compute = false; > for (size_t i = 0; i < node->n_inputs; i++) { > - if (node->inputs[i].node->state == EN_UPDATED) { > + struct engine_node *input_node = node->inputs[i].node; > + if (input_node->state == EN_UPDATED) { > need_compute = true; > > /* Trigger a recompute if we don't have a change handler. */ > if (!node->inputs[i].change_handler) { > engine_recompute(node, recompute_allowed, > "missing handler for input %s", > - node->inputs[i].node->name); > + input_node->name); > + if (input_node->dump_compute_failure_info) { > + input_node->dump_compute_failure_info(input_node); > The dump should also happen in cases when the node returns EN_UNHANDLED. + } > return; > } > } > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h > index 39679ea9c..1b10ff7f2 100644 > --- a/lib/inc-proc-eng.h > +++ b/lib/inc-proc-eng.h > @@ -265,6 +265,11 @@ struct engine_node { > * engine 'data'. It may be NULL. */ > void (*clear_tracked_data)(void *tracked_data); > > + /* Method used to dump info about node input compute failues. It may > be > + * NULL. > + */ > + void (*dump_compute_failure_info)(struct engine_node *); > We should have a way of passing custom info to the callback, maybe in the form of a void pointer in the engine node? > + > /* Engine stats. */ > struct engine_stats stats; > }; > @@ -422,6 +427,9 @@ void engine_ovsdb_node_add_index(struct engine_node *, > const char *name, > #define IS_VALID(NAME) \ > .is_valid = en_##NAME##_is_valid > > +#define COMPUTE_FAIL_INFO(NAME) \ > + .dump_compute_failure_info = en_##NAME##_compute_failure_info, > + > #define ENGINE_NODE2(NAME, ARG1) \ > ENGINE_NODE_DEF_START(NAME, #NAME) \ > ARG1(NAME), \ > @@ -460,6 +468,40 @@ static void *en_##DB_NAME##_##TBL_NAME##_init( \ > } \ > static void en_##DB_NAME##_##TBL_NAME##_cleanup(void *data OVS_UNUSED) \ > { \ > +} \ > +static void \ > +en_##DB_NAME##_##TBL_NAME##_compute_failure_info(struct engine_node > *node) \ > +{ > \ > + if (!VLOG_IS_DBG_ENABLED()) { > \ > + return; > \ > + } > \ > + const struct ovsdb_idl *table = EN_OVSDB_GET(node); > \ > + const void *iter; Both 'ovsdb_idl_track_get_first()' and 'ovsdb_idl_track_get_next()' return 'const struct ovsdb_idl_row *' so there is no need for void. > \ > + for ((iter) = ovsdb_idl_track_get_first(table, > \ > + > &DB_NAME##rec_table_##TBL_NAME);\ > + (iter); > \ > + (iter) = ovsdb_idl_track_get_next(iter)) { > \ > Also I don't think we need to iterate at all if we want only the first one. + const struct ovsdb_idl_row *row = iter; > \ > + if (ovsdb_idl_row_get_seqno((iter), OVSDB_IDL_CHANGE_INSERT) > 0) > { \ > + VLOG_DBG("%s (New) "UUID_FMT, > \ > + #DB_NAME"_"#TBL_NAME, UUID_ARGS(&row->uuid)); > \ > + break; > \ > + } else if (ovsdb_idl_row_get_seqno((iter), > \ > + OVSDB_IDL_CHANGE_DELETE) > 0) > { \ > + VLOG_DBG("%s (Deleted) "UUID_FMT, > \ > + #DB_NAME"_"#TBL_NAME, UUID_ARGS(&row->uuid)); > \ > + break; > \ > + } else { > \ > + for (int i = 0; i < row->table->class_->n_columns; i++) { > \ > nit: size_t i > + if (ovsdb_idl_track_is_updated(row, > \ > + &DB_NAME##rec_##TBL_NAME##_columns[i])) { > \ > + VLOG_DBG("%s (Updated) "UUID_FMT, > \ > + #DB_NAME"_"#TBL_NAME, > UUID_ARGS(&row->uuid)); \ > + break; > \ > It would be nice to get all changed columns, not only the first one. > + } > \ > + } > \ > + } > \ > + } > \ > } > > /* Macro to define member functions of an engine node which represents > @@ -480,6 +522,7 @@ static void en_##DB_NAME##_##TBL_NAME##_cleanup(void > *data OVS_UNUSED) \ > /* Macro to define an engine node which represents a table of OVSDB */ > #define ENGINE_NODE_OVSDB(DB_NAME, DB_NAME_STR, TBL_NAME, TBL_NAME_STR) \ > ENGINE_NODE_DEF_START(DB_NAME##_##TBL_NAME, > DB_NAME_STR"_"TBL_NAME_STR) \ > + COMPUTE_FAIL_INFO(DB_NAME##_##TBL_NAME) \ > ENGINE_NODE_DEF_END > > /* Macro to define an engine node which represents a table of OVN SB DB */ > -- > 2.49.0 > > Thanks, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
