Re: [ovs-dev] [PATCH v6 ovn 2/4] ovn-controller: Add per node states to I-P engine.
On Wed, Nov 27, 2019 at 2:24 AM Han Zhou wrote:
>
> Thanks Dumitru, please see my comments inlined.
Thanks Han for the review.
>
> On Fri, Nov 22, 2019 at 8:13 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().
> > - remove the need for using an engine_run_id by using the newly added
> > states.
> > - turn the global 'engine_abort_recompute' into an argument to be passed to
> > engine_run(). It's relevant only in the current run context anyway as
> > we reset it before every call to engine_run().
> >
> > Signed-off-by: Dumitru Ceara
> > ---
> > controller/ovn-controller.c | 84 ---
> > lib/inc-proc-eng.c | 242
> > ---
> > lib/inc-proc-eng.h | 74 +
> > 3 files changed, 276 insertions(+), 124 deletions(-)
> >
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index c56190f..a588531 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;
> > +
Re: [ovs-dev] [PATCH v6 ovn 2/4] ovn-controller: Add per node states to I-P engine.
Thanks Dumitru, please see my comments inlined.
On Fri, Nov 22, 2019 at 8:13 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().
> - remove the need for using an engine_run_id by using the newly added
states.
> - turn the global 'engine_abort_recompute' into an argument to be passed
to
> engine_run(). It's relevant only in the current run context anyway as
> we reset it before every call to engine_run().
>
> Signed-off-by: Dumitru Ceara
> ---
> controller/ovn-controller.c | 84 ---
> lib/inc-proc-eng.c | 242
---
> lib/inc-proc-eng.h | 74 +
> 3 files changed, 276 insertions(+), 124 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index c56190f..a588531 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 @@
