[ovs-dev] [PATCH ovn v9 4/8] I-P engine: Provide the option for an engine to store changed data
Han Zhou
zhouhan at gmail.com
Thu May 28 20:00:32 UTC 2020
On Thu, May 28, 2020 at 12:15 PM Numan Siddique <numans at ovn.org> wrote:
>
>
>
> On Fri, May 29, 2020 at 12:29 AM Han Zhou <zhouhan at gmail.com> wrote:
>>
>> On Thu, May 28, 2020 at 11:29 AM Dumitru Ceara <dceara at redhat.com> wrote:
>> >
>> > On 5/28/20 1:04 PM, numans at ovn.org wrote:
>> > > From: Numan Siddique <numans at 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 <mmichels at redhat.com>
>> > > Signed-off-by: Numan Siddique <numans at 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.
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?
>>
>> > > };
>> > >
>> > > /* 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
>> > dev at openvswitch.org
>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
More information about the dev
mailing list