On Fri, May 29, 2020 at 1:31 AM Han Zhou <zhou...@gmail.com> wrote: > On Thu, May 28, 2020 at 12:15 PM Numan Siddique <num...@ovn.org> wrote: > > > > > > > > On Fri, May 29, 2020 at 12:29 AM Han Zhou <zhou...@gmail.com> wrote: > >> > >> 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 > > > > > > > > Thanks Han and Dumitru. > > > > Sure. I can have the tracked data maintained as part of the engine data. > > I'll address this in v10. I'll wait for sometime to incorporate any > comments if you > > have for other patches of the series before submitting v10. > > > > Thanks > > Numan > > > > Thanks Numan. I have some more comment for this patch, assuming it will add > interface for clear_tracked_data only. > > 1) I think it is better to pass the callback as argument for the node > definition, instead of dynamically assign it in init(). Since it is > optional and most node doesn't need it, it can be set to NULL in macro > ENGINE_NODE and ENGINE_NODE_CUSTOM_DATA, and define a new macro > ENGINE_NODE_WITH_CLEAR_TRACK to support clear data but no is_valid(). I > know this won't scale if we add more optional callbacks in the future. I am > not sure if there is a smarter way to do it. >
Sounds good to me. You mean an engine node which supports tracking can not support is_valid() ? > > 2) I think it is better be called by the engine_init_run(), instead of by > engine_run(). Although it doesn't make much difference today, but just in > case that in the future the tracked changes needs to be accessed after the > engine run, for some of the output nodes. It is safe if we always clear the > tracked data before the next engine_run() (which is the engine_init_run()). > > What do you think? > Ok. Since the tracked data will be part of the engine data and since the engine data can be accessed outside of the engine, I think it makes sense to clear it in engine_init_run(). Thanks Numan > >> > >> > > }; > >> > > > >> > > /* 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 > >> > _______________________________________________ > 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