| Message ID | 20200528110517.1058340-1-numans@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Incremental processing improvements. | expand |
On Thu, May 28, 2020 at 4:06 AM <numans@ovn.org> wrote: > > From: Numan Siddique <numans@ovn.org> > > When ovn-controller updates the nb_cfg column of its chassis, > this results in full recomputation on all the nodes. This results > in wastage of CPU cycles. To address this, this patch handles > sbrec_chassis changes incrementally. > > We don't need to handle any sbrec_chassis changes during runtime_data > stage, because before engine_run() is called, encaps_run() is called > which will handle any chassis/encap changes. > > For new chassis addition and deletion, we need to add/delete flows for > the tunnel ports created/deleted. So physical_run() is called for this. > > For any chassis updates, we can ignore this for flow computation. > > This patch handles all these. > > Acked-by: Mark Michelson <mmichels@redhat.com> > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > controller/ovn-controller.c | 50 ++++++++++++++++++++++++++++++------- > 1 file changed, 41 insertions(+), 9 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 2a177ef9b..56744a39e 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -1461,6 +1461,7 @@ static void init_physical_ctx(struct engine_node *node, > engine_ovsdb_node_get_index( > engine_get_input("SB_chassis", node), > "name"); > + > if (chassis_id) { > chassis = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); > } > @@ -1536,8 +1537,8 @@ static void init_lflow_ctx(struct engine_node *node, > const struct sbrec_chassis *chassis = NULL; > struct ovsdb_idl_index *sbrec_chassis_by_name = > engine_ovsdb_node_get_index( > - engine_get_input("SB_chassis", node), > - "name"); > + engine_get_input("SB_chassis", node), > + "name"); > if (chassis_id) { > chassis = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); > } > @@ -1617,8 +1618,8 @@ en_flow_output_run(struct engine_node *node, void *data) > > struct ovsdb_idl_index *sbrec_chassis_by_name = > engine_ovsdb_node_get_index( > - engine_get_input("SB_chassis", node), > - "name"); > + engine_get_input("SB_chassis", node), > + "name"); > > const struct sbrec_chassis *chassis = NULL; > if (chassis_id) { > @@ -1775,8 +1776,8 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, > > struct ovsdb_idl_index *sbrec_chassis_by_name = > engine_ovsdb_node_get_index( > - engine_get_input("SB_chassis", node), > - "name"); > + engine_get_input("SB_chassis", node), > + "name"); > const struct sbrec_chassis *chassis = NULL; > if (chassis_id) { > chassis = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); > @@ -1948,6 +1949,31 @@ physical_flow_changes_ovs_iface_handler(struct engine_node *node, > return true; > } > > +/* Handles sbrec_chassis changes. > + * If a new chassis is added or removed return false, so that > + * physical flows are programmed. > + * For any updates, there is no need for any flow computation. Are we sure about this? At least I saw chassis->hostname, and chassis->other_config are used in physical_run(). For example: put_replace_router_port_mac_flows() -> chassis_get_mac() uses other_config:ovn-chassis-mac-mappings. If this is updated, the flows should change. For hostname, it is used here. if (ofport && requested_chassis && requested_chassis[0] && strcmp(requested_chassis, chassis->name) && strcmp(requested_chassis, chassis->hostname)) { /* Even though there is an ofport for this port_binding, it is * requested on a different chassis. So ignore this ofport. */ ofport = 0; } So if chassis->hostname changed, the flow change may be needed as well. There are also many places using chassis->name, but I assume name should never be changed so that should be ok. For every input, we need to check how they are used in full compute and then will know how to handle/ignore in change handlers. (Current test cases can't be relied because there are many corner cases not covered) (sorry that I didn't spot this when reviewing earlier versions) Thanks, Han > + * Encap changes will also result in sbrec_chassis changes, > + * but we handle encap changes separately. > + */ > +static bool > +physical_flow_changes_sb_chassis_handler(struct engine_node *node OVS_UNUSED, > + void *data OVS_UNUSED) > +{ > + struct sbrec_chassis_table *chassis_table = > + (struct sbrec_chassis_table *)EN_OVSDB_GET( > + engine_get_input("SB_chassis", node)); > + > + const struct sbrec_chassis *ch; > + SBREC_CHASSIS_TABLE_FOR_EACH_TRACKED (ch, chassis_table) { > + if (sbrec_chassis_is_deleted(ch) || sbrec_chassis_is_new(ch)) { > + return false; > + } > + } > + > + return true; > +} > + > static bool > flow_output_physical_flow_changes_handler(struct engine_node *node, void *data) > { > @@ -2200,6 +2226,10 @@ main(int argc, char *argv[]) > NULL); > engine_add_input(&en_physical_flow_changes, &en_ovs_interface, > physical_flow_changes_ovs_iface_handler); > + engine_add_input(&en_physical_flow_changes, &en_sb_chassis, > + physical_flow_changes_sb_chassis_handler); > + engine_add_input(&en_physical_flow_changes, &en_sb_encap, > + NULL); > > engine_add_input(&en_flow_output, &en_addr_sets, > flow_output_addr_sets_handler); > @@ -2218,9 +2248,10 @@ main(int argc, char *argv[]) > > 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, > + flow_output_noop_handler); > + 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, > @@ -2247,7 +2278,8 @@ main(int argc, char *argv[]) > runtime_data_ovs_interface_handler); > engine_add_input(&en_runtime_data, &en_ovs_qos, NULL); > > - engine_add_input(&en_runtime_data, &en_sb_chassis, NULL); > + engine_add_input(&en_runtime_data, &en_sb_chassis, > + runtime_data_noop_handler); > engine_add_input(&en_runtime_data, &en_sb_datapath_binding, > runtime_data_sb_datapath_binding_handler); > engine_add_input(&en_runtime_data, &en_sb_port_binding, > -- > 2.26.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Wed, Jun 3, 2020 at 11:43 AM Han Zhou <hzhou@ovn.org> wrote: > On Thu, May 28, 2020 at 4:06 AM <numans@ovn.org> wrote: > > > > From: Numan Siddique <numans@ovn.org> > > > > When ovn-controller updates the nb_cfg column of its chassis, > > this results in full recomputation on all the nodes. This results > > in wastage of CPU cycles. To address this, this patch handles > > sbrec_chassis changes incrementally. > > > > We don't need to handle any sbrec_chassis changes during runtime_data > > stage, because before engine_run() is called, encaps_run() is called > > which will handle any chassis/encap changes. > > > > For new chassis addition and deletion, we need to add/delete flows for > > the tunnel ports created/deleted. So physical_run() is called for this. > > > > For any chassis updates, we can ignore this for flow computation. > > > > This patch handles all these. > > > > Acked-by: Mark Michelson <mmichels@redhat.com> > > Signed-off-by: Numan Siddique <numans@ovn.org> > > --- > > controller/ovn-controller.c | 50 ++++++++++++++++++++++++++++++------- > > 1 file changed, 41 insertions(+), 9 deletions(-) > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 2a177ef9b..56744a39e 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -1461,6 +1461,7 @@ static void init_physical_ctx(struct engine_node > *node, > > engine_ovsdb_node_get_index( > > engine_get_input("SB_chassis", node), > > "name"); > > + > > if (chassis_id) { > > chassis = chassis_lookup_by_name(sbrec_chassis_by_name, > chassis_id); > > } > > @@ -1536,8 +1537,8 @@ static void init_lflow_ctx(struct engine_node > *node, > > const struct sbrec_chassis *chassis = NULL; > > struct ovsdb_idl_index *sbrec_chassis_by_name = > > engine_ovsdb_node_get_index( > > - engine_get_input("SB_chassis", node), > > - "name"); > > + engine_get_input("SB_chassis", node), > > + "name"); > > if (chassis_id) { > > chassis = chassis_lookup_by_name(sbrec_chassis_by_name, > chassis_id); > > } > > @@ -1617,8 +1618,8 @@ en_flow_output_run(struct engine_node *node, void > *data) > > > > struct ovsdb_idl_index *sbrec_chassis_by_name = > > engine_ovsdb_node_get_index( > > - engine_get_input("SB_chassis", node), > > - "name"); > > + engine_get_input("SB_chassis", node), > > + "name"); > > > > const struct sbrec_chassis *chassis = NULL; > > if (chassis_id) { > > @@ -1775,8 +1776,8 @@ _flow_output_resource_ref_handler(struct > engine_node *node, void *data, > > > > struct ovsdb_idl_index *sbrec_chassis_by_name = > > engine_ovsdb_node_get_index( > > - engine_get_input("SB_chassis", node), > > - "name"); > > + engine_get_input("SB_chassis", node), > > + "name"); > > const struct sbrec_chassis *chassis = NULL; > > if (chassis_id) { > > chassis = chassis_lookup_by_name(sbrec_chassis_by_name, > chassis_id); > > @@ -1948,6 +1949,31 @@ physical_flow_changes_ovs_iface_handler(struct > engine_node *node, > > return true; > > } > > > > +/* Handles sbrec_chassis changes. > > + * If a new chassis is added or removed return false, so that > > + * physical flows are programmed. > > + * For any updates, there is no need for any flow computation. > > Are we sure about this? At least I saw chassis->hostname, and > chassis->other_config are used in physical_run(). > For example: > put_replace_router_port_mac_flows() -> chassis_get_mac() uses > other_config:ovn-chassis-mac-mappings. If this is updated, the flows should > change. > > For hostname, it is used here. > if (ofport && requested_chassis && requested_chassis[0] && > strcmp(requested_chassis, chassis->name) && > strcmp(requested_chassis, chassis->hostname)) { > /* Even though there is an ofport for this port_binding, it is > * requested on a different chassis. So ignore this ofport. > */ > ofport = 0; > } > So if chassis->hostname changed, the flow change may be needed as well. > > There are also many places using chassis->name, but I assume name should > never be changed so that should be ok. > > For every input, we need to check how they are used in full compute and > then will know how to handle/ignore in change handlers. (Current test cases > can't be relied because there are many corner cases not covered) > > (sorry that I didn't spot this when reviewing earlier versions) > No worries. Thanks for the review. Great catch. I totally missed that. I'll explore further on this. If it is too complicated to handle, I'll just drop this patch from the series. Thanks Numan > > Thanks, > Han > > > + * Encap changes will also result in sbrec_chassis changes, > > + * but we handle encap changes separately. > > + */ > > +static bool > > +physical_flow_changes_sb_chassis_handler(struct engine_node *node > OVS_UNUSED, > > + void *data OVS_UNUSED) > > +{ > > + struct sbrec_chassis_table *chassis_table = > > + (struct sbrec_chassis_table *)EN_OVSDB_GET( > > + engine_get_input("SB_chassis", node)); > > + > > + const struct sbrec_chassis *ch; > > + SBREC_CHASSIS_TABLE_FOR_EACH_TRACKED (ch, chassis_table) { > > + if (sbrec_chassis_is_deleted(ch) || sbrec_chassis_is_new(ch)) { > > + return false; > > + } > > + } > > + > > + return true; > > +} > > + > > static bool > > flow_output_physical_flow_changes_handler(struct engine_node *node, void > *data) > > { > > @@ -2200,6 +2226,10 @@ main(int argc, char *argv[]) > > NULL); > > engine_add_input(&en_physical_flow_changes, &en_ovs_interface, > > physical_flow_changes_ovs_iface_handler); > > + engine_add_input(&en_physical_flow_changes, &en_sb_chassis, > > + physical_flow_changes_sb_chassis_handler); > > + engine_add_input(&en_physical_flow_changes, &en_sb_encap, > > + NULL); > > > > engine_add_input(&en_flow_output, &en_addr_sets, > > flow_output_addr_sets_handler); > > @@ -2218,9 +2248,10 @@ main(int argc, char *argv[]) > > > > 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, > > + flow_output_noop_handler); > > + 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, > > @@ -2247,7 +2278,8 @@ main(int argc, char *argv[]) > > runtime_data_ovs_interface_handler); > > engine_add_input(&en_runtime_data, &en_ovs_qos, NULL); > > > > - engine_add_input(&en_runtime_data, &en_sb_chassis, NULL); > > + engine_add_input(&en_runtime_data, &en_sb_chassis, > > + runtime_data_noop_handler); > > engine_add_input(&en_runtime_data, &en_sb_datapath_binding, > > runtime_data_sb_datapath_binding_handler); > > engine_add_input(&en_runtime_data, &en_sb_port_binding, > > -- > > 2.26.2 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 2a177ef9b..56744a39e 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1461,6 +1461,7 @@ static void init_physical_ctx(struct engine_node *node, engine_ovsdb_node_get_index( engine_get_input("SB_chassis", node), "name"); + if (chassis_id) { chassis = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); } @@ -1536,8 +1537,8 @@ static void init_lflow_ctx(struct engine_node *node, const struct sbrec_chassis *chassis = NULL; struct ovsdb_idl_index *sbrec_chassis_by_name = engine_ovsdb_node_get_index( - engine_get_input("SB_chassis", node), - "name"); + engine_get_input("SB_chassis", node), + "name"); if (chassis_id) { chassis = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); } @@ -1617,8 +1618,8 @@ en_flow_output_run(struct engine_node *node, void *data) struct ovsdb_idl_index *sbrec_chassis_by_name = engine_ovsdb_node_get_index( - engine_get_input("SB_chassis", node), - "name"); + engine_get_input("SB_chassis", node), + "name"); const struct sbrec_chassis *chassis = NULL; if (chassis_id) { @@ -1775,8 +1776,8 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, struct ovsdb_idl_index *sbrec_chassis_by_name = engine_ovsdb_node_get_index( - engine_get_input("SB_chassis", node), - "name"); + engine_get_input("SB_chassis", node), + "name"); const struct sbrec_chassis *chassis = NULL; if (chassis_id) { chassis = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); @@ -1948,6 +1949,31 @@ physical_flow_changes_ovs_iface_handler(struct engine_node *node, return true; } +/* Handles sbrec_chassis changes. + * If a new chassis is added or removed return false, so that + * physical flows are programmed. + * For any updates, there is no need for any flow computation. + * Encap changes will also result in sbrec_chassis changes, + * but we handle encap changes separately. + */ +static bool +physical_flow_changes_sb_chassis_handler(struct engine_node *node OVS_UNUSED, + void *data OVS_UNUSED) +{ + struct sbrec_chassis_table *chassis_table = + (struct sbrec_chassis_table *)EN_OVSDB_GET( + engine_get_input("SB_chassis", node)); + + const struct sbrec_chassis *ch; + SBREC_CHASSIS_TABLE_FOR_EACH_TRACKED (ch, chassis_table) { + if (sbrec_chassis_is_deleted(ch) || sbrec_chassis_is_new(ch)) { + return false; + } + } + + return true; +} + static bool flow_output_physical_flow_changes_handler(struct engine_node *node, void *data) { @@ -2200,6 +2226,10 @@ main(int argc, char *argv[]) NULL); engine_add_input(&en_physical_flow_changes, &en_ovs_interface, physical_flow_changes_ovs_iface_handler); + engine_add_input(&en_physical_flow_changes, &en_sb_chassis, + physical_flow_changes_sb_chassis_handler); + engine_add_input(&en_physical_flow_changes, &en_sb_encap, + NULL); engine_add_input(&en_flow_output, &en_addr_sets, flow_output_addr_sets_handler); @@ -2218,9 +2248,10 @@ main(int argc, char *argv[]) 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, + flow_output_noop_handler); + 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, @@ -2247,7 +2278,8 @@ main(int argc, char *argv[]) runtime_data_ovs_interface_handler); engine_add_input(&en_runtime_data, &en_ovs_qos, NULL); - engine_add_input(&en_runtime_data, &en_sb_chassis, NULL); + engine_add_input(&en_runtime_data, &en_sb_chassis, + runtime_data_noop_handler); engine_add_input(&en_runtime_data, &en_sb_datapath_binding, runtime_data_sb_datapath_binding_handler); engine_add_input(&en_runtime_data, &en_sb_port_binding,