On Wed, May 5, 2021 at 8:34 PM Han Zhou <[email protected]> wrote: > > On Thu, Apr 29, 2021 at 8:10 AM Numan Siddique <[email protected]> wrote: > > > > On Mon, Apr 26, 2021 at 7:18 PM Han Zhou <[email protected]> wrote: > > > > > > On Mon, Apr 12, 2021 at 6:23 AM <[email protected]> wrote: > > > > > > > > From: Numan Siddique <[email protected]> > > > > > > > > Presently, the 'flow_output' engine node recomputes physical > > > > flows by calling physical_run() in the 'physical_flow_changes' > > > > handler in some scenarios. Because of this, an engine run can > > > > do a full recompute of physical flows but not full recompute > > > > of logical flows. Although this works now, it is problematic > > > > as the same desired flow table is used for both physical and > > > > logical flows. > > > > > > > > This patch now separates the handling of logical flows and > > > > physical flows and removes the 'physical_flow_changes' engine > > > > node. Two separate engine nodes are added - lflow_output and > > > > pflow_output with their own flow tables and these two nodes are > > > > now inputs to the main engine node - flow_output. This separation > > > > reflects the data dependency more clearly. > > > > > > > > CC: Han Zhou <[email protected]> > > > > Signed-off-by: Numan Siddique <[email protected]> > > > > --- > > > > v5 -> v6 > > > > ---- > > > > * Missed out checking in the uncommitted code in ofctrl.c in v4. v5 > > > > fixes it. > > > > * v4 accidently modified ovs submodule commit id. v5 reverts it. > > > > > > > > > > Thanks Numan for the revision. I think you meant "v6" fixes/reverts ..., > > > right? > > > > Thanks for the review comments. > > > > Seems like a typo from me. I will correct it in v7. > > > > > Anyway, please see some more comments below. The major comments are > > > regarding the noop handler usage for lflow_output and pflow_output. > Others > > > are very minor ones. > > > > > > Han > > > > > > > v4 -> v5 > > > > ----- > > > > * Addressed Han's comments. > > > > > > > > v3 -> v4 > > > > ----- > > > > * Addressed Mark G's comments. > > > > * Rebased to resolve conflicts. > > > > > > > > v2 -> v3 > > > > ----- > > > > * Rebased to resolve conflicts. > > > > > > > > v1 -> v2 > > > > ----- > > > > * Rebased to resolve conflicts. > > > > > > > > TODO.rst | 6 + > > > > controller/ofctrl.c | 91 +++-- > > > > controller/ofctrl.h | 6 +- > > > > controller/ovn-controller.c | 687 > ++++++++++++++++++------------------ > > > > 4 files changed, 419 insertions(+), 371 deletions(-) > > > > > > > > diff --git a/TODO.rst b/TODO.rst > > > > index ecfe62870f..0a14b5219a 100644 > > > > --- a/TODO.rst > > > > +++ b/TODO.rst > > > > @@ -166,3 +166,9 @@ OVN To-do List > > > > to find a way of determining if routing has already been executed > > > (on a > > > > different hypervisor) for the IP multicast packet being processed > > > locally > > > > in the router pipeline. > > > > + > > > > +* ovn-controller Incremental processing > > > > + > > > > + * physical.c has a global simap -localvif_to_ofport which stores > the > > > > + local OVS interfaces and the ofport numbers. Move this to the > engine > > > data > > > > + of the engine data node - ed_type_pflow_output. > > > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > > > > index 415d9b7e16..346b791f78 100644 > > > > --- a/controller/ofctrl.c > > > > +++ b/controller/ofctrl.c > > > > @@ -172,7 +172,7 @@ struct sb_flow_ref { > > > > struct uuid sb_uuid; > > > > }; > > > > > > > > -/* A installed flow, in static variable installed_flows. > > > > +/* A installed flow, in static variable > > > installed_lflows/installed_pflows. > > > > * > > > > * Installed flows are updated in ofctrl_put for maintaining the flow > > > > * installation to OVS. They are updated according to desired flows: > > > either by > > > > @@ -233,7 +233,7 @@ static struct desired_flow > > > *desired_flow_lookup_conjunctive( > > > > static void desired_flow_destroy(struct desired_flow *); > > > > > > > > static struct installed_flow *installed_flow_lookup( > > > > - const struct ovn_flow *target); > > > > + const struct ovn_flow *target, struct hmap *installed_flows); > > > > static void installed_flow_destroy(struct installed_flow *); > > > > static struct installed_flow *installed_flow_dup(struct desired_flow > *); > > > > static struct desired_flow *installed_flow_get_active(struct > > > installed_flow *); > > > > @@ -301,9 +301,12 @@ static ovs_be32 xid, xid2; > > > > * zero, to avoid unbounded buffering. */ > > > > static struct rconn_packet_counter *tx_counter; > > > > > > > > -/* Flow table of "struct ovn_flow"s, that holds the flow table > currently > > > > - * installed in the switch. */ > > > > -static struct hmap installed_flows; > > > > +/* Flow table of "struct ovn_flow"s, that holds the logical flow > table > > > > + * currently installed in the switch. */ > > > > +static struct hmap installed_lflows; > > > > +/* Flow table of "struct ovn_flow"s, that holds the physical flow > table > > > > + * currently installed in the switch. */ > > > > +static struct hmap installed_pflows; > > > > > > > > /* A reference to the group_table. */ > > > > static struct ovn_extend_table *groups; > > > > @@ -342,7 +345,8 @@ ofctrl_init(struct ovn_extend_table *group_table, > > > > swconn = rconn_create(inactivity_probe_interval, 0, > > > > DSCP_DEFAULT, 1 << OFP15_VERSION); > > > > tx_counter = rconn_packet_counter_create(); > > > > - hmap_init(&installed_flows); > > > > + hmap_init(&installed_lflows); > > > > + hmap_init(&installed_pflows); > > > > ovs_list_init(&flow_updates); > > > > ovn_init_symtab(&symtab); > > > > groups = group_table; > > > > @@ -1425,11 +1429,11 @@ desired_flow_lookup_conjunctive(struct > > > ovn_desired_flow_table *flow_table, > > > > /* Finds and returns an installed_flow in installed_flows whose key > is > > > > * identical to 'target''s key, or NULL if there is none. */ > > > > static struct installed_flow * > > > > -installed_flow_lookup(const struct ovn_flow *target) > > > > +installed_flow_lookup(const struct ovn_flow *target, struct hmap > > > *installed_flows) > > > > { > > > > struct installed_flow *i; > > > > HMAP_FOR_EACH_WITH_HASH (i, match_hmap_node, target->hash, > > > > - &installed_flows) { > > > > + installed_flows) { > > > > struct ovn_flow *f = &i->flow; > > > > if (f->table_id == target->table_id > > > > && f->priority == target->priority > > > > @@ -1541,8 +1545,14 @@ static void > > > > ovn_installed_flow_table_clear(void) > > > > { > > > > struct installed_flow *f, *next; > > > > - HMAP_FOR_EACH_SAFE (f, next, match_hmap_node, &installed_flows) { > > > > - hmap_remove(&installed_flows, &f->match_hmap_node); > > > > + HMAP_FOR_EACH_SAFE (f, next, match_hmap_node, &installed_lflows) > { > > > > + hmap_remove(&installed_lflows, &f->match_hmap_node); > > > > + unlink_all_refs_for_installed_flow(f); > > > > + installed_flow_destroy(f); > > > > + } > > > > + > > > > + HMAP_FOR_EACH_SAFE (f, next, match_hmap_node, &installed_pflows) > { > > > > + hmap_remove(&installed_pflows, &f->match_hmap_node); > > > > unlink_all_refs_for_installed_flow(f); > > > > installed_flow_destroy(f); > > > > } > > > > @@ -1552,7 +1562,8 @@ static void > > > > ovn_installed_flow_table_destroy(void) > > > > { > > > > ovn_installed_flow_table_clear(); > > > > - hmap_destroy(&installed_flows); > > > > + hmap_destroy(&installed_lflows); > > > > + hmap_destroy(&installed_pflows); > > > > } > > > > > > > > /* Flow table update. */ > > > > @@ -1810,6 +1821,7 @@ installed_flow_del(struct ovn_flow *i, struct > > > ovs_list *msgs) > > > > > > > > static void > > > > update_installed_flows_by_compare(struct ovn_desired_flow_table > > > *flow_table, > > > > + struct hmap *installed_flows, > > > > struct ovs_list *msgs) > > > > { > > > > ovs_assert(ovs_list_is_empty(&flow_table->tracked_flows)); > > > > @@ -1817,7 +1829,7 @@ update_installed_flows_by_compare(struct > > > ovn_desired_flow_table *flow_table, > > > > * longer desired, delete them; if any of them should have > different > > > > * actions, update them. */ > > > > struct installed_flow *i, *next; > > > > - HMAP_FOR_EACH_SAFE (i, next, match_hmap_node, &installed_flows) { > > > > + HMAP_FOR_EACH_SAFE (i, next, match_hmap_node, installed_flows) { > > > > unlink_all_refs_for_installed_flow(i); > > > > struct desired_flow *d = desired_flow_lookup(flow_table, > > > &i->flow); > > > > if (!d) { > > > > @@ -1826,7 +1838,7 @@ update_installed_flows_by_compare(struct > > > ovn_desired_flow_table *flow_table, > > > > installed_flow_del(&i->flow, msgs); > > > > ovn_flow_log(&i->flow, "removing installed"); > > > > > > > > - hmap_remove(&installed_flows, &i->match_hmap_node); > > > > + hmap_remove(installed_flows, &i->match_hmap_node); > > > > installed_flow_destroy(i); > > > > } else { > > > > if (!ofpacts_equal(i->flow.ofpacts, i->flow.ofpacts_len, > > > > @@ -1844,14 +1856,14 @@ update_installed_flows_by_compare(struct > > > ovn_desired_flow_table *flow_table, > > > > * in the installed flow table. */ > > > > struct desired_flow *d; > > > > HMAP_FOR_EACH (d, match_hmap_node, > &flow_table->match_flow_table) { > > > > - i = installed_flow_lookup(&d->flow); > > > > + i = installed_flow_lookup(&d->flow, installed_flows); > > > > if (!i) { > > > > ovn_flow_log(&d->flow, "adding installed"); > > > > installed_flow_add(&d->flow, msgs); > > > > > > > > /* Copy 'd' from 'flow_table' to installed_flows. */ > > > > i = installed_flow_dup(d); > > > > - hmap_insert(&installed_flows, &i->match_hmap_node, > > > i->flow.hash); > > > > + hmap_insert(installed_flows, &i->match_hmap_node, > > > i->flow.hash); > > > > link_installed_to_desired(i, d); > > > > } else if (!d->installed_flow) { > > > > /* This is a desired_flow that conflicts with one > installed > > > > @@ -1941,6 +1953,7 @@ merge_tracked_flows(struct > ovn_desired_flow_table > > > *flow_table) > > > > > > > > static void > > > > update_installed_flows_by_track(struct ovn_desired_flow_table > > > *flow_table, > > > > + struct hmap *installed_flows, > > > > struct ovs_list *msgs) > > > > { > > > > merge_tracked_flows(flow_table); > > > > @@ -1959,7 +1972,7 @@ update_installed_flows_by_track(struct > > > ovn_desired_flow_table *flow_table, > > > > installed_flow_del(&i->flow, msgs); > > > > ovn_flow_log(&i->flow, "removing installed > > > (tracked)"); > > > > > > > > - hmap_remove(&installed_flows, > &i->match_hmap_node); > > > > + hmap_remove(installed_flows, > &i->match_hmap_node); > > > > installed_flow_destroy(i); > > > > } else if (was_active) { > > > > /* There are other desired flow(s) referencing > this > > > > @@ -1973,7 +1986,8 @@ update_installed_flows_by_track(struct > > > ovn_desired_flow_table *flow_table, > > > > desired_flow_destroy(f); > > > > } else { > > > > /* The desired flow was added or modified. */ > > > > - struct installed_flow *i = > installed_flow_lookup(&f->flow); > > > > + struct installed_flow *i = > installed_flow_lookup(&f->flow, > > > > + > > > installed_flows); > > > > if (!i) { > > > > /* Adding a new flow. */ > > > > installed_flow_add(&f->flow, msgs); > > > > @@ -1981,7 +1995,7 @@ update_installed_flows_by_track(struct > > > ovn_desired_flow_table *flow_table, > > > > > > > > /* Copy 'f' from 'flow_table' to installed_flows. */ > > > > struct installed_flow *new_node = > installed_flow_dup(f); > > > > - hmap_insert(&installed_flows, > &new_node->match_hmap_node, > > > > + hmap_insert(installed_flows, > &new_node->match_hmap_node, > > > > new_node->flow.hash); > > > > link_installed_to_desired(new_node, f); > > > > } else if (installed_flow_get_active(i) == f) { > > > > @@ -2035,16 +2049,19 @@ ofctrl_can_put(void) > > > > * > > > > * This should be called after ofctrl_run() within the main loop. */ > > > > void > > > > -ofctrl_put(struct ovn_desired_flow_table *flow_table, > > > > +ofctrl_put(struct ovn_desired_flow_table *lflow_table, > > > > + struct ovn_desired_flow_table *pflow_table, > > > > struct shash *pending_ct_zones, > > > > const struct sbrec_meter_table *meter_table, > > > > uint64_t req_cfg, > > > > - bool flow_changed) > > > > + bool lflows_changed, > > > > + bool pflows_changed) > > > > { > > > > static bool skipped_last_time = false; > > > > static uint64_t old_req_cfg = 0; > > > > bool need_put = false; > > > > - if (flow_changed || skipped_last_time || need_reinstall_flows) { > > > > + if (lflows_changed || pflows_changed || skipped_last_time || > > > > + need_reinstall_flows) { > > > > need_put = true; > > > > old_req_cfg = req_cfg; > > > > } else if (req_cfg != old_req_cfg) { > > > > @@ -2126,10 +2143,24 @@ ofctrl_put(struct ovn_desired_flow_table > > > *flow_table, > > > > } > > > > } > > > > > > > > - if (flow_table->change_tracked) { > > > > - update_installed_flows_by_track(flow_table, &msgs); > > > > - } else { > > > > - update_installed_flows_by_compare(flow_table, &msgs); > > > > + if (lflows_changed) { > > > > + if (lflow_table->change_tracked) { > > > > + update_installed_flows_by_track(lflow_table, > > > &installed_lflows, > > > > + &msgs); > > > > + } else { > > > > + update_installed_flows_by_compare(lflow_table, > > > &installed_lflows, > > > > + &msgs); > > > > + } > > > > + } > > > > + > > > > + if (pflows_changed) { > > > > + if (pflow_table->change_tracked) { > > > > + update_installed_flows_by_track(pflow_table, > > > &installed_pflows, > > > > + &msgs); > > > > + } else { > > > > + update_installed_flows_by_compare(pflow_table, > > > &installed_pflows, > > > > + &msgs); > > > > + } > > > > } > > > > > > > > /* Iterate through the installed groups from previous runs. If > they > > > > @@ -2243,8 +2274,14 @@ ofctrl_put(struct ovn_desired_flow_table > > > *flow_table, > > > > cur_cfg = req_cfg; > > > > } > > > > > > > > - flow_table->change_tracked = true; > > > > - ovs_assert(ovs_list_is_empty(&flow_table->tracked_flows)); > > > > + lflow_table->change_tracked = true; > > > > + if (lflows_changed) { > > > > > > We should just assert both lflow & pflow table's tracked_flows should be > > > empty, without the "if" conditions. > > > > Ack. > > > > > > > > > + ovs_assert(ovs_list_is_empty(&lflow_table->tracked_flows)); > > > > + } > > > > + pflow_table->change_tracked = true; > > > > + if (pflows_changed) { > > > > + ovs_assert(ovs_list_is_empty(&pflow_table->tracked_flows)); > > > > + } > > > > } > > > > > > > > /* Looks up the logical port with the name 'port_name' in > 'br_int_'. If > > > > diff --git a/controller/ofctrl.h b/controller/ofctrl.h > > > > index 88769566ac..ead8088c5b 100644 > > > > --- a/controller/ofctrl.h > > > > +++ b/controller/ofctrl.h > > > > @@ -52,11 +52,13 @@ void ofctrl_init(struct ovn_extend_table > *group_table, > > > > void ofctrl_run(const struct ovsrec_bridge *br_int, > > > > struct shash *pending_ct_zones); > > > > enum mf_field_id ofctrl_get_mf_field_id(void); > > > > -void ofctrl_put(struct ovn_desired_flow_table *, > > > > +void ofctrl_put(struct ovn_desired_flow_table *lflow_table, > > > > + struct ovn_desired_flow_table *pflow_table, > > > > struct shash *pending_ct_zones, > > > > const struct sbrec_meter_table *, > > > > uint64_t nb_cfg, > > > > - bool flow_changed); > > > > + bool lflow_changed, > > > > + bool pflow_changed); > > > > bool ofctrl_can_put(void); > > > > void ofctrl_wait(void); > > > > void ofctrl_destroy(void); > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > > > index 16c8ecb21c..6bcc8264c6 100644 > > > > --- a/controller/ovn-controller.c > > > > +++ b/controller/ovn-controller.c > > > > @@ -1646,106 +1646,13 @@ en_mff_ovn_geneve_run(struct engine_node > *node, > > > void *data) > > > > engine_set_node_state(node, EN_UNCHANGED); > > > > } > > > > > > > > -/* Engine node en_physical_flow_changes indicates whether > > > > - * there is a need to > > > > - * - recompute only physical flows or > > > > - * - we can incrementally process the physical flows. > > > > - * > > > > - * en_physical_flow_changes is an input to flow_output engine node. > > > > - * If the engine node 'en_physical_flow_changes' gets updated during > > > > - * engine run, it means the handler for this - > > > > - * flow_output_physical_flow_changes_handler() will either > > > > - * - recompute the physical flows by calling 'physical_run() or > > > > - * - incrementlly process some of the changes for physical flow > > > > - * calculation. Right now we handle OVS interfaces changes > > > > - * for physical flow computation. > > > > - * > > > > - * When ever a port binding happens, the follow up > > > > - * activity is the zone id allocation for that port binding. > > > > - * With this intermediate engine node, we avoid full recomputation. > > > > - * Instead we do physical flow computation (either full recomputation > > > > - * by calling physical_run() or handling the changes incrementally. > > > > - * > > > > - * Hence this is an intermediate engine node to indicate the > > > > - * flow_output engine to recomputes/compute the physical flows. > > > > - * > > > > - * TODO 1. Ideally this engine node should recompute/compute the > physical > > > > - * flows instead of relegating it to the flow_output node. > > > > - * But this requires splitting the flow_output node to > > > > - * logical_flow_output and physical_flow_output. > > > > - * > > > > - * TODO 2. We can further optimise the en_ct_zone changes to > > > > - * compute the phsyical flows for changed zone ids. > > > > - * > > > > - * TODO 3: physical.c has a global simap -localvif_to_ofport which > > > stores the > > > > - * local OVS interfaces and the ofport numbers. Ideally this > > > should be > > > > - * part of the engine data. > > > > - */ > > > > -struct ed_type_pfc_data { > > > > - /* Both these variables are tracked and set in each engine run. > */ > > > > - bool recompute_physical_flows; > > > > - bool ovs_ifaces_changed; > > > > -}; > > > > - > > > > -static void > > > > -en_physical_flow_changes_clear_tracked_data(void *data_) > > > > -{ > > > > - struct ed_type_pfc_data *data = data_; > > > > - data->recompute_physical_flows = false; > > > > - data->ovs_ifaces_changed = false; > > > > -} > > > > - > > > > -static void * > > > > -en_physical_flow_changes_init(struct engine_node *node OVS_UNUSED, > > > > - struct engine_arg *arg OVS_UNUSED) > > > > -{ > > > > - struct ed_type_pfc_data *data = xzalloc(sizeof *data); > > > > - return data; > > > > -} > > > > - > > > > -static void > > > > -en_physical_flow_changes_cleanup(void *data OVS_UNUSED) > > > > -{ > > > > -} > > > > - > > > > -/* Indicate to the flow_output engine that we need to recompute > physical > > > > - * flows. */ > > > > -static void > > > > -en_physical_flow_changes_run(struct engine_node *node, void *data) > > > > -{ > > > > - struct ed_type_pfc_data *pfc_tdata = data; > > > > - pfc_tdata->recompute_physical_flows = true; > > > > - engine_set_node_state(node, EN_UPDATED); > > > > -} > > > > - > > > > -/* ct_zone changes are not handled incrementally but a handler is > > > required > > > > - * to avoid skipping the ovs_iface incremental change handler. > > > > - */ > > > > -static bool > > > > -physical_flow_changes_ct_zones_handler(struct engine_node *node > > > OVS_UNUSED, > > > > - void *data OVS_UNUSED) > > > > -{ > > > > - return false; > > > > -} > > > > - > > > > -/* There are OVS interface changes. Indicate to the flow_output > engine > > > > - * to handle these OVS interface changes for physical flow > computations. > > > */ > > > > -static bool > > > > -physical_flow_changes_ovs_iface_handler(struct engine_node *node, > void > > > *data) > > > > -{ > > > > - struct ed_type_pfc_data *pfc_tdata = data; > > > > - pfc_tdata->ovs_ifaces_changed = true; > > > > - engine_set_node_state(node, EN_UPDATED); > > > > - return true; > > > > -} > > > > - > > > > -struct flow_output_persistent_data { > > > > +struct lflow_output_persistent_data { > > > > uint32_t conj_id_ofs; > > > > struct lflow_cache *lflow_cache; > > > > }; > > > > > > > > -struct ed_type_flow_output { > > > > - /* desired flows */ > > > > +struct ed_type_lflow_output { > > > > + /* Logical flow table */ > > > > struct ovn_desired_flow_table flow_table; > > > > /* group ids for load balancing */ > > > > struct ovn_extend_table group_table; > > > > @@ -1756,81 +1663,15 @@ struct ed_type_flow_output { > > > > > > > > /* Data which is persistent and not cleared during > > > > * full recompute. */ > > > > - struct flow_output_persistent_data pd; > > > > + struct lflow_output_persistent_data pd; > > > > }; > > > > > > > > -static void init_physical_ctx(struct engine_node *node, > > > > - struct ed_type_runtime_data *rt_data, > > > > - struct physical_ctx *p_ctx) > > > > -{ > > > > - struct ovsdb_idl_index *sbrec_port_binding_by_name = > > > > - engine_ovsdb_node_get_index( > > > > - engine_get_input("SB_port_binding", node), > > > > - "name"); > > > > - > > > > - struct sbrec_multicast_group_table *multicast_group_table = > > > > - (struct sbrec_multicast_group_table *)EN_OVSDB_GET( > > > > - engine_get_input("SB_multicast_group", node)); > > > > - > > > > - struct sbrec_port_binding_table *port_binding_table = > > > > - (struct sbrec_port_binding_table *)EN_OVSDB_GET( > > > > - engine_get_input("SB_port_binding", node)); > > > > - > > > > - struct sbrec_chassis_table *chassis_table = > > > > - (struct sbrec_chassis_table *)EN_OVSDB_GET( > > > > - engine_get_input("SB_chassis", node)); > > > > - > > > > - struct ed_type_mff_ovn_geneve *ed_mff_ovn_geneve = > > > > - engine_get_input_data("mff_ovn_geneve", node); > > > > - > > > > - struct ovsrec_open_vswitch_table *ovs_table = > > > > - (struct ovsrec_open_vswitch_table *)EN_OVSDB_GET( > > > > - engine_get_input("OVS_open_vswitch", node)); > > > > - struct ovsrec_bridge_table *bridge_table = > > > > - (struct ovsrec_bridge_table *)EN_OVSDB_GET( > > > > - engine_get_input("OVS_bridge", node)); > > > > - const struct ovsrec_bridge *br_int = get_br_int(bridge_table, > > > ovs_table); > > > > - const char *chassis_id = get_ovs_chassis_id(ovs_table); > > > > - const struct sbrec_chassis *chassis = NULL; > > > > - struct ovsdb_idl_index *sbrec_chassis_by_name = > > > > - engine_ovsdb_node_get_index( > > > > - engine_get_input("SB_chassis", node), > > > > - "name"); > > > > - if (chassis_id) { > > > > - chassis = chassis_lookup_by_name(sbrec_chassis_by_name, > > > chassis_id); > > > > - } > > > > - > > > > - ovs_assert(br_int && chassis); > > > > - > > > > - struct ovsrec_interface_table *iface_table = > > > > - (struct ovsrec_interface_table *)EN_OVSDB_GET( > > > > - engine_get_input("OVS_interface", node)); > > > > - > > > > - struct ed_type_ct_zones *ct_zones_data = > > > > - engine_get_input_data("ct_zones", node); > > > > - struct simap *ct_zones = &ct_zones_data->current; > > > > - > > > > - p_ctx->sbrec_port_binding_by_name = sbrec_port_binding_by_name; > > > > - p_ctx->port_binding_table = port_binding_table; > > > > - p_ctx->mc_group_table = multicast_group_table; > > > > - p_ctx->br_int = br_int; > > > > - p_ctx->chassis_table = chassis_table; > > > > - p_ctx->iface_table = iface_table; > > > > - p_ctx->chassis = chassis; > > > > - p_ctx->active_tunnels = &rt_data->active_tunnels; > > > > - p_ctx->local_datapaths = &rt_data->local_datapaths; > > > > - p_ctx->local_lports = &rt_data->local_lports; > > > > - p_ctx->ct_zones = ct_zones; > > > > - p_ctx->mff_ovn_geneve = ed_mff_ovn_geneve->mff_ovn_geneve; > > > > - p_ctx->local_bindings = &rt_data->lbinding_data.bindings; > > > > - p_ctx->ct_updated_datapaths = &rt_data->ct_updated_datapaths; > > > > -} > > > > - > > > > -static void init_lflow_ctx(struct engine_node *node, > > > > - struct ed_type_runtime_data *rt_data, > > > > - struct ed_type_flow_output *fo, > > > > - struct lflow_ctx_in *l_ctx_in, > > > > - struct lflow_ctx_out *l_ctx_out) > > > > +static void > > > > +init_lflow_ctx(struct engine_node *node, > > > > + struct ed_type_runtime_data *rt_data, > > > > + struct ed_type_lflow_output *fo, > > > > + struct lflow_ctx_in *l_ctx_in, > > > > + struct lflow_ctx_out *l_ctx_out) > > > > { > > > > struct ovsdb_idl_index *sbrec_port_binding_by_name = > > > > engine_ovsdb_node_get_index( > > > > @@ -1940,11 +1781,10 @@ static void init_lflow_ctx(struct engine_node > > > *node, > > > > } > > > > > > > > static void * > > > > -en_flow_output_init(struct engine_node *node OVS_UNUSED, > > > > - struct engine_arg *arg OVS_UNUSED) > > > > +en_lflow_output_init(struct engine_node *node OVS_UNUSED, > > > > + struct engine_arg *arg OVS_UNUSED) > > > > { > > > > - struct ed_type_flow_output *data = xzalloc(sizeof *data); > > > > - > > > > + struct ed_type_lflow_output *data = xzalloc(sizeof *data); > > > > ovn_desired_flow_table_init(&data->flow_table); > > > > ovn_extend_table_init(&data->group_table); > > > > ovn_extend_table_init(&data->meter_table); > > > > @@ -1954,9 +1794,9 @@ en_flow_output_init(struct engine_node *node > > > OVS_UNUSED, > > > > } > > > > > > > > static void > > > > -en_flow_output_cleanup(void *data) > > > > +en_lflow_output_cleanup(void *data) > > > > { > > > > - struct ed_type_flow_output *flow_output_data = data; > > > > + struct ed_type_lflow_output *flow_output_data = data; > > > > ovn_desired_flow_table_destroy(&flow_output_data->flow_table); > > > > ovn_extend_table_destroy(&flow_output_data->group_table); > > > > ovn_extend_table_destroy(&flow_output_data->meter_table); > > > > @@ -1965,7 +1805,7 @@ en_flow_output_cleanup(void *data) > > > > } > > > > > > > > static void > > > > -en_flow_output_run(struct engine_node *node, void *data) > > > > +en_lflow_output_run(struct engine_node *node, void *data) > > > > { > > > > struct ed_type_runtime_data *rt_data = > > > > engine_get_input_data("runtime_data", node); > > > > @@ -1991,8 +1831,8 @@ en_flow_output_run(struct engine_node *node, > void > > > *data) > > > > > > > > ovs_assert(br_int && chassis); > > > > > > > > - struct ed_type_flow_output *fo = data; > > > > - struct ovn_desired_flow_table *flow_table = &fo->flow_table; > > > > + struct ed_type_lflow_output *fo = data; > > > > + struct ovn_desired_flow_table *lflow_table = &fo->flow_table; > > > > struct ovn_extend_table *group_table = &fo->group_table; > > > > struct ovn_extend_table *meter_table = &fo->meter_table; > > > > struct lflow_resource_ref *lfrr = &fo->lflow_resource_ref; > > > > @@ -2001,7 +1841,7 @@ en_flow_output_run(struct engine_node *node, > void > > > *data) > > > > if (first_run) { > > > > first_run = false; > > > > } else { > > > > - ovn_desired_flow_table_clear(flow_table); > > > > + ovn_desired_flow_table_clear(lflow_table); > > > > ovn_extend_table_clear(group_table, false /* desired */); > > > > ovn_extend_table_clear(meter_table, false /* desired */); > > > > lflow_resource_clear(lfrr); > > > > @@ -2023,7 +1863,7 @@ en_flow_output_run(struct engine_node *node, > void > > > *data) > > > > if (l_ctx_out.conj_id_overflow) { > > > > /* Conjunction ids overflow. There can be many holes in > between. > > > > * Destroy lflow cache and call lflow_run() again. */ > > > > - ovn_desired_flow_table_clear(flow_table); > > > > + ovn_desired_flow_table_clear(lflow_table); > > > > ovn_extend_table_clear(group_table, false /* desired */); > > > > ovn_extend_table_clear(meter_table, false /* desired */); > > > > lflow_resource_clear(lfrr); > > > > @@ -2036,16 +1876,11 @@ en_flow_output_run(struct engine_node *node, > void > > > *data) > > > > } > > > > } > > > > > > > > - struct physical_ctx p_ctx; > > > > - init_physical_ctx(node, rt_data, &p_ctx); > > > > - > > > > - physical_run(&p_ctx, &fo->flow_table); > > > > - > > > > engine_set_node_state(node, EN_UPDATED); > > > > } > > > > > > > > static bool > > > > -flow_output_sb_logical_flow_handler(struct engine_node *node, void > *data) > > > > +lflow_output_sb_logical_flow_handler(struct engine_node *node, void > > > *data) > > > > { > > > > struct ed_type_runtime_data *rt_data = > > > > engine_get_input_data("runtime_data", node); > > > > @@ -2058,7 +1893,7 @@ flow_output_sb_logical_flow_handler(struct > > > engine_node *node, void *data) > > > > const struct ovsrec_bridge *br_int = get_br_int(bridge_table, > > > ovs_table); > > > > ovs_assert(br_int); > > > > > > > > - struct ed_type_flow_output *fo = data; > > > > + struct ed_type_lflow_output *fo = data; > > > > struct lflow_ctx_in l_ctx_in; > > > > struct lflow_ctx_out l_ctx_out; > > > > init_lflow_ctx(node, rt_data, fo, &l_ctx_in, &l_ctx_out); > > > > @@ -2070,7 +1905,7 @@ flow_output_sb_logical_flow_handler(struct > > > engine_node *node, void *data) > > > > } > > > > > > > > static bool > > > > -flow_output_sb_mac_binding_handler(struct engine_node *node, void > *data) > > > > +lflow_output_sb_mac_binding_handler(struct engine_node *node, void > *data) > > > > { > > > > struct ovsdb_idl_index *sbrec_port_binding_by_name = > > > > engine_ovsdb_node_get_index( > > > > @@ -2085,60 +1920,17 @@ flow_output_sb_mac_binding_handler(struct > > > engine_node *node, void *data) > > > > engine_get_input_data("runtime_data", node); > > > > const struct hmap *local_datapaths = &rt_data->local_datapaths; > > > > > > > > - struct ed_type_flow_output *fo = data; > > > > - struct ovn_desired_flow_table *flow_table = &fo->flow_table; > > > > + struct ed_type_lflow_output *lfo = data; > > > > > > > > lflow_handle_changed_neighbors(sbrec_port_binding_by_name, > > > > - mac_binding_table, local_datapaths, flow_table); > > > > - > > > > - engine_set_node_state(node, EN_UPDATED); > > > > - return true; > > > > -} > > > > - > > > > -static bool > > > > -flow_output_sb_port_binding_handler(struct engine_node *node, > > > > - void *data) > > > > -{ > > > > - struct ed_type_runtime_data *rt_data = > > > > - engine_get_input_data("runtime_data", node); > > > > - > > > > - struct ed_type_flow_output *fo = data; > > > > - struct ovn_desired_flow_table *flow_table = &fo->flow_table; > > > > - > > > > - struct physical_ctx p_ctx; > > > > - init_physical_ctx(node, rt_data, &p_ctx); > > > > - > > > > - /* We handle port-binding changes for physical flow processing > > > > - * only. flow_output runtime data handler takes care of > processing > > > > - * logical flows for any port binding changes. > > > > - */ > > > > - physical_handle_port_binding_changes(&p_ctx, flow_table); > > > > - > > > > - engine_set_node_state(node, EN_UPDATED); > > > > - return true; > > > > -} > > > > - > > > > -static bool > > > > -flow_output_sb_multicast_group_handler(struct engine_node *node, void > > > *data) > > > > -{ > > > > - struct ed_type_runtime_data *rt_data = > > > > - engine_get_input_data("runtime_data", node); > > > > - > > > > - struct ed_type_flow_output *fo = data; > > > > - struct ovn_desired_flow_table *flow_table = &fo->flow_table; > > > > - > > > > - struct physical_ctx p_ctx; > > > > - init_physical_ctx(node, rt_data, &p_ctx); > > > > - > > > > - physical_handle_mc_group_changes(&p_ctx, flow_table); > > > > + mac_binding_table, local_datapaths, &lfo->flow_table); > > > > > > > > engine_set_node_state(node, EN_UPDATED); > > > > return true; > > > > - > > > > } > > > > > > > > static bool > > > > -_flow_output_resource_ref_handler(struct engine_node *node, void > *data, > > > > +_lflow_output_resource_ref_handler(struct engine_node *node, void > *data, > > > > enum ref_type ref_type) > > > > { > > > > struct ed_type_runtime_data *rt_data = > > > > @@ -2170,7 +1962,7 @@ _flow_output_resource_ref_handler(struct > > > engine_node *node, void *data, > > > > > > > > ovs_assert(br_int && chassis); > > > > > > > > - struct ed_type_flow_output *fo = data; > > > > + struct ed_type_lflow_output *fo = data; > > > > > > > > struct lflow_ctx_in l_ctx_in; > > > > struct lflow_ctx_out l_ctx_out; > > > > @@ -2239,53 +2031,20 @@ _flow_output_resource_ref_handler(struct > > > engine_node *node, void *data, > > > > } > > > > > > > > static bool > > > > -flow_output_addr_sets_handler(struct engine_node *node, void *data) > > > > +lflow_output_addr_sets_handler(struct engine_node *node, void *data) > > > > { > > > > - return _flow_output_resource_ref_handler(node, data, > > > REF_TYPE_ADDRSET); > > > > + return _lflow_output_resource_ref_handler(node, data, > > > REF_TYPE_ADDRSET); > > > > } > > > > > > > > static bool > > > > -flow_output_port_groups_handler(struct engine_node *node, void *data) > > > > +lflow_output_port_groups_handler(struct engine_node *node, void > *data) > > > > { > > > > - return _flow_output_resource_ref_handler(node, data, > > > REF_TYPE_PORTGROUP); > > > > + return _lflow_output_resource_ref_handler(node, data, > > > REF_TYPE_PORTGROUP); > > > > } > > > > > > > > static bool > > > > -flow_output_physical_flow_changes_handler(struct engine_node *node, > void > > > *data) > > > > -{ > > > > - struct ed_type_runtime_data *rt_data = > > > > - engine_get_input_data("runtime_data", node); > > > > - > > > > - struct ed_type_flow_output *fo = data; > > > > - struct physical_ctx p_ctx; > > > > - init_physical_ctx(node, rt_data, &p_ctx); > > > > - > > > > - engine_set_node_state(node, EN_UPDATED); > > > > - struct ed_type_pfc_data *pfc_data = > > > > - engine_get_input_data("physical_flow_changes", node); > > > > - > > > > - /* If there are OVS interface changes. Try to handle them > > > incrementally. */ > > > > - if (pfc_data->ovs_ifaces_changed) { > > > > - if (!physical_handle_ovs_iface_changes(&p_ctx, > &fo->flow_table)) > > > { > > > > - return false; > > > > - } > > > > - } > > > > - > > > > - if (pfc_data->recompute_physical_flows) { > > > > - /* This indicates that we need to recompute the physical > flows. > > > */ > > > > - physical_clear_unassoc_flows_with_db(&fo->flow_table); > > > > - physical_clear_dp_flows(&p_ctx, > &rt_data->ct_updated_datapaths, > > > > - &fo->flow_table); > > > > - physical_run(&p_ctx, &fo->flow_table); > > > > - return true; > > > > - } > > > > - > > > > - return true; > > > > -} > > > > - > > > > -static bool > > > > -flow_output_runtime_data_handler(struct engine_node *node, > > > > - void *data OVS_UNUSED) > > > > +lflow_output_runtime_data_handler(struct engine_node *node, > > > > + void *data OVS_UNUSED) > > > > { > > > > struct ed_type_runtime_data *rt_data = > > > > engine_get_input_data("runtime_data", node); > > > > @@ -2306,12 +2065,9 @@ flow_output_runtime_data_handler(struct > > > engine_node *node, > > > > > > > > struct lflow_ctx_in l_ctx_in; > > > > struct lflow_ctx_out l_ctx_out; > > > > - struct ed_type_flow_output *fo = data; > > > > + struct ed_type_lflow_output *fo = data; > > > > init_lflow_ctx(node, rt_data, fo, &l_ctx_in, &l_ctx_out); > > > > > > > > - struct physical_ctx p_ctx; > > > > - init_physical_ctx(node, rt_data, &p_ctx); > > > > - > > > > struct tracked_binding_datapath *tdp; > > > > HMAP_FOR_EACH (tdp, node, tracked_dp_bindings) { > > > > if (tdp->is_new) { > > > > @@ -2336,12 +2092,12 @@ flow_output_runtime_data_handler(struct > > > engine_node *node, > > > > } > > > > > > > > static bool > > > > -flow_output_sb_load_balancer_handler(struct engine_node *node, void > > > *data) > > > > +lflow_output_sb_load_balancer_handler(struct engine_node *node, void > > > *data) > > > > { > > > > struct ed_type_runtime_data *rt_data = > > > > engine_get_input_data("runtime_data", node); > > > > > > > > - struct ed_type_flow_output *fo = data; > > > > + struct ed_type_lflow_output *fo = data; > > > > struct lflow_ctx_in l_ctx_in; > > > > struct lflow_ctx_out l_ctx_out; > > > > init_lflow_ctx(node, rt_data, fo, &l_ctx_in, &l_ctx_out); > > > > @@ -2353,12 +2109,12 @@ flow_output_sb_load_balancer_handler(struct > > > engine_node *node, void *data) > > > > } > > > > > > > > static bool > > > > -flow_output_sb_fdb_handler(struct engine_node *node, void *data) > > > > +lflow_output_sb_fdb_handler(struct engine_node *node, void *data) > > > > { > > > > struct ed_type_runtime_data *rt_data = > > > > engine_get_input_data("runtime_data", node); > > > > > > > > - struct ed_type_flow_output *fo = data; > > > > + struct ed_type_lflow_output *fo = data; > > > > struct lflow_ctx_in l_ctx_in; > > > > struct lflow_ctx_out l_ctx_out; > > > > init_lflow_ctx(node, rt_data, fo, &l_ctx_in, &l_ctx_out); > > > > @@ -2369,6 +2125,235 @@ flow_output_sb_fdb_handler(struct engine_node > > > *node, void *data) > > > > return handled; > > > > } > > > > > > > > +struct ed_type_pflow_output { > > > > + /* Desired physical flows. */ > > > > + struct ovn_desired_flow_table flow_table; > > > > +}; > > > > + > > > > +static void init_physical_ctx(struct engine_node *node, > > > > + struct ed_type_runtime_data *rt_data, > > > > + struct physical_ctx *p_ctx) > > > > +{ > > > > + struct ovsdb_idl_index *sbrec_port_binding_by_name = > > > > + engine_ovsdb_node_get_index( > > > > + engine_get_input("SB_port_binding", node), > > > > + "name"); > > > > + > > > > + struct sbrec_multicast_group_table *multicast_group_table = > > > > + (struct sbrec_multicast_group_table *)EN_OVSDB_GET( > > > > + engine_get_input("SB_multicast_group", node)); > > > > + > > > > + struct sbrec_port_binding_table *port_binding_table = > > > > + (struct sbrec_port_binding_table *)EN_OVSDB_GET( > > > > + engine_get_input("SB_port_binding", node)); > > > > + > > > > + struct sbrec_chassis_table *chassis_table = > > > > + (struct sbrec_chassis_table *)EN_OVSDB_GET( > > > > + engine_get_input("SB_chassis", node)); > > > > + > > > > + struct ed_type_mff_ovn_geneve *ed_mff_ovn_geneve = > > > > + engine_get_input_data("mff_ovn_geneve", node); > > > > + > > > > + struct ovsrec_open_vswitch_table *ovs_table = > > > > + (struct ovsrec_open_vswitch_table *)EN_OVSDB_GET( > > > > + engine_get_input("OVS_open_vswitch", node)); > > > > + struct ovsrec_bridge_table *bridge_table = > > > > + (struct ovsrec_bridge_table *)EN_OVSDB_GET( > > > > + engine_get_input("OVS_bridge", node)); > > > > + const struct ovsrec_bridge *br_int = get_br_int(bridge_table, > > > ovs_table); > > > > + const char *chassis_id = get_ovs_chassis_id(ovs_table); > > > > + const struct sbrec_chassis *chassis = NULL; > > > > + struct ovsdb_idl_index *sbrec_chassis_by_name = > > > > + engine_ovsdb_node_get_index( > > > > + engine_get_input("SB_chassis", node), > > > > + "name"); > > > > + if (chassis_id) { > > > > + chassis = chassis_lookup_by_name(sbrec_chassis_by_name, > > > chassis_id); > > > > + } > > > > + > > > > + ovs_assert(br_int && chassis); > > > > + > > > > + struct ovsrec_interface_table *iface_table = > > > > + (struct ovsrec_interface_table *)EN_OVSDB_GET( > > > > + engine_get_input("OVS_interface", node)); > > > > + > > > > + struct ed_type_ct_zones *ct_zones_data = > > > > + engine_get_input_data("ct_zones", node); > > > > + struct simap *ct_zones = &ct_zones_data->current; > > > > + > > > > + p_ctx->sbrec_port_binding_by_name = sbrec_port_binding_by_name; > > > > + p_ctx->port_binding_table = port_binding_table; > > > > + p_ctx->mc_group_table = multicast_group_table; > > > > + p_ctx->br_int = br_int; > > > > + p_ctx->chassis_table = chassis_table; > > > > + p_ctx->iface_table = iface_table; > > > > + p_ctx->chassis = chassis; > > > > + p_ctx->active_tunnels = &rt_data->active_tunnels; > > > > + p_ctx->local_datapaths = &rt_data->local_datapaths; > > > > + p_ctx->local_lports = &rt_data->local_lports; > > > > + p_ctx->ct_zones = ct_zones; > > > > + p_ctx->mff_ovn_geneve = ed_mff_ovn_geneve->mff_ovn_geneve; > > > > + p_ctx->local_bindings = &rt_data->lbinding_data.bindings; > > > > + p_ctx->ct_updated_datapaths = &rt_data->ct_updated_datapaths; > > > > +} > > > > + > > > > +static void * > > > > +en_pflow_output_init(struct engine_node *node OVS_UNUSED, > > > > + struct engine_arg *arg OVS_UNUSED) > > > > +{ > > > > + struct ed_type_pflow_output *data = xzalloc(sizeof *data); > > > > + ovn_desired_flow_table_init(&data->flow_table); > > > > + return data; > > > > +} > > > > + > > > > +static void > > > > +en_pflow_output_cleanup(void *data OVS_UNUSED) > > > > +{ > > > > + struct ed_type_pflow_output *pfo = data; > > > > + ovn_desired_flow_table_destroy(&pfo->flow_table); > > > > +} > > > > + > > > > +/* Indicate to the flow_output engine that we need to recompute > physical > > > > + * flows. */ > > > > > > This comment is irrelevant, can be removed. > > > > Ack. > > > > > > > > > +static void > > > > +en_pflow_output_run(struct engine_node *node, void *data) > > > > +{ > > > > + struct ed_type_pflow_output *pfo = data; > > > > + struct ovn_desired_flow_table *pflow_table = &pfo->flow_table; > > > > + static bool first_run = true; > > > > + if (first_run) { > > > > + first_run = false; > > > > + } else { > > > > + ovn_desired_flow_table_clear(pflow_table); > > > > + } > > > > + > > > > + struct ed_type_runtime_data *rt_data = > > > > + engine_get_input_data("runtime_data", node); > > > > + > > > > + struct physical_ctx p_ctx; > > > > + init_physical_ctx(node, rt_data, &p_ctx); > > > > + physical_run(&p_ctx, pflow_table); > > > > + > > > > + engine_set_node_state(node, EN_UPDATED); > > > > +} > > > > + > > > > +static bool > > > > +pflow_output_sb_port_binding_handler(struct engine_node *node, > > > > + void *data) > > > > +{ > > > > + struct ed_type_runtime_data *rt_data = > > > > + engine_get_input_data("runtime_data", node); > > > > + > > > > + struct ed_type_pflow_output *pfo = data; > > > > + > > > > + struct physical_ctx p_ctx; > > > > + init_physical_ctx(node, rt_data, &p_ctx); > > > > + > > > > + /* We handle port-binding changes for physical flow processing > > > > + * only. flow_output runtime data handler takes care of > processing > > > > + * logical flows for any port binding changes. > > > > + */ > > > > + physical_handle_port_binding_changes(&p_ctx, &pfo->flow_table); > > > > + > > > > + engine_set_node_state(node, EN_UPDATED); > > > > + return true; > > > > +} > > > > + > > > > +static bool > > > > +pflow_output_sb_multicast_group_handler(struct engine_node *node, > void > > > *data) > > > > +{ > > > > + struct ed_type_runtime_data *rt_data = > > > > + engine_get_input_data("runtime_data", node); > > > > + > > > > + struct ed_type_pflow_output *pfo = data; > > > > + > > > > + struct physical_ctx p_ctx; > > > > + init_physical_ctx(node, rt_data, &p_ctx); > > > > + > > > > + physical_handle_mc_group_changes(&p_ctx, &pfo->flow_table); > > > > + > > > > + engine_set_node_state(node, EN_UPDATED); > > > > + return true; > > > > +} > > > > + > > > > +/* There are OVS interface changes. Indicate to the flow_output > engine > > > > + * to handle these OVS interface changes for physical flow > computations. > > > */ > > > > > > Same here, the comment may be removed. > > > > Ack. > > > > > > > > > +static bool > > > > +pflow_output_ovs_iface_handler(struct engine_node *node OVS_UNUSED, > > > > + void *data OVS_UNUSED) > > > > +{ > > > > + struct ed_type_runtime_data *rt_data = > > > > + engine_get_input_data("runtime_data", node); > > > > + > > > > + struct ed_type_pflow_output *pfo = data; > > > > + > > > > + struct physical_ctx p_ctx; > > > > + init_physical_ctx(node, rt_data, &p_ctx); > > > > + > > > > + engine_set_node_state(node, EN_UPDATED); > > > > + return physical_handle_ovs_iface_changes(&p_ctx, > &pfo->flow_table); > > > > +} > > > > + > > > > +/* Handles sbrec_chassis changes. > > > > + * If a new chassis is added or removed return false, so that > > > > + * physical flows are programmed. > > > > + * For any updates, there is no need for any flow computation. > > > > + * Encap changes will also result in sbrec_chassis changes, > > > > + * but we handle encap changes separately. > > > > + */ > > > > +static bool > > > > +pflow_output_sb_chassis_handler(struct engine_node *node, > > > > + void *data OVS_UNUSED) > > > > +{ > > > > + struct sbrec_chassis_table *chassis_table = > > > > + (struct sbrec_chassis_table *)EN_OVSDB_GET( > > > > + engine_get_input("SB_chassis", node)); > > > > + > > > > + const struct sbrec_chassis *ch; > > > > + SBREC_CHASSIS_TABLE_FOR_EACH_TRACKED (ch, chassis_table) { > > > > + if (sbrec_chassis_is_deleted(ch) || > sbrec_chassis_is_new(ch)) { > > > > + return false; > > > > + } > > > > + } > > > > + > > > > + return true; > > > > +} > > > > + > > > > +static void * > > > > +en_flow_output_init(struct engine_node *node OVS_UNUSED, > > > > + struct engine_arg *arg OVS_UNUSED) > > > > +{ > > > > + return NULL; > > > > +} > > > > + > > > > +static void > > > > +en_flow_output_cleanup(void *data OVS_UNUSED) > > > > +{ > > > > + > > > > +} > > > > + > > > > +static void > > > > +en_flow_output_run(struct engine_node *node OVS_UNUSED, void *data > > > OVS_UNUSED) > > > > +{ > > > > + engine_set_node_state(node, EN_UPDATED); > > > > +} > > > > + > > > > +static bool > > > > +flow_output_pflow_output_handler(struct engine_node *node, > > > > + void *data OVS_UNUSED) > > > > +{ > > > > + engine_set_node_state(node, EN_UPDATED); > > > > + return true; > > > > +} > > > > + > > > > +static bool > > > > +flow_output_lflow_output_handler(struct engine_node *node, > > > > + void *data OVS_UNUSED) > > > > +{ > > > > + engine_set_node_state(node, EN_UPDATED); > > > > + return true; > > > > +} > > > > + > > > > struct ovn_controller_exit_args { > > > > bool *exiting; > > > > bool *restart; > > > > @@ -2562,8 +2547,8 @@ main(int argc, char *argv[]) > > > > ENGINE_NODE_WITH_CLEAR_TRACK_DATA(runtime_data, "runtime_data"); > > > > ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve"); > > > > ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected"); > > > > - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(physical_flow_changes, > > > > - "physical_flow_changes"); > > > > + ENGINE_NODE(pflow_output, "physical_flow_output"); > > > > + ENGINE_NODE(lflow_output, "logical_flow_output"); > > > > ENGINE_NODE(flow_output, "flow_output"); > > > > ENGINE_NODE(addr_sets, "addr_sets"); > > > > ENGINE_NODE(port_groups, "port_groups"); > > > > @@ -2583,58 +2568,65 @@ main(int argc, char *argv[]) > > > > engine_add_input(&en_port_groups, &en_sb_port_group, > > > > port_groups_sb_port_group_handler); > > > > > > > > - /* Engine node physical_flow_changes indicates whether > > > > - * we can recompute only physical flows or we can > > > > - * incrementally process the physical flows. > > > > - * > > > > - * Note: The order of inputs is important, all OVS interface > changes > > > must > > > > + /* Note: The order of inputs is important, all OVS interface > changes > > > must > > > > * be handled before any ct_zone changes. > > > > */ > > > > - engine_add_input(&en_physical_flow_changes, &en_ovs_interface, > > > > - physical_flow_changes_ovs_iface_handler); > > > > - engine_add_input(&en_physical_flow_changes, &en_ct_zones, > > > > - physical_flow_changes_ct_zones_handler); > > > > - > > > > - engine_add_input(&en_flow_output, &en_addr_sets, > > > > - flow_output_addr_sets_handler); > > > > - engine_add_input(&en_flow_output, &en_port_groups, > > > > - flow_output_port_groups_handler); > > > > - engine_add_input(&en_flow_output, &en_runtime_data, > > > > - flow_output_runtime_data_handler); > > > > - engine_add_input(&en_flow_output, &en_mff_ovn_geneve, NULL); > > > > - engine_add_input(&en_flow_output, &en_physical_flow_changes, > > > > - flow_output_physical_flow_changes_handler); > > > > + engine_add_input(&en_pflow_output, &en_ovs_interface, > > > > + pflow_output_ovs_iface_handler); > > > > + engine_add_input(&en_pflow_output, &en_ct_zones, > > > > + NULL); > > > > + engine_add_input(&en_pflow_output, &en_sb_chassis, > > > > + pflow_output_sb_chassis_handler); > > > > + engine_add_input(&en_pflow_output, &en_sb_port_binding, > > > > + pflow_output_sb_port_binding_handler); > > > > + engine_add_input(&en_pflow_output, &en_sb_multicast_group, > > > > + pflow_output_sb_multicast_group_handler); > > > > + engine_add_input(&en_pflow_output, &en_runtime_data, > > > > + engine_noop_handler); > > > > > > pflow_output depends on runtime_data as input. So if the input changes, > the > > > physical flows should change accordingly. Why would we use "noop" > handler > > > here? Using noop handler means we just ignore the change which seems > > > incorrect. If the change is handled in other paths we should document it > > > carefully, for each of the field of runtime_data that are used by > > > pflow_output, e.g. local_datapaths, local_lports, active_tunnels, etc. > > > (Sorry that I didn't spot this in last review.) > > > > With the present master, the changes to runtime_data_handler is handled in > > flow_output_runtime_data_handler() and it doesn't do any flow > modifications > > to the physical flows (i.e it doesn't call any functions of physical.c). > > > > And hence we don't need to do any thing for the newly added > > 'en_pflow_output' engine. > > We still need to access the runtime_data_handler() data in physical.c. > > And hence > > a noop handler is added. I will add a comment for this. > > > > Hi Numan, > > Thanks for explain and adding comments in v7. However, I am still not > convinced of the use of noop handler here. In init_physical_ctx() it does > pass in a lot of runtime_data members to the pflow_output node. > > p_ctx->active_tunnels = &rt_data->active_tunnels; > p_ctx->local_datapaths = &rt_data->local_datapaths; > p_ctx->local_lports = &rt_data->local_lports; > p_ctx->local_bindings = &rt_data->lbinding_data.bindings; > p_ctx->ct_updated_datapaths = &rt_data->ct_updated_datapaths; > > All of them are required for computing physical flows, so it is hard to > believe that the physical flows computed wouldn't need any change when any > of these data is changed. > If we believe these changes would somehow trigger recompute and so a change > handler is not needed here, then we should use NULL instead of a noop > handler. > Using a noop handler is telling that we know we need to handle the change, > but we are also sure the change is handled properly through another path of > the I-P engine incrementally, so we don't need to handle the change here > and we don't want to use NULL because a recompute is unnecessary. If this > is the case, let's document for each of these changed fields so that it is > clear where and how these input changes are handled. Otherwise, it is > really hard to reason the correctness and making changes in the future. > I understand this is hard work. Really appreciate your effort on this! >
Hi Han, Sorry for a late reply. I totally understand your concern. In my opinion having a noop handler for runtime data changes for the pflow engine should be fine. But I do understand your concerns. I think we can handle runtime data changes for pflow engine node. I think we can also split physical_run to separate out the localvif_to_ofport simap and make it part of engine data. I'm planning to address that as follow up patches if you're fine. Until then are you fine if we don't add any handler for runtime data changes for v7 patch I posted. Having a NULL handler should be fine and the computation should be similar to what we do now. Right now with the present master, there is no runtime data handler for ct_zones engine and for a ct zone engine change, we do full recompute of physical flows. Does my proposal sounds good ? If so, can you please take a look at v7 considering runtime data handler as NULL for pflow engine and provider your comments ? I'm fine posting v8 instead if you prefer. Thanks Numan > Thanks, > Han > > > > > > > + engine_add_input(&en_pflow_output, &en_sb_encap, NULL); > > > > + engine_add_input(&en_pflow_output, &en_mff_ovn_geneve, NULL); > > > > + engine_add_input(&en_pflow_output, &en_ovs_open_vswitch, NULL); > > > > + engine_add_input(&en_pflow_output, &en_ovs_bridge, NULL); > > > > + > > > > + engine_add_input(&en_lflow_output, &en_addr_sets, > > > > + lflow_output_addr_sets_handler); > > > > + engine_add_input(&en_lflow_output, &en_port_groups, > > > > + lflow_output_port_groups_handler); > > > > + engine_add_input(&en_lflow_output, &en_runtime_data, > > > > + lflow_output_runtime_data_handler); > > > > > > > > /* We need this input nodes for only data. Hence the noop > handler. */ > > > > - engine_add_input(&en_flow_output, &en_ct_zones, > engine_noop_handler); > > > > - engine_add_input(&en_flow_output, &en_ovs_interface, > > > engine_noop_handler); > > > > - > > > > - engine_add_input(&en_flow_output, &en_ovs_open_vswitch, NULL); > > > > - engine_add_input(&en_flow_output, &en_ovs_bridge, NULL); > > > > - > > > > - engine_add_input(&en_flow_output, &en_sb_chassis, NULL); > > > > - engine_add_input(&en_flow_output, &en_sb_encap, NULL); > > > > - engine_add_input(&en_flow_output, &en_sb_multicast_group, > > > > - flow_output_sb_multicast_group_handler); > > > > - engine_add_input(&en_flow_output, &en_sb_port_binding, > > > > - flow_output_sb_port_binding_handler); > > > > - engine_add_input(&en_flow_output, &en_sb_mac_binding, > > > > - flow_output_sb_mac_binding_handler); > > > > - engine_add_input(&en_flow_output, &en_sb_logical_flow, > > > > - flow_output_sb_logical_flow_handler); > > > > + engine_add_input(&en_lflow_output, &en_ct_zones, > > > > + engine_noop_handler); > > > > + engine_add_input(&en_lflow_output, &en_ovs_interface, > > > > + engine_noop_handler); > > > > + engine_add_input(&en_lflow_output, &en_sb_chassis, > > > > + engine_noop_handler); > > > > + engine_add_input(&en_lflow_output, &en_sb_multicast_group, > > > > + engine_noop_handler); > > > > + engine_add_input(&en_lflow_output, &en_sb_port_binding, > > > > + engine_noop_handler); > > > > > > Similar as above, usage of "noop" handlers should be carefully > documented. > > > How do we guanrantee these input changes are properly handled? > > > > Same goes for your this comment. With the present master, > > the changes to port_binding is handled in > flow_output_port_binding_handler() > > and it doesn't do any flow modifications to the logical flows. It doesn't > call > > any functions of lflow.c and hence there is a no-op in the newly added > > "en_lflow_output" > > engine. We still need to have en_sb_port_binding as input for > > indexing (please see init_lflow_ctx()). > > > > I can add a comment for it. > > > > Thanks > > Numan > > > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
