Re: [ovs-dev] [PATCH v5 ovn 2/4] ovn-controller: Add per node states to I-P engine.
On Wed, Nov 20, 2019 at 9:21 AM Dumitru Ceara wrote:
>
> On Tue, Nov 19, 2019 at 7:16 PM Han Zhou wrote:
> >
> >
> >
> > On Tue, Nov 19, 2019 at 1:58 AM Dumitru Ceara wrote:
> > >
> > > On Tue, Nov 19, 2019 at 1:41 AM Han Zhou wrote:
> > > >
> > > > Thanks Dumitru. Please see my comments inline below.
> > > >
> > > > On Mon, Nov 18, 2019 at 6:07 AM Dumitru Ceara wrote:
> > > > >
> > > > > This commit transforms the 'changed' field in struct engine_node in a
> > > > > 'state' field. Possible node states are:
> > > > > - "Stale": data in the node is not up to date with the DB.
> > > > > - "Updated": data in the node is valid but was updated during
> > > > > the last run of the engine.
> > > > > - "Valid": data in the node is valid and didn't change during
> > > > > the last run of the engine.
> > > > > - "Aborted": during the last run, processing was aborted for
> > > > > this node.
> > > > >
> > > > > This commit also further refactors the I-P engine:
> > > > > - instead of recursively performing all the engine processing a
> > > > > preprocessing stage is added (engine_get_nodes()) before the main
> > > > > processing
> > > > > loop is executed in order to topologically sort nodes in the engine
> > > > > such
> > > > > that all inputs of a given node appear in the sorted array before
> > > > > the node
> > > > > itself. This simplifies a bit the code in engine_run().
> > > >
> > > > Could you tell the reason of changing it to non-recursive? It seems
> > > > adding more code rather than simplifying, and effort is needed to
> > > > ensure the correctness for the new code. Probably there are some
> > > > benefit that make the later patches easier, but it is not obvious to
> > > > me. Could you help point out if that's the case?
> > >
> > > My reasoning was that the engine graph is static (i.e., we build it
> > > once at startup and it never changes afterwards) so all recursion
> > > trees are always identical.
> > >
> > I agree that the graph is static, which makes non-recursive a good option,
> > but it doesn't necessarily mean it simplifies code than the recursive
> > version :)
>
> I might also just be my personal view that debugging recursive
> functions is more complicated than with iterative ones :)
>
> >
> > > Moreover, with adding engine node explicit states we don't really need
> > > to store a run_id in the nodes because we have the state of each node
> > > after an engine run. I think removing the run_id is a good idea
> > > because we minimize the external state the user of the engine should
> > > manage (in this case engine_run_id and it's incrementing logic).
> > >
> > > However, if we keep the engine processing in a recursive fashion then
> > > for each of the recursive operations (engine_run, engine_need_run,
> > > engine_init, engine_cleanup) we need a way to avoid executing the
> > > operation twice for a given node. This can be done by adding more
> > > flags to the nodes (or more state values) but given that our DAG is
> > > fixed, precomputing the processing order made more sense to me. What
> > > do you think?
> > >
> > It is true that run_id can be completely avoided with the non-recursive
> > version. However, it can be kept as internal variable for recursive
> > version, to skip repeated access of a node, if we use the engine_init_run()
> > to increment it internally. I think we don't really need any more flags
> > than the non-recursive version other than this internal run_id, and we can
> > remove the external engine_run_id incrementing logic for recursive version,
> > too.
>
> Yes, with an internal run_id this could work.
>
> >
> > > >
> > > > > - remove the need for using an engine_run_id by using the newly added
> > > > > states.
> > > > >
> > > > > Signed-off-by: Dumitru Ceara
> > > > > ---
> > > > > controller/ovn-controller.c | 88 ++---
> > > > > lib/inc-proc-eng.c | 219
> > > > > ---
> > > > > lib/inc-proc-eng.h | 75 +++
> > > > > 3 files changed, 271 insertions(+), 111 deletions(-)
> > > > >
> > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > > > > index c56190f..033eff4 100644
> > > > > --- a/controller/ovn-controller.c
> > > > > +++ b/controller/ovn-controller.c
> > > > > @@ -758,10 +758,10 @@ en_ofctrl_is_connected_run(struct engine_node
> > > > > *node)
> > > > > (struct ed_type_ofctrl_is_connected *)node->data;
> > > > > if (data->connected != ofctrl_is_connected()) {
> > > > > data->connected = !data->connected;
> > > > > -node->changed = true;
> > > > > +engine_set_node_state(node, EN_UPDATED);
> > > > > return;
> > > > > }
> > > > > -node->changed = false;
> > > > > +engine_set_node_state(node, EN_VALID);
> > > > > }
> > > > >
> > > > > struct ed_type_addr_sets {
> > > > > @@ -811,7 +811,7 @@ en_addr_sets_run(struct engine_node *node)
> > > > >
Re: [ovs-dev] [PATCH v5 ovn 2/4] ovn-controller: Add per node states to I-P engine.
On Tue, Nov 19, 2019 at 7:16 PM Han Zhou wrote:
>
>
>
> On Tue, Nov 19, 2019 at 1:58 AM Dumitru Ceara wrote:
> >
> > On Tue, Nov 19, 2019 at 1:41 AM Han Zhou wrote:
> > >
> > > Thanks Dumitru. Please see my comments inline below.
> > >
> > > On Mon, Nov 18, 2019 at 6:07 AM Dumitru Ceara wrote:
> > > >
> > > > This commit transforms the 'changed' field in struct engine_node in a
> > > > 'state' field. Possible node states are:
> > > > - "Stale": data in the node is not up to date with the DB.
> > > > - "Updated": data in the node is valid but was updated during
> > > > the last run of the engine.
> > > > - "Valid": data in the node is valid and didn't change during
> > > > the last run of the engine.
> > > > - "Aborted": during the last run, processing was aborted for
> > > > this node.
> > > >
> > > > This commit also further refactors the I-P engine:
> > > > - instead of recursively performing all the engine processing a
> > > > preprocessing stage is added (engine_get_nodes()) before the main
> > > > processing
> > > > loop is executed in order to topologically sort nodes in the engine
> > > > such
> > > > that all inputs of a given node appear in the sorted array before the
> > > > node
> > > > itself. This simplifies a bit the code in engine_run().
> > >
> > > Could you tell the reason of changing it to non-recursive? It seems
> > > adding more code rather than simplifying, and effort is needed to ensure
> > > the correctness for the new code. Probably there are some benefit that
> > > make the later patches easier, but it is not obvious to me. Could you
> > > help point out if that's the case?
> >
> > My reasoning was that the engine graph is static (i.e., we build it
> > once at startup and it never changes afterwards) so all recursion
> > trees are always identical.
> >
> I agree that the graph is static, which makes non-recursive a good option,
> but it doesn't necessarily mean it simplifies code than the recursive version
> :)
I might also just be my personal view that debugging recursive
functions is more complicated than with iterative ones :)
>
> > Moreover, with adding engine node explicit states we don't really need
> > to store a run_id in the nodes because we have the state of each node
> > after an engine run. I think removing the run_id is a good idea
> > because we minimize the external state the user of the engine should
> > manage (in this case engine_run_id and it's incrementing logic).
> >
> > However, if we keep the engine processing in a recursive fashion then
> > for each of the recursive operations (engine_run, engine_need_run,
> > engine_init, engine_cleanup) we need a way to avoid executing the
> > operation twice for a given node. This can be done by adding more
> > flags to the nodes (or more state values) but given that our DAG is
> > fixed, precomputing the processing order made more sense to me. What
> > do you think?
> >
> It is true that run_id can be completely avoided with the non-recursive
> version. However, it can be kept as internal variable for recursive version,
> to skip repeated access of a node, if we use the engine_init_run() to
> increment it internally. I think we don't really need any more flags than the
> non-recursive version other than this internal run_id, and we can remove the
> external engine_run_id incrementing logic for recursive version, too.
Yes, with an internal run_id this could work.
>
> > >
> > > > - remove the need for using an engine_run_id by using the newly added
> > > > states.
> > > >
> > > > Signed-off-by: Dumitru Ceara
> > > > ---
> > > > controller/ovn-controller.c | 88 ++---
> > > > lib/inc-proc-eng.c | 219
> > > > ---
> > > > lib/inc-proc-eng.h | 75 +++
> > > > 3 files changed, 271 insertions(+), 111 deletions(-)
> > > >
> > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > > > index c56190f..033eff4 100644
> > > > --- a/controller/ovn-controller.c
> > > > +++ b/controller/ovn-controller.c
> > > > @@ -758,10 +758,10 @@ en_ofctrl_is_connected_run(struct engine_node
> > > > *node)
> > > > (struct ed_type_ofctrl_is_connected *)node->data;
> > > > if (data->connected != ofctrl_is_connected()) {
> > > > data->connected = !data->connected;
> > > > -node->changed = true;
> > > > +engine_set_node_state(node, EN_UPDATED);
> > > > return;
> > > > }
> > > > -node->changed = false;
> > > > +engine_set_node_state(node, EN_VALID);
> > > > }
> > > >
> > > > struct ed_type_addr_sets {
> > > > @@ -811,7 +811,7 @@ en_addr_sets_run(struct engine_node *node)
> > > > addr_sets_init(as_table, >addr_sets);
> > > >
> > > > as->change_tracked = false;
> > > > -node->changed = true;
> > > > +engine_set_node_state(node, EN_UPDATED);
> > > > }
> > > >
> > > > static bool
> > > > @@ -830,11 +830,14 @@
Re: [ovs-dev] [PATCH v5 ovn 2/4] ovn-controller: Add per node states to I-P engine.
On Tue, Nov 19, 2019 at 1:58 AM Dumitru Ceara wrote:
>
> On Tue, Nov 19, 2019 at 1:41 AM Han Zhou wrote:
> >
> > Thanks Dumitru. Please see my comments inline below.
> >
> > On Mon, Nov 18, 2019 at 6:07 AM Dumitru Ceara wrote:
> > >
> > > This commit transforms the 'changed' field in struct engine_node in a
> > > 'state' field. Possible node states are:
> > > - "Stale": data in the node is not up to date with the DB.
> > > - "Updated": data in the node is valid but was updated during
> > > the last run of the engine.
> > > - "Valid": data in the node is valid and didn't change during
> > > the last run of the engine.
> > > - "Aborted": during the last run, processing was aborted for
> > > this node.
> > >
> > > This commit also further refactors the I-P engine:
> > > - instead of recursively performing all the engine processing a
> > > preprocessing stage is added (engine_get_nodes()) before the main
processing
> > > loop is executed in order to topologically sort nodes in the engine
such
> > > that all inputs of a given node appear in the sorted array before
the node
> > > itself. This simplifies a bit the code in engine_run().
> >
> > Could you tell the reason of changing it to non-recursive? It seems
adding more code rather than simplifying, and effort is needed to ensure
the correctness for the new code. Probably there are some benefit that make
the later patches easier, but it is not obvious to me. Could you help point
out if that's the case?
>
> My reasoning was that the engine graph is static (i.e., we build it
> once at startup and it never changes afterwards) so all recursion
> trees are always identical.
>
I agree that the graph is static, which makes non-recursive a good option,
but it doesn't necessarily mean it simplifies code than the recursive
version :)
> Moreover, with adding engine node explicit states we don't really need
> to store a run_id in the nodes because we have the state of each node
> after an engine run. I think removing the run_id is a good idea
> because we minimize the external state the user of the engine should
> manage (in this case engine_run_id and it's incrementing logic).
>
> However, if we keep the engine processing in a recursive fashion then
> for each of the recursive operations (engine_run, engine_need_run,
> engine_init, engine_cleanup) we need a way to avoid executing the
> operation twice for a given node. This can be done by adding more
> flags to the nodes (or more state values) but given that our DAG is
> fixed, precomputing the processing order made more sense to me. What
> do you think?
>
It is true that run_id can be completely avoided with the non-recursive
version. However, it can be kept as internal variable for recursive
version, to skip repeated access of a node, if we use the engine_init_run()
to increment it internally. I think we don't really need any more flags
than the non-recursive version other than this internal run_id, and we can
remove the external engine_run_id incrementing logic for recursive version,
too.
> >
> > > - remove the need for using an engine_run_id by using the newly added
states.
> > >
> > > Signed-off-by: Dumitru Ceara
> > > ---
> > > controller/ovn-controller.c | 88 ++---
> > > lib/inc-proc-eng.c | 219
---
> > > lib/inc-proc-eng.h | 75 +++
> > > 3 files changed, 271 insertions(+), 111 deletions(-)
> > >
> > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > > index c56190f..033eff4 100644
> > > --- a/controller/ovn-controller.c
> > > +++ b/controller/ovn-controller.c
> > > @@ -758,10 +758,10 @@ en_ofctrl_is_connected_run(struct engine_node
*node)
> > > (struct ed_type_ofctrl_is_connected *)node->data;
> > > if (data->connected != ofctrl_is_connected()) {
> > > data->connected = !data->connected;
> > > -node->changed = true;
> > > +engine_set_node_state(node, EN_UPDATED);
> > > return;
> > > }
> > > -node->changed = false;
> > > +engine_set_node_state(node, EN_VALID);
> > > }
> > >
> > > struct ed_type_addr_sets {
> > > @@ -811,7 +811,7 @@ en_addr_sets_run(struct engine_node *node)
> > > addr_sets_init(as_table, >addr_sets);
> > >
> > > as->change_tracked = false;
> > > -node->changed = true;
> > > +engine_set_node_state(node, EN_UPDATED);
> > > }
> > >
> > > static bool
> > > @@ -830,11 +830,14 @@ addr_sets_sb_address_set_handler(struct
engine_node *node)
> > > addr_sets_update(as_table, >addr_sets, >new,
> > > >deleted, >updated);
> > >
> > > -node->changed = !sset_is_empty(>new) ||
!sset_is_empty(>deleted)
> > > -|| !sset_is_empty(>updated);
> > > +if (!sset_is_empty(>new) || !sset_is_empty(>deleted) ||
> > > +!sset_is_empty(>updated)) {
> > > +engine_set_node_state(node, EN_UPDATED);
> > > +} else {
> > > +
Re: [ovs-dev] [PATCH v5 ovn 2/4] ovn-controller: Add per node states to I-P engine.
On Tue, Nov 19, 2019 at 1:41 AM Han Zhou wrote:
>
> Thanks Dumitru. Please see my comments inline below.
>
> On Mon, Nov 18, 2019 at 6:07 AM Dumitru Ceara wrote:
> >
> > This commit transforms the 'changed' field in struct engine_node in a
> > 'state' field. Possible node states are:
> > - "Stale": data in the node is not up to date with the DB.
> > - "Updated": data in the node is valid but was updated during
> > the last run of the engine.
> > - "Valid": data in the node is valid and didn't change during
> > the last run of the engine.
> > - "Aborted": during the last run, processing was aborted for
> > this node.
> >
> > This commit also further refactors the I-P engine:
> > - instead of recursively performing all the engine processing a
> > preprocessing stage is added (engine_get_nodes()) before the main
> > processing
> > loop is executed in order to topologically sort nodes in the engine such
> > that all inputs of a given node appear in the sorted array before the node
> > itself. This simplifies a bit the code in engine_run().
>
> Could you tell the reason of changing it to non-recursive? It seems adding
> more code rather than simplifying, and effort is needed to ensure the
> correctness for the new code. Probably there are some benefit that make the
> later patches easier, but it is not obvious to me. Could you help point out
> if that's the case?
My reasoning was that the engine graph is static (i.e., we build it
once at startup and it never changes afterwards) so all recursion
trees are always identical.
Moreover, with adding engine node explicit states we don't really need
to store a run_id in the nodes because we have the state of each node
after an engine run. I think removing the run_id is a good idea
because we minimize the external state the user of the engine should
manage (in this case engine_run_id and it's incrementing logic).
However, if we keep the engine processing in a recursive fashion then
for each of the recursive operations (engine_run, engine_need_run,
engine_init, engine_cleanup) we need a way to avoid executing the
operation twice for a given node. This can be done by adding more
flags to the nodes (or more state values) but given that our DAG is
fixed, precomputing the processing order made more sense to me. What
do you think?
>
> > - remove the need for using an engine_run_id by using the newly added
> > states.
> >
> > Signed-off-by: Dumitru Ceara
> > ---
> > controller/ovn-controller.c | 88 ++---
> > lib/inc-proc-eng.c | 219
> > ---
> > lib/inc-proc-eng.h | 75 +++
> > 3 files changed, 271 insertions(+), 111 deletions(-)
> >
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index c56190f..033eff4 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -758,10 +758,10 @@ en_ofctrl_is_connected_run(struct engine_node *node)
> > (struct ed_type_ofctrl_is_connected *)node->data;
> > if (data->connected != ofctrl_is_connected()) {
> > data->connected = !data->connected;
> > -node->changed = true;
> > +engine_set_node_state(node, EN_UPDATED);
> > return;
> > }
> > -node->changed = false;
> > +engine_set_node_state(node, EN_VALID);
> > }
> >
> > struct ed_type_addr_sets {
> > @@ -811,7 +811,7 @@ en_addr_sets_run(struct engine_node *node)
> > addr_sets_init(as_table, >addr_sets);
> >
> > as->change_tracked = false;
> > -node->changed = true;
> > +engine_set_node_state(node, EN_UPDATED);
> > }
> >
> > static bool
> > @@ -830,11 +830,14 @@ addr_sets_sb_address_set_handler(struct engine_node
> > *node)
> > addr_sets_update(as_table, >addr_sets, >new,
> > >deleted, >updated);
> >
> > -node->changed = !sset_is_empty(>new) ||
> > !sset_is_empty(>deleted)
> > -|| !sset_is_empty(>updated);
> > +if (!sset_is_empty(>new) || !sset_is_empty(>deleted) ||
> > +!sset_is_empty(>updated)) {
> > +engine_set_node_state(node, EN_UPDATED);
> > +} else {
> > +engine_set_node_state(node, EN_VALID);
> > +}
> >
> > as->change_tracked = true;
> > -node->changed = true;
> > return true;
> > }
> >
> > @@ -885,7 +888,7 @@ en_port_groups_run(struct engine_node *node)
> > port_groups_init(pg_table, >port_groups);
> >
> > pg->change_tracked = false;
> > -node->changed = true;
> > +engine_set_node_state(node, EN_UPDATED);
> > }
> >
> > static bool
> > @@ -904,11 +907,14 @@ port_groups_sb_port_group_handler(struct engine_node
> > *node)
> > port_groups_update(pg_table, >port_groups, >new,
> > >deleted, >updated);
> >
> > -node->changed = !sset_is_empty(>new) ||
> > !sset_is_empty(>deleted)
> > -|| !sset_is_empty(>updated);
> > +if (!sset_is_empty(>new) ||
Re: [ovs-dev] [PATCH v5 ovn 2/4] ovn-controller: Add per node states to I-P engine.
Thanks Dumitru. Please see my comments inline below.
On Mon, Nov 18, 2019 at 6:07 AM Dumitru Ceara wrote:
>
> This commit transforms the 'changed' field in struct engine_node in a
> 'state' field. Possible node states are:
> - "Stale": data in the node is not up to date with the DB.
> - "Updated": data in the node is valid but was updated during
> the last run of the engine.
> - "Valid": data in the node is valid and didn't change during
> the last run of the engine.
> - "Aborted": during the last run, processing was aborted for
> this node.
>
> This commit also further refactors the I-P engine:
> - instead of recursively performing all the engine processing a
> preprocessing stage is added (engine_get_nodes()) before the main
processing
> loop is executed in order to topologically sort nodes in the engine such
> that all inputs of a given node appear in the sorted array before the
node
> itself. This simplifies a bit the code in engine_run().
Could you tell the reason of changing it to non-recursive? It seems adding
more code rather than simplifying, and effort is needed to ensure the
correctness for the new code. Probably there are some benefit that make the
later patches easier, but it is not obvious to me. Could you help point out
if that's the case?
> - remove the need for using an engine_run_id by using the newly added
states.
>
> Signed-off-by: Dumitru Ceara
> ---
> controller/ovn-controller.c | 88 ++---
> lib/inc-proc-eng.c | 219
---
> lib/inc-proc-eng.h | 75 +++
> 3 files changed, 271 insertions(+), 111 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index c56190f..033eff4 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -758,10 +758,10 @@ en_ofctrl_is_connected_run(struct engine_node *node)
> (struct ed_type_ofctrl_is_connected *)node->data;
> if (data->connected != ofctrl_is_connected()) {
> data->connected = !data->connected;
> -node->changed = true;
> +engine_set_node_state(node, EN_UPDATED);
> return;
> }
> -node->changed = false;
> +engine_set_node_state(node, EN_VALID);
> }
>
> struct ed_type_addr_sets {
> @@ -811,7 +811,7 @@ en_addr_sets_run(struct engine_node *node)
> addr_sets_init(as_table, >addr_sets);
>
> as->change_tracked = false;
> -node->changed = true;
> +engine_set_node_state(node, EN_UPDATED);
> }
>
> static bool
> @@ -830,11 +830,14 @@ addr_sets_sb_address_set_handler(struct engine_node
*node)
> addr_sets_update(as_table, >addr_sets, >new,
> >deleted, >updated);
>
> -node->changed = !sset_is_empty(>new) ||
!sset_is_empty(>deleted)
> -|| !sset_is_empty(>updated);
> +if (!sset_is_empty(>new) || !sset_is_empty(>deleted) ||
> +!sset_is_empty(>updated)) {
> +engine_set_node_state(node, EN_UPDATED);
> +} else {
> +engine_set_node_state(node, EN_VALID);
> +}
>
> as->change_tracked = true;
> -node->changed = true;
> return true;
> }
>
> @@ -885,7 +888,7 @@ en_port_groups_run(struct engine_node *node)
> port_groups_init(pg_table, >port_groups);
>
> pg->change_tracked = false;
> -node->changed = true;
> +engine_set_node_state(node, EN_UPDATED);
> }
>
> static bool
> @@ -904,11 +907,14 @@ port_groups_sb_port_group_handler(struct
engine_node *node)
> port_groups_update(pg_table, >port_groups, >new,
> >deleted, >updated);
>
> -node->changed = !sset_is_empty(>new) ||
!sset_is_empty(>deleted)
> -|| !sset_is_empty(>updated);
> +if (!sset_is_empty(>new) || !sset_is_empty(>deleted) ||
> +!sset_is_empty(>updated)) {
> +engine_set_node_state(node, EN_UPDATED);
> +} else {
> +engine_set_node_state(node, EN_VALID);
> +}
>
> pg->change_tracked = true;
> -node->changed = true;
> return true;
> }
>
> @@ -1091,7 +1097,7 @@ en_runtime_data_run(struct engine_node *node)
> update_ct_zones(local_lports, local_datapaths, ct_zones,
> ct_zone_bitmap, pending_ct_zones);
>
> -node->changed = true;
> +engine_set_node_state(node, EN_UPDATED);
> }
>
> static bool
> @@ -1157,10 +1163,10 @@ en_mff_ovn_geneve_run(struct engine_node *node)
> enum mf_field_id mff_ovn_geneve = ofctrl_get_mf_field_id();
> if (data->mff_ovn_geneve != mff_ovn_geneve) {
> data->mff_ovn_geneve = mff_ovn_geneve;
> -node->changed = true;
> +engine_set_node_state(node, EN_UPDATED);
> return;
> }
> -node->changed = false;
> +engine_set_node_state(node, EN_VALID);
> }
>
> struct ed_type_flow_output {
> @@ -1322,7 +1328,7 @@ en_flow_output_run(struct engine_node *node)
> active_tunnels,
> flow_table);
>
> -node->changed =
[ovs-dev] [PATCH v5 ovn 2/4] ovn-controller: Add per node states to I-P engine.
This commit transforms the 'changed' field in struct engine_node in a
'state' field. Possible node states are:
- "Stale": data in the node is not up to date with the DB.
- "Updated": data in the node is valid but was updated during
the last run of the engine.
- "Valid": data in the node is valid and didn't change during
the last run of the engine.
- "Aborted": during the last run, processing was aborted for
this node.
This commit also further refactors the I-P engine:
- instead of recursively performing all the engine processing a
preprocessing stage is added (engine_get_nodes()) before the main processing
loop is executed in order to topologically sort nodes in the engine such
that all inputs of a given node appear in the sorted array before the node
itself. This simplifies a bit the code in engine_run().
- remove the need for using an engine_run_id by using the newly added states.
Signed-off-by: Dumitru Ceara
---
controller/ovn-controller.c | 88 ++---
lib/inc-proc-eng.c | 219 ---
lib/inc-proc-eng.h | 75 +++
3 files changed, 271 insertions(+), 111 deletions(-)
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index c56190f..033eff4 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -758,10 +758,10 @@ en_ofctrl_is_connected_run(struct engine_node *node)
(struct ed_type_ofctrl_is_connected *)node->data;
if (data->connected != ofctrl_is_connected()) {
data->connected = !data->connected;
-node->changed = true;
+engine_set_node_state(node, EN_UPDATED);
return;
}
-node->changed = false;
+engine_set_node_state(node, EN_VALID);
}
struct ed_type_addr_sets {
@@ -811,7 +811,7 @@ en_addr_sets_run(struct engine_node *node)
addr_sets_init(as_table, >addr_sets);
as->change_tracked = false;
-node->changed = true;
+engine_set_node_state(node, EN_UPDATED);
}
static bool
@@ -830,11 +830,14 @@ addr_sets_sb_address_set_handler(struct engine_node *node)
addr_sets_update(as_table, >addr_sets, >new,
>deleted, >updated);
-node->changed = !sset_is_empty(>new) || !sset_is_empty(>deleted)
-|| !sset_is_empty(>updated);
+if (!sset_is_empty(>new) || !sset_is_empty(>deleted) ||
+!sset_is_empty(>updated)) {
+engine_set_node_state(node, EN_UPDATED);
+} else {
+engine_set_node_state(node, EN_VALID);
+}
as->change_tracked = true;
-node->changed = true;
return true;
}
@@ -885,7 +888,7 @@ en_port_groups_run(struct engine_node *node)
port_groups_init(pg_table, >port_groups);
pg->change_tracked = false;
-node->changed = true;
+engine_set_node_state(node, EN_UPDATED);
}
static bool
@@ -904,11 +907,14 @@ port_groups_sb_port_group_handler(struct engine_node
*node)
port_groups_update(pg_table, >port_groups, >new,
>deleted, >updated);
-node->changed = !sset_is_empty(>new) || !sset_is_empty(>deleted)
-|| !sset_is_empty(>updated);
+if (!sset_is_empty(>new) || !sset_is_empty(>deleted) ||
+!sset_is_empty(>updated)) {
+engine_set_node_state(node, EN_UPDATED);
+} else {
+engine_set_node_state(node, EN_VALID);
+}
pg->change_tracked = true;
-node->changed = true;
return true;
}
@@ -1091,7 +1097,7 @@ en_runtime_data_run(struct engine_node *node)
update_ct_zones(local_lports, local_datapaths, ct_zones,
ct_zone_bitmap, pending_ct_zones);
-node->changed = true;
+engine_set_node_state(node, EN_UPDATED);
}
static bool
@@ -1157,10 +1163,10 @@ en_mff_ovn_geneve_run(struct engine_node *node)
enum mf_field_id mff_ovn_geneve = ofctrl_get_mf_field_id();
if (data->mff_ovn_geneve != mff_ovn_geneve) {
data->mff_ovn_geneve = mff_ovn_geneve;
-node->changed = true;
+engine_set_node_state(node, EN_UPDATED);
return;
}
-node->changed = false;
+engine_set_node_state(node, EN_VALID);
}
struct ed_type_flow_output {
@@ -1322,7 +1328,7 @@ en_flow_output_run(struct engine_node *node)
active_tunnels,
flow_table);
-node->changed = true;
+engine_set_node_state(node, EN_UPDATED);
}
static bool
@@ -1404,7 +1410,7 @@ flow_output_sb_logical_flow_handler(struct engine_node
*node)
flow_table, group_table, meter_table, lfrr,
conj_id_ofs);
-node->changed = true;
+engine_set_node_state(node, EN_UPDATED);
return handled;
}
@@ -1427,7 +1433,7 @@ flow_output_sb_mac_binding_handler(struct engine_node
*node)
lflow_handle_changed_neighbors(sbrec_port_binding_by_name,
mac_binding_table, flow_table);
-node->changed = true;
+engine_set_node_state(node, EN_UPDATED);
return true;
}
@@
