diff mbox series

[ovs-dev,v2,2/3] ovn-controller: Remove the run_id check in engine_run

Message ID 20190624105920.15725-1-nusiddiq@redhat.com
State Not Applicable
Headers show
Series ovn-controller: Some IP improvements | expand

Commit Message

Numan Siddique June 24, 2019, 10:59 a.m. UTC 
From: Numan Siddique <nusiddiq@redhat.com>

engine_node 'en_sb_port_binding' is added as input to engine nodes
  - 'en_runtime_data' with the handler runtime_data_sb_port_binding_handler() and
  - 'en_flow_output' with the handler flow_output_sb_port_binding_handler() nodes.

Also 'en_runtime_data' is input to node 'en_flow_output'.

The function 'engine_run()' returns immediately if the run_id param is same as
the engine_node->run_id. Because of which the handler 'flow_output_sb_port_binding_handler()'
is never called.

This patch removes this check in engine_run().

Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
---
 ovn/lib/inc-proc-eng.c | 3 ---
 1 file changed, 3 deletions(-)

Comments

Han Zhou June 24, 2019, 5:09 p.m. UTC | #1 
On Mon, Jun 24, 2019 at 3:59 AM <nusiddiq@redhat.com> wrote:
>
> From: Numan Siddique <nusiddiq@redhat.com>
>
> engine_node 'en_sb_port_binding' is added as input to engine nodes
>   - 'en_runtime_data' with the handler
runtime_data_sb_port_binding_handler() and
>   - 'en_flow_output' with the handler
flow_output_sb_port_binding_handler() nodes.
>
> Also 'en_runtime_data' is input to node 'en_flow_output'.
>
> The function 'engine_run()' returns immediately if the run_id param is
same as
> the engine_node->run_id. Because of which the handler
'flow_output_sb_port_binding_handler()'
> is never called.
>
> This patch removes this check in engine_run().
>
> Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
> ---
>  ovn/lib/inc-proc-eng.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/ovn/lib/inc-proc-eng.c b/ovn/lib/inc-proc-eng.c
> index 1ddea1a85..10ebd047b 100644
> --- a/ovn/lib/inc-proc-eng.c
> +++ b/ovn/lib/inc-proc-eng.c
> @@ -124,9 +124,6 @@ engine_ovsdb_node_add_index(struct engine_node *node,
const char *name,
>  void
>  engine_run(struct engine_node *node, uint64_t run_id)
>  {
> -    if (node->run_id == run_id) {
> -        return;
> -    }
>      node->run_id = run_id;
>
>      node->changed = false;
> --
> 2.21.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

I think this change causes problems. engine_run() is called recursively
when traversing the DAG. The check for run_id is just to make sure each
node is visited once only. When a node is visited, it will process each of
its input, and none of the input change handlers will be missed (unless
there is some input triggers recompute of this node already and the rest of
inputs are omitted). You didn't see 'flow_output_sb_port_binding_handler()'
called maybe because in en_runtime_data it is already triggering recompute?
Please let me know if I missed anything.
Numan Siddique June 24, 2019, 5:53 p.m. UTC | #2 
On Mon, Jun 24, 2019 at 10:39 PM Han Zhou <zhouhan@gmail.com> wrote:

>
>
> On Mon, Jun 24, 2019 at 3:59 AM <nusiddiq@redhat.com> wrote:
> >
> > From: Numan Siddique <nusiddiq@redhat.com>
> >
> > engine_node 'en_sb_port_binding' is added as input to engine nodes
> >   - 'en_runtime_data' with the handler
> runtime_data_sb_port_binding_handler() and
> >   - 'en_flow_output' with the handler
> flow_output_sb_port_binding_handler() nodes.
> >
> > Also 'en_runtime_data' is input to node 'en_flow_output'.
> >
> > The function 'engine_run()' returns immediately if the run_id param is
> same as
> > the engine_node->run_id. Because of which the handler
> 'flow_output_sb_port_binding_handler()'
> > is never called.
> >
> > This patch removes this check in engine_run().
> >
> > Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
> > ---
> >  ovn/lib/inc-proc-eng.c | 3 ---
> >  1 file changed, 3 deletions(-)
> >
> > diff --git a/ovn/lib/inc-proc-eng.c b/ovn/lib/inc-proc-eng.c
> > index 1ddea1a85..10ebd047b 100644
> > --- a/ovn/lib/inc-proc-eng.c
> > +++ b/ovn/lib/inc-proc-eng.c
> > @@ -124,9 +124,6 @@ engine_ovsdb_node_add_index(struct engine_node
> *node, const char *name,
> >  void
> >  engine_run(struct engine_node *node, uint64_t run_id)
> >  {
> > -    if (node->run_id == run_id) {
> > -        return;
> > -    }
> >      node->run_id = run_id;
> >
> >      node->changed = false;
> > --
> > 2.21.0
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> I think this change causes problems. engine_run() is called recursively
> when traversing the DAG. The check for run_id is just to make sure each
> node is visited once only. When a node is visited, it will process each of
> its input, and none of the input change handlers will be missed (unless
> there is some input triggers recompute of this node already and the rest of
> inputs are omitted). You didn't see 'flow_output_sb_port_binding_handler()'
> called maybe because in en_runtime_data it is already triggering recompute?
> Please let me know if I missed anything.
>

I had a doubt about this patch. Nonetheless submitted to get your inputs.
Probably with the fix in patch 3, I should see
flow_output_sb_port_binding_handler() being called right ?
I will test it out again without this patch in the series and see if
flow_output_sb_port_binding_handler() is getting called or not.

Thanks
Numan
Han Zhou June 24, 2019, 7:13 p.m. UTC | #3 
On Mon, Jun 24, 2019 at 10:53 AM Numan Siddique <nusiddiq@redhat.com> wrote:
>
>
>
> On Mon, Jun 24, 2019 at 10:39 PM Han Zhou <zhouhan@gmail.com> wrote:
>>
>>
>>
>> On Mon, Jun 24, 2019 at 3:59 AM <nusiddiq@redhat.com> wrote:
>> >
>> > From: Numan Siddique <nusiddiq@redhat.com>
>> >
>> > engine_node 'en_sb_port_binding' is added as input to engine nodes
>> >   - 'en_runtime_data' with the handler
runtime_data_sb_port_binding_handler() and
>> >   - 'en_flow_output' with the handler
flow_output_sb_port_binding_handler() nodes.
>> >
>> > Also 'en_runtime_data' is input to node 'en_flow_output'.
>> >
>> > The function 'engine_run()' returns immediately if the run_id param is
same as
>> > the engine_node->run_id. Because of which the handler
'flow_output_sb_port_binding_handler()'
>> > is never called.
>> >
>> > This patch removes this check in engine_run().
>> >
>> > Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
>> > ---
>> >  ovn/lib/inc-proc-eng.c | 3 ---
>> >  1 file changed, 3 deletions(-)
>> >
>> > diff --git a/ovn/lib/inc-proc-eng.c b/ovn/lib/inc-proc-eng.c
>> > index 1ddea1a85..10ebd047b 100644
>> > --- a/ovn/lib/inc-proc-eng.c
>> > +++ b/ovn/lib/inc-proc-eng.c
>> > @@ -124,9 +124,6 @@ engine_ovsdb_node_add_index(struct engine_node
*node, const char *name,
>> >  void
>> >  engine_run(struct engine_node *node, uint64_t run_id)
>> >  {
>> > -    if (node->run_id == run_id) {
>> > -        return;
>> > -    }
>> >      node->run_id = run_id;
>> >
>> >      node->changed = false;
>> > --
>> > 2.21.0
>> >
>> > _______________________________________________
>> > dev mailing list
>> > dev@openvswitch.org
>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>> I think this change causes problems. engine_run() is called recursively
when traversing the DAG. The check for run_id is just to make sure each
node is visited once only. When a node is visited, it will process each of
its input, and none of the input change handlers will be missed (unless
there is some input triggers recompute of this node already and the rest of
inputs are omitted). You didn't see 'flow_output_sb_port_binding_handler()'
called maybe because in en_runtime_data it is already triggering recompute?
Please let me know if I missed anything.
>
>
> I had a doubt about this patch. Nonetheless submitted to get your
inputs.  Probably with the fix in patch 3, I should see
flow_output_sb_port_binding_handler() being called right ?
> I will test it out again without this patch in the series and see if
flow_output_sb_port_binding_handler() is getting called or not.
>
You should see it called even without patch 3, when you just bind a regular
VIF on other HVs.
diff mbox series

Patch

diff --git a/ovn/lib/inc-proc-eng.c b/ovn/lib/inc-proc-eng.c
index 1ddea1a85..10ebd047b 100644
--- a/ovn/lib/inc-proc-eng.c
+++ b/ovn/lib/inc-proc-eng.c
@@ -124,9 +124,6 @@  engine_ovsdb_node_add_index(struct engine_node *node, const char *name,
 void
 engine_run(struct engine_node *node, uint64_t run_id)
 {
-    if (node->run_id == run_id) {
-        return;
-    }
     node->run_id = run_id;
 
     node->changed = false;