On Mon, May 26, 2025 at 1:46 PM Lorenzo Bianconi < lorenzo.bianc...@redhat.com> wrote:
> On May 26, Ales Musil wrote: > > On Fri, May 16, 2025 at 6:31 PM Lorenzo Bianconi < > > lorenzo.bianc...@redhat.com> 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 <lorenzo.bianc...@redhat.com> > > > --- > > > > > > > Hi Lorenzo, > > > > thank you for the patch. There might have been some misunderstanding > > about the goal of this, see down below. > > Hi Ales, > > thx for the review. > > > > > 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. > > ack, I will fix it. > > > > > + } > > > 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? > > yep. Since at the moment we support this feature just for DB nodes, > I was thinking to add this info working on the non-DB nodes, what do you > think? > Yeah, that would be nice. > > > > > > > > + > > > /* 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. > > ack, I will fix it. > > > > > > > > \ > > > + 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. > > do you mean to report just the first changed element? (e.g. first > inserted/deleted/updated element). Since it is used during debugging, I > guess we > can just report all changed items, what do you think? > We can report all changes, but the current code doesn't do that, it will break after reporting the first change. So we can either use just the 'ovsdb_idl_track_get_first()' or iterate through all, but then we cannot break the loop. > > > > > + 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 > > ack, I will fix it. > > > > > > > > + 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. > > ack, I will fix it. > > Regards, > Lorenzo > > > > > > > > + } > > > \ > > > + } > > > \ > > > + } > > > \ > > > + } > > > \ > > > } > > > > > > /* 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 > Thanks, Ales _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
