[ovs-dev] [PATCH ovn v10 4/8] I-P engine: Provide the option for an engine to clear tracked engine data in every run.
Dumitru Ceara
dceara at redhat.com
Tue Jun 9 09:05:15 UTC 2020
On 6/9/20 10:46 AM, Numan Siddique wrote:
>
>
> On Tue, Jun 9, 2020 at 1:30 PM Dumitru Ceara <dceara at redhat.com
> <mailto:dceara at 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 at redhat.com
> <mailto:dceara at redhat.com>
> > <mailto:dceara at redhat.com <mailto:dceara at redhat.com>>> wrote:
> >
> > On 6/8/20 3:50 PM, numans at ovn.org <mailto:numans at ovn.org>
> <mailto:numans at ovn.org <mailto:numans at ovn.org>> wrote:
> > > From: Numan Siddique <numans at ovn.org <mailto:numans at ovn.org>
> <mailto:numans at ovn.org <mailto:numans at 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 at ovn.org
> <mailto:numans at ovn.org> <mailto:numans at ovn.org <mailto:numans at 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 at redhat.com
> <mailto:dceara at redhat.com> <mailto:dceara at redhat.com
> <mailto:dceara at 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 at openvswitch.org <mailto:dev at openvswitch.org>
> <mailto:dev at openvswitch.org <mailto:dev at openvswitch.org>>
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org <mailto:dev at openvswitch.org>
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list