On Wed, Jun 2, 2021 at 2:22 AM Han Zhou <[email protected]> wrote: > > On Tue, Jun 1, 2021 at 5:10 PM Numan Siddique <[email protected]> wrote: > > > > On Sun, May 30, 2021 at 11:45 PM <[email protected]> wrote: > > > > > > From: Numan Siddique <[email protected]> > > > > > > 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 <[email protected]> > > > Signed-off-by: Numan Siddique <[email protected]> > > > > 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
