[ovs-dev,ovn,v7,8/9] ovn-controller: Use the tracked runtime data changes for flow calculation.

From: Venkata Anil <anilvenkata@redhat.com>

This patch processes the logical flows of tracked datapaths
and tracked logical ports. To handle the tracked logical port
changes, reference of logical flows to port bindings is maintained.

Acked-by: Mark Michelson <mmichels@redhat.com>
Co-Authored-by: Numan Siddique <numans@ovn.org>
Signed-off-by: Venkata Anil <anilvenkata@redhat.com>
Signed-off-by: Numan Siddique <numans@ovn.org>
---
 controller/lflow.c          |  82 +++++++++++++++++++++++-
 controller/lflow.h          |  14 +++-
 controller/ovn-controller.c | 124 ++++++++++++++++++++----------------
 controller/physical.h       |   3 +-
 tests/ovn-performance.at    |   4 +-
 5 files changed, 168 insertions(+), 59 deletions(-)

On Wed, May 20, 2020 at 12:40 PM <numans@ovn.org> wrote:
>
> From: Venkata Anil <anilvenkata@redhat.com>
>
> This patch processes the logical flows of tracked datapaths
> and tracked logical ports. To handle the tracked logical port
> changes, reference of logical flows to port bindings is maintained.
>
> Acked-by: Mark Michelson <mmichels@redhat.com>
> Co-Authored-by: Numan Siddique <numans@ovn.org>
> Signed-off-by: Venkata Anil <anilvenkata@redhat.com>
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
>  controller/lflow.c          |  82 +++++++++++++++++++++++-
>  controller/lflow.h          |  14 +++-
>  controller/ovn-controller.c | 124 ++++++++++++++++++++----------------
>  controller/physical.h       |   3 +-
>  tests/ovn-performance.at    |   4 +-
>  5 files changed, 168 insertions(+), 59 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 01214a3a6..45bf4aa4b 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -258,7 +258,7 @@ lflow_resource_destroy_lflow(struct
lflow_resource_ref *lfrr,
>  /* Adds the logical flows from the Logical_Flow table to flow tables. */
>  static void
>  add_logical_flows(struct lflow_ctx_in *l_ctx_in,
> -                  struct lflow_ctx_out *l_ctx_out)
> +                        struct lflow_ctx_out *l_ctx_out)
>  {
>      const struct sbrec_logical_flow *lflow;
>
> @@ -649,6 +649,8 @@ consider_logical_flow(const struct sbrec_logical_flow
*lflow,
>                  int64_t dp_id = lflow->logical_datapath->tunnel_key;
>                  char buf[16];
>                  snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, dp_id,
port_id);
> +                lflow_resource_add(l_ctx_out->lfrr,
REF_TYPE_PORTBINDING, buf,
> +                                   &lflow->header_.uuid);
>                  if (!sset_contains(l_ctx_in->local_lport_ids, buf)) {
>                      VLOG_DBG("lflow "UUID_FMT
>                               " port %s in match is not local, skip",
> @@ -847,3 +849,81 @@ lflow_destroy(void)
>      expr_symtab_destroy(&symtab);
>      shash_destroy(&symtab);
>  }
> +
> +bool
> +lflow_evaluate_pb_changes(const struct sbrec_port_binding_table
*pb_table)
> +{
> +    const struct sbrec_port_binding *binding;
> +    SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (binding, pb_table) {
> +        if (!strcmp(binding->type, "remote")) {

"remote" can be handled just like any regular VIF, just that it would never
be bound on the current chassis.

> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}
> +
> +bool
> +lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp,
> +                             struct lflow_ctx_in *l_ctx_in,
> +                             struct lflow_ctx_out *l_ctx_out)
> +{
> +    bool handled = true;
> +    struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
> +    struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts);
> +    const struct sbrec_dhcp_options *dhcp_opt_row;
> +    SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row,
> +                                       l_ctx_in->dhcp_options_table) {
> +        dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code,
> +                     dhcp_opt_row->type);
> +    }
> +
> +
> +    const struct sbrec_dhcpv6_options *dhcpv6_opt_row;
> +    SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row,
> +                                         l_ctx_in->dhcpv6_options_table)
{
> +       dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name,
dhcpv6_opt_row->code,
> +                    dhcpv6_opt_row->type);
> +    }
> +
> +    struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts);
> +    nd_ra_opts_init(&nd_ra_opts);
> +
> +    struct controller_event_options controller_event_opts;
> +    controller_event_opts_init(&controller_event_opts);
> +
> +    struct sbrec_logical_flow *lf_row =
sbrec_logical_flow_index_init_row(
> +        l_ctx_in->sbrec_logical_flow_by_logical_datapath);
> +    sbrec_logical_flow_index_set_logical_datapath(lf_row, dp);
> +
> +    const struct sbrec_logical_flow *lflow;
> +    SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL (
> +        lflow, lf_row, l_ctx_in->sbrec_logical_flow_by_logical_datapath)
{
> +        if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> +                                   &nd_ra_opts, &controller_event_opts,
> +                                   l_ctx_in, l_ctx_out)) {
> +            handled = false;
> +            break;
> +        }
> +    }
> +
> +    dhcp_opts_destroy(&dhcp_opts);
> +    dhcp_opts_destroy(&dhcpv6_opts);
> +    nd_ra_opts_destroy(&nd_ra_opts);
> +    controller_event_opts_destroy(&controller_event_opts);
> +    return handled;
> +}
> +
> +bool
> +lflow_handle_flows_for_lport(const struct sbrec_port_binding *pb,
> +                             struct lflow_ctx_in *l_ctx_in,
> +                             struct lflow_ctx_out *l_ctx_out)
> +{
> +    int64_t dp_id = pb->datapath->tunnel_key;
> +    char pb_ref_name[16];
> +    snprintf(pb_ref_name, sizeof(pb_ref_name), "%"PRId64"_%"PRId64,
> +             dp_id, pb->tunnel_key);
> +    bool changed = true;
> +    return lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb_ref_name,
> +                                    l_ctx_in, l_ctx_out, &changed);
> +}
> diff --git a/controller/lflow.h b/controller/lflow.h
> index f02f709d7..00c1b5c47 100644
> --- a/controller/lflow.h
> +++ b/controller/lflow.h
> @@ -48,6 +48,9 @@ struct sbrec_dhcp_options_table;
>  struct sbrec_dhcpv6_options_table;
>  struct sbrec_logical_flow_table;
>  struct sbrec_mac_binding_table;
> +struct sbrec_port_binding_table;
> +struct sbrec_datapath_binding;
> +struct sbrec_port_binding;
>  struct simap;
>  struct sset;
>  struct uuid;
> @@ -72,7 +75,8 @@ struct uuid;
>
>  enum ref_type {
>      REF_TYPE_ADDRSET,
> -    REF_TYPE_PORTGROUP
> +    REF_TYPE_PORTGROUP,
> +    REF_TYPE_PORTBINDING
>  };
>
>  /* Maintains the relationship for a pair of named resource and
> @@ -117,6 +121,7 @@ void lflow_resource_clear(struct lflow_resource_ref
*);
>
>  struct lflow_ctx_in {
>      struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath;
> +    struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath;
>      struct ovsdb_idl_index *sbrec_port_binding_by_name;
>      const struct sbrec_dhcp_options_table *dhcp_options_table;
>      const struct sbrec_dhcpv6_options_table *dhcpv6_options_table;
> @@ -157,4 +162,11 @@ void lflow_destroy(void);
>
>  void lflow_expr_destroy(struct hmap *lflow_expr_cache);
>
> +bool lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *,
> +                                  struct lflow_ctx_in *,
> +                                  struct lflow_ctx_out *);
> +bool lflow_handle_flows_for_lport(const struct sbrec_port_binding *,
> +                                  struct lflow_ctx_in *,
> +                                  struct lflow_ctx_out *);
> +bool lflow_evaluate_pb_changes(const struct sbrec_port_binding_table *);
>  #endif /* controller/lflow.h */
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index dc790f0f7..e53f750bf 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1440,6 +1440,11 @@ static void init_lflow_ctx(struct engine_node
*node,
>                  engine_get_input("SB_port_binding", node),
>                  "name");
>
> +    struct ovsdb_idl_index *sbrec_logical_flow_by_dp =
> +        engine_ovsdb_node_get_index(
> +                engine_get_input("SB_logical_flow", node),
> +                "logical_datapath");
> +
>      struct ovsdb_idl_index *sbrec_mc_group_by_name_dp =
>          engine_ovsdb_node_get_index(
>                  engine_get_input("SB_multicast_group", node),
> @@ -1487,6 +1492,8 @@ static void init_lflow_ctx(struct engine_node *node,
>
>      l_ctx_in->sbrec_multicast_group_by_name_datapath =
>          sbrec_mc_group_by_name_dp;
> +    l_ctx_in->sbrec_logical_flow_by_logical_datapath =
> +        sbrec_logical_flow_by_dp;
>      l_ctx_in->sbrec_port_binding_by_name = sbrec_port_binding_by_name;
>      l_ctx_in->dhcp_options_table  = dhcp_table;
>      l_ctx_in->dhcpv6_options_table = dhcpv6_table;
> @@ -1641,7 +1648,8 @@ flow_output_sb_mac_binding_handler(struct
engine_node *node, void *data)
>  }
>
>  static bool
> -flow_output_sb_port_binding_handler(struct engine_node *node, void *data)
> +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);
> @@ -1649,56 +1657,21 @@ flow_output_sb_port_binding_handler(struct
engine_node *node, void *data)
>      struct ed_type_flow_output *fo = data;
>      struct ovn_desired_flow_table *flow_table = &fo->flow_table;
>
> -    /* XXX: now we handle port-binding changes for physical flow
processing
> -     * only, but port-binding change can have impact to logical flow
> -     * processing, too, in below circumstances:
> -     *
> -     *  - When a port-binding for a lport is inserted/deleted but the
lflow
> -     *    using that lport doesn't change.
> -     *
> -     *    This can happen only when the lport name is used by ACL match
> -     *    condition, which is specified by user. Even in that case, if
the port
> -     *    is actually bound on the current chassis it will trigger
recompute on
> -     *    that chassis since ovs interface would be updated. So the only
> -     *    situation this would have real impact is when user defines an
ACL
> -     *    that includes lport that is not on current chassis, and there
is a
> -     *    port-binding creation/deletion related to that lport.e.g.: an
ACL is
> -     *    defined:
> -     *
> -     *    to-lport 1000 'outport=="A" && inport=="B"' allow-related
> -     *
> -     *    If "A" is on current chassis, but "B" is lport that hasn't been
> -     *    created yet. When a lport "B" is created and bound on another
> -     *    chassis, the ACL will not take effect on the current chassis
until a
> -     *    recompute is triggered later. This case doesn't seem to be a
problem
> -     *    for real world use cases because usually lport is created
before
> -     *    being referenced by name in ACLs.
> -     *
> -     *  - When is_chassis_resident(<lport>) is used in lflow. In this
case the
> -     *    port binding is not a regular VIF. It can be either "patch" or
> -     *    "external", with ha-chassis-group assigned.  In current
> -     *    "runtime_data" handling, port-binding changes for these types
always
> -     *    trigger recomputing. So it is fine even if we do not handle it
here.
> -     *    (due to the ovsdb tracking support for referenced table
changes,
> -     *    ha-chassis-group changes will appear as port-binding change).
> -     *
> -     *  - When a mac-binding doesn't change but the port-binding related
to
> -     *    that mac-binding is deleted. In this case the neighbor flow
generated
> -     *    for the mac-binding should be deleted. This would not cause
any real
> -     *    issue for now, since the port-binding related to mac-binding is
> -     *    always logical router port, and any change to logical router
port
> -     *    would just trigger recompute.
> -     *
> -     * Although there is no correctness issue so far (except the unusual
ACL
> -     * use case, which doesn't seem to be a real problem), it might be
better
> -     * to handle this more gracefully, without the need to consider these
> -     * tricky scenarios.  One approach is to maintain a mapping between
lport
> -     * names and the lflows that uses them, and reprocess the related
lflows
> -     * when related port-bindings change.
> -     */
>      struct physical_ctx p_ctx;
>      init_physical_ctx(node, rt_data, &p_ctx);
>
> +    if (!lflow_evaluate_pb_changes(p_ctx.port_binding_table)) {
> +        /* If this returns false, it means there is an impact on
> +         * the logical flow processing because of some changes in
> +         * port bindings. Return false so that recompute is triggered
> +         * for this stage. */
> +        return false;
> +    }
> +
> +    /* 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);
> @@ -1786,6 +1759,8 @@ _flow_output_resource_ref_handler(struct
engine_node *node, void *data,
>              updated = &pg_data->updated;
>              deleted = &pg_data->deleted;
>              break;
> +
> +        case REF_TYPE_PORTBINDING:
>          default:
>              OVS_NOT_REACHED();
>      }
> @@ -1909,16 +1884,52 @@ flow_output_runtime_data_handler(struct
engine_node *node,
>          return false;
>      }
>
> -    if (!hmap_is_empty(&tracked_data->tracked_dp_bindings)) {
> -        /* We are not yet handling the tracked datapath binding
> -         * changes. Return false to trigger full recompute. */
> -        return false;
> +    if (hmap_is_empty(&tracked_data->tracked_dp_bindings)) {
> +        if (tracked_data->local_lports_changed) {
> +            engine_set_node_state(node, EN_UPDATED);
> +        }
> +        return true;
>      }
>
> -    if (tracked_data->local_lports_changed) {
> +    struct lflow_ctx_in l_ctx_in;
> +    struct lflow_ctx_out l_ctx_out;
> +    struct ed_type_flow_output *fo = data;
> +    struct ed_type_runtime_data *rt_data =
> +        engine_get_input_data("runtime_data", node);
> +    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);
> +
> +    bool handled = true;
> +    struct tracked_binding_datapath *tdp;
> +    HMAP_FOR_EACH (tdp, node, &tracked_data->tracked_dp_bindings) {
> +        if (tdp->is_new) {
> +            handled = lflow_add_flows_for_datapath(tdp->dp, &l_ctx_in,
> +                                                   &l_ctx_out);
> +            if (!handled) {
> +                break;
> +            }
> +        } else {
> +            struct tracked_binding_lport *lport;
> +            LIST_FOR_EACH (lport, list_node, &tdp->lports_head) {
> +                if (!lflow_handle_flows_for_lport(lport->pb, &l_ctx_in,
> +                                                  &l_ctx_out)) {
> +                    handled = false;
> +                    break;
> +                }
> +            }
> +            if (!handled) {
> +                break;
> +            }
> +        }
> +    }
> +
> +    if (handled) {
>          engine_set_node_state(node, EN_UPDATED);
>      }
> -    return true;
> +
> +    return handled;
>  }
>
>  struct ovn_controller_exit_args {
> @@ -1976,6 +1987,9 @@ main(int argc, char *argv[])
>          = chassis_index_create(ovnsb_idl_loop.idl);
>      struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath
>          = mcast_group_index_create(ovnsb_idl_loop.idl);
> +    struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath
> +        = ovsdb_idl_index_create1(ovnsb_idl_loop.idl,
> +
 &sbrec_logical_flow_col_logical_datapath);
>      struct ovsdb_idl_index *sbrec_port_binding_by_name
>          = ovsdb_idl_index_create1(ovnsb_idl_loop.idl,
>                                    &sbrec_port_binding_col_logical_port);
> @@ -2125,6 +2139,8 @@ main(int argc, char *argv[])
>      engine_ovsdb_node_add_index(&en_sb_chassis, "name",
sbrec_chassis_by_name);
>      engine_ovsdb_node_add_index(&en_sb_multicast_group, "name_datapath",
>                                  sbrec_multicast_group_by_name_datapath);
> +    engine_ovsdb_node_add_index(&en_sb_logical_flow, "logical_datapath",
> +                                sbrec_logical_flow_by_logical_datapath);
>      engine_ovsdb_node_add_index(&en_sb_port_binding, "name",
>                                  sbrec_port_binding_by_name);
>      engine_ovsdb_node_add_index(&en_sb_port_binding, "key",
> diff --git a/controller/physical.h b/controller/physical.h
> index 9ca34436a..481f03901 100644
> --- a/controller/physical.h
> +++ b/controller/physical.h
> @@ -33,6 +33,7 @@ struct ovsrec_bridge;
>  struct simap;
>  struct sbrec_multicast_group_table;
>  struct sbrec_port_binding_table;
> +struct sbrec_port_binding;
>  struct sset;
>
>  /* OVN Geneve option information.
> @@ -61,7 +62,7 @@ struct physical_ctx {
>  void physical_register_ovs_idl(struct ovsdb_idl *);
>  void physical_run(struct physical_ctx *,
>                    struct ovn_desired_flow_table *);
> -void physical_handle_port_binding_changes(struct physical_ctx *,
> +void physical_handle_port_binding_changes(struct physical_ctx *p_ctx,
>                                            struct ovn_desired_flow_table
*);
>  void physical_handle_mc_group_changes(struct physical_ctx *,
>                                        struct ovn_desired_flow_table *);
> diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at
> index 6e064e64f..a12757e18 100644
> --- a/tests/ovn-performance.at
> +++ b/tests/ovn-performance.at
> @@ -353,8 +353,8 @@ for i in 1 2; do
>      )
>
>      # Bind port $lp and wait for it to come up
> -    OVN_CONTROLLER_EXPECT_HIT_COND(
> -        [hv$i hv$j], [lflow_run], [>0 =0],
> +    OVN_CONTROLLER_EXPECT_NO_HIT(
> +        [hv$i hv$j], [lflow_run],
>          [as hv$i ovs-vsctl add-port br-int $vif -- set Interface $vif
external-ids:iface-id=$lp &&
>           ovn-nbctl wait-until Logical_Switch_Port $lp 'up=true' &&
>           ovn-nbctl --wait=hv sync]
> --
> 2.26.2
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On Thu, May 21, 2020 at 12:42 PM Han Zhou <hzhou@ovn.org> wrote:

> On Wed, May 20, 2020 at 12:40 PM <numans@ovn.org> wrote:
> >
> > From: Venkata Anil <anilvenkata@redhat.com>
> >
> > This patch processes the logical flows of tracked datapaths
> > and tracked logical ports. To handle the tracked logical port
> > changes, reference of logical flows to port bindings is maintained.
> >
> > Acked-by: Mark Michelson <mmichels@redhat.com>
> > Co-Authored-by: Numan Siddique <numans@ovn.org>
> > Signed-off-by: Venkata Anil <anilvenkata@redhat.com>
> > Signed-off-by: Numan Siddique <numans@ovn.org>
> > ---
> >  controller/lflow.c          |  82 +++++++++++++++++++++++-
> >  controller/lflow.h          |  14 +++-
> >  controller/ovn-controller.c | 124 ++++++++++++++++++++----------------
> >  controller/physical.h       |   3 +-
> >  tests/ovn-performance.at    |   4 +-
> >  5 files changed, 168 insertions(+), 59 deletions(-)
> >
> > diff --git a/controller/lflow.c b/controller/lflow.c
> > index 01214a3a6..45bf4aa4b 100644
> > --- a/controller/lflow.c
> > +++ b/controller/lflow.c
> > @@ -258,7 +258,7 @@ lflow_resource_destroy_lflow(struct
> lflow_resource_ref *lfrr,
> >  /* Adds the logical flows from the Logical_Flow table to flow tables. */
> >  static void
> >  add_logical_flows(struct lflow_ctx_in *l_ctx_in,
> > -                  struct lflow_ctx_out *l_ctx_out)
> > +                        struct lflow_ctx_out *l_ctx_out)
> >  {
> >      const struct sbrec_logical_flow *lflow;
> >
> > @@ -649,6 +649,8 @@ consider_logical_flow(const struct sbrec_logical_flow
> *lflow,
> >                  int64_t dp_id = lflow->logical_datapath->tunnel_key;
> >                  char buf[16];
> >                  snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, dp_id,
> port_id);
> > +                lflow_resource_add(l_ctx_out->lfrr,
> REF_TYPE_PORTBINDING, buf,
> > +                                   &lflow->header_.uuid);
> >                  if (!sset_contains(l_ctx_in->local_lport_ids, buf)) {
> >                      VLOG_DBG("lflow "UUID_FMT
> >                               " port %s in match is not local, skip",
> > @@ -847,3 +849,81 @@ lflow_destroy(void)
> >      expr_symtab_destroy(&symtab);
> >      shash_destroy(&symtab);
> >  }
> > +
> > +bool
> > +lflow_evaluate_pb_changes(const struct sbrec_port_binding_table
> *pb_table)
> > +{
> > +    const struct sbrec_port_binding *binding;
> > +    SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (binding, pb_table) {
> > +        if (!strcmp(binding->type, "remote")) {
>
> "remote" can be handled just like any regular VIF, just that it would never
> be bound on the current chassis.
>

I didn't handle this comment in v8 as I was not to look into the remote
code again
to understand a bit.

The present approach is a bit inefficient because it would resort to full
recompute when
any port bindings of type remote change. Are you fine If I handle this in
next patch
series which I've started working on ?

If I understand correctly from the comment, if any remote port binding
changes,
we need to add this in the tracked_dp_bindings ? so
that lflow_handle_flows_for_lport()
will handle this ?

Thanks
Numan


> > +            return false;
> > +        }
> > +    }
> > +
> > +    return true;
> > +}
> > +
> > +bool
> > +lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp,
> > +                             struct lflow_ctx_in *l_ctx_in,
> > +                             struct lflow_ctx_out *l_ctx_out)
> > +{
> > +    bool handled = true;
> > +    struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
> > +    struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts);
> > +    const struct sbrec_dhcp_options *dhcp_opt_row;
> > +    SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row,
> > +                                       l_ctx_in->dhcp_options_table) {
> > +        dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code,
> > +                     dhcp_opt_row->type);
> > +    }
> > +
> > +
> > +    const struct sbrec_dhcpv6_options *dhcpv6_opt_row;
> > +    SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row,
> > +                                         l_ctx_in->dhcpv6_options_table)
> {
> > +       dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name,
> dhcpv6_opt_row->code,
> > +                    dhcpv6_opt_row->type);
> > +    }
> > +
> > +    struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts);
> > +    nd_ra_opts_init(&nd_ra_opts);
> > +
> > +    struct controller_event_options controller_event_opts;
> > +    controller_event_opts_init(&controller_event_opts);
> > +
> > +    struct sbrec_logical_flow *lf_row =
> sbrec_logical_flow_index_init_row(
> > +        l_ctx_in->sbrec_logical_flow_by_logical_datapath);
> > +    sbrec_logical_flow_index_set_logical_datapath(lf_row, dp);
> > +
> > +    const struct sbrec_logical_flow *lflow;
> > +    SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL (
> > +        lflow, lf_row, l_ctx_in->sbrec_logical_flow_by_logical_datapath)
> {
> > +        if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> > +                                   &nd_ra_opts, &controller_event_opts,
> > +                                   l_ctx_in, l_ctx_out)) {
> > +            handled = false;
> > +            break;
> > +        }
> > +    }
> > +
> > +    dhcp_opts_destroy(&dhcp_opts);
> > +    dhcp_opts_destroy(&dhcpv6_opts);
> > +    nd_ra_opts_destroy(&nd_ra_opts);
> > +    controller_event_opts_destroy(&controller_event_opts);
> > +    return handled;
> > +}
> > +
> > +bool
> > +lflow_handle_flows_for_lport(const struct sbrec_port_binding *pb,
> > +                             struct lflow_ctx_in *l_ctx_in,
> > +                             struct lflow_ctx_out *l_ctx_out)
> > +{
> > +    int64_t dp_id = pb->datapath->tunnel_key;
> > +    char pb_ref_name[16];
> > +    snprintf(pb_ref_name, sizeof(pb_ref_name), "%"PRId64"_%"PRId64,
> > +             dp_id, pb->tunnel_key);
> > +    bool changed = true;
> > +    return lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb_ref_name,
> > +                                    l_ctx_in, l_ctx_out, &changed);
> > +}
> > diff --git a/controller/lflow.h b/controller/lflow.h
> > index f02f709d7..00c1b5c47 100644
> > --- a/controller/lflow.h
> > +++ b/controller/lflow.h
> > @@ -48,6 +48,9 @@ struct sbrec_dhcp_options_table;
> >  struct sbrec_dhcpv6_options_table;
> >  struct sbrec_logical_flow_table;
> >  struct sbrec_mac_binding_table;
> > +struct sbrec_port_binding_table;
> > +struct sbrec_datapath_binding;
> > +struct sbrec_port_binding;
> >  struct simap;
> >  struct sset;
> >  struct uuid;
> > @@ -72,7 +75,8 @@ struct uuid;
> >
> >  enum ref_type {
> >      REF_TYPE_ADDRSET,
> > -    REF_TYPE_PORTGROUP
> > +    REF_TYPE_PORTGROUP,
> > +    REF_TYPE_PORTBINDING
> >  };
> >
> >  /* Maintains the relationship for a pair of named resource and
> > @@ -117,6 +121,7 @@ void lflow_resource_clear(struct lflow_resource_ref
> *);
> >
> >  struct lflow_ctx_in {
> >      struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath;
> > +    struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath;
> >      struct ovsdb_idl_index *sbrec_port_binding_by_name;
> >      const struct sbrec_dhcp_options_table *dhcp_options_table;
> >      const struct sbrec_dhcpv6_options_table *dhcpv6_options_table;
> > @@ -157,4 +162,11 @@ void lflow_destroy(void);
> >
> >  void lflow_expr_destroy(struct hmap *lflow_expr_cache);
> >
> > +bool lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *,
> > +                                  struct lflow_ctx_in *,
> > +                                  struct lflow_ctx_out *);
> > +bool lflow_handle_flows_for_lport(const struct sbrec_port_binding *,
> > +                                  struct lflow_ctx_in *,
> > +                                  struct lflow_ctx_out *);
> > +bool lflow_evaluate_pb_changes(const struct sbrec_port_binding_table *);
> >  #endif /* controller/lflow.h */
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index dc790f0f7..e53f750bf 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -1440,6 +1440,11 @@ static void init_lflow_ctx(struct engine_node
> *node,
> >                  engine_get_input("SB_port_binding", node),
> >                  "name");
> >
> > +    struct ovsdb_idl_index *sbrec_logical_flow_by_dp =
> > +        engine_ovsdb_node_get_index(
> > +                engine_get_input("SB_logical_flow", node),
> > +                "logical_datapath");
> > +
> >      struct ovsdb_idl_index *sbrec_mc_group_by_name_dp =
> >          engine_ovsdb_node_get_index(
> >                  engine_get_input("SB_multicast_group", node),
> > @@ -1487,6 +1492,8 @@ static void init_lflow_ctx(struct engine_node
> *node,
> >
> >      l_ctx_in->sbrec_multicast_group_by_name_datapath =
> >          sbrec_mc_group_by_name_dp;
> > +    l_ctx_in->sbrec_logical_flow_by_logical_datapath =
> > +        sbrec_logical_flow_by_dp;
> >      l_ctx_in->sbrec_port_binding_by_name = sbrec_port_binding_by_name;
> >      l_ctx_in->dhcp_options_table  = dhcp_table;
> >      l_ctx_in->dhcpv6_options_table = dhcpv6_table;
> > @@ -1641,7 +1648,8 @@ flow_output_sb_mac_binding_handler(struct
> engine_node *node, void *data)
> >  }
> >
> >  static bool
> > -flow_output_sb_port_binding_handler(struct engine_node *node, void
> *data)
> > +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);
> > @@ -1649,56 +1657,21 @@ flow_output_sb_port_binding_handler(struct
> engine_node *node, void *data)
> >      struct ed_type_flow_output *fo = data;
> >      struct ovn_desired_flow_table *flow_table = &fo->flow_table;
> >
> > -    /* XXX: now we handle port-binding changes for physical flow
> processing
> > -     * only, but port-binding change can have impact to logical flow
> > -     * processing, too, in below circumstances:
> > -     *
> > -     *  - When a port-binding for a lport is inserted/deleted but the
> lflow
> > -     *    using that lport doesn't change.
> > -     *
> > -     *    This can happen only when the lport name is used by ACL match
> > -     *    condition, which is specified by user. Even in that case, if
> the port
> > -     *    is actually bound on the current chassis it will trigger
> recompute on
> > -     *    that chassis since ovs interface would be updated. So the only
> > -     *    situation this would have real impact is when user defines an
> ACL
> > -     *    that includes lport that is not on current chassis, and there
> is a
> > -     *    port-binding creation/deletion related to that lport.e.g.: an
> ACL is
> > -     *    defined:
> > -     *
> > -     *    to-lport 1000 'outport=="A" && inport=="B"' allow-related
> > -     *
> > -     *    If "A" is on current chassis, but "B" is lport that hasn't
> been
> > -     *    created yet. When a lport "B" is created and bound on another
> > -     *    chassis, the ACL will not take effect on the current chassis
> until a
> > -     *    recompute is triggered later. This case doesn't seem to be a
> problem
> > -     *    for real world use cases because usually lport is created
> before
> > -     *    being referenced by name in ACLs.
> > -     *
> > -     *  - When is_chassis_resident(<lport>) is used in lflow. In this
> case the
> > -     *    port binding is not a regular VIF. It can be either "patch" or
> > -     *    "external", with ha-chassis-group assigned.  In current
> > -     *    "runtime_data" handling, port-binding changes for these types
> always
> > -     *    trigger recomputing. So it is fine even if we do not handle it
> here.
> > -     *    (due to the ovsdb tracking support for referenced table
> changes,
> > -     *    ha-chassis-group changes will appear as port-binding change).
> > -     *
> > -     *  - When a mac-binding doesn't change but the port-binding related
> to
> > -     *    that mac-binding is deleted. In this case the neighbor flow
> generated
> > -     *    for the mac-binding should be deleted. This would not cause
> any real
> > -     *    issue for now, since the port-binding related to mac-binding
> is
> > -     *    always logical router port, and any change to logical router
> port
> > -     *    would just trigger recompute.
> > -     *
> > -     * Although there is no correctness issue so far (except the unusual
> ACL
> > -     * use case, which doesn't seem to be a real problem), it might be
> better
> > -     * to handle this more gracefully, without the need to consider
> these
> > -     * tricky scenarios.  One approach is to maintain a mapping between
> lport
> > -     * names and the lflows that uses them, and reprocess the related
> lflows
> > -     * when related port-bindings change.
> > -     */
> >      struct physical_ctx p_ctx;
> >      init_physical_ctx(node, rt_data, &p_ctx);
> >
> > +    if (!lflow_evaluate_pb_changes(p_ctx.port_binding_table)) {
> > +        /* If this returns false, it means there is an impact on
> > +         * the logical flow processing because of some changes in
> > +         * port bindings. Return false so that recompute is triggered
> > +         * for this stage. */
> > +        return false;
> > +    }
> > +
> > +    /* 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);
> > @@ -1786,6 +1759,8 @@ _flow_output_resource_ref_handler(struct
> engine_node *node, void *data,
> >              updated = &pg_data->updated;
> >              deleted = &pg_data->deleted;
> >              break;
> > +
> > +        case REF_TYPE_PORTBINDING:
> >          default:
> >              OVS_NOT_REACHED();
> >      }
> > @@ -1909,16 +1884,52 @@ flow_output_runtime_data_handler(struct
> engine_node *node,
> >          return false;
> >      }
> >
> > -    if (!hmap_is_empty(&tracked_data->tracked_dp_bindings)) {
> > -        /* We are not yet handling the tracked datapath binding
> > -         * changes. Return false to trigger full recompute. */
> > -        return false;
> > +    if (hmap_is_empty(&tracked_data->tracked_dp_bindings)) {
> > +        if (tracked_data->local_lports_changed) {
> > +            engine_set_node_state(node, EN_UPDATED);
> > +        }
> > +        return true;
> >      }
> >
> > -    if (tracked_data->local_lports_changed) {
> > +    struct lflow_ctx_in l_ctx_in;
> > +    struct lflow_ctx_out l_ctx_out;
> > +    struct ed_type_flow_output *fo = data;
> > +    struct ed_type_runtime_data *rt_data =
> > +        engine_get_input_data("runtime_data", node);
> > +    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);
> > +
> > +    bool handled = true;
> > +    struct tracked_binding_datapath *tdp;
> > +    HMAP_FOR_EACH (tdp, node, &tracked_data->tracked_dp_bindings) {
> > +        if (tdp->is_new) {
> > +            handled = lflow_add_flows_for_datapath(tdp->dp, &l_ctx_in,
> > +                                                   &l_ctx_out);
> > +            if (!handled) {
> > +                break;
> > +            }
> > +        } else {
> > +            struct tracked_binding_lport *lport;
> > +            LIST_FOR_EACH (lport, list_node, &tdp->lports_head) {
> > +                if (!lflow_handle_flows_for_lport(lport->pb, &l_ctx_in,
> > +                                                  &l_ctx_out)) {
> > +                    handled = false;
> > +                    break;
> > +                }
> > +            }
> > +            if (!handled) {
> > +                break;
> > +            }
> > +        }
> > +    }
> > +
> > +    if (handled) {
> >          engine_set_node_state(node, EN_UPDATED);
> >      }
> > -    return true;
> > +
> > +    return handled;
> >  }
> >
> >  struct ovn_controller_exit_args {
> > @@ -1976,6 +1987,9 @@ main(int argc, char *argv[])
> >          = chassis_index_create(ovnsb_idl_loop.idl);
> >      struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath
> >          = mcast_group_index_create(ovnsb_idl_loop.idl);
> > +    struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath
> > +        = ovsdb_idl_index_create1(ovnsb_idl_loop.idl,
> > +
>  &sbrec_logical_flow_col_logical_datapath);
> >      struct ovsdb_idl_index *sbrec_port_binding_by_name
> >          = ovsdb_idl_index_create1(ovnsb_idl_loop.idl,
> >                                    &sbrec_port_binding_col_logical_port);
> > @@ -2125,6 +2139,8 @@ main(int argc, char *argv[])
> >      engine_ovsdb_node_add_index(&en_sb_chassis, "name",
> sbrec_chassis_by_name);
> >      engine_ovsdb_node_add_index(&en_sb_multicast_group, "name_datapath",
> >                                  sbrec_multicast_group_by_name_datapath);
> > +    engine_ovsdb_node_add_index(&en_sb_logical_flow, "logical_datapath",
> > +                                sbrec_logical_flow_by_logical_datapath);
> >      engine_ovsdb_node_add_index(&en_sb_port_binding, "name",
> >                                  sbrec_port_binding_by_name);
> >      engine_ovsdb_node_add_index(&en_sb_port_binding, "key",
> > diff --git a/controller/physical.h b/controller/physical.h
> > index 9ca34436a..481f03901 100644
> > --- a/controller/physical.h
> > +++ b/controller/physical.h
> > @@ -33,6 +33,7 @@ struct ovsrec_bridge;
> >  struct simap;
> >  struct sbrec_multicast_group_table;
> >  struct sbrec_port_binding_table;
> > +struct sbrec_port_binding;
> >  struct sset;
> >
> >  /* OVN Geneve option information.
> > @@ -61,7 +62,7 @@ struct physical_ctx {
> >  void physical_register_ovs_idl(struct ovsdb_idl *);
> >  void physical_run(struct physical_ctx *,
> >                    struct ovn_desired_flow_table *);
> > -void physical_handle_port_binding_changes(struct physical_ctx *,
> > +void physical_handle_port_binding_changes(struct physical_ctx *p_ctx,
> >                                            struct ovn_desired_flow_table
> *);
> >  void physical_handle_mc_group_changes(struct physical_ctx *,
> >                                        struct ovn_desired_flow_table *);
> > diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at
> > index 6e064e64f..a12757e18 100644
> > --- a/tests/ovn-performance.at
> > +++ b/tests/ovn-performance.at
> > @@ -353,8 +353,8 @@ for i in 1 2; do
> >      )
> >
> >      # Bind port $lp and wait for it to come up
> > -    OVN_CONTROLLER_EXPECT_HIT_COND(
> > -        [hv$i hv$j], [lflow_run], [>0 =0],
> > +    OVN_CONTROLLER_EXPECT_NO_HIT(
> > +        [hv$i hv$j], [lflow_run],
> >          [as hv$i ovs-vsctl add-port br-int $vif -- set Interface $vif
> external-ids:iface-id=$lp &&
> >           ovn-nbctl wait-until Logical_Switch_Port $lp 'up=true' &&
> >           ovn-nbctl --wait=hv sync]
> > --
> > 2.26.2
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>

On Tue, May 26, 2020 at 6:57 AM Numan Siddique <numans@ovn.org> wrote:
>
>
>
> On Thu, May 21, 2020 at 12:42 PM Han Zhou <hzhou@ovn.org> wrote:
>>
>> On Wed, May 20, 2020 at 12:40 PM <numans@ovn.org> wrote:
>> >
>> > From: Venkata Anil <anilvenkata@redhat.com>
>> >
>> > This patch processes the logical flows of tracked datapaths
>> > and tracked logical ports. To handle the tracked logical port
>> > changes, reference of logical flows to port bindings is maintained.
>> >
>> > Acked-by: Mark Michelson <mmichels@redhat.com>
>> > Co-Authored-by: Numan Siddique <numans@ovn.org>
>> > Signed-off-by: Venkata Anil <anilvenkata@redhat.com>
>> > Signed-off-by: Numan Siddique <numans@ovn.org>
>> > ---
>> >  controller/lflow.c          |  82 +++++++++++++++++++++++-
>> >  controller/lflow.h          |  14 +++-
>> >  controller/ovn-controller.c | 124 ++++++++++++++++++++----------------
>> >  controller/physical.h       |   3 +-
>> >  tests/ovn-performance.at    |   4 +-
>> >  5 files changed, 168 insertions(+), 59 deletions(-)
>> >
>> > diff --git a/controller/lflow.c b/controller/lflow.c
>> > index 01214a3a6..45bf4aa4b 100644
>> > --- a/controller/lflow.c
>> > +++ b/controller/lflow.c
>> > @@ -258,7 +258,7 @@ lflow_resource_destroy_lflow(struct
>> lflow_resource_ref *lfrr,
>> >  /* Adds the logical flows from the Logical_Flow table to flow tables.
*/
>> >  static void
>> >  add_logical_flows(struct lflow_ctx_in *l_ctx_in,
>> > -                  struct lflow_ctx_out *l_ctx_out)
>> > +                        struct lflow_ctx_out *l_ctx_out)
>> >  {
>> >      const struct sbrec_logical_flow *lflow;
>> >
>> > @@ -649,6 +649,8 @@ consider_logical_flow(const struct
sbrec_logical_flow
>> *lflow,
>> >                  int64_t dp_id = lflow->logical_datapath->tunnel_key;
>> >                  char buf[16];
>> >                  snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, dp_id,
>> port_id);
>> > +                lflow_resource_add(l_ctx_out->lfrr,
>> REF_TYPE_PORTBINDING, buf,
>> > +                                   &lflow->header_.uuid);
>> >                  if (!sset_contains(l_ctx_in->local_lport_ids, buf)) {
>> >                      VLOG_DBG("lflow "UUID_FMT
>> >                               " port %s in match is not local, skip",
>> > @@ -847,3 +849,81 @@ lflow_destroy(void)
>> >      expr_symtab_destroy(&symtab);
>> >      shash_destroy(&symtab);
>> >  }
>> > +
>> > +bool
>> > +lflow_evaluate_pb_changes(const struct sbrec_port_binding_table
>> *pb_table)
>> > +{
>> > +    const struct sbrec_port_binding *binding;
>> > +    SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (binding, pb_table) {
>> > +        if (!strcmp(binding->type, "remote")) {
>>
>> "remote" can be handled just like any regular VIF, just that it would
never
>> be bound on the current chassis.
>
>
> I didn't handle this comment in v8 as I was not to look into the remote
code again
> to understand a bit.
>
> The present approach is a bit inefficient because it would resort to full
recompute when
> any port bindings of type remote change. Are you fine If I handle this in
next patch
> series which I've started working on ?
>
Hi Numan,

I think it would be better to address it in this series because "remote"
port binding changes were handled incrementally even before this series.
Without this, any remote port changes will trigger recompute on all HVs in
all AZs. It is in fact the "easy" part because we are sure that "remote" is
never going to be bound or unbound from current chassis.

> If I understand correctly from the comment, if any remote port binding
changes,
> we need to add this in the tracked_dp_bindings ? so that
lflow_handle_flows_for_lport()
> will handle this ?

I assume that we don't need to handle anything in extra, just keeping the
original change-handler for port-bindings in flow_output. What do you think?

>
> Thanks
> Numan
>
>>
>> > +            return false;
>> > +        }
>> > +    }
>> > +
>> > +    return true;
>> > +}
>> > +
>> > +bool
>> > +lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp,
>> > +                             struct lflow_ctx_in *l_ctx_in,
>> > +                             struct lflow_ctx_out *l_ctx_out)
>> > +{
>> > +    bool handled = true;
>> > +    struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
>> > +    struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts);
>> > +    const struct sbrec_dhcp_options *dhcp_opt_row;
>> > +    SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row,
>> > +                                       l_ctx_in->dhcp_options_table) {
>> > +        dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name,
dhcp_opt_row->code,
>> > +                     dhcp_opt_row->type);
>> > +    }
>> > +
>> > +
>> > +    const struct sbrec_dhcpv6_options *dhcpv6_opt_row;
>> > +    SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row,
>> > +
l_ctx_in->dhcpv6_options_table)
>> {
>> > +       dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name,
>> dhcpv6_opt_row->code,
>> > +                    dhcpv6_opt_row->type);
>> > +    }
>> > +
>> > +    struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts);
>> > +    nd_ra_opts_init(&nd_ra_opts);
>> > +
>> > +    struct controller_event_options controller_event_opts;
>> > +    controller_event_opts_init(&controller_event_opts);
>> > +
>> > +    struct sbrec_logical_flow *lf_row =
>> sbrec_logical_flow_index_init_row(
>> > +        l_ctx_in->sbrec_logical_flow_by_logical_datapath);
>> > +    sbrec_logical_flow_index_set_logical_datapath(lf_row, dp);
>> > +
>> > +    const struct sbrec_logical_flow *lflow;
>> > +    SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL (
>> > +        lflow, lf_row,
l_ctx_in->sbrec_logical_flow_by_logical_datapath)
>> {
>> > +        if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
>> > +                                   &nd_ra_opts,
&controller_event_opts,
>> > +                                   l_ctx_in, l_ctx_out)) {
>> > +            handled = false;
>> > +            break;
>> > +        }
>> > +    }
>> > +
>> > +    dhcp_opts_destroy(&dhcp_opts);
>> > +    dhcp_opts_destroy(&dhcpv6_opts);
>> > +    nd_ra_opts_destroy(&nd_ra_opts);
>> > +    controller_event_opts_destroy(&controller_event_opts);
>> > +    return handled;
>> > +}
>> > +
>> > +bool
>> > +lflow_handle_flows_for_lport(const struct sbrec_port_binding *pb,
>> > +                             struct lflow_ctx_in *l_ctx_in,
>> > +                             struct lflow_ctx_out *l_ctx_out)
>> > +{
>> > +    int64_t dp_id = pb->datapath->tunnel_key;
>> > +    char pb_ref_name[16];
>> > +    snprintf(pb_ref_name, sizeof(pb_ref_name), "%"PRId64"_%"PRId64,
>> > +             dp_id, pb->tunnel_key);
>> > +    bool changed = true;
>> > +    return lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb_ref_name,
>> > +                                    l_ctx_in, l_ctx_out, &changed);
>> > +}
>> > diff --git a/controller/lflow.h b/controller/lflow.h
>> > index f02f709d7..00c1b5c47 100644
>> > --- a/controller/lflow.h
>> > +++ b/controller/lflow.h
>> > @@ -48,6 +48,9 @@ struct sbrec_dhcp_options_table;
>> >  struct sbrec_dhcpv6_options_table;
>> >  struct sbrec_logical_flow_table;
>> >  struct sbrec_mac_binding_table;
>> > +struct sbrec_port_binding_table;
>> > +struct sbrec_datapath_binding;
>> > +struct sbrec_port_binding;
>> >  struct simap;
>> >  struct sset;
>> >  struct uuid;
>> > @@ -72,7 +75,8 @@ struct uuid;
>> >
>> >  enum ref_type {
>> >      REF_TYPE_ADDRSET,
>> > -    REF_TYPE_PORTGROUP
>> > +    REF_TYPE_PORTGROUP,
>> > +    REF_TYPE_PORTBINDING
>> >  };
>> >
>> >  /* Maintains the relationship for a pair of named resource and
>> > @@ -117,6 +121,7 @@ void lflow_resource_clear(struct lflow_resource_ref
>> *);
>> >
>> >  struct lflow_ctx_in {
>> >      struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath;
>> > +    struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath;
>> >      struct ovsdb_idl_index *sbrec_port_binding_by_name;
>> >      const struct sbrec_dhcp_options_table *dhcp_options_table;
>> >      const struct sbrec_dhcpv6_options_table *dhcpv6_options_table;
>> > @@ -157,4 +162,11 @@ void lflow_destroy(void);
>> >
>> >  void lflow_expr_destroy(struct hmap *lflow_expr_cache);
>> >
>> > +bool lflow_add_flows_for_datapath(const struct sbrec_datapath_binding
*,
>> > +                                  struct lflow_ctx_in *,
>> > +                                  struct lflow_ctx_out *);
>> > +bool lflow_handle_flows_for_lport(const struct sbrec_port_binding *,
>> > +                                  struct lflow_ctx_in *,
>> > +                                  struct lflow_ctx_out *);
>> > +bool lflow_evaluate_pb_changes(const struct sbrec_port_binding_table
*);
>> >  #endif /* controller/lflow.h */
>> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> > index dc790f0f7..e53f750bf 100644
>> > --- a/controller/ovn-controller.c
>> > +++ b/controller/ovn-controller.c
>> > @@ -1440,6 +1440,11 @@ static void init_lflow_ctx(struct engine_node
>> *node,
>> >                  engine_get_input("SB_port_binding", node),
>> >                  "name");
>> >
>> > +    struct ovsdb_idl_index *sbrec_logical_flow_by_dp =
>> > +        engine_ovsdb_node_get_index(
>> > +                engine_get_input("SB_logical_flow", node),
>> > +                "logical_datapath");
>> > +
>> >      struct ovsdb_idl_index *sbrec_mc_group_by_name_dp =
>> >          engine_ovsdb_node_get_index(
>> >                  engine_get_input("SB_multicast_group", node),
>> > @@ -1487,6 +1492,8 @@ static void init_lflow_ctx(struct engine_node
*node,
>> >
>> >      l_ctx_in->sbrec_multicast_group_by_name_datapath =
>> >          sbrec_mc_group_by_name_dp;
>> > +    l_ctx_in->sbrec_logical_flow_by_logical_datapath =
>> > +        sbrec_logical_flow_by_dp;
>> >      l_ctx_in->sbrec_port_binding_by_name = sbrec_port_binding_by_name;
>> >      l_ctx_in->dhcp_options_table  = dhcp_table;
>> >      l_ctx_in->dhcpv6_options_table = dhcpv6_table;
>> > @@ -1641,7 +1648,8 @@ flow_output_sb_mac_binding_handler(struct
>> engine_node *node, void *data)
>> >  }
>> >
>> >  static bool
>> > -flow_output_sb_port_binding_handler(struct engine_node *node, void
*data)
>> > +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);
>> > @@ -1649,56 +1657,21 @@ flow_output_sb_port_binding_handler(struct
>> engine_node *node, void *data)
>> >      struct ed_type_flow_output *fo = data;
>> >      struct ovn_desired_flow_table *flow_table = &fo->flow_table;
>> >
>> > -    /* XXX: now we handle port-binding changes for physical flow
>> processing
>> > -     * only, but port-binding change can have impact to logical flow
>> > -     * processing, too, in below circumstances:
>> > -     *
>> > -     *  - When a port-binding for a lport is inserted/deleted but the
>> lflow
>> > -     *    using that lport doesn't change.
>> > -     *
>> > -     *    This can happen only when the lport name is used by ACL
match
>> > -     *    condition, which is specified by user. Even in that case, if
>> the port
>> > -     *    is actually bound on the current chassis it will trigger
>> recompute on
>> > -     *    that chassis since ovs interface would be updated. So the
only
>> > -     *    situation this would have real impact is when user defines
an
>> ACL
>> > -     *    that includes lport that is not on current chassis, and
there
>> is a
>> > -     *    port-binding creation/deletion related to that lport.e.g.:
an
>> ACL is
>> > -     *    defined:
>> > -     *
>> > -     *    to-lport 1000 'outport=="A" && inport=="B"' allow-related
>> > -     *
>> > -     *    If "A" is on current chassis, but "B" is lport that hasn't
been
>> > -     *    created yet. When a lport "B" is created and bound on
another
>> > -     *    chassis, the ACL will not take effect on the current chassis
>> until a
>> > -     *    recompute is triggered later. This case doesn't seem to be a
>> problem
>> > -     *    for real world use cases because usually lport is created
>> before
>> > -     *    being referenced by name in ACLs.
>> > -     *
>> > -     *  - When is_chassis_resident(<lport>) is used in lflow. In this
>> case the
>> > -     *    port binding is not a regular VIF. It can be either "patch"
or
>> > -     *    "external", with ha-chassis-group assigned.  In current
>> > -     *    "runtime_data" handling, port-binding changes for these
types
>> always
>> > -     *    trigger recomputing. So it is fine even if we do not handle
it
>> here.
>> > -     *    (due to the ovsdb tracking support for referenced table
>> changes,
>> > -     *    ha-chassis-group changes will appear as port-binding
change).
>> > -     *
>> > -     *  - When a mac-binding doesn't change but the port-binding
related
>> to
>> > -     *    that mac-binding is deleted. In this case the neighbor flow
>> generated
>> > -     *    for the mac-binding should be deleted. This would not cause
>> any real
>> > -     *    issue for now, since the port-binding related to
mac-binding is
>> > -     *    always logical router port, and any change to logical router
>> port
>> > -     *    would just trigger recompute.
>> > -     *
>> > -     * Although there is no correctness issue so far (except the
unusual
>> ACL
>> > -     * use case, which doesn't seem to be a real problem), it might be
>> better
>> > -     * to handle this more gracefully, without the need to consider
these
>> > -     * tricky scenarios.  One approach is to maintain a mapping
between
>> lport
>> > -     * names and the lflows that uses them, and reprocess the related
>> lflows
>> > -     * when related port-bindings change.
>> > -     */
>> >      struct physical_ctx p_ctx;
>> >      init_physical_ctx(node, rt_data, &p_ctx);
>> >
>> > +    if (!lflow_evaluate_pb_changes(p_ctx.port_binding_table)) {
>> > +        /* If this returns false, it means there is an impact on
>> > +         * the logical flow processing because of some changes in
>> > +         * port bindings. Return false so that recompute is triggered
>> > +         * for this stage. */
>> > +        return false;
>> > +    }
>> > +
>> > +    /* 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);
>> > @@ -1786,6 +1759,8 @@ _flow_output_resource_ref_handler(struct
>> engine_node *node, void *data,
>> >              updated = &pg_data->updated;
>> >              deleted = &pg_data->deleted;
>> >              break;
>> > +
>> > +        case REF_TYPE_PORTBINDING:
>> >          default:
>> >              OVS_NOT_REACHED();
>> >      }
>> > @@ -1909,16 +1884,52 @@ flow_output_runtime_data_handler(struct
>> engine_node *node,
>> >          return false;
>> >      }
>> >
>> > -    if (!hmap_is_empty(&tracked_data->tracked_dp_bindings)) {
>> > -        /* We are not yet handling the tracked datapath binding
>> > -         * changes. Return false to trigger full recompute. */
>> > -        return false;
>> > +    if (hmap_is_empty(&tracked_data->tracked_dp_bindings)) {
>> > +        if (tracked_data->local_lports_changed) {
>> > +            engine_set_node_state(node, EN_UPDATED);
>> > +        }
>> > +        return true;
>> >      }
>> >
>> > -    if (tracked_data->local_lports_changed) {
>> > +    struct lflow_ctx_in l_ctx_in;
>> > +    struct lflow_ctx_out l_ctx_out;
>> > +    struct ed_type_flow_output *fo = data;
>> > +    struct ed_type_runtime_data *rt_data =
>> > +        engine_get_input_data("runtime_data", node);
>> > +    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);
>> > +
>> > +    bool handled = true;
>> > +    struct tracked_binding_datapath *tdp;
>> > +    HMAP_FOR_EACH (tdp, node, &tracked_data->tracked_dp_bindings) {
>> > +        if (tdp->is_new) {
>> > +            handled = lflow_add_flows_for_datapath(tdp->dp, &l_ctx_in,
>> > +                                                   &l_ctx_out);
>> > +            if (!handled) {
>> > +                break;
>> > +            }
>> > +        } else {
>> > +            struct tracked_binding_lport *lport;
>> > +            LIST_FOR_EACH (lport, list_node, &tdp->lports_head) {
>> > +                if (!lflow_handle_flows_for_lport(lport->pb,
&l_ctx_in,
>> > +                                                  &l_ctx_out)) {
>> > +                    handled = false;
>> > +                    break;
>> > +                }
>> > +            }
>> > +            if (!handled) {
>> > +                break;
>> > +            }
>> > +        }
>> > +    }
>> > +
>> > +    if (handled) {
>> >          engine_set_node_state(node, EN_UPDATED);
>> >      }
>> > -    return true;
>> > +
>> > +    return handled;
>> >  }
>> >
>> >  struct ovn_controller_exit_args {
>> > @@ -1976,6 +1987,9 @@ main(int argc, char *argv[])
>> >          = chassis_index_create(ovnsb_idl_loop.idl);
>> >      struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath
>> >          = mcast_group_index_create(ovnsb_idl_loop.idl);
>> > +    struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath
>> > +        = ovsdb_idl_index_create1(ovnsb_idl_loop.idl,
>> > +
>>  &sbrec_logical_flow_col_logical_datapath);
>> >      struct ovsdb_idl_index *sbrec_port_binding_by_name
>> >          = ovsdb_idl_index_create1(ovnsb_idl_loop.idl,
>> >
 &sbrec_port_binding_col_logical_port);
>> > @@ -2125,6 +2139,8 @@ main(int argc, char *argv[])
>> >      engine_ovsdb_node_add_index(&en_sb_chassis, "name",
>> sbrec_chassis_by_name);
>> >      engine_ovsdb_node_add_index(&en_sb_multicast_group,
"name_datapath",
>> >
 sbrec_multicast_group_by_name_datapath);
>> > +    engine_ovsdb_node_add_index(&en_sb_logical_flow,
"logical_datapath",
>> > +
 sbrec_logical_flow_by_logical_datapath);
>> >      engine_ovsdb_node_add_index(&en_sb_port_binding, "name",
>> >                                  sbrec_port_binding_by_name);
>> >      engine_ovsdb_node_add_index(&en_sb_port_binding, "key",
>> > diff --git a/controller/physical.h b/controller/physical.h
>> > index 9ca34436a..481f03901 100644
>> > --- a/controller/physical.h
>> > +++ b/controller/physical.h
>> > @@ -33,6 +33,7 @@ struct ovsrec_bridge;
>> >  struct simap;
>> >  struct sbrec_multicast_group_table;
>> >  struct sbrec_port_binding_table;
>> > +struct sbrec_port_binding;
>> >  struct sset;
>> >
>> >  /* OVN Geneve option information.
>> > @@ -61,7 +62,7 @@ struct physical_ctx {
>> >  void physical_register_ovs_idl(struct ovsdb_idl *);
>> >  void physical_run(struct physical_ctx *,
>> >                    struct ovn_desired_flow_table *);
>> > -void physical_handle_port_binding_changes(struct physical_ctx *,
>> > +void physical_handle_port_binding_changes(struct physical_ctx *p_ctx,
>> >                                            struct
ovn_desired_flow_table
>> *);
>> >  void physical_handle_mc_group_changes(struct physical_ctx *,
>> >                                        struct ovn_desired_flow_table
*);
>> > diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at
>> > index 6e064e64f..a12757e18 100644
>> > --- a/tests/ovn-performance.at
>> > +++ b/tests/ovn-performance.at
>> > @@ -353,8 +353,8 @@ for i in 1 2; do
>> >      )
>> >
>> >      # Bind port $lp and wait for it to come up
>> > -    OVN_CONTROLLER_EXPECT_HIT_COND(
>> > -        [hv$i hv$j], [lflow_run], [>0 =0],
>> > +    OVN_CONTROLLER_EXPECT_NO_HIT(
>> > +        [hv$i hv$j], [lflow_run],
>> >          [as hv$i ovs-vsctl add-port br-int $vif -- set Interface $vif
>> external-ids:iface-id=$lp &&
>> >           ovn-nbctl wait-until Logical_Switch_Port $lp 'up=true' &&
>> >           ovn-nbctl --wait=hv sync]
>> > --
>> > 2.26.2
>> >
>> > _______________________________________________
>> > dev mailing list
>> > dev@openvswitch.org
>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>

On Wed, May 27, 2020 at 3:30 AM Han Zhou <hzhou@ovn.org> wrote:

> On Tue, May 26, 2020 at 6:57 AM Numan Siddique <numans@ovn.org> wrote:
> >
> >
> >
> > On Thu, May 21, 2020 at 12:42 PM Han Zhou <hzhou@ovn.org> wrote:
> >>
> >> On Wed, May 20, 2020 at 12:40 PM <numans@ovn.org> wrote:
> >> >
> >> > From: Venkata Anil <anilvenkata@redhat.com>
> >> >
> >> > This patch processes the logical flows of tracked datapaths
> >> > and tracked logical ports. To handle the tracked logical port
> >> > changes, reference of logical flows to port bindings is maintained.
> >> >
> >> > Acked-by: Mark Michelson <mmichels@redhat.com>
> >> > Co-Authored-by: Numan Siddique <numans@ovn.org>
> >> > Signed-off-by: Venkata Anil <anilvenkata@redhat.com>
> >> > Signed-off-by: Numan Siddique <numans@ovn.org>
> >> > ---
> >> >  controller/lflow.c          |  82 +++++++++++++++++++++++-
> >> >  controller/lflow.h          |  14 +++-
> >> >  controller/ovn-controller.c | 124
> ++++++++++++++++++++----------------
> >> >  controller/physical.h       |   3 +-
> >> >  tests/ovn-performance.at    |   4 +-
> >> >  5 files changed, 168 insertions(+), 59 deletions(-)
> >> >
> >> > diff --git a/controller/lflow.c b/controller/lflow.c
> >> > index 01214a3a6..45bf4aa4b 100644
> >> > --- a/controller/lflow.c
> >> > +++ b/controller/lflow.c
> >> > @@ -258,7 +258,7 @@ lflow_resource_destroy_lflow(struct
> >> lflow_resource_ref *lfrr,
> >> >  /* Adds the logical flows from the Logical_Flow table to flow tables.
> */
> >> >  static void
> >> >  add_logical_flows(struct lflow_ctx_in *l_ctx_in,
> >> > -                  struct lflow_ctx_out *l_ctx_out)
> >> > +                        struct lflow_ctx_out *l_ctx_out)
> >> >  {
> >> >      const struct sbrec_logical_flow *lflow;
> >> >
> >> > @@ -649,6 +649,8 @@ consider_logical_flow(const struct
> sbrec_logical_flow
> >> *lflow,
> >> >                  int64_t dp_id = lflow->logical_datapath->tunnel_key;
> >> >                  char buf[16];
> >> >                  snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64,
> dp_id,
> >> port_id);
> >> > +                lflow_resource_add(l_ctx_out->lfrr,
> >> REF_TYPE_PORTBINDING, buf,
> >> > +                                   &lflow->header_.uuid);
> >> >                  if (!sset_contains(l_ctx_in->local_lport_ids, buf)) {
> >> >                      VLOG_DBG("lflow "UUID_FMT
> >> >                               " port %s in match is not local, skip",
> >> > @@ -847,3 +849,81 @@ lflow_destroy(void)
> >> >      expr_symtab_destroy(&symtab);
> >> >      shash_destroy(&symtab);
> >> >  }
> >> > +
> >> > +bool
> >> > +lflow_evaluate_pb_changes(const struct sbrec_port_binding_table
> >> *pb_table)
> >> > +{
> >> > +    const struct sbrec_port_binding *binding;
> >> > +    SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (binding, pb_table) {
> >> > +        if (!strcmp(binding->type, "remote")) {
> >>
> >> "remote" can be handled just like any regular VIF, just that it would
> never
> >> be bound on the current chassis.
> >
> >
> > I didn't handle this comment in v8 as I was not to look into the remote
> code again
> > to understand a bit.
> >
> > The present approach is a bit inefficient because it would resort to full
> recompute when
> > any port bindings of type remote change. Are you fine If I handle this in
> next patch
> > series which I've started working on ?
> >
> Hi Numan,
>
> I think it would be better to address it in this series because "remote"
> port binding changes were handled incrementally even before this series.
> Without this, any remote port changes will trigger recompute on all HVs in
> all AZs. It is in fact the "easy" part because we are sure that "remote" is
> never going to be bound or unbound from current chassis.
>

Thanks. It would be wrong to make it non I-P now. I'll address it in v9 and
submit soon.

Thanks
Numan


>
> > If I understand correctly from the comment, if any remote port binding
> changes,
> > we need to add this in the tracked_dp_bindings ? so that
> lflow_handle_flows_for_lport()
> > will handle this ?
>
> I assume that we don't need to handle anything in extra, just keeping the
> original change-handler for port-bindings in flow_output. What do you
> think?
>
> >
> > Thanks
> > Numan
> >
> >>
> >> > +            return false;
> >> > +        }
> >> > +    }
> >> > +
> >> > +    return true;
> >> > +}
> >> > +
> >> > +bool
> >> > +lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp,
> >> > +                             struct lflow_ctx_in *l_ctx_in,
> >> > +                             struct lflow_ctx_out *l_ctx_out)
> >> > +{
> >> > +    bool handled = true;
> >> > +    struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
> >> > +    struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts);
> >> > +    const struct sbrec_dhcp_options *dhcp_opt_row;
> >> > +    SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row,
> >> > +                                       l_ctx_in->dhcp_options_table)
> {
> >> > +        dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name,
> dhcp_opt_row->code,
> >> > +                     dhcp_opt_row->type);
> >> > +    }
> >> > +
> >> > +
> >> > +    const struct sbrec_dhcpv6_options *dhcpv6_opt_row;
> >> > +    SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row,
> >> > +
> l_ctx_in->dhcpv6_options_table)
> >> {
> >> > +       dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name,
> >> dhcpv6_opt_row->code,
> >> > +                    dhcpv6_opt_row->type);
> >> > +    }
> >> > +
> >> > +    struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts);
> >> > +    nd_ra_opts_init(&nd_ra_opts);
> >> > +
> >> > +    struct controller_event_options controller_event_opts;
> >> > +    controller_event_opts_init(&controller_event_opts);
> >> > +
> >> > +    struct sbrec_logical_flow *lf_row =
> >> sbrec_logical_flow_index_init_row(
> >> > +        l_ctx_in->sbrec_logical_flow_by_logical_datapath);
> >> > +    sbrec_logical_flow_index_set_logical_datapath(lf_row, dp);
> >> > +
> >> > +    const struct sbrec_logical_flow *lflow;
> >> > +    SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL (
> >> > +        lflow, lf_row,
> l_ctx_in->sbrec_logical_flow_by_logical_datapath)
> >> {
> >> > +        if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> >> > +                                   &nd_ra_opts,
> &controller_event_opts,
> >> > +                                   l_ctx_in, l_ctx_out)) {
> >> > +            handled = false;
> >> > +            break;
> >> > +        }
> >> > +    }
> >> > +
> >> > +    dhcp_opts_destroy(&dhcp_opts);
> >> > +    dhcp_opts_destroy(&dhcpv6_opts);
> >> > +    nd_ra_opts_destroy(&nd_ra_opts);
> >> > +    controller_event_opts_destroy(&controller_event_opts);
> >> > +    return handled;
> >> > +}
> >> > +
> >> > +bool
> >> > +lflow_handle_flows_for_lport(const struct sbrec_port_binding *pb,
> >> > +                             struct lflow_ctx_in *l_ctx_in,
> >> > +                             struct lflow_ctx_out *l_ctx_out)
> >> > +{
> >> > +    int64_t dp_id = pb->datapath->tunnel_key;
> >> > +    char pb_ref_name[16];
> >> > +    snprintf(pb_ref_name, sizeof(pb_ref_name), "%"PRId64"_%"PRId64,
> >> > +             dp_id, pb->tunnel_key);
> >> > +    bool changed = true;
> >> > +    return lflow_handle_changed_ref(REF_TYPE_PORTBINDING,
> pb_ref_name,
> >> > +                                    l_ctx_in, l_ctx_out, &changed);
> >> > +}
> >> > diff --git a/controller/lflow.h b/controller/lflow.h
> >> > index f02f709d7..00c1b5c47 100644
> >> > --- a/controller/lflow.h
> >> > +++ b/controller/lflow.h
> >> > @@ -48,6 +48,9 @@ struct sbrec_dhcp_options_table;
> >> >  struct sbrec_dhcpv6_options_table;
> >> >  struct sbrec_logical_flow_table;
> >> >  struct sbrec_mac_binding_table;
> >> > +struct sbrec_port_binding_table;
> >> > +struct sbrec_datapath_binding;
> >> > +struct sbrec_port_binding;
> >> >  struct simap;
> >> >  struct sset;
> >> >  struct uuid;
> >> > @@ -72,7 +75,8 @@ struct uuid;
> >> >
> >> >  enum ref_type {
> >> >      REF_TYPE_ADDRSET,
> >> > -    REF_TYPE_PORTGROUP
> >> > +    REF_TYPE_PORTGROUP,
> >> > +    REF_TYPE_PORTBINDING
> >> >  };
> >> >
> >> >  /* Maintains the relationship for a pair of named resource and
> >> > @@ -117,6 +121,7 @@ void lflow_resource_clear(struct
> lflow_resource_ref
> >> *);
> >> >
> >> >  struct lflow_ctx_in {
> >> >      struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath;
> >> > +    struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath;
> >> >      struct ovsdb_idl_index *sbrec_port_binding_by_name;
> >> >      const struct sbrec_dhcp_options_table *dhcp_options_table;
> >> >      const struct sbrec_dhcpv6_options_table *dhcpv6_options_table;
> >> > @@ -157,4 +162,11 @@ void lflow_destroy(void);
> >> >
> >> >  void lflow_expr_destroy(struct hmap *lflow_expr_cache);
> >> >
> >> > +bool lflow_add_flows_for_datapath(const struct sbrec_datapath_binding
> *,
> >> > +                                  struct lflow_ctx_in *,
> >> > +                                  struct lflow_ctx_out *);
> >> > +bool lflow_handle_flows_for_lport(const struct sbrec_port_binding *,
> >> > +                                  struct lflow_ctx_in *,
> >> > +                                  struct lflow_ctx_out *);
> >> > +bool lflow_evaluate_pb_changes(const struct sbrec_port_binding_table
> *);
> >> >  #endif /* controller/lflow.h */
> >> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> >> > index dc790f0f7..e53f750bf 100644
> >> > --- a/controller/ovn-controller.c
> >> > +++ b/controller/ovn-controller.c
> >> > @@ -1440,6 +1440,11 @@ static void init_lflow_ctx(struct engine_node
> >> *node,
> >> >                  engine_get_input("SB_port_binding", node),
> >> >                  "name");
> >> >
> >> > +    struct ovsdb_idl_index *sbrec_logical_flow_by_dp =
> >> > +        engine_ovsdb_node_get_index(
> >> > +                engine_get_input("SB_logical_flow", node),
> >> > +                "logical_datapath");
> >> > +
> >> >      struct ovsdb_idl_index *sbrec_mc_group_by_name_dp =
> >> >          engine_ovsdb_node_get_index(
> >> >                  engine_get_input("SB_multicast_group", node),
> >> > @@ -1487,6 +1492,8 @@ static void init_lflow_ctx(struct engine_node
> *node,
> >> >
> >> >      l_ctx_in->sbrec_multicast_group_by_name_datapath =
> >> >          sbrec_mc_group_by_name_dp;
> >> > +    l_ctx_in->sbrec_logical_flow_by_logical_datapath =
> >> > +        sbrec_logical_flow_by_dp;
> >> >      l_ctx_in->sbrec_port_binding_by_name =
> sbrec_port_binding_by_name;
> >> >      l_ctx_in->dhcp_options_table  = dhcp_table;
> >> >      l_ctx_in->dhcpv6_options_table = dhcpv6_table;
> >> > @@ -1641,7 +1648,8 @@ flow_output_sb_mac_binding_handler(struct
> >> engine_node *node, void *data)
> >> >  }
> >> >
> >> >  static bool
> >> > -flow_output_sb_port_binding_handler(struct engine_node *node, void
> *data)
> >> > +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);
> >> > @@ -1649,56 +1657,21 @@ flow_output_sb_port_binding_handler(struct
> >> engine_node *node, void *data)
> >> >      struct ed_type_flow_output *fo = data;
> >> >      struct ovn_desired_flow_table *flow_table = &fo->flow_table;
> >> >
> >> > -    /* XXX: now we handle port-binding changes for physical flow
> >> processing
> >> > -     * only, but port-binding change can have impact to logical flow
> >> > -     * processing, too, in below circumstances:
> >> > -     *
> >> > -     *  - When a port-binding for a lport is inserted/deleted but the
> >> lflow
> >> > -     *    using that lport doesn't change.
> >> > -     *
> >> > -     *    This can happen only when the lport name is used by ACL
> match
> >> > -     *    condition, which is specified by user. Even in that case,
> if
> >> the port
> >> > -     *    is actually bound on the current chassis it will trigger
> >> recompute on
> >> > -     *    that chassis since ovs interface would be updated. So the
> only
> >> > -     *    situation this would have real impact is when user defines
> an
> >> ACL
> >> > -     *    that includes lport that is not on current chassis, and
> there
> >> is a
> >> > -     *    port-binding creation/deletion related to that lport.e.g.:
> an
> >> ACL is
> >> > -     *    defined:
> >> > -     *
> >> > -     *    to-lport 1000 'outport=="A" && inport=="B"' allow-related
> >> > -     *
> >> > -     *    If "A" is on current chassis, but "B" is lport that hasn't
> been
> >> > -     *    created yet. When a lport "B" is created and bound on
> another
> >> > -     *    chassis, the ACL will not take effect on the current
> chassis
> >> until a
> >> > -     *    recompute is triggered later. This case doesn't seem to be
> a
> >> problem
> >> > -     *    for real world use cases because usually lport is created
> >> before
> >> > -     *    being referenced by name in ACLs.
> >> > -     *
> >> > -     *  - When is_chassis_resident(<lport>) is used in lflow. In this
> >> case the
> >> > -     *    port binding is not a regular VIF. It can be either "patch"
> or
> >> > -     *    "external", with ha-chassis-group assigned.  In current
> >> > -     *    "runtime_data" handling, port-binding changes for these
> types
> >> always
> >> > -     *    trigger recomputing. So it is fine even if we do not handle
> it
> >> here.
> >> > -     *    (due to the ovsdb tracking support for referenced table
> >> changes,
> >> > -     *    ha-chassis-group changes will appear as port-binding
> change).
> >> > -     *
> >> > -     *  - When a mac-binding doesn't change but the port-binding
> related
> >> to
> >> > -     *    that mac-binding is deleted. In this case the neighbor flow
> >> generated
> >> > -     *    for the mac-binding should be deleted. This would not cause
> >> any real
> >> > -     *    issue for now, since the port-binding related to
> mac-binding is
> >> > -     *    always logical router port, and any change to logical
> router
> >> port
> >> > -     *    would just trigger recompute.
> >> > -     *
> >> > -     * Although there is no correctness issue so far (except the
> unusual
> >> ACL
> >> > -     * use case, which doesn't seem to be a real problem), it might
> be
> >> better
> >> > -     * to handle this more gracefully, without the need to consider
> these
> >> > -     * tricky scenarios.  One approach is to maintain a mapping
> between
> >> lport
> >> > -     * names and the lflows that uses them, and reprocess the related
> >> lflows
> >> > -     * when related port-bindings change.
> >> > -     */
> >> >      struct physical_ctx p_ctx;
> >> >      init_physical_ctx(node, rt_data, &p_ctx);
> >> >
> >> > +    if (!lflow_evaluate_pb_changes(p_ctx.port_binding_table)) {
> >> > +        /* If this returns false, it means there is an impact on
> >> > +         * the logical flow processing because of some changes in
> >> > +         * port bindings. Return false so that recompute is triggered
> >> > +         * for this stage. */
> >> > +        return false;
> >> > +    }
> >> > +
> >> > +    /* 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);
> >> > @@ -1786,6 +1759,8 @@ _flow_output_resource_ref_handler(struct
> >> engine_node *node, void *data,
> >> >              updated = &pg_data->updated;
> >> >              deleted = &pg_data->deleted;
> >> >              break;
> >> > +
> >> > +        case REF_TYPE_PORTBINDING:
> >> >          default:
> >> >              OVS_NOT_REACHED();
> >> >      }
> >> > @@ -1909,16 +1884,52 @@ flow_output_runtime_data_handler(struct
> >> engine_node *node,
> >> >          return false;
> >> >      }
> >> >
> >> > -    if (!hmap_is_empty(&tracked_data->tracked_dp_bindings)) {
> >> > -        /* We are not yet handling the tracked datapath binding
> >> > -         * changes. Return false to trigger full recompute. */
> >> > -        return false;
> >> > +    if (hmap_is_empty(&tracked_data->tracked_dp_bindings)) {
> >> > +        if (tracked_data->local_lports_changed) {
> >> > +            engine_set_node_state(node, EN_UPDATED);
> >> > +        }
> >> > +        return true;
> >> >      }
> >> >
> >> > -    if (tracked_data->local_lports_changed) {
> >> > +    struct lflow_ctx_in l_ctx_in;
> >> > +    struct lflow_ctx_out l_ctx_out;
> >> > +    struct ed_type_flow_output *fo = data;
> >> > +    struct ed_type_runtime_data *rt_data =
> >> > +        engine_get_input_data("runtime_data", node);
> >> > +    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);
> >> > +
> >> > +    bool handled = true;
> >> > +    struct tracked_binding_datapath *tdp;
> >> > +    HMAP_FOR_EACH (tdp, node, &tracked_data->tracked_dp_bindings) {
> >> > +        if (tdp->is_new) {
> >> > +            handled = lflow_add_flows_for_datapath(tdp->dp,
> &l_ctx_in,
> >> > +                                                   &l_ctx_out);
> >> > +            if (!handled) {
> >> > +                break;
> >> > +            }
> >> > +        } else {
> >> > +            struct tracked_binding_lport *lport;
> >> > +            LIST_FOR_EACH (lport, list_node, &tdp->lports_head) {
> >> > +                if (!lflow_handle_flows_for_lport(lport->pb,
> &l_ctx_in,
> >> > +                                                  &l_ctx_out)) {
> >> > +                    handled = false;
> >> > +                    break;
> >> > +                }
> >> > +            }
> >> > +            if (!handled) {
> >> > +                break;
> >> > +            }
> >> > +        }
> >> > +    }
> >> > +
> >> > +    if (handled) {
> >> >          engine_set_node_state(node, EN_UPDATED);
> >> >      }
> >> > -    return true;
> >> > +
> >> > +    return handled;
> >> >  }
> >> >
> >> >  struct ovn_controller_exit_args {
> >> > @@ -1976,6 +1987,9 @@ main(int argc, char *argv[])
> >> >          = chassis_index_create(ovnsb_idl_loop.idl);
> >> >      struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath
> >> >          = mcast_group_index_create(ovnsb_idl_loop.idl);
> >> > +    struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath
> >> > +        = ovsdb_idl_index_create1(ovnsb_idl_loop.idl,
> >> > +
> >>  &sbrec_logical_flow_col_logical_datapath);
> >> >      struct ovsdb_idl_index *sbrec_port_binding_by_name
> >> >          = ovsdb_idl_index_create1(ovnsb_idl_loop.idl,
> >> >
>  &sbrec_port_binding_col_logical_port);
> >> > @@ -2125,6 +2139,8 @@ main(int argc, char *argv[])
> >> >      engine_ovsdb_node_add_index(&en_sb_chassis, "name",
> >> sbrec_chassis_by_name);
> >> >      engine_ovsdb_node_add_index(&en_sb_multicast_group,
> "name_datapath",
> >> >
>  sbrec_multicast_group_by_name_datapath);
> >> > +    engine_ovsdb_node_add_index(&en_sb_logical_flow,
> "logical_datapath",
> >> > +
>  sbrec_logical_flow_by_logical_datapath);
> >> >      engine_ovsdb_node_add_index(&en_sb_port_binding, "name",
> >> >                                  sbrec_port_binding_by_name);
> >> >      engine_ovsdb_node_add_index(&en_sb_port_binding, "key",
> >> > diff --git a/controller/physical.h b/controller/physical.h
> >> > index 9ca34436a..481f03901 100644
> >> > --- a/controller/physical.h
> >> > +++ b/controller/physical.h
> >> > @@ -33,6 +33,7 @@ struct ovsrec_bridge;
> >> >  struct simap;
> >> >  struct sbrec_multicast_group_table;
> >> >  struct sbrec_port_binding_table;
> >> > +struct sbrec_port_binding;
> >> >  struct sset;
> >> >
> >> >  /* OVN Geneve option information.
> >> > @@ -61,7 +62,7 @@ struct physical_ctx {
> >> >  void physical_register_ovs_idl(struct ovsdb_idl *);
> >> >  void physical_run(struct physical_ctx *,
> >> >                    struct ovn_desired_flow_table *);
> >> > -void physical_handle_port_binding_changes(struct physical_ctx *,
> >> > +void physical_handle_port_binding_changes(struct physical_ctx *p_ctx,
> >> >                                            struct
> ovn_desired_flow_table
> >> *);
> >> >  void physical_handle_mc_group_changes(struct physical_ctx *,
> >> >                                        struct ovn_desired_flow_table
> *);
> >> > diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at
> >> > index 6e064e64f..a12757e18 100644
> >> > --- a/tests/ovn-performance.at
> >> > +++ b/tests/ovn-performance.at
> >> > @@ -353,8 +353,8 @@ for i in 1 2; do
> >> >      )
> >> >
> >> >      # Bind port $lp and wait for it to come up
> >> > -    OVN_CONTROLLER_EXPECT_HIT_COND(
> >> > -        [hv$i hv$j], [lflow_run], [>0 =0],
> >> > +    OVN_CONTROLLER_EXPECT_NO_HIT(
> >> > +        [hv$i hv$j], [lflow_run],
> >> >          [as hv$i ovs-vsctl add-port br-int $vif -- set Interface $vif
> >> external-ids:iface-id=$lp &&
> >> >           ovn-nbctl wait-until Logical_Switch_Port $lp 'up=true' &&
> >> >           ovn-nbctl --wait=hv sync]
> >> > --
> >> > 2.26.2
> >> >
> >> > _______________________________________________
> >> > dev mailing list
> >> > dev@openvswitch.org
> >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >> _______________________________________________
> >> dev mailing list
> >> dev@openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>