[ovs-dev] [PATCH ovn v8 1/4] ovn-controller: Split logical flow and physical flow processing.

Numan Siddique numans at ovn.org
Wed Jun 2 16:39:06 UTC 2021


On Wed, Jun 2, 2021 at 2:22 AM Han Zhou <hzhou at ovn.org> wrote:
>
> On Tue, Jun 1, 2021 at 5:10 PM Numan Siddique <numans at ovn.org> wrote:
> >
> > On Sun, May 30, 2021 at 11:45 PM <numans at ovn.org> wrote:
> > >
> > > From: Numan Siddique <numans at ovn.org>
> > >
> > > Presently, the 'flow_output' engine node recomputes physical
> > > flows by calling physical_run() in the 'physical_flow_changes'
> > > handler in some scenarios.  Because of this, an engine run can
> > > do a full recompute of physical flows but not full recompute
> > > of logical flows.  Although this works now, it is problematic
> > > as the same desired flow table is used for both physical and
> > > logical flows.
> > >
> > > This patch now separates the handling of logical flows and
> > > physical flows and removes the 'physical_flow_changes' engine
> > > node.  Two separate engine nodes are added - lflow_output and
> > > pflow_output with their own flow tables and these two nodes are
> > > now inputs to the main engine node - flow_output.  This separation
> > > reflects the data dependency more clearly.
> > >
> > > CC: Han Zhou <hzhou at ovn.org>
> > > Signed-off-by: Numan Siddique <numans at ovn.org>
> >
> > Hi Han,
> >
> > Gentle ping.  Wondering if you got the chance to take a look at the
> > first patch of the series.
> >
>
> Hi Numan,
>
> Thanks for the revision and sorry for the slow response. I see that you
> updated some change handlers from noop to NULL for pflow_output, which
> makes more sense now. However, I see there are still several noop handlers
> for lflow_output node.
> When we use a noop handler, it means that the input is used for compute the
> output (otherwise it wouldn't be a dependency), but we know that the
> related change is handled *indirectly* when handling another input. I think
> this should be the only situation we use noop handler, and when using it we
> need to document where is the change handled. Please see my comments
> inlined for each of the input below.
>
> > It would be great if the first patch can be considered before we
> > branch (or before the 21.06 release).
> >
> I understand this patch has been hanging around for quite long, but since
> it is more of a refactoring than feature or bug-fix, I'd in fact prefer
> merging it *after* the release, together with the related patches in the
> series, because of the experiences we had before for big changes in I-P
> (including many of my patches) that caused regressions. When we are
> confident enough we could still backport it to 21.06 when necessary. What
> do you think?

Hi Han,

Thanks for the review comments.  I was hoping to get this patch in.
But I agree.  We can consider these patches after the branch and
if they are stable may be backport later.  I am more interesting in
other patches in this series which are required for scale setups.


>
>
> > > @@ -2731,58 +2691,71 @@ main(int argc, char *argv[])
> > >      engine_add_input(&en_port_groups, &en_runtime_data,
> > >                       port_groups_runtime_data_handler);
> > >
> > > -    /* Engine node physical_flow_changes indicates whether
> > > -     * we can recompute only physical flows or we can
> > > -     * incrementally process the physical flows.
> > > -     *
> > > -     * Note: The order of inputs is important, all OVS interface
> changes must
> > > +    /* Note: The order of inputs is important, all OVS interface
> changes must
> > >       * be handled before any ct_zone changes.
> > >       */
> > > -    engine_add_input(&en_physical_flow_changes, &en_ovs_interface,
> > > -                     physical_flow_changes_ovs_iface_handler);
> > > -    engine_add_input(&en_physical_flow_changes, &en_ct_zones,
> > > -                     physical_flow_changes_ct_zones_handler);
> > > -
> > > -    engine_add_input(&en_flow_output, &en_addr_sets,
> > > -                     flow_output_addr_sets_handler);
> > > -    engine_add_input(&en_flow_output, &en_port_groups,
> > > -                     flow_output_port_groups_handler);
> > > -    engine_add_input(&en_flow_output, &en_runtime_data,
> > > -                     flow_output_runtime_data_handler);
> > > -    engine_add_input(&en_flow_output, &en_mff_ovn_geneve, NULL);
> > > -    engine_add_input(&en_flow_output, &en_physical_flow_changes,
> > > -                     flow_output_physical_flow_changes_handler);
> > > +    engine_add_input(&en_pflow_output, &en_ovs_interface,
> > > +                     pflow_output_ovs_iface_handler);
> > > +    engine_add_input(&en_pflow_output, &en_ct_zones,
> > > +                     NULL);
> > > +    engine_add_input(&en_pflow_output, &en_sb_chassis,
> > > +                     pflow_output_sb_chassis_handler);
> > > +    engine_add_input(&en_pflow_output, &en_sb_port_binding,
> > > +                     pflow_output_sb_port_binding_handler);
> > > +    engine_add_input(&en_pflow_output, &en_sb_multicast_group,
> > > +                     pflow_output_sb_multicast_group_handler);
> > > +
> > > +    engine_add_input(&en_pflow_output, &en_runtime_data,
> > > +                     NULL);
> > > +    engine_add_input(&en_pflow_output, &en_sb_encap, NULL);
> > > +    engine_add_input(&en_pflow_output, &en_mff_ovn_geneve, NULL);
> > > +    engine_add_input(&en_pflow_output, &en_ovs_open_vswitch, NULL);
> > > +    engine_add_input(&en_pflow_output, &en_ovs_bridge, NULL);
> > > +
> > > +    engine_add_input(&en_lflow_output, &en_addr_sets,
> > > +                     lflow_output_addr_sets_handler);
> > > +    engine_add_input(&en_lflow_output, &en_port_groups,
> > > +                     lflow_output_port_groups_handler);
> > > +    engine_add_input(&en_lflow_output, &en_runtime_data,
> > > +                     lflow_output_runtime_data_handler);
> > >
> > >      /* We need this input nodes for only data. Hence the noop handler.
> */
> > > -    engine_add_input(&en_flow_output, &en_ct_zones,
> engine_noop_handler);
> > > -    engine_add_input(&en_flow_output, &en_ovs_interface,
> engine_noop_handler);
> > > -
> > > -    engine_add_input(&en_flow_output, &en_ovs_open_vswitch, NULL);
> > > -    engine_add_input(&en_flow_output, &en_ovs_bridge, NULL);
> > > -
> > > -    engine_add_input(&en_flow_output, &en_sb_chassis, NULL);
> > > -    engine_add_input(&en_flow_output, &en_sb_encap, NULL);
> > > -    engine_add_input(&en_flow_output, &en_sb_multicast_group,
> > > -                     flow_output_sb_multicast_group_handler);
> > > -    engine_add_input(&en_flow_output, &en_sb_port_binding,
> > > -                     flow_output_sb_port_binding_handler);
> > > -    engine_add_input(&en_flow_output, &en_sb_mac_binding,
> > > -                     flow_output_sb_mac_binding_handler);
> > > -    engine_add_input(&en_flow_output, &en_sb_logical_flow,
> > > -                     flow_output_sb_logical_flow_handler);
> > > +    engine_add_input(&en_lflow_output, &en_ct_zones,
> > > +                     engine_noop_handler);
>
> It seems ct_zones is for physical flow computing, and we handles it by
> recomputing physical flows. I didn't see it used for logical flow
> computing. So can we remove it from the dependency?

I agree.  I'll remove in v9.

>
> > > +    engine_add_input(&en_lflow_output, &en_ovs_interface,
> > > +                     engine_noop_handler);
>
> Same for ovs_interface, I didn't see it used for lflow_output. Shall we
> remove it?

Ack.

>
> > > +    engine_add_input(&en_lflow_output, &en_sb_chassis,
> > > +                     engine_noop_handler);
>
> This one doesn't seem to justify a noop handler. It is used for lflow
> computing and we cannot handle the changes incrementally (for now), so we
> should use NULL just to trigger recompute.

Ok.  I thought that handling the chassis changs in pflow_output is enough.
In v9 I will make sb_chassis input handler as NULL for both pflow_output
and lflow_output and in the next patch add a handler for both pflow and lflow.

>
> > > +    engine_add_input(&en_lflow_output, &en_sb_multicast_group,
> > > +                     engine_noop_handler);
> > >
> > > +    /* Any changes to the port binding, need not be handled
> > > +     * for lflow_outout engine.  We still need sb_port_binding
> > > +     * as input to access the port binding data in lflow.c and
> > > +     * hence the noop handler. */
> > > +    engine_add_input(&en_lflow_output, &en_sb_port_binding,
> > > +                     engine_noop_handler);
>
> For multicast_group and port_binding, we didn't handle it previously, but
> it is not correct. I submitted a patch series last week. Please help review
> it and probably it can be incorporated to this patch later if it looks ok.
> https://patchwork.ozlabs.org/project/ovn/list/?series=246286

Ok.  For v9 I'll leave AS IS.  I think either you can update your patch series
or I can update mine depending on whose patches get in first.

Thanks
Numan

>
> Thanks,
> Han
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list