| Message ID | 20200608135015.1379055-1-numans@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Incremental processing improvements. | expand |
On 6/8/20 3:50 PM, numans@ovn.org wrote: > From: Numan Siddique <numans@ovn.org> > > A new function is added in the engine node called - clear_tracked_data() to > clear any engine data which was tracked during the engine run. This tracked data > has to be part of the engine 'data'. engine_init_run() calls clear_tracked_data() > and each engine node interested in tracking the data needs to implement the > en_<ENGINE_NODE_NAME>clear_tracked_data() function. > > With this patch, an engine node can store any changes done to the engine data > separately in the engine change handlers. The parent of this engine node > can use this tracked data for incrementally processing the changes. Upcoming > patches in the series will make use of this. > > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > lib/inc-proc-eng.c | 6 +++++- > lib/inc-proc-eng.h | 9 +++++++++ > 2 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c > index 9b1479a1c..a9389f75b 100644 > --- a/lib/inc-proc-eng.c > +++ b/lib/inc-proc-eng.c > @@ -260,6 +260,10 @@ 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); > + > + if (engine_nodes[i]->clear_tracked_data) { > + engine_nodes[i]->clear_tracked_data(engine_nodes[i]->data); > + } > } > } > > @@ -370,7 +374,7 @@ engine_run(bool recompute_allowed) > > if (engine_nodes[i]->state == EN_ABORTED) { > engine_run_aborted = true; > - return; > + break; I guess this is a leftover from a previous iteration of the series. It should be fine to return here. With this fixed: Acked-by: Dumitru Ceara <dceara@redhat.com> Thanks, Dumitru > } > } > } > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h > index 780c3cd22..552f2051a 100644 > --- a/lib/inc-proc-eng.h > +++ b/lib/inc-proc-eng.h > @@ -149,6 +149,10 @@ struct engine_node { > * doesn't store pointers to DB records it's still safe to use). > */ > bool (*is_valid)(struct engine_node *); > + > + /* Method to clear up tracked data maintained by the engine node in the > + * engine 'data'. It may be NULL. */ > + void (*clear_tracked_data)(void *tracked_data); > }; > > /* Initialize the data for the engine nodes. It calls each node's > @@ -275,6 +279,7 @@ void engine_ovsdb_node_add_index(struct engine_node *, const char *name, > .run = en_##NAME##_run, \ > .cleanup = en_##NAME##_cleanup, \ > .is_valid = en_##NAME##_is_valid, \ > + .clear_tracked_data = NULL, \ > }; > > #define ENGINE_NODE_CUSTOM_DATA(NAME, NAME_STR) \ > @@ -284,6 +289,10 @@ void engine_ovsdb_node_add_index(struct engine_node *, const char *name, > static bool (*en_##NAME##_is_valid)(struct engine_node *node) = NULL; \ > ENGINE_NODE_DEF(NAME, NAME_STR) > > +#define ENGINE_NODE_WITH_CLEAR_TRACK_DATA(NAME, NAME_STR) \ > + ENGINE_NODE(NAME, NAME_STR) \ > + en_##NAME.clear_tracked_data = en_##NAME##_clear_tracked_data; > + > /* Macro to define member functions of an engine node which represents > * a table of OVSDB */ > #define ENGINE_FUNC_OVSDB(DB_NAME, TBL_NAME) \ >
On Mon, Jun 8, 2020 at 9:42 PM Dumitru Ceara <dceara@redhat.com> wrote: > On 6/8/20 3:50 PM, numans@ovn.org wrote: > > From: Numan Siddique <numans@ovn.org> > > > > A new function is added in the engine node called - clear_tracked_data() > to > > clear any engine data which was tracked during the engine run. This > tracked data > > has to be part of the engine 'data'. engine_init_run() calls > clear_tracked_data() > > and each engine node interested in tracking the data needs to implement > the > > en_<ENGINE_NODE_NAME>clear_tracked_data() function. > > > > With this patch, an engine node can store any changes done to the engine > data > > separately in the engine change handlers. The parent of this engine node > > can use this tracked data for incrementally processing the changes. > Upcoming > > patches in the series will make use of this. > > > > Signed-off-by: Numan Siddique <numans@ovn.org> > > --- > > lib/inc-proc-eng.c | 6 +++++- > > lib/inc-proc-eng.h | 9 +++++++++ > > 2 files changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c > > index 9b1479a1c..a9389f75b 100644 > > --- a/lib/inc-proc-eng.c > > +++ b/lib/inc-proc-eng.c > > @@ -260,6 +260,10 @@ 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); > > + > > + if (engine_nodes[i]->clear_tracked_data) { > > + engine_nodes[i]->clear_tracked_data(engine_nodes[i]->data); > > + } > > } > > } > > > > @@ -370,7 +374,7 @@ engine_run(bool recompute_allowed) > > > > if (engine_nodes[i]->state == EN_ABORTED) { > > engine_run_aborted = true; > > - return; > > + break; > > I guess this is a leftover from a previous iteration of the series. It > should be fine to return here. > Oops. Thanks. I'll fix it. Thanks Numan > > With this fixed: > > Acked-by: Dumitru Ceara <dceara@redhat.com> > > Thanks, > Dumitru > > > } > > } > > } > > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h > > index 780c3cd22..552f2051a 100644 > > --- a/lib/inc-proc-eng.h > > +++ b/lib/inc-proc-eng.h > > @@ -149,6 +149,10 @@ struct engine_node { > > * doesn't store pointers to DB records it's still safe to use). > > */ > > bool (*is_valid)(struct engine_node *); > > + > > + /* Method to clear up tracked data maintained by the engine node in > the > > + * engine 'data'. It may be NULL. */ > > + void (*clear_tracked_data)(void *tracked_data); > > }; > > > > /* Initialize the data for the engine nodes. It calls each node's > > @@ -275,6 +279,7 @@ void engine_ovsdb_node_add_index(struct engine_node > *, const char *name, > > .run = en_##NAME##_run, \ > > .cleanup = en_##NAME##_cleanup, \ > > .is_valid = en_##NAME##_is_valid, \ > > + .clear_tracked_data = NULL, \ > > }; > > > > #define ENGINE_NODE_CUSTOM_DATA(NAME, NAME_STR) \ > > @@ -284,6 +289,10 @@ void engine_ovsdb_node_add_index(struct engine_node > *, const char *name, > > static bool (*en_##NAME##_is_valid)(struct engine_node *node) = > NULL; \ > > ENGINE_NODE_DEF(NAME, NAME_STR) > > > > +#define ENGINE_NODE_WITH_CLEAR_TRACK_DATA(NAME, NAME_STR) \ > > + ENGINE_NODE(NAME, NAME_STR) \ > > + en_##NAME.clear_tracked_data = en_##NAME##_clear_tracked_data; > > + > > /* Macro to define member functions of an engine node which represents > > * a table of OVSDB */ > > #define ENGINE_FUNC_OVSDB(DB_NAME, TBL_NAME) \ > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On 6/8/20 7:34 PM, Numan Siddique wrote: > > > On Mon, Jun 8, 2020 at 9:42 PM Dumitru Ceara <dceara@redhat.com > <mailto:dceara@redhat.com>> wrote: > > On 6/8/20 3:50 PM, numans@ovn.org <mailto:numans@ovn.org> wrote: > > From: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> > > > > A new function is added in the engine node called - > clear_tracked_data() to > > clear any engine data which was tracked during the engine run. > This tracked data > > has to be part of the engine 'data'. engine_init_run() calls > clear_tracked_data() > > and each engine node interested in tracking the data needs to > implement the > > en_<ENGINE_NODE_NAME>clear_tracked_data() function. > > > > With this patch, an engine node can store any changes done to the > engine data > > separately in the engine change handlers. The parent of this > engine node > > can use this tracked data for incrementally processing the > changes. Upcoming > > patches in the series will make use of this. > > > > Signed-off-by: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> > > --- > > lib/inc-proc-eng.c | 6 +++++- > > lib/inc-proc-eng.h | 9 +++++++++ > > 2 files changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c > > index 9b1479a1c..a9389f75b 100644 > > --- a/lib/inc-proc-eng.c > > +++ b/lib/inc-proc-eng.c > > @@ -260,6 +260,10 @@ 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); > > + > > + if (engine_nodes[i]->clear_tracked_data) { > > + > engine_nodes[i]->clear_tracked_data(engine_nodes[i]->data); > > + } > > } > > } > > > > @@ -370,7 +374,7 @@ engine_run(bool recompute_allowed) > > > > if (engine_nodes[i]->state == EN_ABORTED) { > > engine_run_aborted = true; > > - return; > > + break; > > I guess this is a leftover from a previous iteration of the series. It > should be fine to return here. > > > Oops. Thanks. I'll fix it. > > Thanks > Numan Hi Numan, While reviewing the following patches I realized that it might be useful if in this patch we'd also call engine_nodes[i]->clear_tracked_data() in engine_cleanup(), what do you think? Thanks, Dumitru > > > > With this fixed: > > Acked-by: Dumitru Ceara <dceara@redhat.com <mailto:dceara@redhat.com>> > > Thanks, > Dumitru > > > } > > } > > } > > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h > > index 780c3cd22..552f2051a 100644 > > --- a/lib/inc-proc-eng.h > > +++ b/lib/inc-proc-eng.h > > @@ -149,6 +149,10 @@ struct engine_node { > > * doesn't store pointers to DB records it's still safe to use). > > */ > > bool (*is_valid)(struct engine_node *); > > + > > + /* Method to clear up tracked data maintained by the engine > node in the > > + * engine 'data'. It may be NULL. */ > > + void (*clear_tracked_data)(void *tracked_data); > > }; > > > > /* Initialize the data for the engine nodes. It calls each node's > > @@ -275,6 +279,7 @@ void engine_ovsdb_node_add_index(struct > engine_node *, const char *name, > > .run = en_##NAME##_run, \ > > .cleanup = en_##NAME##_cleanup, \ > > .is_valid = en_##NAME##_is_valid, \ > > + .clear_tracked_data = NULL, \ > > }; > > > > #define ENGINE_NODE_CUSTOM_DATA(NAME, NAME_STR) \ > > @@ -284,6 +289,10 @@ void engine_ovsdb_node_add_index(struct > engine_node *, const char *name, > > static bool (*en_##NAME##_is_valid)(struct engine_node *node) > = NULL; \ > > ENGINE_NODE_DEF(NAME, NAME_STR) > > > > +#define ENGINE_NODE_WITH_CLEAR_TRACK_DATA(NAME, NAME_STR) \ > > + ENGINE_NODE(NAME, NAME_STR) \ > > + en_##NAME.clear_tracked_data = en_##NAME##_clear_tracked_data; > > + > > /* Macro to define member functions of an engine node which > represents > > * a table of OVSDB */ > > #define ENGINE_FUNC_OVSDB(DB_NAME, TBL_NAME) \ > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org <mailto:dev@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Tue, Jun 9, 2020 at 1:30 PM Dumitru Ceara <dceara@redhat.com> wrote: > On 6/8/20 7:34 PM, Numan Siddique wrote: > > > > > > On Mon, Jun 8, 2020 at 9:42 PM Dumitru Ceara <dceara@redhat.com > > <mailto:dceara@redhat.com>> wrote: > > > > On 6/8/20 3:50 PM, numans@ovn.org <mailto:numans@ovn.org> wrote: > > > From: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> > > > > > > A new function is added in the engine node called - > > clear_tracked_data() to > > > clear any engine data which was tracked during the engine run. > > This tracked data > > > has to be part of the engine 'data'. engine_init_run() calls > > clear_tracked_data() > > > and each engine node interested in tracking the data needs to > > implement the > > > en_<ENGINE_NODE_NAME>clear_tracked_data() function. > > > > > > With this patch, an engine node can store any changes done to the > > engine data > > > separately in the engine change handlers. The parent of this > > engine node > > > can use this tracked data for incrementally processing the > > changes. Upcoming > > > patches in the series will make use of this. > > > > > > Signed-off-by: Numan Siddique <numans@ovn.org <mailto: > numans@ovn.org>> > > > --- > > > lib/inc-proc-eng.c | 6 +++++- > > > lib/inc-proc-eng.h | 9 +++++++++ > > > 2 files changed, 14 insertions(+), 1 deletion(-) > > > > > > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c > > > index 9b1479a1c..a9389f75b 100644 > > > --- a/lib/inc-proc-eng.c > > > +++ b/lib/inc-proc-eng.c > > > @@ -260,6 +260,10 @@ 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); > > > + > > > + if (engine_nodes[i]->clear_tracked_data) { > > > + > > engine_nodes[i]->clear_tracked_data(engine_nodes[i]->data); > > > + } > > > } > > > } > > > > > > @@ -370,7 +374,7 @@ engine_run(bool recompute_allowed) > > > > > > if (engine_nodes[i]->state == EN_ABORTED) { > > > engine_run_aborted = true; > > > - return; > > > + break; > > > > I guess this is a leftover from a previous iteration of the series. > It > > should be fine to return here. > > > > > > Oops. Thanks. I'll fix it. > > > > Thanks > > Numan > > Hi Numan, > > While reviewing the following patches I realized that it might be useful > if in this patch we'd also call engine_nodes[i]->clear_tracked_data() in > engine_cleanup(), what do you think? > I think we can call. But since its part of engine data, engine data cleanup should free up any memory right ? Even if clear_tracked_data() is not called, engine_data_cleanup() should not leak the memory. The patch 6 in this series does that. Thanks Numan > Thanks, > Dumitru > > > > > > > > > With this fixed: > > > > Acked-by: Dumitru Ceara <dceara@redhat.com <mailto:dceara@redhat.com > >> > > > > Thanks, > > Dumitru > > > > > } > > > } > > > } > > > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h > > > index 780c3cd22..552f2051a 100644 > > > --- a/lib/inc-proc-eng.h > > > +++ b/lib/inc-proc-eng.h > > > @@ -149,6 +149,10 @@ struct engine_node { > > > * doesn't store pointers to DB records it's still safe to > use). > > > */ > > > bool (*is_valid)(struct engine_node *); > > > + > > > + /* Method to clear up tracked data maintained by the engine > > node in the > > > + * engine 'data'. It may be NULL. */ > > > + void (*clear_tracked_data)(void *tracked_data); > > > }; > > > > > > /* Initialize the data for the engine nodes. It calls each node's > > > @@ -275,6 +279,7 @@ void engine_ovsdb_node_add_index(struct > > engine_node *, const char *name, > > > .run = en_##NAME##_run, \ > > > .cleanup = en_##NAME##_cleanup, \ > > > .is_valid = en_##NAME##_is_valid, \ > > > + .clear_tracked_data = NULL, \ > > > }; > > > > > > #define ENGINE_NODE_CUSTOM_DATA(NAME, NAME_STR) \ > > > @@ -284,6 +289,10 @@ void engine_ovsdb_node_add_index(struct > > engine_node *, const char *name, > > > static bool (*en_##NAME##_is_valid)(struct engine_node *node) > > = NULL; \ > > > ENGINE_NODE_DEF(NAME, NAME_STR) > > > > > > +#define ENGINE_NODE_WITH_CLEAR_TRACK_DATA(NAME, NAME_STR) \ > > > + ENGINE_NODE(NAME, NAME_STR) \ > > > + en_##NAME.clear_tracked_data = en_##NAME##_clear_tracked_data; > > > + > > > /* Macro to define member functions of an engine node which > > represents > > > * a table of OVSDB */ > > > #define ENGINE_FUNC_OVSDB(DB_NAME, TBL_NAME) \ > > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org <mailto:dev@openvswitch.org> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 6/9/20 10:46 AM, Numan Siddique wrote: > > > On Tue, Jun 9, 2020 at 1:30 PM Dumitru Ceara <dceara@redhat.com > <mailto:dceara@redhat.com>> wrote: > > On 6/8/20 7:34 PM, Numan Siddique wrote: > > > > > > On Mon, Jun 8, 2020 at 9:42 PM Dumitru Ceara <dceara@redhat.com > <mailto:dceara@redhat.com> > > <mailto:dceara@redhat.com <mailto:dceara@redhat.com>>> wrote: > > > > On 6/8/20 3:50 PM, numans@ovn.org <mailto:numans@ovn.org> > <mailto:numans@ovn.org <mailto:numans@ovn.org>> wrote: > > > From: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org> > <mailto:numans@ovn.org <mailto:numans@ovn.org>>> > > > > > > A new function is added in the engine node called - > > clear_tracked_data() to > > > clear any engine data which was tracked during the engine run. > > This tracked data > > > has to be part of the engine 'data'. engine_init_run() calls > > clear_tracked_data() > > > and each engine node interested in tracking the data needs to > > implement the > > > en_<ENGINE_NODE_NAME>clear_tracked_data() function. > > > > > > With this patch, an engine node can store any changes done > to the > > engine data > > > separately in the engine change handlers. The parent of this > > engine node > > > can use this tracked data for incrementally processing the > > changes. Upcoming > > > patches in the series will make use of this. > > > > > > Signed-off-by: Numan Siddique <numans@ovn.org > <mailto:numans@ovn.org> <mailto:numans@ovn.org <mailto:numans@ovn.org>>> > > > --- > > > lib/inc-proc-eng.c | 6 +++++- > > > lib/inc-proc-eng.h | 9 +++++++++ > > > 2 files changed, 14 insertions(+), 1 deletion(-) > > > > > > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c > > > index 9b1479a1c..a9389f75b 100644 > > > --- a/lib/inc-proc-eng.c > > > +++ b/lib/inc-proc-eng.c > > > @@ -260,6 +260,10 @@ 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); > > > + > > > + if (engine_nodes[i]->clear_tracked_data) { > > > + > > engine_nodes[i]->clear_tracked_data(engine_nodes[i]->data); > > > + } > > > } > > > } > > > > > > @@ -370,7 +374,7 @@ engine_run(bool recompute_allowed) > > > > > > if (engine_nodes[i]->state == EN_ABORTED) { > > > engine_run_aborted = true; > > > - return; > > > + break; > > > > I guess this is a leftover from a previous iteration of the > series. It > > should be fine to return here. > > > > > > Oops. Thanks. I'll fix it. > > > > Thanks > > Numan > > Hi Numan, > > While reviewing the following patches I realized that it might be useful > if in this patch we'd also call engine_nodes[i]->clear_tracked_data() in > engine_cleanup(), what do you think? > > > I think we can call. But since its part of engine data, engine data > cleanup should > free up any memory right ? Even if clear_tracked_data() is not called, > engine_data_cleanup() > should not leak the memory. The patch 6 in this series does that. > In patch 6 we call en_runtime_data_clear_tracked_data() from en_runtime_data_cleanup() which puts the burden on the I-P engine user to make sure to not forget to call the clear_tracked_data callback from the cleanup handler I think this should always be done, for all nodes, regardless of data contents, so I'd make sure that happens by calling the clear_tracked_data callback from within engine_data_cleanup(). Thanks, Dumitru > Thanks > Numan > > > Thanks, > Dumitru > > > > > > > > > With this fixed: > > > > Acked-by: Dumitru Ceara <dceara@redhat.com > <mailto:dceara@redhat.com> <mailto:dceara@redhat.com > <mailto:dceara@redhat.com>>> > > > > Thanks, > > Dumitru > > > > > } > > > } > > > } > > > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h > > > index 780c3cd22..552f2051a 100644 > > > --- a/lib/inc-proc-eng.h > > > +++ b/lib/inc-proc-eng.h > > > @@ -149,6 +149,10 @@ struct engine_node { > > > * doesn't store pointers to DB records it's still safe > to use). > > > */ > > > bool (*is_valid)(struct engine_node *); > > > + > > > + /* Method to clear up tracked data maintained by the engine > > node in the > > > + * engine 'data'. It may be NULL. */ > > > + void (*clear_tracked_data)(void *tracked_data); > > > }; > > > > > > /* Initialize the data for the engine nodes. It calls each > node's > > > @@ -275,6 +279,7 @@ void engine_ovsdb_node_add_index(struct > > engine_node *, const char *name, > > > .run = en_##NAME##_run, \ > > > .cleanup = en_##NAME##_cleanup, \ > > > .is_valid = en_##NAME##_is_valid, \ > > > + .clear_tracked_data = NULL, \ > > > }; > > > > > > #define ENGINE_NODE_CUSTOM_DATA(NAME, NAME_STR) \ > > > @@ -284,6 +289,10 @@ void engine_ovsdb_node_add_index(struct > > engine_node *, const char *name, > > > static bool (*en_##NAME##_is_valid)(struct engine_node > *node) > > = NULL; \ > > > ENGINE_NODE_DEF(NAME, NAME_STR) > > > > > > +#define ENGINE_NODE_WITH_CLEAR_TRACK_DATA(NAME, NAME_STR) \ > > > + ENGINE_NODE(NAME, NAME_STR) \ > > > + en_##NAME.clear_tracked_data = > en_##NAME##_clear_tracked_data; > > > + > > > /* Macro to define member functions of an engine node which > > represents > > > * a table of OVSDB */ > > > #define ENGINE_FUNC_OVSDB(DB_NAME, TBL_NAME) \ > > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org <mailto:dev@openvswitch.org> > <mailto:dev@openvswitch.org <mailto:dev@openvswitch.org>> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org <mailto:dev@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Tue, Jun 9, 2020 at 2:35 PM Dumitru Ceara <dceara@redhat.com> wrote: > On 6/9/20 10:46 AM, Numan Siddique wrote: > > > > > > On Tue, Jun 9, 2020 at 1:30 PM Dumitru Ceara <dceara@redhat.com > > <mailto:dceara@redhat.com>> wrote: > > > > On 6/8/20 7:34 PM, Numan Siddique wrote: > > > > > > > > > On Mon, Jun 8, 2020 at 9:42 PM Dumitru Ceara <dceara@redhat.com > > <mailto:dceara@redhat.com> > > > <mailto:dceara@redhat.com <mailto:dceara@redhat.com>>> wrote: > > > > > > On 6/8/20 3:50 PM, numans@ovn.org <mailto:numans@ovn.org> > > <mailto:numans@ovn.org <mailto:numans@ovn.org>> wrote: > > > > From: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org> > > <mailto:numans@ovn.org <mailto:numans@ovn.org>>> > > > > > > > > A new function is added in the engine node called - > > > clear_tracked_data() to > > > > clear any engine data which was tracked during the engine > run. > > > This tracked data > > > > has to be part of the engine 'data'. engine_init_run() calls > > > clear_tracked_data() > > > > and each engine node interested in tracking the data needs to > > > implement the > > > > en_<ENGINE_NODE_NAME>clear_tracked_data() function. > > > > > > > > With this patch, an engine node can store any changes done > > to the > > > engine data > > > > separately in the engine change handlers. The parent of this > > > engine node > > > > can use this tracked data for incrementally processing the > > > changes. Upcoming > > > > patches in the series will make use of this. > > > > > > > > Signed-off-by: Numan Siddique <numans@ovn.org > > <mailto:numans@ovn.org> <mailto:numans@ovn.org <mailto: > numans@ovn.org>>> > > > > --- > > > > lib/inc-proc-eng.c | 6 +++++- > > > > lib/inc-proc-eng.h | 9 +++++++++ > > > > 2 files changed, 14 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c > > > > index 9b1479a1c..a9389f75b 100644 > > > > --- a/lib/inc-proc-eng.c > > > > +++ b/lib/inc-proc-eng.c > > > > @@ -260,6 +260,10 @@ 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); > > > > + > > > > + if (engine_nodes[i]->clear_tracked_data) { > > > > + > > > engine_nodes[i]->clear_tracked_data(engine_nodes[i]->data); > > > > + } > > > > } > > > > } > > > > > > > > @@ -370,7 +374,7 @@ engine_run(bool recompute_allowed) > > > > > > > > if (engine_nodes[i]->state == EN_ABORTED) { > > > > engine_run_aborted = true; > > > > - return; > > > > + break; > > > > > > I guess this is a leftover from a previous iteration of the > > series. It > > > should be fine to return here. > > > > > > > > > Oops. Thanks. I'll fix it. > > > > > > Thanks > > > Numan > > > > Hi Numan, > > > > While reviewing the following patches I realized that it might be > useful > > if in this patch we'd also call > engine_nodes[i]->clear_tracked_data() in > > engine_cleanup(), what do you think? > > > > > > I think we can call. But since its part of engine data, engine data > > cleanup should > > free up any memory right ? Even if clear_tracked_data() is not called, > > engine_data_cleanup() > > should not leak the memory. The patch 6 in this series does that. > > > > In patch 6 we call en_runtime_data_clear_tracked_data() from > en_runtime_data_cleanup() which puts the burden on the I-P engine user > to make sure to not forget to call the clear_tracked_data callback from > the cleanup handler > > I think this should always be done, for all nodes, regardless of data > contents, so I'd make sure that happens by calling the > clear_tracked_data callback from within engine_data_cleanup(). > Ok. I agree with you. If engine was calling clear_tracked_data() at the end of every run, there was no need to call it during engine_data_cleanup(). Thanks Numan > Thanks, > Dumitru > > > Thanks > > Numan > > > > > > Thanks, > > Dumitru > > > > > > > > > > > > > > With this fixed: > > > > > > Acked-by: Dumitru Ceara <dceara@redhat.com > > <mailto:dceara@redhat.com> <mailto:dceara@redhat.com > > <mailto:dceara@redhat.com>>> > > > > > > Thanks, > > > Dumitru > > > > > > > } > > > > } > > > > } > > > > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h > > > > index 780c3cd22..552f2051a 100644 > > > > --- a/lib/inc-proc-eng.h > > > > +++ b/lib/inc-proc-eng.h > > > > @@ -149,6 +149,10 @@ struct engine_node { > > > > * doesn't store pointers to DB records it's still safe > > to use). > > > > */ > > > > bool (*is_valid)(struct engine_node *); > > > > + > > > > + /* Method to clear up tracked data maintained by the > engine > > > node in the > > > > + * engine 'data'. It may be NULL. */ > > > > + void (*clear_tracked_data)(void *tracked_data); > > > > }; > > > > > > > > /* Initialize the data for the engine nodes. It calls each > > node's > > > > @@ -275,6 +279,7 @@ void engine_ovsdb_node_add_index(struct > > > engine_node *, const char *name, > > > > .run = en_##NAME##_run, \ > > > > .cleanup = en_##NAME##_cleanup, \ > > > > .is_valid = en_##NAME##_is_valid, \ > > > > + .clear_tracked_data = NULL, \ > > > > }; > > > > > > > > #define ENGINE_NODE_CUSTOM_DATA(NAME, NAME_STR) \ > > > > @@ -284,6 +289,10 @@ void engine_ovsdb_node_add_index(struct > > > engine_node *, const char *name, > > > > static bool (*en_##NAME##_is_valid)(struct engine_node > > *node) > > > = NULL; \ > > > > ENGINE_NODE_DEF(NAME, NAME_STR) > > > > > > > > +#define ENGINE_NODE_WITH_CLEAR_TRACK_DATA(NAME, NAME_STR) \ > > > > + ENGINE_NODE(NAME, NAME_STR) \ > > > > + en_##NAME.clear_tracked_data = > > en_##NAME##_clear_tracked_data; > > > > + > > > > /* Macro to define member functions of an engine node which > > > represents > > > > * a table of OVSDB */ > > > > #define ENGINE_FUNC_OVSDB(DB_NAME, TBL_NAME) \ > > > > > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org <mailto:dev@openvswitch.org> > > <mailto:dev@openvswitch.org <mailto:dev@openvswitch.org>> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org <mailto:dev@openvswitch.org> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c index 9b1479a1c..a9389f75b 100644 --- a/lib/inc-proc-eng.c +++ b/lib/inc-proc-eng.c @@ -260,6 +260,10 @@ 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); + + if (engine_nodes[i]->clear_tracked_data) { + engine_nodes[i]->clear_tracked_data(engine_nodes[i]->data); + } } } @@ -370,7 +374,7 @@ engine_run(bool recompute_allowed) if (engine_nodes[i]->state == EN_ABORTED) { engine_run_aborted = true; - return; + break; } } } diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h index 780c3cd22..552f2051a 100644 --- a/lib/inc-proc-eng.h +++ b/lib/inc-proc-eng.h @@ -149,6 +149,10 @@ struct engine_node { * doesn't store pointers to DB records it's still safe to use). */ bool (*is_valid)(struct engine_node *); + + /* Method to clear up tracked data maintained by the engine node in the + * engine 'data'. It may be NULL. */ + void (*clear_tracked_data)(void *tracked_data); }; /* Initialize the data for the engine nodes. It calls each node's @@ -275,6 +279,7 @@ void engine_ovsdb_node_add_index(struct engine_node *, const char *name, .run = en_##NAME##_run, \ .cleanup = en_##NAME##_cleanup, \ .is_valid = en_##NAME##_is_valid, \ + .clear_tracked_data = NULL, \ }; #define ENGINE_NODE_CUSTOM_DATA(NAME, NAME_STR) \ @@ -284,6 +289,10 @@ void engine_ovsdb_node_add_index(struct engine_node *, const char *name, static bool (*en_##NAME##_is_valid)(struct engine_node *node) = NULL; \ ENGINE_NODE_DEF(NAME, NAME_STR) +#define ENGINE_NODE_WITH_CLEAR_TRACK_DATA(NAME, NAME_STR) \ + ENGINE_NODE(NAME, NAME_STR) \ + en_##NAME.clear_tracked_data = en_##NAME##_clear_tracked_data; + /* Macro to define member functions of an engine node which represents * a table of OVSDB */ #define ENGINE_FUNC_OVSDB(DB_NAME, TBL_NAME) \