[ovs-dev] [PATCH ovn v9 4/8] I-P engine: Provide the option for an engine to store changed data

Numan Siddique numans at ovn.org
Thu May 28 20:13:33 UTC 2020


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() ?



>
> 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