[ovs-dev] [PATCH ovn v4] ovn-controller: Split logical flow and phsyical flow processing.

Numan Siddique numans at ovn.org
Thu Mar 11 19:08:02 UTC 2021


On Thu, Mar 11, 2021 at 1:32 PM Han Zhou <hzhou at ovn.org> wrote:
>
> Hi Numan,
>
> Thanks a lot for this refactoring! Overall I think it makes the engine node
> dependency much more clear. Please see my comments below.
>
> Regards,
> Han
>
> On Mon, Mar 8, 2021 at 10:10 PM <numans at ovn.org> wrote:
> >
> > From: Numan Siddique <numans at ovn.org>
> >
> > 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.  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.
> >
> Maybe we can call it out clearly that a specific motivation of this patch
> is to remove the engine node: physical_flow_changes, so that the engine
> nodes reflect the data dependency more clearly. (As a result, the lflows
> and physical flows are splitted.)

Thanks for the review Han. Ack. sounds good to me. I will update in v5.

>
> > CC: Han Zhou <hzhou at ovn.org>
> > Signed-off-by: Numan Siddique <numans at ovn.org>
> > ---
> >
> > 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/lflow.c          |  18 +-
> >  controller/lflow.h          |   6 +-
> >  controller/ofctrl.c         | 214 +++++++----
> >  controller/ofctrl.h         |  37 +-
> >  controller/ovn-controller.c | 699 ++++++++++++++++++------------------
> >  controller/physical.c       |  28 +-
> >  controller/physical.h       |  12 +-
> >  8 files changed, 548 insertions(+), 472 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/lflow.c b/controller/lflow.c
> > index a3d84aff4e..efb39acfe2 100644
> > --- a/controller/lflow.c
> > +++ b/controller/lflow.c
> > @@ -967,7 +967,7 @@ static void
> >  consider_neighbor_flow(struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
> >                         const struct hmap *local_datapaths,
> >                         const struct sbrec_mac_binding *b,
> > -                       struct ovn_desired_flow_table *flow_table)
> > +                       struct ovn_flow_table *flow_table)
> >  {
> >      const struct sbrec_port_binding *pb
> >          = lport_lookup_by_name(sbrec_port_binding_by_name,
> b->logical_port);
> > @@ -1047,7 +1047,7 @@ static void
> >  add_neighbor_flows(struct ovsdb_idl_index *sbrec_port_binding_by_name,
> >                     const struct sbrec_mac_binding_table
> *mac_binding_table,
> >                     const struct hmap *local_datapaths,
> > -                   struct ovn_desired_flow_table *flow_table)
> > +                   struct ovn_flow_table *flow_table)
> >  {
> >      const struct sbrec_mac_binding *b;
> >      SBREC_MAC_BINDING_TABLE_FOR_EACH (b, mac_binding_table) {
> > @@ -1239,7 +1239,7 @@ add_lb_vip_hairpin_flows(struct ovn_controller_lb
> *lb,
> >                           struct ovn_lb_vip *lb_vip,
> >                           struct ovn_lb_backend *lb_backend,
> >                           uint8_t lb_proto,
> > -                         struct ovn_desired_flow_table *flow_table)
> > +                         struct ovn_flow_table *flow_table)
> >  {
> >      uint64_t stub[1024 / 8];
> >      struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(stub);
> > @@ -1351,7 +1351,7 @@ static void
> >  add_lb_ct_snat_vip_flows(struct ovn_controller_lb *lb,
> >                           struct ovn_lb_vip *lb_vip,
> >                           uint8_t lb_proto,
> > -                         struct ovn_desired_flow_table *flow_table)
> > +                         struct ovn_flow_table *flow_table)
> >  {
> >      uint64_t stub[1024 / 8];
> >      struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(stub);
> > @@ -1449,7 +1449,7 @@ add_lb_ct_snat_vip_flows(struct ovn_controller_lb
> *lb,
> >  static void
> >  consider_lb_hairpin_flows(const struct sbrec_load_balancer *sbrec_lb,
> >                            const struct hmap *local_datapaths,
> > -                          struct ovn_desired_flow_table *flow_table)
> > +                          struct ovn_flow_table *flow_table)
> >  {
> >      /* Check if we need to add flows or not.  If there is one datapath
> >       * in the local_datapaths, it means all the datapaths of the lb
> > @@ -1497,7 +1497,7 @@ consider_lb_hairpin_flows(const struct
> sbrec_load_balancer *sbrec_lb,
> >  static void
> >  add_lb_hairpin_flows(const struct sbrec_load_balancer_table *lb_table,
> >                       const struct hmap *local_datapaths,
> > -                     struct ovn_desired_flow_table *flow_table)
> > +                     struct ovn_flow_table *flow_table)
> >  {
> >      const struct sbrec_load_balancer *lb;
> >      SBREC_LOAD_BALANCER_TABLE_FOR_EACH (lb, lb_table) {
> > @@ -1511,7 +1511,7 @@ lflow_handle_changed_neighbors(
> >      struct ovsdb_idl_index *sbrec_port_binding_by_name,
> >      const struct sbrec_mac_binding_table *mac_binding_table,
> >      const struct hmap *local_datapaths,
> > -    struct ovn_desired_flow_table *flow_table)
> > +    struct ovn_flow_table *flow_table)
> >  {
> >      const struct sbrec_mac_binding *mb;
> >      /* Handle deleted mac_bindings first, to avoid *duplicated flow*
> problem
> > @@ -1542,7 +1542,7 @@ lflow_handle_changed_neighbors(
> >  static void
> >  consider_fdb_flows(const struct sbrec_fdb *fdb,
> >                     const struct hmap *local_datapaths,
> > -                   struct ovn_desired_flow_table *flow_table)
> > +                   struct ovn_flow_table *flow_table)
> >  {
> >      if (!get_local_datapath(local_datapaths, fdb->dp_key)) {
> >          return;
> > @@ -1586,7 +1586,7 @@ consider_fdb_flows(const struct sbrec_fdb *fdb,
> >  static void
> >  add_fdb_flows(const struct sbrec_fdb_table *fdb_table,
> >                const struct hmap *local_datapaths,
> > -              struct ovn_desired_flow_table *flow_table)
> > +              struct ovn_flow_table *flow_table)
> >  {
> >      const struct sbrec_fdb *fdb;
> >      SBREC_FDB_TABLE_FOR_EACH (fdb, fdb_table) {
> > diff --git a/controller/lflow.h b/controller/lflow.h
> > index 3c929d8a6a..c4ba044516 100644
> > --- a/controller/lflow.h
> > +++ b/controller/lflow.h
> > @@ -41,7 +41,7 @@
> >
> >  struct ovn_extend_table;
> >  struct ovsdb_idl_index;
> > -struct ovn_desired_flow_table;
> > +struct ovn_flow_table;
> >  struct hmap;
> >  struct hmap_node;
> >  struct sbrec_chassis;
> > @@ -147,7 +147,7 @@ struct lflow_ctx_in {
> >  };
> >
> >  struct lflow_ctx_out {
> > -    struct ovn_desired_flow_table *flow_table;
> > +    struct ovn_flow_table *flow_table;
> >      struct ovn_extend_table *group_table;
> >      struct ovn_extend_table *meter_table;
> >      struct lflow_resource_ref *lfrr;
> > @@ -168,7 +168,7 @@ void lflow_handle_changed_neighbors(
> >      struct ovsdb_idl_index *sbrec_port_binding_by_name,
> >      const struct sbrec_mac_binding_table *,
> >      const struct hmap *local_datapaths,
> > -    struct ovn_desired_flow_table *);
> > +    struct ovn_flow_table *);
> >  bool lflow_handle_changed_lbs(struct lflow_ctx_in *, struct
> lflow_ctx_out *);
> >  bool lflow_handle_changed_fdbs(struct lflow_ctx_in *, struct
> lflow_ctx_out *);
> >  void lflow_destroy(void);
> > diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> > index 415d9b7e16..03120c2865 100644
> > --- a/controller/ofctrl.c
> > +++ b/controller/ofctrl.c
> > @@ -51,6 +51,25 @@
> >
> >  VLOG_DEFINE_THIS_MODULE(ofctrl);
> >
> > +struct ovn_desired_flow_table {
> > +    /* Hash map flow table using flow match conditions as hash key.*/
> > +    struct hmap match_flow_table;
> > +
> > +    /* SB uuid index for the cross reference nodes that link to the
> nodes in
> > +     * match_flow_table.*/
> > +    struct hmap uuid_flow_table;
> > +
> > +    /* Is flow changes tracked. */
> > +    bool change_tracked;
> > +    /* Tracked flow changes. */
> > +    struct ovs_list tracked_flows;
> > +};
> > +
> > +struct ovn_flow_table {
> > +    struct ovn_desired_flow_table desired_ftable;
> > +    struct hmap installed_flows;
>
> It seems to me "installed_flows" shouldn't belong to the engine data. Would
> it be better to keep installed_flows static in ofctrl module?

I thought about this.  If we keep it as static, then we need to have
hmap installed_pflows  and hmap installed_lflows.

How would we figure out which hmap to update when lflow.c calls
ofctrl_add_flow() ?

Or do you mean just have a single hmap - installed_flows ?


>
> > +};
> > +
> >  /* An OpenFlow flow. */
> >  struct ovn_flow {
> >      /* Key. */
> > @@ -172,7 +191,7 @@ struct sb_flow_ref {
> >      struct uuid sb_uuid;
> >  };
> >
> > -/* A installed flow, in static variable installed_flows.
> > +/* An installed flow, in installed_flows hmap of 'ovn_flow_table'.
> >   *
> >   * Installed flows are updated in ofctrl_put for maintaining the flow
> >   * installation to OVS. They are updated according to desired flows:
> either by
> > @@ -211,6 +230,10 @@ struct installed_flow {
> >      struct ovs_list desired_refs;
> >  };
> >
> > +static void ovn_desired_flow_table_init(struct ovn_desired_flow_table *);
> > +static void ovn_desired_flow_table_clear(struct ovn_desired_flow_table
> *);
> > +static void ovn_desired_flow_table_destroy(struct ovn_desired_flow_table
> *);
> > +
> >  typedef bool
> >  (*desired_flow_match_cb)(const struct desired_flow *candidate,
> >                           const void *arg);
> > @@ -233,7 +256,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 +324,15 @@ 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;
> > +/* Reference to flow table of "struct ovn_flow"s, that holds the flow
> table
> > + * currently installed in the switch.
> > + *
> > + * 'installed_lflows' - Represents flows for logical flows processed by
> > + *                      lflow.c.
> > + * 'installed_pflows' - Represents physical flows generated by
> physical.c.
> > + */
> > +static struct hmap *installed_lflows;
> > +static struct hmap *installed_pflows;
> >
> >  /* A reference to the group_table. */
> >  static struct ovn_extend_table *groups;
> > @@ -328,25 +357,51 @@ static struct ofpbuf *encode_group_mod(const struct
> ofputil_group_mod *);
> >
> >  static struct ofpbuf *encode_meter_mod(const struct ofputil_meter_mod *);
> >
> > -static void ovn_installed_flow_table_clear(void);
> > -static void ovn_installed_flow_table_destroy(void);
> > -
> > +static void ovn_installed_flow_table_clear(struct hmap *installed_flows);
> >
> >  static void ofctrl_recv(const struct ofp_header *, enum ofptype);
> >
> > +/* ovn_flow_table operations. */
> > +struct ovn_flow_table *
> > +ovn_flow_table_alloc(void)
> > +{
> > +    struct ovn_flow_table *flow_table = xzalloc(sizeof *flow_table);
> > +    ovn_desired_flow_table_init(&flow_table->desired_ftable);
> > +    hmap_init(&flow_table->installed_flows);
> > +    return flow_table;
> > +}
> > +
> > +void
> > +ovn_flow_table_clear(struct ovn_flow_table *flow_table)
> > +{
> > +    ovn_desired_flow_table_clear(&flow_table->desired_ftable);
> > +}
> > +
> > +void
> > +ovn_flow_table_destroy(struct ovn_flow_table *flow_table)
> > +{
> > +    ovn_desired_flow_table_destroy(&flow_table->desired_ftable);
> > +    ovn_installed_flow_table_clear(&flow_table->installed_flows);
> > +    hmap_destroy(&flow_table->installed_flows);
> > +    free(flow_table);
> > +}
> > +
> >  void
> >  ofctrl_init(struct ovn_extend_table *group_table,
> >              struct ovn_extend_table *meter_table,
> > +            struct ovn_flow_table *lflow_table,
> > +            struct ovn_flow_table *pflow_table,
> >              int inactivity_probe_interval)
> >  {
> >      swconn = rconn_create(inactivity_probe_interval, 0,
> >                            DSCP_DEFAULT, 1 << OFP15_VERSION);
> >      tx_counter = rconn_packet_counter_create();
> > -    hmap_init(&installed_flows);
> >      ovs_list_init(&flow_updates);
> >      ovn_init_symtab(&symtab);
> >      groups = group_table;
> >      meters = meter_table;
> > +    installed_lflows = &lflow_table->installed_flows;
> > +    installed_pflows = &pflow_table->installed_flows;
> >  }
> >
> >  /* S_NEW, for a new connection.
> > @@ -573,7 +628,8 @@ run_S_CLEAR_FLOWS(void)
> >      ofputil_uninit_group_mod(&gm);
> >
> >      /* Clear installed_flows, to match the state of the switch. */
> > -    ovn_installed_flow_table_clear();
> > +    ovn_installed_flow_table_clear(installed_lflows);
> > +    ovn_installed_flow_table_clear(installed_pflows);
> >
> >      /* Clear existing groups, to match the state of the switch. */
> >      if (groups) {
> > @@ -758,7 +814,6 @@ void
> >  ofctrl_destroy(void)
> >  {
> >      rconn_destroy(swconn);
> > -    ovn_installed_flow_table_destroy();
> >      rconn_packet_counter_destroy(tx_counter);
> >      expr_symtab_destroy(&symtab);
> >      shash_destroy(&symtab);
> > @@ -1019,17 +1074,18 @@ link_flow_to_sb(struct ovn_desired_flow_table
> *flow_table,
> >   *
> >   * The caller should initialize its own hmap to hold the flows. */
> >  void
> > -ofctrl_check_and_add_flow(struct ovn_desired_flow_table *flow_table,
> > +ofctrl_check_and_add_flow(struct ovn_flow_table *flow_table,
> >                            uint8_t table_id, uint16_t priority,
> >                            uint64_t cookie, const struct match *match,
> >                            const struct ofpbuf *actions,
> >                            const struct uuid *sb_uuid,
> >                            bool log_duplicate_flow)
> >  {
> > +    struct ovn_desired_flow_table *d_ftable =
> &flow_table->desired_ftable;
> >      struct desired_flow *f = desired_flow_alloc(table_id, priority,
> cookie,
> >                                                  match, actions);
> >
> > -    if (desired_flow_lookup_check_uuid(flow_table, &f->flow, sb_uuid)) {
> > +    if (desired_flow_lookup_check_uuid(d_ftable, &f->flow, sb_uuid)) {
> >          if (log_duplicate_flow) {
> >              static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
> 5);
> >              if (!VLOG_DROP_DBG(&rl)) {
> > @@ -1042,20 +1098,20 @@ ofctrl_check_and_add_flow(struct
> ovn_desired_flow_table *flow_table,
> >          return;
> >      }
> >
> > -    hmap_insert(&flow_table->match_flow_table, &f->match_hmap_node,
> > +    hmap_insert(&d_ftable->match_flow_table, &f->match_hmap_node,
> >                  f->flow.hash);
> > -    link_flow_to_sb(flow_table, f, sb_uuid);
> > -    track_flow_add_or_modify(flow_table, f);
> > +    link_flow_to_sb(d_ftable, f, sb_uuid);
> > +    track_flow_add_or_modify(d_ftable, f);
> >      ovn_flow_log(&f->flow, "ofctrl_add_flow");
> >  }
> >
> >  void
> > -ofctrl_add_flow(struct ovn_desired_flow_table *desired_flows,
> > +ofctrl_add_flow(struct ovn_flow_table *flow_table,
> >                  uint8_t table_id, uint16_t priority, uint64_t cookie,
> >                  const struct match *match, const struct ofpbuf *actions,
> >                  const struct uuid *sb_uuid)
> >  {
> > -    ofctrl_check_and_add_flow(desired_flows, table_id, priority, cookie,
> > +    ofctrl_check_and_add_flow(flow_table, table_id, priority, cookie,
> >                                match, actions, sb_uuid, true);
> >  }
> >
> > @@ -1063,7 +1119,7 @@ ofctrl_add_flow(struct ovn_desired_flow_table
> *desired_flows,
> >   * flow existed, a new link will also be created between the new sb_uuid
> >   * and the existing flow. */
> >  void
> > -ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows,
> > +ofctrl_add_or_append_flow(struct ovn_flow_table *flow_table,
> >                            uint8_t table_id, uint16_t priority, uint64_t
> cookie,
> >                            const struct match *match,
> >                            const struct ofpbuf *actions,
> > @@ -1072,6 +1128,7 @@ ofctrl_add_or_append_flow(struct
> ovn_desired_flow_table *desired_flows,
> >      struct desired_flow *existing;
> >      struct desired_flow *f;
> >
> > +    struct ovn_desired_flow_table *desired_flows =
> &flow_table->desired_ftable;
> >      f = desired_flow_alloc(table_id, priority, cookie, match, actions);
> >      existing = desired_flow_lookup_conjunctive(desired_flows, &f->flow);
> >      if (existing) {
> > @@ -1136,13 +1193,14 @@ remove_flows_from_sb_to_flow(struct
> ovn_desired_flow_table *flow_table,
> >  }
> >
> >  void
> > -ofctrl_remove_flows(struct ovn_desired_flow_table *flow_table,
> > +ofctrl_remove_flows(struct ovn_flow_table *flow_table,
> >                      const struct uuid *sb_uuid)
> >  {
> > -    struct sb_to_flow *stf =
> sb_to_flow_find(&flow_table->uuid_flow_table,
> > +    struct ovn_desired_flow_table *d_ftable =
> &flow_table->desired_ftable;
> > +    struct sb_to_flow *stf = sb_to_flow_find(&d_ftable->uuid_flow_table,
> >                                               sb_uuid);
> >      if (stf) {
> > -        remove_flows_from_sb_to_flow(flow_table, stf,
> "ofctrl_remove_flow");
> > +        remove_flows_from_sb_to_flow(d_ftable, stf,
> "ofctrl_remove_flow");
> >      }
> >
> >      /* remove any related group and meter info */
> > @@ -1243,9 +1301,10 @@ flood_remove_flows_for_sb_uuid(struct
> ovn_desired_flow_table *flow_table,
> >  }
> >
> >  void
> > -ofctrl_flood_remove_flows(struct ovn_desired_flow_table *flow_table,
> > +ofctrl_flood_remove_flows(struct ovn_flow_table *flow_table,
> >                            struct hmap *flood_remove_nodes)
> >  {
> > +    struct ovn_desired_flow_table *d_ftable =
> &flow_table->desired_ftable;
> >      struct ofctrl_flood_remove_node *ofrn;
> >      int i, n = 0;
> >
> > @@ -1260,7 +1319,7 @@ ofctrl_flood_remove_flows(struct
> ovn_desired_flow_table *flow_table,
> >      }
> >
> >      for (i = 0; i < n; i++) {
> > -        flood_remove_flows_for_sb_uuid(flow_table, &sb_uuids[i],
> > +        flood_remove_flows_for_sb_uuid(d_ftable, &sb_uuids[i],
> >                                         flood_remove_nodes);
> >      }
> >      free(sb_uuids);
> > @@ -1425,11 +1484,12 @@ 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
> > @@ -1494,23 +1554,23 @@ installed_flow_destroy(struct installed_flow *f)
> >  }
> >
> >  /* Desired flow table operations. */
> > -void
> > -ovn_desired_flow_table_init(struct ovn_desired_flow_table *flow_table)
> > +static void
> > +ovn_desired_flow_table_init(struct ovn_desired_flow_table
> *desired_ftable)
> >  {
> > -    hmap_init(&flow_table->match_flow_table);
> > -    hmap_init(&flow_table->uuid_flow_table);
> > -    ovs_list_init(&flow_table->tracked_flows);
> > -    flow_table->change_tracked = false;
> > +    hmap_init(&desired_ftable->match_flow_table);
> > +    hmap_init(&desired_ftable->uuid_flow_table);
> > +    ovs_list_init(&desired_ftable->tracked_flows);
> > +    desired_ftable->change_tracked = false;
> >  }
> >
> > -void
> > -ovn_desired_flow_table_clear(struct ovn_desired_flow_table *flow_table)
> > +static void
> > +ovn_desired_flow_table_clear(struct ovn_desired_flow_table
> *desired_ftable)
> >  {
> > -    flow_table->change_tracked = false;
> > +    desired_ftable->change_tracked = false;
> >
> >      struct desired_flow *f, *f_next;
> >      LIST_FOR_EACH_SAFE (f, f_next, track_list_node,
> > -                        &flow_table->tracked_flows) {
> > +                        &desired_ftable->tracked_flows) {
> >          ovs_list_remove(&f->track_list_node);
> >          if (f->is_deleted) {
> >              if (f->installed_flow) {
> > @@ -1522,38 +1582,32 @@ ovn_desired_flow_table_clear(struct
> ovn_desired_flow_table *flow_table)
> >
> >      struct sb_to_flow *stf, *next;
> >      HMAP_FOR_EACH_SAFE (stf, next, hmap_node,
> > -                        &flow_table->uuid_flow_table) {
> > -        remove_flows_from_sb_to_flow(flow_table, stf, NULL);
> > +                        &desired_ftable->uuid_flow_table) {
> > +        remove_flows_from_sb_to_flow(desired_ftable, stf, NULL);
> >      }
> >  }
> >
> > -void
> > -ovn_desired_flow_table_destroy(struct ovn_desired_flow_table *flow_table)
> > +static void
> > +ovn_desired_flow_table_destroy(struct ovn_desired_flow_table
> *desired_ftable)
> >  {
> > -    ovn_desired_flow_table_clear(flow_table);
> > -    hmap_destroy(&flow_table->match_flow_table);
> > -    hmap_destroy(&flow_table->uuid_flow_table);
> > +    ovn_desired_flow_table_clear(desired_ftable);
> > +    hmap_destroy(&desired_ftable->match_flow_table);
> > +    hmap_destroy(&desired_ftable->uuid_flow_table);
> >  }
> >
> >
> >  /* Installed flow table operations. */
> >  static void
> > -ovn_installed_flow_table_clear(void)
> > +ovn_installed_flow_table_clear(struct hmap *installed_flows)
> >  {
> >      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_flows) {
> > +        hmap_remove(installed_flows, &f->match_hmap_node);
> >          unlink_all_refs_for_installed_flow(f);
> >          installed_flow_destroy(f);
> >      }
> >  }
> >
> > -static void
> > -ovn_installed_flow_table_destroy(void)
> > -{
> > -    ovn_installed_flow_table_clear();
> > -    hmap_destroy(&installed_flows);
> > -}
> >
> >  /* Flow table update. */
> >
> > @@ -1809,24 +1863,27 @@ 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,
> > +update_installed_flows_by_compare(struct ovn_flow_table *flow_table,
> >                                    struct ovs_list *msgs)
> >  {
> > -    ovs_assert(ovs_list_is_empty(&flow_table->tracked_flows));
> > +    struct ovn_desired_flow_table *d_ftable =
> &flow_table->desired_ftable;
> > +
> > +    ovs_assert(ovs_list_is_empty(&d_ftable->tracked_flows));
> >      /* Iterate through all of the installed flows.  If any of them are no
> >       * 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,
> > +                        &flow_table->installed_flows) {
> >          unlink_all_refs_for_installed_flow(i);
> > -        struct desired_flow *d = desired_flow_lookup(flow_table,
> &i->flow);
> > +        struct desired_flow *d = desired_flow_lookup(d_ftable, &i->flow);
> >          if (!d) {
> >              /* Installed flow is no longer desirable.  Delete it from the
> >               * switch and from installed_flows. */
> >              installed_flow_del(&i->flow, msgs);
> >              ovn_flow_log(&i->flow, "removing installed");
> >
> > -            hmap_remove(&installed_flows, &i->match_hmap_node);
> > +            hmap_remove(&flow_table->installed_flows,
> &i->match_hmap_node);
> >              installed_flow_destroy(i);
> >          } else {
> >              if (!ofpacts_equal(i->flow.ofpacts, i->flow.ofpacts_len,
> > @@ -1843,15 +1900,16 @@ update_installed_flows_by_compare(struct
> ovn_desired_flow_table *flow_table,
> >      /* Iterate through the desired flows and add those that aren't found
> >       * 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);
> > +    HMAP_FOR_EACH (d, match_hmap_node, &d_ftable->match_flow_table) {
> > +        i = installed_flow_lookup(&d->flow,
> &flow_table->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(&flow_table->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
> > @@ -1940,13 +1998,13 @@ merge_tracked_flows(struct ovn_desired_flow_table
> *flow_table)
> >  }
> >
> >  static void
> > -update_installed_flows_by_track(struct ovn_desired_flow_table
> *flow_table,
> > +update_installed_flows_by_track(struct ovn_flow_table *flow_table,
> >                                  struct ovs_list *msgs)
> >  {
> > -    merge_tracked_flows(flow_table);
> > +    merge_tracked_flows(&flow_table->desired_ftable);
> >      struct desired_flow *f, *f_next;
> >      LIST_FOR_EACH_SAFE (f, f_next, track_list_node,
> > -                        &flow_table->tracked_flows) {
> > +                        &flow_table->desired_ftable.tracked_flows) {
> >          ovs_list_remove(&f->track_list_node);
> >          if (f->is_deleted) {
> >              /* The desired flow was deleted */
> > @@ -1959,7 +2017,8 @@ 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(&flow_table->installed_flows,
> > +                                &i->match_hmap_node);
> >                      installed_flow_destroy(i);
> >                  } else if (was_active) {
> >                      /* There are other desired flow(s) referencing this
> > @@ -1973,7 +2032,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,
> &flow_table->installed_flows);
> >              if (!i) {
> >                  /* Adding a new flow. */
> >                  installed_flow_add(&f->flow, msgs);
> > @@ -1981,7 +2041,8 @@ 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(&flow_table->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,7 +2096,8 @@ 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_flow_table *lflow_table,
> > +           struct ovn_flow_table *pflow_table,
> >             struct shash *pending_ct_zones,
> >             const struct sbrec_meter_table *meter_table,
> >             uint64_t req_cfg,
> > @@ -2126,10 +2188,16 @@ ofctrl_put(struct ovn_desired_flow_table
> *flow_table,
> >          }
> >      }
> >
> > -    if (flow_table->change_tracked) {
> > -        update_installed_flows_by_track(flow_table, &msgs);
> > +    if (lflow_table->desired_ftable.change_tracked) {
> > +        update_installed_flows_by_track(lflow_table, &msgs);
> > +    } else {
> > +        update_installed_flows_by_compare(lflow_table, &msgs);
> > +    }
> > +
> > +    if (pflow_table->desired_ftable.change_tracked) {
> > +        update_installed_flows_by_track(pflow_table, &msgs);
> >      } else {
> > -        update_installed_flows_by_compare(flow_table, &msgs);
> > +        update_installed_flows_by_compare(pflow_table, &msgs);
> >      }
> >
> Would it be possible to optimize here since we know if each of lflow and
> pflow tables are changed? We can split the "flow_changed" arg of this
> function into lflow_changed and pflow_changed, so that we only call the
> above update_installed_flows_by_track()/update_installed_flows_by_compare()
> functions for the one that's changed? Maybe it is not a critical
> optimization but I think it is natural since we already separated the flows
> into two tables.

I agree. I'll address this in v5.

Thanks
Numan

>
> >      /* Iterate through the installed groups from previous runs. If they
> > @@ -2243,8 +2311,10 @@ 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->desired_ftable.change_tracked = true;
> > +
>  ovs_assert(ovs_list_is_empty(&lflow_table->desired_ftable.tracked_flows));
> > +    pflow_table->desired_ftable.change_tracked = true;
> > +
>  ovs_assert(ovs_list_is_empty(&pflow_table->desired_ftable.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..61b3f166f2 100644
> > --- a/controller/ofctrl.h
> > +++ b/controller/ofctrl.h
> > @@ -31,28 +31,23 @@ struct ovsrec_bridge;
> >  struct sbrec_meter_table;
> >  struct shash;
> >
> > -struct ovn_desired_flow_table {
> > -    /* Hash map flow table using flow match conditions as hash key.*/
> > -    struct hmap match_flow_table;
> > -
> > -    /* SB uuid index for the cross reference nodes that link to the
> nodes in
> > -     * match_flow_table.*/
> > -    struct hmap uuid_flow_table;
> > -
> > -    /* Is flow changes tracked. */
> > -    bool change_tracked;
> > -    /* Tracked flow changes. */
> > -    struct ovs_list tracked_flows;
> > -};
> > +struct ovn_flow_table;
> > +
> > +struct ovn_flow_table *ovn_flow_table_alloc(void);
> > +void ovn_flow_table_clear(struct ovn_flow_table *);
> > +void ovn_flow_table_destroy(struct ovn_flow_table *);
> >
> >  /* Interface for OVN main loop. */
> >  void ofctrl_init(struct ovn_extend_table *group_table,
> >                   struct ovn_extend_table *meter_table,
> > +                 struct ovn_flow_table *lflow_table,
> > +                 struct ovn_flow_table *pflow_table,
> >                   int inactivity_probe_interval);
> >  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_flow_table *lflow_table,
> > +                struct ovn_flow_table *pflow_table,
> >                  struct shash *pending_ct_zones,
> >                  const struct sbrec_meter_table *,
> >                  uint64_t nb_cfg,
> > @@ -69,12 +64,12 @@ char *ofctrl_inject_pkt(const struct ovsrec_bridge
> *br_int,
> >                          const struct shash *port_groups);
> >
> >  /* Flow table interfaces to the rest of ovn-controller. */
> > -void ofctrl_add_flow(struct ovn_desired_flow_table *, uint8_t table_id,
> > +void ofctrl_add_flow(struct ovn_flow_table *, uint8_t table_id,
> >                       uint16_t priority, uint64_t cookie,
> >                       const struct match *, const struct ofpbuf *ofpacts,
> >                       const struct uuid *);
> >
> > -void ofctrl_add_or_append_flow(struct ovn_desired_flow_table
> *desired_flows,
> > +void ofctrl_add_or_append_flow(struct ovn_flow_table *,
> >                                 uint8_t table_id, uint16_t priority,
> >                                 uint64_t cookie, const struct match
> *match,
> >                                 const struct ofpbuf *actions,
> > @@ -84,7 +79,7 @@ void ofctrl_add_or_append_flow(struct
> ovn_desired_flow_table *desired_flows,
> >   * flows are removed only if they are not referenced by any other
> sb_uuid(s).
> >   * For flood-removing all related flows referenced by other sb_uuid(s),
> use
> >   * ofctrl_flood_remove_flows(). */
> > -void ofctrl_remove_flows(struct ovn_desired_flow_table *,
> > +void ofctrl_remove_flows(struct ovn_flow_table *,
> >                           const struct uuid *sb_uuid);
> >
> >  /* The function ofctrl_flood_remove_flows flood-removes flows from the
> desired
> > @@ -100,15 +95,11 @@ struct ofctrl_flood_remove_node {
> >      struct hmap_node hmap_node;
> >      struct uuid sb_uuid;
> >  };
> > -void ofctrl_flood_remove_flows(struct ovn_desired_flow_table *,
> > +void ofctrl_flood_remove_flows(struct ovn_flow_table *,
> >                                 struct hmap *flood_remove_nodes);
> >  void ofctrl_flood_remove_add_node(struct hmap *flood_remove_nodes,
> >                                    const struct uuid *sb_uuid);
> > -void ovn_desired_flow_table_init(struct ovn_desired_flow_table *);
> > -void ovn_desired_flow_table_clear(struct ovn_desired_flow_table *);
> > -void ovn_desired_flow_table_destroy(struct ovn_desired_flow_table *);
> > -
> > -void ofctrl_check_and_add_flow(struct ovn_desired_flow_table *,
> > +void ofctrl_check_and_add_flow(struct ovn_flow_table *,
> >                                 uint8_t table_id, uint16_t priority,
> >                                 uint64_t cookie, const struct match *,
> >                                 const struct ofpbuf *ofpacts,
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 5dd643f52b..1d0fb0badb 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -1646,107 +1646,14 @@ 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 ovn_desired_flow_table flow_table;
> > +struct ed_type_lflow_output {
> > +    /* Logical flow table */
> > +    struct ovn_flow_table *flow_table;
> >      /* group ids for load balancing */
> >      struct ovn_extend_table group_table;
> >      /* meter ids for QoS */
> > @@ -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->local_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(
> > @@ -1930,7 +1771,7 @@ static void init_lflow_ctx(struct engine_node *node,
> >      l_ctx_in->active_tunnels = &rt_data->active_tunnels;
> >      l_ctx_in->local_lport_ids = &rt_data->local_lport_ids;
> >
> > -    l_ctx_out->flow_table = &fo->flow_table;
> > +    l_ctx_out->flow_table = fo->flow_table;
> >      l_ctx_out->group_table = &fo->group_table;
> >      l_ctx_out->meter_table = &fo->meter_table;
> >      l_ctx_out->lfrr = &fo->lflow_resource_ref;
> > @@ -1940,12 +1781,12 @@ 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);
> > +    data->flow_table = ovn_flow_table_alloc();
> >      ovn_extend_table_init(&data->group_table);
> >      ovn_extend_table_init(&data->meter_table);
> >      data->pd.conj_id_ofs = 1;
> > @@ -1954,10 +1795,10 @@ 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;
> > -    ovn_desired_flow_table_destroy(&flow_output_data->flow_table);
> > +    struct ed_type_lflow_output *flow_output_data = data;
> > +    ovn_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);
> >      lflow_resource_destroy(&flow_output_data->lflow_resource_ref);
> > @@ -1965,7 +1806,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 +1832,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_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 +1842,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_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 +1864,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_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 +1877,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 +1894,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 +1906,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 +1921,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);
> > +            mac_binding_table, local_datapaths, lfo->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);
> > -
> > -    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 +1963,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 +2032,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);
> > -}
> > -
> > -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;
> > +    return _lflow_output_resource_ref_handler(node, data,
> REF_TYPE_PORTGROUP);
> >  }
> >
> >  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 +2066,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 +2093,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 +2110,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 +2126,235 @@ flow_output_sb_fdb_handler(struct engine_node
> *node, void *data)
> >      return handled;
> >  }
> >
> > +struct ed_type_pflow_output {
> > +    /* Desired physical flows. */
> > +    struct ovn_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->local_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);
> > +    data->flow_table = ovn_flow_table_alloc();
> > +    return data;
> > +}
> > +
> > +static void
> > +en_pflow_output_cleanup(void *data OVS_UNUSED)
> > +{
> > +    struct ed_type_pflow_output *pfo = data;
> > +    ovn_flow_table_destroy(pfo->flow_table);
> > +}
> > +
> > +/* Indicate to the flow_output engine that we need to recompute physical
> > + * flows. */
>
> This comment seems out of date.
>
> > +static void
> > +en_pflow_output_run(struct engine_node *node, void *data)
> > +{
> > +    struct ed_type_pflow_output *pfo = data;
> > +    struct ovn_flow_table *pflow_table = pfo->flow_table;
> > +    static bool first_run = true;
> > +    if (first_run) {
> > +        first_run = false;
> > +    } else {
> > +        ovn_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.
> */
> > +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;
> > @@ -2559,8 +2545,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");
> > @@ -2580,58 +2566,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);
> > +    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);
> > +
> > +    engine_add_input(&en_lflow_output, &en_ovs_open_vswitch, NULL);
> > +    engine_add_input(&en_lflow_output, &en_ovs_bridge, NULL);
> > +
> > +    engine_add_input(&en_lflow_output, &en_sb_mac_binding,
> > +                     lflow_output_sb_mac_binding_handler);
> > +    engine_add_input(&en_lflow_output, &en_sb_logical_flow,
> > +                     lflow_output_sb_logical_flow_handler);
> >      /* Using a noop handler since we don't really need any data from
> datapath
> >       * groups or a full recompute.  Update of a datapath group will put
> >       * logical flow into the tracked list, so the logical flow handler
> will
> >       * process all changes. */
> > -    engine_add_input(&en_flow_output, &en_sb_logical_dp_group,
> > +    engine_add_input(&en_lflow_output, &en_sb_logical_dp_group,
> >                       engine_noop_handler);
> > -    engine_add_input(&en_flow_output, &en_sb_dhcp_options, NULL);
> > -    engine_add_input(&en_flow_output, &en_sb_dhcpv6_options, NULL);
> > -    engine_add_input(&en_flow_output, &en_sb_dns, NULL);
> > -    engine_add_input(&en_flow_output, &en_sb_load_balancer,
> > -                     flow_output_sb_load_balancer_handler);
> > -    engine_add_input(&en_flow_output, &en_sb_fdb,
> > -                     flow_output_sb_fdb_handler);
> > +    engine_add_input(&en_lflow_output, &en_sb_dhcp_options, NULL);
> > +    engine_add_input(&en_lflow_output, &en_sb_dhcpv6_options, NULL);
> > +    engine_add_input(&en_lflow_output, &en_sb_dns, NULL);
> > +    engine_add_input(&en_lflow_output, &en_sb_load_balancer,
> > +                     lflow_output_sb_load_balancer_handler);
> > +    engine_add_input(&en_lflow_output, &en_sb_fdb,
> > +                     lflow_output_sb_fdb_handler);
> >
> >      engine_add_input(&en_ct_zones, &en_ovs_open_vswitch, NULL);
> >      engine_add_input(&en_ct_zones, &en_ovs_bridge, NULL);
> > @@ -2659,6 +2652,11 @@ main(int argc, char *argv[])
> >      engine_add_input(&en_runtime_data, &en_ovs_interface,
> >                       runtime_data_ovs_interface_handler);
> >
> > +    engine_add_input(&en_flow_output, &en_lflow_output,
> > +                     flow_output_lflow_output_handler);
> > +    engine_add_input(&en_flow_output, &en_pflow_output,
> > +                     flow_output_pflow_output_handler);
> > +
> >      struct engine_arg engine_arg = {
> >          .sb_idl = ovnsb_idl_loop.idl,
> >          .ovs_idl = ovs_idl_loop.idl,
> > @@ -2681,24 +2679,28 @@ main(int argc, char *argv[])
> >      engine_ovsdb_node_add_index(&en_sb_datapath_binding, "key",
> >                                  sbrec_datapath_binding_by_key);
> >
> > -    struct ed_type_flow_output *flow_output_data =
> > -        engine_get_internal_data(&en_flow_output);
> > +    struct ed_type_lflow_output *lflow_output_data =
> > +        engine_get_internal_data(&en_lflow_output);
> > +    struct ed_type_lflow_output *pflow_output_data =
> > +        engine_get_internal_data(&en_pflow_output);
> >      struct ed_type_ct_zones *ct_zones_data =
> >          engine_get_internal_data(&en_ct_zones);
> >      struct ed_type_runtime_data *runtime_data = NULL;
> >
> > -    ofctrl_init(&flow_output_data->group_table,
> > -                &flow_output_data->meter_table,
> > +    ofctrl_init(&lflow_output_data->group_table,
> > +                &lflow_output_data->meter_table,
> > +                lflow_output_data->flow_table,
> > +                pflow_output_data->flow_table,
> >                  get_ofctrl_probe_interval(ovs_idl_loop.idl));
> >      ofctrl_seqno_init();
> >
> >      unixctl_command_register("group-table-list", "", 0, 0,
> >                               extend_table_list,
> > -                             &flow_output_data->group_table);
> > +                             &lflow_output_data->group_table);
> >
> >      unixctl_command_register("meter-table-list", "", 0, 0,
> >                               extend_table_list,
> > -                             &flow_output_data->meter_table);
> > +                             &lflow_output_data->meter_table);
> >
> >      unixctl_command_register("ct-zone-list", "", 0, 0,
> >                               ct_zone_list,
> > @@ -2712,14 +2714,14 @@ main(int argc, char *argv[])
> >                               NULL);
> >      unixctl_command_register("lflow-cache/flush", "", 0, 0,
> >                               lflow_cache_flush_cmd,
> > -                             &flow_output_data->pd);
> > +                             &lflow_output_data->pd);
> >      /* Keep deprecated 'flush-lflow-cache' command for now. */
> >      unixctl_command_register("flush-lflow-cache", "[deprecated]", 0, 0,
> >                               lflow_cache_flush_cmd,
> > -                             &flow_output_data->pd);
> > +                             &lflow_output_data->pd);
> >      unixctl_command_register("lflow-cache/show-stats", "", 0, 0,
> >                               lflow_cache_show_stats_cmd,
> > -                             &flow_output_data->pd);
> > +                             &lflow_output_data->pd);
> >
> >      bool reset_ovnsb_idl_min_index = false;
> >      unixctl_command_register("sb-cluster-state-reset", "", 0, 0,
> > @@ -2958,13 +2960,20 @@ main(int argc, char *argv[])
> >                          binding_seqno_run(&runtime_data->local_bindings);
> >                      }
> >
> > -                    flow_output_data = engine_get_data(&en_flow_output);
> > -                    if (flow_output_data && ct_zones_data) {
> > -                        ofctrl_put(&flow_output_data->flow_table,
> > +                    lflow_output_data =
> engine_get_data(&en_lflow_output);
> > +                    pflow_output_data =
> engine_get_data(&en_pflow_output);
> > +                    if (lflow_output_data && pflow_output_data &&
> > +                        ct_zones_data) {
> > +                        bool flow_changed =
> > +                            (engine_node_changed(&en_lflow_output) ||
> > +                             engine_node_changed(&en_pflow_output));
> > +
> > +                        ofctrl_put(lflow_output_data->flow_table,
> > +                                   pflow_output_data->flow_table,
> >                                     &ct_zones_data->pending,
> >
> sbrec_meter_table_get(ovnsb_idl_loop.idl),
> >                                     ofctrl_seqno_get_req_cfg(),
> > -                                   engine_node_changed(&en_flow_output));
> > +                                   flow_changed);
> >                      }
> >                      ofctrl_seqno_run(ofctrl_get_cur_cfg());
> >                      if (runtime_data && ovs_idl_txn && ovnsb_idl_txn) {
> > @@ -3320,7 +3329,7 @@ lflow_cache_flush_cmd(struct unixctl_conn *conn
> OVS_UNUSED,
> >                        void *arg_)
> >  {
> >      VLOG_INFO("User triggered lflow cache flush.");
> > -    struct flow_output_persistent_data *fo_pd = arg_;
> > +    struct lflow_output_persistent_data *fo_pd = arg_;
> >      lflow_cache_flush(fo_pd->lflow_cache);
> >      fo_pd->conj_id_ofs = 1;
> >      engine_set_force_recompute(true);
> > @@ -3332,7 +3341,7 @@ static void
> >  lflow_cache_show_stats_cmd(struct unixctl_conn *conn, int argc
> OVS_UNUSED,
> >                             const char *argv[] OVS_UNUSED, void *arg_)
> >  {
> > -    struct flow_output_persistent_data *fo_pd = arg_;
> > +    struct lflow_output_persistent_data *fo_pd = arg_;
> >      struct lflow_cache *lc = fo_pd->lflow_cache;
> >      struct ds ds = DS_EMPTY_INITIALIZER;
> >
> > diff --git a/controller/physical.c b/controller/physical.c
> > index fa5d0d692d..7ecf6f72ef 100644
> > --- a/controller/physical.c
> > +++ b/controller/physical.c
> > @@ -249,7 +249,7 @@ put_remote_port_redirect_bridged(const struct
> >                                   struct local_datapath *ld,
> >                                   struct match *match,
> >                                   struct ofpbuf *ofpacts_p,
> > -                                 struct ovn_desired_flow_table
> *flow_table)
> > +                                 struct ovn_flow_table *flow_table)
> >  {
> >          if (strcmp(binding->type, "chassisredirect")) {
> >              /* bridged based redirect is only supported for
> chassisredirect
> > @@ -321,7 +321,7 @@ put_remote_port_redirect_overlay(const struct
> >                                   uint32_t port_key,
> >                                   struct match *match,
> >                                   struct ofpbuf *ofpacts_p,
> > -                                 struct ovn_desired_flow_table
> *flow_table)
> > +                                 struct ovn_flow_table *flow_table)
> >  {
> >      if (!is_ha_remote) {
> >          /* Setup encapsulation */
> > @@ -482,7 +482,7 @@ static void
> >  put_chassis_mac_conj_id_flow(const struct sbrec_chassis_table
> *chassis_table,
> >                               const struct sbrec_chassis *chassis,
> >                               struct ofpbuf *ofpacts_p,
> > -                             struct ovn_desired_flow_table *flow_table)
> > +                             struct ovn_flow_table *flow_table)
> >  {
> >      struct match match;
> >      struct remote_chassis_mac *mac;
> > @@ -525,7 +525,7 @@ put_replace_chassis_mac_flows(const struct simap
> *ct_zones,
> >                                const struct hmap *local_datapaths,
> >                                struct ofpbuf *ofpacts_p,
> >                                ofp_port_t ofport,
> > -                              struct ovn_desired_flow_table *flow_table)
> > +                              struct ovn_flow_table *flow_table)
> >  {
> >      /* Packets arriving on localnet port, could have been routed on
> >       * source chassis and hence will have a chassis mac.
> > @@ -619,7 +619,7 @@ put_replace_router_port_mac_flows(struct
> ovsdb_idl_index
> >                                    const struct hmap *local_datapaths,
> >                                    struct ofpbuf *ofpacts_p,
> >                                    ofp_port_t ofport,
> > -                                  struct ovn_desired_flow_table
> *flow_table)
> > +                                  struct ovn_flow_table *flow_table)
> >  {
> >      struct local_datapath *ld = get_local_datapath(local_datapaths,
> >
> localnet_port->datapath->
> > @@ -715,7 +715,7 @@ put_local_common_flows(uint32_t dp_key, uint32_t
> port_key,
> >                         uint32_t parent_port_key,
> >                         const struct zone_ids *zone_ids,
> >                         struct ofpbuf *ofpacts_p,
> > -                       struct ovn_desired_flow_table *flow_table)
> > +                       struct ovn_flow_table *flow_table)
> >  {
> >      struct match match;
> >
> > @@ -891,7 +891,7 @@ consider_port_binding(struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
> >                        const struct hmap *local_datapaths,
> >                        const struct sbrec_port_binding *binding,
> >                        const struct sbrec_chassis *chassis,
> > -                      struct ovn_desired_flow_table *flow_table,
> > +                      struct ovn_flow_table *flow_table,
> >                        struct ofpbuf *ofpacts_p)
> >  {
> >      uint32_t dp_key = binding->datapath->tunnel_key;
> > @@ -1287,7 +1287,7 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve,
> >                    const struct hmap *local_datapaths,
> >                    const struct sbrec_chassis *chassis,
> >                    const struct sbrec_multicast_group *mc,
> > -                  struct ovn_desired_flow_table *flow_table)
> > +                  struct ovn_flow_table *flow_table)
> >  {
> >      uint32_t dp_key = mc->datapath->tunnel_key;
> >      if (!get_local_datapath(local_datapaths, dp_key)) {
> > @@ -1426,7 +1426,7 @@ update_ofports(struct simap *old, struct simap *new)
> >
> >  void
> >  physical_handle_port_binding_changes(struct physical_ctx *p_ctx,
> > -                                     struct ovn_desired_flow_table
> *flow_table)
> > +                                     struct ovn_flow_table *flow_table)
> >  {
> >      const struct sbrec_port_binding *binding;
> >      struct ofpbuf ofpacts;
> > @@ -1452,7 +1452,7 @@ physical_handle_port_binding_changes(struct
> physical_ctx *p_ctx,
> >
> >  void
> >  physical_handle_mc_group_changes(struct physical_ctx *p_ctx,
> > -                                 struct ovn_desired_flow_table
> *flow_table)
> > +                                 struct ovn_flow_table *flow_table)
> >  {
> >      const struct sbrec_multicast_group *mc;
> >      SBREC_MULTICAST_GROUP_TABLE_FOR_EACH_TRACKED (mc,
> p_ctx->mc_group_table) {
> > @@ -1471,7 +1471,7 @@ physical_handle_mc_group_changes(struct
> physical_ctx *p_ctx,
> >
> >  void
> >  physical_run(struct physical_ctx *p_ctx,
> > -             struct ovn_desired_flow_table *flow_table)
> > +             struct ovn_flow_table *flow_table)
> >  {
> >      if (!hc_uuid) {
> >          hc_uuid = xmalloc(sizeof(struct uuid));
> > @@ -1818,7 +1818,7 @@ physical_run(struct physical_ctx *p_ctx,
> >
> >  bool
> >  physical_handle_ovs_iface_changes(struct physical_ctx *p_ctx,
> > -                                  struct ovn_desired_flow_table
> *flow_table)
> > +                                  struct ovn_flow_table *flow_table)
> >  {
> >      const struct ovsrec_interface *iface_rec;
> >      OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec,
> p_ctx->iface_table) {
> > @@ -1883,7 +1883,7 @@ get_tunnel_ofport(const char *chassis_name, char
> *encap_ip, ofp_port_t *ofport)
> >  }
> >
> >  void
> > -physical_clear_unassoc_flows_with_db(struct ovn_desired_flow_table
> *flow_table)
> > +physical_clear_unassoc_flows_with_db(struct ovn_flow_table *flow_table)
> >  {
> >      if (hc_uuid) {
> >          ofctrl_remove_flows(flow_table, hc_uuid);
> > @@ -1893,7 +1893,7 @@ physical_clear_unassoc_flows_with_db(struct
> ovn_desired_flow_table *flow_table)
> >  void
> >  physical_clear_dp_flows(struct physical_ctx *p_ctx,
> >                          struct hmapx *ct_updated_datapaths,
> > -                        struct ovn_desired_flow_table *flow_table)
> > +                        struct ovn_flow_table *flow_table)
> >  {
> >      const struct sbrec_port_binding *binding;
> >      SBREC_PORT_BINDING_TABLE_FOR_EACH (binding,
> p_ctx->port_binding_table) {
> > diff --git a/controller/physical.h b/controller/physical.h
> > index 0bf13f2683..ccd0672ec4 100644
> > --- a/controller/physical.h
> > +++ b/controller/physical.h
> > @@ -61,17 +61,17 @@ struct physical_ctx {
> >
> >  void physical_register_ovs_idl(struct ovsdb_idl *);
> >  void physical_run(struct physical_ctx *,
> > -                  struct ovn_desired_flow_table *);
> > -void physical_clear_unassoc_flows_with_db(struct ovn_desired_flow_table
> *);
> > +                  struct ovn_flow_table *);
> > +void physical_clear_unassoc_flows_with_db(struct ovn_flow_table *);
> >  void physical_clear_dp_flows(struct physical_ctx *p_ctx,
> >                               struct hmapx *ct_updated_datapaths,
> > -                             struct ovn_desired_flow_table *flow_table);
> > +                             struct ovn_flow_table *flow_table);
> >  void physical_handle_port_binding_changes(struct physical_ctx *,
> > -                                          struct ovn_desired_flow_table
> *);
> > +                                          struct ovn_flow_table *);
> >  void physical_handle_mc_group_changes(struct physical_ctx *,
> > -                                      struct ovn_desired_flow_table *);
> > +                                      struct ovn_flow_table *);
> >  bool physical_handle_ovs_iface_changes(struct physical_ctx *,
> > -                                       struct ovn_desired_flow_table *);
> > +                                       struct ovn_flow_table *);
> >  bool get_tunnel_ofport(const char *chassis_name, char *encap_ip,
> >                         ofp_port_t *ofport);
> >  #endif /* controller/physical.h */
> > --
> > 2.29.2
> >
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list