| Message ID | 166316094939.13381.14940783736708105661.stgit@dceara.remote.csb |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Refactor and simplify a bit ovn-controller I-P code. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Wed, Sep 14, 2022 at 3:09 PM Dumitru Ceara <dceara@redhat.com> wrote: > The nd_ra_opts and controller_event_ops are actually static they never > change at runtime. DHCP records can instead be computed when populating > the lflow "input context" to be used during incremental processing. This > is likely more efficient than building the DHCP opts maps for every logical > flow recomputed incrementally. > > An even more efficient solution would be to introduce proper incremental > processing for the DHCP opt maps but that seems like too much complexity > without enough benefit. It's probably OK to recompute the maps at every > ovn-controller iteration. > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > controller/lflow.c | 206 > +++---------------------------------------- > controller/lflow.h | 9 +- > controller/ovn-controller.c | 128 +++++++++++++++++++-------- > lib/ovn-l7.h | 2 > 4 files changed, 110 insertions(+), 235 deletions(-) > > diff --git a/controller/lflow.c b/controller/lflow.c > index eef44389f..de9f17b9a 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c > @@ -90,9 +90,6 @@ add_matches_to_flow_table(const struct > sbrec_logical_flow *, > struct lflow_ctx_out *); > static void > consider_logical_flow(const struct sbrec_logical_flow *lflow, > - struct hmap *dhcp_opts, struct hmap *dhcpv6_opts, > - struct hmap *nd_ra_opts, > - struct controller_event_options > *controller_event_opts, > bool is_recompute, > struct lflow_ctx_in *l_ctx_in, > struct lflow_ctx_out *l_ctx_out); > @@ -371,40 +368,9 @@ add_logical_flows(struct lflow_ctx_in *l_ctx_in, > struct lflow_ctx_out *l_ctx_out) > { > const struct sbrec_logical_flow *lflow; > - > - struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); > - struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); > - const struct sbrec_dhcp_options *dhcp_opt_row; > - SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, > - l_ctx_in->dhcp_options_table) { > - dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code, > - dhcp_opt_row->type); > - } > - > - > - const struct sbrec_dhcpv6_options *dhcpv6_opt_row; > - SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row, > - l_ctx_in->dhcpv6_options_table) { > - dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, > dhcpv6_opt_row->code, > - dhcpv6_opt_row->type); > - } > - > - struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); > - nd_ra_opts_init(&nd_ra_opts); > - > - struct controller_event_options controller_event_opts; > - controller_event_opts_init(&controller_event_opts); > - > SBREC_LOGICAL_FLOW_TABLE_FOR_EACH (lflow, > l_ctx_in->logical_flow_table) { > - consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, > - &nd_ra_opts, &controller_event_opts, true, > - l_ctx_in, l_ctx_out); > + consider_logical_flow(lflow, true, l_ctx_in, l_ctx_out); > } > - > - dhcp_opts_destroy(&dhcp_opts); > - dhcp_opts_destroy(&dhcpv6_opts); > - nd_ra_opts_destroy(&nd_ra_opts); > - controller_event_opts_destroy(&controller_event_opts); > } > > bool > @@ -414,29 +380,6 @@ lflow_handle_changed_flows(struct lflow_ctx_in > *l_ctx_in, > bool ret = true; > const struct sbrec_logical_flow *lflow; > > - struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); > - struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); > - const struct sbrec_dhcp_options *dhcp_opt_row; > - SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, > - l_ctx_in->dhcp_options_table) { > - dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code, > - dhcp_opt_row->type); > - } > - > - > - const struct sbrec_dhcpv6_options *dhcpv6_opt_row; > - SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row, > - l_ctx_in->dhcpv6_options_table) { > - dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, > dhcpv6_opt_row->code, > - dhcpv6_opt_row->type); > - } > - > - struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); > - nd_ra_opts_init(&nd_ra_opts); > - > - struct controller_event_options controller_event_opts; > - controller_event_opts_init(&controller_event_opts); > - > /* Flood remove the flows for all the tracked lflows. Its possible > that > * lflow_add_flows_for_datapath() may have been called before calling > * this function. */ > @@ -486,9 +429,7 @@ lflow_handle_changed_flows(struct lflow_ctx_in > *l_ctx_in, > lflows_processed_remove(l_ctx_out->lflows_processed, > lfp_node); > } > > - consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, > - &nd_ra_opts, &controller_event_opts, > false, > - l_ctx_in, l_ctx_out); > + consider_logical_flow(lflow, false, l_ctx_in, l_ctx_out); > } > } > HMAP_FOR_EACH_SAFE (ofrn, hmap_node, &flood_remove_nodes) { > @@ -497,10 +438,6 @@ lflow_handle_changed_flows(struct lflow_ctx_in > *l_ctx_in, > } > hmap_destroy(&flood_remove_nodes); > > - dhcp_opts_destroy(&dhcp_opts); > - dhcp_opts_destroy(&dhcpv6_opts); > - nd_ra_opts_destroy(&nd_ra_opts); > - controller_event_opts_destroy(&controller_event_opts); > return ret; > } > > @@ -556,10 +493,6 @@ consider_lflow_for_added_as_ips__( > const char *as_name, > size_t as_ref_count, > const struct expr_constant_set *as_diff_added, > - struct hmap *dhcp_opts, > - struct hmap *dhcpv6_opts, > - struct hmap *nd_ra_opts, > - struct controller_event_options > *controller_event_opts, > struct lflow_ctx_in *l_ctx_in, > struct lflow_ctx_out *l_ctx_out) > { > @@ -588,10 +521,6 @@ consider_lflow_for_added_as_ips__( > struct ofpbuf ovnacts = OFPBUF_STUB_INITIALIZER(ovnacts_stub); > struct ovnact_parse_params pp = { > .symtab = &symtab, > - .dhcp_opts = dhcp_opts, > - .dhcpv6_opts = dhcpv6_opts, > - .nd_ra_opts = nd_ra_opts, > - .controller_event_opts = controller_event_opts, > > .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS, > .n_tables = LOG_PIPELINE_LEN, > @@ -751,10 +680,6 @@ consider_lflow_for_added_as_ips( > const char *as_name, > size_t as_ref_count, > const struct expr_constant_set *as_diff_added, > - struct hmap *dhcp_opts, > - struct hmap *dhcpv6_opts, > - struct hmap *nd_ra_opts, > - struct controller_event_options > *controller_event_opts, > struct lflow_ctx_in *l_ctx_in, > struct lflow_ctx_out *l_ctx_out) > { > @@ -771,17 +696,12 @@ consider_lflow_for_added_as_ips( > if (dp) { > return consider_lflow_for_added_as_ips__(lflow, dp, as_name, > as_ref_count, > as_diff_added, > - dhcp_opts, dhcpv6_opts, > - nd_ra_opts, > - controller_event_opts, > l_ctx_in, l_ctx_out); > } > for (size_t i = 0; dp_group && i < dp_group->n_datapaths; i++) { > if (!consider_lflow_for_added_as_ips__(lflow, > dp_group->datapaths[i], > as_name, as_ref_count, > - as_diff_added, dhcp_opts, > - dhcpv6_opts, nd_ra_opts, > - controller_event_opts, > l_ctx_in, > + as_diff_added, l_ctx_in, > l_ctx_out)) { > return false; > } > @@ -886,30 +806,6 @@ lflow_handle_addr_set_update(const char *as_name, > > *changed = false; > > - struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); > - struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); > - struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); > - struct controller_event_options controller_event_opts; > - > - if (as_diff->added) { > - const struct sbrec_dhcp_options *dhcp_opt_row; > - SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, > - l_ctx_in->dhcp_options_table) { > - dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, > dhcp_opt_row->code, > - dhcp_opt_row->type); > - } > - > - const struct sbrec_dhcpv6_options *dhcpv6_opt_row; > - SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH(dhcpv6_opt_row, > - > l_ctx_in->dhcpv6_options_table) { > - dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, > - dhcpv6_opt_row->code, dhcpv6_opt_row->type); > - } > - > - nd_ra_opts_init(&nd_ra_opts); > - controller_event_opts_init(&controller_event_opts); > - } > - > bool ret = true; > struct lflow_ref_list_node *lrln; > HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) { > @@ -951,9 +847,7 @@ lflow_handle_addr_set_update(const char *as_name, > if (as_diff->added) { > if (!consider_lflow_for_added_as_ips(lflow, as_name, > lrln->ref_count, > - as_diff->added, > &dhcp_opts, > - &dhcpv6_opts, > &nd_ra_opts, > - &controller_event_opts, > + as_diff->added, > l_ctx_in, l_ctx_out)) { > ret = false; > goto done; > @@ -962,12 +856,6 @@ lflow_handle_addr_set_update(const char *as_name, > } > > done: > - if (as_diff->added) { > - dhcp_opts_destroy(&dhcp_opts); > - dhcp_opts_destroy(&dhcpv6_opts); > - nd_ra_opts_destroy(&nd_ra_opts); > - controller_event_opts_destroy(&controller_event_opts); > - } > return ret; > } > > @@ -1008,28 +896,6 @@ lflow_handle_changed_ref(enum ref_type ref_type, > const char *ref_name, > } > *changed = true; > > - struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); > - struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); > - const struct sbrec_dhcp_options *dhcp_opt_row; > - SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, > - l_ctx_in->dhcp_options_table) { > - dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code, > - dhcp_opt_row->type); > - } > - > - const struct sbrec_dhcpv6_options *dhcpv6_opt_row; > - SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH(dhcpv6_opt_row, > - l_ctx_in->dhcpv6_options_table) { > - dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, > dhcpv6_opt_row->code, > - dhcpv6_opt_row->type); > - } > - > - struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); > - nd_ra_opts_init(&nd_ra_opts); > - > - struct controller_event_options controller_event_opts; > - controller_event_opts_init(&controller_event_opts); > - > /* Re-parse the related lflows. */ > /* Firstly, flood remove the flows from desired flow table. */ > struct hmap flood_remove_nodes = > HMAP_INITIALIZER(&flood_remove_nodes); > @@ -1072,9 +938,7 @@ lflow_handle_changed_ref(enum ref_type ref_type, > const char *ref_name, > lflows_processed_remove(l_ctx_out->lflows_processed, > lfp_node); > } > > - consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, > - &nd_ra_opts, &controller_event_opts, false, > - l_ctx_in, l_ctx_out); > + consider_logical_flow(lflow, false, l_ctx_in, l_ctx_out); > } > HMAP_FOR_EACH_SAFE (ofrn, hmap_node, &flood_remove_nodes) { > hmap_remove(&flood_remove_nodes, &ofrn->hmap_node); > @@ -1082,10 +946,6 @@ lflow_handle_changed_ref(enum ref_type ref_type, > const char *ref_name, > } > hmap_destroy(&flood_remove_nodes); > > - dhcp_opts_destroy(&dhcp_opts); > - dhcp_opts_destroy(&dhcpv6_opts); > - nd_ra_opts_destroy(&nd_ra_opts); > - controller_event_opts_destroy(&controller_event_opts); > return ret; > } > > @@ -1308,9 +1168,6 @@ convert_match_to_expr(const struct > sbrec_logical_flow *lflow, > static void > consider_logical_flow__(const struct sbrec_logical_flow *lflow, > const struct sbrec_datapath_binding *dp, > - struct hmap *dhcp_opts, struct hmap *dhcpv6_opts, > - struct hmap *nd_ra_opts, > - struct controller_event_options > *controller_event_opts, > struct lflow_ctx_in *l_ctx_in, > struct lflow_ctx_out *l_ctx_out) > { > @@ -1362,10 +1219,10 @@ consider_logical_flow__(const struct > sbrec_logical_flow *lflow, > struct ofpbuf ovnacts = OFPBUF_STUB_INITIALIZER(ovnacts_stub); > struct ovnact_parse_params pp = { > .symtab = &symtab, > - .dhcp_opts = dhcp_opts, > - .dhcpv6_opts = dhcpv6_opts, > - .nd_ra_opts = nd_ra_opts, > - .controller_event_opts = controller_event_opts, > + .dhcp_opts = &l_ctx_in->dhcp_opts, > + .dhcpv6_opts = &l_ctx_in->dhcpv6_opts, > + .nd_ra_opts = l_ctx_in->nd_ra_opts, > + .controller_event_opts = l_ctx_in->controller_event_opts, > > .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS, > .n_tables = LOG_PIPELINE_LEN, > @@ -1580,9 +1437,6 @@ lflows_processed_destroy(struct hmap > *lflows_processed) > > static void > consider_logical_flow(const struct sbrec_logical_flow *lflow, > - struct hmap *dhcp_opts, struct hmap *dhcpv6_opts, > - struct hmap *nd_ra_opts, > - struct controller_event_options > *controller_event_opts, > bool is_recompute, > struct lflow_ctx_in *l_ctx_in, > struct lflow_ctx_out *l_ctx_out) > @@ -1606,16 +1460,11 @@ consider_logical_flow(const struct > sbrec_logical_flow *lflow, > } > > if (dp) { > - consider_logical_flow__(lflow, dp, > - dhcp_opts, dhcpv6_opts, nd_ra_opts, > - controller_event_opts, > - l_ctx_in, l_ctx_out); > + consider_logical_flow__(lflow, dp, l_ctx_in, l_ctx_out); > return; > } > for (size_t i = 0; dp_group && i < dp_group->n_datapaths; i++) { > consider_logical_flow__(lflow, dp_group->datapaths[i], > - dhcp_opts, dhcpv6_opts, nd_ra_opts, > - controller_event_opts, > l_ctx_in, l_ctx_out); > } > } > @@ -2618,28 +2467,6 @@ lflow_add_flows_for_datapath(const struct > sbrec_datapath_binding *dp, > struct lflow_ctx_out *l_ctx_out) > { > bool handled = true; > - struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); > - struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); > - const struct sbrec_dhcp_options *dhcp_opt_row; > - SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, > - l_ctx_in->dhcp_options_table) { > - dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code, > - dhcp_opt_row->type); > - } > - > - > - const struct sbrec_dhcpv6_options *dhcpv6_opt_row; > - SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row, > - l_ctx_in->dhcpv6_options_table) { > - dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, > dhcpv6_opt_row->code, > - dhcpv6_opt_row->type); > - } > - > - struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); > - nd_ra_opts_init(&nd_ra_opts); > - > - struct controller_event_options controller_event_opts; > - controller_event_opts_init(&controller_event_opts); > > struct sbrec_logical_flow *lf_row = sbrec_logical_flow_index_init_row( > l_ctx_in->sbrec_logical_flow_by_logical_datapath); > @@ -2654,9 +2481,7 @@ lflow_add_flows_for_datapath(const struct > sbrec_datapath_binding *dp, > } > lflows_processed_add(l_ctx_out->lflows_processed, > &lflow->header_.uuid); > - consider_logical_flow__(lflow, dp, &dhcp_opts, &dhcpv6_opts, > - &nd_ra_opts, &controller_event_opts, > - l_ctx_in, l_ctx_out); > + consider_logical_flow__(lflow, dp, l_ctx_in, l_ctx_out); > } > sbrec_logical_flow_index_destroy_row(lf_row); > > @@ -2687,9 +2512,7 @@ lflow_add_flows_for_datapath(const struct > sbrec_datapath_binding *dp, > /* Don't call lflows_processed_add() because here we process > the > * lflow only for one of the DPs in the DP group, which may be > * incomplete. */ > - consider_logical_flow__(lflow, dp, &dhcp_opts, &dhcpv6_opts, > - &nd_ra_opts, &controller_event_opts, > - l_ctx_in, l_ctx_out); > + consider_logical_flow__(lflow, dp, l_ctx_in, l_ctx_out); > } > } > sbrec_logical_flow_index_destroy_row(lf_row); > @@ -2731,11 +2554,6 @@ lflow_add_flows_for_datapath(const struct > sbrec_datapath_binding *dp, > } > sbrec_static_mac_binding_index_destroy_row(smb_index_row); > > - dhcp_opts_destroy(&dhcp_opts); > - dhcp_opts_destroy(&dhcpv6_opts); > - nd_ra_opts_destroy(&nd_ra_opts); > - controller_event_opts_destroy(&controller_event_opts); > - > /* Add load balancer hairpin flows if the datapath has any load > balancers > * associated. */ > for (size_t i = 0; i < n_dp_lbs; i++) { > diff --git a/controller/lflow.h b/controller/lflow.h > index 543d3cd96..3d1bb38dd 100644 > --- a/controller/lflow.h > +++ b/controller/lflow.h > @@ -46,8 +46,6 @@ struct ovn_desired_flow_table; > struct hmap; > struct hmap_node; > struct sbrec_chassis; > -struct sbrec_dhcp_options_table; > -struct sbrec_dhcpv6_options_table; > struct sbrec_load_balancer; > struct sbrec_logical_flow_table; > struct sbrec_mac_binding_table; > @@ -144,8 +142,6 @@ struct lflow_ctx_in { > struct ovsdb_idl_index *sbrec_mac_binding_by_datapath; > struct ovsdb_idl_index *sbrec_static_mac_binding_by_datapath; > const struct sbrec_port_binding_table *port_binding_table; > - const struct sbrec_dhcp_options_table *dhcp_options_table; > - const struct sbrec_dhcpv6_options_table *dhcpv6_options_table; > const struct sbrec_datapath_binding_table *dp_binding_table; > const struct sbrec_mac_binding_table *mac_binding_table; > const struct sbrec_logical_flow_table *logical_flow_table; > @@ -162,7 +158,12 @@ struct lflow_ctx_in { > const struct sset *related_lport_ids; > const struct shash *binding_lports; > const struct hmap *chassis_tunnels; > + const struct hmap *nd_ra_opts; > + const struct controller_event_options *controller_event_opts; > bool lb_hairpin_use_ct_mark; > + > + struct hmap dhcp_opts; > + struct hmap dhcpv6_opts; > }; > > struct lflow_ctx_out { > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 6bedb91dc..18a01bbab 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -76,6 +76,7 @@ > #include "timer.h" > #include "stopwatch.h" > #include "lib/inc-proc-eng.h" > +#include "lib/ovn-l7.h" > #include "hmapx.h" > > VLOG_DEFINE_THIS_MODULE(main); > @@ -2538,6 +2539,12 @@ struct ed_type_lflow_output { > > /* Data for managing hairpin flow conjunctive flow ids. */ > struct lflow_output_hairpin_data hd; > + > + /* Fixed neighbor discovery supported options. */ > + struct hmap nd_ra_opts; > + > + /* Fixed controller_event supported options. */ > + struct controller_event_options controller_event_opts; > }; > > static void > @@ -2655,8 +2662,6 @@ init_lflow_ctx(struct engine_node *node, > l_ctx_in->sbrec_static_mac_binding_by_datapath = > sbrec_static_mac_binding_by_datapath; > l_ctx_in->port_binding_table = port_binding_table; > - l_ctx_in->dhcp_options_table = dhcp_table; > - l_ctx_in->dhcpv6_options_table = dhcpv6_table; > l_ctx_in->mac_binding_table = mac_binding_table; > l_ctx_in->logical_flow_table = logical_flow_table; > l_ctx_in->logical_dp_group_table = logical_dp_group_table; > @@ -2673,6 +2678,22 @@ init_lflow_ctx(struct engine_node *node, > l_ctx_in->binding_lports = &rt_data->lbinding_data.lports; > l_ctx_in->chassis_tunnels = &non_vif_data->chassis_tunnels; > l_ctx_in->lb_hairpin_use_ct_mark = n_opts->lb_hairpin_use_ct_mark; > + l_ctx_in->nd_ra_opts = &fo->nd_ra_opts; > + l_ctx_in->controller_event_opts = &fo->controller_event_opts; > + > + hmap_init(&l_ctx_in->dhcp_opts); > + const struct sbrec_dhcp_options *dhcp_opt_row; > + SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, dhcp_table) { > + dhcp_opt_add(&l_ctx_in->dhcp_opts, dhcp_opt_row->name, > + dhcp_opt_row->code, dhcp_opt_row->type); > + } > + > + hmap_init(&l_ctx_in->dhcpv6_opts); > + const struct sbrec_dhcpv6_options *dhcpv6_opt_row; > + SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row, dhcpv6_table) { > + dhcp_opt_add(&l_ctx_in->dhcpv6_opts, dhcpv6_opt_row->name, > + dhcpv6_opt_row->code, dhcpv6_opt_row->type); > + } > > l_ctx_out->flow_table = &fo->flow_table; > l_ctx_out->group_table = &fo->group_table; > @@ -2685,6 +2706,14 @@ init_lflow_ctx(struct engine_node *node, > l_ctx_out->hairpin_lb_ids = &fo->hd.ids; > } > > +static void > +destroy_lflow_ctx(struct lflow_ctx_in *l_ctx_in, > + struct lflow_ctx_out *l_ctx_out OVS_UNUSED) > +{ > + dhcp_opts_destroy(&l_ctx_in->dhcp_opts); > + dhcp_opts_destroy(&l_ctx_in->dhcpv6_opts); > +} > + > static void * > en_lflow_output_init(struct engine_node *node OVS_UNUSED, > struct engine_arg *arg OVS_UNUSED) > @@ -2698,6 +2727,8 @@ en_lflow_output_init(struct engine_node *node > OVS_UNUSED, > hmap_init(&data->lflows_processed); > simap_init(&data->hd.ids); > data->hd.pool = id_pool_create(1, UINT32_MAX - 1); > + nd_ra_opts_init(&data->nd_ra_opts); > + controller_event_opts_init(&data->controller_event_opts); > return data; > } > > @@ -2722,6 +2753,8 @@ en_lflow_output_cleanup(void *data) > lflow_cache_destroy(flow_output_data->pd.lflow_cache); > simap_destroy(&flow_output_data->hd.ids); > id_pool_destroy(flow_output_data->hd.pool); > + nd_ra_opts_destroy(&flow_output_data->nd_ra_opts); > + > controller_event_opts_destroy(&flow_output_data->controller_event_opts); > } > > static void > @@ -2771,6 +2804,7 @@ en_lflow_output_run(struct engine_node *node, void > *data) > struct lflow_ctx_out l_ctx_out; > init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); > lflow_run(&l_ctx_in, &l_ctx_out); > + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); > > engine_set_node_state(node, EN_UPDATED); > } > @@ -2784,6 +2818,7 @@ lflow_output_sb_logical_flow_handler(struct > engine_node *node, void *data) > init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); > > bool handled = lflow_handle_changed_flows(&l_ctx_in, &l_ctx_out); > + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); > > engine_set_node_state(node, EN_UPDATED); > return handled; > @@ -2846,12 +2881,12 @@ lflow_output_sb_multicast_group_handler(struct > engine_node *node, void *data) > struct lflow_ctx_in l_ctx_in; > struct lflow_ctx_out l_ctx_out; > init_lflow_ctx(node, lfo, &l_ctx_in, &l_ctx_out); > - if (!lflow_handle_changed_mc_groups(&l_ctx_in, &l_ctx_out)) { > - return false; > - } > + > + bool handled = lflow_handle_changed_mc_groups(&l_ctx_in, &l_ctx_out); > + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); > > engine_set_node_state(node, EN_UPDATED); > - return true; > + return handled; > } > > static bool > @@ -2862,12 +2897,12 @@ lflow_output_sb_port_binding_handler(struct > engine_node *node, void *data) > struct lflow_ctx_in l_ctx_in; > struct lflow_ctx_out l_ctx_out; > init_lflow_ctx(node, lfo, &l_ctx_in, &l_ctx_out); > - if (!lflow_handle_changed_port_bindings(&l_ctx_in, &l_ctx_out)) { > - return false; > - } > + > + bool handled = lflow_handle_changed_port_bindings(&l_ctx_in, > &l_ctx_out); > + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); > > engine_set_node_state(node, EN_UPDATED); > - return true; > + return handled; > } > > static bool > @@ -2876,23 +2911,24 @@ lflow_output_addr_sets_handler(struct engine_node > *node, void *data) > struct ed_type_addr_sets *as_data = > engine_get_input_data("addr_sets", node); > > - struct ed_type_lflow_output *fo = data; > + if (!as_data->change_tracked) { > + return false; > + } > > - struct lflow_ctx_in l_ctx_in; > + struct ed_type_lflow_output *fo = data; > struct lflow_ctx_out l_ctx_out; > - init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); > - > - bool changed; > + struct lflow_ctx_in l_ctx_in; > const char *ref_name; > + bool handled = true; > + bool changed; > > - if (!as_data->change_tracked) { > - return false; > - } > + init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); > > SSET_FOR_EACH (ref_name, &as_data->deleted) { > if (!lflow_handle_changed_ref(REF_TYPE_ADDRSET, ref_name, > &l_ctx_in, > &l_ctx_out, &changed)) { > - return false; > + handled = false; > + goto done; > } > if (changed) { > engine_set_node_state(node, EN_UPDATED); > @@ -2907,7 +2943,8 @@ lflow_output_addr_sets_handler(struct engine_node > *node, void *data) > " Reprocess related lflows.", shash_node->name); > if (!lflow_handle_changed_ref(REF_TYPE_ADDRSET, > shash_node->name, > &l_ctx_in, &l_ctx_out, > &changed)) { > - return false; > + handled = false; > + goto done; > } > } > if (changed) { > @@ -2917,14 +2954,17 @@ lflow_output_addr_sets_handler(struct engine_node > *node, void *data) > SSET_FOR_EACH (ref_name, &as_data->new) { > if (!lflow_handle_changed_ref(REF_TYPE_ADDRSET, ref_name, > &l_ctx_in, > &l_ctx_out, &changed)) { > - return false; > + handled = false; > + goto done; > } > if (changed) { > engine_set_node_state(node, EN_UPDATED); > } > } > > - return true; > +done: > + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); > + return handled; > } > > static bool > @@ -2933,23 +2973,24 @@ lflow_output_port_groups_handler(struct > engine_node *node, void *data) > struct ed_type_port_groups *pg_data = > engine_get_input_data("port_groups", node); > > - struct ed_type_lflow_output *fo = data; > + if (!pg_data->change_tracked) { > + return false; > + } > > - struct lflow_ctx_in l_ctx_in; > + struct ed_type_lflow_output *fo = data; > struct lflow_ctx_out l_ctx_out; > - init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); > - > - bool changed; > + struct lflow_ctx_in l_ctx_in; > const char *ref_name; > + bool handled = true; > + bool changed; > > - if (!pg_data->change_tracked) { > - return false; > - } > + init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); > > SSET_FOR_EACH (ref_name, &pg_data->deleted) { > if (!lflow_handle_changed_ref(REF_TYPE_PORTGROUP, ref_name, > &l_ctx_in, > &l_ctx_out, &changed)) { > - return false; > + handled = false; > + goto done; > } > if (changed) { > engine_set_node_state(node, EN_UPDATED); > @@ -2958,7 +2999,8 @@ lflow_output_port_groups_handler(struct engine_node > *node, void *data) > SSET_FOR_EACH (ref_name, &pg_data->updated) { > if (!lflow_handle_changed_ref(REF_TYPE_PORTGROUP, ref_name, > &l_ctx_in, > &l_ctx_out, &changed)) { > - return false; > + handled = false; > + goto done; > } > if (changed) { > engine_set_node_state(node, EN_UPDATED); > @@ -2967,14 +3009,17 @@ lflow_output_port_groups_handler(struct > engine_node *node, void *data) > SSET_FOR_EACH (ref_name, &pg_data->new) { > if (!lflow_handle_changed_ref(REF_TYPE_PORTGROUP, ref_name, > &l_ctx_in, > &l_ctx_out, &changed)) { > - return false; > + handled = false; > + goto done; > } > if (changed) { > engine_set_node_state(node, EN_UPDATED); > } > } > > - return true; > +done: > + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); > + return handled; > } > > static bool > @@ -3002,6 +3047,8 @@ lflow_output_runtime_data_handler(struct engine_node > *node, > struct lflow_ctx_out l_ctx_out; > struct ed_type_lflow_output *fo = data; > struct hmap *lbs = NULL; > + bool handled = true; > + > init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); > > struct tracked_datapath *tdp; > @@ -3018,7 +3065,8 @@ lflow_output_runtime_data_handler(struct engine_node > *node, > tdp->dp, lbs_by_dp ? lbs_by_dp->dp_lbs : NULL, > lbs_by_dp ? lbs_by_dp->n_dp_lbs : 0, > &l_ctx_in, &l_ctx_out)) { > - return false; > + handled = false; > + goto done; > } > } > struct shash_node *shash_node; > @@ -3026,14 +3074,18 @@ lflow_output_runtime_data_handler(struct > engine_node *node, > struct tracked_lport *lport = shash_node->data; > if (!lflow_handle_flows_for_lport(lport->pb, &l_ctx_in, > &l_ctx_out)) { > - return false; > + handled = false; > + goto done; > } > } > } > > load_balancers_by_dp_cleanup(lbs); > engine_set_node_state(node, EN_UPDATED); > - return true; > + > +done: > + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); > + return handled; > } > > static bool > @@ -3045,6 +3097,7 @@ lflow_output_sb_load_balancer_handler(struct > engine_node *node, void *data) > init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); > > bool handled = lflow_handle_changed_lbs(&l_ctx_in, &l_ctx_out); > + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); > > engine_set_node_state(node, EN_UPDATED); > return handled; > @@ -3059,6 +3112,7 @@ lflow_output_sb_fdb_handler(struct engine_node > *node, void *data) > init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); > > bool handled = lflow_handle_changed_fdbs(&l_ctx_in, &l_ctx_out); > + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); > > engine_set_node_state(node, EN_UPDATED); > return handled; > diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h > index 0b2da9f7b..2c7d0add4 100644 > --- a/lib/ovn-l7.h > +++ b/lib/ovn-l7.h > @@ -412,6 +412,8 @@ nd_ra_opts_destroy(struct hmap *nd_ra_opts) > static inline void > nd_ra_opts_init(struct hmap *nd_ra_opts) > { > + hmap_init(nd_ra_opts); > + > nd_ra_opt_add(nd_ra_opts, "addr_mode", ND_RA_FLAG_ADDR_MODE, "str"); > nd_ra_opt_add(nd_ra_opts, "router_preference", ND_RA_FLAG_PRF, "str"); > nd_ra_opt_add(nd_ra_opts, "slla", ND_OPT_SOURCE_LINKADDR, "mac"); > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Hi, the change looks good overall, one thing I find a bit unfitting is the need for the "destroy_lflow_ctx" function. But to properly get rid of it we would need to hide the ctx into "ed_type_lflow_output". That's probably a story for another time. Thanks, Ales Acked-by: Ales Musil <amusil@redhat.com>
On 9/19/22 09:23, Ales Musil wrote: > On Wed, Sep 14, 2022 at 3:09 PM Dumitru Ceara <dceara@redhat.com> wrote: > >> The nd_ra_opts and controller_event_ops are actually static they never >> change at runtime. DHCP records can instead be computed when populating >> the lflow "input context" to be used during incremental processing. This >> is likely more efficient than building the DHCP opts maps for every logical >> flow recomputed incrementally. >> >> An even more efficient solution would be to introduce proper incremental >> processing for the DHCP opt maps but that seems like too much complexity >> without enough benefit. It's probably OK to recompute the maps at every >> ovn-controller iteration. >> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- >> controller/lflow.c | 206 >> +++---------------------------------------- >> controller/lflow.h | 9 +- >> controller/ovn-controller.c | 128 +++++++++++++++++++-------- >> lib/ovn-l7.h | 2 >> 4 files changed, 110 insertions(+), 235 deletions(-) >> >> diff --git a/controller/lflow.c b/controller/lflow.c >> index eef44389f..de9f17b9a 100644 >> --- a/controller/lflow.c >> +++ b/controller/lflow.c >> @@ -90,9 +90,6 @@ add_matches_to_flow_table(const struct >> sbrec_logical_flow *, >> struct lflow_ctx_out *); >> static void >> consider_logical_flow(const struct sbrec_logical_flow *lflow, >> - struct hmap *dhcp_opts, struct hmap *dhcpv6_opts, >> - struct hmap *nd_ra_opts, >> - struct controller_event_options >> *controller_event_opts, >> bool is_recompute, >> struct lflow_ctx_in *l_ctx_in, >> struct lflow_ctx_out *l_ctx_out); >> @@ -371,40 +368,9 @@ add_logical_flows(struct lflow_ctx_in *l_ctx_in, >> struct lflow_ctx_out *l_ctx_out) >> { >> const struct sbrec_logical_flow *lflow; >> - >> - struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); >> - struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); >> - const struct sbrec_dhcp_options *dhcp_opt_row; >> - SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, >> - l_ctx_in->dhcp_options_table) { >> - dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code, >> - dhcp_opt_row->type); >> - } >> - >> - >> - const struct sbrec_dhcpv6_options *dhcpv6_opt_row; >> - SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row, >> - l_ctx_in->dhcpv6_options_table) { >> - dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, >> dhcpv6_opt_row->code, >> - dhcpv6_opt_row->type); >> - } >> - >> - struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); >> - nd_ra_opts_init(&nd_ra_opts); >> - >> - struct controller_event_options controller_event_opts; >> - controller_event_opts_init(&controller_event_opts); >> - >> SBREC_LOGICAL_FLOW_TABLE_FOR_EACH (lflow, >> l_ctx_in->logical_flow_table) { >> - consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, >> - &nd_ra_opts, &controller_event_opts, true, >> - l_ctx_in, l_ctx_out); >> + consider_logical_flow(lflow, true, l_ctx_in, l_ctx_out); >> } >> - >> - dhcp_opts_destroy(&dhcp_opts); >> - dhcp_opts_destroy(&dhcpv6_opts); >> - nd_ra_opts_destroy(&nd_ra_opts); >> - controller_event_opts_destroy(&controller_event_opts); >> } >> >> bool >> @@ -414,29 +380,6 @@ lflow_handle_changed_flows(struct lflow_ctx_in >> *l_ctx_in, >> bool ret = true; >> const struct sbrec_logical_flow *lflow; >> >> - struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); >> - struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); >> - const struct sbrec_dhcp_options *dhcp_opt_row; >> - SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, >> - l_ctx_in->dhcp_options_table) { >> - dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code, >> - dhcp_opt_row->type); >> - } >> - >> - >> - const struct sbrec_dhcpv6_options *dhcpv6_opt_row; >> - SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row, >> - l_ctx_in->dhcpv6_options_table) { >> - dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, >> dhcpv6_opt_row->code, >> - dhcpv6_opt_row->type); >> - } >> - >> - struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); >> - nd_ra_opts_init(&nd_ra_opts); >> - >> - struct controller_event_options controller_event_opts; >> - controller_event_opts_init(&controller_event_opts); >> - >> /* Flood remove the flows for all the tracked lflows. Its possible >> that >> * lflow_add_flows_for_datapath() may have been called before calling >> * this function. */ >> @@ -486,9 +429,7 @@ lflow_handle_changed_flows(struct lflow_ctx_in >> *l_ctx_in, >> lflows_processed_remove(l_ctx_out->lflows_processed, >> lfp_node); >> } >> >> - consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, >> - &nd_ra_opts, &controller_event_opts, >> false, >> - l_ctx_in, l_ctx_out); >> + consider_logical_flow(lflow, false, l_ctx_in, l_ctx_out); >> } >> } >> HMAP_FOR_EACH_SAFE (ofrn, hmap_node, &flood_remove_nodes) { >> @@ -497,10 +438,6 @@ lflow_handle_changed_flows(struct lflow_ctx_in >> *l_ctx_in, >> } >> hmap_destroy(&flood_remove_nodes); >> >> - dhcp_opts_destroy(&dhcp_opts); >> - dhcp_opts_destroy(&dhcpv6_opts); >> - nd_ra_opts_destroy(&nd_ra_opts); >> - controller_event_opts_destroy(&controller_event_opts); >> return ret; >> } >> >> @@ -556,10 +493,6 @@ consider_lflow_for_added_as_ips__( >> const char *as_name, >> size_t as_ref_count, >> const struct expr_constant_set *as_diff_added, >> - struct hmap *dhcp_opts, >> - struct hmap *dhcpv6_opts, >> - struct hmap *nd_ra_opts, >> - struct controller_event_options >> *controller_event_opts, >> struct lflow_ctx_in *l_ctx_in, >> struct lflow_ctx_out *l_ctx_out) >> { >> @@ -588,10 +521,6 @@ consider_lflow_for_added_as_ips__( >> struct ofpbuf ovnacts = OFPBUF_STUB_INITIALIZER(ovnacts_stub); >> struct ovnact_parse_params pp = { >> .symtab = &symtab, >> - .dhcp_opts = dhcp_opts, >> - .dhcpv6_opts = dhcpv6_opts, >> - .nd_ra_opts = nd_ra_opts, >> - .controller_event_opts = controller_event_opts, >> >> .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS, >> .n_tables = LOG_PIPELINE_LEN, >> @@ -751,10 +680,6 @@ consider_lflow_for_added_as_ips( >> const char *as_name, >> size_t as_ref_count, >> const struct expr_constant_set *as_diff_added, >> - struct hmap *dhcp_opts, >> - struct hmap *dhcpv6_opts, >> - struct hmap *nd_ra_opts, >> - struct controller_event_options >> *controller_event_opts, >> struct lflow_ctx_in *l_ctx_in, >> struct lflow_ctx_out *l_ctx_out) >> { >> @@ -771,17 +696,12 @@ consider_lflow_for_added_as_ips( >> if (dp) { >> return consider_lflow_for_added_as_ips__(lflow, dp, as_name, >> as_ref_count, >> as_diff_added, >> - dhcp_opts, dhcpv6_opts, >> - nd_ra_opts, >> - controller_event_opts, >> l_ctx_in, l_ctx_out); >> } >> for (size_t i = 0; dp_group && i < dp_group->n_datapaths; i++) { >> if (!consider_lflow_for_added_as_ips__(lflow, >> dp_group->datapaths[i], >> as_name, as_ref_count, >> - as_diff_added, dhcp_opts, >> - dhcpv6_opts, nd_ra_opts, >> - controller_event_opts, >> l_ctx_in, >> + as_diff_added, l_ctx_in, >> l_ctx_out)) { >> return false; >> } >> @@ -886,30 +806,6 @@ lflow_handle_addr_set_update(const char *as_name, >> >> *changed = false; >> >> - struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); >> - struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); >> - struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); >> - struct controller_event_options controller_event_opts; >> - >> - if (as_diff->added) { >> - const struct sbrec_dhcp_options *dhcp_opt_row; >> - SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, >> - l_ctx_in->dhcp_options_table) { >> - dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, >> dhcp_opt_row->code, >> - dhcp_opt_row->type); >> - } >> - >> - const struct sbrec_dhcpv6_options *dhcpv6_opt_row; >> - SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH(dhcpv6_opt_row, >> - >> l_ctx_in->dhcpv6_options_table) { >> - dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, >> - dhcpv6_opt_row->code, dhcpv6_opt_row->type); >> - } >> - >> - nd_ra_opts_init(&nd_ra_opts); >> - controller_event_opts_init(&controller_event_opts); >> - } >> - >> bool ret = true; >> struct lflow_ref_list_node *lrln; >> HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) { >> @@ -951,9 +847,7 @@ lflow_handle_addr_set_update(const char *as_name, >> if (as_diff->added) { >> if (!consider_lflow_for_added_as_ips(lflow, as_name, >> lrln->ref_count, >> - as_diff->added, >> &dhcp_opts, >> - &dhcpv6_opts, >> &nd_ra_opts, >> - &controller_event_opts, >> + as_diff->added, >> l_ctx_in, l_ctx_out)) { >> ret = false; >> goto done; >> @@ -962,12 +856,6 @@ lflow_handle_addr_set_update(const char *as_name, >> } >> >> done: >> - if (as_diff->added) { >> - dhcp_opts_destroy(&dhcp_opts); >> - dhcp_opts_destroy(&dhcpv6_opts); >> - nd_ra_opts_destroy(&nd_ra_opts); >> - controller_event_opts_destroy(&controller_event_opts); >> - } >> return ret; >> } >> >> @@ -1008,28 +896,6 @@ lflow_handle_changed_ref(enum ref_type ref_type, >> const char *ref_name, >> } >> *changed = true; >> >> - struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); >> - struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); >> - const struct sbrec_dhcp_options *dhcp_opt_row; >> - SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, >> - l_ctx_in->dhcp_options_table) { >> - dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code, >> - dhcp_opt_row->type); >> - } >> - >> - const struct sbrec_dhcpv6_options *dhcpv6_opt_row; >> - SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH(dhcpv6_opt_row, >> - l_ctx_in->dhcpv6_options_table) { >> - dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, >> dhcpv6_opt_row->code, >> - dhcpv6_opt_row->type); >> - } >> - >> - struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); >> - nd_ra_opts_init(&nd_ra_opts); >> - >> - struct controller_event_options controller_event_opts; >> - controller_event_opts_init(&controller_event_opts); >> - >> /* Re-parse the related lflows. */ >> /* Firstly, flood remove the flows from desired flow table. */ >> struct hmap flood_remove_nodes = >> HMAP_INITIALIZER(&flood_remove_nodes); >> @@ -1072,9 +938,7 @@ lflow_handle_changed_ref(enum ref_type ref_type, >> const char *ref_name, >> lflows_processed_remove(l_ctx_out->lflows_processed, >> lfp_node); >> } >> >> - consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, >> - &nd_ra_opts, &controller_event_opts, false, >> - l_ctx_in, l_ctx_out); >> + consider_logical_flow(lflow, false, l_ctx_in, l_ctx_out); >> } >> HMAP_FOR_EACH_SAFE (ofrn, hmap_node, &flood_remove_nodes) { >> hmap_remove(&flood_remove_nodes, &ofrn->hmap_node); >> @@ -1082,10 +946,6 @@ lflow_handle_changed_ref(enum ref_type ref_type, >> const char *ref_name, >> } >> hmap_destroy(&flood_remove_nodes); >> >> - dhcp_opts_destroy(&dhcp_opts); >> - dhcp_opts_destroy(&dhcpv6_opts); >> - nd_ra_opts_destroy(&nd_ra_opts); >> - controller_event_opts_destroy(&controller_event_opts); >> return ret; >> } >> >> @@ -1308,9 +1168,6 @@ convert_match_to_expr(const struct >> sbrec_logical_flow *lflow, >> static void >> consider_logical_flow__(const struct sbrec_logical_flow *lflow, >> const struct sbrec_datapath_binding *dp, >> - struct hmap *dhcp_opts, struct hmap *dhcpv6_opts, >> - struct hmap *nd_ra_opts, >> - struct controller_event_options >> *controller_event_opts, >> struct lflow_ctx_in *l_ctx_in, >> struct lflow_ctx_out *l_ctx_out) >> { >> @@ -1362,10 +1219,10 @@ consider_logical_flow__(const struct >> sbrec_logical_flow *lflow, >> struct ofpbuf ovnacts = OFPBUF_STUB_INITIALIZER(ovnacts_stub); >> struct ovnact_parse_params pp = { >> .symtab = &symtab, >> - .dhcp_opts = dhcp_opts, >> - .dhcpv6_opts = dhcpv6_opts, >> - .nd_ra_opts = nd_ra_opts, >> - .controller_event_opts = controller_event_opts, >> + .dhcp_opts = &l_ctx_in->dhcp_opts, >> + .dhcpv6_opts = &l_ctx_in->dhcpv6_opts, >> + .nd_ra_opts = l_ctx_in->nd_ra_opts, >> + .controller_event_opts = l_ctx_in->controller_event_opts, >> >> .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS, >> .n_tables = LOG_PIPELINE_LEN, >> @@ -1580,9 +1437,6 @@ lflows_processed_destroy(struct hmap >> *lflows_processed) >> >> static void >> consider_logical_flow(const struct sbrec_logical_flow *lflow, >> - struct hmap *dhcp_opts, struct hmap *dhcpv6_opts, >> - struct hmap *nd_ra_opts, >> - struct controller_event_options >> *controller_event_opts, >> bool is_recompute, >> struct lflow_ctx_in *l_ctx_in, >> struct lflow_ctx_out *l_ctx_out) >> @@ -1606,16 +1460,11 @@ consider_logical_flow(const struct >> sbrec_logical_flow *lflow, >> } >> >> if (dp) { >> - consider_logical_flow__(lflow, dp, >> - dhcp_opts, dhcpv6_opts, nd_ra_opts, >> - controller_event_opts, >> - l_ctx_in, l_ctx_out); >> + consider_logical_flow__(lflow, dp, l_ctx_in, l_ctx_out); >> return; >> } >> for (size_t i = 0; dp_group && i < dp_group->n_datapaths; i++) { >> consider_logical_flow__(lflow, dp_group->datapaths[i], >> - dhcp_opts, dhcpv6_opts, nd_ra_opts, >> - controller_event_opts, >> l_ctx_in, l_ctx_out); >> } >> } >> @@ -2618,28 +2467,6 @@ lflow_add_flows_for_datapath(const struct >> sbrec_datapath_binding *dp, >> struct lflow_ctx_out *l_ctx_out) >> { >> bool handled = true; >> - struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); >> - struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); >> - const struct sbrec_dhcp_options *dhcp_opt_row; >> - SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, >> - l_ctx_in->dhcp_options_table) { >> - dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code, >> - dhcp_opt_row->type); >> - } >> - >> - >> - const struct sbrec_dhcpv6_options *dhcpv6_opt_row; >> - SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row, >> - l_ctx_in->dhcpv6_options_table) { >> - dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, >> dhcpv6_opt_row->code, >> - dhcpv6_opt_row->type); >> - } >> - >> - struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); >> - nd_ra_opts_init(&nd_ra_opts); >> - >> - struct controller_event_options controller_event_opts; >> - controller_event_opts_init(&controller_event_opts); >> >> struct sbrec_logical_flow *lf_row = sbrec_logical_flow_index_init_row( >> l_ctx_in->sbrec_logical_flow_by_logical_datapath); >> @@ -2654,9 +2481,7 @@ lflow_add_flows_for_datapath(const struct >> sbrec_datapath_binding *dp, >> } >> lflows_processed_add(l_ctx_out->lflows_processed, >> &lflow->header_.uuid); >> - consider_logical_flow__(lflow, dp, &dhcp_opts, &dhcpv6_opts, >> - &nd_ra_opts, &controller_event_opts, >> - l_ctx_in, l_ctx_out); >> + consider_logical_flow__(lflow, dp, l_ctx_in, l_ctx_out); >> } >> sbrec_logical_flow_index_destroy_row(lf_row); >> >> @@ -2687,9 +2512,7 @@ lflow_add_flows_for_datapath(const struct >> sbrec_datapath_binding *dp, >> /* Don't call lflows_processed_add() because here we process >> the >> * lflow only for one of the DPs in the DP group, which may be >> * incomplete. */ >> - consider_logical_flow__(lflow, dp, &dhcp_opts, &dhcpv6_opts, >> - &nd_ra_opts, &controller_event_opts, >> - l_ctx_in, l_ctx_out); >> + consider_logical_flow__(lflow, dp, l_ctx_in, l_ctx_out); >> } >> } >> sbrec_logical_flow_index_destroy_row(lf_row); >> @@ -2731,11 +2554,6 @@ lflow_add_flows_for_datapath(const struct >> sbrec_datapath_binding *dp, >> } >> sbrec_static_mac_binding_index_destroy_row(smb_index_row); >> >> - dhcp_opts_destroy(&dhcp_opts); >> - dhcp_opts_destroy(&dhcpv6_opts); >> - nd_ra_opts_destroy(&nd_ra_opts); >> - controller_event_opts_destroy(&controller_event_opts); >> - >> /* Add load balancer hairpin flows if the datapath has any load >> balancers >> * associated. */ >> for (size_t i = 0; i < n_dp_lbs; i++) { >> diff --git a/controller/lflow.h b/controller/lflow.h >> index 543d3cd96..3d1bb38dd 100644 >> --- a/controller/lflow.h >> +++ b/controller/lflow.h >> @@ -46,8 +46,6 @@ struct ovn_desired_flow_table; >> struct hmap; >> struct hmap_node; >> struct sbrec_chassis; >> -struct sbrec_dhcp_options_table; >> -struct sbrec_dhcpv6_options_table; >> struct sbrec_load_balancer; >> struct sbrec_logical_flow_table; >> struct sbrec_mac_binding_table; >> @@ -144,8 +142,6 @@ struct lflow_ctx_in { >> struct ovsdb_idl_index *sbrec_mac_binding_by_datapath; >> struct ovsdb_idl_index *sbrec_static_mac_binding_by_datapath; >> const struct sbrec_port_binding_table *port_binding_table; >> - const struct sbrec_dhcp_options_table *dhcp_options_table; >> - const struct sbrec_dhcpv6_options_table *dhcpv6_options_table; >> const struct sbrec_datapath_binding_table *dp_binding_table; >> const struct sbrec_mac_binding_table *mac_binding_table; >> const struct sbrec_logical_flow_table *logical_flow_table; >> @@ -162,7 +158,12 @@ struct lflow_ctx_in { >> const struct sset *related_lport_ids; >> const struct shash *binding_lports; >> const struct hmap *chassis_tunnels; >> + const struct hmap *nd_ra_opts; >> + const struct controller_event_options *controller_event_opts; >> bool lb_hairpin_use_ct_mark; >> + >> + struct hmap dhcp_opts; >> + struct hmap dhcpv6_opts; >> }; >> >> struct lflow_ctx_out { >> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >> index 6bedb91dc..18a01bbab 100644 >> --- a/controller/ovn-controller.c >> +++ b/controller/ovn-controller.c >> @@ -76,6 +76,7 @@ >> #include "timer.h" >> #include "stopwatch.h" >> #include "lib/inc-proc-eng.h" >> +#include "lib/ovn-l7.h" >> #include "hmapx.h" >> >> VLOG_DEFINE_THIS_MODULE(main); >> @@ -2538,6 +2539,12 @@ struct ed_type_lflow_output { >> >> /* Data for managing hairpin flow conjunctive flow ids. */ >> struct lflow_output_hairpin_data hd; >> + >> + /* Fixed neighbor discovery supported options. */ >> + struct hmap nd_ra_opts; >> + >> + /* Fixed controller_event supported options. */ >> + struct controller_event_options controller_event_opts; >> }; >> >> static void >> @@ -2655,8 +2662,6 @@ init_lflow_ctx(struct engine_node *node, >> l_ctx_in->sbrec_static_mac_binding_by_datapath = >> sbrec_static_mac_binding_by_datapath; >> l_ctx_in->port_binding_table = port_binding_table; >> - l_ctx_in->dhcp_options_table = dhcp_table; >> - l_ctx_in->dhcpv6_options_table = dhcpv6_table; >> l_ctx_in->mac_binding_table = mac_binding_table; >> l_ctx_in->logical_flow_table = logical_flow_table; >> l_ctx_in->logical_dp_group_table = logical_dp_group_table; >> @@ -2673,6 +2678,22 @@ init_lflow_ctx(struct engine_node *node, >> l_ctx_in->binding_lports = &rt_data->lbinding_data.lports; >> l_ctx_in->chassis_tunnels = &non_vif_data->chassis_tunnels; >> l_ctx_in->lb_hairpin_use_ct_mark = n_opts->lb_hairpin_use_ct_mark; >> + l_ctx_in->nd_ra_opts = &fo->nd_ra_opts; >> + l_ctx_in->controller_event_opts = &fo->controller_event_opts; >> + >> + hmap_init(&l_ctx_in->dhcp_opts); >> + const struct sbrec_dhcp_options *dhcp_opt_row; >> + SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, dhcp_table) { >> + dhcp_opt_add(&l_ctx_in->dhcp_opts, dhcp_opt_row->name, >> + dhcp_opt_row->code, dhcp_opt_row->type); >> + } >> + >> + hmap_init(&l_ctx_in->dhcpv6_opts); >> + const struct sbrec_dhcpv6_options *dhcpv6_opt_row; >> + SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row, dhcpv6_table) { >> + dhcp_opt_add(&l_ctx_in->dhcpv6_opts, dhcpv6_opt_row->name, >> + dhcpv6_opt_row->code, dhcpv6_opt_row->type); >> + } >> >> l_ctx_out->flow_table = &fo->flow_table; >> l_ctx_out->group_table = &fo->group_table; >> @@ -2685,6 +2706,14 @@ init_lflow_ctx(struct engine_node *node, >> l_ctx_out->hairpin_lb_ids = &fo->hd.ids; >> } >> >> +static void >> +destroy_lflow_ctx(struct lflow_ctx_in *l_ctx_in, >> + struct lflow_ctx_out *l_ctx_out OVS_UNUSED) >> +{ >> + dhcp_opts_destroy(&l_ctx_in->dhcp_opts); >> + dhcp_opts_destroy(&l_ctx_in->dhcpv6_opts); >> +} >> + >> static void * >> en_lflow_output_init(struct engine_node *node OVS_UNUSED, >> struct engine_arg *arg OVS_UNUSED) >> @@ -2698,6 +2727,8 @@ en_lflow_output_init(struct engine_node *node >> OVS_UNUSED, >> hmap_init(&data->lflows_processed); >> simap_init(&data->hd.ids); >> data->hd.pool = id_pool_create(1, UINT32_MAX - 1); >> + nd_ra_opts_init(&data->nd_ra_opts); >> + controller_event_opts_init(&data->controller_event_opts); >> return data; >> } >> >> @@ -2722,6 +2753,8 @@ en_lflow_output_cleanup(void *data) >> lflow_cache_destroy(flow_output_data->pd.lflow_cache); >> simap_destroy(&flow_output_data->hd.ids); >> id_pool_destroy(flow_output_data->hd.pool); >> + nd_ra_opts_destroy(&flow_output_data->nd_ra_opts); >> + >> controller_event_opts_destroy(&flow_output_data->controller_event_opts); >> } >> >> static void >> @@ -2771,6 +2804,7 @@ en_lflow_output_run(struct engine_node *node, void >> *data) >> struct lflow_ctx_out l_ctx_out; >> init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); >> lflow_run(&l_ctx_in, &l_ctx_out); >> + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); >> >> engine_set_node_state(node, EN_UPDATED); >> } >> @@ -2784,6 +2818,7 @@ lflow_output_sb_logical_flow_handler(struct >> engine_node *node, void *data) >> init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); >> >> bool handled = lflow_handle_changed_flows(&l_ctx_in, &l_ctx_out); >> + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); >> >> engine_set_node_state(node, EN_UPDATED); >> return handled; >> @@ -2846,12 +2881,12 @@ lflow_output_sb_multicast_group_handler(struct >> engine_node *node, void *data) >> struct lflow_ctx_in l_ctx_in; >> struct lflow_ctx_out l_ctx_out; >> init_lflow_ctx(node, lfo, &l_ctx_in, &l_ctx_out); >> - if (!lflow_handle_changed_mc_groups(&l_ctx_in, &l_ctx_out)) { >> - return false; >> - } >> + >> + bool handled = lflow_handle_changed_mc_groups(&l_ctx_in, &l_ctx_out); >> + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); >> >> engine_set_node_state(node, EN_UPDATED); >> - return true; >> + return handled; >> } >> >> static bool >> @@ -2862,12 +2897,12 @@ lflow_output_sb_port_binding_handler(struct >> engine_node *node, void *data) >> struct lflow_ctx_in l_ctx_in; >> struct lflow_ctx_out l_ctx_out; >> init_lflow_ctx(node, lfo, &l_ctx_in, &l_ctx_out); >> - if (!lflow_handle_changed_port_bindings(&l_ctx_in, &l_ctx_out)) { >> - return false; >> - } >> + >> + bool handled = lflow_handle_changed_port_bindings(&l_ctx_in, >> &l_ctx_out); >> + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); >> >> engine_set_node_state(node, EN_UPDATED); >> - return true; >> + return handled; >> } >> >> static bool >> @@ -2876,23 +2911,24 @@ lflow_output_addr_sets_handler(struct engine_node >> *node, void *data) >> struct ed_type_addr_sets *as_data = >> engine_get_input_data("addr_sets", node); >> >> - struct ed_type_lflow_output *fo = data; >> + if (!as_data->change_tracked) { >> + return false; >> + } >> >> - struct lflow_ctx_in l_ctx_in; >> + struct ed_type_lflow_output *fo = data; >> struct lflow_ctx_out l_ctx_out; >> - init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); >> - >> - bool changed; >> + struct lflow_ctx_in l_ctx_in; >> const char *ref_name; >> + bool handled = true; >> + bool changed; >> >> - if (!as_data->change_tracked) { >> - return false; >> - } >> + init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); >> >> SSET_FOR_EACH (ref_name, &as_data->deleted) { >> if (!lflow_handle_changed_ref(REF_TYPE_ADDRSET, ref_name, >> &l_ctx_in, >> &l_ctx_out, &changed)) { >> - return false; >> + handled = false; >> + goto done; >> } >> if (changed) { >> engine_set_node_state(node, EN_UPDATED); >> @@ -2907,7 +2943,8 @@ lflow_output_addr_sets_handler(struct engine_node >> *node, void *data) >> " Reprocess related lflows.", shash_node->name); >> if (!lflow_handle_changed_ref(REF_TYPE_ADDRSET, >> shash_node->name, >> &l_ctx_in, &l_ctx_out, >> &changed)) { >> - return false; >> + handled = false; >> + goto done; >> } >> } >> if (changed) { >> @@ -2917,14 +2954,17 @@ lflow_output_addr_sets_handler(struct engine_node >> *node, void *data) >> SSET_FOR_EACH (ref_name, &as_data->new) { >> if (!lflow_handle_changed_ref(REF_TYPE_ADDRSET, ref_name, >> &l_ctx_in, >> &l_ctx_out, &changed)) { >> - return false; >> + handled = false; >> + goto done; >> } >> if (changed) { >> engine_set_node_state(node, EN_UPDATED); >> } >> } >> >> - return true; >> +done: >> + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); >> + return handled; >> } >> >> static bool >> @@ -2933,23 +2973,24 @@ lflow_output_port_groups_handler(struct >> engine_node *node, void *data) >> struct ed_type_port_groups *pg_data = >> engine_get_input_data("port_groups", node); >> >> - struct ed_type_lflow_output *fo = data; >> + if (!pg_data->change_tracked) { >> + return false; >> + } >> >> - struct lflow_ctx_in l_ctx_in; >> + struct ed_type_lflow_output *fo = data; >> struct lflow_ctx_out l_ctx_out; >> - init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); >> - >> - bool changed; >> + struct lflow_ctx_in l_ctx_in; >> const char *ref_name; >> + bool handled = true; >> + bool changed; >> >> - if (!pg_data->change_tracked) { >> - return false; >> - } >> + init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); >> >> SSET_FOR_EACH (ref_name, &pg_data->deleted) { >> if (!lflow_handle_changed_ref(REF_TYPE_PORTGROUP, ref_name, >> &l_ctx_in, >> &l_ctx_out, &changed)) { >> - return false; >> + handled = false; >> + goto done; >> } >> if (changed) { >> engine_set_node_state(node, EN_UPDATED); >> @@ -2958,7 +2999,8 @@ lflow_output_port_groups_handler(struct engine_node >> *node, void *data) >> SSET_FOR_EACH (ref_name, &pg_data->updated) { >> if (!lflow_handle_changed_ref(REF_TYPE_PORTGROUP, ref_name, >> &l_ctx_in, >> &l_ctx_out, &changed)) { >> - return false; >> + handled = false; >> + goto done; >> } >> if (changed) { >> engine_set_node_state(node, EN_UPDATED); >> @@ -2967,14 +3009,17 @@ lflow_output_port_groups_handler(struct >> engine_node *node, void *data) >> SSET_FOR_EACH (ref_name, &pg_data->new) { >> if (!lflow_handle_changed_ref(REF_TYPE_PORTGROUP, ref_name, >> &l_ctx_in, >> &l_ctx_out, &changed)) { >> - return false; >> + handled = false; >> + goto done; >> } >> if (changed) { >> engine_set_node_state(node, EN_UPDATED); >> } >> } >> >> - return true; >> +done: >> + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); >> + return handled; >> } >> >> static bool >> @@ -3002,6 +3047,8 @@ lflow_output_runtime_data_handler(struct engine_node >> *node, >> struct lflow_ctx_out l_ctx_out; >> struct ed_type_lflow_output *fo = data; >> struct hmap *lbs = NULL; >> + bool handled = true; >> + >> init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); >> >> struct tracked_datapath *tdp; >> @@ -3018,7 +3065,8 @@ lflow_output_runtime_data_handler(struct engine_node >> *node, >> tdp->dp, lbs_by_dp ? lbs_by_dp->dp_lbs : NULL, >> lbs_by_dp ? lbs_by_dp->n_dp_lbs : 0, >> &l_ctx_in, &l_ctx_out)) { >> - return false; >> + handled = false; >> + goto done; >> } >> } >> struct shash_node *shash_node; >> @@ -3026,14 +3074,18 @@ lflow_output_runtime_data_handler(struct >> engine_node *node, >> struct tracked_lport *lport = shash_node->data; >> if (!lflow_handle_flows_for_lport(lport->pb, &l_ctx_in, >> &l_ctx_out)) { >> - return false; >> + handled = false; >> + goto done; >> } >> } >> } >> >> load_balancers_by_dp_cleanup(lbs); >> engine_set_node_state(node, EN_UPDATED); >> - return true; >> + >> +done: >> + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); >> + return handled; >> } >> >> static bool >> @@ -3045,6 +3097,7 @@ lflow_output_sb_load_balancer_handler(struct >> engine_node *node, void *data) >> init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); >> >> bool handled = lflow_handle_changed_lbs(&l_ctx_in, &l_ctx_out); >> + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); >> >> engine_set_node_state(node, EN_UPDATED); >> return handled; >> @@ -3059,6 +3112,7 @@ lflow_output_sb_fdb_handler(struct engine_node >> *node, void *data) >> init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); >> >> bool handled = lflow_handle_changed_fdbs(&l_ctx_in, &l_ctx_out); >> + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); >> >> engine_set_node_state(node, EN_UPDATED); >> return handled; >> diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h >> index 0b2da9f7b..2c7d0add4 100644 >> --- a/lib/ovn-l7.h >> +++ b/lib/ovn-l7.h >> @@ -412,6 +412,8 @@ nd_ra_opts_destroy(struct hmap *nd_ra_opts) >> static inline void >> nd_ra_opts_init(struct hmap *nd_ra_opts) >> { >> + hmap_init(nd_ra_opts); >> + >> nd_ra_opt_add(nd_ra_opts, "addr_mode", ND_RA_FLAG_ADDR_MODE, "str"); >> nd_ra_opt_add(nd_ra_opts, "router_preference", ND_RA_FLAG_PRF, "str"); >> nd_ra_opt_add(nd_ra_opts, "slla", ND_OPT_SOURCE_LINKADDR, "mac"); >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > Hi, Hi Ales, Thanks for the review! > > the change looks good overall, one thing I find a bit unfitting is the need > for the "destroy_lflow_ctx" function. But to properly get rid of it we > would need > to hide the ctx into "ed_type_lflow_output". That's probably a story for > another time. > That's a very good point. I wasn't too hapy with the destroy function either. Arguably, we don't really need all the stuff in the in/out context so maybe we can just remove the whole context structures and pass just the data we need to lflow functions. However, when the context structures were added the goal was to reduce the number of arguments we pass around: https://github.com/ovn-org/ovn/commit/474162d21680819058d57a8da5d23cd7a5c14e55 While that is a benefit, I'm not convinced about the approach. It's my personal opinion, but I have the impression it's more complex to determine where the data is coming from. We now essentially have two (in/out) global variables. I'll think more about this and report back. > Thanks, > Ales > > Acked-by: Ales Musil <amusil@redhat.com> > Thanks, Dumitru
On Wed, Sep 14, 2022 at 6:09 AM Dumitru Ceara <dceara@redhat.com> wrote: > Thanks Dumitru for the improvement. > The nd_ra_opts and controller_event_ops are actually static they never > change at runtime. DHCP records can instead be computed when populating > the lflow "input context" to be used during incremental processing. This > is likely more efficient than building the DHCP opts maps for every logical > flow recomputed incrementally. Maybe a slight correction here. Before this patch it wasn't rebuilding the maps for *every logical flow*. For lflow changes, it was performed once for all the changed lflows. However, it was indeed inefficient for "reference" changes such as port-bindings, where it was performed for every port-binding. > > An even more efficient solution would be to introduce proper incremental > processing for the DHCP opt maps but that seems like too much complexity > without enough benefit. It's probably OK to recompute the maps at every > ovn-controller iteration. Well, every iteration for every handler, but still, I think it is totally OK in terms of performance (and already better than before). However, there may be a benefit of splitting the DHCP opt to a separate I-P node, as input to the lflow_output node. It would avoid introducing the destroy_lflow_ctx(). I don't see complexity here because the dependency looks quite clear. But I am ok either way for this patch. For the destroy_lflow_ctx(), if we want to keep it, I'd remove the l_ctx_out parameter, because the output is essentially the data of the lflow_output engine node, and naturally shouldn't contain anything to be destroyed here. Please see one more finding below. > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > controller/lflow.c | 206 +++---------------------------------------- > controller/lflow.h | 9 +- > controller/ovn-controller.c | 128 +++++++++++++++++++-------- > lib/ovn-l7.h | 2 > 4 files changed, 110 insertions(+), 235 deletions(-) > > diff --git a/controller/lflow.c b/controller/lflow.c > index eef44389f..de9f17b9a 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c > @@ -90,9 +90,6 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *, > struct lflow_ctx_out *); > static void > consider_logical_flow(const struct sbrec_logical_flow *lflow, > - struct hmap *dhcp_opts, struct hmap *dhcpv6_opts, > - struct hmap *nd_ra_opts, > - struct controller_event_options *controller_event_opts, > bool is_recompute, > struct lflow_ctx_in *l_ctx_in, > struct lflow_ctx_out *l_ctx_out); > @@ -371,40 +368,9 @@ add_logical_flows(struct lflow_ctx_in *l_ctx_in, > struct lflow_ctx_out *l_ctx_out) > { > const struct sbrec_logical_flow *lflow; > - > - struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); > - struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); > - const struct sbrec_dhcp_options *dhcp_opt_row; > - SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, > - l_ctx_in->dhcp_options_table) { > - dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code, > - dhcp_opt_row->type); > - } > - > - > - const struct sbrec_dhcpv6_options *dhcpv6_opt_row; > - SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row, > - l_ctx_in->dhcpv6_options_table) { > - dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, dhcpv6_opt_row->code, > - dhcpv6_opt_row->type); > - } > - > - struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); > - nd_ra_opts_init(&nd_ra_opts); > - > - struct controller_event_options controller_event_opts; > - controller_event_opts_init(&controller_event_opts); > - > SBREC_LOGICAL_FLOW_TABLE_FOR_EACH (lflow, l_ctx_in->logical_flow_table) { > - consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, > - &nd_ra_opts, &controller_event_opts, true, > - l_ctx_in, l_ctx_out); > + consider_logical_flow(lflow, true, l_ctx_in, l_ctx_out); > } > - > - dhcp_opts_destroy(&dhcp_opts); > - dhcp_opts_destroy(&dhcpv6_opts); > - nd_ra_opts_destroy(&nd_ra_opts); > - controller_event_opts_destroy(&controller_event_opts); > } > > bool > @@ -414,29 +380,6 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in, > bool ret = true; > const struct sbrec_logical_flow *lflow; > > - struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); > - struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); > - const struct sbrec_dhcp_options *dhcp_opt_row; > - SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, > - l_ctx_in->dhcp_options_table) { > - dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code, > - dhcp_opt_row->type); > - } > - > - > - const struct sbrec_dhcpv6_options *dhcpv6_opt_row; > - SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row, > - l_ctx_in->dhcpv6_options_table) { > - dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, dhcpv6_opt_row->code, > - dhcpv6_opt_row->type); > - } > - > - struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); > - nd_ra_opts_init(&nd_ra_opts); > - > - struct controller_event_options controller_event_opts; > - controller_event_opts_init(&controller_event_opts); > - > /* Flood remove the flows for all the tracked lflows. Its possible that > * lflow_add_flows_for_datapath() may have been called before calling > * this function. */ > @@ -486,9 +429,7 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in, > lflows_processed_remove(l_ctx_out->lflows_processed, lfp_node); > } > > - consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, > - &nd_ra_opts, &controller_event_opts, false, > - l_ctx_in, l_ctx_out); > + consider_logical_flow(lflow, false, l_ctx_in, l_ctx_out); > } > } > HMAP_FOR_EACH_SAFE (ofrn, hmap_node, &flood_remove_nodes) { > @@ -497,10 +438,6 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in, > } > hmap_destroy(&flood_remove_nodes); > > - dhcp_opts_destroy(&dhcp_opts); > - dhcp_opts_destroy(&dhcpv6_opts); > - nd_ra_opts_destroy(&nd_ra_opts); > - controller_event_opts_destroy(&controller_event_opts); > return ret; > } > > @@ -556,10 +493,6 @@ consider_lflow_for_added_as_ips__( > const char *as_name, > size_t as_ref_count, > const struct expr_constant_set *as_diff_added, > - struct hmap *dhcp_opts, > - struct hmap *dhcpv6_opts, > - struct hmap *nd_ra_opts, > - struct controller_event_options *controller_event_opts, > struct lflow_ctx_in *l_ctx_in, > struct lflow_ctx_out *l_ctx_out) > { > @@ -588,10 +521,6 @@ consider_lflow_for_added_as_ips__( > struct ofpbuf ovnacts = OFPBUF_STUB_INITIALIZER(ovnacts_stub); > struct ovnact_parse_params pp = { > .symtab = &symtab, > - .dhcp_opts = dhcp_opts, > - .dhcpv6_opts = dhcpv6_opts, > - .nd_ra_opts = nd_ra_opts, > - .controller_event_opts = controller_event_opts, I think this should be replaced with l_ctx_in->xxx, instead of deleting them. Thanks, Han > > .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS, > .n_tables = LOG_PIPELINE_LEN, > @@ -751,10 +680,6 @@ consider_lflow_for_added_as_ips( > const char *as_name, > size_t as_ref_count, > const struct expr_constant_set *as_diff_added, > - struct hmap *dhcp_opts, > - struct hmap *dhcpv6_opts, > - struct hmap *nd_ra_opts, > - struct controller_event_options *controller_event_opts, > struct lflow_ctx_in *l_ctx_in, > struct lflow_ctx_out *l_ctx_out) > { > @@ -771,17 +696,12 @@ consider_lflow_for_added_as_ips( > if (dp) { > return consider_lflow_for_added_as_ips__(lflow, dp, as_name, > as_ref_count, as_diff_added, > - dhcp_opts, dhcpv6_opts, > - nd_ra_opts, > - controller_event_opts, > l_ctx_in, l_ctx_out); > } > for (size_t i = 0; dp_group && i < dp_group->n_datapaths; i++) { > if (!consider_lflow_for_added_as_ips__(lflow, dp_group->datapaths[i], > as_name, as_ref_count, > - as_diff_added, dhcp_opts, > - dhcpv6_opts, nd_ra_opts, > - controller_event_opts, l_ctx_in, > + as_diff_added, l_ctx_in, > l_ctx_out)) { > return false; > } > @@ -886,30 +806,6 @@ lflow_handle_addr_set_update(const char *as_name, > > *changed = false; > > - struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); > - struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); > - struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); > - struct controller_event_options controller_event_opts; > - > - if (as_diff->added) { > - const struct sbrec_dhcp_options *dhcp_opt_row; > - SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, > - l_ctx_in->dhcp_options_table) { > - dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code, > - dhcp_opt_row->type); > - } > - > - const struct sbrec_dhcpv6_options *dhcpv6_opt_row; > - SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH(dhcpv6_opt_row, > - l_ctx_in->dhcpv6_options_table) { > - dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, > - dhcpv6_opt_row->code, dhcpv6_opt_row->type); > - } > - > - nd_ra_opts_init(&nd_ra_opts); > - controller_event_opts_init(&controller_event_opts); > - } > - > bool ret = true; > struct lflow_ref_list_node *lrln; > HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) { > @@ -951,9 +847,7 @@ lflow_handle_addr_set_update(const char *as_name, > if (as_diff->added) { > if (!consider_lflow_for_added_as_ips(lflow, as_name, > lrln->ref_count, > - as_diff->added, &dhcp_opts, > - &dhcpv6_opts, &nd_ra_opts, > - &controller_event_opts, > + as_diff->added, > l_ctx_in, l_ctx_out)) { > ret = false; > goto done; > @@ -962,12 +856,6 @@ lflow_handle_addr_set_update(const char *as_name, > } > > done: > - if (as_diff->added) { > - dhcp_opts_destroy(&dhcp_opts); > - dhcp_opts_destroy(&dhcpv6_opts); > - nd_ra_opts_destroy(&nd_ra_opts); > - controller_event_opts_destroy(&controller_event_opts); > - } > return ret; > } > > @@ -1008,28 +896,6 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name, > } > *changed = true; > > - struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); > - struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); > - const struct sbrec_dhcp_options *dhcp_opt_row; > - SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, > - l_ctx_in->dhcp_options_table) { > - dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code, > - dhcp_opt_row->type); > - } > - > - const struct sbrec_dhcpv6_options *dhcpv6_opt_row; > - SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH(dhcpv6_opt_row, > - l_ctx_in->dhcpv6_options_table) { > - dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, dhcpv6_opt_row->code, > - dhcpv6_opt_row->type); > - } > - > - struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); > - nd_ra_opts_init(&nd_ra_opts); > - > - struct controller_event_options controller_event_opts; > - controller_event_opts_init(&controller_event_opts); > - > /* Re-parse the related lflows. */ > /* Firstly, flood remove the flows from desired flow table. */ > struct hmap flood_remove_nodes = HMAP_INITIALIZER(&flood_remove_nodes); > @@ -1072,9 +938,7 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name, > lflows_processed_remove(l_ctx_out->lflows_processed, lfp_node); > } > > - consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, > - &nd_ra_opts, &controller_event_opts, false, > - l_ctx_in, l_ctx_out); > + consider_logical_flow(lflow, false, l_ctx_in, l_ctx_out); > } > HMAP_FOR_EACH_SAFE (ofrn, hmap_node, &flood_remove_nodes) { > hmap_remove(&flood_remove_nodes, &ofrn->hmap_node); > @@ -1082,10 +946,6 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name, > } > hmap_destroy(&flood_remove_nodes); > > - dhcp_opts_destroy(&dhcp_opts); > - dhcp_opts_destroy(&dhcpv6_opts); > - nd_ra_opts_destroy(&nd_ra_opts); > - controller_event_opts_destroy(&controller_event_opts); > return ret; > } > > @@ -1308,9 +1168,6 @@ convert_match_to_expr(const struct sbrec_logical_flow *lflow, > static void > consider_logical_flow__(const struct sbrec_logical_flow *lflow, > const struct sbrec_datapath_binding *dp, > - struct hmap *dhcp_opts, struct hmap *dhcpv6_opts, > - struct hmap *nd_ra_opts, > - struct controller_event_options *controller_event_opts, > struct lflow_ctx_in *l_ctx_in, > struct lflow_ctx_out *l_ctx_out) > { > @@ -1362,10 +1219,10 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow, > struct ofpbuf ovnacts = OFPBUF_STUB_INITIALIZER(ovnacts_stub); > struct ovnact_parse_params pp = { > .symtab = &symtab, > - .dhcp_opts = dhcp_opts, > - .dhcpv6_opts = dhcpv6_opts, > - .nd_ra_opts = nd_ra_opts, > - .controller_event_opts = controller_event_opts, > + .dhcp_opts = &l_ctx_in->dhcp_opts, > + .dhcpv6_opts = &l_ctx_in->dhcpv6_opts, > + .nd_ra_opts = l_ctx_in->nd_ra_opts, > + .controller_event_opts = l_ctx_in->controller_event_opts, > > .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS, > .n_tables = LOG_PIPELINE_LEN, > @@ -1580,9 +1437,6 @@ lflows_processed_destroy(struct hmap *lflows_processed) > > static void > consider_logical_flow(const struct sbrec_logical_flow *lflow, > - struct hmap *dhcp_opts, struct hmap *dhcpv6_opts, > - struct hmap *nd_ra_opts, > - struct controller_event_options *controller_event_opts, > bool is_recompute, > struct lflow_ctx_in *l_ctx_in, > struct lflow_ctx_out *l_ctx_out) > @@ -1606,16 +1460,11 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, > } > > if (dp) { > - consider_logical_flow__(lflow, dp, > - dhcp_opts, dhcpv6_opts, nd_ra_opts, > - controller_event_opts, > - l_ctx_in, l_ctx_out); > + consider_logical_flow__(lflow, dp, l_ctx_in, l_ctx_out); > return; > } > for (size_t i = 0; dp_group && i < dp_group->n_datapaths; i++) { > consider_logical_flow__(lflow, dp_group->datapaths[i], > - dhcp_opts, dhcpv6_opts, nd_ra_opts, > - controller_event_opts, > l_ctx_in, l_ctx_out); > } > } > @@ -2618,28 +2467,6 @@ lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp, > struct lflow_ctx_out *l_ctx_out) > { > bool handled = true; > - struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); > - struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); > - const struct sbrec_dhcp_options *dhcp_opt_row; > - SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, > - l_ctx_in->dhcp_options_table) { > - dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code, > - dhcp_opt_row->type); > - } > - > - > - const struct sbrec_dhcpv6_options *dhcpv6_opt_row; > - SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row, > - l_ctx_in->dhcpv6_options_table) { > - dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, dhcpv6_opt_row->code, > - dhcpv6_opt_row->type); > - } > - > - struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); > - nd_ra_opts_init(&nd_ra_opts); > - > - struct controller_event_options controller_event_opts; > - controller_event_opts_init(&controller_event_opts); > > struct sbrec_logical_flow *lf_row = sbrec_logical_flow_index_init_row( > l_ctx_in->sbrec_logical_flow_by_logical_datapath); > @@ -2654,9 +2481,7 @@ lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp, > } > lflows_processed_add(l_ctx_out->lflows_processed, > &lflow->header_.uuid); > - consider_logical_flow__(lflow, dp, &dhcp_opts, &dhcpv6_opts, > - &nd_ra_opts, &controller_event_opts, > - l_ctx_in, l_ctx_out); > + consider_logical_flow__(lflow, dp, l_ctx_in, l_ctx_out); > } > sbrec_logical_flow_index_destroy_row(lf_row); > > @@ -2687,9 +2512,7 @@ lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp, > /* Don't call lflows_processed_add() because here we process the > * lflow only for one of the DPs in the DP group, which may be > * incomplete. */ > - consider_logical_flow__(lflow, dp, &dhcp_opts, &dhcpv6_opts, > - &nd_ra_opts, &controller_event_opts, > - l_ctx_in, l_ctx_out); > + consider_logical_flow__(lflow, dp, l_ctx_in, l_ctx_out); > } > } > sbrec_logical_flow_index_destroy_row(lf_row); > @@ -2731,11 +2554,6 @@ lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp, > } > sbrec_static_mac_binding_index_destroy_row(smb_index_row); > > - dhcp_opts_destroy(&dhcp_opts); > - dhcp_opts_destroy(&dhcpv6_opts); > - nd_ra_opts_destroy(&nd_ra_opts); > - controller_event_opts_destroy(&controller_event_opts); > - > /* Add load balancer hairpin flows if the datapath has any load balancers > * associated. */ > for (size_t i = 0; i < n_dp_lbs; i++) { > diff --git a/controller/lflow.h b/controller/lflow.h > index 543d3cd96..3d1bb38dd 100644 > --- a/controller/lflow.h > +++ b/controller/lflow.h > @@ -46,8 +46,6 @@ struct ovn_desired_flow_table; > struct hmap; > struct hmap_node; > struct sbrec_chassis; > -struct sbrec_dhcp_options_table; > -struct sbrec_dhcpv6_options_table; > struct sbrec_load_balancer; > struct sbrec_logical_flow_table; > struct sbrec_mac_binding_table; > @@ -144,8 +142,6 @@ struct lflow_ctx_in { > struct ovsdb_idl_index *sbrec_mac_binding_by_datapath; > struct ovsdb_idl_index *sbrec_static_mac_binding_by_datapath; > const struct sbrec_port_binding_table *port_binding_table; > - const struct sbrec_dhcp_options_table *dhcp_options_table; > - const struct sbrec_dhcpv6_options_table *dhcpv6_options_table; > const struct sbrec_datapath_binding_table *dp_binding_table; > const struct sbrec_mac_binding_table *mac_binding_table; > const struct sbrec_logical_flow_table *logical_flow_table; > @@ -162,7 +158,12 @@ struct lflow_ctx_in { > const struct sset *related_lport_ids; > const struct shash *binding_lports; > const struct hmap *chassis_tunnels; > + const struct hmap *nd_ra_opts; > + const struct controller_event_options *controller_event_opts; > bool lb_hairpin_use_ct_mark; > + > + struct hmap dhcp_opts; > + struct hmap dhcpv6_opts; > }; > > struct lflow_ctx_out { > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 6bedb91dc..18a01bbab 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -76,6 +76,7 @@ > #include "timer.h" > #include "stopwatch.h" > #include "lib/inc-proc-eng.h" > +#include "lib/ovn-l7.h" > #include "hmapx.h" > > VLOG_DEFINE_THIS_MODULE(main); > @@ -2538,6 +2539,12 @@ struct ed_type_lflow_output { > > /* Data for managing hairpin flow conjunctive flow ids. */ > struct lflow_output_hairpin_data hd; > + > + /* Fixed neighbor discovery supported options. */ > + struct hmap nd_ra_opts; > + > + /* Fixed controller_event supported options. */ > + struct controller_event_options controller_event_opts; > }; > > static void > @@ -2655,8 +2662,6 @@ init_lflow_ctx(struct engine_node *node, > l_ctx_in->sbrec_static_mac_binding_by_datapath = > sbrec_static_mac_binding_by_datapath; > l_ctx_in->port_binding_table = port_binding_table; > - l_ctx_in->dhcp_options_table = dhcp_table; > - l_ctx_in->dhcpv6_options_table = dhcpv6_table; > l_ctx_in->mac_binding_table = mac_binding_table; > l_ctx_in->logical_flow_table = logical_flow_table; > l_ctx_in->logical_dp_group_table = logical_dp_group_table; > @@ -2673,6 +2678,22 @@ init_lflow_ctx(struct engine_node *node, > l_ctx_in->binding_lports = &rt_data->lbinding_data.lports; > l_ctx_in->chassis_tunnels = &non_vif_data->chassis_tunnels; > l_ctx_in->lb_hairpin_use_ct_mark = n_opts->lb_hairpin_use_ct_mark; > + l_ctx_in->nd_ra_opts = &fo->nd_ra_opts; > + l_ctx_in->controller_event_opts = &fo->controller_event_opts; > + > + hmap_init(&l_ctx_in->dhcp_opts); > + const struct sbrec_dhcp_options *dhcp_opt_row; > + SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, dhcp_table) { > + dhcp_opt_add(&l_ctx_in->dhcp_opts, dhcp_opt_row->name, > + dhcp_opt_row->code, dhcp_opt_row->type); > + } > + > + hmap_init(&l_ctx_in->dhcpv6_opts); > + const struct sbrec_dhcpv6_options *dhcpv6_opt_row; > + SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row, dhcpv6_table) { > + dhcp_opt_add(&l_ctx_in->dhcpv6_opts, dhcpv6_opt_row->name, > + dhcpv6_opt_row->code, dhcpv6_opt_row->type); > + } > > l_ctx_out->flow_table = &fo->flow_table; > l_ctx_out->group_table = &fo->group_table; > @@ -2685,6 +2706,14 @@ init_lflow_ctx(struct engine_node *node, > l_ctx_out->hairpin_lb_ids = &fo->hd.ids; > } > > +static void > +destroy_lflow_ctx(struct lflow_ctx_in *l_ctx_in, > + struct lflow_ctx_out *l_ctx_out OVS_UNUSED) > +{ > + dhcp_opts_destroy(&l_ctx_in->dhcp_opts); > + dhcp_opts_destroy(&l_ctx_in->dhcpv6_opts); > +} > + > static void * > en_lflow_output_init(struct engine_node *node OVS_UNUSED, > struct engine_arg *arg OVS_UNUSED) > @@ -2698,6 +2727,8 @@ en_lflow_output_init(struct engine_node *node OVS_UNUSED, > hmap_init(&data->lflows_processed); > simap_init(&data->hd.ids); > data->hd.pool = id_pool_create(1, UINT32_MAX - 1); > + nd_ra_opts_init(&data->nd_ra_opts); > + controller_event_opts_init(&data->controller_event_opts); > return data; > } > > @@ -2722,6 +2753,8 @@ en_lflow_output_cleanup(void *data) > lflow_cache_destroy(flow_output_data->pd.lflow_cache); > simap_destroy(&flow_output_data->hd.ids); > id_pool_destroy(flow_output_data->hd.pool); > + nd_ra_opts_destroy(&flow_output_data->nd_ra_opts); > + controller_event_opts_destroy(&flow_output_data->controller_event_opts); > } > > static void > @@ -2771,6 +2804,7 @@ en_lflow_output_run(struct engine_node *node, void *data) > struct lflow_ctx_out l_ctx_out; > init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); > lflow_run(&l_ctx_in, &l_ctx_out); > + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); > > engine_set_node_state(node, EN_UPDATED); > } > @@ -2784,6 +2818,7 @@ lflow_output_sb_logical_flow_handler(struct engine_node *node, void *data) > init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); > > bool handled = lflow_handle_changed_flows(&l_ctx_in, &l_ctx_out); > + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); > > engine_set_node_state(node, EN_UPDATED); > return handled; > @@ -2846,12 +2881,12 @@ lflow_output_sb_multicast_group_handler(struct engine_node *node, void *data) > struct lflow_ctx_in l_ctx_in; > struct lflow_ctx_out l_ctx_out; > init_lflow_ctx(node, lfo, &l_ctx_in, &l_ctx_out); > - if (!lflow_handle_changed_mc_groups(&l_ctx_in, &l_ctx_out)) { > - return false; > - } > + > + bool handled = lflow_handle_changed_mc_groups(&l_ctx_in, &l_ctx_out); > + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); > > engine_set_node_state(node, EN_UPDATED); > - return true; > + return handled; > } > > static bool > @@ -2862,12 +2897,12 @@ lflow_output_sb_port_binding_handler(struct engine_node *node, void *data) > struct lflow_ctx_in l_ctx_in; > struct lflow_ctx_out l_ctx_out; > init_lflow_ctx(node, lfo, &l_ctx_in, &l_ctx_out); > - if (!lflow_handle_changed_port_bindings(&l_ctx_in, &l_ctx_out)) { > - return false; > - } > + > + bool handled = lflow_handle_changed_port_bindings(&l_ctx_in, &l_ctx_out); > + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); > > engine_set_node_state(node, EN_UPDATED); > - return true; > + return handled; > } > > static bool > @@ -2876,23 +2911,24 @@ lflow_output_addr_sets_handler(struct engine_node *node, void *data) > struct ed_type_addr_sets *as_data = > engine_get_input_data("addr_sets", node); > > - struct ed_type_lflow_output *fo = data; > + if (!as_data->change_tracked) { > + return false; > + } > > - struct lflow_ctx_in l_ctx_in; > + struct ed_type_lflow_output *fo = data; > struct lflow_ctx_out l_ctx_out; > - init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); > - > - bool changed; > + struct lflow_ctx_in l_ctx_in; > const char *ref_name; > + bool handled = true; > + bool changed; > > - if (!as_data->change_tracked) { > - return false; > - } > + init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); > > SSET_FOR_EACH (ref_name, &as_data->deleted) { > if (!lflow_handle_changed_ref(REF_TYPE_ADDRSET, ref_name, &l_ctx_in, > &l_ctx_out, &changed)) { > - return false; > + handled = false; > + goto done; > } > if (changed) { > engine_set_node_state(node, EN_UPDATED); > @@ -2907,7 +2943,8 @@ lflow_output_addr_sets_handler(struct engine_node *node, void *data) > " Reprocess related lflows.", shash_node->name); > if (!lflow_handle_changed_ref(REF_TYPE_ADDRSET, shash_node->name, > &l_ctx_in, &l_ctx_out, &changed)) { > - return false; > + handled = false; > + goto done; > } > } > if (changed) { > @@ -2917,14 +2954,17 @@ lflow_output_addr_sets_handler(struct engine_node *node, void *data) > SSET_FOR_EACH (ref_name, &as_data->new) { > if (!lflow_handle_changed_ref(REF_TYPE_ADDRSET, ref_name, &l_ctx_in, > &l_ctx_out, &changed)) { > - return false; > + handled = false; > + goto done; > } > if (changed) { > engine_set_node_state(node, EN_UPDATED); > } > } > > - return true; > +done: > + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); > + return handled; > } > > static bool > @@ -2933,23 +2973,24 @@ lflow_output_port_groups_handler(struct engine_node *node, void *data) > struct ed_type_port_groups *pg_data = > engine_get_input_data("port_groups", node); > > - struct ed_type_lflow_output *fo = data; > + if (!pg_data->change_tracked) { > + return false; > + } > > - struct lflow_ctx_in l_ctx_in; > + struct ed_type_lflow_output *fo = data; > struct lflow_ctx_out l_ctx_out; > - init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); > - > - bool changed; > + struct lflow_ctx_in l_ctx_in; > const char *ref_name; > + bool handled = true; > + bool changed; > > - if (!pg_data->change_tracked) { > - return false; > - } > + init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); > > SSET_FOR_EACH (ref_name, &pg_data->deleted) { > if (!lflow_handle_changed_ref(REF_TYPE_PORTGROUP, ref_name, &l_ctx_in, > &l_ctx_out, &changed)) { > - return false; > + handled = false; > + goto done; > } > if (changed) { > engine_set_node_state(node, EN_UPDATED); > @@ -2958,7 +2999,8 @@ lflow_output_port_groups_handler(struct engine_node *node, void *data) > SSET_FOR_EACH (ref_name, &pg_data->updated) { > if (!lflow_handle_changed_ref(REF_TYPE_PORTGROUP, ref_name, &l_ctx_in, > &l_ctx_out, &changed)) { > - return false; > + handled = false; > + goto done; > } > if (changed) { > engine_set_node_state(node, EN_UPDATED); > @@ -2967,14 +3009,17 @@ lflow_output_port_groups_handler(struct engine_node *node, void *data) > SSET_FOR_EACH (ref_name, &pg_data->new) { > if (!lflow_handle_changed_ref(REF_TYPE_PORTGROUP, ref_name, &l_ctx_in, > &l_ctx_out, &changed)) { > - return false; > + handled = false; > + goto done; > } > if (changed) { > engine_set_node_state(node, EN_UPDATED); > } > } > > - return true; > +done: > + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); > + return handled; > } > > static bool > @@ -3002,6 +3047,8 @@ lflow_output_runtime_data_handler(struct engine_node *node, > struct lflow_ctx_out l_ctx_out; > struct ed_type_lflow_output *fo = data; > struct hmap *lbs = NULL; > + bool handled = true; > + > init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); > > struct tracked_datapath *tdp; > @@ -3018,7 +3065,8 @@ lflow_output_runtime_data_handler(struct engine_node *node, > tdp->dp, lbs_by_dp ? lbs_by_dp->dp_lbs : NULL, > lbs_by_dp ? lbs_by_dp->n_dp_lbs : 0, > &l_ctx_in, &l_ctx_out)) { > - return false; > + handled = false; > + goto done; > } > } > struct shash_node *shash_node; > @@ -3026,14 +3074,18 @@ lflow_output_runtime_data_handler(struct engine_node *node, > struct tracked_lport *lport = shash_node->data; > if (!lflow_handle_flows_for_lport(lport->pb, &l_ctx_in, > &l_ctx_out)) { > - return false; > + handled = false; > + goto done; > } > } > } > > load_balancers_by_dp_cleanup(lbs); > engine_set_node_state(node, EN_UPDATED); > - return true; > + > +done: > + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); > + return handled; > } > > static bool > @@ -3045,6 +3097,7 @@ lflow_output_sb_load_balancer_handler(struct engine_node *node, void *data) > init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); > > bool handled = lflow_handle_changed_lbs(&l_ctx_in, &l_ctx_out); > + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); > > engine_set_node_state(node, EN_UPDATED); > return handled; > @@ -3059,6 +3112,7 @@ lflow_output_sb_fdb_handler(struct engine_node *node, void *data) > init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); > > bool handled = lflow_handle_changed_fdbs(&l_ctx_in, &l_ctx_out); > + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); > > engine_set_node_state(node, EN_UPDATED); > return handled; > diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h > index 0b2da9f7b..2c7d0add4 100644 > --- a/lib/ovn-l7.h > +++ b/lib/ovn-l7.h > @@ -412,6 +412,8 @@ nd_ra_opts_destroy(struct hmap *nd_ra_opts) > static inline void > nd_ra_opts_init(struct hmap *nd_ra_opts) > { > + hmap_init(nd_ra_opts); > + > nd_ra_opt_add(nd_ra_opts, "addr_mode", ND_RA_FLAG_ADDR_MODE, "str"); > nd_ra_opt_add(nd_ra_opts, "router_preference", ND_RA_FLAG_PRF, "str"); > nd_ra_opt_add(nd_ra_opts, "slla", ND_OPT_SOURCE_LINKADDR, "mac"); > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 9/22/22 08:24, Han Zhou wrote: > On Wed, Sep 14, 2022 at 6:09 AM Dumitru Ceara <dceara@redhat.com> wrote: >> > > Thanks Dumitru for the improvement. > >> The nd_ra_opts and controller_event_ops are actually static they never >> change at runtime. DHCP records can instead be computed when populating >> the lflow "input context" to be used during incremental processing. This >> is likely more efficient than building the DHCP opts maps for every > logical >> flow recomputed incrementally. > > Maybe a slight correction here. Before this patch it wasn't rebuilding the > maps for *every logical flow*. For lflow changes, it was performed once for > all the changed lflows. However, it was indeed inefficient for "reference" > changes such as port-bindings, where it was performed for every > port-binding. > Sure, it's more clear like this. >> >> An even more efficient solution would be to introduce proper incremental >> processing for the DHCP opt maps but that seems like too much complexity >> without enough benefit. It's probably OK to recompute the maps at every >> ovn-controller iteration. > > Well, every iteration for every handler, but still, I think it is totally > OK in terms of performance (and already better than before). > However, there may be a benefit of splitting the DHCP opt to a separate I-P > node, as input to the lflow_output node. It would avoid introducing the > destroy_lflow_ctx(). I don't see complexity here because the dependency > looks quite clear. But I am ok either way for this patch. > For the destroy_lflow_ctx(), if we want to keep it, I'd remove the > l_ctx_out parameter, because the output is essentially the data of the > lflow_output engine node, and naturally shouldn't contain anything to be > destroyed here. Ok, I'll try to just split the dhcp opt part. Should be more clear, I agree. > > Please see one more finding below. > >> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- >> controller/lflow.c | 206 > +++---------------------------------------- >> controller/lflow.h | 9 +- >> controller/ovn-controller.c | 128 +++++++++++++++++++-------- >> lib/ovn-l7.h | 2 >> 4 files changed, 110 insertions(+), 235 deletions(-) >> >> diff --git a/controller/lflow.c b/controller/lflow.c >> index eef44389f..de9f17b9a 100644 >> --- a/controller/lflow.c >> +++ b/controller/lflow.c >> @@ -90,9 +90,6 @@ add_matches_to_flow_table(const struct > sbrec_logical_flow *, >> struct lflow_ctx_out *); >> static void >> consider_logical_flow(const struct sbrec_logical_flow *lflow, >> - struct hmap *dhcp_opts, struct hmap *dhcpv6_opts, >> - struct hmap *nd_ra_opts, >> - struct controller_event_options > *controller_event_opts, >> bool is_recompute, >> struct lflow_ctx_in *l_ctx_in, >> struct lflow_ctx_out *l_ctx_out); >> @@ -371,40 +368,9 @@ add_logical_flows(struct lflow_ctx_in *l_ctx_in, >> struct lflow_ctx_out *l_ctx_out) >> { >> const struct sbrec_logical_flow *lflow; >> - >> - struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); >> - struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); >> - const struct sbrec_dhcp_options *dhcp_opt_row; >> - SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, >> - l_ctx_in->dhcp_options_table) { >> - dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code, >> - dhcp_opt_row->type); >> - } >> - >> - >> - const struct sbrec_dhcpv6_options *dhcpv6_opt_row; >> - SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row, >> - l_ctx_in->dhcpv6_options_table) > { >> - dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, > dhcpv6_opt_row->code, >> - dhcpv6_opt_row->type); >> - } >> - >> - struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); >> - nd_ra_opts_init(&nd_ra_opts); >> - >> - struct controller_event_options controller_event_opts; >> - controller_event_opts_init(&controller_event_opts); >> - >> SBREC_LOGICAL_FLOW_TABLE_FOR_EACH (lflow, > l_ctx_in->logical_flow_table) { >> - consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, >> - &nd_ra_opts, &controller_event_opts, true, >> - l_ctx_in, l_ctx_out); >> + consider_logical_flow(lflow, true, l_ctx_in, l_ctx_out); >> } >> - >> - dhcp_opts_destroy(&dhcp_opts); >> - dhcp_opts_destroy(&dhcpv6_opts); >> - nd_ra_opts_destroy(&nd_ra_opts); >> - controller_event_opts_destroy(&controller_event_opts); >> } >> >> bool >> @@ -414,29 +380,6 @@ lflow_handle_changed_flows(struct lflow_ctx_in > *l_ctx_in, >> bool ret = true; >> const struct sbrec_logical_flow *lflow; >> >> - struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); >> - struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); >> - const struct sbrec_dhcp_options *dhcp_opt_row; >> - SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, >> - l_ctx_in->dhcp_options_table) { >> - dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code, >> - dhcp_opt_row->type); >> - } >> - >> - >> - const struct sbrec_dhcpv6_options *dhcpv6_opt_row; >> - SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row, >> - l_ctx_in->dhcpv6_options_table) > { >> - dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, > dhcpv6_opt_row->code, >> - dhcpv6_opt_row->type); >> - } >> - >> - struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); >> - nd_ra_opts_init(&nd_ra_opts); >> - >> - struct controller_event_options controller_event_opts; >> - controller_event_opts_init(&controller_event_opts); >> - >> /* Flood remove the flows for all the tracked lflows. Its possible > that >> * lflow_add_flows_for_datapath() may have been called before calling >> * this function. */ >> @@ -486,9 +429,7 @@ lflow_handle_changed_flows(struct lflow_ctx_in > *l_ctx_in, >> lflows_processed_remove(l_ctx_out->lflows_processed, > lfp_node); >> } >> >> - consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, >> - &nd_ra_opts, &controller_event_opts, > false, >> - l_ctx_in, l_ctx_out); >> + consider_logical_flow(lflow, false, l_ctx_in, l_ctx_out); >> } >> } >> HMAP_FOR_EACH_SAFE (ofrn, hmap_node, &flood_remove_nodes) { >> @@ -497,10 +438,6 @@ lflow_handle_changed_flows(struct lflow_ctx_in > *l_ctx_in, >> } >> hmap_destroy(&flood_remove_nodes); >> >> - dhcp_opts_destroy(&dhcp_opts); >> - dhcp_opts_destroy(&dhcpv6_opts); >> - nd_ra_opts_destroy(&nd_ra_opts); >> - controller_event_opts_destroy(&controller_event_opts); >> return ret; >> } >> >> @@ -556,10 +493,6 @@ consider_lflow_for_added_as_ips__( >> const char *as_name, >> size_t as_ref_count, >> const struct expr_constant_set *as_diff_added, >> - struct hmap *dhcp_opts, >> - struct hmap *dhcpv6_opts, >> - struct hmap *nd_ra_opts, >> - struct controller_event_options > *controller_event_opts, >> struct lflow_ctx_in *l_ctx_in, >> struct lflow_ctx_out *l_ctx_out) >> { >> @@ -588,10 +521,6 @@ consider_lflow_for_added_as_ips__( >> struct ofpbuf ovnacts = OFPBUF_STUB_INITIALIZER(ovnacts_stub); >> struct ovnact_parse_params pp = { >> .symtab = &symtab, >> - .dhcp_opts = dhcp_opts, >> - .dhcpv6_opts = dhcpv6_opts, >> - .nd_ra_opts = nd_ra_opts, >> - .controller_event_opts = controller_event_opts, > > I think this should be replaced with l_ctx_in->xxx, instead of deleting > them. Oops. Yes, you're right. I'll fix it in the next revision. > > Thanks, > Han > Thanks, Dumitru
diff --git a/controller/lflow.c b/controller/lflow.c index eef44389f..de9f17b9a 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -90,9 +90,6 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *, struct lflow_ctx_out *); static void consider_logical_flow(const struct sbrec_logical_flow *lflow, - struct hmap *dhcp_opts, struct hmap *dhcpv6_opts, - struct hmap *nd_ra_opts, - struct controller_event_options *controller_event_opts, bool is_recompute, struct lflow_ctx_in *l_ctx_in, struct lflow_ctx_out *l_ctx_out); @@ -371,40 +368,9 @@ add_logical_flows(struct lflow_ctx_in *l_ctx_in, struct lflow_ctx_out *l_ctx_out) { const struct sbrec_logical_flow *lflow; - - struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); - struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); - const struct sbrec_dhcp_options *dhcp_opt_row; - SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, - l_ctx_in->dhcp_options_table) { - dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code, - dhcp_opt_row->type); - } - - - const struct sbrec_dhcpv6_options *dhcpv6_opt_row; - SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row, - l_ctx_in->dhcpv6_options_table) { - dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, dhcpv6_opt_row->code, - dhcpv6_opt_row->type); - } - - struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); - nd_ra_opts_init(&nd_ra_opts); - - struct controller_event_options controller_event_opts; - controller_event_opts_init(&controller_event_opts); - SBREC_LOGICAL_FLOW_TABLE_FOR_EACH (lflow, l_ctx_in->logical_flow_table) { - consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, - &nd_ra_opts, &controller_event_opts, true, - l_ctx_in, l_ctx_out); + consider_logical_flow(lflow, true, l_ctx_in, l_ctx_out); } - - dhcp_opts_destroy(&dhcp_opts); - dhcp_opts_destroy(&dhcpv6_opts); - nd_ra_opts_destroy(&nd_ra_opts); - controller_event_opts_destroy(&controller_event_opts); } bool @@ -414,29 +380,6 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in, bool ret = true; const struct sbrec_logical_flow *lflow; - struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); - struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); - const struct sbrec_dhcp_options *dhcp_opt_row; - SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, - l_ctx_in->dhcp_options_table) { - dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code, - dhcp_opt_row->type); - } - - - const struct sbrec_dhcpv6_options *dhcpv6_opt_row; - SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row, - l_ctx_in->dhcpv6_options_table) { - dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, dhcpv6_opt_row->code, - dhcpv6_opt_row->type); - } - - struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); - nd_ra_opts_init(&nd_ra_opts); - - struct controller_event_options controller_event_opts; - controller_event_opts_init(&controller_event_opts); - /* Flood remove the flows for all the tracked lflows. Its possible that * lflow_add_flows_for_datapath() may have been called before calling * this function. */ @@ -486,9 +429,7 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in, lflows_processed_remove(l_ctx_out->lflows_processed, lfp_node); } - consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, - &nd_ra_opts, &controller_event_opts, false, - l_ctx_in, l_ctx_out); + consider_logical_flow(lflow, false, l_ctx_in, l_ctx_out); } } HMAP_FOR_EACH_SAFE (ofrn, hmap_node, &flood_remove_nodes) { @@ -497,10 +438,6 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in, } hmap_destroy(&flood_remove_nodes); - dhcp_opts_destroy(&dhcp_opts); - dhcp_opts_destroy(&dhcpv6_opts); - nd_ra_opts_destroy(&nd_ra_opts); - controller_event_opts_destroy(&controller_event_opts); return ret; } @@ -556,10 +493,6 @@ consider_lflow_for_added_as_ips__( const char *as_name, size_t as_ref_count, const struct expr_constant_set *as_diff_added, - struct hmap *dhcp_opts, - struct hmap *dhcpv6_opts, - struct hmap *nd_ra_opts, - struct controller_event_options *controller_event_opts, struct lflow_ctx_in *l_ctx_in, struct lflow_ctx_out *l_ctx_out) { @@ -588,10 +521,6 @@ consider_lflow_for_added_as_ips__( struct ofpbuf ovnacts = OFPBUF_STUB_INITIALIZER(ovnacts_stub); struct ovnact_parse_params pp = { .symtab = &symtab, - .dhcp_opts = dhcp_opts, - .dhcpv6_opts = dhcpv6_opts, - .nd_ra_opts = nd_ra_opts, - .controller_event_opts = controller_event_opts, .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS, .n_tables = LOG_PIPELINE_LEN, @@ -751,10 +680,6 @@ consider_lflow_for_added_as_ips( const char *as_name, size_t as_ref_count, const struct expr_constant_set *as_diff_added, - struct hmap *dhcp_opts, - struct hmap *dhcpv6_opts, - struct hmap *nd_ra_opts, - struct controller_event_options *controller_event_opts, struct lflow_ctx_in *l_ctx_in, struct lflow_ctx_out *l_ctx_out) { @@ -771,17 +696,12 @@ consider_lflow_for_added_as_ips( if (dp) { return consider_lflow_for_added_as_ips__(lflow, dp, as_name, as_ref_count, as_diff_added, - dhcp_opts, dhcpv6_opts, - nd_ra_opts, - controller_event_opts, l_ctx_in, l_ctx_out); } for (size_t i = 0; dp_group && i < dp_group->n_datapaths; i++) { if (!consider_lflow_for_added_as_ips__(lflow, dp_group->datapaths[i], as_name, as_ref_count, - as_diff_added, dhcp_opts, - dhcpv6_opts, nd_ra_opts, - controller_event_opts, l_ctx_in, + as_diff_added, l_ctx_in, l_ctx_out)) { return false; } @@ -886,30 +806,6 @@ lflow_handle_addr_set_update(const char *as_name, *changed = false; - struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); - struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); - struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); - struct controller_event_options controller_event_opts; - - if (as_diff->added) { - const struct sbrec_dhcp_options *dhcp_opt_row; - SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, - l_ctx_in->dhcp_options_table) { - dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code, - dhcp_opt_row->type); - } - - const struct sbrec_dhcpv6_options *dhcpv6_opt_row; - SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH(dhcpv6_opt_row, - l_ctx_in->dhcpv6_options_table) { - dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, - dhcpv6_opt_row->code, dhcpv6_opt_row->type); - } - - nd_ra_opts_init(&nd_ra_opts); - controller_event_opts_init(&controller_event_opts); - } - bool ret = true; struct lflow_ref_list_node *lrln; HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) { @@ -951,9 +847,7 @@ lflow_handle_addr_set_update(const char *as_name, if (as_diff->added) { if (!consider_lflow_for_added_as_ips(lflow, as_name, lrln->ref_count, - as_diff->added, &dhcp_opts, - &dhcpv6_opts, &nd_ra_opts, - &controller_event_opts, + as_diff->added, l_ctx_in, l_ctx_out)) { ret = false; goto done; @@ -962,12 +856,6 @@ lflow_handle_addr_set_update(const char *as_name, } done: - if (as_diff->added) { - dhcp_opts_destroy(&dhcp_opts); - dhcp_opts_destroy(&dhcpv6_opts); - nd_ra_opts_destroy(&nd_ra_opts); - controller_event_opts_destroy(&controller_event_opts); - } return ret; } @@ -1008,28 +896,6 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name, } *changed = true; - struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); - struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); - const struct sbrec_dhcp_options *dhcp_opt_row; - SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, - l_ctx_in->dhcp_options_table) { - dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code, - dhcp_opt_row->type); - } - - const struct sbrec_dhcpv6_options *dhcpv6_opt_row; - SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH(dhcpv6_opt_row, - l_ctx_in->dhcpv6_options_table) { - dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, dhcpv6_opt_row->code, - dhcpv6_opt_row->type); - } - - struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); - nd_ra_opts_init(&nd_ra_opts); - - struct controller_event_options controller_event_opts; - controller_event_opts_init(&controller_event_opts); - /* Re-parse the related lflows. */ /* Firstly, flood remove the flows from desired flow table. */ struct hmap flood_remove_nodes = HMAP_INITIALIZER(&flood_remove_nodes); @@ -1072,9 +938,7 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name, lflows_processed_remove(l_ctx_out->lflows_processed, lfp_node); } - consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, - &nd_ra_opts, &controller_event_opts, false, - l_ctx_in, l_ctx_out); + consider_logical_flow(lflow, false, l_ctx_in, l_ctx_out); } HMAP_FOR_EACH_SAFE (ofrn, hmap_node, &flood_remove_nodes) { hmap_remove(&flood_remove_nodes, &ofrn->hmap_node); @@ -1082,10 +946,6 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name, } hmap_destroy(&flood_remove_nodes); - dhcp_opts_destroy(&dhcp_opts); - dhcp_opts_destroy(&dhcpv6_opts); - nd_ra_opts_destroy(&nd_ra_opts); - controller_event_opts_destroy(&controller_event_opts); return ret; } @@ -1308,9 +1168,6 @@ convert_match_to_expr(const struct sbrec_logical_flow *lflow, static void consider_logical_flow__(const struct sbrec_logical_flow *lflow, const struct sbrec_datapath_binding *dp, - struct hmap *dhcp_opts, struct hmap *dhcpv6_opts, - struct hmap *nd_ra_opts, - struct controller_event_options *controller_event_opts, struct lflow_ctx_in *l_ctx_in, struct lflow_ctx_out *l_ctx_out) { @@ -1362,10 +1219,10 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow, struct ofpbuf ovnacts = OFPBUF_STUB_INITIALIZER(ovnacts_stub); struct ovnact_parse_params pp = { .symtab = &symtab, - .dhcp_opts = dhcp_opts, - .dhcpv6_opts = dhcpv6_opts, - .nd_ra_opts = nd_ra_opts, - .controller_event_opts = controller_event_opts, + .dhcp_opts = &l_ctx_in->dhcp_opts, + .dhcpv6_opts = &l_ctx_in->dhcpv6_opts, + .nd_ra_opts = l_ctx_in->nd_ra_opts, + .controller_event_opts = l_ctx_in->controller_event_opts, .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS, .n_tables = LOG_PIPELINE_LEN, @@ -1580,9 +1437,6 @@ lflows_processed_destroy(struct hmap *lflows_processed) static void consider_logical_flow(const struct sbrec_logical_flow *lflow, - struct hmap *dhcp_opts, struct hmap *dhcpv6_opts, - struct hmap *nd_ra_opts, - struct controller_event_options *controller_event_opts, bool is_recompute, struct lflow_ctx_in *l_ctx_in, struct lflow_ctx_out *l_ctx_out) @@ -1606,16 +1460,11 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, } if (dp) { - consider_logical_flow__(lflow, dp, - dhcp_opts, dhcpv6_opts, nd_ra_opts, - controller_event_opts, - l_ctx_in, l_ctx_out); + consider_logical_flow__(lflow, dp, l_ctx_in, l_ctx_out); return; } for (size_t i = 0; dp_group && i < dp_group->n_datapaths; i++) { consider_logical_flow__(lflow, dp_group->datapaths[i], - dhcp_opts, dhcpv6_opts, nd_ra_opts, - controller_event_opts, l_ctx_in, l_ctx_out); } } @@ -2618,28 +2467,6 @@ lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp, struct lflow_ctx_out *l_ctx_out) { bool handled = true; - struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); - struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); - const struct sbrec_dhcp_options *dhcp_opt_row; - SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, - l_ctx_in->dhcp_options_table) { - dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code, - dhcp_opt_row->type); - } - - - const struct sbrec_dhcpv6_options *dhcpv6_opt_row; - SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row, - l_ctx_in->dhcpv6_options_table) { - dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, dhcpv6_opt_row->code, - dhcpv6_opt_row->type); - } - - struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); - nd_ra_opts_init(&nd_ra_opts); - - struct controller_event_options controller_event_opts; - controller_event_opts_init(&controller_event_opts); struct sbrec_logical_flow *lf_row = sbrec_logical_flow_index_init_row( l_ctx_in->sbrec_logical_flow_by_logical_datapath); @@ -2654,9 +2481,7 @@ lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp, } lflows_processed_add(l_ctx_out->lflows_processed, &lflow->header_.uuid); - consider_logical_flow__(lflow, dp, &dhcp_opts, &dhcpv6_opts, - &nd_ra_opts, &controller_event_opts, - l_ctx_in, l_ctx_out); + consider_logical_flow__(lflow, dp, l_ctx_in, l_ctx_out); } sbrec_logical_flow_index_destroy_row(lf_row); @@ -2687,9 +2512,7 @@ lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp, /* Don't call lflows_processed_add() because here we process the * lflow only for one of the DPs in the DP group, which may be * incomplete. */ - consider_logical_flow__(lflow, dp, &dhcp_opts, &dhcpv6_opts, - &nd_ra_opts, &controller_event_opts, - l_ctx_in, l_ctx_out); + consider_logical_flow__(lflow, dp, l_ctx_in, l_ctx_out); } } sbrec_logical_flow_index_destroy_row(lf_row); @@ -2731,11 +2554,6 @@ lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp, } sbrec_static_mac_binding_index_destroy_row(smb_index_row); - dhcp_opts_destroy(&dhcp_opts); - dhcp_opts_destroy(&dhcpv6_opts); - nd_ra_opts_destroy(&nd_ra_opts); - controller_event_opts_destroy(&controller_event_opts); - /* Add load balancer hairpin flows if the datapath has any load balancers * associated. */ for (size_t i = 0; i < n_dp_lbs; i++) { diff --git a/controller/lflow.h b/controller/lflow.h index 543d3cd96..3d1bb38dd 100644 --- a/controller/lflow.h +++ b/controller/lflow.h @@ -46,8 +46,6 @@ struct ovn_desired_flow_table; struct hmap; struct hmap_node; struct sbrec_chassis; -struct sbrec_dhcp_options_table; -struct sbrec_dhcpv6_options_table; struct sbrec_load_balancer; struct sbrec_logical_flow_table; struct sbrec_mac_binding_table; @@ -144,8 +142,6 @@ struct lflow_ctx_in { struct ovsdb_idl_index *sbrec_mac_binding_by_datapath; struct ovsdb_idl_index *sbrec_static_mac_binding_by_datapath; const struct sbrec_port_binding_table *port_binding_table; - const struct sbrec_dhcp_options_table *dhcp_options_table; - const struct sbrec_dhcpv6_options_table *dhcpv6_options_table; const struct sbrec_datapath_binding_table *dp_binding_table; const struct sbrec_mac_binding_table *mac_binding_table; const struct sbrec_logical_flow_table *logical_flow_table; @@ -162,7 +158,12 @@ struct lflow_ctx_in { const struct sset *related_lport_ids; const struct shash *binding_lports; const struct hmap *chassis_tunnels; + const struct hmap *nd_ra_opts; + const struct controller_event_options *controller_event_opts; bool lb_hairpin_use_ct_mark; + + struct hmap dhcp_opts; + struct hmap dhcpv6_opts; }; struct lflow_ctx_out { diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 6bedb91dc..18a01bbab 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -76,6 +76,7 @@ #include "timer.h" #include "stopwatch.h" #include "lib/inc-proc-eng.h" +#include "lib/ovn-l7.h" #include "hmapx.h" VLOG_DEFINE_THIS_MODULE(main); @@ -2538,6 +2539,12 @@ struct ed_type_lflow_output { /* Data for managing hairpin flow conjunctive flow ids. */ struct lflow_output_hairpin_data hd; + + /* Fixed neighbor discovery supported options. */ + struct hmap nd_ra_opts; + + /* Fixed controller_event supported options. */ + struct controller_event_options controller_event_opts; }; static void @@ -2655,8 +2662,6 @@ init_lflow_ctx(struct engine_node *node, l_ctx_in->sbrec_static_mac_binding_by_datapath = sbrec_static_mac_binding_by_datapath; l_ctx_in->port_binding_table = port_binding_table; - l_ctx_in->dhcp_options_table = dhcp_table; - l_ctx_in->dhcpv6_options_table = dhcpv6_table; l_ctx_in->mac_binding_table = mac_binding_table; l_ctx_in->logical_flow_table = logical_flow_table; l_ctx_in->logical_dp_group_table = logical_dp_group_table; @@ -2673,6 +2678,22 @@ init_lflow_ctx(struct engine_node *node, l_ctx_in->binding_lports = &rt_data->lbinding_data.lports; l_ctx_in->chassis_tunnels = &non_vif_data->chassis_tunnels; l_ctx_in->lb_hairpin_use_ct_mark = n_opts->lb_hairpin_use_ct_mark; + l_ctx_in->nd_ra_opts = &fo->nd_ra_opts; + l_ctx_in->controller_event_opts = &fo->controller_event_opts; + + hmap_init(&l_ctx_in->dhcp_opts); + const struct sbrec_dhcp_options *dhcp_opt_row; + SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, dhcp_table) { + dhcp_opt_add(&l_ctx_in->dhcp_opts, dhcp_opt_row->name, + dhcp_opt_row->code, dhcp_opt_row->type); + } + + hmap_init(&l_ctx_in->dhcpv6_opts); + const struct sbrec_dhcpv6_options *dhcpv6_opt_row; + SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row, dhcpv6_table) { + dhcp_opt_add(&l_ctx_in->dhcpv6_opts, dhcpv6_opt_row->name, + dhcpv6_opt_row->code, dhcpv6_opt_row->type); + } l_ctx_out->flow_table = &fo->flow_table; l_ctx_out->group_table = &fo->group_table; @@ -2685,6 +2706,14 @@ init_lflow_ctx(struct engine_node *node, l_ctx_out->hairpin_lb_ids = &fo->hd.ids; } +static void +destroy_lflow_ctx(struct lflow_ctx_in *l_ctx_in, + struct lflow_ctx_out *l_ctx_out OVS_UNUSED) +{ + dhcp_opts_destroy(&l_ctx_in->dhcp_opts); + dhcp_opts_destroy(&l_ctx_in->dhcpv6_opts); +} + static void * en_lflow_output_init(struct engine_node *node OVS_UNUSED, struct engine_arg *arg OVS_UNUSED) @@ -2698,6 +2727,8 @@ en_lflow_output_init(struct engine_node *node OVS_UNUSED, hmap_init(&data->lflows_processed); simap_init(&data->hd.ids); data->hd.pool = id_pool_create(1, UINT32_MAX - 1); + nd_ra_opts_init(&data->nd_ra_opts); + controller_event_opts_init(&data->controller_event_opts); return data; } @@ -2722,6 +2753,8 @@ en_lflow_output_cleanup(void *data) lflow_cache_destroy(flow_output_data->pd.lflow_cache); simap_destroy(&flow_output_data->hd.ids); id_pool_destroy(flow_output_data->hd.pool); + nd_ra_opts_destroy(&flow_output_data->nd_ra_opts); + controller_event_opts_destroy(&flow_output_data->controller_event_opts); } static void @@ -2771,6 +2804,7 @@ en_lflow_output_run(struct engine_node *node, void *data) struct lflow_ctx_out l_ctx_out; init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); lflow_run(&l_ctx_in, &l_ctx_out); + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); engine_set_node_state(node, EN_UPDATED); } @@ -2784,6 +2818,7 @@ lflow_output_sb_logical_flow_handler(struct engine_node *node, void *data) init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); bool handled = lflow_handle_changed_flows(&l_ctx_in, &l_ctx_out); + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); engine_set_node_state(node, EN_UPDATED); return handled; @@ -2846,12 +2881,12 @@ lflow_output_sb_multicast_group_handler(struct engine_node *node, void *data) struct lflow_ctx_in l_ctx_in; struct lflow_ctx_out l_ctx_out; init_lflow_ctx(node, lfo, &l_ctx_in, &l_ctx_out); - if (!lflow_handle_changed_mc_groups(&l_ctx_in, &l_ctx_out)) { - return false; - } + + bool handled = lflow_handle_changed_mc_groups(&l_ctx_in, &l_ctx_out); + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); engine_set_node_state(node, EN_UPDATED); - return true; + return handled; } static bool @@ -2862,12 +2897,12 @@ lflow_output_sb_port_binding_handler(struct engine_node *node, void *data) struct lflow_ctx_in l_ctx_in; struct lflow_ctx_out l_ctx_out; init_lflow_ctx(node, lfo, &l_ctx_in, &l_ctx_out); - if (!lflow_handle_changed_port_bindings(&l_ctx_in, &l_ctx_out)) { - return false; - } + + bool handled = lflow_handle_changed_port_bindings(&l_ctx_in, &l_ctx_out); + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); engine_set_node_state(node, EN_UPDATED); - return true; + return handled; } static bool @@ -2876,23 +2911,24 @@ lflow_output_addr_sets_handler(struct engine_node *node, void *data) struct ed_type_addr_sets *as_data = engine_get_input_data("addr_sets", node); - struct ed_type_lflow_output *fo = data; + if (!as_data->change_tracked) { + return false; + } - struct lflow_ctx_in l_ctx_in; + struct ed_type_lflow_output *fo = data; struct lflow_ctx_out l_ctx_out; - init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); - - bool changed; + struct lflow_ctx_in l_ctx_in; const char *ref_name; + bool handled = true; + bool changed; - if (!as_data->change_tracked) { - return false; - } + init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); SSET_FOR_EACH (ref_name, &as_data->deleted) { if (!lflow_handle_changed_ref(REF_TYPE_ADDRSET, ref_name, &l_ctx_in, &l_ctx_out, &changed)) { - return false; + handled = false; + goto done; } if (changed) { engine_set_node_state(node, EN_UPDATED); @@ -2907,7 +2943,8 @@ lflow_output_addr_sets_handler(struct engine_node *node, void *data) " Reprocess related lflows.", shash_node->name); if (!lflow_handle_changed_ref(REF_TYPE_ADDRSET, shash_node->name, &l_ctx_in, &l_ctx_out, &changed)) { - return false; + handled = false; + goto done; } } if (changed) { @@ -2917,14 +2954,17 @@ lflow_output_addr_sets_handler(struct engine_node *node, void *data) SSET_FOR_EACH (ref_name, &as_data->new) { if (!lflow_handle_changed_ref(REF_TYPE_ADDRSET, ref_name, &l_ctx_in, &l_ctx_out, &changed)) { - return false; + handled = false; + goto done; } if (changed) { engine_set_node_state(node, EN_UPDATED); } } - return true; +done: + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); + return handled; } static bool @@ -2933,23 +2973,24 @@ lflow_output_port_groups_handler(struct engine_node *node, void *data) struct ed_type_port_groups *pg_data = engine_get_input_data("port_groups", node); - struct ed_type_lflow_output *fo = data; + if (!pg_data->change_tracked) { + return false; + } - struct lflow_ctx_in l_ctx_in; + struct ed_type_lflow_output *fo = data; struct lflow_ctx_out l_ctx_out; - init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); - - bool changed; + struct lflow_ctx_in l_ctx_in; const char *ref_name; + bool handled = true; + bool changed; - if (!pg_data->change_tracked) { - return false; - } + init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); SSET_FOR_EACH (ref_name, &pg_data->deleted) { if (!lflow_handle_changed_ref(REF_TYPE_PORTGROUP, ref_name, &l_ctx_in, &l_ctx_out, &changed)) { - return false; + handled = false; + goto done; } if (changed) { engine_set_node_state(node, EN_UPDATED); @@ -2958,7 +2999,8 @@ lflow_output_port_groups_handler(struct engine_node *node, void *data) SSET_FOR_EACH (ref_name, &pg_data->updated) { if (!lflow_handle_changed_ref(REF_TYPE_PORTGROUP, ref_name, &l_ctx_in, &l_ctx_out, &changed)) { - return false; + handled = false; + goto done; } if (changed) { engine_set_node_state(node, EN_UPDATED); @@ -2967,14 +3009,17 @@ lflow_output_port_groups_handler(struct engine_node *node, void *data) SSET_FOR_EACH (ref_name, &pg_data->new) { if (!lflow_handle_changed_ref(REF_TYPE_PORTGROUP, ref_name, &l_ctx_in, &l_ctx_out, &changed)) { - return false; + handled = false; + goto done; } if (changed) { engine_set_node_state(node, EN_UPDATED); } } - return true; +done: + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); + return handled; } static bool @@ -3002,6 +3047,8 @@ lflow_output_runtime_data_handler(struct engine_node *node, struct lflow_ctx_out l_ctx_out; struct ed_type_lflow_output *fo = data; struct hmap *lbs = NULL; + bool handled = true; + init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); struct tracked_datapath *tdp; @@ -3018,7 +3065,8 @@ lflow_output_runtime_data_handler(struct engine_node *node, tdp->dp, lbs_by_dp ? lbs_by_dp->dp_lbs : NULL, lbs_by_dp ? lbs_by_dp->n_dp_lbs : 0, &l_ctx_in, &l_ctx_out)) { - return false; + handled = false; + goto done; } } struct shash_node *shash_node; @@ -3026,14 +3074,18 @@ lflow_output_runtime_data_handler(struct engine_node *node, struct tracked_lport *lport = shash_node->data; if (!lflow_handle_flows_for_lport(lport->pb, &l_ctx_in, &l_ctx_out)) { - return false; + handled = false; + goto done; } } } load_balancers_by_dp_cleanup(lbs); engine_set_node_state(node, EN_UPDATED); - return true; + +done: + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); + return handled; } static bool @@ -3045,6 +3097,7 @@ lflow_output_sb_load_balancer_handler(struct engine_node *node, void *data) init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); bool handled = lflow_handle_changed_lbs(&l_ctx_in, &l_ctx_out); + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); engine_set_node_state(node, EN_UPDATED); return handled; @@ -3059,6 +3112,7 @@ lflow_output_sb_fdb_handler(struct engine_node *node, void *data) init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out); bool handled = lflow_handle_changed_fdbs(&l_ctx_in, &l_ctx_out); + destroy_lflow_ctx(&l_ctx_in, &l_ctx_out); engine_set_node_state(node, EN_UPDATED); return handled; diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h index 0b2da9f7b..2c7d0add4 100644 --- a/lib/ovn-l7.h +++ b/lib/ovn-l7.h @@ -412,6 +412,8 @@ nd_ra_opts_destroy(struct hmap *nd_ra_opts) static inline void nd_ra_opts_init(struct hmap *nd_ra_opts) { + hmap_init(nd_ra_opts); + nd_ra_opt_add(nd_ra_opts, "addr_mode", ND_RA_FLAG_ADDR_MODE, "str"); nd_ra_opt_add(nd_ra_opts, "router_preference", ND_RA_FLAG_PRF, "str"); nd_ra_opt_add(nd_ra_opts, "slla", ND_OPT_SOURCE_LINKADDR, "mac");
The nd_ra_opts and controller_event_ops are actually static they never change at runtime. DHCP records can instead be computed when populating the lflow "input context" to be used during incremental processing. This is likely more efficient than building the DHCP opts maps for every logical flow recomputed incrementally. An even more efficient solution would be to introduce proper incremental processing for the DHCP opt maps but that seems like too much complexity without enough benefit. It's probably OK to recompute the maps at every ovn-controller iteration. Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- controller/lflow.c | 206 +++---------------------------------------- controller/lflow.h | 9 +- controller/ovn-controller.c | 128 +++++++++++++++++++-------- lib/ovn-l7.h | 2 4 files changed, 110 insertions(+), 235 deletions(-)