[ovs-dev,v3,3/3] ovn-controller: Fix incremental processing for logical port references.

If a lflow has an lport name in the match, but when the lflow is
processed the port-binding is not seen by ovn-controller, the
corresponding openflow will not be created. Later if the port-binding is
created/monitored by ovn-controller, the lflow is not reprocessed
because the lflow didn't change and ovn-controller doesn't know that the
port-binding affects the lflow. This patch fixes the problem by tracking
the references when parsing the lflow, even if the port-binding is not
found when the lflow is firstly parsed. A test case is also added to
cover the scenario.

Signed-off-by: Han Zhou <hzhou@ovn.org>
---
 controller/lflow.c          | 63 ++++++++++++++++++++++++++-----------
 controller/lflow.h          |  3 ++
 controller/ovn-controller.c | 35 ++++++++++++++++-----
 include/ovn/expr.h          |  2 +-
 lib/expr.c                  | 14 +++------
 tests/ovn.at                | 47 +++++++++++++++++++++++++++
 tests/test-ovn.c            |  4 +--
 utilities/ovn-trace.c       |  2 +-
 8 files changed, 132 insertions(+), 38 deletions(-)

On Mon, Jun 21, 2021 at 2:52 AM Han Zhou <hzhou@ovn.org> wrote:
>
> If a lflow has an lport name in the match, but when the lflow is
> processed the port-binding is not seen by ovn-controller, the
> corresponding openflow will not be created. Later if the port-binding is
> created/monitored by ovn-controller, the lflow is not reprocessed
> because the lflow didn't change and ovn-controller doesn't know that the
> port-binding affects the lflow. This patch fixes the problem by tracking
> the references when parsing the lflow, even if the port-binding is not
> found when the lflow is firstly parsed. A test case is also added to
> cover the scenario.
>
> Signed-off-by: Han Zhou <hzhou@ovn.org>

Hi Han,

Thanks for fixing these issues.   I've a few questions.  I haven't
reviewed the patch completely.


> ---
>  controller/lflow.c          | 63 ++++++++++++++++++++++++++-----------
>  controller/lflow.h          |  3 ++
>  controller/ovn-controller.c | 35 ++++++++++++++++-----
>  include/ovn/expr.h          |  2 +-
>  lib/expr.c                  | 14 +++------
>  tests/ovn.at                | 47 +++++++++++++++++++++++++++
>  tests/test-ovn.c            |  4 +--
>  utilities/ovn-trace.c       |  2 +-
>  8 files changed, 132 insertions(+), 38 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 34eca135a..b7699a309 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -61,6 +61,7 @@ struct lookup_port_aux {
>
>  struct condition_aux {
>      struct ovsdb_idl_index *sbrec_port_binding_by_name;
> +    const struct sbrec_datapath_binding *dp;
>      const struct sbrec_chassis *chassis;
>      const struct sset *active_tunnels;
>      const struct sbrec_logical_flow *lflow;
> @@ -98,6 +99,12 @@ lookup_port_cb(const void *aux_, const char *port_name, unsigned int *portp)
>
>      const struct lookup_port_aux *aux = aux_;
>
> +    /* Store the name that used to lookup the lport to lflow reference, so that
> +     * in the future when the lport's port binding changes, the logical flow
> +     * that references this lport can be reprocessed. */
> +    lflow_resource_add(aux->lfrr, REF_TYPE_PORTBINDING, port_name,
> +                       &aux->lflow->header_.uuid);
> +
>      const struct sbrec_port_binding *pb
>          = lport_lookup_by_name(aux->sbrec_port_binding_by_name, port_name);
>      if (pb && pb->datapath == aux->dp) {
> @@ -149,19 +156,18 @@ is_chassis_resident_cb(const void *c_aux_, const char *port_name)
>  {
>      const struct condition_aux *c_aux = c_aux_;
>
> +    /* Store the port name that used to lookup the lport to lflow reference, so
> +     * that in the future when the lport's port-binding changes the logical
> +     * flow that references this lport can be reprocessed. */
> +    lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, port_name,
> +                       &c_aux->lflow->header_.uuid);
> +
>      const struct sbrec_port_binding *pb
>          = lport_lookup_by_name(c_aux->sbrec_port_binding_by_name, port_name);
>      if (!pb) {
>          return false;
>      }
>
> -    /* Store the port_name to lflow reference. */
> -    int64_t dp_id = pb->datapath->tunnel_key;
> -    char buf[16];
> -    get_unique_lport_key(dp_id, pb->tunnel_key, buf, sizeof(buf));
> -    lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, buf,
> -                       &c_aux->lflow->header_.uuid);
> -
>      if (strcmp(pb->type, "chassisredirect")) {
>          /* for non-chassisredirect ports */
>          return pb->chassis && pb->chassis == c_aux->chassis;
> @@ -623,8 +629,6 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
>                  int64_t dp_id = dp->tunnel_key;
>                  char buf[16];
>                  get_unique_lport_key(dp_id, port_id, buf, sizeof(buf));
> -                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",
> @@ -788,6 +792,7 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow,
>      };
>      struct condition_aux cond_aux = {
>          .sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name,
> +        .dp = dp,
>          .chassis = l_ctx_in->chassis,
>          .active_tunnels = l_ctx_in->active_tunnels,
>          .lflow = lflow,
> @@ -805,7 +810,6 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow,
>      struct hmap *matches = NULL;
>      size_t matches_size = 0;
>
> -    bool is_cr_cond_present = false;
>      bool pg_addr_set_ref = false;
>      uint32_t n_conjs = 0;
>
> @@ -843,8 +847,8 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow,
>      case LCACHE_T_NONE:
>      case LCACHE_T_CONJ_ID:
>      case LCACHE_T_EXPR:
> -        expr = expr_evaluate_condition(expr, is_chassis_resident_cb, &cond_aux,
> -                                       &is_cr_cond_present);

I'm not very clear why the variable "is_cr_cond_present" not required any more.

For logical flows with "is_chassis_resident" match condition, we cache
only the "expr" right ?

What is the behavior after this patch ?


> +        expr = expr_evaluate_condition(expr, is_chassis_resident_cb,
> +                                       &cond_aux);
>          expr = expr_normalize(expr);
>          break;
>      case LCACHE_T_MATCHES:
> @@ -883,7 +887,9 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow,
>
>          /* Cache new entry if caching is enabled. */
>          if (lflow_cache_is_enabled(l_ctx_out->lflow_cache)) {
> -            if (cached_expr && !is_cr_cond_present) {
> +            if (cached_expr
> +                && !lflow_ref_lookup(&l_ctx_out->lfrr->lflow_ref_table,
> +                                     &lflow->header_.uuid)) {

This check - lflow_ref_lookup() is not very clear to me.  Can you
please explain a bit about this ?

From what I understand we cache the expr matches if the flow is not
referenced by resources.

Before this patch we were caching the 'expr matches' for a logical
flow like - 'ip4 && inport == "lp1"'
But now since there is a reference for this logical flow in the
lflow_ref table, we only cache the expr tree.
Is this intentional ?  Correct me if my understanding is wrong.


>                  lflow_cache_add_matches(l_ctx_out->lflow_cache,
>                                          &lflow->header_.uuid, matches,
>                                          matches_size);
> @@ -1746,21 +1752,42 @@ lflow_processing_end:
>      return handled;
>  }
>
> +/* Handles a port-binding change that is possibly related to a lport's
> + * residence status on this chassis. */
>  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)
>  {
>      bool changed;
> -    int64_t dp_id = pb->datapath->tunnel_key;
> -    char pb_ref_name[16];
> -    get_unique_lport_key(dp_id, pb->tunnel_key, pb_ref_name,
> -                         sizeof(pb_ref_name));
>
> -    return lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb_ref_name,
> +    return lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb->logical_port,
>                                      l_ctx_in, l_ctx_out, &changed);
>  }
>
> +/* Handles port-binding add/deletions. */
> +bool
> +lflow_handle_changed_port_bindings(struct lflow_ctx_in *l_ctx_in,
> +                                   struct lflow_ctx_out *l_ctx_out)
> +{
> +    bool ret = true;
> +    bool changed;
> +    const struct sbrec_port_binding *pb;
> +    SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb,
> +                                               l_ctx_in->port_binding_table) {
> +        if (!sbrec_port_binding_is_new(pb)
> +            && !sbrec_port_binding_is_deleted(pb)) {
> +            continue;
> +        }
> +        if (!lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb->logical_port,
> +                                      l_ctx_in, l_ctx_out, &changed)) {
> +            ret = false;
> +            break;
> +        }
> +    }
> +    return ret;
> +}
> +

One downside of adding the port binding handler is that we will process
a logical flow 'A' twice if a port binding is created and it is
claimed in the same loop
as the added test case does for the port - "lp2".

We do that first in lflow_handle_flows_for_lport() and then in
lflow_handle_changed_port_bindings().

I'm also not very clear why we need the port_binding handler here ?
Can you please give a scenario for this ?
The added test case passes for me even If I make the function
lflow_handle_changed_port_bindings()
return at the beginning without doing anything.

Thanks
Numan

>  bool
>  lflow_handle_changed_mc_groups(struct lflow_ctx_in *l_ctx_in,
>                                 struct lflow_ctx_out *l_ctx_out)
> diff --git a/controller/lflow.h b/controller/lflow.h
> index e98edf81d..9d8882ae5 100644
> --- a/controller/lflow.h
> +++ b/controller/lflow.h
> @@ -130,6 +130,7 @@ struct lflow_ctx_in {
>      struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath;
>      struct ovsdb_idl_index *sbrec_logical_flow_by_logical_dp_group;
>      struct ovsdb_idl_index *sbrec_port_binding_by_name;
> +    const struct sbrec_port_binding_table *port_binding_table;
>      const struct sbrec_dhcp_options_table *dhcp_options_table;
>      const struct sbrec_dhcpv6_options_table *dhcpv6_options_table;
>      const struct sbrec_datapath_binding_table *dp_binding_table;
> @@ -182,4 +183,6 @@ bool lflow_handle_flows_for_lport(const struct sbrec_port_binding *,
>                                    struct lflow_ctx_out *);
>  bool lflow_handle_changed_mc_groups(struct lflow_ctx_in *,
>                                      struct lflow_ctx_out *);
> +bool lflow_handle_changed_port_bindings(struct lflow_ctx_in *,
> +                                        struct lflow_ctx_out *);
>  #endif /* controller/lflow.h */
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index c28fd6f2d..8136afe5c 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1966,6 +1966,10 @@ init_lflow_ctx(struct engine_node *node,
>                  engine_get_input("SB_multicast_group", node),
>                  "name_datapath");
>
> +    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_dhcp_options_table *dhcp_table =
>          (struct sbrec_dhcp_options_table *)EN_OVSDB_GET(
>              engine_get_input("SB_dhcp_options", node));
> @@ -2029,6 +2033,7 @@ init_lflow_ctx(struct engine_node *node,
>      l_ctx_in->sbrec_logical_flow_by_logical_dp_group =
>          sbrec_logical_flow_by_dp_group;
>      l_ctx_in->sbrec_port_binding_by_name = sbrec_port_binding_by_name;
> +    l_ctx_in->port_binding_table = port_binding_table;
>      l_ctx_in->dhcp_options_table  = dhcp_table;
>      l_ctx_in->dhcpv6_options_table = dhcpv6_table;
>      l_ctx_in->mac_binding_table = mac_binding_table;
> @@ -2221,6 +2226,25 @@ lflow_output_sb_multicast_group_handler(struct engine_node *node, void *data)
>      return true;
>  }
>
> +static bool
> +lflow_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_lflow_output *lfo = data;
> +
> +    struct lflow_ctx_in l_ctx_in;
> +    struct lflow_ctx_out l_ctx_out;
> +    init_lflow_ctx(node, rt_data, lfo, &l_ctx_in, &l_ctx_out);
> +    if (!lflow_handle_changed_port_bindings(&l_ctx_in, &l_ctx_out)) {
> +        return false;
> +    }
> +
> +    engine_set_node_state(node, EN_UPDATED);
> +    return true;
> +}
> +
>  static bool
>  _lflow_output_resource_ref_handler(struct engine_node *node, void *data,
>                                    enum ref_type ref_type)
> @@ -2285,8 +2309,9 @@ _lflow_output_resource_ref_handler(struct engine_node *node, void *data,
>              break;
>
>          case REF_TYPE_PORTBINDING:
> -            /* This ref type is handled in the
> -             * flow_output_runtime_data_handler. */
> +            /* This ref type is handled in:
> +             * - flow_output_runtime_data_handler
> +             * - flow_output_sb_port_binding_handler. */
>          case REF_TYPE_MC_GROUP:
>              /* This ref type is handled in the
>               * flow_output_sb_multicast_group_handler. */
> @@ -2895,12 +2920,8 @@ main(int argc, char *argv[])
>      engine_add_input(&en_lflow_output, &en_sb_chassis,
>                       pflow_lflow_output_sb_chassis_handler);
>
> -    /* Any changes to the port binding, need not be handled
> -     * for lflow_outout engine.  We still need sb_port_binding
> -     * as input to access the port binding data in lflow.c and
> -     * hence the noop handler. */
>      engine_add_input(&en_lflow_output, &en_sb_port_binding,
> -                     engine_noop_handler);
> +                     lflow_output_sb_port_binding_handler);
>
>      engine_add_input(&en_lflow_output, &en_ovs_open_vswitch, NULL);
>      engine_add_input(&en_lflow_output, &en_ovs_bridge, NULL);
> diff --git a/include/ovn/expr.h b/include/ovn/expr.h
> index de90e87e1..3b5653f7b 100644
> --- a/include/ovn/expr.h
> +++ b/include/ovn/expr.h
> @@ -438,7 +438,7 @@ struct expr *expr_evaluate_condition(
>      struct expr *,
>      bool (*is_chassis_resident)(const void *c_aux,
>                                  const char *port_name),
> -    const void *c_aux, bool *condition_present);
> +    const void *c_aux);
>  struct expr *expr_normalize(struct expr *);
>
>  bool expr_honors_invariants(const struct expr *);
> diff --git a/lib/expr.c b/lib/expr.c
> index f728f9537..e3f6bb892 100644
> --- a/lib/expr.c
> +++ b/lib/expr.c
> @@ -2121,16 +2121,13 @@ static struct expr *
>  expr_evaluate_condition__(struct expr *expr,
>                            bool (*is_chassis_resident)(const void *c_aux,
>                                                        const char *port_name),
> -                          const void *c_aux, bool *condition_present)
> +                          const void *c_aux)
>  {
>      bool result;
>
>      switch (expr->cond.type) {
>      case EXPR_COND_CHASSIS_RESIDENT:
>          result = is_chassis_resident(c_aux, expr->cond.string);
> -        if (condition_present != NULL) {
> -            *condition_present = true;
> -        }
>          break;
>
>      default:
> @@ -2146,7 +2143,7 @@ struct expr *
>  expr_evaluate_condition(struct expr *expr,
>                          bool (*is_chassis_resident)(const void *c_aux,
>                                                      const char *port_name),
> -                        const void *c_aux, bool *condition_present)
> +                        const void *c_aux)
>  {
>      struct expr *sub, *next;
>
> @@ -2156,15 +2153,14 @@ expr_evaluate_condition(struct expr *expr,
>           LIST_FOR_EACH_SAFE (sub, next, node, &expr->andor) {
>              ovs_list_remove(&sub->node);
>              struct expr *e = expr_evaluate_condition(sub, is_chassis_resident,
> -                                                     c_aux, condition_present);
> +                                                     c_aux);
>              e = expr_fix(e);
>              expr_insert_andor(expr, next, e);
>          }
>          return expr_fix(expr);
>
>      case EXPR_T_CONDITION:
> -        return expr_evaluate_condition__(expr, is_chassis_resident, c_aux,
> -                                         condition_present);
> +        return expr_evaluate_condition__(expr, is_chassis_resident, c_aux);
>
>      case EXPR_T_CMP:
>      case EXPR_T_BOOLEAN:
> @@ -3517,7 +3513,7 @@ expr_parse_microflow__(struct lexer *lexer,
>
>      e = expr_simplify(e);
>      e = expr_evaluate_condition(e, microflow_is_chassis_resident_cb,
> -                                NULL, NULL);
> +                                NULL);
>      e = expr_normalize(e);
>
>      struct match m = MATCH_CATCHALL_INITIALIZER;
> diff --git a/tests/ovn.at b/tests/ovn.at
> index bc494fcad..e6d609b5b 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -26782,3 +26782,50 @@ AT_CHECK([ovs-ofctl dump-flows br-int "nw_src=10.0.0.0/24" | \
>  OVN_CLEANUP([hv1])
>  AT_CLEANUP
>  ])
> +
> +# Test when a logical port name is used in an ACL before it is created. When it
> +# is created later, the ACL should be programmed as openflow rules by
> +# ovn-controller. Although this is not likely to happen in real world use
> +# cases, it is possible that a port-binding is observed by ovn-controller AFTER
> +# an lflow that references the port is processed. So this test case is to make
> +# sure the incremental processing in ovn-controller reprocesses the lflow when
> +# the port-binding is observed.
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn -- ACL referencing lport before creation])
> +ovn_start
> +
> +net_add n1
> +
> +sim_add hv1
> +as hv1
> +check ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +
> +# Bind both lp1 and lp2 on the chasis.
> +check ovs-vsctl add-port br-int lp1 -- set interface lp1 external_ids:iface-id=lp1
> +check ovs-vsctl add-port br-int lp2 -- set interface lp2 external_ids:iface-id=lp2
> +
> +# Create only lport lp1, but not lp2.
> +check ovn-nbctl ls-add lsw0
> +check ovn-nbctl lsp-add lsw0 lp1 \
> +    -- lsp-set-addresses lp1 "f0:00:00:00:00:01 10.0.0.11"
> +
> +# Each lport is referenced by an ACL.
> +check ovn-nbctl acl-add lsw0 to-lport 1002 'outport == "lp1" && ip4.src == 10.0.0.111'  allow-related
> +check ovn-nbctl acl-add lsw0 to-lport 1002 'outport == "lp2" && ip4.src == 10.0.0.222'  allow-related
> +
> +# The first ACL should be programmed.
> +check ovn-nbctl --wait=hv sync
> +AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.111], [0], [ignore])
> +
> +# Now create the lport lp2.
> +check ovn-nbctl lsp-add lsw0 lp2 \
> +    -- lsp-set-addresses lp2 "f0:00:00:00:00:02 10.0.0.22"
> +
> +check ovn-nbctl --wait=hv sync
> +# Now the second ACL should be programmed.
> +AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.222], [0], [ignore])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> index 98cc2c503..c6d8b287b 100644
> --- a/tests/test-ovn.c
> +++ b/tests/test-ovn.c
> @@ -318,7 +318,7 @@ test_parse_expr__(int steps)
>              if (steps > 1) {
>                  expr = expr_simplify(expr);
>                  expr = expr_evaluate_condition(expr, is_chassis_resident_cb,
> -                                               &ports, NULL);
> +                                               &ports);
>              }
>              if (steps > 2) {
>                  expr = expr_normalize(expr);
> @@ -922,7 +922,7 @@ test_tree_shape_exhaustively(struct expr *expr, struct shash *symtab,
>              modified = expr_simplify(expr_clone(expr));
>              modified = expr_evaluate_condition(
>                  modified, tree_shape_is_chassis_resident_cb,
> -                NULL, NULL);
> +                NULL);
>              ovs_assert(expr_honors_invariants(modified));
>
>              if (operation >= OP_NORMALIZE) {
> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> index 49463c5c2..5d016b757 100644
> --- a/utilities/ovn-trace.c
> +++ b/utilities/ovn-trace.c
> @@ -973,7 +973,7 @@ parse_lflow_for_datapath(const struct sbrec_logical_flow *sblf,
>              match = expr_simplify(match);
>              match = expr_evaluate_condition(match,
>                                              ovntrace_is_chassis_resident,
> -                                            NULL, NULL);
> +                                            NULL);
>          }
>
>          struct ovntrace_flow *flow = xzalloc(sizeof *flow);
> --
> 2.30.2
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On Wed, Jun 23, 2021 at 7:48 AM Numan Siddique <numans@ovn.org> wrote:
>
> On Mon, Jun 21, 2021 at 2:52 AM Han Zhou <hzhou@ovn.org> wrote:
> >
> > If a lflow has an lport name in the match, but when the lflow is
> > processed the port-binding is not seen by ovn-controller, the
> > corresponding openflow will not be created. Later if the port-binding is
> > created/monitored by ovn-controller, the lflow is not reprocessed
> > because the lflow didn't change and ovn-controller doesn't know that the
> > port-binding affects the lflow. This patch fixes the problem by tracking
> > the references when parsing the lflow, even if the port-binding is not
> > found when the lflow is firstly parsed. A test case is also added to
> > cover the scenario.
> >
> > Signed-off-by: Han Zhou <hzhou@ovn.org>
>
> Hi Han,
>
> Thanks for fixing these issues.   I've a few questions.  I haven't
> reviewed the patch completely.
>
>
> > ---
> >  controller/lflow.c          | 63 ++++++++++++++++++++++++++-----------
> >  controller/lflow.h          |  3 ++
> >  controller/ovn-controller.c | 35 ++++++++++++++++-----
> >  include/ovn/expr.h          |  2 +-
> >  lib/expr.c                  | 14 +++------
> >  tests/ovn.at                | 47 +++++++++++++++++++++++++++
> >  tests/test-ovn.c            |  4 +--
> >  utilities/ovn-trace.c       |  2 +-
> >  8 files changed, 132 insertions(+), 38 deletions(-)
> >
> > diff --git a/controller/lflow.c b/controller/lflow.c
> > index 34eca135a..b7699a309 100644
> > --- a/controller/lflow.c
> > +++ b/controller/lflow.c
> > @@ -61,6 +61,7 @@ struct lookup_port_aux {
> >
> >  struct condition_aux {
> >      struct ovsdb_idl_index *sbrec_port_binding_by_name;
> > +    const struct sbrec_datapath_binding *dp;
> >      const struct sbrec_chassis *chassis;
> >      const struct sset *active_tunnels;
> >      const struct sbrec_logical_flow *lflow;
> > @@ -98,6 +99,12 @@ lookup_port_cb(const void *aux_, const char
*port_name, unsigned int *portp)
> >
> >      const struct lookup_port_aux *aux = aux_;
> >
> > +    /* Store the name that used to lookup the lport to lflow
reference, so that
> > +     * in the future when the lport's port binding changes, the
logical flow
> > +     * that references this lport can be reprocessed. */
> > +    lflow_resource_add(aux->lfrr, REF_TYPE_PORTBINDING, port_name,
> > +                       &aux->lflow->header_.uuid);
> > +
> >      const struct sbrec_port_binding *pb
> >          = lport_lookup_by_name(aux->sbrec_port_binding_by_name,
port_name);
> >      if (pb && pb->datapath == aux->dp) {
> > @@ -149,19 +156,18 @@ is_chassis_resident_cb(const void *c_aux_, const
char *port_name)
> >  {
> >      const struct condition_aux *c_aux = c_aux_;
> >
> > +    /* Store the port name that used to lookup the lport to lflow
reference, so
> > +     * that in the future when the lport's port-binding changes the
logical
> > +     * flow that references this lport can be reprocessed. */
> > +    lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, port_name,
> > +                       &c_aux->lflow->header_.uuid);
> > +
> >      const struct sbrec_port_binding *pb
> >          = lport_lookup_by_name(c_aux->sbrec_port_binding_by_name,
port_name);
> >      if (!pb) {
> >          return false;
> >      }
> >
> > -    /* Store the port_name to lflow reference. */
> > -    int64_t dp_id = pb->datapath->tunnel_key;
> > -    char buf[16];
> > -    get_unique_lport_key(dp_id, pb->tunnel_key, buf, sizeof(buf));
> > -    lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, buf,
> > -                       &c_aux->lflow->header_.uuid);
> > -
> >      if (strcmp(pb->type, "chassisredirect")) {
> >          /* for non-chassisredirect ports */
> >          return pb->chassis && pb->chassis == c_aux->chassis;
> > @@ -623,8 +629,6 @@ add_matches_to_flow_table(const struct
sbrec_logical_flow *lflow,
> >                  int64_t dp_id = dp->tunnel_key;
> >                  char buf[16];
> >                  get_unique_lport_key(dp_id, port_id, buf, sizeof(buf));
> > -                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",
> > @@ -788,6 +792,7 @@ consider_logical_flow__(const struct
sbrec_logical_flow *lflow,
> >      };
> >      struct condition_aux cond_aux = {
> >          .sbrec_port_binding_by_name =
l_ctx_in->sbrec_port_binding_by_name,
> > +        .dp = dp,
> >          .chassis = l_ctx_in->chassis,
> >          .active_tunnels = l_ctx_in->active_tunnels,
> >          .lflow = lflow,
> > @@ -805,7 +810,6 @@ consider_logical_flow__(const struct
sbrec_logical_flow *lflow,
> >      struct hmap *matches = NULL;
> >      size_t matches_size = 0;
> >
> > -    bool is_cr_cond_present = false;
> >      bool pg_addr_set_ref = false;
> >      uint32_t n_conjs = 0;
> >
> > @@ -843,8 +847,8 @@ consider_logical_flow__(const struct
sbrec_logical_flow *lflow,
> >      case LCACHE_T_NONE:
> >      case LCACHE_T_CONJ_ID:
> >      case LCACHE_T_EXPR:
> > -        expr = expr_evaluate_condition(expr, is_chassis_resident_cb,
&cond_aux,
> > -                                       &is_cr_cond_present);
>
> I'm not very clear why the variable "is_cr_cond_present" not required any
more.
>
> For logical flows with "is_chassis_resident" match condition, we cache
> only the "expr" right ?
>
> What is the behavior after this patch ?
>
Thanks Numan for the review!

After this patch, it is the same for "is_chassis_resident", which we cache
only the "expr" just like before.
However, it changes the cache behavior for lflows that reference logical
ports. Some of the lflows may previously cache the "match", but now they
will cache "expr" only. This is because the logical port parsing is done
when converting expr to matches. So we can't cache the match. Otherwise the
test case would fail.

>
> > +        expr = expr_evaluate_condition(expr, is_chassis_resident_cb,
> > +                                       &cond_aux);
> >          expr = expr_normalize(expr);
> >          break;
> >      case LCACHE_T_MATCHES:
> > @@ -883,7 +887,9 @@ consider_logical_flow__(const struct
sbrec_logical_flow *lflow,
> >
> >          /* Cache new entry if caching is enabled. */
> >          if (lflow_cache_is_enabled(l_ctx_out->lflow_cache)) {
> > -            if (cached_expr && !is_cr_cond_present) {
> > +            if (cached_expr
> > +                && !lflow_ref_lookup(&l_ctx_out->lfrr->lflow_ref_table,
> > +                                     &lflow->header_.uuid)) {
>
> This check - lflow_ref_lookup() is not very clear to me.  Can you
> please explain a bit about this ?
>
> From what I understand we cache the expr matches if the flow is not
> referenced by resources.
>
> Before this patch we were caching the 'expr matches' for a logical
> flow like - 'ip4 && inport == "lp1"'
> But now since there is a reference for this logical flow in the
> lflow_ref table, we only cache the expr tree.
> Is this intentional ?  Correct me if my understanding is wrong.

Yes this is intentional as explained above. We can't cache the match
whenever a lport is referenced by the logical flow, which includes
"is_chassis_resident" and other lport references as well.
There is a performance impact of the effectiveness of lflow-cache. I reran
the scale test with a setup closer to an ovn-k8s setup with 500 chassises
and 5 LSPs per chassis, and GRs, some ACLs with port-group/address-set size
1k, and some LBs, etc. On my test machine the time spent by ovn-controller
for recompute increased from 2.9s to 3.2s, around 10% increase. (of course
the memory cost is much lower, around 40% less). I didn't notice this
obvious performance difference from an earlier test when I was replying to
Dumitru, probably because I messed up with the lflow-cache flag settings.
(I reran the tests multiple times and results are consistent so I believe
there were mistakes in my earlier test).

Probably this ~10% difference is still not that much for the recompute
scenario from my perspective, while the scenario I am trying to fix may not
be critical either, and I am not very sure if this not-so-critical fix is
worth the not-so-critical performance cost. I just feel it is better to be
correct, unless we know exactly what are the corner cases that would lead
to a lflow being processed before the port-binding in SB is seen by the
ovn-controller. The test case I crafted is one but not sure if there are
others. Let me know your thoughts on this.

As a note, I also retested both versions with lflow-cache disabled, and the
performance of recompute with this patch is in fact slightly better ~2% for
the same scenario. In our current deployment we have critical concerns for
memory, so didn't enable lflow-cache for now.

>
>
> >                  lflow_cache_add_matches(l_ctx_out->lflow_cache,
> >                                          &lflow->header_.uuid, matches,
> >                                          matches_size);
> > @@ -1746,21 +1752,42 @@ lflow_processing_end:
> >      return handled;
> >  }
> >
> > +/* Handles a port-binding change that is possibly related to a lport's
> > + * residence status on this chassis. */
> >  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)
> >  {
> >      bool changed;
> > -    int64_t dp_id = pb->datapath->tunnel_key;
> > -    char pb_ref_name[16];
> > -    get_unique_lport_key(dp_id, pb->tunnel_key, pb_ref_name,
> > -                         sizeof(pb_ref_name));
> >
> > -    return lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb_ref_name,
> > +    return lflow_handle_changed_ref(REF_TYPE_PORTBINDING,
pb->logical_port,
> >                                      l_ctx_in, l_ctx_out, &changed);
> >  }
> >
> > +/* Handles port-binding add/deletions. */
> > +bool
> > +lflow_handle_changed_port_bindings(struct lflow_ctx_in *l_ctx_in,
> > +                                   struct lflow_ctx_out *l_ctx_out)
> > +{
> > +    bool ret = true;
> > +    bool changed;
> > +    const struct sbrec_port_binding *pb;
> > +    SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb,
> > +
l_ctx_in->port_binding_table) {
> > +        if (!sbrec_port_binding_is_new(pb)
> > +            && !sbrec_port_binding_is_deleted(pb)) {
> > +            continue;
> > +        }
> > +        if (!lflow_handle_changed_ref(REF_TYPE_PORTBINDING,
pb->logical_port,
> > +                                      l_ctx_in, l_ctx_out, &changed)) {
> > +            ret = false;
> > +            break;
> > +        }
> > +    }
> > +    return ret;
> > +}
> > +
>
> One downside of adding the port binding handler is that we will process
> a logical flow 'A' twice if a port binding is created and it is
> claimed in the same loop
> as the added test case does for the port - "lp2".
>
> We do that first in lflow_handle_flows_for_lport() and then in
> lflow_handle_changed_port_bindings().
>
Yes, in such cases the lflow will be handled twice. However, it is a
general problem in I-P engine for different input changes in the same
iteration. Although it is not a critical penalty in most cases because of
I-P, I am still trying to optimize this to make sure a lflow is handled at
most once in one engine run, so that for lflows with a big
port-group/address-set reference won't cause more latency in I-P.

> I'm also not very clear why we need the port_binding handler here ?
> Can you please give a scenario for this ?
> The added test case passes for me even If I make the function
> lflow_handle_changed_port_bindings()
> return at the beginning without doing anything.

The lflow_handle_flows_for_lport() is called only when a port-binding is
affecting local residency, while lflow_handle_changed_port_bindings() takes
care of all port-binding adding/deletions, including the ports bound on
other chassises. The test case I used only one HV so it didn't matter even
if you returned directly. (but the current test case fails without this
patch). Probably I should update the test with two HVs to cover the
scenario better.

Thanks,
Han

>
> Thanks
> Numan
>
> >  bool
> >  lflow_handle_changed_mc_groups(struct lflow_ctx_in *l_ctx_in,
> >                                 struct lflow_ctx_out *l_ctx_out)
> > diff --git a/controller/lflow.h b/controller/lflow.h
> > index e98edf81d..9d8882ae5 100644
> > --- a/controller/lflow.h
> > +++ b/controller/lflow.h
> > @@ -130,6 +130,7 @@ struct lflow_ctx_in {
> >      struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath;
> >      struct ovsdb_idl_index *sbrec_logical_flow_by_logical_dp_group;
> >      struct ovsdb_idl_index *sbrec_port_binding_by_name;
> > +    const struct sbrec_port_binding_table *port_binding_table;
> >      const struct sbrec_dhcp_options_table *dhcp_options_table;
> >      const struct sbrec_dhcpv6_options_table *dhcpv6_options_table;
> >      const struct sbrec_datapath_binding_table *dp_binding_table;
> > @@ -182,4 +183,6 @@ bool lflow_handle_flows_for_lport(const struct
sbrec_port_binding *,
> >                                    struct lflow_ctx_out *);
> >  bool lflow_handle_changed_mc_groups(struct lflow_ctx_in *,
> >                                      struct lflow_ctx_out *);
> > +bool lflow_handle_changed_port_bindings(struct lflow_ctx_in *,
> > +                                        struct lflow_ctx_out *);
> >  #endif /* controller/lflow.h */
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index c28fd6f2d..8136afe5c 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -1966,6 +1966,10 @@ init_lflow_ctx(struct engine_node *node,
> >                  engine_get_input("SB_multicast_group", node),
> >                  "name_datapath");
> >
> > +    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_dhcp_options_table *dhcp_table =
> >          (struct sbrec_dhcp_options_table *)EN_OVSDB_GET(
> >              engine_get_input("SB_dhcp_options", node));
> > @@ -2029,6 +2033,7 @@ init_lflow_ctx(struct engine_node *node,
> >      l_ctx_in->sbrec_logical_flow_by_logical_dp_group =
> >          sbrec_logical_flow_by_dp_group;
> >      l_ctx_in->sbrec_port_binding_by_name = sbrec_port_binding_by_name;
> > +    l_ctx_in->port_binding_table = port_binding_table;
> >      l_ctx_in->dhcp_options_table  = dhcp_table;
> >      l_ctx_in->dhcpv6_options_table = dhcpv6_table;
> >      l_ctx_in->mac_binding_table = mac_binding_table;
> > @@ -2221,6 +2226,25 @@ lflow_output_sb_multicast_group_handler(struct
engine_node *node, void *data)
> >      return true;
> >  }
> >
> > +static bool
> > +lflow_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_lflow_output *lfo = data;
> > +
> > +    struct lflow_ctx_in l_ctx_in;
> > +    struct lflow_ctx_out l_ctx_out;
> > +    init_lflow_ctx(node, rt_data, lfo, &l_ctx_in, &l_ctx_out);
> > +    if (!lflow_handle_changed_port_bindings(&l_ctx_in, &l_ctx_out)) {
> > +        return false;
> > +    }
> > +
> > +    engine_set_node_state(node, EN_UPDATED);
> > +    return true;
> > +}
> > +
> >  static bool
> >  _lflow_output_resource_ref_handler(struct engine_node *node, void
*data,
> >                                    enum ref_type ref_type)
> > @@ -2285,8 +2309,9 @@ _lflow_output_resource_ref_handler(struct
engine_node *node, void *data,
> >              break;
> >
> >          case REF_TYPE_PORTBINDING:
> > -            /* This ref type is handled in the
> > -             * flow_output_runtime_data_handler. */
> > +            /* This ref type is handled in:
> > +             * - flow_output_runtime_data_handler
> > +             * - flow_output_sb_port_binding_handler. */
> >          case REF_TYPE_MC_GROUP:
> >              /* This ref type is handled in the
> >               * flow_output_sb_multicast_group_handler. */
> > @@ -2895,12 +2920,8 @@ main(int argc, char *argv[])
> >      engine_add_input(&en_lflow_output, &en_sb_chassis,
> >                       pflow_lflow_output_sb_chassis_handler);
> >
> > -    /* Any changes to the port binding, need not be handled
> > -     * for lflow_outout engine.  We still need sb_port_binding
> > -     * as input to access the port binding data in lflow.c and
> > -     * hence the noop handler. */
> >      engine_add_input(&en_lflow_output, &en_sb_port_binding,
> > -                     engine_noop_handler);
> > +                     lflow_output_sb_port_binding_handler);
> >
> >      engine_add_input(&en_lflow_output, &en_ovs_open_vswitch, NULL);
> >      engine_add_input(&en_lflow_output, &en_ovs_bridge, NULL);
> > diff --git a/include/ovn/expr.h b/include/ovn/expr.h
> > index de90e87e1..3b5653f7b 100644
> > --- a/include/ovn/expr.h
> > +++ b/include/ovn/expr.h
> > @@ -438,7 +438,7 @@ struct expr *expr_evaluate_condition(
> >      struct expr *,
> >      bool (*is_chassis_resident)(const void *c_aux,
> >                                  const char *port_name),
> > -    const void *c_aux, bool *condition_present);
> > +    const void *c_aux);
> >  struct expr *expr_normalize(struct expr *);
> >
> >  bool expr_honors_invariants(const struct expr *);
> > diff --git a/lib/expr.c b/lib/expr.c
> > index f728f9537..e3f6bb892 100644
> > --- a/lib/expr.c
> > +++ b/lib/expr.c
> > @@ -2121,16 +2121,13 @@ static struct expr *
> >  expr_evaluate_condition__(struct expr *expr,
> >                            bool (*is_chassis_resident)(const void
*c_aux,
> >                                                        const char
*port_name),
> > -                          const void *c_aux, bool *condition_present)
> > +                          const void *c_aux)
> >  {
> >      bool result;
> >
> >      switch (expr->cond.type) {
> >      case EXPR_COND_CHASSIS_RESIDENT:
> >          result = is_chassis_resident(c_aux, expr->cond.string);
> > -        if (condition_present != NULL) {
> > -            *condition_present = true;
> > -        }
> >          break;
> >
> >      default:
> > @@ -2146,7 +2143,7 @@ struct expr *
> >  expr_evaluate_condition(struct expr *expr,
> >                          bool (*is_chassis_resident)(const void *c_aux,
> >                                                      const char
*port_name),
> > -                        const void *c_aux, bool *condition_present)
> > +                        const void *c_aux)
> >  {
> >      struct expr *sub, *next;
> >
> > @@ -2156,15 +2153,14 @@ expr_evaluate_condition(struct expr *expr,
> >           LIST_FOR_EACH_SAFE (sub, next, node, &expr->andor) {
> >              ovs_list_remove(&sub->node);
> >              struct expr *e = expr_evaluate_condition(sub,
is_chassis_resident,
> > -                                                     c_aux,
condition_present);
> > +                                                     c_aux);
> >              e = expr_fix(e);
> >              expr_insert_andor(expr, next, e);
> >          }
> >          return expr_fix(expr);
> >
> >      case EXPR_T_CONDITION:
> > -        return expr_evaluate_condition__(expr, is_chassis_resident,
c_aux,
> > -                                         condition_present);
> > +        return expr_evaluate_condition__(expr, is_chassis_resident,
c_aux);
> >
> >      case EXPR_T_CMP:
> >      case EXPR_T_BOOLEAN:
> > @@ -3517,7 +3513,7 @@ expr_parse_microflow__(struct lexer *lexer,
> >
> >      e = expr_simplify(e);
> >      e = expr_evaluate_condition(e, microflow_is_chassis_resident_cb,
> > -                                NULL, NULL);
> > +                                NULL);
> >      e = expr_normalize(e);
> >
> >      struct match m = MATCH_CATCHALL_INITIALIZER;
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index bc494fcad..e6d609b5b 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -26782,3 +26782,50 @@ AT_CHECK([ovs-ofctl dump-flows br-int "nw_src=
10.0.0.0/24" | \
> >  OVN_CLEANUP([hv1])
> >  AT_CLEANUP
> >  ])
> > +
> > +# Test when a logical port name is used in an ACL before it is
created. When it
> > +# is created later, the ACL should be programmed as openflow rules by
> > +# ovn-controller. Although this is not likely to happen in real world
use
> > +# cases, it is possible that a port-binding is observed by
ovn-controller AFTER
> > +# an lflow that references the port is processed. So this test case is
to make
> > +# sure the incremental processing in ovn-controller reprocesses the
lflow when
> > +# the port-binding is observed.
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([ovn -- ACL referencing lport before creation])
> > +ovn_start
> > +
> > +net_add n1
> > +
> > +sim_add hv1
> > +as hv1
> > +check ovs-vsctl add-br br-phys
> > +ovn_attach n1 br-phys 192.168.0.1
> > +
> > +# Bind both lp1 and lp2 on the chasis.
> > +check ovs-vsctl add-port br-int lp1 -- set interface lp1
external_ids:iface-id=lp1
> > +check ovs-vsctl add-port br-int lp2 -- set interface lp2
external_ids:iface-id=lp2
> > +
> > +# Create only lport lp1, but not lp2.
> > +check ovn-nbctl ls-add lsw0
> > +check ovn-nbctl lsp-add lsw0 lp1 \
> > +    -- lsp-set-addresses lp1 "f0:00:00:00:00:01 10.0.0.11"
> > +
> > +# Each lport is referenced by an ACL.
> > +check ovn-nbctl acl-add lsw0 to-lport 1002 'outport == "lp1" &&
ip4.src == 10.0.0.111'  allow-related
> > +check ovn-nbctl acl-add lsw0 to-lport 1002 'outport == "lp2" &&
ip4.src == 10.0.0.222'  allow-related
> > +
> > +# The first ACL should be programmed.
> > +check ovn-nbctl --wait=hv sync
> > +AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.111],
[0], [ignore])
> > +
> > +# Now create the lport lp2.
> > +check ovn-nbctl lsp-add lsw0 lp2 \
> > +    -- lsp-set-addresses lp2 "f0:00:00:00:00:02 10.0.0.22"
> > +
> > +check ovn-nbctl --wait=hv sync
> > +# Now the second ACL should be programmed.
> > +AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.222],
[0], [ignore])
> > +
> > +OVN_CLEANUP([hv1])
> > +AT_CLEANUP
> > +])
> > diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> > index 98cc2c503..c6d8b287b 100644
> > --- a/tests/test-ovn.c
> > +++ b/tests/test-ovn.c
> > @@ -318,7 +318,7 @@ test_parse_expr__(int steps)
> >              if (steps > 1) {
> >                  expr = expr_simplify(expr);
> >                  expr = expr_evaluate_condition(expr,
is_chassis_resident_cb,
> > -                                               &ports, NULL);
> > +                                               &ports);
> >              }
> >              if (steps > 2) {
> >                  expr = expr_normalize(expr);
> > @@ -922,7 +922,7 @@ test_tree_shape_exhaustively(struct expr *expr,
struct shash *symtab,
> >              modified = expr_simplify(expr_clone(expr));
> >              modified = expr_evaluate_condition(
> >                  modified, tree_shape_is_chassis_resident_cb,
> > -                NULL, NULL);
> > +                NULL);
> >              ovs_assert(expr_honors_invariants(modified));
> >
> >              if (operation >= OP_NORMALIZE) {
> > diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> > index 49463c5c2..5d016b757 100644
> > --- a/utilities/ovn-trace.c
> > +++ b/utilities/ovn-trace.c
> > @@ -973,7 +973,7 @@ parse_lflow_for_datapath(const struct
sbrec_logical_flow *sblf,
> >              match = expr_simplify(match);
> >              match = expr_evaluate_condition(match,
> >
 ovntrace_is_chassis_resident,
> > -                                            NULL, NULL);
> > +                                            NULL);
> >          }
> >
> >          struct ovntrace_flow *flow = xzalloc(sizeof *flow);
> > --
> > 2.30.2
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >

On Wed, Jun 23, 2021 at 8:10 PM Han Zhou <hzhou@ovn.org> wrote:
>
> On Wed, Jun 23, 2021 at 7:48 AM Numan Siddique <numans@ovn.org> wrote:
> >
> > On Mon, Jun 21, 2021 at 2:52 AM Han Zhou <hzhou@ovn.org> wrote:
> > >
> > > If a lflow has an lport name in the match, but when the lflow is
> > > processed the port-binding is not seen by ovn-controller, the
> > > corresponding openflow will not be created. Later if the port-binding is
> > > created/monitored by ovn-controller, the lflow is not reprocessed
> > > because the lflow didn't change and ovn-controller doesn't know that the
> > > port-binding affects the lflow. This patch fixes the problem by tracking
> > > the references when parsing the lflow, even if the port-binding is not
> > > found when the lflow is firstly parsed. A test case is also added to
> > > cover the scenario.
> > >
> > > Signed-off-by: Han Zhou <hzhou@ovn.org>
> >
> > Hi Han,
> >
> > Thanks for fixing these issues.   I've a few questions.  I haven't
> > reviewed the patch completely.
> >
> >
> > > ---
> > >  controller/lflow.c          | 63 ++++++++++++++++++++++++++-----------
> > >  controller/lflow.h          |  3 ++
> > >  controller/ovn-controller.c | 35 ++++++++++++++++-----
> > >  include/ovn/expr.h          |  2 +-
> > >  lib/expr.c                  | 14 +++------
> > >  tests/ovn.at                | 47 +++++++++++++++++++++++++++
> > >  tests/test-ovn.c            |  4 +--
> > >  utilities/ovn-trace.c       |  2 +-
> > >  8 files changed, 132 insertions(+), 38 deletions(-)
> > >
> > > diff --git a/controller/lflow.c b/controller/lflow.c
> > > index 34eca135a..b7699a309 100644
> > > --- a/controller/lflow.c
> > > +++ b/controller/lflow.c
> > > @@ -61,6 +61,7 @@ struct lookup_port_aux {
> > >
> > >  struct condition_aux {
> > >      struct ovsdb_idl_index *sbrec_port_binding_by_name;
> > > +    const struct sbrec_datapath_binding *dp;
> > >      const struct sbrec_chassis *chassis;
> > >      const struct sset *active_tunnels;
> > >      const struct sbrec_logical_flow *lflow;
> > > @@ -98,6 +99,12 @@ lookup_port_cb(const void *aux_, const char
> *port_name, unsigned int *portp)
> > >
> > >      const struct lookup_port_aux *aux = aux_;
> > >
> > > +    /* Store the name that used to lookup the lport to lflow
> reference, so that
> > > +     * in the future when the lport's port binding changes, the
> logical flow
> > > +     * that references this lport can be reprocessed. */
> > > +    lflow_resource_add(aux->lfrr, REF_TYPE_PORTBINDING, port_name,
> > > +                       &aux->lflow->header_.uuid);
> > > +
> > >      const struct sbrec_port_binding *pb
> > >          = lport_lookup_by_name(aux->sbrec_port_binding_by_name,
> port_name);
> > >      if (pb && pb->datapath == aux->dp) {
> > > @@ -149,19 +156,18 @@ is_chassis_resident_cb(const void *c_aux_, const
> char *port_name)
> > >  {
> > >      const struct condition_aux *c_aux = c_aux_;
> > >
> > > +    /* Store the port name that used to lookup the lport to lflow
> reference, so
> > > +     * that in the future when the lport's port-binding changes the
> logical
> > > +     * flow that references this lport can be reprocessed. */
> > > +    lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, port_name,
> > > +                       &c_aux->lflow->header_.uuid);
> > > +
> > >      const struct sbrec_port_binding *pb
> > >          = lport_lookup_by_name(c_aux->sbrec_port_binding_by_name,
> port_name);
> > >      if (!pb) {
> > >          return false;
> > >      }
> > >
> > > -    /* Store the port_name to lflow reference. */
> > > -    int64_t dp_id = pb->datapath->tunnel_key;
> > > -    char buf[16];
> > > -    get_unique_lport_key(dp_id, pb->tunnel_key, buf, sizeof(buf));
> > > -    lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, buf,
> > > -                       &c_aux->lflow->header_.uuid);
> > > -
> > >      if (strcmp(pb->type, "chassisredirect")) {
> > >          /* for non-chassisredirect ports */
> > >          return pb->chassis && pb->chassis == c_aux->chassis;
> > > @@ -623,8 +629,6 @@ add_matches_to_flow_table(const struct
> sbrec_logical_flow *lflow,
> > >                  int64_t dp_id = dp->tunnel_key;
> > >                  char buf[16];
> > >                  get_unique_lport_key(dp_id, port_id, buf, sizeof(buf));
> > > -                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",
> > > @@ -788,6 +792,7 @@ consider_logical_flow__(const struct
> sbrec_logical_flow *lflow,
> > >      };
> > >      struct condition_aux cond_aux = {
> > >          .sbrec_port_binding_by_name =
> l_ctx_in->sbrec_port_binding_by_name,
> > > +        .dp = dp,
> > >          .chassis = l_ctx_in->chassis,
> > >          .active_tunnels = l_ctx_in->active_tunnels,
> > >          .lflow = lflow,
> > > @@ -805,7 +810,6 @@ consider_logical_flow__(const struct
> sbrec_logical_flow *lflow,
> > >      struct hmap *matches = NULL;
> > >      size_t matches_size = 0;
> > >
> > > -    bool is_cr_cond_present = false;
> > >      bool pg_addr_set_ref = false;
> > >      uint32_t n_conjs = 0;
> > >
> > > @@ -843,8 +847,8 @@ consider_logical_flow__(const struct
> sbrec_logical_flow *lflow,
> > >      case LCACHE_T_NONE:
> > >      case LCACHE_T_CONJ_ID:
> > >      case LCACHE_T_EXPR:
> > > -        expr = expr_evaluate_condition(expr, is_chassis_resident_cb,
> &cond_aux,
> > > -                                       &is_cr_cond_present);
> >
> > I'm not very clear why the variable "is_cr_cond_present" not required any
> more.
> >
> > For logical flows with "is_chassis_resident" match condition, we cache
> > only the "expr" right ?
> >
> > What is the behavior after this patch ?
> >
> Thanks Numan for the review!
>
> After this patch, it is the same for "is_chassis_resident", which we cache
> only the "expr" just like before.
> However, it changes the cache behavior for lflows that reference logical
> ports. Some of the lflows may previously cache the "match", but now they
> will cache "expr" only. This is because the logical port parsing is done
> when converting expr to matches. So we can't cache the match. Otherwise the
> test case would fail.
>
> >
> > > +        expr = expr_evaluate_condition(expr, is_chassis_resident_cb,
> > > +                                       &cond_aux);
> > >          expr = expr_normalize(expr);
> > >          break;
> > >      case LCACHE_T_MATCHES:
> > > @@ -883,7 +887,9 @@ consider_logical_flow__(const struct
> sbrec_logical_flow *lflow,
> > >
> > >          /* Cache new entry if caching is enabled. */
> > >          if (lflow_cache_is_enabled(l_ctx_out->lflow_cache)) {
> > > -            if (cached_expr && !is_cr_cond_present) {
> > > +            if (cached_expr
> > > +                && !lflow_ref_lookup(&l_ctx_out->lfrr->lflow_ref_table,
> > > +                                     &lflow->header_.uuid)) {
> >
> > This check - lflow_ref_lookup() is not very clear to me.  Can you
> > please explain a bit about this ?
> >
> > From what I understand we cache the expr matches if the flow is not
> > referenced by resources.
> >
> > Before this patch we were caching the 'expr matches' for a logical
> > flow like - 'ip4 && inport == "lp1"'
> > But now since there is a reference for this logical flow in the
> > lflow_ref table, we only cache the expr tree.
> > Is this intentional ?  Correct me if my understanding is wrong.
>
> Yes this is intentional as explained above. We can't cache the match
> whenever a lport is referenced by the logical flow, which includes
> "is_chassis_resident" and other lport references as well.
> There is a performance impact of the effectiveness of lflow-cache. I reran
> the scale test with a setup closer to an ovn-k8s setup with 500 chassises
> and 5 LSPs per chassis, and GRs, some ACLs with port-group/address-set size
> 1k, and some LBs, etc. On my test machine the time spent by ovn-controller
> for recompute increased from 2.9s to 3.2s, around 10% increase. (of course
> the memory cost is much lower, around 40% less). I didn't notice this
> obvious performance difference from an earlier test when I was replying to
> Dumitru, probably because I messed up with the lflow-cache flag settings.
> (I reran the tests multiple times and results are consistent so I believe
> there were mistakes in my earlier test).
>
> Probably this ~10% difference is still not that much for the recompute
> scenario from my perspective, while the scenario I am trying to fix may not
> be critical either, and I am not very sure if this not-so-critical fix is
> worth the not-so-critical performance cost. I just feel it is better to be
> correct, unless we know exactly what are the corner cases that would lead
> to a lflow being processed before the port-binding in SB is seen by the
> ovn-controller. The test case I crafted is one but not sure if there are
> others. Let me know your thoughts on this.
>
> As a note, I also retested both versions with lflow-cache disabled, and the
> performance of recompute with this patch is in fact slightly better ~2% for
> the same scenario. In our current deployment we have critical concerns for
> memory, so didn't enable lflow-cache for now.



Thanks for the detailed explanation.

So we only cache the expr tree for the flows which match on a logical port
and that logical port is not seen yet.  I guess this should be fine.


Instead of doing

 if (cached_expr  && !lflow_ref_lookup(&l_ctx_out->lfrr->lflow_ref_table,

&lflow->header_.uuid))

We could probably do something like

if (cached_expr && !is_cr_cond_present && lookup_port_succeeded) {
   // cache expr matches ..
} else {
  //  cache expr tree
}
..
..


lookup_port_cb() can probably set lookup_port_succeeded to false if
the lookup failed.

This would avoid the lflow_ref_lookup().

> >
> >
> > >                  lflow_cache_add_matches(l_ctx_out->lflow_cache,
> > >                                          &lflow->header_.uuid, matches,
> > >                                          matches_size);
> > > @@ -1746,21 +1752,42 @@ lflow_processing_end:
> > >      return handled;
> > >  }
> > >
> > > +/* Handles a port-binding change that is possibly related to a lport's
> > > + * residence status on this chassis. */
> > >  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)
> > >  {
> > >      bool changed;
> > > -    int64_t dp_id = pb->datapath->tunnel_key;
> > > -    char pb_ref_name[16];
> > > -    get_unique_lport_key(dp_id, pb->tunnel_key, pb_ref_name,
> > > -                         sizeof(pb_ref_name));
> > >
> > > -    return lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb_ref_name,
> > > +    return lflow_handle_changed_ref(REF_TYPE_PORTBINDING,
> pb->logical_port,
> > >                                      l_ctx_in, l_ctx_out, &changed);
> > >  }
> > >
> > > +/* Handles port-binding add/deletions. */
> > > +bool
> > > +lflow_handle_changed_port_bindings(struct lflow_ctx_in *l_ctx_in,
> > > +                                   struct lflow_ctx_out *l_ctx_out)
> > > +{
> > > +    bool ret = true;
> > > +    bool changed;
> > > +    const struct sbrec_port_binding *pb;
> > > +    SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb,
> > > +
> l_ctx_in->port_binding_table) {
> > > +        if (!sbrec_port_binding_is_new(pb)
> > > +            && !sbrec_port_binding_is_deleted(pb)) {
> > > +            continue;
> > > +        }
> > > +        if (!lflow_handle_changed_ref(REF_TYPE_PORTBINDING,
> pb->logical_port,
> > > +                                      l_ctx_in, l_ctx_out, &changed)) {
> > > +            ret = false;
> > > +            break;
> > > +        }
> > > +    }
> > > +    return ret;
> > > +}
> > > +
> >
> > One downside of adding the port binding handler is that we will process
> > a logical flow 'A' twice if a port binding is created and it is
> > claimed in the same loop
> > as the added test case does for the port - "lp2".
> >
> > We do that first in lflow_handle_flows_for_lport() and then in
> > lflow_handle_changed_port_bindings().
> >
> Yes, in such cases the lflow will be handled twice. However, it is a
> general problem in I-P engine for different input changes in the same
> iteration. Although it is not a critical penalty in most cases because of
> I-P, I am still trying to optimize this to make sure a lflow is handled at
> most once in one engine run, so that for lflows with a big
> port-group/address-set reference won't cause more latency in I-P.
>
> > I'm also not very clear why we need the port_binding handler here ?
> > Can you please give a scenario for this ?
> > The added test case passes for me even If I make the function
> > lflow_handle_changed_port_bindings()
> > return at the beginning without doing anything.
>
> The lflow_handle_flows_for_lport() is called only when a port-binding is
> affecting local residency, while lflow_handle_changed_port_bindings() takes
> care of all port-binding adding/deletions, including the ports bound on
> other chassises. The test case I used only one HV so it didn't matter even
> if you returned directly. (but the current test case fails without this
> patch). Probably I should update the test with two HVs to cover the
> scenario better.

Ok.  I think it would be great to enhance the test with two HVs.

Thanks
Numan

>
> Thanks,
> Han
>
> >
> > Thanks
> > Numan
> >
> > >  bool
> > >  lflow_handle_changed_mc_groups(struct lflow_ctx_in *l_ctx_in,
> > >                                 struct lflow_ctx_out *l_ctx_out)
> > > diff --git a/controller/lflow.h b/controller/lflow.h
> > > index e98edf81d..9d8882ae5 100644
> > > --- a/controller/lflow.h
> > > +++ b/controller/lflow.h
> > > @@ -130,6 +130,7 @@ struct lflow_ctx_in {
> > >      struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath;
> > >      struct ovsdb_idl_index *sbrec_logical_flow_by_logical_dp_group;
> > >      struct ovsdb_idl_index *sbrec_port_binding_by_name;
> > > +    const struct sbrec_port_binding_table *port_binding_table;
> > >      const struct sbrec_dhcp_options_table *dhcp_options_table;
> > >      const struct sbrec_dhcpv6_options_table *dhcpv6_options_table;
> > >      const struct sbrec_datapath_binding_table *dp_binding_table;
> > > @@ -182,4 +183,6 @@ bool lflow_handle_flows_for_lport(const struct
> sbrec_port_binding *,
> > >                                    struct lflow_ctx_out *);
> > >  bool lflow_handle_changed_mc_groups(struct lflow_ctx_in *,
> > >                                      struct lflow_ctx_out *);
> > > +bool lflow_handle_changed_port_bindings(struct lflow_ctx_in *,
> > > +                                        struct lflow_ctx_out *);
> > >  #endif /* controller/lflow.h */
> > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > > index c28fd6f2d..8136afe5c 100644
> > > --- a/controller/ovn-controller.c
> > > +++ b/controller/ovn-controller.c
> > > @@ -1966,6 +1966,10 @@ init_lflow_ctx(struct engine_node *node,
> > >                  engine_get_input("SB_multicast_group", node),
> > >                  "name_datapath");
> > >
> > > +    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_dhcp_options_table *dhcp_table =
> > >          (struct sbrec_dhcp_options_table *)EN_OVSDB_GET(
> > >              engine_get_input("SB_dhcp_options", node));
> > > @@ -2029,6 +2033,7 @@ init_lflow_ctx(struct engine_node *node,
> > >      l_ctx_in->sbrec_logical_flow_by_logical_dp_group =
> > >          sbrec_logical_flow_by_dp_group;
> > >      l_ctx_in->sbrec_port_binding_by_name = sbrec_port_binding_by_name;
> > > +    l_ctx_in->port_binding_table = port_binding_table;
> > >      l_ctx_in->dhcp_options_table  = dhcp_table;
> > >      l_ctx_in->dhcpv6_options_table = dhcpv6_table;
> > >      l_ctx_in->mac_binding_table = mac_binding_table;
> > > @@ -2221,6 +2226,25 @@ lflow_output_sb_multicast_group_handler(struct
> engine_node *node, void *data)
> > >      return true;
> > >  }
> > >
> > > +static bool
> > > +lflow_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_lflow_output *lfo = data;
> > > +
> > > +    struct lflow_ctx_in l_ctx_in;
> > > +    struct lflow_ctx_out l_ctx_out;
> > > +    init_lflow_ctx(node, rt_data, lfo, &l_ctx_in, &l_ctx_out);
> > > +    if (!lflow_handle_changed_port_bindings(&l_ctx_in, &l_ctx_out)) {
> > > +        return false;
> > > +    }
> > > +
> > > +    engine_set_node_state(node, EN_UPDATED);
> > > +    return true;
> > > +}
> > > +
> > >  static bool
> > >  _lflow_output_resource_ref_handler(struct engine_node *node, void
> *data,
> > >                                    enum ref_type ref_type)
> > > @@ -2285,8 +2309,9 @@ _lflow_output_resource_ref_handler(struct
> engine_node *node, void *data,
> > >              break;
> > >
> > >          case REF_TYPE_PORTBINDING:
> > > -            /* This ref type is handled in the
> > > -             * flow_output_runtime_data_handler. */
> > > +            /* This ref type is handled in:
> > > +             * - flow_output_runtime_data_handler
> > > +             * - flow_output_sb_port_binding_handler. */
> > >          case REF_TYPE_MC_GROUP:
> > >              /* This ref type is handled in the
> > >               * flow_output_sb_multicast_group_handler. */
> > > @@ -2895,12 +2920,8 @@ main(int argc, char *argv[])
> > >      engine_add_input(&en_lflow_output, &en_sb_chassis,
> > >                       pflow_lflow_output_sb_chassis_handler);
> > >
> > > -    /* Any changes to the port binding, need not be handled
> > > -     * for lflow_outout engine.  We still need sb_port_binding
> > > -     * as input to access the port binding data in lflow.c and
> > > -     * hence the noop handler. */
> > >      engine_add_input(&en_lflow_output, &en_sb_port_binding,
> > > -                     engine_noop_handler);
> > > +                     lflow_output_sb_port_binding_handler);
> > >
> > >      engine_add_input(&en_lflow_output, &en_ovs_open_vswitch, NULL);
> > >      engine_add_input(&en_lflow_output, &en_ovs_bridge, NULL);
> > > diff --git a/include/ovn/expr.h b/include/ovn/expr.h
> > > index de90e87e1..3b5653f7b 100644
> > > --- a/include/ovn/expr.h
> > > +++ b/include/ovn/expr.h
> > > @@ -438,7 +438,7 @@ struct expr *expr_evaluate_condition(
> > >      struct expr *,
> > >      bool (*is_chassis_resident)(const void *c_aux,
> > >                                  const char *port_name),
> > > -    const void *c_aux, bool *condition_present);
> > > +    const void *c_aux);
> > >  struct expr *expr_normalize(struct expr *);
> > >
> > >  bool expr_honors_invariants(const struct expr *);
> > > diff --git a/lib/expr.c b/lib/expr.c
> > > index f728f9537..e3f6bb892 100644
> > > --- a/lib/expr.c
> > > +++ b/lib/expr.c
> > > @@ -2121,16 +2121,13 @@ static struct expr *
> > >  expr_evaluate_condition__(struct expr *expr,
> > >                            bool (*is_chassis_resident)(const void
> *c_aux,
> > >                                                        const char
> *port_name),
> > > -                          const void *c_aux, bool *condition_present)
> > > +                          const void *c_aux)
> > >  {
> > >      bool result;
> > >
> > >      switch (expr->cond.type) {
> > >      case EXPR_COND_CHASSIS_RESIDENT:
> > >          result = is_chassis_resident(c_aux, expr->cond.string);
> > > -        if (condition_present != NULL) {
> > > -            *condition_present = true;
> > > -        }
> > >          break;
> > >
> > >      default:
> > > @@ -2146,7 +2143,7 @@ struct expr *
> > >  expr_evaluate_condition(struct expr *expr,
> > >                          bool (*is_chassis_resident)(const void *c_aux,
> > >                                                      const char
> *port_name),
> > > -                        const void *c_aux, bool *condition_present)
> > > +                        const void *c_aux)
> > >  {
> > >      struct expr *sub, *next;
> > >
> > > @@ -2156,15 +2153,14 @@ expr_evaluate_condition(struct expr *expr,
> > >           LIST_FOR_EACH_SAFE (sub, next, node, &expr->andor) {
> > >              ovs_list_remove(&sub->node);
> > >              struct expr *e = expr_evaluate_condition(sub,
> is_chassis_resident,
> > > -                                                     c_aux,
> condition_present);
> > > +                                                     c_aux);
> > >              e = expr_fix(e);
> > >              expr_insert_andor(expr, next, e);
> > >          }
> > >          return expr_fix(expr);
> > >
> > >      case EXPR_T_CONDITION:
> > > -        return expr_evaluate_condition__(expr, is_chassis_resident,
> c_aux,
> > > -                                         condition_present);
> > > +        return expr_evaluate_condition__(expr, is_chassis_resident,
> c_aux);
> > >
> > >      case EXPR_T_CMP:
> > >      case EXPR_T_BOOLEAN:
> > > @@ -3517,7 +3513,7 @@ expr_parse_microflow__(struct lexer *lexer,
> > >
> > >      e = expr_simplify(e);
> > >      e = expr_evaluate_condition(e, microflow_is_chassis_resident_cb,
> > > -                                NULL, NULL);
> > > +                                NULL);
> > >      e = expr_normalize(e);
> > >
> > >      struct match m = MATCH_CATCHALL_INITIALIZER;
> > > diff --git a/tests/ovn.at b/tests/ovn.at
> > > index bc494fcad..e6d609b5b 100644
> > > --- a/tests/ovn.at
> > > +++ b/tests/ovn.at
> > > @@ -26782,3 +26782,50 @@ AT_CHECK([ovs-ofctl dump-flows br-int "nw_src=
> 10.0.0.0/24" | \
> > >  OVN_CLEANUP([hv1])
> > >  AT_CLEANUP
> > >  ])
> > > +
> > > +# Test when a logical port name is used in an ACL before it is
> created. When it
> > > +# is created later, the ACL should be programmed as openflow rules by
> > > +# ovn-controller. Although this is not likely to happen in real world
> use
> > > +# cases, it is possible that a port-binding is observed by
> ovn-controller AFTER
> > > +# an lflow that references the port is processed. So this test case is
> to make
> > > +# sure the incremental processing in ovn-controller reprocesses the
> lflow when
> > > +# the port-binding is observed.
> > > +OVN_FOR_EACH_NORTHD([
> > > +AT_SETUP([ovn -- ACL referencing lport before creation])
> > > +ovn_start
> > > +
> > > +net_add n1
> > > +
> > > +sim_add hv1
> > > +as hv1
> > > +check ovs-vsctl add-br br-phys
> > > +ovn_attach n1 br-phys 192.168.0.1
> > > +
> > > +# Bind both lp1 and lp2 on the chasis.
> > > +check ovs-vsctl add-port br-int lp1 -- set interface lp1
> external_ids:iface-id=lp1
> > > +check ovs-vsctl add-port br-int lp2 -- set interface lp2
> external_ids:iface-id=lp2
> > > +
> > > +# Create only lport lp1, but not lp2.
> > > +check ovn-nbctl ls-add lsw0
> > > +check ovn-nbctl lsp-add lsw0 lp1 \
> > > +    -- lsp-set-addresses lp1 "f0:00:00:00:00:01 10.0.0.11"
> > > +
> > > +# Each lport is referenced by an ACL.
> > > +check ovn-nbctl acl-add lsw0 to-lport 1002 'outport == "lp1" &&
> ip4.src == 10.0.0.111'  allow-related
> > > +check ovn-nbctl acl-add lsw0 to-lport 1002 'outport == "lp2" &&
> ip4.src == 10.0.0.222'  allow-related
> > > +
> > > +# The first ACL should be programmed.
> > > +check ovn-nbctl --wait=hv sync
> > > +AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.111],
> [0], [ignore])
> > > +
> > > +# Now create the lport lp2.
> > > +check ovn-nbctl lsp-add lsw0 lp2 \
> > > +    -- lsp-set-addresses lp2 "f0:00:00:00:00:02 10.0.0.22"
> > > +
> > > +check ovn-nbctl --wait=hv sync
> > > +# Now the second ACL should be programmed.
> > > +AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.222],
> [0], [ignore])
> > > +
> > > +OVN_CLEANUP([hv1])
> > > +AT_CLEANUP
> > > +])
> > > diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> > > index 98cc2c503..c6d8b287b 100644
> > > --- a/tests/test-ovn.c
> > > +++ b/tests/test-ovn.c
> > > @@ -318,7 +318,7 @@ test_parse_expr__(int steps)
> > >              if (steps > 1) {
> > >                  expr = expr_simplify(expr);
> > >                  expr = expr_evaluate_condition(expr,
> is_chassis_resident_cb,
> > > -                                               &ports, NULL);
> > > +                                               &ports);
> > >              }
> > >              if (steps > 2) {
> > >                  expr = expr_normalize(expr);
> > > @@ -922,7 +922,7 @@ test_tree_shape_exhaustively(struct expr *expr,
> struct shash *symtab,
> > >              modified = expr_simplify(expr_clone(expr));
> > >              modified = expr_evaluate_condition(
> > >                  modified, tree_shape_is_chassis_resident_cb,
> > > -                NULL, NULL);
> > > +                NULL);
> > >              ovs_assert(expr_honors_invariants(modified));
> > >
> > >              if (operation >= OP_NORMALIZE) {
> > > diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> > > index 49463c5c2..5d016b757 100644
> > > --- a/utilities/ovn-trace.c
> > > +++ b/utilities/ovn-trace.c
> > > @@ -973,7 +973,7 @@ parse_lflow_for_datapath(const struct
> sbrec_logical_flow *sblf,
> > >              match = expr_simplify(match);
> > >              match = expr_evaluate_condition(match,
> > >
>  ovntrace_is_chassis_resident,
> > > -                                            NULL, NULL);
> > > +                                            NULL);
> > >          }
> > >
> > >          struct ovntrace_flow *flow = xzalloc(sizeof *flow);
> > > --
> > > 2.30.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 Thu, Jun 24, 2021 at 7:46 AM Numan Siddique <numans@ovn.org> wrote:
>
> On Wed, Jun 23, 2021 at 8:10 PM Han Zhou <hzhou@ovn.org> wrote:
> >
> > On Wed, Jun 23, 2021 at 7:48 AM Numan Siddique <numans@ovn.org> wrote:
> > >
> > > On Mon, Jun 21, 2021 at 2:52 AM Han Zhou <hzhou@ovn.org> wrote:
> > > >
> > > > If a lflow has an lport name in the match, but when the lflow is
> > > > processed the port-binding is not seen by ovn-controller, the
> > > > corresponding openflow will not be created. Later if the
port-binding is
> > > > created/monitored by ovn-controller, the lflow is not reprocessed
> > > > because the lflow didn't change and ovn-controller doesn't know
that the
> > > > port-binding affects the lflow. This patch fixes the problem by
tracking
> > > > the references when parsing the lflow, even if the port-binding is
not
> > > > found when the lflow is firstly parsed. A test case is also added to
> > > > cover the scenario.
> > > >
> > > > Signed-off-by: Han Zhou <hzhou@ovn.org>
> > >
> > > Hi Han,
> > >
> > > Thanks for fixing these issues.   I've a few questions.  I haven't
> > > reviewed the patch completely.
> > >
> > >
> > > > ---
> > > >  controller/lflow.c          | 63
++++++++++++++++++++++++++-----------
> > > >  controller/lflow.h          |  3 ++
> > > >  controller/ovn-controller.c | 35 ++++++++++++++++-----
> > > >  include/ovn/expr.h          |  2 +-
> > > >  lib/expr.c                  | 14 +++------
> > > >  tests/ovn.at                | 47 +++++++++++++++++++++++++++
> > > >  tests/test-ovn.c            |  4 +--
> > > >  utilities/ovn-trace.c       |  2 +-
> > > >  8 files changed, 132 insertions(+), 38 deletions(-)
> > > >
> > > > diff --git a/controller/lflow.c b/controller/lflow.c
> > > > index 34eca135a..b7699a309 100644
> > > > --- a/controller/lflow.c
> > > > +++ b/controller/lflow.c
> > > > @@ -61,6 +61,7 @@ struct lookup_port_aux {
> > > >
> > > >  struct condition_aux {
> > > >      struct ovsdb_idl_index *sbrec_port_binding_by_name;
> > > > +    const struct sbrec_datapath_binding *dp;
> > > >      const struct sbrec_chassis *chassis;
> > > >      const struct sset *active_tunnels;
> > > >      const struct sbrec_logical_flow *lflow;
> > > > @@ -98,6 +99,12 @@ lookup_port_cb(const void *aux_, const char
> > *port_name, unsigned int *portp)
> > > >
> > > >      const struct lookup_port_aux *aux = aux_;
> > > >
> > > > +    /* Store the name that used to lookup the lport to lflow
> > reference, so that
> > > > +     * in the future when the lport's port binding changes, the
> > logical flow
> > > > +     * that references this lport can be reprocessed. */
> > > > +    lflow_resource_add(aux->lfrr, REF_TYPE_PORTBINDING, port_name,
> > > > +                       &aux->lflow->header_.uuid);
> > > > +
> > > >      const struct sbrec_port_binding *pb
> > > >          = lport_lookup_by_name(aux->sbrec_port_binding_by_name,
> > port_name);
> > > >      if (pb && pb->datapath == aux->dp) {
> > > > @@ -149,19 +156,18 @@ is_chassis_resident_cb(const void *c_aux_,
const
> > char *port_name)
> > > >  {
> > > >      const struct condition_aux *c_aux = c_aux_;
> > > >
> > > > +    /* Store the port name that used to lookup the lport to lflow
> > reference, so
> > > > +     * that in the future when the lport's port-binding changes the
> > logical
> > > > +     * flow that references this lport can be reprocessed. */
> > > > +    lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING,
port_name,
> > > > +                       &c_aux->lflow->header_.uuid);
> > > > +
> > > >      const struct sbrec_port_binding *pb
> > > >          = lport_lookup_by_name(c_aux->sbrec_port_binding_by_name,
> > port_name);
> > > >      if (!pb) {
> > > >          return false;
> > > >      }
> > > >
> > > > -    /* Store the port_name to lflow reference. */
> > > > -    int64_t dp_id = pb->datapath->tunnel_key;
> > > > -    char buf[16];
> > > > -    get_unique_lport_key(dp_id, pb->tunnel_key, buf, sizeof(buf));
> > > > -    lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, buf,
> > > > -                       &c_aux->lflow->header_.uuid);
> > > > -
> > > >      if (strcmp(pb->type, "chassisredirect")) {
> > > >          /* for non-chassisredirect ports */
> > > >          return pb->chassis && pb->chassis == c_aux->chassis;
> > > > @@ -623,8 +629,6 @@ add_matches_to_flow_table(const struct
> > sbrec_logical_flow *lflow,
> > > >                  int64_t dp_id = dp->tunnel_key;
> > > >                  char buf[16];
> > > >                  get_unique_lport_key(dp_id, port_id, buf,
sizeof(buf));
> > > > -                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",
> > > > @@ -788,6 +792,7 @@ consider_logical_flow__(const struct
> > sbrec_logical_flow *lflow,
> > > >      };
> > > >      struct condition_aux cond_aux = {
> > > >          .sbrec_port_binding_by_name =
> > l_ctx_in->sbrec_port_binding_by_name,
> > > > +        .dp = dp,
> > > >          .chassis = l_ctx_in->chassis,
> > > >          .active_tunnels = l_ctx_in->active_tunnels,
> > > >          .lflow = lflow,
> > > > @@ -805,7 +810,6 @@ consider_logical_flow__(const struct
> > sbrec_logical_flow *lflow,
> > > >      struct hmap *matches = NULL;
> > > >      size_t matches_size = 0;
> > > >
> > > > -    bool is_cr_cond_present = false;
> > > >      bool pg_addr_set_ref = false;
> > > >      uint32_t n_conjs = 0;
> > > >
> > > > @@ -843,8 +847,8 @@ consider_logical_flow__(const struct
> > sbrec_logical_flow *lflow,
> > > >      case LCACHE_T_NONE:
> > > >      case LCACHE_T_CONJ_ID:
> > > >      case LCACHE_T_EXPR:
> > > > -        expr = expr_evaluate_condition(expr,
is_chassis_resident_cb,
> > &cond_aux,
> > > > -                                       &is_cr_cond_present);
> > >
> > > I'm not very clear why the variable "is_cr_cond_present" not required
any
> > more.
> > >
> > > For logical flows with "is_chassis_resident" match condition, we cache
> > > only the "expr" right ?
> > >
> > > What is the behavior after this patch ?
> > >
> > Thanks Numan for the review!
> >
> > After this patch, it is the same for "is_chassis_resident", which we
cache
> > only the "expr" just like before.
> > However, it changes the cache behavior for lflows that reference logical
> > ports. Some of the lflows may previously cache the "match", but now they
> > will cache "expr" only. This is because the logical port parsing is done
> > when converting expr to matches. So we can't cache the match. Otherwise
the
> > test case would fail.
> >
> > >
> > > > +        expr = expr_evaluate_condition(expr,
is_chassis_resident_cb,
> > > > +                                       &cond_aux);
> > > >          expr = expr_normalize(expr);
> > > >          break;
> > > >      case LCACHE_T_MATCHES:
> > > > @@ -883,7 +887,9 @@ consider_logical_flow__(const struct
> > sbrec_logical_flow *lflow,
> > > >
> > > >          /* Cache new entry if caching is enabled. */
> > > >          if (lflow_cache_is_enabled(l_ctx_out->lflow_cache)) {
> > > > -            if (cached_expr && !is_cr_cond_present) {
> > > > +            if (cached_expr
> > > > +                &&
!lflow_ref_lookup(&l_ctx_out->lfrr->lflow_ref_table,
> > > > +                                     &lflow->header_.uuid)) {
> > >
> > > This check - lflow_ref_lookup() is not very clear to me.  Can you
> > > please explain a bit about this ?
> > >
> > > From what I understand we cache the expr matches if the flow is not
> > > referenced by resources.
> > >
> > > Before this patch we were caching the 'expr matches' for a logical
> > > flow like - 'ip4 && inport == "lp1"'
> > > But now since there is a reference for this logical flow in the
> > > lflow_ref table, we only cache the expr tree.
> > > Is this intentional ?  Correct me if my understanding is wrong.
> >
> > Yes this is intentional as explained above. We can't cache the match
> > whenever a lport is referenced by the logical flow, which includes
> > "is_chassis_resident" and other lport references as well.
> > There is a performance impact of the effectiveness of lflow-cache. I
reran
> > the scale test with a setup closer to an ovn-k8s setup with 500
chassises
> > and 5 LSPs per chassis, and GRs, some ACLs with port-group/address-set
size
> > 1k, and some LBs, etc. On my test machine the time spent by
ovn-controller
> > for recompute increased from 2.9s to 3.2s, around 10% increase. (of
course
> > the memory cost is much lower, around 40% less). I didn't notice this
> > obvious performance difference from an earlier test when I was replying
to
> > Dumitru, probably because I messed up with the lflow-cache flag
settings.
> > (I reran the tests multiple times and results are consistent so I
believe
> > there were mistakes in my earlier test).
> >
> > Probably this ~10% difference is still not that much for the recompute
> > scenario from my perspective, while the scenario I am trying to fix may
not
> > be critical either, and I am not very sure if this not-so-critical fix
is
> > worth the not-so-critical performance cost. I just feel it is better to
be
> > correct, unless we know exactly what are the corner cases that would
lead
> > to a lflow being processed before the port-binding in SB is seen by the
> > ovn-controller. The test case I crafted is one but not sure if there are
> > others. Let me know your thoughts on this.
> >
> > As a note, I also retested both versions with lflow-cache disabled, and
the
> > performance of recompute with this patch is in fact slightly better ~2%
for
> > the same scenario. In our current deployment we have critical concerns
for
> > memory, so didn't enable lflow-cache for now.
>
>
>
> Thanks for the detailed explanation.
>
> So we only cache the expr tree for the flows which match on a logical port
> and that logical port is not seen yet.  I guess this should be fine.
>
This is half correct. We can't cache the match even if the logical port is
seen, because if it disappears we need to uninstall the flow. So whenever a
port-binding is added or deleted we need to reprocess the lflows that
reference the logical port. I think I can update the test case to cover
this as well.

>
> Instead of doing
>
>  if (cached_expr  && !lflow_ref_lookup(&l_ctx_out->lfrr->lflow_ref_table,
>
> &lflow->header_.uuid))
>
> We could probably do something like
>
> if (cached_expr && !is_cr_cond_present && lookup_port_succeeded) {
>    // cache expr matches ..
> } else {
>   //  cache expr tree
> }
> ..
> ..
>
>
> lookup_port_cb() can probably set lookup_port_succeeded to false if
> the lookup failed.
>
> This would avoid the lflow_ref_lookup().
>

For the reason mentioned above, we can't make this change. In fact, I
wouldn't worry much about lflow_ref_lookup()'s cost. It is O(1) operation.
If it really turns out to be a bottleneck, we could optimize the
function/data-structure, without worrying about the logic.
The real performance impact part is probably not being able to cache the
"match" for lflows that have logical port references, but I will work on
some other solutions to optimize that.

> > >
> > >
> > > >                  lflow_cache_add_matches(l_ctx_out->lflow_cache,
> > > >                                          &lflow->header_.uuid,
matches,
> > > >                                          matches_size);
> > > > @@ -1746,21 +1752,42 @@ lflow_processing_end:
> > > >      return handled;
> > > >  }
> > > >
> > > > +/* Handles a port-binding change that is possibly related to a
lport's
> > > > + * residence status on this chassis. */
> > > >  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)
> > > >  {
> > > >      bool changed;
> > > > -    int64_t dp_id = pb->datapath->tunnel_key;
> > > > -    char pb_ref_name[16];
> > > > -    get_unique_lport_key(dp_id, pb->tunnel_key, pb_ref_name,
> > > > -                         sizeof(pb_ref_name));
> > > >
> > > > -    return lflow_handle_changed_ref(REF_TYPE_PORTBINDING,
pb_ref_name,
> > > > +    return lflow_handle_changed_ref(REF_TYPE_PORTBINDING,
> > pb->logical_port,
> > > >                                      l_ctx_in, l_ctx_out, &changed);
> > > >  }
> > > >
> > > > +/* Handles port-binding add/deletions. */
> > > > +bool
> > > > +lflow_handle_changed_port_bindings(struct lflow_ctx_in *l_ctx_in,
> > > > +                                   struct lflow_ctx_out *l_ctx_out)
> > > > +{
> > > > +    bool ret = true;
> > > > +    bool changed;
> > > > +    const struct sbrec_port_binding *pb;
> > > > +    SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb,
> > > > +
> > l_ctx_in->port_binding_table) {
> > > > +        if (!sbrec_port_binding_is_new(pb)
> > > > +            && !sbrec_port_binding_is_deleted(pb)) {
> > > > +            continue;
> > > > +        }
> > > > +        if (!lflow_handle_changed_ref(REF_TYPE_PORTBINDING,
> > pb->logical_port,
> > > > +                                      l_ctx_in, l_ctx_out,
&changed)) {
> > > > +            ret = false;
> > > > +            break;
> > > > +        }
> > > > +    }
> > > > +    return ret;
> > > > +}
> > > > +
> > >
> > > One downside of adding the port binding handler is that we will
process
> > > a logical flow 'A' twice if a port binding is created and it is
> > > claimed in the same loop
> > > as the added test case does for the port - "lp2".
> > >
> > > We do that first in lflow_handle_flows_for_lport() and then in
> > > lflow_handle_changed_port_bindings().
> > >
> > Yes, in such cases the lflow will be handled twice. However, it is a
> > general problem in I-P engine for different input changes in the same
> > iteration. Although it is not a critical penalty in most cases because
of
> > I-P, I am still trying to optimize this to make sure a lflow is handled
at
> > most once in one engine run, so that for lflows with a big
> > port-group/address-set reference won't cause more latency in I-P.
> >
> > > I'm also not very clear why we need the port_binding handler here ?
> > > Can you please give a scenario for this ?
> > > The added test case passes for me even If I make the function
> > > lflow_handle_changed_port_bindings()
> > > return at the beginning without doing anything.
> >
> > The lflow_handle_flows_for_lport() is called only when a port-binding is
> > affecting local residency, while lflow_handle_changed_port_bindings()
takes
> > care of all port-binding adding/deletions, including the ports bound on
> > other chassises. The test case I used only one HV so it didn't matter
even
> > if you returned directly. (but the current test case fails without this
> > patch). Probably I should update the test with two HVs to cover the
> > scenario better.
>
> Ok.  I think it would be great to enhance the test with two HVs.

Ok, will do.

Thanks,
han

>
> Thanks
> Numan
>
> >
> > Thanks,
> > Han
> >
> > >
> > > Thanks
> > > Numan
> > >
> > > >  bool
> > > >  lflow_handle_changed_mc_groups(struct lflow_ctx_in *l_ctx_in,
> > > >                                 struct lflow_ctx_out *l_ctx_out)
> > > > diff --git a/controller/lflow.h b/controller/lflow.h
> > > > index e98edf81d..9d8882ae5 100644
> > > > --- a/controller/lflow.h
> > > > +++ b/controller/lflow.h
> > > > @@ -130,6 +130,7 @@ struct lflow_ctx_in {
> > > >      struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath;
> > > >      struct ovsdb_idl_index *sbrec_logical_flow_by_logical_dp_group;
> > > >      struct ovsdb_idl_index *sbrec_port_binding_by_name;
> > > > +    const struct sbrec_port_binding_table *port_binding_table;
> > > >      const struct sbrec_dhcp_options_table *dhcp_options_table;
> > > >      const struct sbrec_dhcpv6_options_table *dhcpv6_options_table;
> > > >      const struct sbrec_datapath_binding_table *dp_binding_table;
> > > > @@ -182,4 +183,6 @@ bool lflow_handle_flows_for_lport(const struct
> > sbrec_port_binding *,
> > > >                                    struct lflow_ctx_out *);
> > > >  bool lflow_handle_changed_mc_groups(struct lflow_ctx_in *,
> > > >                                      struct lflow_ctx_out *);
> > > > +bool lflow_handle_changed_port_bindings(struct lflow_ctx_in *,
> > > > +                                        struct lflow_ctx_out *);
> > > >  #endif /* controller/lflow.h */
> > > > diff --git a/controller/ovn-controller.c
b/controller/ovn-controller.c
> > > > index c28fd6f2d..8136afe5c 100644
> > > > --- a/controller/ovn-controller.c
> > > > +++ b/controller/ovn-controller.c
> > > > @@ -1966,6 +1966,10 @@ init_lflow_ctx(struct engine_node *node,
> > > >                  engine_get_input("SB_multicast_group", node),
> > > >                  "name_datapath");
> > > >
> > > > +    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_dhcp_options_table *dhcp_table =
> > > >          (struct sbrec_dhcp_options_table *)EN_OVSDB_GET(
> > > >              engine_get_input("SB_dhcp_options", node));
> > > > @@ -2029,6 +2033,7 @@ init_lflow_ctx(struct engine_node *node,
> > > >      l_ctx_in->sbrec_logical_flow_by_logical_dp_group =
> > > >          sbrec_logical_flow_by_dp_group;
> > > >      l_ctx_in->sbrec_port_binding_by_name =
sbrec_port_binding_by_name;
> > > > +    l_ctx_in->port_binding_table = port_binding_table;
> > > >      l_ctx_in->dhcp_options_table  = dhcp_table;
> > > >      l_ctx_in->dhcpv6_options_table = dhcpv6_table;
> > > >      l_ctx_in->mac_binding_table = mac_binding_table;
> > > > @@ -2221,6 +2226,25 @@
lflow_output_sb_multicast_group_handler(struct
> > engine_node *node, void *data)
> > > >      return true;
> > > >  }
> > > >
> > > > +static bool
> > > > +lflow_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_lflow_output *lfo = data;
> > > > +
> > > > +    struct lflow_ctx_in l_ctx_in;
> > > > +    struct lflow_ctx_out l_ctx_out;
> > > > +    init_lflow_ctx(node, rt_data, lfo, &l_ctx_in, &l_ctx_out);
> > > > +    if (!lflow_handle_changed_port_bindings(&l_ctx_in,
&l_ctx_out)) {
> > > > +        return false;
> > > > +    }
> > > > +
> > > > +    engine_set_node_state(node, EN_UPDATED);
> > > > +    return true;
> > > > +}
> > > > +
> > > >  static bool
> > > >  _lflow_output_resource_ref_handler(struct engine_node *node, void
> > *data,
> > > >                                    enum ref_type ref_type)
> > > > @@ -2285,8 +2309,9 @@ _lflow_output_resource_ref_handler(struct
> > engine_node *node, void *data,
> > > >              break;
> > > >
> > > >          case REF_TYPE_PORTBINDING:
> > > > -            /* This ref type is handled in the
> > > > -             * flow_output_runtime_data_handler. */
> > > > +            /* This ref type is handled in:
> > > > +             * - flow_output_runtime_data_handler
> > > > +             * - flow_output_sb_port_binding_handler. */
> > > >          case REF_TYPE_MC_GROUP:
> > > >              /* This ref type is handled in the
> > > >               * flow_output_sb_multicast_group_handler. */
> > > > @@ -2895,12 +2920,8 @@ main(int argc, char *argv[])
> > > >      engine_add_input(&en_lflow_output, &en_sb_chassis,
> > > >                       pflow_lflow_output_sb_chassis_handler);
> > > >
> > > > -    /* Any changes to the port binding, need not be handled
> > > > -     * for lflow_outout engine.  We still need sb_port_binding
> > > > -     * as input to access the port binding data in lflow.c and
> > > > -     * hence the noop handler. */
> > > >      engine_add_input(&en_lflow_output, &en_sb_port_binding,
> > > > -                     engine_noop_handler);
> > > > +                     lflow_output_sb_port_binding_handler);
> > > >
> > > >      engine_add_input(&en_lflow_output, &en_ovs_open_vswitch, NULL);
> > > >      engine_add_input(&en_lflow_output, &en_ovs_bridge, NULL);
> > > > diff --git a/include/ovn/expr.h b/include/ovn/expr.h
> > > > index de90e87e1..3b5653f7b 100644
> > > > --- a/include/ovn/expr.h
> > > > +++ b/include/ovn/expr.h
> > > > @@ -438,7 +438,7 @@ struct expr *expr_evaluate_condition(
> > > >      struct expr *,
> > > >      bool (*is_chassis_resident)(const void *c_aux,
> > > >                                  const char *port_name),
> > > > -    const void *c_aux, bool *condition_present);
> > > > +    const void *c_aux);
> > > >  struct expr *expr_normalize(struct expr *);
> > > >
> > > >  bool expr_honors_invariants(const struct expr *);
> > > > diff --git a/lib/expr.c b/lib/expr.c
> > > > index f728f9537..e3f6bb892 100644
> > > > --- a/lib/expr.c
> > > > +++ b/lib/expr.c
> > > > @@ -2121,16 +2121,13 @@ static struct expr *
> > > >  expr_evaluate_condition__(struct expr *expr,
> > > >                            bool (*is_chassis_resident)(const void
> > *c_aux,
> > > >                                                        const char
> > *port_name),
> > > > -                          const void *c_aux, bool
*condition_present)
> > > > +                          const void *c_aux)
> > > >  {
> > > >      bool result;
> > > >
> > > >      switch (expr->cond.type) {
> > > >      case EXPR_COND_CHASSIS_RESIDENT:
> > > >          result = is_chassis_resident(c_aux, expr->cond.string);
> > > > -        if (condition_present != NULL) {
> > > > -            *condition_present = true;
> > > > -        }
> > > >          break;
> > > >
> > > >      default:
> > > > @@ -2146,7 +2143,7 @@ struct expr *
> > > >  expr_evaluate_condition(struct expr *expr,
> > > >                          bool (*is_chassis_resident)(const void
*c_aux,
> > > >                                                      const char
> > *port_name),
> > > > -                        const void *c_aux, bool *condition_present)
> > > > +                        const void *c_aux)
> > > >  {
> > > >      struct expr *sub, *next;
> > > >
> > > > @@ -2156,15 +2153,14 @@ expr_evaluate_condition(struct expr *expr,
> > > >           LIST_FOR_EACH_SAFE (sub, next, node, &expr->andor) {
> > > >              ovs_list_remove(&sub->node);
> > > >              struct expr *e = expr_evaluate_condition(sub,
> > is_chassis_resident,
> > > > -                                                     c_aux,
> > condition_present);
> > > > +                                                     c_aux);
> > > >              e = expr_fix(e);
> > > >              expr_insert_andor(expr, next, e);
> > > >          }
> > > >          return expr_fix(expr);
> > > >
> > > >      case EXPR_T_CONDITION:
> > > > -        return expr_evaluate_condition__(expr, is_chassis_resident,
> > c_aux,
> > > > -                                         condition_present);
> > > > +        return expr_evaluate_condition__(expr, is_chassis_resident,
> > c_aux);
> > > >
> > > >      case EXPR_T_CMP:
> > > >      case EXPR_T_BOOLEAN:
> > > > @@ -3517,7 +3513,7 @@ expr_parse_microflow__(struct lexer *lexer,
> > > >
> > > >      e = expr_simplify(e);
> > > >      e = expr_evaluate_condition(e,
microflow_is_chassis_resident_cb,
> > > > -                                NULL, NULL);
> > > > +                                NULL);
> > > >      e = expr_normalize(e);
> > > >
> > > >      struct match m = MATCH_CATCHALL_INITIALIZER;
> > > > diff --git a/tests/ovn.at b/tests/ovn.at
> > > > index bc494fcad..e6d609b5b 100644
> > > > --- a/tests/ovn.at
> > > > +++ b/tests/ovn.at
> > > > @@ -26782,3 +26782,50 @@ AT_CHECK([ovs-ofctl dump-flows br-int
"nw_src=
> > 10.0.0.0/24" | \
> > > >  OVN_CLEANUP([hv1])
> > > >  AT_CLEANUP
> > > >  ])
> > > > +
> > > > +# Test when a logical port name is used in an ACL before it is
> > created. When it
> > > > +# is created later, the ACL should be programmed as openflow rules
by
> > > > +# ovn-controller. Although this is not likely to happen in real
world
> > use
> > > > +# cases, it is possible that a port-binding is observed by
> > ovn-controller AFTER
> > > > +# an lflow that references the port is processed. So this test
case is
> > to make
> > > > +# sure the incremental processing in ovn-controller reprocesses the
> > lflow when
> > > > +# the port-binding is observed.
> > > > +OVN_FOR_EACH_NORTHD([
> > > > +AT_SETUP([ovn -- ACL referencing lport before creation])
> > > > +ovn_start
> > > > +
> > > > +net_add n1
> > > > +
> > > > +sim_add hv1
> > > > +as hv1
> > > > +check ovs-vsctl add-br br-phys
> > > > +ovn_attach n1 br-phys 192.168.0.1
> > > > +
> > > > +# Bind both lp1 and lp2 on the chasis.
> > > > +check ovs-vsctl add-port br-int lp1 -- set interface lp1
> > external_ids:iface-id=lp1
> > > > +check ovs-vsctl add-port br-int lp2 -- set interface lp2
> > external_ids:iface-id=lp2
> > > > +
> > > > +# Create only lport lp1, but not lp2.
> > > > +check ovn-nbctl ls-add lsw0
> > > > +check ovn-nbctl lsp-add lsw0 lp1 \
> > > > +    -- lsp-set-addresses lp1 "f0:00:00:00:00:01 10.0.0.11"
> > > > +
> > > > +# Each lport is referenced by an ACL.
> > > > +check ovn-nbctl acl-add lsw0 to-lport 1002 'outport == "lp1" &&
> > ip4.src == 10.0.0.111'  allow-related
> > > > +check ovn-nbctl acl-add lsw0 to-lport 1002 'outport == "lp2" &&
> > ip4.src == 10.0.0.222'  allow-related
> > > > +
> > > > +# The first ACL should be programmed.
> > > > +check ovn-nbctl --wait=hv sync
> > > > +AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.111],
> > [0], [ignore])
> > > > +
> > > > +# Now create the lport lp2.
> > > > +check ovn-nbctl lsp-add lsw0 lp2 \
> > > > +    -- lsp-set-addresses lp2 "f0:00:00:00:00:02 10.0.0.22"
> > > > +
> > > > +check ovn-nbctl --wait=hv sync
> > > > +# Now the second ACL should be programmed.
> > > > +AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.222],
> > [0], [ignore])
> > > > +
> > > > +OVN_CLEANUP([hv1])
> > > > +AT_CLEANUP
> > > > +])
> > > > diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> > > > index 98cc2c503..c6d8b287b 100644
> > > > --- a/tests/test-ovn.c
> > > > +++ b/tests/test-ovn.c
> > > > @@ -318,7 +318,7 @@ test_parse_expr__(int steps)
> > > >              if (steps > 1) {
> > > >                  expr = expr_simplify(expr);
> > > >                  expr = expr_evaluate_condition(expr,
> > is_chassis_resident_cb,
> > > > -                                               &ports, NULL);
> > > > +                                               &ports);
> > > >              }
> > > >              if (steps > 2) {
> > > >                  expr = expr_normalize(expr);
> > > > @@ -922,7 +922,7 @@ test_tree_shape_exhaustively(struct expr *expr,
> > struct shash *symtab,
> > > >              modified = expr_simplify(expr_clone(expr));
> > > >              modified = expr_evaluate_condition(
> > > >                  modified, tree_shape_is_chassis_resident_cb,
> > > > -                NULL, NULL);
> > > > +                NULL);
> > > >              ovs_assert(expr_honors_invariants(modified));
> > > >
> > > >              if (operation >= OP_NORMALIZE) {
> > > > diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> > > > index 49463c5c2..5d016b757 100644
> > > > --- a/utilities/ovn-trace.c
> > > > +++ b/utilities/ovn-trace.c
> > > > @@ -973,7 +973,7 @@ parse_lflow_for_datapath(const struct
> > sbrec_logical_flow *sblf,
> > > >              match = expr_simplify(match);
> > > >              match = expr_evaluate_condition(match,
> > > >
> >  ovntrace_is_chassis_resident,
> > > > -                                            NULL, NULL);
> > > > +                                            NULL);
> > > >          }
> > > >
> > > >          struct ovntrace_flow *flow = xzalloc(sizeof *flow);
> > > > --
> > > > 2.30.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 6/24/21 7:20 PM, Han Zhou wrote:
> For the reason mentioned above, we can't make this change. In fact, I
> wouldn't worry much about lflow_ref_lookup()'s cost. It is O(1) operation.
> If it really turns out to be a bottleneck, we could optimize the
> function/data-structure, without worrying about the logic.
> The real performance impact part is probably not being able to cache the
> "match" for lflows that have logical port references, but I will work on
> some other solutions to optimize that.

OTOH, on real deployments the lflow cache limits should be enforced by
the CMS.  Therefore I would expect some of these flows to not make it in
the cache anyway (even without your change).  I don't have data to back
this up but I'm guessing the impact of the change in this patch will be
minimal.

Regards,
Dumitru

On Thu, Jun 24, 2021 at 10:33 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 6/24/21 7:20 PM, Han Zhou wrote:
> > For the reason mentioned above, we can't make this change. In fact, I
> > wouldn't worry much about lflow_ref_lookup()'s cost. It is O(1)
operation.
> > If it really turns out to be a bottleneck, we could optimize the
> > function/data-structure, without worrying about the logic.
> > The real performance impact part is probably not being able to cache the
> > "match" for lflows that have logical port references, but I will work on
> > some other solutions to optimize that.
>
> OTOH, on real deployments the lflow cache limits should be enforced by
> the CMS.  Therefore I would expect some of these flows to not make it in
> the cache anyway (even without your change).  I don't have data to back
> this up but I'm guessing the impact of the change in this patch will be
> minimal.
>
> Regards,
> Dumitru
>

Thanks Dumitru.
Numan, I sent v4 that adds more coverage in the test case. Please take a
look:
https://patchwork.ozlabs.org/project/ovn/patch/20210629192257.1699504-1-hzhou@ovn.org/

Thanks,
Han