On 9/22/22 08:24, Han Zhou wrote: > On Wed, Sep 14, 2022 at 6:09 AM Dumitru Ceara <[email protected]> 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 <[email protected]> >> --- >> 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
