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? > > @@ -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? > > + 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? > > + 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. > > + 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 Thanks, Han _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
