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?
>
>
> > +
> > /* 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?
>
> + 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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
