diff mbox series

[ovs-dev,v2,11/11] ovn-controller: Handle addresses addition in address set incrementally.

Message ID 20220209175450.1179778-12-hzhou@ovn.org
State Accepted
Headers show
Series ovn-controller: Fine-grained address set incremental processing. | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test fail github build: failed
ovsrobot/github-robot-_ovn-kubernetes fail github build: failed

Commit Message

Han Zhou Feb. 9, 2022, 5:54 p.m. UTC 
To avoid reprocessing the lflow when a referenced address set has new
addresses added, this patch generates a fake address set that only
contains the added addresses for flow generation, and then eliminates
the flows that are not related to the newly added addresses.

Scale test shows obvious performance gains because the time complexity
changed from O(n) to O(1). The bigger the size of address set, the more
CPU savings. With the AS size of 10k, the test shows ~40x speed up.

Test setup:
CPU: Intel(R) Core(TM) i9-7920X CPU @ 2.90GHz.
5 ACL all referencing an address set of 10,000 IPs.

Measure the time spent by ovn-controller for handling one IP
addition from the address set.

Before: ~400ms
After: 11-12ms

Signed-off-by: Han Zhou <hzhou@ovn.org>
---
 controller/lflow-conj-ids.c |  20 ++
 controller/lflow-conj-ids.h |   1 +
 controller/lflow.c          | 360 ++++++++++++++++++++++++++++++++++--
 controller/ovn-controller.c |   8 +
 include/ovn/expr.h          |   1 +
 lib/expr.c                  |   2 +-
 tests/ovn-controller.at     |  41 ++--
 7 files changed, 397 insertions(+), 36 deletions(-)

Comments

0-day Robot Feb. 9, 2022, 7:27 p.m. UTC | #1 
Bleep bloop.  Greetings Han Zhou, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Comment with 'xxx' marker
#233 FILE: controller/lflow.c:655:
     * XXX: if necessary, we can optimize this by checking all the address set

Lines checked: 721, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Numan Siddique Feb. 23, 2022, 10:44 p.m. UTC | #2 
/

On Wed, Feb 9, 2022 at 12:56 PM Han Zhou <hzhou@ovn.org> wrote:
>
> To avoid reprocessing the lflow when a referenced address set has new
> addresses added, this patch generates a fake address set that only
> contains the added addresses for flow generation, and then eliminates
> the flows that are not related to the newly added addresses.
>
> Scale test shows obvious performance gains because the time complexity
> changed from O(n) to O(1). The bigger the size of address set, the more
> CPU savings. With the AS size of 10k, the test shows ~40x speed up.
>
> Test setup:
> CPU: Intel(R) Core(TM) i9-7920X CPU @ 2.90GHz.
> 5 ACL all referencing an address set of 10,000 IPs.
>
> Measure the time spent by ovn-controller for handling one IP
> addition from the address set.
>
> Before: ~400ms
> After: 11-12ms
>
> Signed-off-by: Han Zhou <hzhou@ovn.org>

Hi Han,

I found a couple of small typos which you can consider fixing while
applying the patches.

Please see the patch 0 which has my Ack.

Thanks
Numan

> ---
>  controller/lflow-conj-ids.c |  20 ++
>  controller/lflow-conj-ids.h |   1 +
>  controller/lflow.c          | 360 ++++++++++++++++++++++++++++++++++--
>  controller/ovn-controller.c |   8 +
>  include/ovn/expr.h          |   1 +
>  lib/expr.c                  |   2 +-
>  tests/ovn-controller.at     |  41 ++--
>  7 files changed, 397 insertions(+), 36 deletions(-)
>
> diff --git a/controller/lflow-conj-ids.c b/controller/lflow-conj-ids.c
> index bfe63862a..372391e5a 100644
> --- a/controller/lflow-conj-ids.c
> +++ b/controller/lflow-conj-ids.c
> @@ -157,6 +157,26 @@ lflow_conj_ids_alloc_specified(struct conj_ids *conj_ids,
>      return true;
>  }
>
> +/* Find and return the start id that is allocated to the logical flow. Return
> + * 0 if not found. */
> +uint32_t
> +lflow_conj_ids_find(struct conj_ids *conj_ids, const struct uuid *lflow_uuid)
> +{
> +    struct lflow_conj_node *lflow_conj;
> +    bool found = false;
> +    HMAP_FOR_EACH_WITH_HASH (lflow_conj, hmap_node, uuid_hash(lflow_uuid),
> +                             &conj_ids->lflow_conj_ids) {
> +        if (uuid_equals(&lflow_conj->lflow_uuid, lflow_uuid)) {
> +            found = true;
> +            break;
> +        }
> +    }
> +    if (!found) {
> +        return 0;
> +    }
> +    return lflow_conj->start_conj_id;
> +}
> +
>  /* Frees the conjunction IDs used by lflow_uuid. */
>  void
>  lflow_conj_ids_free(struct conj_ids *conj_ids, const struct uuid *lflow_uuid)
> diff --git a/controller/lflow-conj-ids.h b/controller/lflow-conj-ids.h
> index 6da0a612c..a1f7a95a5 100644
> --- a/controller/lflow-conj-ids.h
> +++ b/controller/lflow-conj-ids.h
> @@ -35,6 +35,7 @@ bool lflow_conj_ids_alloc_specified(struct conj_ids *,
>                                      const struct uuid *lflow_uuid,
>                                      uint32_t start_conj_id, uint32_t n_conjs);
>  void lflow_conj_ids_free(struct conj_ids *, const struct uuid *lflow_uuid);
> +uint32_t lflow_conj_ids_find(struct conj_ids *, const struct uuid *lflow_uuid);
>  void lflow_conj_ids_init(struct conj_ids *);
>  void lflow_conj_ids_destroy(struct conj_ids *);
>  void lflow_conj_ids_clear(struct conj_ids *);
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 9b1184c0e..658b3109b 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -74,6 +74,19 @@ struct condition_aux {
>      struct lflow_resource_ref *lfrr;
>  };
>
> +static struct expr *
> +convert_match_to_expr(const struct sbrec_logical_flow *,
> +                      const struct sbrec_datapath_binding *,
> +                      struct expr **prereqs, const struct shash *addr_sets,
> +                      const struct shash *port_groups,
> +                      struct lflow_resource_ref *, bool *pg_addr_set_ref);
> +static void
> +add_matches_to_flow_table(const struct sbrec_logical_flow *,
> +                          const struct sbrec_datapath_binding *,
> +                          struct hmap *matches, uint8_t ptable,
> +                          uint8_t output_ptable, struct ofpbuf *ovnacts,
> +                          bool ingress, struct lflow_ctx_in *,
> +                          struct lflow_ctx_out *);
>  static void
>  consider_logical_flow(const struct sbrec_logical_flow *lflow,
>                        struct hmap *dhcp_opts, struct hmap *dhcpv6_opts,
> @@ -513,6 +526,262 @@ as_info_from_expr_const(const char *as_name, const union expr_constant *c,
>      return true;
>  }
>
> +/* Parses the lflow regarding the changed address set 'as_name', and gererates/
s/gererates/generates


> + * ovs flows for the newly added addresses in 'as_diff_added' only. It is
> + * similar to consider_logical_flow__, with the below differences:
> + *
> + * - It has one more arg 'as_ref_count' to deduce how many flows are expected
> + *   to be added.
> + * - It uses a small fake address set that contains only the added addresses
> + *   to replace the original address set temporarily and restores it after
> + *   parsing.
> + * - It doesn't check or touch lflow-cache, because lflow-cache is disabled
> + *   when address-sets/port-groups are used.
> + * - It doesn't check non-local lports because it should have been checked
> + *   when the lflow is initially parsed, and if it is non-local and skipped
> + *   then it wouldn't have the address set parsed and referenced.
> + *
> + * Because of these differences, it is just cleaner to keep it as a separate
> + * function. */
> +static bool
> +consider_lflow_for_added_as_ips__(
> +                        const struct sbrec_logical_flow *lflow,
> +                        const struct sbrec_datapath_binding *dp,
> +                        const char *as_name,
> +                        size_t as_ref_count,
> +                        const struct expr_constant_set *as_diff_added,
> +                        struct hmap *dhcp_opts,
> +                        struct hmap *dhcpv6_opts,
> +                        struct hmap *nd_ra_opts,
> +                        struct controller_event_options *controller_event_opts,
> +                        struct lflow_ctx_in *l_ctx_in,
> +                        struct lflow_ctx_out *l_ctx_out)
> +{
> +    bool handled = true;
> +    if (!get_local_datapath(l_ctx_in->local_datapaths, dp->tunnel_key)) {
> +        VLOG_DBG("Skip lflow "UUID_FMT" for non-local datapath %"PRId64,
> +                 UUID_ARGS(&lflow->header_.uuid), dp->tunnel_key);
> +        return true;
> +    }
> +
> +    /* Determine translation of logical table IDs to physical table IDs. */
> +    bool ingress = !strcmp(lflow->pipeline, "ingress");
> +
> +    /* Determine translation of logical table IDs to physical table IDs. */
> +    uint8_t first_ptable = (ingress
> +                            ? OFTABLE_LOG_INGRESS_PIPELINE
> +                            : OFTABLE_LOG_EGRESS_PIPELINE);
> +    uint8_t ptable = first_ptable + lflow->table_id;
> +    uint8_t output_ptable = (ingress
> +                             ? OFTABLE_REMOTE_OUTPUT
> +                             : OFTABLE_SAVE_INPORT);
> +
> +    uint64_t ovnacts_stub[1024 / 8];
> +    struct ofpbuf ovnacts = OFPBUF_STUB_INITIALIZER(ovnacts_stub);
> +    struct ovnact_parse_params pp = {
> +        .symtab = &symtab,
> +        .dhcp_opts = dhcp_opts,
> +        .dhcpv6_opts = dhcpv6_opts,
> +        .nd_ra_opts = nd_ra_opts,
> +        .controller_event_opts = controller_event_opts,
> +
> +        .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS,
> +        .n_tables = LOG_PIPELINE_LEN,
> +        .cur_ltable = lflow->table_id,
> +    };
> +    struct expr *prereqs = NULL;
> +    char *error;
> +
> +    error = ovnacts_parse_string(lflow->actions, &pp, &ovnacts, &prereqs);
> +    if (error) {
> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> +        VLOG_WARN_RL(&rl, "error parsing actions \"%s\": %s",
> +                     lflow->actions, error);
> +        free(error);
> +        ovnacts_free(ovnacts.data, ovnacts.size);
> +        ofpbuf_uninit(&ovnacts);
> +        return true;
> +    }
> +
> +    struct lookup_port_aux aux = {
> +        .sbrec_multicast_group_by_name_datapath
> +            = l_ctx_in->sbrec_multicast_group_by_name_datapath,
> +        .sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name,
> +        .dp = dp,
> +        .lflow = lflow,
> +        .lfrr = l_ctx_out->lfrr,
> +    };
> +    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,
> +        .lfrr = l_ctx_out->lfrr,
> +    };
> +
> +    struct hmap matches = HMAP_INITIALIZER(&matches);
> +    const struct expr_constant_set *fake_as = as_diff_added;
> +    struct expr_constant_set *new_fake_as = NULL;
> +    struct in6_addr dummy_ip;
> +    bool has_dummy_ip = false;
> +    ovs_assert(as_diff_added->n_values);
> +
> +    /* When there is only 1 element, we append a dummy address and create a
> +     * fake address set with 2 elements, so that the lflow parsing would
> +     * generate exactly the same format of flows as it would when parsing with
> +     * the original address set. */
> +    if (as_diff_added->n_values == 1) {
> +        new_fake_as = xzalloc(sizeof *new_fake_as);
> +        new_fake_as->values = xzalloc(sizeof *new_fake_as->values * 2);
> +        new_fake_as->n_values = 2;
> +        new_fake_as->values[0] = new_fake_as->values[1] =
> +            as_diff_added->values[0];
> +        /* Make a dummy ip that is different from the real one. */
> +        new_fake_as->values[1].value.u8_val++;
> +        dummy_ip = new_fake_as->values[1].value.ipv6;
> +        has_dummy_ip = true;
> +        fake_as = new_fake_as;
> +    }
> +
> +    /* Temporarily replace the address set in addr_sets with the fake_as, so
> +     * that the cost of lflow parsing is related to the delta but not the
> +     * original size of the address set. It is possible that there are other
> +     * address sets used by this logical flow and their size can be big. In
> +     * such case the parsing cost is still high. In practice, big address
> +     * sets are likely to be updated more frequently that small address sets,
> +     * so this approach should still be effetive overall.

s/effetive/effective

> +     *
> +     * XXX: if necessary, we can optimize this by checking all the address set
> +     * references in this lflow, and replace all the "big" address sets with a
> +     * small faked one. */
> +    struct expr_constant_set *real_as =
> +        shash_replace((struct shash *)l_ctx_in->addr_sets, as_name, fake_as);
> +    /* We are here because of the address set update, so it must be found. */
> +    ovs_assert(real_as);
> +
> +    struct expr *expr = convert_match_to_expr(lflow, dp, &prereqs,
> +                                              l_ctx_in->addr_sets,
> +                                              l_ctx_in->port_groups,
> +                                              l_ctx_out->lfrr, NULL);
> +    shash_replace((struct shash *)l_ctx_in->addr_sets, as_name, real_as);
> +    if (new_fake_as) {
> +        expr_constant_set_destroy(new_fake_as);
> +        free(new_fake_as);
> +    }
> +    if (!expr) {
> +        goto done;
> +    }
> +
> +    expr = expr_evaluate_condition(expr, is_chassis_resident_cb,
> +                                   &cond_aux);
> +    expr = expr_normalize(expr);
> +
> +    uint32_t start_conj_id = 0;
> +    uint32_t n_conjs = 0;
> +    n_conjs = expr_to_matches(expr, lookup_port_cb, &aux, &matches);
> +    if (hmap_is_empty(&matches)) {
> +        VLOG_DBG("lflow "UUID_FMT" matches are empty, skip",
> +                 UUID_ARGS(&lflow->header_.uuid));
> +        goto done;
> +    }
> +
> +    /* Discard the matches unrelated to the added addresses in the AS
> +     * 'as_name'. */
> +    struct expr_match *m, *m_next;
> +    HMAP_FOR_EACH_SAFE (m, m_next, hmap_node, &matches) {
> +        if (!m->as_name || strcmp(m->as_name, as_name) ||
> +            (has_dummy_ip && !memcmp(&m->as_ip, &dummy_ip, sizeof dummy_ip))) {
> +            hmap_remove(&matches, &m->hmap_node);
> +            expr_match_destroy(m);
> +            continue;
> +        }
> +    }
> +
> +    /* The number of matches generated by the new addresses should match the
> +     * number of items in the as_diff_added and the reference count of the AS
> +     * in this lflow. Otherwise, it means we hit some complex/corner cases that
> +     * the generated matches can't be mapped from the items in the
> +     * as_diff_added. So we need to fall back to reprocessing the lflow.
> +     */
> +    if (hmap_count(&matches) != as_ref_count * as_diff_added->n_values) {
> +        VLOG_DBG("lflow "UUID_FMT", addrset %s: Generated flows count "
> +                 "(%"PRIuSIZE") " "doesn't match added addresses count "
> +                 "(%"PRIuSIZE") and ref_count (%"PRIuSIZE"). "
> +                 "Need reprocessing.",
> +                 UUID_ARGS(&lflow->header_.uuid), as_name,
> +                 hmap_count(&matches), as_diff_added->n_values, as_ref_count);
> +        handled = false;
> +        goto done;
> +    }
> +    if (n_conjs) {
> +        start_conj_id = lflow_conj_ids_find(l_ctx_out->conj_ids,
> +                                            &lflow->header_.uuid);
> +        if (!start_conj_id) {
> +            VLOG_DBG("lflow "UUID_FMT" didn't have conjunctions. "
> +                     "Need reprocessing", UUID_ARGS(&lflow->header_.uuid));
> +            handled = false;
> +            goto done;
> +        }
> +        expr_matches_prepare(&matches, start_conj_id - 1);
> +    }
> +    add_matches_to_flow_table(lflow, dp, &matches, ptable, output_ptable,
> +                              &ovnacts, ingress, l_ctx_in, l_ctx_out);
> +done:
> +    expr_destroy(prereqs);
> +    ovnacts_free(ovnacts.data, ovnacts.size);
> +    ofpbuf_uninit(&ovnacts);
> +    expr_destroy(expr);
> +    expr_matches_destroy(&matches);
> +    return handled;
> +}
> +
> +static bool
> +consider_lflow_for_added_as_ips(
> +                        const struct sbrec_logical_flow *lflow,
> +                        const char *as_name,
> +                        size_t as_ref_count,
> +                        const struct expr_constant_set *as_diff_added,
> +                        struct hmap *dhcp_opts,
> +                        struct hmap *dhcpv6_opts,
> +                        struct hmap *nd_ra_opts,
> +                        struct controller_event_options *controller_event_opts,
> +                        struct lflow_ctx_in *l_ctx_in,
> +                        struct lflow_ctx_out *l_ctx_out)
> +{
> +    const struct sbrec_logical_dp_group *dp_group = lflow->logical_dp_group;
> +    const struct sbrec_datapath_binding *dp = lflow->logical_datapath;
> +
> +    if (!dp_group && !dp) {
> +        VLOG_DBG("lflow "UUID_FMT" has no datapath binding, skip",
> +                 UUID_ARGS(&lflow->header_.uuid));
> +        return true;
> +    }
> +    ovs_assert(!dp_group || !dp);
> +
> +    if (dp) {
> +        return consider_lflow_for_added_as_ips__(lflow, dp, as_name,
> +                                                 as_ref_count, as_diff_added,
> +                                                 dhcp_opts, dhcpv6_opts,
> +                                                 nd_ra_opts,
> +                                                 controller_event_opts,
> +                                                 l_ctx_in, l_ctx_out);
> +    }
> +    for (size_t i = 0; dp_group && i < dp_group->n_datapaths; i++) {
> +        if (!consider_lflow_for_added_as_ips__(lflow, dp_group->datapaths[i],
> +                                               as_name, as_ref_count,
> +                                               as_diff_added, dhcp_opts,
> +                                               dhcpv6_opts, nd_ra_opts,
> +                                               controller_event_opts, l_ctx_in,
> +                                               l_ctx_out)) {
> +            return false;
> +        }
> +    }
> +    return true;
> +}
> +
> +/* Check if an address set update can be handled without reprocessing the
> + * lflow. */
>  static bool
>  as_update_can_be_handled(const char *as_name, struct addr_set_diff *as_diff,
>                           struct lflow_ctx_in *l_ctx_in)
> @@ -582,19 +851,18 @@ as_update_can_be_handled(const char *as_name, struct addr_set_diff *as_diff,
>   *        It could have been combined as:
>   *
>   *          ip.src == $as1 && tcp.dst == {p1, p2, p3, p4}
> + *
> + *        Note: addresses additions still can be processed incrementally in
> + *        this case, although deletions cannot.
>   */
>  bool
>  lflow_handle_addr_set_update(const char *as_name,
>                               struct addr_set_diff *as_diff,
> -                             struct lflow_ctx_in *l_ctx_in OVS_UNUSED,
> +                             struct lflow_ctx_in *l_ctx_in,
>                               struct lflow_ctx_out *l_ctx_out,
>                               bool *changed)
>  {
> -    if (as_diff->added) {
> -        return false;
> -    }
> -    ovs_assert(as_diff->deleted);
> -
> +    ovs_assert(as_diff->added || as_diff->deleted);
>      if (!as_update_can_be_handled(as_name, as_diff, l_ctx_in)) {
>          return false;
>      }
> @@ -609,6 +877,31 @@ lflow_handle_addr_set_update(const char *as_name,
>
>      *changed = false;
>
> +    struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
> +    struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts);
> +    struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts);
> +    struct controller_event_options controller_event_opts;
> +
> +    if (as_diff->added) {
> +        const struct sbrec_dhcp_options *dhcp_opt_row;
> +        SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row,
> +                                           l_ctx_in->dhcp_options_table) {
> +            dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code,
> +                         dhcp_opt_row->type);
> +        }
> +
> +        const struct sbrec_dhcpv6_options *dhcpv6_opt_row;
> +        SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH(dhcpv6_opt_row,
> +                                            l_ctx_in->dhcpv6_options_table) {
> +           dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name,
> +                        dhcpv6_opt_row->code, dhcpv6_opt_row->type);
> +        }
> +
> +        nd_ra_opts_init(&nd_ra_opts);
> +        controller_event_opts_init(&controller_event_opts);
> +    }
> +
> +    bool ret = true;
>      struct lflow_ref_list_node *lrln;
>      HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) {
>          if (lflows_processed_find(l_ctx_out->lflows_processed,
> @@ -617,21 +910,56 @@ lflow_handle_addr_set_update(const char *as_name,
>                       UUID_ARGS(&lrln->lflow_uuid));
>              continue;
>          }
> -        for (size_t i = 0; i < as_diff->deleted->n_values; i++) {
> -            union expr_constant *c = &as_diff->deleted->values[i];
> +        const struct sbrec_logical_flow *lflow =
> +            sbrec_logical_flow_table_get_for_uuid(l_ctx_in->logical_flow_table,
> +                                                  &lrln->lflow_uuid);
> +        if (!lflow) {
> +            /* lflow deletion should be handled in the corresponding input
> +             * handler, so we can skip here. */
> +            VLOG_DBG("lflow "UUID_FMT" not found while handling updates of "
> +                     "address set %s, skip.",
> +                     UUID_ARGS(&lrln->lflow_uuid), as_name);
> +            continue;
> +        }
> +        *changed = true;
> +
> +        if (as_diff->deleted) {
>              struct addrset_info as_info;
> -            if (!as_info_from_expr_const(as_name, c, &as_info)) {
> -                continue;
> +            for (size_t i = 0; i < as_diff->deleted->n_values; i++) {
> +                union expr_constant *c = &as_diff->deleted->values[i];
> +                if (!as_info_from_expr_const(as_name, c, &as_info)) {
> +                    continue;
> +                }
> +                if (!ofctrl_remove_flows_for_as_ip(l_ctx_out->flow_table,
> +                                                   &lrln->lflow_uuid, &as_info,
> +                                                   lrln->ref_count)) {
> +                    ret = false;
> +                    goto done;
> +                }
>              }
> -            if (!ofctrl_remove_flows_for_as_ip(l_ctx_out->flow_table,
> -                                               &lrln->lflow_uuid, &as_info,
> -                                               lrln->ref_count)) {
> -                return false;
> +        }
> +
> +        if (as_diff->added) {
> +            if (!consider_lflow_for_added_as_ips(lflow, as_name,
> +                                                 lrln->ref_count,
> +                                                 as_diff->added, &dhcp_opts,
> +                                                 &dhcpv6_opts, &nd_ra_opts,
> +                                                 &controller_event_opts,
> +                                                 l_ctx_in, l_ctx_out)) {
> +                ret = false;
> +                goto done;
>              }
> -            *changed = true;
>          }
>      }
> -    return true;
> +
> +done:
> +    if (as_diff->added) {
> +        dhcp_opts_destroy(&dhcp_opts);
> +        dhcp_opts_destroy(&dhcpv6_opts);
> +        nd_ra_opts_destroy(&nd_ra_opts);
> +        controller_event_opts_destroy(&controller_event_opts);
> +    }
> +    return ret;
>  }
>
>  bool
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 5119a3277..b8cd11378 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -3263,6 +3263,14 @@ main(int argc, char *argv[])
>      engine_add_input(&en_pflow_output, &en_ovs_open_vswitch, NULL);
>      engine_add_input(&en_pflow_output, &en_ovs_bridge, NULL);
>
> +    /* Keep en_addr_sets before en_runtime_data because
> +     * lflow_output_runtime_data_handler may *partially* reprocess a lflow when
> +     * the lflow is attached to a DP group and a new DP in that DP group is
> +     * added locally, i.e. reprocessing the lflow for the new DP only but not
> +     * for the other DPs in the group. If we handle en_addr_sets after this,
> +     * incrementally processing an updated address set for the added IPs may
> +     * end up adding redundant flows/conjunctions for the lflow agaist the new
> +     * DP because it has been processed on the DP already. */
>      engine_add_input(&en_lflow_output, &en_addr_sets,
>                       lflow_output_addr_sets_handler);
>      engine_add_input(&en_lflow_output, &en_port_groups,
> diff --git a/include/ovn/expr.h b/include/ovn/expr.h
> index cb3baf001..3b141b034 100644
> --- a/include/ovn/expr.h
> +++ b/include/ovn/expr.h
> @@ -484,6 +484,7 @@ uint32_t expr_to_matches(const struct expr *,
>                                               unsigned int *portp),
>                           const void *aux,
>                           struct hmap *matches);
> +void expr_match_destroy(struct expr_match *);
>  void expr_matches_destroy(struct hmap *matches);
>  size_t expr_matches_prepare(struct hmap *matches, uint32_t conj_id_ofs);
>  void expr_matches_print(const struct hmap *matches, FILE *);
> diff --git a/lib/expr.c b/lib/expr.c
> index 2c178d87f..47ef6108e 100644
> --- a/lib/expr.c
> +++ b/lib/expr.c
> @@ -3054,7 +3054,7 @@ expr_match_new(const struct match *m, uint8_t clause, uint8_t n_clauses,
>      return match;
>  }
>
> -static void
> +void
>  expr_match_destroy(struct expr_match *match)
>  {
>      free(match->as_name);
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index b5f19b442..260986f15 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -904,7 +904,7 @@ priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3 actions=dr
>  done
>
>  reprocess_count_new=$(read_counter consider_logical_flow)
> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [10
> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [2
>  ])
>
>  # Remove the IPs from as1, 1 IP each time.
> @@ -955,7 +955,8 @@ priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.1.3 actions=dr
>  done
>
>  reprocess_count_new=$(read_counter consider_logical_flow)
> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [10
> +# When change from 0 to 2, still reprocessing.
> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
>  ])
>
>  # Add and remove IPs at the same time.
> @@ -973,7 +974,7 @@ AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep -c 10\.0\.0\.22], [0], [1
>  AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10\.0\.0\.10], [1], [ignore])
>
>  reprocess_count_new=$(read_counter consider_logical_flow)
> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0
>  ])
>
>  # Add 1 and remove 2
> @@ -988,7 +989,7 @@ AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep -c 10\.0\.0\.10], [0], [1
>  ])
>
>  reprocess_count_new=$(read_counter consider_logical_flow)
> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0
>  ])
>
>  # Add 1 and remove 1
> @@ -1002,7 +1003,7 @@ AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep -c 10\.0\.0\.21], [0], [1
>  AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10\.0\.0\.10], [1], [ignore])
>
>  reprocess_count_new=$(read_counter consider_logical_flow)
> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0
>  ])
>
>  # Add 2 and remove 2
> @@ -1019,7 +1020,7 @@ AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10\.0\.0\.8], [1], [ignore
>  AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10\.0\.0\.9], [1], [ignore])
>
>  reprocess_count_new=$(read_counter consider_logical_flow)
> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0
>  ])
>
>  OVN_CLEANUP([hv1])
> @@ -1093,7 +1094,7 @@ priority=1100,tcp,reg15=0x$port_key,metadata=0x$dp_key,tp_dst=333 actions=conjun
>  done
>
>  reprocess_count_new=$(read_counter consider_logical_flow)
> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [10
> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [2
>  ])
>
>  # Remove the IPs from as1, 1 IP each time.
> @@ -1150,7 +1151,7 @@ priority=1100,tcp,reg15=0x$port_key,metadata=0x$dp_key,tp_dst=333 actions=conjun
>  done
>
>  reprocess_count_new=$(read_counter consider_logical_flow)
> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [10
> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
>  ])
>
>  # Add and remove IPs at the same time.
> @@ -1168,7 +1169,7 @@ AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep -c 10\.0\.0\.22], [0], [1
>  AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10\.0\.0\.10], [1], [ignore])
>
>  reprocess_count_new=$(read_counter consider_logical_flow)
> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0
>  ])
>
>  # Add 1 and remove 2
> @@ -1183,7 +1184,7 @@ AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep -c 10\.0\.0\.10], [0], [1
>  ])
>
>  reprocess_count_new=$(read_counter consider_logical_flow)
> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0
>  ])
>
>  # Add 1 and remove 1
> @@ -1197,7 +1198,7 @@ AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep -c 10\.0\.0\.21], [0], [1
>  AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10\.0\.0\.10], [1], [ignore])
>
>  reprocess_count_new=$(read_counter consider_logical_flow)
> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0
>  ])
>
>  # Add 2 and remove 2
> @@ -1214,7 +1215,7 @@ AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10\.0\.0\.8], [1], [ignore
>  AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10\.0\.0\.9], [1], [ignore])
>
>  reprocess_count_new=$(read_counter consider_logical_flow)
> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0
>  ])
>
>  OVN_CLEANUP([hv1])
> @@ -1288,7 +1289,7 @@ priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3 actions=co
>  done
>
>  reprocess_count_new=$(read_counter consider_logical_flow)
> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [10
> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [2
>  ])
>
>  # Remove the IPs from as1 and as2, 1 IP each time.
> @@ -1341,7 +1342,7 @@ priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3,nw_dst=10.
>  done
>
>  reprocess_count_new=$(read_counter consider_logical_flow)
> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [9
> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
>  ])
>
>  # Add 1 more IP back to as2
> @@ -1639,7 +1640,7 @@ priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3 actions=co
>  done
>
>  reprocess_count_new=$(read_counter consider_logical_flow)
> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [10
> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [2
>  ])
>
>  # Remove the IPs from as1, 1 IP each time.
> @@ -1697,7 +1698,7 @@ priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.1.3 actions=co
>  done
>
>  reprocess_count_new=$(read_counter consider_logical_flow)
> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [10
> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
>  ])
>
>  # Test the case when there are two references to the same AS but one of the
> @@ -1842,7 +1843,7 @@ priority=1100,tcp,reg15=0x$port_key,metadata=0x$dp_key,tp_dst=202 actions=conjun
>  ])
>
>  reprocess_count_new=$(read_counter consider_logical_flow)
> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [2
> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0
>  ])
>
>  # Remove those 2 IPs from each AS, should return to the initial state
> @@ -1870,6 +1871,8 @@ priority=1100,tcp,reg15=0x$port_key,metadata=0x$dp_key,tp_dst=202 actions=conjun
>  ])
>
>  reprocess_count_new=$(read_counter consider_logical_flow)
> +# Because of the combined conjunction, AS cannot be tracked for the flow for
> +# 10.0.0.33, so removing would trigger reprocessing.
>  AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [2
>  ])
>
> @@ -1926,7 +1929,7 @@ priority=1100,reg15=0x$port_key,metadata=0x$dp_key,dl_src=aa:aa:aa:aa:aa:03 acti
>  done
>
>  reprocess_count_new=$(read_counter consider_logical_flow)
> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [5
> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [2
>  ])
>
>  # Remove the MACs from as1.
> @@ -2007,7 +2010,7 @@ priority=1100,ipv6,reg15=0x$port_key,metadata=0x$dp_key,ipv6_src=ff::3 actions=d
>  done
>
>  reprocess_count_new=$(read_counter consider_logical_flow)
> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [5
> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [2
>  ])
>
>  # Remove the IPs from as1, 1 IP each time.
> --
> 2.30.2
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Han Zhou Feb. 24, 2022, 12:55 a.m. UTC | #3 
On Wed, Feb 23, 2022 at 2:44 PM Numan Siddique <numans@ovn.org> wrote:
>
> /
>
> On Wed, Feb 9, 2022 at 12:56 PM Han Zhou <hzhou@ovn.org> wrote:
> >
> > To avoid reprocessing the lflow when a referenced address set has new
> > addresses added, this patch generates a fake address set that only
> > contains the added addresses for flow generation, and then eliminates
> > the flows that are not related to the newly added addresses.
> >
> > Scale test shows obvious performance gains because the time complexity
> > changed from O(n) to O(1). The bigger the size of address set, the more
> > CPU savings. With the AS size of 10k, the test shows ~40x speed up.
> >
> > Test setup:
> > CPU: Intel(R) Core(TM) i9-7920X CPU @ 2.90GHz.
> > 5 ACL all referencing an address set of 10,000 IPs.
> >
> > Measure the time spent by ovn-controller for handling one IP
> > addition from the address set.
> >
> > Before: ~400ms
> > After: 11-12ms
> >
> > Signed-off-by: Han Zhou <hzhou@ovn.org>
>
> Hi Han,
>
> I found a couple of small typos which you can consider fixing while
> applying the patches.
>
> Please see the patch 0 which has my Ack.
>
> Thanks
> Numan
>
> > ---
> >  controller/lflow-conj-ids.c |  20 ++
> >  controller/lflow-conj-ids.h |   1 +
> >  controller/lflow.c          | 360 ++++++++++++++++++++++++++++++++++--
> >  controller/ovn-controller.c |   8 +
> >  include/ovn/expr.h          |   1 +
> >  lib/expr.c                  |   2 +-
> >  tests/ovn-controller.at     |  41 ++--
> >  7 files changed, 397 insertions(+), 36 deletions(-)
> >
> > diff --git a/controller/lflow-conj-ids.c b/controller/lflow-conj-ids.c
> > index bfe63862a..372391e5a 100644
> > --- a/controller/lflow-conj-ids.c
> > +++ b/controller/lflow-conj-ids.c
> > @@ -157,6 +157,26 @@ lflow_conj_ids_alloc_specified(struct conj_ids
*conj_ids,
> >      return true;
> >  }
> >
> > +/* Find and return the start id that is allocated to the logical flow.
Return
> > + * 0 if not found. */
> > +uint32_t
> > +lflow_conj_ids_find(struct conj_ids *conj_ids, const struct uuid
*lflow_uuid)
> > +{
> > +    struct lflow_conj_node *lflow_conj;
> > +    bool found = false;
> > +    HMAP_FOR_EACH_WITH_HASH (lflow_conj, hmap_node,
uuid_hash(lflow_uuid),
> > +                             &conj_ids->lflow_conj_ids) {
> > +        if (uuid_equals(&lflow_conj->lflow_uuid, lflow_uuid)) {
> > +            found = true;
> > +            break;
> > +        }
> > +    }
> > +    if (!found) {
> > +        return 0;
> > +    }
> > +    return lflow_conj->start_conj_id;
> > +}
> > +
> >  /* Frees the conjunction IDs used by lflow_uuid. */
> >  void
> >  lflow_conj_ids_free(struct conj_ids *conj_ids, const struct uuid
*lflow_uuid)
> > diff --git a/controller/lflow-conj-ids.h b/controller/lflow-conj-ids.h
> > index 6da0a612c..a1f7a95a5 100644
> > --- a/controller/lflow-conj-ids.h
> > +++ b/controller/lflow-conj-ids.h
> > @@ -35,6 +35,7 @@ bool lflow_conj_ids_alloc_specified(struct conj_ids *,
> >                                      const struct uuid *lflow_uuid,
> >                                      uint32_t start_conj_id, uint32_t
n_conjs);
> >  void lflow_conj_ids_free(struct conj_ids *, const struct uuid
*lflow_uuid);
> > +uint32_t lflow_conj_ids_find(struct conj_ids *, const struct uuid
*lflow_uuid);
> >  void lflow_conj_ids_init(struct conj_ids *);
> >  void lflow_conj_ids_destroy(struct conj_ids *);
> >  void lflow_conj_ids_clear(struct conj_ids *);
> > diff --git a/controller/lflow.c b/controller/lflow.c
> > index 9b1184c0e..658b3109b 100644
> > --- a/controller/lflow.c
> > +++ b/controller/lflow.c
> > @@ -74,6 +74,19 @@ struct condition_aux {
> >      struct lflow_resource_ref *lfrr;
> >  };
> >
> > +static struct expr *
> > +convert_match_to_expr(const struct sbrec_logical_flow *,
> > +                      const struct sbrec_datapath_binding *,
> > +                      struct expr **prereqs, const struct shash
*addr_sets,
> > +                      const struct shash *port_groups,
> > +                      struct lflow_resource_ref *, bool
*pg_addr_set_ref);
> > +static void
> > +add_matches_to_flow_table(const struct sbrec_logical_flow *,
> > +                          const struct sbrec_datapath_binding *,
> > +                          struct hmap *matches, uint8_t ptable,
> > +                          uint8_t output_ptable, struct ofpbuf
*ovnacts,
> > +                          bool ingress, struct lflow_ctx_in *,
> > +                          struct lflow_ctx_out *);
> >  static void
> >  consider_logical_flow(const struct sbrec_logical_flow *lflow,
> >                        struct hmap *dhcp_opts, struct hmap *dhcpv6_opts,
> > @@ -513,6 +526,262 @@ as_info_from_expr_const(const char *as_name,
const union expr_constant *c,
> >      return true;
> >  }
> >
> > +/* Parses the lflow regarding the changed address set 'as_name', and
gererates/
> s/gererates/generates
>
Ack.

>
> > + * ovs flows for the newly added addresses in 'as_diff_added' only. It
is
> > + * similar to consider_logical_flow__, with the below differences:
> > + *
> > + * - It has one more arg 'as_ref_count' to deduce how many flows are
expected
> > + *   to be added.
> > + * - It uses a small fake address set that contains only the added
addresses
> > + *   to replace the original address set temporarily and restores it
after
> > + *   parsing.
> > + * - It doesn't check or touch lflow-cache, because lflow-cache is
disabled
> > + *   when address-sets/port-groups are used.
> > + * - It doesn't check non-local lports because it should have been
checked
> > + *   when the lflow is initially parsed, and if it is non-local and
skipped
> > + *   then it wouldn't have the address set parsed and referenced.
> > + *
> > + * Because of these differences, it is just cleaner to keep it as a
separate
> > + * function. */
> > +static bool
> > +consider_lflow_for_added_as_ips__(
> > +                        const struct sbrec_logical_flow *lflow,
> > +                        const struct sbrec_datapath_binding *dp,
> > +                        const char *as_name,
> > +                        size_t as_ref_count,
> > +                        const struct expr_constant_set *as_diff_added,
> > +                        struct hmap *dhcp_opts,
> > +                        struct hmap *dhcpv6_opts,
> > +                        struct hmap *nd_ra_opts,
> > +                        struct controller_event_options
*controller_event_opts,
> > +                        struct lflow_ctx_in *l_ctx_in,
> > +                        struct lflow_ctx_out *l_ctx_out)
> > +{
> > +    bool handled = true;
> > +    if (!get_local_datapath(l_ctx_in->local_datapaths,
dp->tunnel_key)) {
> > +        VLOG_DBG("Skip lflow "UUID_FMT" for non-local datapath
%"PRId64,
> > +                 UUID_ARGS(&lflow->header_.uuid), dp->tunnel_key);
> > +        return true;
> > +    }
> > +
> > +    /* Determine translation of logical table IDs to physical table
IDs. */
> > +    bool ingress = !strcmp(lflow->pipeline, "ingress");
> > +
> > +    /* Determine translation of logical table IDs to physical table
IDs. */
> > +    uint8_t first_ptable = (ingress
> > +                            ? OFTABLE_LOG_INGRESS_PIPELINE
> > +                            : OFTABLE_LOG_EGRESS_PIPELINE);
> > +    uint8_t ptable = first_ptable + lflow->table_id;
> > +    uint8_t output_ptable = (ingress
> > +                             ? OFTABLE_REMOTE_OUTPUT
> > +                             : OFTABLE_SAVE_INPORT);
> > +
> > +    uint64_t ovnacts_stub[1024 / 8];
> > +    struct ofpbuf ovnacts = OFPBUF_STUB_INITIALIZER(ovnacts_stub);
> > +    struct ovnact_parse_params pp = {
> > +        .symtab = &symtab,
> > +        .dhcp_opts = dhcp_opts,
> > +        .dhcpv6_opts = dhcpv6_opts,
> > +        .nd_ra_opts = nd_ra_opts,
> > +        .controller_event_opts = controller_event_opts,
> > +
> > +        .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS,
> > +        .n_tables = LOG_PIPELINE_LEN,
> > +        .cur_ltable = lflow->table_id,
> > +    };
> > +    struct expr *prereqs = NULL;
> > +    char *error;
> > +
> > +    error = ovnacts_parse_string(lflow->actions, &pp, &ovnacts,
&prereqs);
> > +    if (error) {
> > +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> > +        VLOG_WARN_RL(&rl, "error parsing actions \"%s\": %s",
> > +                     lflow->actions, error);
> > +        free(error);
> > +        ovnacts_free(ovnacts.data, ovnacts.size);
> > +        ofpbuf_uninit(&ovnacts);
> > +        return true;
> > +    }
> > +
> > +    struct lookup_port_aux aux = {
> > +        .sbrec_multicast_group_by_name_datapath
> > +            = l_ctx_in->sbrec_multicast_group_by_name_datapath,
> > +        .sbrec_port_binding_by_name =
l_ctx_in->sbrec_port_binding_by_name,
> > +        .dp = dp,
> > +        .lflow = lflow,
> > +        .lfrr = l_ctx_out->lfrr,
> > +    };
> > +    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,
> > +        .lfrr = l_ctx_out->lfrr,
> > +    };
> > +
> > +    struct hmap matches = HMAP_INITIALIZER(&matches);
> > +    const struct expr_constant_set *fake_as = as_diff_added;
> > +    struct expr_constant_set *new_fake_as = NULL;
> > +    struct in6_addr dummy_ip;
> > +    bool has_dummy_ip = false;
> > +    ovs_assert(as_diff_added->n_values);
> > +
> > +    /* When there is only 1 element, we append a dummy address and
create a
> > +     * fake address set with 2 elements, so that the lflow parsing
would
> > +     * generate exactly the same format of flows as it would when
parsing with
> > +     * the original address set. */
> > +    if (as_diff_added->n_values == 1) {
> > +        new_fake_as = xzalloc(sizeof *new_fake_as);
> > +        new_fake_as->values = xzalloc(sizeof *new_fake_as->values * 2);
> > +        new_fake_as->n_values = 2;
> > +        new_fake_as->values[0] = new_fake_as->values[1] =
> > +            as_diff_added->values[0];
> > +        /* Make a dummy ip that is different from the real one. */
> > +        new_fake_as->values[1].value.u8_val++;
> > +        dummy_ip = new_fake_as->values[1].value.ipv6;
> > +        has_dummy_ip = true;
> > +        fake_as = new_fake_as;
> > +    }
> > +
> > +    /* Temporarily replace the address set in addr_sets with the
fake_as, so
> > +     * that the cost of lflow parsing is related to the delta but not
the
> > +     * original size of the address set. It is possible that there are
other
> > +     * address sets used by this logical flow and their size can be
big. In
> > +     * such case the parsing cost is still high. In practice, big
address
> > +     * sets are likely to be updated more frequently that small
address sets,
> > +     * so this approach should still be effetive overall.
>
> s/effetive/effective

Ack.

All fixed. Thanks Numan.

>
> > +     *
> > +     * XXX: if necessary, we can optimize this by checking all the
address set
> > +     * references in this lflow, and replace all the "big" address
sets with a
> > +     * small faked one. */
> > +    struct expr_constant_set *real_as =
> > +        shash_replace((struct shash *)l_ctx_in->addr_sets, as_name,
fake_as);
> > +    /* We are here because of the address set update, so it must be
found. */
> > +    ovs_assert(real_as);
> > +
> > +    struct expr *expr = convert_match_to_expr(lflow, dp, &prereqs,
> > +                                              l_ctx_in->addr_sets,
> > +                                              l_ctx_in->port_groups,
> > +                                              l_ctx_out->lfrr, NULL);
> > +    shash_replace((struct shash *)l_ctx_in->addr_sets, as_name,
real_as);
> > +    if (new_fake_as) {
> > +        expr_constant_set_destroy(new_fake_as);
> > +        free(new_fake_as);
> > +    }
> > +    if (!expr) {
> > +        goto done;
> > +    }
> > +
> > +    expr = expr_evaluate_condition(expr, is_chassis_resident_cb,
> > +                                   &cond_aux);
> > +    expr = expr_normalize(expr);
> > +
> > +    uint32_t start_conj_id = 0;
> > +    uint32_t n_conjs = 0;
> > +    n_conjs = expr_to_matches(expr, lookup_port_cb, &aux, &matches);
> > +    if (hmap_is_empty(&matches)) {
> > +        VLOG_DBG("lflow "UUID_FMT" matches are empty, skip",
> > +                 UUID_ARGS(&lflow->header_.uuid));
> > +        goto done;
> > +    }
> > +
> > +    /* Discard the matches unrelated to the added addresses in the AS
> > +     * 'as_name'. */
> > +    struct expr_match *m, *m_next;
> > +    HMAP_FOR_EACH_SAFE (m, m_next, hmap_node, &matches) {
> > +        if (!m->as_name || strcmp(m->as_name, as_name) ||
> > +            (has_dummy_ip && !memcmp(&m->as_ip, &dummy_ip, sizeof
dummy_ip))) {
> > +            hmap_remove(&matches, &m->hmap_node);
> > +            expr_match_destroy(m);
> > +            continue;
> > +        }
> > +    }
> > +
> > +    /* The number of matches generated by the new addresses should
match the
> > +     * number of items in the as_diff_added and the reference count of
the AS
> > +     * in this lflow. Otherwise, it means we hit some complex/corner
cases that
> > +     * the generated matches can't be mapped from the items in the
> > +     * as_diff_added. So we need to fall back to reprocessing the
lflow.
> > +     */
> > +    if (hmap_count(&matches) != as_ref_count *
as_diff_added->n_values) {
> > +        VLOG_DBG("lflow "UUID_FMT", addrset %s: Generated flows count "
> > +                 "(%"PRIuSIZE") " "doesn't match added addresses count
"
> > +                 "(%"PRIuSIZE") and ref_count (%"PRIuSIZE"). "
> > +                 "Need reprocessing.",
> > +                 UUID_ARGS(&lflow->header_.uuid), as_name,
> > +                 hmap_count(&matches), as_diff_added->n_values,
as_ref_count);
> > +        handled = false;
> > +        goto done;
> > +    }
> > +    if (n_conjs) {
> > +        start_conj_id = lflow_conj_ids_find(l_ctx_out->conj_ids,
> > +                                            &lflow->header_.uuid);
> > +        if (!start_conj_id) {
> > +            VLOG_DBG("lflow "UUID_FMT" didn't have conjunctions. "
> > +                     "Need reprocessing",
UUID_ARGS(&lflow->header_.uuid));
> > +            handled = false;
> > +            goto done;
> > +        }
> > +        expr_matches_prepare(&matches, start_conj_id - 1);
> > +    }
> > +    add_matches_to_flow_table(lflow, dp, &matches, ptable,
output_ptable,
> > +                              &ovnacts, ingress, l_ctx_in, l_ctx_out);
> > +done:
> > +    expr_destroy(prereqs);
> > +    ovnacts_free(ovnacts.data, ovnacts.size);
> > +    ofpbuf_uninit(&ovnacts);
> > +    expr_destroy(expr);
> > +    expr_matches_destroy(&matches);
> > +    return handled;
> > +}
> > +
> > +static bool
> > +consider_lflow_for_added_as_ips(
> > +                        const struct sbrec_logical_flow *lflow,
> > +                        const char *as_name,
> > +                        size_t as_ref_count,
> > +                        const struct expr_constant_set *as_diff_added,
> > +                        struct hmap *dhcp_opts,
> > +                        struct hmap *dhcpv6_opts,
> > +                        struct hmap *nd_ra_opts,
> > +                        struct controller_event_options
*controller_event_opts,
> > +                        struct lflow_ctx_in *l_ctx_in,
> > +                        struct lflow_ctx_out *l_ctx_out)
> > +{
> > +    const struct sbrec_logical_dp_group *dp_group =
lflow->logical_dp_group;
> > +    const struct sbrec_datapath_binding *dp = lflow->logical_datapath;
> > +
> > +    if (!dp_group && !dp) {
> > +        VLOG_DBG("lflow "UUID_FMT" has no datapath binding, skip",
> > +                 UUID_ARGS(&lflow->header_.uuid));
> > +        return true;
> > +    }
> > +    ovs_assert(!dp_group || !dp);
> > +
> > +    if (dp) {
> > +        return consider_lflow_for_added_as_ips__(lflow, dp, as_name,
> > +                                                 as_ref_count,
as_diff_added,
> > +                                                 dhcp_opts,
dhcpv6_opts,
> > +                                                 nd_ra_opts,
> > +                                                 controller_event_opts,
> > +                                                 l_ctx_in, l_ctx_out);
> > +    }
> > +    for (size_t i = 0; dp_group && i < dp_group->n_datapaths; i++) {
> > +        if (!consider_lflow_for_added_as_ips__(lflow,
dp_group->datapaths[i],
> > +                                               as_name, as_ref_count,
> > +                                               as_diff_added,
dhcp_opts,
> > +                                               dhcpv6_opts, nd_ra_opts,
> > +                                               controller_event_opts,
l_ctx_in,
> > +                                               l_ctx_out)) {
> > +            return false;
> > +        }
> > +    }
> > +    return true;
> > +}
> > +
> > +/* Check if an address set update can be handled without reprocessing
the
> > + * lflow. */
> >  static bool
> >  as_update_can_be_handled(const char *as_name, struct addr_set_diff
*as_diff,
> >                           struct lflow_ctx_in *l_ctx_in)
> > @@ -582,19 +851,18 @@ as_update_can_be_handled(const char *as_name,
struct addr_set_diff *as_diff,
> >   *        It could have been combined as:
> >   *
> >   *          ip.src == $as1 && tcp.dst == {p1, p2, p3, p4}
> > + *
> > + *        Note: addresses additions still can be processed
incrementally in
> > + *        this case, although deletions cannot.
> >   */
> >  bool
> >  lflow_handle_addr_set_update(const char *as_name,
> >                               struct addr_set_diff *as_diff,
> > -                             struct lflow_ctx_in *l_ctx_in OVS_UNUSED,
> > +                             struct lflow_ctx_in *l_ctx_in,
> >                               struct lflow_ctx_out *l_ctx_out,
> >                               bool *changed)
> >  {
> > -    if (as_diff->added) {
> > -        return false;
> > -    }
> > -    ovs_assert(as_diff->deleted);
> > -
> > +    ovs_assert(as_diff->added || as_diff->deleted);
> >      if (!as_update_can_be_handled(as_name, as_diff, l_ctx_in)) {
> >          return false;
> >      }
> > @@ -609,6 +877,31 @@ lflow_handle_addr_set_update(const char *as_name,
> >
> >      *changed = false;
> >
> > +    struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
> > +    struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts);
> > +    struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts);
> > +    struct controller_event_options controller_event_opts;
> > +
> > +    if (as_diff->added) {
> > +        const struct sbrec_dhcp_options *dhcp_opt_row;
> > +        SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row,
> > +
l_ctx_in->dhcp_options_table) {
> > +            dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name,
dhcp_opt_row->code,
> > +                         dhcp_opt_row->type);
> > +        }
> > +
> > +        const struct sbrec_dhcpv6_options *dhcpv6_opt_row;
> > +        SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH(dhcpv6_opt_row,
> > +
 l_ctx_in->dhcpv6_options_table) {
> > +           dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name,
> > +                        dhcpv6_opt_row->code, dhcpv6_opt_row->type);
> > +        }
> > +
> > +        nd_ra_opts_init(&nd_ra_opts);
> > +        controller_event_opts_init(&controller_event_opts);
> > +    }
> > +
> > +    bool ret = true;
> >      struct lflow_ref_list_node *lrln;
> >      HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) {
> >          if (lflows_processed_find(l_ctx_out->lflows_processed,
> > @@ -617,21 +910,56 @@ lflow_handle_addr_set_update(const char *as_name,
> >                       UUID_ARGS(&lrln->lflow_uuid));
> >              continue;
> >          }
> > -        for (size_t i = 0; i < as_diff->deleted->n_values; i++) {
> > -            union expr_constant *c = &as_diff->deleted->values[i];
> > +        const struct sbrec_logical_flow *lflow =
> > +
 sbrec_logical_flow_table_get_for_uuid(l_ctx_in->logical_flow_table,
> > +                                                  &lrln->lflow_uuid);
> > +        if (!lflow) {
> > +            /* lflow deletion should be handled in the corresponding
input
> > +             * handler, so we can skip here. */
> > +            VLOG_DBG("lflow "UUID_FMT" not found while handling
updates of "
> > +                     "address set %s, skip.",
> > +                     UUID_ARGS(&lrln->lflow_uuid), as_name);
> > +            continue;
> > +        }
> > +        *changed = true;
> > +
> > +        if (as_diff->deleted) {
> >              struct addrset_info as_info;
> > -            if (!as_info_from_expr_const(as_name, c, &as_info)) {
> > -                continue;
> > +            for (size_t i = 0; i < as_diff->deleted->n_values; i++) {
> > +                union expr_constant *c = &as_diff->deleted->values[i];
> > +                if (!as_info_from_expr_const(as_name, c, &as_info)) {
> > +                    continue;
> > +                }
> > +                if
(!ofctrl_remove_flows_for_as_ip(l_ctx_out->flow_table,
> > +                                                   &lrln->lflow_uuid,
&as_info,
> > +                                                   lrln->ref_count)) {
> > +                    ret = false;
> > +                    goto done;
> > +                }
> >              }
> > -            if (!ofctrl_remove_flows_for_as_ip(l_ctx_out->flow_table,
> > -                                               &lrln->lflow_uuid,
&as_info,
> > -                                               lrln->ref_count)) {
> > -                return false;
> > +        }
> > +
> > +        if (as_diff->added) {
> > +            if (!consider_lflow_for_added_as_ips(lflow, as_name,
> > +                                                 lrln->ref_count,
> > +                                                 as_diff->added,
&dhcp_opts,
> > +                                                 &dhcpv6_opts,
&nd_ra_opts,
> > +
&controller_event_opts,
> > +                                                 l_ctx_in, l_ctx_out))
{
> > +                ret = false;
> > +                goto done;
> >              }
> > -            *changed = true;
> >          }
> >      }
> > -    return true;
> > +
> > +done:
> > +    if (as_diff->added) {
> > +        dhcp_opts_destroy(&dhcp_opts);
> > +        dhcp_opts_destroy(&dhcpv6_opts);
> > +        nd_ra_opts_destroy(&nd_ra_opts);
> > +        controller_event_opts_destroy(&controller_event_opts);
> > +    }
> > +    return ret;
> >  }
> >
> >  bool
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 5119a3277..b8cd11378 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -3263,6 +3263,14 @@ main(int argc, char *argv[])
> >      engine_add_input(&en_pflow_output, &en_ovs_open_vswitch, NULL);
> >      engine_add_input(&en_pflow_output, &en_ovs_bridge, NULL);
> >
> > +    /* Keep en_addr_sets before en_runtime_data because
> > +     * lflow_output_runtime_data_handler may *partially* reprocess a
lflow when
> > +     * the lflow is attached to a DP group and a new DP in that DP
group is
> > +     * added locally, i.e. reprocessing the lflow for the new DP only
but not
> > +     * for the other DPs in the group. If we handle en_addr_sets after
this,
> > +     * incrementally processing an updated address set for the added
IPs may
> > +     * end up adding redundant flows/conjunctions for the lflow agaist
the new
> > +     * DP because it has been processed on the DP already. */
> >      engine_add_input(&en_lflow_output, &en_addr_sets,
> >                       lflow_output_addr_sets_handler);
> >      engine_add_input(&en_lflow_output, &en_port_groups,
> > diff --git a/include/ovn/expr.h b/include/ovn/expr.h
> > index cb3baf001..3b141b034 100644
> > --- a/include/ovn/expr.h
> > +++ b/include/ovn/expr.h
> > @@ -484,6 +484,7 @@ uint32_t expr_to_matches(const struct expr *,
> >                                               unsigned int *portp),
> >                           const void *aux,
> >                           struct hmap *matches);
> > +void expr_match_destroy(struct expr_match *);
> >  void expr_matches_destroy(struct hmap *matches);
> >  size_t expr_matches_prepare(struct hmap *matches, uint32_t
conj_id_ofs);
> >  void expr_matches_print(const struct hmap *matches, FILE *);
> > diff --git a/lib/expr.c b/lib/expr.c
> > index 2c178d87f..47ef6108e 100644
> > --- a/lib/expr.c
> > +++ b/lib/expr.c
> > @@ -3054,7 +3054,7 @@ expr_match_new(const struct match *m, uint8_t
clause, uint8_t n_clauses,
> >      return match;
> >  }
> >
> > -static void
> > +void
> >  expr_match_destroy(struct expr_match *match)
> >  {
> >      free(match->as_name);
> > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> > index b5f19b442..260986f15 100644
> > --- a/tests/ovn-controller.at
> > +++ b/tests/ovn-controller.at
> > @@ -904,7 +904,7 @@
priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3
actions=dr
> >  done
> >
> >  reprocess_count_new=$(read_counter consider_logical_flow)
> > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[10
> > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[2
> >  ])
> >
> >  # Remove the IPs from as1, 1 IP each time.
> > @@ -955,7 +955,8 @@
priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.1.3
actions=dr
> >  done
> >
> >  reprocess_count_new=$(read_counter consider_logical_flow)
> > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[10
> > +# When change from 0 to 2, still reprocessing.
> > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[1
> >  ])
> >
> >  # Add and remove IPs at the same time.
> > @@ -973,7 +974,7 @@ AT_CHECK([ovs-ofctl dump-flows br-int table=44 |
grep -c 10\.0\.0\.22], [0], [1
> >  AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10\.0\.0\.10],
[1], [ignore])
> >
> >  reprocess_count_new=$(read_counter consider_logical_flow)
> > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[1
> > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[0
> >  ])
> >
> >  # Add 1 and remove 2
> > @@ -988,7 +989,7 @@ AT_CHECK([ovs-ofctl dump-flows br-int table=44 |
grep -c 10\.0\.0\.10], [0], [1
> >  ])
> >
> >  reprocess_count_new=$(read_counter consider_logical_flow)
> > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[1
> > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[0
> >  ])
> >
> >  # Add 1 and remove 1
> > @@ -1002,7 +1003,7 @@ AT_CHECK([ovs-ofctl dump-flows br-int table=44 |
grep -c 10\.0\.0\.21], [0], [1
> >  AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10\.0\.0\.10],
[1], [ignore])
> >
> >  reprocess_count_new=$(read_counter consider_logical_flow)
> > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[1
> > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[0
> >  ])
> >
> >  # Add 2 and remove 2
> > @@ -1019,7 +1020,7 @@ AT_CHECK([ovs-ofctl dump-flows br-int table=44 |
grep 10\.0\.0\.8], [1], [ignore
> >  AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10\.0\.0\.9],
[1], [ignore])
> >
> >  reprocess_count_new=$(read_counter consider_logical_flow)
> > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[1
> > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[0
> >  ])
> >
> >  OVN_CLEANUP([hv1])
> > @@ -1093,7 +1094,7 @@
priority=1100,tcp,reg15=0x$port_key,metadata=0x$dp_key,tp_dst=333
actions=conjun
> >  done
> >
> >  reprocess_count_new=$(read_counter consider_logical_flow)
> > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[10
> > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[2
> >  ])
> >
> >  # Remove the IPs from as1, 1 IP each time.
> > @@ -1150,7 +1151,7 @@
priority=1100,tcp,reg15=0x$port_key,metadata=0x$dp_key,tp_dst=333
actions=conjun
> >  done
> >
> >  reprocess_count_new=$(read_counter consider_logical_flow)
> > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[10
> > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[1
> >  ])
> >
> >  # Add and remove IPs at the same time.
> > @@ -1168,7 +1169,7 @@ AT_CHECK([ovs-ofctl dump-flows br-int table=44 |
grep -c 10\.0\.0\.22], [0], [1
> >  AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10\.0\.0\.10],
[1], [ignore])
> >
> >  reprocess_count_new=$(read_counter consider_logical_flow)
> > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[1
> > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[0
> >  ])
> >
> >  # Add 1 and remove 2
> > @@ -1183,7 +1184,7 @@ AT_CHECK([ovs-ofctl dump-flows br-int table=44 |
grep -c 10\.0\.0\.10], [0], [1
> >  ])
> >
> >  reprocess_count_new=$(read_counter consider_logical_flow)
> > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[1
> > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[0
> >  ])
> >
> >  # Add 1 and remove 1
> > @@ -1197,7 +1198,7 @@ AT_CHECK([ovs-ofctl dump-flows br-int table=44 |
grep -c 10\.0\.0\.21], [0], [1
> >  AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10\.0\.0\.10],
[1], [ignore])
> >
> >  reprocess_count_new=$(read_counter consider_logical_flow)
> > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[1
> > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[0
> >  ])
> >
> >  # Add 2 and remove 2
> > @@ -1214,7 +1215,7 @@ AT_CHECK([ovs-ofctl dump-flows br-int table=44 |
grep 10\.0\.0\.8], [1], [ignore
> >  AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10\.0\.0\.9],
[1], [ignore])
> >
> >  reprocess_count_new=$(read_counter consider_logical_flow)
> > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[1
> > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[0
> >  ])
> >
> >  OVN_CLEANUP([hv1])
> > @@ -1288,7 +1289,7 @@
priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3
actions=co
> >  done
> >
> >  reprocess_count_new=$(read_counter consider_logical_flow)
> > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[10
> > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[2
> >  ])
> >
> >  # Remove the IPs from as1 and as2, 1 IP each time.
> > @@ -1341,7 +1342,7 @@
priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3,nw_dst=10.
> >  done
> >
> >  reprocess_count_new=$(read_counter consider_logical_flow)
> > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[9
> > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[1
> >  ])
> >
> >  # Add 1 more IP back to as2
> > @@ -1639,7 +1640,7 @@
priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3
actions=co
> >  done
> >
> >  reprocess_count_new=$(read_counter consider_logical_flow)
> > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[10
> > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[2
> >  ])
> >
> >  # Remove the IPs from as1, 1 IP each time.
> > @@ -1697,7 +1698,7 @@
priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.1.3
actions=co
> >  done
> >
> >  reprocess_count_new=$(read_counter consider_logical_flow)
> > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[10
> > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[1
> >  ])
> >
> >  # Test the case when there are two references to the same AS but one
of the
> > @@ -1842,7 +1843,7 @@
priority=1100,tcp,reg15=0x$port_key,metadata=0x$dp_key,tp_dst=202
actions=conjun
> >  ])
> >
> >  reprocess_count_new=$(read_counter consider_logical_flow)
> > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[2
> > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[0
> >  ])
> >
> >  # Remove those 2 IPs from each AS, should return to the initial state
> > @@ -1870,6 +1871,8 @@
priority=1100,tcp,reg15=0x$port_key,metadata=0x$dp_key,tp_dst=202
actions=conjun
> >  ])
> >
> >  reprocess_count_new=$(read_counter consider_logical_flow)
> > +# Because of the combined conjunction, AS cannot be tracked for the
flow for
> > +# 10.0.0.33, so removing would trigger reprocessing.
> >  AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[2
> >  ])
> >
> > @@ -1926,7 +1929,7 @@
priority=1100,reg15=0x$port_key,metadata=0x$dp_key,dl_src=aa:aa:aa:aa:aa:03
acti
> >  done
> >
> >  reprocess_count_new=$(read_counter consider_logical_flow)
> > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[5
> > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[2
> >  ])
> >
> >  # Remove the MACs from as1.
> > @@ -2007,7 +2010,7 @@
priority=1100,ipv6,reg15=0x$port_key,metadata=0x$dp_key,ipv6_src=ff::3
actions=d
> >  done
> >
> >  reprocess_count_new=$(read_counter consider_logical_flow)
> > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[5
> > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[2
> >  ])
> >
> >  # Remove the IPs from as1, 1 IP each time.
> > --
> > 2.30.2
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
diff mbox series

Patch

diff --git a/controller/lflow-conj-ids.c b/controller/lflow-conj-ids.c
index bfe63862a..372391e5a 100644
--- a/controller/lflow-conj-ids.c
+++ b/controller/lflow-conj-ids.c
@@ -157,6 +157,26 @@  lflow_conj_ids_alloc_specified(struct conj_ids *conj_ids,
     return true;
 }
 
+/* Find and return the start id that is allocated to the logical flow. Return
+ * 0 if not found. */
+uint32_t
+lflow_conj_ids_find(struct conj_ids *conj_ids, const struct uuid *lflow_uuid)
+{
+    struct lflow_conj_node *lflow_conj;
+    bool found = false;
+    HMAP_FOR_EACH_WITH_HASH (lflow_conj, hmap_node, uuid_hash(lflow_uuid),
+                             &conj_ids->lflow_conj_ids) {
+        if (uuid_equals(&lflow_conj->lflow_uuid, lflow_uuid)) {
+            found = true;
+            break;
+        }
+    }
+    if (!found) {
+        return 0;
+    }
+    return lflow_conj->start_conj_id;
+}
+
 /* Frees the conjunction IDs used by lflow_uuid. */
 void
 lflow_conj_ids_free(struct conj_ids *conj_ids, const struct uuid *lflow_uuid)
diff --git a/controller/lflow-conj-ids.h b/controller/lflow-conj-ids.h
index 6da0a612c..a1f7a95a5 100644
--- a/controller/lflow-conj-ids.h
+++ b/controller/lflow-conj-ids.h
@@ -35,6 +35,7 @@  bool lflow_conj_ids_alloc_specified(struct conj_ids *,
                                     const struct uuid *lflow_uuid,
                                     uint32_t start_conj_id, uint32_t n_conjs);
 void lflow_conj_ids_free(struct conj_ids *, const struct uuid *lflow_uuid);
+uint32_t lflow_conj_ids_find(struct conj_ids *, const struct uuid *lflow_uuid);
 void lflow_conj_ids_init(struct conj_ids *);
 void lflow_conj_ids_destroy(struct conj_ids *);
 void lflow_conj_ids_clear(struct conj_ids *);
diff --git a/controller/lflow.c b/controller/lflow.c
index 9b1184c0e..658b3109b 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -74,6 +74,19 @@  struct condition_aux {
     struct lflow_resource_ref *lfrr;
 };
 
+static struct expr *
+convert_match_to_expr(const struct sbrec_logical_flow *,
+                      const struct sbrec_datapath_binding *,
+                      struct expr **prereqs, const struct shash *addr_sets,
+                      const struct shash *port_groups,
+                      struct lflow_resource_ref *, bool *pg_addr_set_ref);
+static void
+add_matches_to_flow_table(const struct sbrec_logical_flow *,
+                          const struct sbrec_datapath_binding *,
+                          struct hmap *matches, uint8_t ptable,
+                          uint8_t output_ptable, struct ofpbuf *ovnacts,
+                          bool ingress, struct lflow_ctx_in *,
+                          struct lflow_ctx_out *);
 static void
 consider_logical_flow(const struct sbrec_logical_flow *lflow,
                       struct hmap *dhcp_opts, struct hmap *dhcpv6_opts,
@@ -513,6 +526,262 @@  as_info_from_expr_const(const char *as_name, const union expr_constant *c,
     return true;
 }
 
+/* Parses the lflow regarding the changed address set 'as_name', and gererates
+ * ovs flows for the newly added addresses in 'as_diff_added' only. It is
+ * similar to consider_logical_flow__, with the below differences:
+ *
+ * - It has one more arg 'as_ref_count' to deduce how many flows are expected
+ *   to be added.
+ * - It uses a small fake address set that contains only the added addresses
+ *   to replace the original address set temporarily and restores it after
+ *   parsing.
+ * - It doesn't check or touch lflow-cache, because lflow-cache is disabled
+ *   when address-sets/port-groups are used.
+ * - It doesn't check non-local lports because it should have been checked
+ *   when the lflow is initially parsed, and if it is non-local and skipped
+ *   then it wouldn't have the address set parsed and referenced.
+ *
+ * Because of these differences, it is just cleaner to keep it as a separate
+ * function. */
+static bool
+consider_lflow_for_added_as_ips__(
+                        const struct sbrec_logical_flow *lflow,
+                        const struct sbrec_datapath_binding *dp,
+                        const char *as_name,
+                        size_t as_ref_count,
+                        const struct expr_constant_set *as_diff_added,
+                        struct hmap *dhcp_opts,
+                        struct hmap *dhcpv6_opts,
+                        struct hmap *nd_ra_opts,
+                        struct controller_event_options *controller_event_opts,
+                        struct lflow_ctx_in *l_ctx_in,
+                        struct lflow_ctx_out *l_ctx_out)
+{
+    bool handled = true;
+    if (!get_local_datapath(l_ctx_in->local_datapaths, dp->tunnel_key)) {
+        VLOG_DBG("Skip lflow "UUID_FMT" for non-local datapath %"PRId64,
+                 UUID_ARGS(&lflow->header_.uuid), dp->tunnel_key);
+        return true;
+    }
+
+    /* Determine translation of logical table IDs to physical table IDs. */
+    bool ingress = !strcmp(lflow->pipeline, "ingress");
+
+    /* Determine translation of logical table IDs to physical table IDs. */
+    uint8_t first_ptable = (ingress
+                            ? OFTABLE_LOG_INGRESS_PIPELINE
+                            : OFTABLE_LOG_EGRESS_PIPELINE);
+    uint8_t ptable = first_ptable + lflow->table_id;
+    uint8_t output_ptable = (ingress
+                             ? OFTABLE_REMOTE_OUTPUT
+                             : OFTABLE_SAVE_INPORT);
+
+    uint64_t ovnacts_stub[1024 / 8];
+    struct ofpbuf ovnacts = OFPBUF_STUB_INITIALIZER(ovnacts_stub);
+    struct ovnact_parse_params pp = {
+        .symtab = &symtab,
+        .dhcp_opts = dhcp_opts,
+        .dhcpv6_opts = dhcpv6_opts,
+        .nd_ra_opts = nd_ra_opts,
+        .controller_event_opts = controller_event_opts,
+
+        .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS,
+        .n_tables = LOG_PIPELINE_LEN,
+        .cur_ltable = lflow->table_id,
+    };
+    struct expr *prereqs = NULL;
+    char *error;
+
+    error = ovnacts_parse_string(lflow->actions, &pp, &ovnacts, &prereqs);
+    if (error) {
+        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
+        VLOG_WARN_RL(&rl, "error parsing actions \"%s\": %s",
+                     lflow->actions, error);
+        free(error);
+        ovnacts_free(ovnacts.data, ovnacts.size);
+        ofpbuf_uninit(&ovnacts);
+        return true;
+    }
+
+    struct lookup_port_aux aux = {
+        .sbrec_multicast_group_by_name_datapath
+            = l_ctx_in->sbrec_multicast_group_by_name_datapath,
+        .sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name,
+        .dp = dp,
+        .lflow = lflow,
+        .lfrr = l_ctx_out->lfrr,
+    };
+    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,
+        .lfrr = l_ctx_out->lfrr,
+    };
+
+    struct hmap matches = HMAP_INITIALIZER(&matches);
+    const struct expr_constant_set *fake_as = as_diff_added;
+    struct expr_constant_set *new_fake_as = NULL;
+    struct in6_addr dummy_ip;
+    bool has_dummy_ip = false;
+    ovs_assert(as_diff_added->n_values);
+
+    /* When there is only 1 element, we append a dummy address and create a
+     * fake address set with 2 elements, so that the lflow parsing would
+     * generate exactly the same format of flows as it would when parsing with
+     * the original address set. */
+    if (as_diff_added->n_values == 1) {
+        new_fake_as = xzalloc(sizeof *new_fake_as);
+        new_fake_as->values = xzalloc(sizeof *new_fake_as->values * 2);
+        new_fake_as->n_values = 2;
+        new_fake_as->values[0] = new_fake_as->values[1] =
+            as_diff_added->values[0];
+        /* Make a dummy ip that is different from the real one. */
+        new_fake_as->values[1].value.u8_val++;
+        dummy_ip = new_fake_as->values[1].value.ipv6;
+        has_dummy_ip = true;
+        fake_as = new_fake_as;
+    }
+
+    /* Temporarily replace the address set in addr_sets with the fake_as, so
+     * that the cost of lflow parsing is related to the delta but not the
+     * original size of the address set. It is possible that there are other
+     * address sets used by this logical flow and their size can be big. In
+     * such case the parsing cost is still high. In practice, big address
+     * sets are likely to be updated more frequently that small address sets,
+     * so this approach should still be effetive overall.
+     *
+     * XXX: if necessary, we can optimize this by checking all the address set
+     * references in this lflow, and replace all the "big" address sets with a
+     * small faked one. */
+    struct expr_constant_set *real_as =
+        shash_replace((struct shash *)l_ctx_in->addr_sets, as_name, fake_as);
+    /* We are here because of the address set update, so it must be found. */
+    ovs_assert(real_as);
+
+    struct expr *expr = convert_match_to_expr(lflow, dp, &prereqs,
+                                              l_ctx_in->addr_sets,
+                                              l_ctx_in->port_groups,
+                                              l_ctx_out->lfrr, NULL);
+    shash_replace((struct shash *)l_ctx_in->addr_sets, as_name, real_as);
+    if (new_fake_as) {
+        expr_constant_set_destroy(new_fake_as);
+        free(new_fake_as);
+    }
+    if (!expr) {
+        goto done;
+    }
+
+    expr = expr_evaluate_condition(expr, is_chassis_resident_cb,
+                                   &cond_aux);
+    expr = expr_normalize(expr);
+
+    uint32_t start_conj_id = 0;
+    uint32_t n_conjs = 0;
+    n_conjs = expr_to_matches(expr, lookup_port_cb, &aux, &matches);
+    if (hmap_is_empty(&matches)) {
+        VLOG_DBG("lflow "UUID_FMT" matches are empty, skip",
+                 UUID_ARGS(&lflow->header_.uuid));
+        goto done;
+    }
+
+    /* Discard the matches unrelated to the added addresses in the AS
+     * 'as_name'. */
+    struct expr_match *m, *m_next;
+    HMAP_FOR_EACH_SAFE (m, m_next, hmap_node, &matches) {
+        if (!m->as_name || strcmp(m->as_name, as_name) ||
+            (has_dummy_ip && !memcmp(&m->as_ip, &dummy_ip, sizeof dummy_ip))) {
+            hmap_remove(&matches, &m->hmap_node);
+            expr_match_destroy(m);
+            continue;
+        }
+    }
+
+    /* The number of matches generated by the new addresses should match the
+     * number of items in the as_diff_added and the reference count of the AS
+     * in this lflow. Otherwise, it means we hit some complex/corner cases that
+     * the generated matches can't be mapped from the items in the
+     * as_diff_added. So we need to fall back to reprocessing the lflow.
+     */
+    if (hmap_count(&matches) != as_ref_count * as_diff_added->n_values) {
+        VLOG_DBG("lflow "UUID_FMT", addrset %s: Generated flows count "
+                 "(%"PRIuSIZE") " "doesn't match added addresses count "
+                 "(%"PRIuSIZE") and ref_count (%"PRIuSIZE"). "
+                 "Need reprocessing.",
+                 UUID_ARGS(&lflow->header_.uuid), as_name,
+                 hmap_count(&matches), as_diff_added->n_values, as_ref_count);
+        handled = false;
+        goto done;
+    }
+    if (n_conjs) {
+        start_conj_id = lflow_conj_ids_find(l_ctx_out->conj_ids,
+                                            &lflow->header_.uuid);
+        if (!start_conj_id) {
+            VLOG_DBG("lflow "UUID_FMT" didn't have conjunctions. "
+                     "Need reprocessing", UUID_ARGS(&lflow->header_.uuid));
+            handled = false;
+            goto done;
+        }
+        expr_matches_prepare(&matches, start_conj_id - 1);
+    }
+    add_matches_to_flow_table(lflow, dp, &matches, ptable, output_ptable,
+                              &ovnacts, ingress, l_ctx_in, l_ctx_out);
+done:
+    expr_destroy(prereqs);
+    ovnacts_free(ovnacts.data, ovnacts.size);
+    ofpbuf_uninit(&ovnacts);
+    expr_destroy(expr);
+    expr_matches_destroy(&matches);
+    return handled;
+}
+
+static bool
+consider_lflow_for_added_as_ips(
+                        const struct sbrec_logical_flow *lflow,
+                        const char *as_name,
+                        size_t as_ref_count,
+                        const struct expr_constant_set *as_diff_added,
+                        struct hmap *dhcp_opts,
+                        struct hmap *dhcpv6_opts,
+                        struct hmap *nd_ra_opts,
+                        struct controller_event_options *controller_event_opts,
+                        struct lflow_ctx_in *l_ctx_in,
+                        struct lflow_ctx_out *l_ctx_out)
+{
+    const struct sbrec_logical_dp_group *dp_group = lflow->logical_dp_group;
+    const struct sbrec_datapath_binding *dp = lflow->logical_datapath;
+
+    if (!dp_group && !dp) {
+        VLOG_DBG("lflow "UUID_FMT" has no datapath binding, skip",
+                 UUID_ARGS(&lflow->header_.uuid));
+        return true;
+    }
+    ovs_assert(!dp_group || !dp);
+
+    if (dp) {
+        return consider_lflow_for_added_as_ips__(lflow, dp, as_name,
+                                                 as_ref_count, as_diff_added,
+                                                 dhcp_opts, dhcpv6_opts,
+                                                 nd_ra_opts,
+                                                 controller_event_opts,
+                                                 l_ctx_in, l_ctx_out);
+    }
+    for (size_t i = 0; dp_group && i < dp_group->n_datapaths; i++) {
+        if (!consider_lflow_for_added_as_ips__(lflow, dp_group->datapaths[i],
+                                               as_name, as_ref_count,
+                                               as_diff_added, dhcp_opts,
+                                               dhcpv6_opts, nd_ra_opts,
+                                               controller_event_opts, l_ctx_in,
+                                               l_ctx_out)) {
+            return false;
+        }
+    }
+    return true;
+}
+
+/* Check if an address set update can be handled without reprocessing the
+ * lflow. */
 static bool
 as_update_can_be_handled(const char *as_name, struct addr_set_diff *as_diff,
                          struct lflow_ctx_in *l_ctx_in)
@@ -582,19 +851,18 @@  as_update_can_be_handled(const char *as_name, struct addr_set_diff *as_diff,
  *        It could have been combined as:
  *
  *          ip.src == $as1 && tcp.dst == {p1, p2, p3, p4}
+ *
+ *        Note: addresses additions still can be processed incrementally in
+ *        this case, although deletions cannot.
  */
 bool
 lflow_handle_addr_set_update(const char *as_name,
                              struct addr_set_diff *as_diff,
-                             struct lflow_ctx_in *l_ctx_in OVS_UNUSED,
+                             struct lflow_ctx_in *l_ctx_in,
                              struct lflow_ctx_out *l_ctx_out,
                              bool *changed)
 {
-    if (as_diff->added) {
-        return false;
-    }
-    ovs_assert(as_diff->deleted);
-
+    ovs_assert(as_diff->added || as_diff->deleted);
     if (!as_update_can_be_handled(as_name, as_diff, l_ctx_in)) {
         return false;
     }
@@ -609,6 +877,31 @@  lflow_handle_addr_set_update(const char *as_name,
 
     *changed = false;
 
+    struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
+    struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts);
+    struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts);
+    struct controller_event_options controller_event_opts;
+
+    if (as_diff->added) {
+        const struct sbrec_dhcp_options *dhcp_opt_row;
+        SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row,
+                                           l_ctx_in->dhcp_options_table) {
+            dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code,
+                         dhcp_opt_row->type);
+        }
+
+        const struct sbrec_dhcpv6_options *dhcpv6_opt_row;
+        SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH(dhcpv6_opt_row,
+                                            l_ctx_in->dhcpv6_options_table) {
+           dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name,
+                        dhcpv6_opt_row->code, dhcpv6_opt_row->type);
+        }
+
+        nd_ra_opts_init(&nd_ra_opts);
+        controller_event_opts_init(&controller_event_opts);
+    }
+
+    bool ret = true;
     struct lflow_ref_list_node *lrln;
     HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) {
         if (lflows_processed_find(l_ctx_out->lflows_processed,
@@ -617,21 +910,56 @@  lflow_handle_addr_set_update(const char *as_name,
                      UUID_ARGS(&lrln->lflow_uuid));
             continue;
         }
-        for (size_t i = 0; i < as_diff->deleted->n_values; i++) {
-            union expr_constant *c = &as_diff->deleted->values[i];
+        const struct sbrec_logical_flow *lflow =
+            sbrec_logical_flow_table_get_for_uuid(l_ctx_in->logical_flow_table,
+                                                  &lrln->lflow_uuid);
+        if (!lflow) {
+            /* lflow deletion should be handled in the corresponding input
+             * handler, so we can skip here. */
+            VLOG_DBG("lflow "UUID_FMT" not found while handling updates of "
+                     "address set %s, skip.",
+                     UUID_ARGS(&lrln->lflow_uuid), as_name);
+            continue;
+        }
+        *changed = true;
+
+        if (as_diff->deleted) {
             struct addrset_info as_info;
-            if (!as_info_from_expr_const(as_name, c, &as_info)) {
-                continue;
+            for (size_t i = 0; i < as_diff->deleted->n_values; i++) {
+                union expr_constant *c = &as_diff->deleted->values[i];
+                if (!as_info_from_expr_const(as_name, c, &as_info)) {
+                    continue;
+                }
+                if (!ofctrl_remove_flows_for_as_ip(l_ctx_out->flow_table,
+                                                   &lrln->lflow_uuid, &as_info,
+                                                   lrln->ref_count)) {
+                    ret = false;
+                    goto done;
+                }
             }
-            if (!ofctrl_remove_flows_for_as_ip(l_ctx_out->flow_table,
-                                               &lrln->lflow_uuid, &as_info,
-                                               lrln->ref_count)) {
-                return false;
+        }
+
+        if (as_diff->added) {
+            if (!consider_lflow_for_added_as_ips(lflow, as_name,
+                                                 lrln->ref_count,
+                                                 as_diff->added, &dhcp_opts,
+                                                 &dhcpv6_opts, &nd_ra_opts,
+                                                 &controller_event_opts,
+                                                 l_ctx_in, l_ctx_out)) {
+                ret = false;
+                goto done;
             }
-            *changed = true;
         }
     }
-    return true;
+
+done:
+    if (as_diff->added) {
+        dhcp_opts_destroy(&dhcp_opts);
+        dhcp_opts_destroy(&dhcpv6_opts);
+        nd_ra_opts_destroy(&nd_ra_opts);
+        controller_event_opts_destroy(&controller_event_opts);
+    }
+    return ret;
 }
 
 bool
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 5119a3277..b8cd11378 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -3263,6 +3263,14 @@  main(int argc, char *argv[])
     engine_add_input(&en_pflow_output, &en_ovs_open_vswitch, NULL);
     engine_add_input(&en_pflow_output, &en_ovs_bridge, NULL);
 
+    /* Keep en_addr_sets before en_runtime_data because
+     * lflow_output_runtime_data_handler may *partially* reprocess a lflow when
+     * the lflow is attached to a DP group and a new DP in that DP group is
+     * added locally, i.e. reprocessing the lflow for the new DP only but not
+     * for the other DPs in the group. If we handle en_addr_sets after this,
+     * incrementally processing an updated address set for the added IPs may
+     * end up adding redundant flows/conjunctions for the lflow agaist the new
+     * DP because it has been processed on the DP already. */
     engine_add_input(&en_lflow_output, &en_addr_sets,
                      lflow_output_addr_sets_handler);
     engine_add_input(&en_lflow_output, &en_port_groups,
diff --git a/include/ovn/expr.h b/include/ovn/expr.h
index cb3baf001..3b141b034 100644
--- a/include/ovn/expr.h
+++ b/include/ovn/expr.h
@@ -484,6 +484,7 @@  uint32_t expr_to_matches(const struct expr *,
                                              unsigned int *portp),
                          const void *aux,
                          struct hmap *matches);
+void expr_match_destroy(struct expr_match *);
 void expr_matches_destroy(struct hmap *matches);
 size_t expr_matches_prepare(struct hmap *matches, uint32_t conj_id_ofs);
 void expr_matches_print(const struct hmap *matches, FILE *);
diff --git a/lib/expr.c b/lib/expr.c
index 2c178d87f..47ef6108e 100644
--- a/lib/expr.c
+++ b/lib/expr.c
@@ -3054,7 +3054,7 @@  expr_match_new(const struct match *m, uint8_t clause, uint8_t n_clauses,
     return match;
 }
 
-static void
+void
 expr_match_destroy(struct expr_match *match)
 {
     free(match->as_name);
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index b5f19b442..260986f15 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -904,7 +904,7 @@  priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3 actions=dr
 done
 
 reprocess_count_new=$(read_counter consider_logical_flow)
-AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [10
+AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [2
 ])
 
 # Remove the IPs from as1, 1 IP each time.
@@ -955,7 +955,8 @@  priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.1.3 actions=dr
 done
 
 reprocess_count_new=$(read_counter consider_logical_flow)
-AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [10
+# When change from 0 to 2, still reprocessing.
+AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
 ])
 
 # Add and remove IPs at the same time.
@@ -973,7 +974,7 @@  AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep -c 10\.0\.0\.22], [0], [1
 AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10\.0\.0\.10], [1], [ignore])
 
 reprocess_count_new=$(read_counter consider_logical_flow)
-AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
+AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0
 ])
 
 # Add 1 and remove 2
@@ -988,7 +989,7 @@  AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep -c 10\.0\.0\.10], [0], [1
 ])
 
 reprocess_count_new=$(read_counter consider_logical_flow)
-AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
+AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0
 ])
 
 # Add 1 and remove 1
@@ -1002,7 +1003,7 @@  AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep -c 10\.0\.0\.21], [0], [1
 AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10\.0\.0\.10], [1], [ignore])
 
 reprocess_count_new=$(read_counter consider_logical_flow)
-AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
+AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0
 ])
 
 # Add 2 and remove 2
@@ -1019,7 +1020,7 @@  AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10\.0\.0\.8], [1], [ignore
 AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10\.0\.0\.9], [1], [ignore])
 
 reprocess_count_new=$(read_counter consider_logical_flow)
-AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
+AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0
 ])
 
 OVN_CLEANUP([hv1])
@@ -1093,7 +1094,7 @@  priority=1100,tcp,reg15=0x$port_key,metadata=0x$dp_key,tp_dst=333 actions=conjun
 done
 
 reprocess_count_new=$(read_counter consider_logical_flow)
-AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [10
+AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [2
 ])
 
 # Remove the IPs from as1, 1 IP each time.
@@ -1150,7 +1151,7 @@  priority=1100,tcp,reg15=0x$port_key,metadata=0x$dp_key,tp_dst=333 actions=conjun
 done
 
 reprocess_count_new=$(read_counter consider_logical_flow)
-AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [10
+AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
 ])
 
 # Add and remove IPs at the same time.
@@ -1168,7 +1169,7 @@  AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep -c 10\.0\.0\.22], [0], [1
 AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10\.0\.0\.10], [1], [ignore])
 
 reprocess_count_new=$(read_counter consider_logical_flow)
-AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
+AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0
 ])
 
 # Add 1 and remove 2
@@ -1183,7 +1184,7 @@  AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep -c 10\.0\.0\.10], [0], [1
 ])
 
 reprocess_count_new=$(read_counter consider_logical_flow)
-AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
+AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0
 ])
 
 # Add 1 and remove 1
@@ -1197,7 +1198,7 @@  AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep -c 10\.0\.0\.21], [0], [1
 AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10\.0\.0\.10], [1], [ignore])
 
 reprocess_count_new=$(read_counter consider_logical_flow)
-AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
+AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0
 ])
 
 # Add 2 and remove 2
@@ -1214,7 +1215,7 @@  AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10\.0\.0\.8], [1], [ignore
 AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10\.0\.0\.9], [1], [ignore])
 
 reprocess_count_new=$(read_counter consider_logical_flow)
-AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
+AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0
 ])
 
 OVN_CLEANUP([hv1])
@@ -1288,7 +1289,7 @@  priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3 actions=co
 done
 
 reprocess_count_new=$(read_counter consider_logical_flow)
-AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [10
+AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [2
 ])
 
 # Remove the IPs from as1 and as2, 1 IP each time.
@@ -1341,7 +1342,7 @@  priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3,nw_dst=10.
 done
 
 reprocess_count_new=$(read_counter consider_logical_flow)
-AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [9
+AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
 ])
 
 # Add 1 more IP back to as2
@@ -1639,7 +1640,7 @@  priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3 actions=co
 done
 
 reprocess_count_new=$(read_counter consider_logical_flow)
-AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [10
+AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [2
 ])
 
 # Remove the IPs from as1, 1 IP each time.
@@ -1697,7 +1698,7 @@  priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.1.3 actions=co
 done
 
 reprocess_count_new=$(read_counter consider_logical_flow)
-AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [10
+AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1
 ])
 
 # Test the case when there are two references to the same AS but one of the
@@ -1842,7 +1843,7 @@  priority=1100,tcp,reg15=0x$port_key,metadata=0x$dp_key,tp_dst=202 actions=conjun
 ])
 
 reprocess_count_new=$(read_counter consider_logical_flow)
-AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [2
+AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0
 ])
 
 # Remove those 2 IPs from each AS, should return to the initial state
@@ -1870,6 +1871,8 @@  priority=1100,tcp,reg15=0x$port_key,metadata=0x$dp_key,tp_dst=202 actions=conjun
 ])
 
 reprocess_count_new=$(read_counter consider_logical_flow)
+# Because of the combined conjunction, AS cannot be tracked for the flow for
+# 10.0.0.33, so removing would trigger reprocessing.
 AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [2
 ])
 
@@ -1926,7 +1929,7 @@  priority=1100,reg15=0x$port_key,metadata=0x$dp_key,dl_src=aa:aa:aa:aa:aa:03 acti
 done
 
 reprocess_count_new=$(read_counter consider_logical_flow)
-AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [5
+AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [2
 ])
 
 # Remove the MACs from as1.
@@ -2007,7 +2010,7 @@  priority=1100,ipv6,reg15=0x$port_key,metadata=0x$dp_key,ipv6_src=ff::3 actions=d
 done
 
 reprocess_count_new=$(read_counter consider_logical_flow)
-AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [5
+AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [2
 ])
 
 # Remove the IPs from as1, 1 IP each time.