[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 21:02:38 UTC 2020
On Thu, May 28, 2020 at 1:13 PM Numan Siddique <numans at ovn.org> wrote:
>
>
>
> On Fri, May 29, 2020 at 1:31 AM Han Zhou <zhouhan at gmail.com> wrote:
>>
>> 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.
>
>
> Sounds good to me. You mean an engine node which supports tracking
> can not support is_valid() ?
>
No I didn't mean that. If a node need both clear_tracked_data() and
is_valid(), it can use ENGINE_NODE_DEF. Right now I think this is ok, but
if there are more optional callbacks needed in the future we will need a
more clever way.
>
>>
>>
>> 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
>> >> > 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
>> >>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
More information about the dev
mailing list