On Thu, May 28, 2020 at 11:29 AM Dumitru Ceara <dce...@redhat.com> wrote:
>
> On 5/28/20 1:04 PM, num...@ovn.org wrote:
> > From: Numan Siddique <num...@ovn.org>
> >
> > With this patch, an engine node which is an input to another engine node
> > can provide the tracked data. The parent of this engine node can handle
> > this tracked data incrementally.
> >
> > At the end of the engine_run(), the tracked data of the nodes are
> > cleared.
> >
> > Acked-by: Mark Michelson <mmich...@redhat.com>
> > Signed-off-by: Numan Siddique <num...@ovn.org>
> > ---
> > lib/inc-proc-eng.c | 30 +++++++++++++++++++++++++++++-
> > lib/inc-proc-eng.h | 20 ++++++++++++++++++++
> > 2 files changed, 49 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c
> > index 9b1479a1c..8d63feac7 100644
> > --- a/lib/inc-proc-eng.c
> > +++ b/lib/inc-proc-eng.c
> > @@ -125,6 +125,11 @@ engine_cleanup(void)
> > engine_nodes[i]->cleanup(engine_nodes[i]->data);
> > }
> > free(engine_nodes[i]->data);
> > +
> > + if (engine_nodes[i]->clear_tracked_data) {
> > +
engine_nodes[i]->clear_tracked_data(engine_nodes[i]->tracked_data);
> > + }
> > + free(engine_nodes[i]->tracked_data);
> > }
> > free(engine_nodes);
> > engine_nodes = NULL;
> > @@ -222,6 +227,23 @@ engine_node_changed(struct engine_node *node)
> > return node->state == EN_UPDATED;
> > }
> >
> > +void *
> > +engine_get_tracked_data(struct engine_node *node)
> > +{
> > + if (engine_node_valid(node)) {
> > + return node->tracked_data;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +void *
> > +engine_get_input_tracked_data(const char *input_name, struct
engine_node *node)
> > +{
> > + struct engine_node *input_node = engine_get_input(input_name,
node);
> > + return engine_get_tracked_data(input_node);
> > +}
> > +
> > bool
> > engine_has_run(void)
> > {
> > @@ -370,7 +392,13 @@ engine_run(bool recompute_allowed)
> >
> > if (engine_nodes[i]->state == EN_ABORTED) {
> > engine_run_aborted = true;
> > - return;
> > + break;
> > + }
> > + }
> > +
> > + for (size_t i = 0; i < engine_n_nodes; i++) {
> > + if (engine_nodes[i]->clear_tracked_data) {
> > +
engine_nodes[i]->clear_tracked_data(engine_nodes[i]->tracked_data);
> > }
> > }
> > }
> > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h
> > index 780c3cd22..4b5e69edb 100644
> > --- a/lib/inc-proc-eng.h
> > +++ b/lib/inc-proc-eng.h
> > @@ -123,6 +123,15 @@ struct engine_node {
> > */
> > void *data;
> >
> > + /* A pointer to node internal tracked data. The tracke data can be
> > + * used by en engine node to provide the changed data during the
> > + * engine run if its an input to other engine node. This data can
> > + * be accessed during the engine run using the function
> > + * engine_get_tracked_data(). This data can be cleared by
> > + * calling the clean_tracked_data() engine node function.
> > + */
> > + void *tracked_data;
> > +
> > /* State of the node after the last engine run. */
> > enum engine_node_state state;
> >
> > @@ -149,6 +158,9 @@ 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 if any. It may be NULL. */
> > + void (*clear_tracked_data)(void *tracked_data);
>
>
> Hi Numan,
>
> As discussed during the IRC meeting, this patch is OK but I found it a
> bit confusing that the only difference between "data" and "tracked_data"
> is the fact that tracked_data can be cleared after an engine run. It's
> not a big deal but it forces readers to look at how the tracked_data
> APIs are used to understand what the tracked_data can be. I think we
> could just rename "clear_tracked_data" to "clear_data" and let users
> store whatever tracked data they need in "data" and provide callbacks to
> clear what needs to be cleared after every run.
>
> What do you think?
>
> Thanks,
> Dumitru
>
Just to add my 2 cents. I understand the motivation of trying to abstract
"tracked_data". However, since the data in I-P node are completely vague
and unstructured, getting tracked data from the API doesn't make things
easier than just get the tracked data from the data. By contrast, tracked
changes of OVSDB IDL has unified APIs (templates) to traverse and access
the data, which makes the tracking APIs useful and necessary. So I tend to
agree with Dumitru that adding an API to clean tracked data after each
iteration would be enough and clear, and let each node maintain its tracked
data (if necessary) as part of its "data", which can keep the data
interface of I-P as simple as possible. (but let's make sure the API name
is not confused with cleanup() that is called before program exits. Maybe
we can still name the API as clear_tracked_data()).
Thanks,
Han
> > };
> >
> > /* Initialize the data for the engine nodes. It calls each node's
> > @@ -183,6 +195,12 @@ struct engine_node * engine_get_input(const char
*input_name,
> > /* Get the data from the input node with <name> for <node> */
> > void *engine_get_input_data(const char *input_name, struct engine_node
*);
> >
> > +void *engine_get_tracked_data(struct engine_node *);
> > +
> > +/* Get the tracked data from the input node with <name> for <node> */
> > +void *engine_get_input_tracked_data(const char *input_name,
> > + struct engine_node *);
> > +
> > /* Add an input (dependency) for <node>, with corresponding
change_handler,
> > * which can be NULL. If the change_handler is NULL, the engine will
not
> > * be able to process the change incrementally, and will fall back to
call
> > @@ -270,11 +288,13 @@ void engine_ovsdb_node_add_index(struct
engine_node *, const char *name,
> > struct engine_node en_##NAME = { \
> > .name = NAME_STR, \
> > .data = NULL, \
> > + .tracked_data = NULL, \
> > .state = EN_STALE, \
> > .init = en_##NAME##_init, \
> > .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) \
> >
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
