[ovs-dev] controller: Track individual address set constants.

Instead of tracking address set per struct expr_constant_set track it
per individual struct expr_constant. This allows more fine grained
control for I-P processing of address sets in controller. It helps with
scenarios like matching on two address sets in one expression e.g.
"ip4.src == {$as1, $as2}". This allows any addition or removal of
individual adress from the set to be incrementally processed instead
of reprocessing all the flows.

This unfortunately doesn't help with the following flows:
"ip4.src == $as1 && ip4.dst == $as2"
"ip4.src == $as1 || ip4.dst == $as2"

The memory impact should be minimal as there is only increase of 8 bytes
per the struct expr_constant.

Signed-off-by: Ales Musil <amusil@redhat.com>
---
 controller/lflow.c      |  4 +-
 include/ovn/actions.h   |  2 +-
 include/ovn/expr.h      | 46 ++++++++++----------
 lib/actions.c           | 20 ++++-----
 lib/expr.c              | 95 +++++++++++++++++------------------------
 tests/ovn-controller.at | 14 +++---
 6 files changed, 83 insertions(+), 98 deletions(-)

On Tue, Mar 19, 2024 at 5:43 PM Ales Musil <amusil@redhat.com> wrote:

> Instead of tracking address set per struct expr_constant_set track it
> per individual struct expr_constant. This allows more fine grained
> control for I-P processing of address sets in controller. It helps with
> scenarios like matching on two address sets in one expression e.g.
> "ip4.src == {$as1, $as2}". This allows any addition or removal of
> individual adress from the set to be incrementally processed instead
> of reprocessing all the flows.
>
> This unfortunately doesn't help with the following flows:
> "ip4.src == $as1 && ip4.dst == $as2"
> "ip4.src == $as1 || ip4.dst == $as2"
>
> The memory impact should be minimal as there is only increase of 8 bytes
> per the struct expr_constant.
>
> Signed-off-by: Ales Musil <amusil@redhat.com>
> ---
>  controller/lflow.c      |  4 +-
>  include/ovn/actions.h   |  2 +-
>  include/ovn/expr.h      | 46 ++++++++++----------
>  lib/actions.c           | 20 ++++-----
>  lib/expr.c              | 95 +++++++++++++++++------------------------
>  tests/ovn-controller.at | 14 +++---
>  6 files changed, 83 insertions(+), 98 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 895d17d19..730dc879d 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -278,7 +278,7 @@ lflow_handle_changed_flows(struct lflow_ctx_in
> *l_ctx_in,
>  }
>
>  static bool
> -as_info_from_expr_const(const char *as_name, const union expr_constant *c,
> +as_info_from_expr_const(const char *as_name, const struct expr_constant
> *c,
>                          struct addrset_info *as_info)
>  {
>      as_info->name = as_name;
> @@ -714,7 +714,7 @@ lflow_handle_addr_set_update(const char *as_name,
>          if (as_diff->deleted) {
>              struct addrset_info as_info;
>              for (size_t i = 0; i < as_diff->deleted->n_values; i++) {
> -                union expr_constant *c = &as_diff->deleted->values[i];
> +                struct expr_constant *c = &as_diff->deleted->values[i];
>                  if (!as_info_from_expr_const(as_name, c, &as_info)) {
>                      continue;
>                  }
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index dcacbb1ff..1e20f9b81 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -238,7 +238,7 @@ struct ovnact_next {
>  struct ovnact_load {
>      struct ovnact ovnact;
>      struct expr_field dst;
> -    union expr_constant imm;
> +    struct expr_constant imm;
>  };
>
>  /* OVNACT_MOVE, OVNACT_EXCHANGE. */
> diff --git a/include/ovn/expr.h b/include/ovn/expr.h
> index c48f82398..e54edb5bf 100644
> --- a/include/ovn/expr.h
> +++ b/include/ovn/expr.h
> @@ -368,7 +368,7 @@ bool expr_relop_from_token(enum lex_type type, enum
> expr_relop *relop);
>  struct expr {
>      struct ovs_list node;       /* In parent EXPR_T_AND or EXPR_T_OR if
> any. */
>      enum expr_type type;        /* Expression type. */
> -    char *as_name;              /* Address set name. Null if it is not an
> +    const char *as_name;        /* Address set name. Null if it is not an
>                                     address set. */
>
>      union {
> @@ -505,40 +505,42 @@ enum expr_constant_type {
>  };
>
>  /* A string or integer constant (one must know which from context). */
> -union expr_constant {
> -    /* Integer constant.
> -     *
> -     * The width of a constant isn't always clear, e.g. if you write "1",
> -     * there's no way to tell whether you mean for that to be a 1-bit
> constant
> -     * or a 128-bit constant or somewhere in between. */
> -    struct {
> -        union mf_subvalue value;
> -        union mf_subvalue mask; /* Only initialized if 'masked'. */
> -        bool masked;
> -
> -        enum lex_format format; /* From the constant's lex_token. */
> -    };
> +struct expr_constant {
> +    const char *as_name;
>
> -    /* Null-terminated string constant. */
> -    char *string;
> +    union {
> +        /* Integer constant.
> +         *
> +         * The width of a constant isn't always clear, e.g. if you write
> "1",
> +         * there's no way to tell whether you mean for that to be a 1-bit
> +         * constant or a 128-bit constant or somewhere in between. */
> +        struct {
> +            union mf_subvalue value;
> +            union mf_subvalue mask; /* Only initialized if 'masked'. */
> +            bool masked;
> +
> +            enum lex_format format; /* From the constant's lex_token. */
> +        };
> +
> +        /* Null-terminated string constant. */
> +        char *string;
> +    };
>  };
>
>  bool expr_constant_parse(struct lexer *,
>                           const struct expr_field *,
> -                         union expr_constant *);
> -void expr_constant_format(const union expr_constant *,
> +                         struct expr_constant *);
> +void expr_constant_format(const struct expr_constant *,
>                            enum expr_constant_type, struct ds *);
> -void expr_constant_destroy(const union expr_constant *,
> +void expr_constant_destroy(const struct expr_constant *,
>                             enum expr_constant_type);
>
>  /* A collection of "union expr_constant"s of the same type. */
>  struct expr_constant_set {
> -    union expr_constant *values;  /* Constants. */
> +    struct expr_constant *values; /* Constants. */
>      size_t n_values;              /* Number of constants. */
>      enum expr_constant_type type; /* Type of the constants. */
>      bool in_curlies;              /* Whether the constants were in {}. */
> -    char *as_name;                /* Name of an address set. It is NULL
> if not
> -                                     an address set. */
>  };
>
>  bool expr_constant_set_parse(struct lexer *, struct expr_constant_set *);
> diff --git a/lib/actions.c b/lib/actions.c
> index 71615fc53..6574c4df4 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -450,7 +450,7 @@ encode_LOAD(const struct ovnact_load *load,
>              const struct ovnact_encode_params *ep,
>              struct ofpbuf *ofpacts)
>  {
> -    const union expr_constant *c = &load->imm;
> +    const struct expr_constant *c = &load->imm;
>      struct mf_subfield dst = expr_resolve_field(&load->dst);
>      struct ofpact_set_field *sf = ofpact_put_set_field(ofpacts, dst.field,
>                                                         NULL, NULL);
> @@ -2122,7 +2122,7 @@ encode_event_empty_lb_backends_opts(struct ofpbuf
> *ofpacts,
>          /* All empty_lb_backends fields are of type 'str' */
>          ovs_assert(!strcmp(o->option->type, "str"));
>
> -        const union expr_constant *c = o->value.values;
> +        const struct expr_constant *c = o->value.values;
>          size_t size = strlen(c->string);
>          struct controller_event_opt_header hdr =
>              (struct controller_event_opt_header) {
> @@ -2598,7 +2598,7 @@ validate_empty_lb_backends(struct action_context
> *ctx,
>  {
>      for (size_t i = 0; i < n_options; i++) {
>          const struct ovnact_gen_option *o = &options[i];
> -        const union expr_constant *c = o->value.values;
> +        const struct expr_constant *c = o->value.values;
>          struct sockaddr_storage ss;
>          struct uuid uuid;
>
> @@ -2801,7 +2801,7 @@ encode_put_dhcpv4_option(const struct
> ovnact_gen_option *o,
>      uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2);
>      opt_header[0] = o->option->code;
>
> -    const union expr_constant *c = o->value.values;
> +    const struct expr_constant *c = o->value.values;
>      size_t n_values = o->value.n_values;
>      if (!strcmp(o->option->type, "bool") ||
>          !strcmp(o->option->type, "uint8")) {
> @@ -2967,7 +2967,7 @@ encode_put_dhcpv6_option(const struct
> ovnact_gen_option *o,
>                           struct ofpbuf *ofpacts)
>  {
>      struct dhcpv6_opt_header *opt = ofpbuf_put_uninit(ofpacts, sizeof
> *opt);
> -    const union expr_constant *c = o->value.values;
> +    const struct expr_constant *c = o->value.values;
>      size_t n_values = o->value.n_values;
>      size_t size;
>
> @@ -3023,7 +3023,7 @@ encode_PUT_DHCPV4_OPTS(const struct ovnact_put_opts
> *pdo,
>          find_opt(pdo->options, pdo->n_options, DHCP_OPT_BOOTFILE_CODE);
>      if (boot_opt) {
>          uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2);
> -        const union expr_constant *c = boot_opt->value.values;
> +        const struct expr_constant *c = boot_opt->value.values;
>          opt_header[0] = boot_opt->option->code;
>          opt_header[1] = strlen(c->string);
>          ofpbuf_put(ofpacts, c->string, opt_header[1]);
> @@ -3033,7 +3033,7 @@ encode_PUT_DHCPV4_OPTS(const struct ovnact_put_opts
> *pdo,
>          find_opt(pdo->options, pdo->n_options,
> DHCP_OPT_BOOTFILE_ALT_CODE);
>      if (boot_alt_opt) {
>          uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2);
> -        const union expr_constant *c = boot_alt_opt->value.values;
> +        const struct expr_constant *c = boot_alt_opt->value.values;
>          opt_header[0] = boot_alt_opt->option->code;
>          opt_header[1] = strlen(c->string);
>          ofpbuf_put(ofpacts, c->string, opt_header[1]);
> @@ -3043,7 +3043,7 @@ encode_PUT_DHCPV4_OPTS(const struct ovnact_put_opts
> *pdo,
>          pdo->options, pdo->n_options, DHCP_OPT_NEXT_SERVER_CODE);
>      if (next_server_opt) {
>          uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2);
> -        const union expr_constant *c = next_server_opt->value.values;
> +        const struct expr_constant *c = next_server_opt->value.values;
>          opt_header[0] = next_server_opt->option->code;
>          opt_header[1] = sizeof(ovs_be32);
>          ofpbuf_put(ofpacts, &c->value.ipv4, sizeof(ovs_be32));
> @@ -3247,7 +3247,7 @@ parse_put_nd_ra_opts(struct action_context *ctx,
> const struct expr_field *dst,
>      /* Let's validate the options. */
>      for (size_t i = 0; i < po->n_options; i++) {
>          const struct ovnact_gen_option *o = &po->options[i];
> -        const union expr_constant *c = o->value.values;
> +        const struct expr_constant *c = o->value.values;
>          if (o->value.n_values > 1) {
>              lexer_error(ctx->lexer, "Invalid value for \"%s\" option",
>                          o->option->name);
> @@ -3430,7 +3430,7 @@ static void
>  encode_put_nd_ra_option(const struct ovnact_gen_option *o,
>                          struct ofpbuf *ofpacts, ptrdiff_t ra_offset)
>  {
> -    const union expr_constant *c = o->value.values;
> +    const struct expr_constant *c = o->value.values;
>
>      switch (o->option->code) {
>      case ND_RA_FLAG_ADDR_MODE:
> diff --git a/lib/expr.c b/lib/expr.c
> index 0cb44e1b6..286782025 100644
> --- a/lib/expr.c
> +++ b/lib/expr.c
> @@ -179,12 +179,10 @@ expr_combine(enum expr_type type, struct expr *a,
> struct expr *b)
>          } else {
>              ovs_list_push_back(&a->andor, &b->node);
>          }
> -        free(a->as_name);
>          a->as_name = NULL;
>          return a;
>      } else if (b->type == type) {
>          ovs_list_push_front(&b->andor, &a->node);
> -        free(b->as_name);
>          b->as_name = NULL;
>          return b;
>      } else {
> @@ -521,12 +519,13 @@ static bool parse_field(struct expr_context *,
> struct expr_field *);
>
>  static struct expr *
>  make_cmp__(const struct expr_field *f, enum expr_relop r,
> -             const union expr_constant *c)
> +             const struct expr_constant *c)
>  {
>      struct expr *e = xzalloc(sizeof *e);
>      e->type = EXPR_T_CMP;
>      e->cmp.symbol = f->symbol;
>      e->cmp.relop = r;
> +    e->as_name = c->as_name;
>      if (f->symbol->width) {
>          bitwise_copy(&c->value, sizeof c->value, 0,
>                       &e->cmp.value, sizeof e->cmp.value, f->ofs,
> @@ -547,7 +546,7 @@ make_cmp__(const struct expr_field *f, enum expr_relop
> r,
>
>  /* Returns the minimum reasonable width for integer constant 'c'. */
>  static int
> -expr_constant_width(const union expr_constant *c)
> +expr_constant_width(const struct expr_constant *c)
>  {
>      if (c->masked) {
>          return mf_subvalue_width(&c->mask);
> @@ -674,10 +673,7 @@ make_cmp(struct expr_context *ctx,
>          e = expr_combine(r == EXPR_R_EQ ? EXPR_T_OR : EXPR_T_AND,
>                           e, make_cmp__(f, r, &cs->values[i]));
>      }
> -    /* Track address set */
> -    if (r == EXPR_R_EQ && e->type == EXPR_T_OR && cs->as_name) {
> -        e->as_name = xstrdup(cs->as_name);
> -    }
> +
>  exit:
>      expr_constant_set_destroy(cs);
>      return e;
> @@ -797,11 +793,10 @@ parse_addr_sets(struct expr_context *ctx, struct
> expr_constant_set *cs,
>          }
>      }
>
> -    struct expr_constant_set *addr_sets
> -        = (ctx->addr_sets
> -           ? shash_find_data(ctx->addr_sets, ctx->lexer->token.s)
> -           : NULL);
> -    if (!addr_sets) {
> +    struct shash_node *node = ctx->addr_sets
> +                              ? shash_find(ctx->addr_sets,
> ctx->lexer->token.s)
> +                              : NULL;
> +    if (!node) {
>          lexer_syntax_error(ctx->lexer, "expecting address set name");
>          return false;
>      }
> @@ -810,17 +805,16 @@ parse_addr_sets(struct expr_context *ctx, struct
> expr_constant_set *cs,
>          return false;
>      }
>
> -    if (!cs->n_values) {
> -        cs->as_name = xstrdup(ctx->lexer->token.s);
> -    }
> -
> +    struct expr_constant_set *addr_sets = node->data;
>      size_t n_values = cs->n_values + addr_sets->n_values;
>      if (n_values >= *allocated_values) {
>          cs->values = xrealloc(cs->values, n_values * sizeof *cs->values);
>          *allocated_values = n_values;
>      }
>      for (size_t i = 0; i < addr_sets->n_values; i++) {
> -        cs->values[cs->n_values++] = addr_sets->values[i];
> +        struct expr_constant *c = &cs->values[cs->n_values++];
> +        *c = addr_sets->values[i];
> +        c->as_name = node->name;
>      }
>
>      return true;
> @@ -859,8 +853,9 @@ parse_port_group(struct expr_context *ctx, struct
> expr_constant_set *cs,
>          *allocated_values = n_values;
>      }
>      for (size_t i = 0; i < port_group->n_values; i++) {
> -        cs->values[cs->n_values++].string =
> -            xstrdup(port_group->values[i].string);
> +        struct expr_constant *c = &cs->values[cs->n_values++];
> +        c->string = xstrdup(port_group->values[i].string);
> +        c->as_name = NULL;
>      }
>
>      return true;
> @@ -875,11 +870,6 @@ parse_constant(struct expr_context *ctx, struct
> expr_constant_set *cs,
>                                  sizeof *cs->values);
>      }
>
> -    /* Combining other values to the constant set that is tracking an
> -     * address set, so untrack it. */
> -    free(cs->as_name);
> -    cs->as_name = NULL;
> -
>      if (ctx->lexer->token.type == LEX_T_TEMPLATE) {
>          lexer_error(ctx->lexer, "Unexpanded template.");
>          return false;
> @@ -887,7 +877,9 @@ parse_constant(struct expr_context *ctx, struct
> expr_constant_set *cs,
>          if (!assign_constant_set_type(ctx, cs, EXPR_C_STRING)) {
>              return false;
>          }
> -        cs->values[cs->n_values++].string = xstrdup(ctx->lexer->token.s);
> +        struct expr_constant *c = &cs->values[cs->n_values++];
> +        c->string = xstrdup(ctx->lexer->token.s);
> +        c->as_name = NULL;
>          lexer_get(ctx->lexer);
>          return true;
>      } else if (ctx->lexer->token.type == LEX_T_INTEGER ||
> @@ -896,13 +888,14 @@ parse_constant(struct expr_context *ctx, struct
> expr_constant_set *cs,
>              return false;
>          }
>
> -        union expr_constant *c = &cs->values[cs->n_values++];
> +        struct expr_constant *c = &cs->values[cs->n_values++];
>          c->value = ctx->lexer->token.value;
>          c->format = ctx->lexer->token.format;
>          c->masked = ctx->lexer->token.type == LEX_T_MASKED_INTEGER;
>          if (c->masked) {
>              c->mask = ctx->lexer->token.mask;
>          }
> +        c->as_name = NULL;
>          lexer_get(ctx->lexer);
>          return true;
>      } else if (ctx->lexer->token.type == LEX_T_MACRO) {
> @@ -961,7 +954,7 @@ parse_constant_set(struct expr_context *ctx, struct
> expr_constant_set *cs)
>   * indeterminate. */
>  bool
>  expr_constant_parse(struct lexer *lexer, const struct expr_field *f,
> -                    union expr_constant *c)
> +                    struct expr_constant *c)
>  {
>      if (lexer->error) {
>          return false;
> @@ -987,7 +980,7 @@ expr_constant_parse(struct lexer *lexer, const struct
> expr_field *f,
>  /* Appends to 's' a re-parseable representation of constant 'c' with the
> given
>   * 'type'. */
>  void
> -expr_constant_format(const union expr_constant *c,
> +expr_constant_format(const struct expr_constant *c,
>                       enum expr_constant_type type, struct ds *s)
>  {
>      if (type == EXPR_C_STRING) {
> @@ -1010,7 +1003,7 @@ expr_constant_format(const union expr_constant *c,
>   *
>   * Does not free(c). */
>  void
> -expr_constant_destroy(const union expr_constant *c,
> +expr_constant_destroy(const struct expr_constant *c,
>                        enum expr_constant_type type)
>  {
>      if (c && type == EXPR_C_STRING) {
> @@ -1043,7 +1036,7 @@ expr_constant_set_format(const struct
> expr_constant_set *cs, struct ds *s)
>          ds_put_char(s, '{');
>      }
>
> -    for (const union expr_constant *c = cs->values;
> +    for (const struct expr_constant *c = cs->values;
>           c < &cs->values[cs->n_values]; c++) {
>          if (c != cs->values) {
>              ds_put_cstr(s, ", ");
> @@ -1067,15 +1060,14 @@ expr_constant_set_destroy(struct expr_constant_set
> *cs)
>              }
>          }
>          free(cs->values);
> -        free(cs->as_name);
>      }
>  }
>
>  static int
>  compare_expr_constant_integer_cb(const void *a_, const void *b_)
>  {
> -    const union expr_constant *a = a_;
> -    const union expr_constant *b = b_;
> +    const struct expr_constant *a = a_;
> +    const struct expr_constant *b = b_;
>
>      int d = memcmp(&a->value, &b->value, sizeof a->value);
>      if (d) {
> @@ -1114,7 +1106,7 @@ expr_constant_set_create_integers(const char *const
> *values, size_t n_values)
>              VLOG_WARN("Invalid constant set entry: '%s', token type: %d",
>                        values[i], lex.token.type);
>          } else {
> -            union expr_constant *c = &cs->values[cs->n_values++];
> +            struct expr_constant *c = &cs->values[cs->n_values++];
>              c->value = lex.token.value;
>              c->format = lex.token.format;
>              c->masked = lex.token.type == LEX_T_MASKED_INTEGER;
> @@ -1135,7 +1127,7 @@ expr_constant_set_create_integers(const char *const
> *values, size_t n_values)
>
>  static void
>  expr_constant_set_add_value(struct expr_constant_set **p_cs,
> -                            union expr_constant *c, size_t *allocated)
> +                            struct expr_constant *c, size_t *allocated)
>  {
>      struct expr_constant_set *cs = *p_cs;
>      if (!cs) {
> @@ -1246,7 +1238,7 @@ expr_const_sets_add_strings(struct shash
> *const_sets, const char *name,
>                          values[i], name);
>              continue;
>          }
> -        union expr_constant *c = &cs->values[cs->n_values++];
> +        struct expr_constant *c = &cs->values[cs->n_values++];
>          c->string = xstrdup(values[i]);
>      }
>
> @@ -1359,7 +1351,7 @@ expr_parse_primary(struct expr_context *ctx, bool
> *atomic)
>
>              *atomic = true;
>
> -            union expr_constant *cst = xzalloc(sizeof *cst);
> +            struct expr_constant *cst = xzalloc(sizeof *cst);
>              cst->format = LEX_F_HEXADECIMAL;
>              cst->masked = false;
>
> @@ -1367,7 +1359,6 @@ expr_parse_primary(struct expr_context *ctx, bool
> *atomic)
>              c.values = cst;
>              c.n_values = 1;
>              c.in_curlies = false;
> -            c.as_name = NULL;
>              return make_cmp(ctx, &f, EXPR_R_NE, &c);
>          } else if (parse_relop(ctx, &r) && parse_constant_set(ctx, &c)) {
>              return make_cmp(ctx, &f, r, &c);
> @@ -1834,7 +1825,6 @@ expr_symtab_destroy(struct shash *symtab)
>  static struct expr *
>  expr_clone_cmp(struct expr *expr)
>  {
> -    ovs_assert(!expr->as_name);
>      struct expr *new = xmemdup(expr, sizeof *expr);
>      if (!new->cmp.symbol->width) {
>          new->cmp.string = xstrdup(new->cmp.string);
> @@ -1858,7 +1848,6 @@ expr_clone_andor(struct expr *expr)
>  static struct expr *
>  expr_clone_condition(struct expr *expr)
>  {
> -    ovs_assert(!expr->as_name);
>      struct expr *new = xmemdup(expr, sizeof *expr);
>      new->cond.string = xstrdup(new->cond.string);
>      return new;
> @@ -1894,8 +1883,6 @@ expr_destroy(struct expr *expr)
>          return;
>      }
>
> -    free(expr->as_name);
> -
>      struct expr *sub;
>
>      switch (expr->type) {
> @@ -2567,7 +2554,6 @@ crush_or_supersets(struct expr *expr, const struct
> expr_symbol *symbol)
>       * mask sizes. */
>      size_t n = ovs_list_size(&expr->andor);
>      struct expr **subs = xmalloc(n * sizeof *subs);
> -    bool modified = false;
>      /* Linked list over the 'subs' array to quickly skip deleted elements,
>       * i.e. the index of the next potentially non-NULL element. */
>      size_t *next = xmalloc(n * sizeof *next);
> @@ -2596,14 +2582,14 @@ crush_or_supersets(struct expr *expr, const struct
> expr_symbol *symbol)
>              next[last] = i;
>              last = i;
>          } else {
> +            /* Remove address set reference from the duplicate. */
> +            subs[last]->as_name = NULL;
>              expr_destroy(subs[i]);
>              subs[i] = NULL;
> -            modified = true;
>          }
>      }
>
> -    if (!symbol->width || symbol->level != EXPR_L_ORDINAL
> -        || (!modified && expr->as_name)) {
> +    if (!symbol->width || symbol->level != EXPR_L_ORDINAL) {
>          /* Not a fully maskable field or this expression is tracking an
>           * address set.  Don't try to optimize to preserve address set
> I-P. */
>          goto done;
> @@ -2658,10 +2644,11 @@ crush_or_supersets(struct expr *expr, const struct
> expr_symbol *symbol)
>              if (expr_bitmap_intersect_check(a_value, a_mask, b_value,
> b_mask,
>                                              bit_width)
>                  && bitmap_is_superset(b_mask, a_mask, bit_width)) {
> -                /* 'a' is the same expression with a smaller mask. */
> +                /* 'a' is the same expression with a smaller mask.
> +                 * Remove address set reference from the duplicate. */
> +                a->as_name = NULL;
>                  expr_destroy(subs[j]);
>                  subs[j] = NULL;
> -                modified = true;
>
>                  /* Shorten the path for the next round. */
>                  if (last) {
> @@ -2685,12 +2672,6 @@ done:
>          }
>      }
>
> -    if (modified) {
> -        /* Members modified, so untrack address set. */
> -        free(expr->as_name);
> -        expr->as_name = NULL;
> -    }
> -
>      free(next);
>      free(subs);
>      return expr;
> @@ -3271,10 +3252,10 @@ add_disjunction(const struct expr *or,
>      LIST_FOR_EACH (sub, node, &or->andor) {
>          struct expr_match *match = expr_match_new(m, clause, n_clauses,
>                                                    conj_id);
> -        if (or->as_name) {
> +        if (sub->as_name) {
>              ovs_assert(sub->type == EXPR_T_CMP);
>              ovs_assert(sub->cmp.symbol->width);
> -            match->as_name = xstrdup(or->as_name);
> +            match->as_name = xstrdup(sub->as_name);
>              match->as_ip = sub->cmp.value.ipv6;
>              match->as_mask = sub->cmp.mask.ipv6;
>          }
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index fdcc5aab2..626dd34f6 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -1630,7 +1630,9 @@
> priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.8
> actions=lo
>  done
>
>  reprocess_count_new=$(read_counter consider_logical_flow)
> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [10
> +# First 2 reprocess are due to change from 1 IP in AS to 2.
> +# Last 5 is due to overlap in IP addresses between as1 and as2.
> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [7
>  ])
>
>  # Remove the IPs from as1 and as2, 1 IP each time.
> @@ -1653,9 +1655,9 @@ for i in $(seq 10); do
>  done
>
>  reprocess_count_new=$(read_counter consider_logical_flow)
> -# In this case the as1 and as2 are merged to a single OR expr, so we lose
> track of
> -# address set information - can't handle deletion incrementally.
> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [10
> +# First 5 are due to overlap in IP addresses between as1 and as2.
> +# Last 2 are due to change from 2 IP in AS to 1.
> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [7
>  ])
>
>  OVN_CLEANUP([hv1])
> @@ -1822,7 +1824,7 @@
> priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.5
> actions=co
>  priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.10.10.10
> actions=conjunction,2/2)
>  ])
>  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
>  ])
>
>  # Delete 2 IPs
> @@ -1842,7 +1844,7 @@
> priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3
> actions=co
>  priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.10.10.10
> actions=conjunction,2/2)
>  ])
>  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])
> --
> 2.44.0
>
>
Please consider it as RFC, I forgot to add the tag into subject.

On 3/19/24 17:43, Ales Musil wrote:
> Instead of tracking address set per struct expr_constant_set track it
> per individual struct expr_constant. This allows more fine grained
> control for I-P processing of address sets in controller. It helps with
> scenarios like matching on two address sets in one expression e.g.
> "ip4.src == {$as1, $as2}". This allows any addition or removal of
> individual adress from the set to be incrementally processed instead
> of reprocessing all the flows.
> 
> This unfortunately doesn't help with the following flows:
> "ip4.src == $as1 && ip4.dst == $as2"
> "ip4.src == $as1 || ip4.dst == $as2"
> 
> The memory impact should be minimal as there is only increase of 8 bytes
> per the struct expr_constant.
> 
> Signed-off-by: Ales Musil <amusil@redhat.com>
> ---

Hi, Ales.  I didn't fully read the patch, but I have a quick question:
Did you test it with the case where address sets contain the same
addresses? i.e. if we have "ip4.src == {$as1, $as2}" and both contain
the same IP 'A'.  Will removal of the 'A' from one of the sets be handled
correctly?

Database protects us from such a case with a single address set, but it
can't with multiple.

Also, we'll need a test for this scenario in case we don't have one already.

Best regards, Ilya Maximets.

On Tue, Mar 19, 2024 at 6:03 PM Ilya Maximets <i.maximets@ovn.org> wrote:

> On 3/19/24 17:43, Ales Musil wrote:
> > Instead of tracking address set per struct expr_constant_set track it
> > per individual struct expr_constant. This allows more fine grained
> > control for I-P processing of address sets in controller. It helps with
> > scenarios like matching on two address sets in one expression e.g.
> > "ip4.src == {$as1, $as2}". This allows any addition or removal of
> > individual adress from the set to be incrementally processed instead
> > of reprocessing all the flows.
> >
> > This unfortunately doesn't help with the following flows:
> > "ip4.src == $as1 && ip4.dst == $as2"
> > "ip4.src == $as1 || ip4.dst == $as2"
> >
> > The memory impact should be minimal as there is only increase of 8 bytes
> > per the struct expr_constant.
> >
> > Signed-off-by: Ales Musil <amusil@redhat.com>
> > ---
>
> Hi, Ales.  I didn't fully read the patch, but I have a quick question:
> Did you test it with the case where address sets contain the same
> addresses? i.e. if we have "ip4.src == {$as1, $as2}" and both contain
> the same IP 'A'.  Will removal of the 'A' from one of the sets be handled
> correctly?
>
> Database protects us from such a case with a single address set, but it
> can't with multiple.
>
> Also, we'll need a test for this scenario in case we don't have one
> already.
>
> Best regards, Ilya Maximets.
>
>
 Hi Ilya,

There are actually several tests that have duplicates across two address
sets like you described. The I-P in that case will lead to reprocessing of
the flows, the AS reference is removed when there are duplicates.

Thanks,
Ales

On Tue, Mar 19, 2024 at 9:45 AM Ales Musil <amusil@redhat.com> wrote:
>
>
>
> On Tue, Mar 19, 2024 at 5:43 PM Ales Musil <amusil@redhat.com> wrote:
>>
>> Instead of tracking address set per struct expr_constant_set track it
>> per individual struct expr_constant. This allows more fine grained
>> control for I-P processing of address sets in controller. It helps with
>> scenarios like matching on two address sets in one expression e.g.
>> "ip4.src == {$as1, $as2}". This allows any addition or removal of
>> individual adress from the set to be incrementally processed instead
>> of reprocessing all the flows.
>>
>> This unfortunately doesn't help with the following flows:
>> "ip4.src == $as1 && ip4.dst == $as2"
>> "ip4.src == $as1 || ip4.dst == $as2"
>>
>> The memory impact should be minimal as there is only increase of 8 bytes
>> per the struct expr_constant.
>>
>> Signed-off-by: Ales Musil <amusil@redhat.com>

Thanks Ales for the improvement! The approach looks good to me. Please see
some comments below:

>> ---
>>  controller/lflow.c      |  4 +-
>>  include/ovn/actions.h   |  2 +-
>>  include/ovn/expr.h      | 46 ++++++++++----------
>>  lib/actions.c           | 20 ++++-----
>>  lib/expr.c              | 95 +++++++++++++++++------------------------
>>  tests/ovn-controller.at | 14 +++---
>>  6 files changed, 83 insertions(+), 98 deletions(-)
>>
>> diff --git a/controller/lflow.c b/controller/lflow.c
>> index 895d17d19..730dc879d 100644
>> --- a/controller/lflow.c
>> +++ b/controller/lflow.c
>> @@ -278,7 +278,7 @@ lflow_handle_changed_flows(struct lflow_ctx_in
*l_ctx_in,
>>  }
>>
>>  static bool
>> -as_info_from_expr_const(const char *as_name, const union expr_constant
*c,
>> +as_info_from_expr_const(const char *as_name, const struct expr_constant
*c,
>>                          struct addrset_info *as_info)
>>  {
>>      as_info->name = as_name;
>> @@ -714,7 +714,7 @@ lflow_handle_addr_set_update(const char *as_name,
>>          if (as_diff->deleted) {
>>              struct addrset_info as_info;
>>              for (size_t i = 0; i < as_diff->deleted->n_values; i++) {
>> -                union expr_constant *c = &as_diff->deleted->values[i];
>> +                struct expr_constant *c = &as_diff->deleted->values[i];
>>                  if (!as_info_from_expr_const(as_name, c, &as_info)) {
>>                      continue;
>>                  }
>> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
>> index dcacbb1ff..1e20f9b81 100644
>> --- a/include/ovn/actions.h
>> +++ b/include/ovn/actions.h
>> @@ -238,7 +238,7 @@ struct ovnact_next {
>>  struct ovnact_load {
>>      struct ovnact ovnact;
>>      struct expr_field dst;
>> -    union expr_constant imm;
>> +    struct expr_constant imm;
>>  };
>>
>>  /* OVNACT_MOVE, OVNACT_EXCHANGE. */
>> diff --git a/include/ovn/expr.h b/include/ovn/expr.h
>> index c48f82398..e54edb5bf 100644
>> --- a/include/ovn/expr.h
>> +++ b/include/ovn/expr.h
>> @@ -368,7 +368,7 @@ bool expr_relop_from_token(enum lex_type type, enum
expr_relop *relop);
>>  struct expr {
>>      struct ovs_list node;       /* In parent EXPR_T_AND or EXPR_T_OR if
any. */
>>      enum expr_type type;        /* Expression type. */
>> -    char *as_name;              /* Address set name. Null if it is not
an
>> +    const char *as_name;        /* Address set name. Null if it is not
an
>>                                     address set. */
>>
>>      union {
>> @@ -505,40 +505,42 @@ enum expr_constant_type {
>>  };
>>
>>  /* A string or integer constant (one must know which from context). */
>> -union expr_constant {
>> -    /* Integer constant.
>> -     *
>> -     * The width of a constant isn't always clear, e.g. if you write
"1",
>> -     * there's no way to tell whether you mean for that to be a 1-bit
constant
>> -     * or a 128-bit constant or somewhere in between. */
>> -    struct {
>> -        union mf_subvalue value;
>> -        union mf_subvalue mask; /* Only initialized if 'masked'. */
>> -        bool masked;
>> -
>> -        enum lex_format format; /* From the constant's lex_token. */
>> -    };
>> +struct expr_constant {
>> +    const char *as_name;
>>
>> -    /* Null-terminated string constant. */
>> -    char *string;
>> +    union {
>> +        /* Integer constant.
>> +         *
>> +         * The width of a constant isn't always clear, e.g. if you
write "1",
>> +         * there's no way to tell whether you mean for that to be a
1-bit
>> +         * constant or a 128-bit constant or somewhere in between. */
>> +        struct {
>> +            union mf_subvalue value;
>> +            union mf_subvalue mask; /* Only initialized if 'masked'. */
>> +            bool masked;
>> +
>> +            enum lex_format format; /* From the constant's lex_token. */
>> +        };
>> +
>> +        /* Null-terminated string constant. */
>> +        char *string;
>> +    };
>>  };
>>
>>  bool expr_constant_parse(struct lexer *,
>>                           const struct expr_field *,
>> -                         union expr_constant *);
>> -void expr_constant_format(const union expr_constant *,
>> +                         struct expr_constant *);
>> +void expr_constant_format(const struct expr_constant *,
>>                            enum expr_constant_type, struct ds *);
>> -void expr_constant_destroy(const union expr_constant *,
>> +void expr_constant_destroy(const struct expr_constant *,
>>                             enum expr_constant_type);
>>
>>  /* A collection of "union expr_constant"s of the same type. */
>>  struct expr_constant_set {
>> -    union expr_constant *values;  /* Constants. */
>> +    struct expr_constant *values; /* Constants. */
>>      size_t n_values;              /* Number of constants. */
>>      enum expr_constant_type type; /* Type of the constants. */
>>      bool in_curlies;              /* Whether the constants were in {}.
*/
>> -    char *as_name;                /* Name of an address set. It is NULL
if not
>> -                                     an address set. */
>>  };
>>
>>  bool expr_constant_set_parse(struct lexer *, struct expr_constant_set
*);
>> diff --git a/lib/actions.c b/lib/actions.c
>> index 71615fc53..6574c4df4 100644
>> --- a/lib/actions.c
>> +++ b/lib/actions.c
>> @@ -450,7 +450,7 @@ encode_LOAD(const struct ovnact_load *load,
>>              const struct ovnact_encode_params *ep,
>>              struct ofpbuf *ofpacts)
>>  {
>> -    const union expr_constant *c = &load->imm;
>> +    const struct expr_constant *c = &load->imm;
>>      struct mf_subfield dst = expr_resolve_field(&load->dst);
>>      struct ofpact_set_field *sf = ofpact_put_set_field(ofpacts,
dst.field,
>>                                                         NULL, NULL);
>> @@ -2122,7 +2122,7 @@ encode_event_empty_lb_backends_opts(struct ofpbuf
*ofpacts,
>>          /* All empty_lb_backends fields are of type 'str' */
>>          ovs_assert(!strcmp(o->option->type, "str"));
>>
>> -        const union expr_constant *c = o->value.values;
>> +        const struct expr_constant *c = o->value.values;
>>          size_t size = strlen(c->string);
>>          struct controller_event_opt_header hdr =
>>              (struct controller_event_opt_header) {
>> @@ -2598,7 +2598,7 @@ validate_empty_lb_backends(struct action_context
*ctx,
>>  {
>>      for (size_t i = 0; i < n_options; i++) {
>>          const struct ovnact_gen_option *o = &options[i];
>> -        const union expr_constant *c = o->value.values;
>> +        const struct expr_constant *c = o->value.values;
>>          struct sockaddr_storage ss;
>>          struct uuid uuid;
>>
>> @@ -2801,7 +2801,7 @@ encode_put_dhcpv4_option(const struct
ovnact_gen_option *o,
>>      uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2);
>>      opt_header[0] = o->option->code;
>>
>> -    const union expr_constant *c = o->value.values;
>> +    const struct expr_constant *c = o->value.values;
>>      size_t n_values = o->value.n_values;
>>      if (!strcmp(o->option->type, "bool") ||
>>          !strcmp(o->option->type, "uint8")) {
>> @@ -2967,7 +2967,7 @@ encode_put_dhcpv6_option(const struct
ovnact_gen_option *o,
>>                           struct ofpbuf *ofpacts)
>>  {
>>      struct dhcpv6_opt_header *opt = ofpbuf_put_uninit(ofpacts, sizeof
*opt);
>> -    const union expr_constant *c = o->value.values;
>> +    const struct expr_constant *c = o->value.values;
>>      size_t n_values = o->value.n_values;
>>      size_t size;
>>
>> @@ -3023,7 +3023,7 @@ encode_PUT_DHCPV4_OPTS(const struct
ovnact_put_opts *pdo,
>>          find_opt(pdo->options, pdo->n_options, DHCP_OPT_BOOTFILE_CODE);
>>      if (boot_opt) {
>>          uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2);
>> -        const union expr_constant *c = boot_opt->value.values;
>> +        const struct expr_constant *c = boot_opt->value.values;
>>          opt_header[0] = boot_opt->option->code;
>>          opt_header[1] = strlen(c->string);
>>          ofpbuf_put(ofpacts, c->string, opt_header[1]);
>> @@ -3033,7 +3033,7 @@ encode_PUT_DHCPV4_OPTS(const struct
ovnact_put_opts *pdo,
>>          find_opt(pdo->options, pdo->n_options,
DHCP_OPT_BOOTFILE_ALT_CODE);
>>      if (boot_alt_opt) {
>>          uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2);
>> -        const union expr_constant *c = boot_alt_opt->value.values;
>> +        const struct expr_constant *c = boot_alt_opt->value.values;
>>          opt_header[0] = boot_alt_opt->option->code;
>>          opt_header[1] = strlen(c->string);
>>          ofpbuf_put(ofpacts, c->string, opt_header[1]);
>> @@ -3043,7 +3043,7 @@ encode_PUT_DHCPV4_OPTS(const struct
ovnact_put_opts *pdo,
>>          pdo->options, pdo->n_options, DHCP_OPT_NEXT_SERVER_CODE);
>>      if (next_server_opt) {
>>          uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2);
>> -        const union expr_constant *c = next_server_opt->value.values;
>> +        const struct expr_constant *c = next_server_opt->value.values;
>>          opt_header[0] = next_server_opt->option->code;
>>          opt_header[1] = sizeof(ovs_be32);
>>          ofpbuf_put(ofpacts, &c->value.ipv4, sizeof(ovs_be32));
>> @@ -3247,7 +3247,7 @@ parse_put_nd_ra_opts(struct action_context *ctx,
const struct expr_field *dst,
>>      /* Let's validate the options. */
>>      for (size_t i = 0; i < po->n_options; i++) {
>>          const struct ovnact_gen_option *o = &po->options[i];
>> -        const union expr_constant *c = o->value.values;
>> +        const struct expr_constant *c = o->value.values;
>>          if (o->value.n_values > 1) {
>>              lexer_error(ctx->lexer, "Invalid value for \"%s\" option",
>>                          o->option->name);
>> @@ -3430,7 +3430,7 @@ static void
>>  encode_put_nd_ra_option(const struct ovnact_gen_option *o,
>>                          struct ofpbuf *ofpacts, ptrdiff_t ra_offset)
>>  {
>> -    const union expr_constant *c = o->value.values;
>> +    const struct expr_constant *c = o->value.values;
>>
>>      switch (o->option->code) {
>>      case ND_RA_FLAG_ADDR_MODE:
>> diff --git a/lib/expr.c b/lib/expr.c
>> index 0cb44e1b6..286782025 100644
>> --- a/lib/expr.c
>> +++ b/lib/expr.c
>> @@ -179,12 +179,10 @@ expr_combine(enum expr_type type, struct expr *a,
struct expr *b)
>>          } else {
>>              ovs_list_push_back(&a->andor, &b->node);
>>          }
>> -        free(a->as_name);
>>          a->as_name = NULL;
>>          return a;
>>      } else if (b->type == type) {
>>          ovs_list_push_front(&b->andor, &a->node);
>> -        free(b->as_name);
>>          b->as_name = NULL;
>>          return b;
>>      } else {
>> @@ -521,12 +519,13 @@ static bool parse_field(struct expr_context *,
struct expr_field *);
>>
>>  static struct expr *
>>  make_cmp__(const struct expr_field *f, enum expr_relop r,
>> -             const union expr_constant *c)
>> +             const struct expr_constant *c)
>>  {
>>      struct expr *e = xzalloc(sizeof *e);
>>      e->type = EXPR_T_CMP;
>>      e->cmp.symbol = f->symbol;
>>      e->cmp.relop = r;
>> +    e->as_name = c->as_name;
>>      if (f->symbol->width) {
>>          bitwise_copy(&c->value, sizeof c->value, 0,
>>                       &e->cmp.value, sizeof e->cmp.value, f->ofs,
>> @@ -547,7 +546,7 @@ make_cmp__(const struct expr_field *f, enum
expr_relop r,
>>
>>  /* Returns the minimum reasonable width for integer constant 'c'. */
>>  static int
>> -expr_constant_width(const union expr_constant *c)
>> +expr_constant_width(const struct expr_constant *c)
>>  {
>>      if (c->masked) {
>>          return mf_subvalue_width(&c->mask);
>> @@ -674,10 +673,7 @@ make_cmp(struct expr_context *ctx,
>>          e = expr_combine(r == EXPR_R_EQ ? EXPR_T_OR : EXPR_T_AND,
>>                           e, make_cmp__(f, r, &cs->values[i]));
>>      }
>> -    /* Track address set */
>> -    if (r == EXPR_R_EQ && e->type == EXPR_T_OR && cs->as_name) {
>> -        e->as_name = xstrdup(cs->as_name);
>> -    }
>> +
>>  exit:
>>      expr_constant_set_destroy(cs);
>>      return e;
>> @@ -797,11 +793,10 @@ parse_addr_sets(struct expr_context *ctx, struct
expr_constant_set *cs,
>>          }
>>      }
>>
>> -    struct expr_constant_set *addr_sets
>> -        = (ctx->addr_sets
>> -           ? shash_find_data(ctx->addr_sets, ctx->lexer->token.s)
>> -           : NULL);
>> -    if (!addr_sets) {
>> +    struct shash_node *node = ctx->addr_sets
>> +                              ? shash_find(ctx->addr_sets,
ctx->lexer->token.s)
>> +                              : NULL;
>> +    if (!node) {
>>          lexer_syntax_error(ctx->lexer, "expecting address set name");
>>          return false;
>>      }
>> @@ -810,17 +805,16 @@ parse_addr_sets(struct expr_context *ctx, struct
expr_constant_set *cs,
>>          return false;
>>      }
>>
>> -    if (!cs->n_values) {
>> -        cs->as_name = xstrdup(ctx->lexer->token.s);
>> -    }
>> -
>> +    struct expr_constant_set *addr_sets = node->data;
>>      size_t n_values = cs->n_values + addr_sets->n_values;
>>      if (n_values >= *allocated_values) {
>>          cs->values = xrealloc(cs->values, n_values * sizeof
*cs->values);
>>          *allocated_values = n_values;
>>      }
>>      for (size_t i = 0; i < addr_sets->n_values; i++) {
>> -        cs->values[cs->n_values++] = addr_sets->values[i];
>> +        struct expr_constant *c = &cs->values[cs->n_values++];
>> +        *c = addr_sets->values[i];
>> +        c->as_name = node->name;

I am not sure if it is safe enough to just store the reference. We need to
check carefully if there is any chance the pointer becomes dangling when
the corresponding address set is destroyed or in any other circumstances.

>>      }
>>
>>      return true;
>> @@ -859,8 +853,9 @@ parse_port_group(struct expr_context *ctx, struct
expr_constant_set *cs,
>>          *allocated_values = n_values;
>>      }
>>      for (size_t i = 0; i < port_group->n_values; i++) {
>> -        cs->values[cs->n_values++].string =
>> -            xstrdup(port_group->values[i].string);
>> +        struct expr_constant *c = &cs->values[cs->n_values++];
>> +        c->string = xstrdup(port_group->values[i].string);
>> +        c->as_name = NULL;
>>      }
>>
>>      return true;
>> @@ -875,11 +870,6 @@ parse_constant(struct expr_context *ctx, struct
expr_constant_set *cs,
>>                                  sizeof *cs->values);
>>      }
>>
>> -    /* Combining other values to the constant set that is tracking an
>> -     * address set, so untrack it. */
>> -    free(cs->as_name);
>> -    cs->as_name = NULL;
>> -
>>      if (ctx->lexer->token.type == LEX_T_TEMPLATE) {
>>          lexer_error(ctx->lexer, "Unexpanded template.");
>>          return false;
>> @@ -887,7 +877,9 @@ parse_constant(struct expr_context *ctx, struct
expr_constant_set *cs,
>>          if (!assign_constant_set_type(ctx, cs, EXPR_C_STRING)) {
>>              return false;
>>          }
>> -        cs->values[cs->n_values++].string =
xstrdup(ctx->lexer->token.s);
>> +        struct expr_constant *c = &cs->values[cs->n_values++];
>> +        c->string = xstrdup(ctx->lexer->token.s);
>> +        c->as_name = NULL;
>>          lexer_get(ctx->lexer);
>>          return true;
>>      } else if (ctx->lexer->token.type == LEX_T_INTEGER ||
>> @@ -896,13 +888,14 @@ parse_constant(struct expr_context *ctx, struct
expr_constant_set *cs,
>>              return false;
>>          }
>>
>> -        union expr_constant *c = &cs->values[cs->n_values++];
>> +        struct expr_constant *c = &cs->values[cs->n_values++];
>>          c->value = ctx->lexer->token.value;
>>          c->format = ctx->lexer->token.format;
>>          c->masked = ctx->lexer->token.type == LEX_T_MASKED_INTEGER;
>>          if (c->masked) {
>>              c->mask = ctx->lexer->token.mask;
>>          }
>> +        c->as_name = NULL;
>>          lexer_get(ctx->lexer);
>>          return true;
>>      } else if (ctx->lexer->token.type == LEX_T_MACRO) {
>> @@ -961,7 +954,7 @@ parse_constant_set(struct expr_context *ctx, struct
expr_constant_set *cs)
>>   * indeterminate. */
>>  bool
>>  expr_constant_parse(struct lexer *lexer, const struct expr_field *f,
>> -                    union expr_constant *c)
>> +                    struct expr_constant *c)
>>  {
>>      if (lexer->error) {
>>          return false;
>> @@ -987,7 +980,7 @@ expr_constant_parse(struct lexer *lexer, const
struct expr_field *f,
>>  /* Appends to 's' a re-parseable representation of constant 'c' with
the given
>>   * 'type'. */
>>  void
>> -expr_constant_format(const union expr_constant *c,
>> +expr_constant_format(const struct expr_constant *c,
>>                       enum expr_constant_type type, struct ds *s)
>>  {
>>      if (type == EXPR_C_STRING) {
>> @@ -1010,7 +1003,7 @@ expr_constant_format(const union expr_constant *c,
>>   *
>>   * Does not free(c). */
>>  void
>> -expr_constant_destroy(const union expr_constant *c,
>> +expr_constant_destroy(const struct expr_constant *c,
>>                        enum expr_constant_type type)
>>  {
>>      if (c && type == EXPR_C_STRING) {
>> @@ -1043,7 +1036,7 @@ expr_constant_set_format(const struct
expr_constant_set *cs, struct ds *s)
>>          ds_put_char(s, '{');
>>      }
>>
>> -    for (const union expr_constant *c = cs->values;
>> +    for (const struct expr_constant *c = cs->values;
>>           c < &cs->values[cs->n_values]; c++) {
>>          if (c != cs->values) {
>>              ds_put_cstr(s, ", ");
>> @@ -1067,15 +1060,14 @@ expr_constant_set_destroy(struct
expr_constant_set *cs)
>>              }
>>          }
>>          free(cs->values);
>> -        free(cs->as_name);
>>      }
>>  }
>>
>>  static int
>>  compare_expr_constant_integer_cb(const void *a_, const void *b_)
>>  {
>> -    const union expr_constant *a = a_;
>> -    const union expr_constant *b = b_;
>> +    const struct expr_constant *a = a_;
>> +    const struct expr_constant *b = b_;
>>
>>      int d = memcmp(&a->value, &b->value, sizeof a->value);
>>      if (d) {
>> @@ -1114,7 +1106,7 @@ expr_constant_set_create_integers(const char
*const *values, size_t n_values)
>>              VLOG_WARN("Invalid constant set entry: '%s', token type:
%d",
>>                        values[i], lex.token.type);
>>          } else {
>> -            union expr_constant *c = &cs->values[cs->n_values++];
>> +            struct expr_constant *c = &cs->values[cs->n_values++];
>>              c->value = lex.token.value;
>>              c->format = lex.token.format;
>>              c->masked = lex.token.type == LEX_T_MASKED_INTEGER;
>> @@ -1135,7 +1127,7 @@ expr_constant_set_create_integers(const char
*const *values, size_t n_values)
>>
>>  static void
>>  expr_constant_set_add_value(struct expr_constant_set **p_cs,
>> -                            union expr_constant *c, size_t *allocated)
>> +                            struct expr_constant *c, size_t *allocated)
>>  {
>>      struct expr_constant_set *cs = *p_cs;
>>      if (!cs) {
>> @@ -1246,7 +1238,7 @@ expr_const_sets_add_strings(struct shash
*const_sets, const char *name,
>>                          values[i], name);
>>              continue;
>>          }
>> -        union expr_constant *c = &cs->values[cs->n_values++];
>> +        struct expr_constant *c = &cs->values[cs->n_values++];
>>          c->string = xstrdup(values[i]);
>>      }
>>
>> @@ -1359,7 +1351,7 @@ expr_parse_primary(struct expr_context *ctx, bool
*atomic)
>>
>>              *atomic = true;
>>
>> -            union expr_constant *cst = xzalloc(sizeof *cst);
>> +            struct expr_constant *cst = xzalloc(sizeof *cst);
>>              cst->format = LEX_F_HEXADECIMAL;
>>              cst->masked = false;
>>
>> @@ -1367,7 +1359,6 @@ expr_parse_primary(struct expr_context *ctx, bool
*atomic)
>>              c.values = cst;
>>              c.n_values = 1;
>>              c.in_curlies = false;
>> -            c.as_name = NULL;
>>              return make_cmp(ctx, &f, EXPR_R_NE, &c);
>>          } else if (parse_relop(ctx, &r) && parse_constant_set(ctx, &c))
{
>>              return make_cmp(ctx, &f, r, &c);
>> @@ -1834,7 +1825,6 @@ expr_symtab_destroy(struct shash *symtab)
>>  static struct expr *
>>  expr_clone_cmp(struct expr *expr)
>>  {
>> -    ovs_assert(!expr->as_name);
>>      struct expr *new = xmemdup(expr, sizeof *expr);
>>      if (!new->cmp.symbol->width) {
>>          new->cmp.string = xstrdup(new->cmp.string);
>> @@ -1858,7 +1848,6 @@ expr_clone_andor(struct expr *expr)
>>  static struct expr *
>>  expr_clone_condition(struct expr *expr)
>>  {
>> -    ovs_assert(!expr->as_name);
>>      struct expr *new = xmemdup(expr, sizeof *expr);
>>      new->cond.string = xstrdup(new->cond.string);
>>      return new;
>> @@ -1894,8 +1883,6 @@ expr_destroy(struct expr *expr)
>>          return;
>>      }
>>
>> -    free(expr->as_name);
>> -
>>      struct expr *sub;
>>
>>      switch (expr->type) {
>> @@ -2567,7 +2554,6 @@ crush_or_supersets(struct expr *expr, const struct
expr_symbol *symbol)
>>       * mask sizes. */
>>      size_t n = ovs_list_size(&expr->andor);
>>      struct expr **subs = xmalloc(n * sizeof *subs);
>> -    bool modified = false;
>>      /* Linked list over the 'subs' array to quickly skip deleted
elements,
>>       * i.e. the index of the next potentially non-NULL element. */
>>      size_t *next = xmalloc(n * sizeof *next);
>> @@ -2596,14 +2582,14 @@ crush_or_supersets(struct expr *expr, const
struct expr_symbol *symbol)
>>              next[last] = i;
>>              last = i;
>>          } else {
>> +            /* Remove address set reference from the duplicate. */
>> +            subs[last]->as_name = NULL;
>>              expr_destroy(subs[i]);
>>              subs[i] = NULL;
>> -            modified = true;
>>          }
>>      }
>>
>> -    if (!symbol->width || symbol->level != EXPR_L_ORDINAL
>> -        || (!modified && expr->as_name)) {
>> +    if (!symbol->width || symbol->level != EXPR_L_ORDINAL) {
>>          /* Not a fully maskable field or this expression is tracking an
>>           * address set.  Don't try to optimize to preserve address set
I-P. */

The above comment needs to be updated since it obviously doesn't preserve
address set I-P any more. What's the reason it is not considered any more?

>>          goto done;
>> @@ -2658,10 +2644,11 @@ crush_or_supersets(struct expr *expr, const
struct expr_symbol *symbol)
>>              if (expr_bitmap_intersect_check(a_value, a_mask, b_value,
b_mask,
>>                                              bit_width)
>>                  && bitmap_is_superset(b_mask, a_mask, bit_width)) {
>> -                /* 'a' is the same expression with a smaller mask. */
>> +                /* 'a' is the same expression with a smaller mask.
>> +                 * Remove address set reference from the duplicate. */
>> +                a->as_name = NULL;
>>                  expr_destroy(subs[j]);
>>                  subs[j] = NULL;
>> -                modified = true;
>>
>>                  /* Shorten the path for the next round. */
>>                  if (last) {
>> @@ -2685,12 +2672,6 @@ done:
>>          }
>>      }
>>
>> -    if (modified) {
>> -        /* Members modified, so untrack address set. */
>> -        free(expr->as_name);
>> -        expr->as_name = NULL;
>> -    }
>> -
>>      free(next);
>>      free(subs);
>>      return expr;
>> @@ -3271,10 +3252,10 @@ add_disjunction(const struct expr *or,
>>      LIST_FOR_EACH (sub, node, &or->andor) {
>>          struct expr_match *match = expr_match_new(m, clause, n_clauses,
>>                                                    conj_id);
>> -        if (or->as_name) {
>> +        if (sub->as_name) {
>>              ovs_assert(sub->type == EXPR_T_CMP);
>>              ovs_assert(sub->cmp.symbol->width);
>> -            match->as_name = xstrdup(or->as_name);
>> +            match->as_name = xstrdup(sub->as_name);
>>              match->as_ip = sub->cmp.value.ipv6;
>>              match->as_mask = sub->cmp.mask.ipv6;
>>          }
>> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
>> index fdcc5aab2..626dd34f6 100644
>> --- a/tests/ovn-controller.at
>> +++ b/tests/ovn-controller.at
>> @@ -1630,7 +1630,9 @@
priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.8
actions=lo
>>  done
>>
>>  reprocess_count_new=$(read_counter consider_logical_flow)
>> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[10
>> +# First 2 reprocess are due to change from 1 IP in AS to 2.
>> +# Last 5 is due to overlap in IP addresses between as1 and as2.
>> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[7
>>  ])
>>
>>  # Remove the IPs from as1 and as2, 1 IP each time.
>> @@ -1653,9 +1655,9 @@ for i in $(seq 10); do
>>  done
>>
>>  reprocess_count_new=$(read_counter consider_logical_flow)
>> -# In this case the as1 and as2 are merged to a single OR expr, so we
lose track of
>> -# address set information - can't handle deletion incrementally.
>> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[10
>> +# First 5 are due to overlap in IP addresses between as1 and as2.
>> +# Last 2 are due to change from 2 IP in AS to 1.
>> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
[7
>>  ])
>>
>>  OVN_CLEANUP([hv1])
>> @@ -1822,7 +1824,7 @@
priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.5
actions=co
>>
 priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.10.10.10
actions=conjunction,2/2)
>>  ])
>>  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
>>  ])
>>
>>  # Delete 2 IPs
>> @@ -1842,7 +1844,7 @@
priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3
actions=co
>>
 priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.10.10.10
actions=conjunction,2/2)
>>  ])
>>  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])
>> --
>> 2.44.0
>>
>
> Please consider it as RFC, I forgot to add the tag into subject.

Is there a reason why this is RFC? Do you have any TODOs in mind?
I am just not 100% confident about corner cases. The existing test cases
seem to be handled correctly. We might need to think about if there are any
other potentially risky scenarios that need to be tested, although I don't
see any obvious problem for now.

Thanks,
Han

> --
>
> Ales Musil
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA
>
> amusil@redhat.com

On Wed, Mar 27, 2024 at 7:14 AM Han Zhou <hzhou@ovn.org> wrote:

>
>
> On Tue, Mar 19, 2024 at 9:45 AM Ales Musil <amusil@redhat.com> wrote:
> >
> >
> >
> > On Tue, Mar 19, 2024 at 5:43 PM Ales Musil <amusil@redhat.com> wrote:
> >>
> >> Instead of tracking address set per struct expr_constant_set track it
> >> per individual struct expr_constant. This allows more fine grained
> >> control for I-P processing of address sets in controller. It helps with
> >> scenarios like matching on two address sets in one expression e.g.
> >> "ip4.src == {$as1, $as2}". This allows any addition or removal of
> >> individual adress from the set to be incrementally processed instead
> >> of reprocessing all the flows.
> >>
> >> This unfortunately doesn't help with the following flows:
> >> "ip4.src == $as1 && ip4.dst == $as2"
> >> "ip4.src == $as1 || ip4.dst == $as2"
> >>
> >> The memory impact should be minimal as there is only increase of 8 bytes
> >> per the struct expr_constant.
> >>
> >> Signed-off-by: Ales Musil <amusil@redhat.com>
>
> Thanks Ales for the improvement! The approach looks good to me. Please see
> some comments below:
>
>
>
Hi Han,

thank you for the review.


>
> >> ---
> >>  controller/lflow.c      |  4 +-
> >>  include/ovn/actions.h   |  2 +-
> >>  include/ovn/expr.h      | 46 ++++++++++----------
> >>  lib/actions.c           | 20 ++++-----
> >>  lib/expr.c              | 95 +++++++++++++++++------------------------
> >>  tests/ovn-controller.at | 14 +++---
> >>  6 files changed, 83 insertions(+), 98 deletions(-)
> >>
> >> diff --git a/controller/lflow.c b/controller/lflow.c
> >> index 895d17d19..730dc879d 100644
> >> --- a/controller/lflow.c
> >> +++ b/controller/lflow.c
> >> @@ -278,7 +278,7 @@ lflow_handle_changed_flows(struct lflow_ctx_in
> *l_ctx_in,
> >>  }
> >>
> >>  static bool
> >> -as_info_from_expr_const(const char *as_name, const union expr_constant
> *c,
> >> +as_info_from_expr_const(const char *as_name, const struct
> expr_constant *c,
> >>                          struct addrset_info *as_info)
> >>  {
> >>      as_info->name = as_name;
> >> @@ -714,7 +714,7 @@ lflow_handle_addr_set_update(const char *as_name,
> >>          if (as_diff->deleted) {
> >>              struct addrset_info as_info;
> >>              for (size_t i = 0; i < as_diff->deleted->n_values; i++) {
> >> -                union expr_constant *c = &as_diff->deleted->values[i];
> >> +                struct expr_constant *c = &as_diff->deleted->values[i];
> >>                  if (!as_info_from_expr_const(as_name, c, &as_info)) {
> >>                      continue;
> >>                  }
> >> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> >> index dcacbb1ff..1e20f9b81 100644
> >> --- a/include/ovn/actions.h
> >> +++ b/include/ovn/actions.h
> >> @@ -238,7 +238,7 @@ struct ovnact_next {
> >>  struct ovnact_load {
> >>      struct ovnact ovnact;
> >>      struct expr_field dst;
> >> -    union expr_constant imm;
> >> +    struct expr_constant imm;
> >>  };
> >>
> >>  /* OVNACT_MOVE, OVNACT_EXCHANGE. */
> >> diff --git a/include/ovn/expr.h b/include/ovn/expr.h
> >> index c48f82398..e54edb5bf 100644
> >> --- a/include/ovn/expr.h
> >> +++ b/include/ovn/expr.h
> >> @@ -368,7 +368,7 @@ bool expr_relop_from_token(enum lex_type type, enum
> expr_relop *relop);
> >>  struct expr {
> >>      struct ovs_list node;       /* In parent EXPR_T_AND or EXPR_T_OR
> if any. */
> >>      enum expr_type type;        /* Expression type. */
> >> -    char *as_name;              /* Address set name. Null if it is not
> an
> >> +    const char *as_name;        /* Address set name. Null if it is not
> an
> >>                                     address set. */
> >>
> >>      union {
> >> @@ -505,40 +505,42 @@ enum expr_constant_type {
> >>  };
> >>
> >>  /* A string or integer constant (one must know which from context). */
> >> -union expr_constant {
> >> -    /* Integer constant.
> >> -     *
> >> -     * The width of a constant isn't always clear, e.g. if you write
> "1",
> >> -     * there's no way to tell whether you mean for that to be a 1-bit
> constant
> >> -     * or a 128-bit constant or somewhere in between. */
> >> -    struct {
> >> -        union mf_subvalue value;
> >> -        union mf_subvalue mask; /* Only initialized if 'masked'. */
> >> -        bool masked;
> >> -
> >> -        enum lex_format format; /* From the constant's lex_token. */
> >> -    };
> >> +struct expr_constant {
> >> +    const char *as_name;
> >>
> >> -    /* Null-terminated string constant. */
> >> -    char *string;
> >> +    union {
> >> +        /* Integer constant.
> >> +         *
> >> +         * The width of a constant isn't always clear, e.g. if you
> write "1",
> >> +         * there's no way to tell whether you mean for that to be a
> 1-bit
> >> +         * constant or a 128-bit constant or somewhere in between. */
> >> +        struct {
> >> +            union mf_subvalue value;
> >> +            union mf_subvalue mask; /* Only initialized if 'masked'. */
> >> +            bool masked;
> >> +
> >> +            enum lex_format format; /* From the constant's lex_token.
> */
> >> +        };
> >> +
> >> +        /* Null-terminated string constant. */
> >> +        char *string;
> >> +    };
> >>  };
> >>
> >>  bool expr_constant_parse(struct lexer *,
> >>                           const struct expr_field *,
> >> -                         union expr_constant *);
> >> -void expr_constant_format(const union expr_constant *,
> >> +                         struct expr_constant *);
> >> +void expr_constant_format(const struct expr_constant *,
> >>                            enum expr_constant_type, struct ds *);
> >> -void expr_constant_destroy(const union expr_constant *,
> >> +void expr_constant_destroy(const struct expr_constant *,
> >>                             enum expr_constant_type);
> >>
> >>  /* A collection of "union expr_constant"s of the same type. */
> >>  struct expr_constant_set {
> >> -    union expr_constant *values;  /* Constants. */
> >> +    struct expr_constant *values; /* Constants. */
> >>      size_t n_values;              /* Number of constants. */
> >>      enum expr_constant_type type; /* Type of the constants. */
> >>      bool in_curlies;              /* Whether the constants were in {}.
> */
> >> -    char *as_name;                /* Name of an address set. It is
> NULL if not
> >> -                                     an address set. */
> >>  };
> >>
> >>  bool expr_constant_set_parse(struct lexer *, struct expr_constant_set
> *);
> >> diff --git a/lib/actions.c b/lib/actions.c
> >> index 71615fc53..6574c4df4 100644
> >> --- a/lib/actions.c
> >> +++ b/lib/actions.c
> >> @@ -450,7 +450,7 @@ encode_LOAD(const struct ovnact_load *load,
> >>              const struct ovnact_encode_params *ep,
> >>              struct ofpbuf *ofpacts)
> >>  {
> >> -    const union expr_constant *c = &load->imm;
> >> +    const struct expr_constant *c = &load->imm;
> >>      struct mf_subfield dst = expr_resolve_field(&load->dst);
> >>      struct ofpact_set_field *sf = ofpact_put_set_field(ofpacts,
> dst.field,
> >>                                                         NULL, NULL);
> >> @@ -2122,7 +2122,7 @@ encode_event_empty_lb_backends_opts(struct ofpbuf
> *ofpacts,
> >>          /* All empty_lb_backends fields are of type 'str' */
> >>          ovs_assert(!strcmp(o->option->type, "str"));
> >>
> >> -        const union expr_constant *c = o->value.values;
> >> +        const struct expr_constant *c = o->value.values;
> >>          size_t size = strlen(c->string);
> >>          struct controller_event_opt_header hdr =
> >>              (struct controller_event_opt_header) {
> >> @@ -2598,7 +2598,7 @@ validate_empty_lb_backends(struct action_context
> *ctx,
> >>  {
> >>      for (size_t i = 0; i < n_options; i++) {
> >>          const struct ovnact_gen_option *o = &options[i];
> >> -        const union expr_constant *c = o->value.values;
> >> +        const struct expr_constant *c = o->value.values;
> >>          struct sockaddr_storage ss;
> >>          struct uuid uuid;
> >>
> >> @@ -2801,7 +2801,7 @@ encode_put_dhcpv4_option(const struct
> ovnact_gen_option *o,
> >>      uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2);
> >>      opt_header[0] = o->option->code;
> >>
> >> -    const union expr_constant *c = o->value.values;
> >> +    const struct expr_constant *c = o->value.values;
> >>      size_t n_values = o->value.n_values;
> >>      if (!strcmp(o->option->type, "bool") ||
> >>          !strcmp(o->option->type, "uint8")) {
> >> @@ -2967,7 +2967,7 @@ encode_put_dhcpv6_option(const struct
> ovnact_gen_option *o,
> >>                           struct ofpbuf *ofpacts)
> >>  {
> >>      struct dhcpv6_opt_header *opt = ofpbuf_put_uninit(ofpacts, sizeof
> *opt);
> >> -    const union expr_constant *c = o->value.values;
> >> +    const struct expr_constant *c = o->value.values;
> >>      size_t n_values = o->value.n_values;
> >>      size_t size;
> >>
> >> @@ -3023,7 +3023,7 @@ encode_PUT_DHCPV4_OPTS(const struct
> ovnact_put_opts *pdo,
> >>          find_opt(pdo->options, pdo->n_options, DHCP_OPT_BOOTFILE_CODE);
> >>      if (boot_opt) {
> >>          uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2);
> >> -        const union expr_constant *c = boot_opt->value.values;
> >> +        const struct expr_constant *c = boot_opt->value.values;
> >>          opt_header[0] = boot_opt->option->code;
> >>          opt_header[1] = strlen(c->string);
> >>          ofpbuf_put(ofpacts, c->string, opt_header[1]);
> >> @@ -3033,7 +3033,7 @@ encode_PUT_DHCPV4_OPTS(const struct
> ovnact_put_opts *pdo,
> >>          find_opt(pdo->options, pdo->n_options,
> DHCP_OPT_BOOTFILE_ALT_CODE);
> >>      if (boot_alt_opt) {
> >>          uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2);
> >> -        const union expr_constant *c = boot_alt_opt->value.values;
> >> +        const struct expr_constant *c = boot_alt_opt->value.values;
> >>          opt_header[0] = boot_alt_opt->option->code;
> >>          opt_header[1] = strlen(c->string);
> >>          ofpbuf_put(ofpacts, c->string, opt_header[1]);
> >> @@ -3043,7 +3043,7 @@ encode_PUT_DHCPV4_OPTS(const struct
> ovnact_put_opts *pdo,
> >>          pdo->options, pdo->n_options, DHCP_OPT_NEXT_SERVER_CODE);
> >>      if (next_server_opt) {
> >>          uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2);
> >> -        const union expr_constant *c = next_server_opt->value.values;
> >> +        const struct expr_constant *c = next_server_opt->value.values;
> >>          opt_header[0] = next_server_opt->option->code;
> >>          opt_header[1] = sizeof(ovs_be32);
> >>          ofpbuf_put(ofpacts, &c->value.ipv4, sizeof(ovs_be32));
> >> @@ -3247,7 +3247,7 @@ parse_put_nd_ra_opts(struct action_context *ctx,
> const struct expr_field *dst,
> >>      /* Let's validate the options. */
> >>      for (size_t i = 0; i < po->n_options; i++) {
> >>          const struct ovnact_gen_option *o = &po->options[i];
> >> -        const union expr_constant *c = o->value.values;
> >> +        const struct expr_constant *c = o->value.values;
> >>          if (o->value.n_values > 1) {
> >>              lexer_error(ctx->lexer, "Invalid value for \"%s\" option",
> >>                          o->option->name);
> >> @@ -3430,7 +3430,7 @@ static void
> >>  encode_put_nd_ra_option(const struct ovnact_gen_option *o,
> >>                          struct ofpbuf *ofpacts, ptrdiff_t ra_offset)
> >>  {
> >> -    const union expr_constant *c = o->value.values;
> >> +    const struct expr_constant *c = o->value.values;
> >>
> >>      switch (o->option->code) {
> >>      case ND_RA_FLAG_ADDR_MODE:
> >> diff --git a/lib/expr.c b/lib/expr.c
> >> index 0cb44e1b6..286782025 100644
> >> --- a/lib/expr.c
> >> +++ b/lib/expr.c
> >> @@ -179,12 +179,10 @@ expr_combine(enum expr_type type, struct expr *a,
> struct expr *b)
> >>          } else {
> >>              ovs_list_push_back(&a->andor, &b->node);
> >>          }
> >> -        free(a->as_name);
> >>          a->as_name = NULL;
> >>          return a;
> >>      } else if (b->type == type) {
> >>          ovs_list_push_front(&b->andor, &a->node);
> >> -        free(b->as_name);
> >>          b->as_name = NULL;
> >>          return b;
> >>      } else {
> >> @@ -521,12 +519,13 @@ static bool parse_field(struct expr_context *,
> struct expr_field *);
> >>
> >>  static struct expr *
> >>  make_cmp__(const struct expr_field *f, enum expr_relop r,
> >> -             const union expr_constant *c)
> >> +             const struct expr_constant *c)
> >>  {
> >>      struct expr *e = xzalloc(sizeof *e);
> >>      e->type = EXPR_T_CMP;
> >>      e->cmp.symbol = f->symbol;
> >>      e->cmp.relop = r;
> >> +    e->as_name = c->as_name;
> >>      if (f->symbol->width) {
> >>          bitwise_copy(&c->value, sizeof c->value, 0,
> >>                       &e->cmp.value, sizeof e->cmp.value, f->ofs,
> >> @@ -547,7 +546,7 @@ make_cmp__(const struct expr_field *f, enum
> expr_relop r,
> >>
> >>  /* Returns the minimum reasonable width for integer constant 'c'. */
> >>  static int
> >> -expr_constant_width(const union expr_constant *c)
> >> +expr_constant_width(const struct expr_constant *c)
> >>  {
> >>      if (c->masked) {
> >>          return mf_subvalue_width(&c->mask);
> >> @@ -674,10 +673,7 @@ make_cmp(struct expr_context *ctx,
> >>          e = expr_combine(r == EXPR_R_EQ ? EXPR_T_OR : EXPR_T_AND,
> >>                           e, make_cmp__(f, r, &cs->values[i]));
> >>      }
> >> -    /* Track address set */
> >> -    if (r == EXPR_R_EQ && e->type == EXPR_T_OR && cs->as_name) {
> >> -        e->as_name = xstrdup(cs->as_name);
> >> -    }
> >> +
> >>  exit:
> >>      expr_constant_set_destroy(cs);
> >>      return e;
> >> @@ -797,11 +793,10 @@ parse_addr_sets(struct expr_context *ctx, struct
> expr_constant_set *cs,
> >>          }
> >>      }
> >>
> >> -    struct expr_constant_set *addr_sets
> >> -        = (ctx->addr_sets
> >> -           ? shash_find_data(ctx->addr_sets, ctx->lexer->token.s)
> >> -           : NULL);
> >> -    if (!addr_sets) {
> >> +    struct shash_node *node = ctx->addr_sets
> >> +                              ? shash_find(ctx->addr_sets,
> ctx->lexer->token.s)
> >> +                              : NULL;
> >> +    if (!node) {
> >>          lexer_syntax_error(ctx->lexer, "expecting address set name");
> >>          return false;
> >>      }
> >> @@ -810,17 +805,16 @@ parse_addr_sets(struct expr_context *ctx, struct
> expr_constant_set *cs,
> >>          return false;
> >>      }
> >>
> >> -    if (!cs->n_values) {
> >> -        cs->as_name = xstrdup(ctx->lexer->token.s);
> >> -    }
> >> -
> >> +    struct expr_constant_set *addr_sets = node->data;
> >>      size_t n_values = cs->n_values + addr_sets->n_values;
> >>      if (n_values >= *allocated_values) {
> >>          cs->values = xrealloc(cs->values, n_values * sizeof
> *cs->values);
> >>          *allocated_values = n_values;
> >>      }
> >>      for (size_t i = 0; i < addr_sets->n_values; i++) {
> >> -        cs->values[cs->n_values++] = addr_sets->values[i];
> >> +        struct expr_constant *c = &cs->values[cs->n_values++];
> >> +        *c = addr_sets->values[i];
> >> +        c->as_name = node->name;
>
> I am not sure if it is safe enough to just store the reference. We need to
> check carefully if there is any chance the pointer becomes dangling when
> the corresponding address set is destroyed or in any other circumstances.
>

Currently it is safe to store as it is only for the duration of the whole
expression, in the current code base the set outlives the expression.
However it might be a problem later on, I don't know how viable it would be
in terms of memory to store clone.


>
> >>      }
> >>
> >>      return true;
> >> @@ -859,8 +853,9 @@ parse_port_group(struct expr_context *ctx, struct
> expr_constant_set *cs,
> >>          *allocated_values = n_values;
> >>      }
> >>      for (size_t i = 0; i < port_group->n_values; i++) {
> >> -        cs->values[cs->n_values++].string =
> >> -            xstrdup(port_group->values[i].string);
> >> +        struct expr_constant *c = &cs->values[cs->n_values++];
> >> +        c->string = xstrdup(port_group->values[i].string);
> >> +        c->as_name = NULL;
> >>      }
> >>
> >>      return true;
> >> @@ -875,11 +870,6 @@ parse_constant(struct expr_context *ctx, struct
> expr_constant_set *cs,
> >>                                  sizeof *cs->values);
> >>      }
> >>
> >> -    /* Combining other values to the constant set that is tracking an
> >> -     * address set, so untrack it. */
> >> -    free(cs->as_name);
> >> -    cs->as_name = NULL;
> >> -
> >>      if (ctx->lexer->token.type == LEX_T_TEMPLATE) {
> >>          lexer_error(ctx->lexer, "Unexpanded template.");
> >>          return false;
> >> @@ -887,7 +877,9 @@ parse_constant(struct expr_context *ctx, struct
> expr_constant_set *cs,
> >>          if (!assign_constant_set_type(ctx, cs, EXPR_C_STRING)) {
> >>              return false;
> >>          }
> >> -        cs->values[cs->n_values++].string =
> xstrdup(ctx->lexer->token.s);
> >> +        struct expr_constant *c = &cs->values[cs->n_values++];
> >> +        c->string = xstrdup(ctx->lexer->token.s);
> >> +        c->as_name = NULL;
> >>          lexer_get(ctx->lexer);
> >>          return true;
> >>      } else if (ctx->lexer->token.type == LEX_T_INTEGER ||
> >> @@ -896,13 +888,14 @@ parse_constant(struct expr_context *ctx, struct
> expr_constant_set *cs,
> >>              return false;
> >>          }
> >>
> >> -        union expr_constant *c = &cs->values[cs->n_values++];
> >> +        struct expr_constant *c = &cs->values[cs->n_values++];
> >>          c->value = ctx->lexer->token.value;
> >>          c->format = ctx->lexer->token.format;
> >>          c->masked = ctx->lexer->token.type == LEX_T_MASKED_INTEGER;
> >>          if (c->masked) {
> >>              c->mask = ctx->lexer->token.mask;
> >>          }
> >> +        c->as_name = NULL;
> >>          lexer_get(ctx->lexer);
> >>          return true;
> >>      } else if (ctx->lexer->token.type == LEX_T_MACRO) {
> >> @@ -961,7 +954,7 @@ parse_constant_set(struct expr_context *ctx, struct
> expr_constant_set *cs)
> >>   * indeterminate. */
> >>  bool
> >>  expr_constant_parse(struct lexer *lexer, const struct expr_field *f,
> >> -                    union expr_constant *c)
> >> +                    struct expr_constant *c)
> >>  {
> >>      if (lexer->error) {
> >>          return false;
> >> @@ -987,7 +980,7 @@ expr_constant_parse(struct lexer *lexer, const
> struct expr_field *f,
> >>  /* Appends to 's' a re-parseable representation of constant 'c' with
> the given
> >>   * 'type'. */
> >>  void
> >> -expr_constant_format(const union expr_constant *c,
> >> +expr_constant_format(const struct expr_constant *c,
> >>                       enum expr_constant_type type, struct ds *s)
> >>  {
> >>      if (type == EXPR_C_STRING) {
> >> @@ -1010,7 +1003,7 @@ expr_constant_format(const union expr_constant *c,
> >>   *
> >>   * Does not free(c). */
> >>  void
> >> -expr_constant_destroy(const union expr_constant *c,
> >> +expr_constant_destroy(const struct expr_constant *c,
> >>                        enum expr_constant_type type)
> >>  {
> >>      if (c && type == EXPR_C_STRING) {
> >> @@ -1043,7 +1036,7 @@ expr_constant_set_format(const struct
> expr_constant_set *cs, struct ds *s)
> >>          ds_put_char(s, '{');
> >>      }
> >>
> >> -    for (const union expr_constant *c = cs->values;
> >> +    for (const struct expr_constant *c = cs->values;
> >>           c < &cs->values[cs->n_values]; c++) {
> >>          if (c != cs->values) {
> >>              ds_put_cstr(s, ", ");
> >> @@ -1067,15 +1060,14 @@ expr_constant_set_destroy(struct
> expr_constant_set *cs)
> >>              }
> >>          }
> >>          free(cs->values);
> >> -        free(cs->as_name);
> >>      }
> >>  }
> >>
> >>  static int
> >>  compare_expr_constant_integer_cb(const void *a_, const void *b_)
> >>  {
> >> -    const union expr_constant *a = a_;
> >> -    const union expr_constant *b = b_;
> >> +    const struct expr_constant *a = a_;
> >> +    const struct expr_constant *b = b_;
> >>
> >>      int d = memcmp(&a->value, &b->value, sizeof a->value);
> >>      if (d) {
> >> @@ -1114,7 +1106,7 @@ expr_constant_set_create_integers(const char
> *const *values, size_t n_values)
> >>              VLOG_WARN("Invalid constant set entry: '%s', token type:
> %d",
> >>                        values[i], lex.token.type);
> >>          } else {
> >> -            union expr_constant *c = &cs->values[cs->n_values++];
> >> +            struct expr_constant *c = &cs->values[cs->n_values++];
> >>              c->value = lex.token.value;
> >>              c->format = lex.token.format;
> >>              c->masked = lex.token.type == LEX_T_MASKED_INTEGER;
> >> @@ -1135,7 +1127,7 @@ expr_constant_set_create_integers(const char
> *const *values, size_t n_values)
> >>
> >>  static void
> >>  expr_constant_set_add_value(struct expr_constant_set **p_cs,
> >> -                            union expr_constant *c, size_t *allocated)
> >> +                            struct expr_constant *c, size_t *allocated)
> >>  {
> >>      struct expr_constant_set *cs = *p_cs;
> >>      if (!cs) {
> >> @@ -1246,7 +1238,7 @@ expr_const_sets_add_strings(struct shash
> *const_sets, const char *name,
> >>                          values[i], name);
> >>              continue;
> >>          }
> >> -        union expr_constant *c = &cs->values[cs->n_values++];
> >> +        struct expr_constant *c = &cs->values[cs->n_values++];
> >>          c->string = xstrdup(values[i]);
> >>      }
> >>
> >> @@ -1359,7 +1351,7 @@ expr_parse_primary(struct expr_context *ctx, bool
> *atomic)
> >>
> >>              *atomic = true;
> >>
> >> -            union expr_constant *cst = xzalloc(sizeof *cst);
> >> +            struct expr_constant *cst = xzalloc(sizeof *cst);
> >>              cst->format = LEX_F_HEXADECIMAL;
> >>              cst->masked = false;
> >>
> >> @@ -1367,7 +1359,6 @@ expr_parse_primary(struct expr_context *ctx, bool
> *atomic)
> >>              c.values = cst;
> >>              c.n_values = 1;
> >>              c.in_curlies = false;
> >> -            c.as_name = NULL;
> >>              return make_cmp(ctx, &f, EXPR_R_NE, &c);
> >>          } else if (parse_relop(ctx, &r) && parse_constant_set(ctx,
> &c)) {
> >>              return make_cmp(ctx, &f, r, &c);
> >> @@ -1834,7 +1825,6 @@ expr_symtab_destroy(struct shash *symtab)
> >>  static struct expr *
> >>  expr_clone_cmp(struct expr *expr)
> >>  {
> >> -    ovs_assert(!expr->as_name);
> >>      struct expr *new = xmemdup(expr, sizeof *expr);
> >>      if (!new->cmp.symbol->width) {
> >>          new->cmp.string = xstrdup(new->cmp.string);
> >> @@ -1858,7 +1848,6 @@ expr_clone_andor(struct expr *expr)
> >>  static struct expr *
> >>  expr_clone_condition(struct expr *expr)
> >>  {
> >> -    ovs_assert(!expr->as_name);
> >>      struct expr *new = xmemdup(expr, sizeof *expr);
> >>      new->cond.string = xstrdup(new->cond.string);
> >>      return new;
> >> @@ -1894,8 +1883,6 @@ expr_destroy(struct expr *expr)
> >>          return;
> >>      }
> >>
> >> -    free(expr->as_name);
> >> -
> >>      struct expr *sub;
> >>
> >>      switch (expr->type) {
> >> @@ -2567,7 +2554,6 @@ crush_or_supersets(struct expr *expr, const
> struct expr_symbol *symbol)
> >>       * mask sizes. */
> >>      size_t n = ovs_list_size(&expr->andor);
> >>      struct expr **subs = xmalloc(n * sizeof *subs);
> >> -    bool modified = false;
> >>      /* Linked list over the 'subs' array to quickly skip deleted
> elements,
> >>       * i.e. the index of the next potentially non-NULL element. */
> >>      size_t *next = xmalloc(n * sizeof *next);
> >> @@ -2596,14 +2582,14 @@ crush_or_supersets(struct expr *expr, const
> struct expr_symbol *symbol)
> >>              next[last] = i;
> >>              last = i;
> >>          } else {
> >> +            /* Remove address set reference from the duplicate. */
> >> +            subs[last]->as_name = NULL;
> >>              expr_destroy(subs[i]);
> >>              subs[i] = NULL;
> >> -            modified = true;
> >>          }
> >>      }
> >>
> >> -    if (!symbol->width || symbol->level != EXPR_L_ORDINAL
> >> -        || (!modified && expr->as_name)) {
> >> +    if (!symbol->width || symbol->level != EXPR_L_ORDINAL) {
> >>          /* Not a fully maskable field or this expression is tracking an
> >>           * address set.  Don't try to optimize to preserve address set
> I-P. */
>
> The above comment needs to be updated since it obviously doesn't preserve
> address set I-P any more. What's the reason it is not considered any more?
>

Because of the AS reference per sub expression we can actually allow this
optimization to happen. In the case of the smaller mask we will lose track
of only that address instead of the whole set. I'll update the comment.


>
> >>          goto done;
> >> @@ -2658,10 +2644,11 @@ crush_or_supersets(struct expr *expr, const
> struct expr_symbol *symbol)
> >>              if (expr_bitmap_intersect_check(a_value, a_mask, b_value,
> b_mask,
> >>                                              bit_width)
> >>                  && bitmap_is_superset(b_mask, a_mask, bit_width)) {
> >> -                /* 'a' is the same expression with a smaller mask. */
> >> +                /* 'a' is the same expression with a smaller mask.
> >> +                 * Remove address set reference from the duplicate. */
> >> +                a->as_name = NULL;
> >>                  expr_destroy(subs[j]);
> >>                  subs[j] = NULL;
> >> -                modified = true;
> >>
> >>                  /* Shorten the path for the next round. */
> >>                  if (last) {
> >> @@ -2685,12 +2672,6 @@ done:
> >>          }
> >>      }
> >>
> >> -    if (modified) {
> >> -        /* Members modified, so untrack address set. */
> >> -        free(expr->as_name);
> >> -        expr->as_name = NULL;
> >> -    }
> >> -
> >>      free(next);
> >>      free(subs);
> >>      return expr;
> >> @@ -3271,10 +3252,10 @@ add_disjunction(const struct expr *or,
> >>      LIST_FOR_EACH (sub, node, &or->andor) {
> >>          struct expr_match *match = expr_match_new(m, clause, n_clauses,
> >>                                                    conj_id);
> >> -        if (or->as_name) {
> >> +        if (sub->as_name) {
> >>              ovs_assert(sub->type == EXPR_T_CMP);
> >>              ovs_assert(sub->cmp.symbol->width);
> >> -            match->as_name = xstrdup(or->as_name);
> >> +            match->as_name = xstrdup(sub->as_name);
> >>              match->as_ip = sub->cmp.value.ipv6;
> >>              match->as_mask = sub->cmp.mask.ipv6;
> >>          }
> >> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> >> index fdcc5aab2..626dd34f6 100644
> >> --- a/tests/ovn-controller.at
> >> +++ b/tests/ovn-controller.at
> >> @@ -1630,7 +1630,9 @@
> priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.8
> actions=lo
> >>  done
> >>
> >>  reprocess_count_new=$(read_counter consider_logical_flow)
> >> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
> [10
> >> +# First 2 reprocess are due to change from 1 IP in AS to 2.
> >> +# Last 5 is due to overlap in IP addresses between as1 and as2.
> >> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
> [7
> >>  ])
> >>
> >>  # Remove the IPs from as1 and as2, 1 IP each time.
> >> @@ -1653,9 +1655,9 @@ for i in $(seq 10); do
> >>  done
> >>
> >>  reprocess_count_new=$(read_counter consider_logical_flow)
> >> -# In this case the as1 and as2 are merged to a single OR expr, so we
> lose track of
> >> -# address set information - can't handle deletion incrementally.
> >> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
> [10
> >> +# First 5 are due to overlap in IP addresses between as1 and as2.
> >> +# Last 2 are due to change from 2 IP in AS to 1.
> >> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
> [7
> >>  ])
> >>
> >>  OVN_CLEANUP([hv1])
> >> @@ -1822,7 +1824,7 @@
> priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.5
> actions=co
> >>
>  priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.10.10.10
> actions=conjunction,2/2)
> >>  ])
> >>  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
> >>  ])
> >>
> >>  # Delete 2 IPs
> >> @@ -1842,7 +1844,7 @@
> priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3
> actions=co
> >>
>  priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.10.10.10
> actions=conjunction,2/2)
> >>  ])
> >>  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])
> >> --
> >> 2.44.0
> >>
> >
> > Please consider it as RFC, I forgot to add the tag into subject.
>
> Is there a reason why this is RFC? Do you have any TODOs in mind?
>

Mainly to be sure if this approach is heading the right direction, which it
seems it does. There shouldn't be any additional TODO right now.


> I am just not 100% confident about corner cases. The existing test cases
> seem to be handled correctly. We might need to think about if there are any
> other potentially risky scenarios that need to be tested, although I don't
> see any obvious problem for now.
>


So things that came to my mind are covered by the tests, let's see if
others will think about potentially dangerous scenarios.


> Thanks,
> Han
>
> > --
> >
> > Ales Musil
> >
> > Senior Software Engineer - OVN Core
> >
> > Red Hat EMEA
> >
> > amusil@redhat.com
>


I'll wait a bit for feedback from others before posting v2.


Thanks,
Ales

On Wed, Mar 27, 2024 at 12:46 AM Ales Musil <amusil@redhat.com> wrote:
>
> On Wed, Mar 27, 2024 at 7:14 AM Han Zhou <hzhou@ovn.org> wrote:
>
> >
> >
> > On Tue, Mar 19, 2024 at 9:45 AM Ales Musil <amusil@redhat.com> wrote:
> > >
> > >
> > >
> > > On Tue, Mar 19, 2024 at 5:43 PM Ales Musil <amusil@redhat.com> wrote:
> > >>
> > >> Instead of tracking address set per struct expr_constant_set track it
> > >> per individual struct expr_constant. This allows more fine grained
> > >> control for I-P processing of address sets in controller. It helps
with
> > >> scenarios like matching on two address sets in one expression e.g.
> > >> "ip4.src == {$as1, $as2}". This allows any addition or removal of
> > >> individual adress from the set to be incrementally processed instead
> > >> of reprocessing all the flows.
> > >>
> > >> This unfortunately doesn't help with the following flows:
> > >> "ip4.src == $as1 && ip4.dst == $as2"
> > >> "ip4.src == $as1 || ip4.dst == $as2"
> > >>
> > >> The memory impact should be minimal as there is only increase of 8
bytes
> > >> per the struct expr_constant.
> > >>
> > >> Signed-off-by: Ales Musil <amusil@redhat.com>
> >
> > Thanks Ales for the improvement! The approach looks good to me. Please
see
> > some comments below:
> >
> >
> >
> Hi Han,
>
> thank you for the review.
>
>
> >
> > >> ---
> > >>  controller/lflow.c      |  4 +-
> > >>  include/ovn/actions.h   |  2 +-
> > >>  include/ovn/expr.h      | 46 ++++++++++----------
> > >>  lib/actions.c           | 20 ++++-----
> > >>  lib/expr.c              | 95
+++++++++++++++++------------------------
> > >>  tests/ovn-controller.at | 14 +++---
> > >>  6 files changed, 83 insertions(+), 98 deletions(-)
> > >>
> > >> diff --git a/controller/lflow.c b/controller/lflow.c
> > >> index 895d17d19..730dc879d 100644
> > >> --- a/controller/lflow.c
> > >> +++ b/controller/lflow.c
> > >> @@ -278,7 +278,7 @@ lflow_handle_changed_flows(struct lflow_ctx_in
> > *l_ctx_in,
> > >>  }
> > >>
> > >>  static bool
> > >> -as_info_from_expr_const(const char *as_name, const union
expr_constant
> > *c,
> > >> +as_info_from_expr_const(const char *as_name, const struct
> > expr_constant *c,
> > >>                          struct addrset_info *as_info)
> > >>  {
> > >>      as_info->name = as_name;
> > >> @@ -714,7 +714,7 @@ lflow_handle_addr_set_update(const char *as_name,
> > >>          if (as_diff->deleted) {
> > >>              struct addrset_info as_info;
> > >>              for (size_t i = 0; i < as_diff->deleted->n_values; i++)
{
> > >> -                union expr_constant *c =
&as_diff->deleted->values[i];
> > >> +                struct expr_constant *c =
&as_diff->deleted->values[i];
> > >>                  if (!as_info_from_expr_const(as_name, c, &as_info))
{
> > >>                      continue;
> > >>                  }
> > >> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > >> index dcacbb1ff..1e20f9b81 100644
> > >> --- a/include/ovn/actions.h
> > >> +++ b/include/ovn/actions.h
> > >> @@ -238,7 +238,7 @@ struct ovnact_next {
> > >>  struct ovnact_load {
> > >>      struct ovnact ovnact;
> > >>      struct expr_field dst;
> > >> -    union expr_constant imm;
> > >> +    struct expr_constant imm;
> > >>  };
> > >>
> > >>  /* OVNACT_MOVE, OVNACT_EXCHANGE. */
> > >> diff --git a/include/ovn/expr.h b/include/ovn/expr.h
> > >> index c48f82398..e54edb5bf 100644
> > >> --- a/include/ovn/expr.h
> > >> +++ b/include/ovn/expr.h
> > >> @@ -368,7 +368,7 @@ bool expr_relop_from_token(enum lex_type type,
enum
> > expr_relop *relop);
> > >>  struct expr {
> > >>      struct ovs_list node;       /* In parent EXPR_T_AND or EXPR_T_OR
> > if any. */
> > >>      enum expr_type type;        /* Expression type. */
> > >> -    char *as_name;              /* Address set name. Null if it is
not
> > an
> > >> +    const char *as_name;        /* Address set name. Null if it is
not
> > an
> > >>                                     address set. */
> > >>
> > >>      union {
> > >> @@ -505,40 +505,42 @@ enum expr_constant_type {
> > >>  };
> > >>
> > >>  /* A string or integer constant (one must know which from context).
*/
> > >> -union expr_constant {
> > >> -    /* Integer constant.
> > >> -     *
> > >> -     * The width of a constant isn't always clear, e.g. if you write
> > "1",
> > >> -     * there's no way to tell whether you mean for that to be a
1-bit
> > constant
> > >> -     * or a 128-bit constant or somewhere in between. */
> > >> -    struct {
> > >> -        union mf_subvalue value;
> > >> -        union mf_subvalue mask; /* Only initialized if 'masked'. */
> > >> -        bool masked;
> > >> -
> > >> -        enum lex_format format; /* From the constant's lex_token. */
> > >> -    };
> > >> +struct expr_constant {
> > >> +    const char *as_name;
> > >>
> > >> -    /* Null-terminated string constant. */
> > >> -    char *string;
> > >> +    union {
> > >> +        /* Integer constant.
> > >> +         *
> > >> +         * The width of a constant isn't always clear, e.g. if you
> > write "1",
> > >> +         * there's no way to tell whether you mean for that to be a
> > 1-bit
> > >> +         * constant or a 128-bit constant or somewhere in between.
*/
> > >> +        struct {
> > >> +            union mf_subvalue value;
> > >> +            union mf_subvalue mask; /* Only initialized if
'masked'. */
> > >> +            bool masked;
> > >> +
> > >> +            enum lex_format format; /* From the constant's
lex_token.
> > */
> > >> +        };
> > >> +
> > >> +        /* Null-terminated string constant. */
> > >> +        char *string;
> > >> +    };
> > >>  };
> > >>
> > >>  bool expr_constant_parse(struct lexer *,
> > >>                           const struct expr_field *,
> > >> -                         union expr_constant *);
> > >> -void expr_constant_format(const union expr_constant *,
> > >> +                         struct expr_constant *);
> > >> +void expr_constant_format(const struct expr_constant *,
> > >>                            enum expr_constant_type, struct ds *);
> > >> -void expr_constant_destroy(const union expr_constant *,
> > >> +void expr_constant_destroy(const struct expr_constant *,
> > >>                             enum expr_constant_type);
> > >>
> > >>  /* A collection of "union expr_constant"s of the same type. */
> > >>  struct expr_constant_set {
> > >> -    union expr_constant *values;  /* Constants. */
> > >> +    struct expr_constant *values; /* Constants. */
> > >>      size_t n_values;              /* Number of constants. */
> > >>      enum expr_constant_type type; /* Type of the constants. */
> > >>      bool in_curlies;              /* Whether the constants were in
{}.
> > */
> > >> -    char *as_name;                /* Name of an address set. It is
> > NULL if not
> > >> -                                     an address set. */
> > >>  };
> > >>
> > >>  bool expr_constant_set_parse(struct lexer *, struct
expr_constant_set
> > *);
> > >> diff --git a/lib/actions.c b/lib/actions.c
> > >> index 71615fc53..6574c4df4 100644
> > >> --- a/lib/actions.c
> > >> +++ b/lib/actions.c
> > >> @@ -450,7 +450,7 @@ encode_LOAD(const struct ovnact_load *load,
> > >>              const struct ovnact_encode_params *ep,
> > >>              struct ofpbuf *ofpacts)
> > >>  {
> > >> -    const union expr_constant *c = &load->imm;
> > >> +    const struct expr_constant *c = &load->imm;
> > >>      struct mf_subfield dst = expr_resolve_field(&load->dst);
> > >>      struct ofpact_set_field *sf = ofpact_put_set_field(ofpacts,
> > dst.field,
> > >>                                                         NULL, NULL);
> > >> @@ -2122,7 +2122,7 @@ encode_event_empty_lb_backends_opts(struct
ofpbuf
> > *ofpacts,
> > >>          /* All empty_lb_backends fields are of type 'str' */
> > >>          ovs_assert(!strcmp(o->option->type, "str"));
> > >>
> > >> -        const union expr_constant *c = o->value.values;
> > >> +        const struct expr_constant *c = o->value.values;
> > >>          size_t size = strlen(c->string);
> > >>          struct controller_event_opt_header hdr =
> > >>              (struct controller_event_opt_header) {
> > >> @@ -2598,7 +2598,7 @@ validate_empty_lb_backends(struct
action_context
> > *ctx,
> > >>  {
> > >>      for (size_t i = 0; i < n_options; i++) {
> > >>          const struct ovnact_gen_option *o = &options[i];
> > >> -        const union expr_constant *c = o->value.values;
> > >> +        const struct expr_constant *c = o->value.values;
> > >>          struct sockaddr_storage ss;
> > >>          struct uuid uuid;
> > >>
> > >> @@ -2801,7 +2801,7 @@ encode_put_dhcpv4_option(const struct
> > ovnact_gen_option *o,
> > >>      uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2);
> > >>      opt_header[0] = o->option->code;
> > >>
> > >> -    const union expr_constant *c = o->value.values;
> > >> +    const struct expr_constant *c = o->value.values;
> > >>      size_t n_values = o->value.n_values;
> > >>      if (!strcmp(o->option->type, "bool") ||
> > >>          !strcmp(o->option->type, "uint8")) {
> > >> @@ -2967,7 +2967,7 @@ encode_put_dhcpv6_option(const struct
> > ovnact_gen_option *o,
> > >>                           struct ofpbuf *ofpacts)
> > >>  {
> > >>      struct dhcpv6_opt_header *opt = ofpbuf_put_uninit(ofpacts,
sizeof
> > *opt);
> > >> -    const union expr_constant *c = o->value.values;
> > >> +    const struct expr_constant *c = o->value.values;
> > >>      size_t n_values = o->value.n_values;
> > >>      size_t size;
> > >>
> > >> @@ -3023,7 +3023,7 @@ encode_PUT_DHCPV4_OPTS(const struct
> > ovnact_put_opts *pdo,
> > >>          find_opt(pdo->options, pdo->n_options,
DHCP_OPT_BOOTFILE_CODE);
> > >>      if (boot_opt) {
> > >>          uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2);
> > >> -        const union expr_constant *c = boot_opt->value.values;
> > >> +        const struct expr_constant *c = boot_opt->value.values;
> > >>          opt_header[0] = boot_opt->option->code;
> > >>          opt_header[1] = strlen(c->string);
> > >>          ofpbuf_put(ofpacts, c->string, opt_header[1]);
> > >> @@ -3033,7 +3033,7 @@ encode_PUT_DHCPV4_OPTS(const struct
> > ovnact_put_opts *pdo,
> > >>          find_opt(pdo->options, pdo->n_options,
> > DHCP_OPT_BOOTFILE_ALT_CODE);
> > >>      if (boot_alt_opt) {
> > >>          uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2);
> > >> -        const union expr_constant *c = boot_alt_opt->value.values;
> > >> +        const struct expr_constant *c = boot_alt_opt->value.values;
> > >>          opt_header[0] = boot_alt_opt->option->code;
> > >>          opt_header[1] = strlen(c->string);
> > >>          ofpbuf_put(ofpacts, c->string, opt_header[1]);
> > >> @@ -3043,7 +3043,7 @@ encode_PUT_DHCPV4_OPTS(const struct
> > ovnact_put_opts *pdo,
> > >>          pdo->options, pdo->n_options, DHCP_OPT_NEXT_SERVER_CODE);
> > >>      if (next_server_opt) {
> > >>          uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2);
> > >> -        const union expr_constant *c =
next_server_opt->value.values;
> > >> +        const struct expr_constant *c =
next_server_opt->value.values;
> > >>          opt_header[0] = next_server_opt->option->code;
> > >>          opt_header[1] = sizeof(ovs_be32);
> > >>          ofpbuf_put(ofpacts, &c->value.ipv4, sizeof(ovs_be32));
> > >> @@ -3247,7 +3247,7 @@ parse_put_nd_ra_opts(struct action_context
*ctx,
> > const struct expr_field *dst,
> > >>      /* Let's validate the options. */
> > >>      for (size_t i = 0; i < po->n_options; i++) {
> > >>          const struct ovnact_gen_option *o = &po->options[i];
> > >> -        const union expr_constant *c = o->value.values;
> > >> +        const struct expr_constant *c = o->value.values;
> > >>          if (o->value.n_values > 1) {
> > >>              lexer_error(ctx->lexer, "Invalid value for \"%s\"
option",
> > >>                          o->option->name);
> > >> @@ -3430,7 +3430,7 @@ static void
> > >>  encode_put_nd_ra_option(const struct ovnact_gen_option *o,
> > >>                          struct ofpbuf *ofpacts, ptrdiff_t ra_offset)
> > >>  {
> > >> -    const union expr_constant *c = o->value.values;
> > >> +    const struct expr_constant *c = o->value.values;
> > >>
> > >>      switch (o->option->code) {
> > >>      case ND_RA_FLAG_ADDR_MODE:
> > >> diff --git a/lib/expr.c b/lib/expr.c
> > >> index 0cb44e1b6..286782025 100644
> > >> --- a/lib/expr.c
> > >> +++ b/lib/expr.c
> > >> @@ -179,12 +179,10 @@ expr_combine(enum expr_type type, struct expr
*a,
> > struct expr *b)
> > >>          } else {
> > >>              ovs_list_push_back(&a->andor, &b->node);
> > >>          }
> > >> -        free(a->as_name);
> > >>          a->as_name = NULL;
> > >>          return a;
> > >>      } else if (b->type == type) {
> > >>          ovs_list_push_front(&b->andor, &a->node);
> > >> -        free(b->as_name);
> > >>          b->as_name = NULL;
> > >>          return b;
> > >>      } else {
> > >> @@ -521,12 +519,13 @@ static bool parse_field(struct expr_context *,
> > struct expr_field *);
> > >>
> > >>  static struct expr *
> > >>  make_cmp__(const struct expr_field *f, enum expr_relop r,
> > >> -             const union expr_constant *c)
> > >> +             const struct expr_constant *c)
> > >>  {
> > >>      struct expr *e = xzalloc(sizeof *e);
> > >>      e->type = EXPR_T_CMP;
> > >>      e->cmp.symbol = f->symbol;
> > >>      e->cmp.relop = r;
> > >> +    e->as_name = c->as_name;
> > >>      if (f->symbol->width) {
> > >>          bitwise_copy(&c->value, sizeof c->value, 0,
> > >>                       &e->cmp.value, sizeof e->cmp.value, f->ofs,
> > >> @@ -547,7 +546,7 @@ make_cmp__(const struct expr_field *f, enum
> > expr_relop r,
> > >>
> > >>  /* Returns the minimum reasonable width for integer constant 'c'. */
> > >>  static int
> > >> -expr_constant_width(const union expr_constant *c)
> > >> +expr_constant_width(const struct expr_constant *c)
> > >>  {
> > >>      if (c->masked) {
> > >>          return mf_subvalue_width(&c->mask);
> > >> @@ -674,10 +673,7 @@ make_cmp(struct expr_context *ctx,
> > >>          e = expr_combine(r == EXPR_R_EQ ? EXPR_T_OR : EXPR_T_AND,
> > >>                           e, make_cmp__(f, r, &cs->values[i]));
> > >>      }
> > >> -    /* Track address set */
> > >> -    if (r == EXPR_R_EQ && e->type == EXPR_T_OR && cs->as_name) {
> > >> -        e->as_name = xstrdup(cs->as_name);
> > >> -    }
> > >> +
> > >>  exit:
> > >>      expr_constant_set_destroy(cs);
> > >>      return e;
> > >> @@ -797,11 +793,10 @@ parse_addr_sets(struct expr_context *ctx,
struct
> > expr_constant_set *cs,
> > >>          }
> > >>      }
> > >>
> > >> -    struct expr_constant_set *addr_sets
> > >> -        = (ctx->addr_sets
> > >> -           ? shash_find_data(ctx->addr_sets, ctx->lexer->token.s)
> > >> -           : NULL);
> > >> -    if (!addr_sets) {
> > >> +    struct shash_node *node = ctx->addr_sets
> > >> +                              ? shash_find(ctx->addr_sets,
> > ctx->lexer->token.s)
> > >> +                              : NULL;
> > >> +    if (!node) {
> > >>          lexer_syntax_error(ctx->lexer, "expecting address set
name");
> > >>          return false;
> > >>      }
> > >> @@ -810,17 +805,16 @@ parse_addr_sets(struct expr_context *ctx,
struct
> > expr_constant_set *cs,
> > >>          return false;
> > >>      }
> > >>
> > >> -    if (!cs->n_values) {
> > >> -        cs->as_name = xstrdup(ctx->lexer->token.s);
> > >> -    }
> > >> -
> > >> +    struct expr_constant_set *addr_sets = node->data;
> > >>      size_t n_values = cs->n_values + addr_sets->n_values;
> > >>      if (n_values >= *allocated_values) {
> > >>          cs->values = xrealloc(cs->values, n_values * sizeof
> > *cs->values);
> > >>          *allocated_values = n_values;
> > >>      }
> > >>      for (size_t i = 0; i < addr_sets->n_values; i++) {
> > >> -        cs->values[cs->n_values++] = addr_sets->values[i];
> > >> +        struct expr_constant *c = &cs->values[cs->n_values++];
> > >> +        *c = addr_sets->values[i];
> > >> +        c->as_name = node->name;
> >
> > I am not sure if it is safe enough to just store the reference. We need
to
> > check carefully if there is any chance the pointer becomes dangling when
> > the corresponding address set is destroyed or in any other
circumstances.
> >
>
> Currently it is safe to store as it is only for the duration of the whole
> expression, in the current code base the set outlives the expression.
> However it might be a problem later on, I don't know how viable it would
be
> in terms of memory to store clone.
>
>
> >
> > >>      }
> > >>
> > >>      return true;
> > >> @@ -859,8 +853,9 @@ parse_port_group(struct expr_context *ctx, struct
> > expr_constant_set *cs,
> > >>          *allocated_values = n_values;
> > >>      }
> > >>      for (size_t i = 0; i < port_group->n_values; i++) {
> > >> -        cs->values[cs->n_values++].string =
> > >> -            xstrdup(port_group->values[i].string);
> > >> +        struct expr_constant *c = &cs->values[cs->n_values++];
> > >> +        c->string = xstrdup(port_group->values[i].string);
> > >> +        c->as_name = NULL;
> > >>      }
> > >>
> > >>      return true;
> > >> @@ -875,11 +870,6 @@ parse_constant(struct expr_context *ctx, struct
> > expr_constant_set *cs,
> > >>                                  sizeof *cs->values);
> > >>      }
> > >>
> > >> -    /* Combining other values to the constant set that is tracking
an
> > >> -     * address set, so untrack it. */
> > >> -    free(cs->as_name);
> > >> -    cs->as_name = NULL;
> > >> -
> > >>      if (ctx->lexer->token.type == LEX_T_TEMPLATE) {
> > >>          lexer_error(ctx->lexer, "Unexpanded template.");
> > >>          return false;
> > >> @@ -887,7 +877,9 @@ parse_constant(struct expr_context *ctx, struct
> > expr_constant_set *cs,
> > >>          if (!assign_constant_set_type(ctx, cs, EXPR_C_STRING)) {
> > >>              return false;
> > >>          }
> > >> -        cs->values[cs->n_values++].string =
> > xstrdup(ctx->lexer->token.s);
> > >> +        struct expr_constant *c = &cs->values[cs->n_values++];
> > >> +        c->string = xstrdup(ctx->lexer->token.s);
> > >> +        c->as_name = NULL;
> > >>          lexer_get(ctx->lexer);
> > >>          return true;
> > >>      } else if (ctx->lexer->token.type == LEX_T_INTEGER ||
> > >> @@ -896,13 +888,14 @@ parse_constant(struct expr_context *ctx, struct
> > expr_constant_set *cs,
> > >>              return false;
> > >>          }
> > >>
> > >> -        union expr_constant *c = &cs->values[cs->n_values++];
> > >> +        struct expr_constant *c = &cs->values[cs->n_values++];
> > >>          c->value = ctx->lexer->token.value;
> > >>          c->format = ctx->lexer->token.format;
> > >>          c->masked = ctx->lexer->token.type == LEX_T_MASKED_INTEGER;
> > >>          if (c->masked) {
> > >>              c->mask = ctx->lexer->token.mask;
> > >>          }
> > >> +        c->as_name = NULL;
> > >>          lexer_get(ctx->lexer);
> > >>          return true;
> > >>      } else if (ctx->lexer->token.type == LEX_T_MACRO) {
> > >> @@ -961,7 +954,7 @@ parse_constant_set(struct expr_context *ctx,
struct
> > expr_constant_set *cs)
> > >>   * indeterminate. */
> > >>  bool
> > >>  expr_constant_parse(struct lexer *lexer, const struct expr_field *f,
> > >> -                    union expr_constant *c)
> > >> +                    struct expr_constant *c)
> > >>  {
> > >>      if (lexer->error) {
> > >>          return false;
> > >> @@ -987,7 +980,7 @@ expr_constant_parse(struct lexer *lexer, const
> > struct expr_field *f,
> > >>  /* Appends to 's' a re-parseable representation of constant 'c' with
> > the given
> > >>   * 'type'. */
> > >>  void
> > >> -expr_constant_format(const union expr_constant *c,
> > >> +expr_constant_format(const struct expr_constant *c,
> > >>                       enum expr_constant_type type, struct ds *s)
> > >>  {
> > >>      if (type == EXPR_C_STRING) {
> > >> @@ -1010,7 +1003,7 @@ expr_constant_format(const union expr_constant
*c,
> > >>   *
> > >>   * Does not free(c). */
> > >>  void
> > >> -expr_constant_destroy(const union expr_constant *c,
> > >> +expr_constant_destroy(const struct expr_constant *c,
> > >>                        enum expr_constant_type type)
> > >>  {
> > >>      if (c && type == EXPR_C_STRING) {
> > >> @@ -1043,7 +1036,7 @@ expr_constant_set_format(const struct
> > expr_constant_set *cs, struct ds *s)
> > >>          ds_put_char(s, '{');
> > >>      }
> > >>
> > >> -    for (const union expr_constant *c = cs->values;
> > >> +    for (const struct expr_constant *c = cs->values;
> > >>           c < &cs->values[cs->n_values]; c++) {
> > >>          if (c != cs->values) {
> > >>              ds_put_cstr(s, ", ");
> > >> @@ -1067,15 +1060,14 @@ expr_constant_set_destroy(struct
> > expr_constant_set *cs)
> > >>              }
> > >>          }
> > >>          free(cs->values);
> > >> -        free(cs->as_name);
> > >>      }
> > >>  }
> > >>
> > >>  static int
> > >>  compare_expr_constant_integer_cb(const void *a_, const void *b_)
> > >>  {
> > >> -    const union expr_constant *a = a_;
> > >> -    const union expr_constant *b = b_;
> > >> +    const struct expr_constant *a = a_;
> > >> +    const struct expr_constant *b = b_;
> > >>
> > >>      int d = memcmp(&a->value, &b->value, sizeof a->value);
> > >>      if (d) {
> > >> @@ -1114,7 +1106,7 @@ expr_constant_set_create_integers(const char
> > *const *values, size_t n_values)
> > >>              VLOG_WARN("Invalid constant set entry: '%s', token type:
> > %d",
> > >>                        values[i], lex.token.type);
> > >>          } else {
> > >> -            union expr_constant *c = &cs->values[cs->n_values++];
> > >> +            struct expr_constant *c = &cs->values[cs->n_values++];
> > >>              c->value = lex.token.value;
> > >>              c->format = lex.token.format;
> > >>              c->masked = lex.token.type == LEX_T_MASKED_INTEGER;
> > >> @@ -1135,7 +1127,7 @@ expr_constant_set_create_integers(const char
> > *const *values, size_t n_values)
> > >>
> > >>  static void
> > >>  expr_constant_set_add_value(struct expr_constant_set **p_cs,
> > >> -                            union expr_constant *c, size_t
*allocated)
> > >> +                            struct expr_constant *c, size_t
*allocated)
> > >>  {
> > >>      struct expr_constant_set *cs = *p_cs;
> > >>      if (!cs) {
> > >> @@ -1246,7 +1238,7 @@ expr_const_sets_add_strings(struct shash
> > *const_sets, const char *name,
> > >>                          values[i], name);
> > >>              continue;
> > >>          }
> > >> -        union expr_constant *c = &cs->values[cs->n_values++];
> > >> +        struct expr_constant *c = &cs->values[cs->n_values++];
> > >>          c->string = xstrdup(values[i]);
> > >>      }
> > >>
> > >> @@ -1359,7 +1351,7 @@ expr_parse_primary(struct expr_context *ctx,
bool
> > *atomic)
> > >>
> > >>              *atomic = true;
> > >>
> > >> -            union expr_constant *cst = xzalloc(sizeof *cst);
> > >> +            struct expr_constant *cst = xzalloc(sizeof *cst);
> > >>              cst->format = LEX_F_HEXADECIMAL;
> > >>              cst->masked = false;
> > >>
> > >> @@ -1367,7 +1359,6 @@ expr_parse_primary(struct expr_context *ctx,
bool
> > *atomic)
> > >>              c.values = cst;
> > >>              c.n_values = 1;
> > >>              c.in_curlies = false;
> > >> -            c.as_name = NULL;
> > >>              return make_cmp(ctx, &f, EXPR_R_NE, &c);
> > >>          } else if (parse_relop(ctx, &r) && parse_constant_set(ctx,
> > &c)) {
> > >>              return make_cmp(ctx, &f, r, &c);
> > >> @@ -1834,7 +1825,6 @@ expr_symtab_destroy(struct shash *symtab)
> > >>  static struct expr *
> > >>  expr_clone_cmp(struct expr *expr)
> > >>  {
> > >> -    ovs_assert(!expr->as_name);
> > >>      struct expr *new = xmemdup(expr, sizeof *expr);
> > >>      if (!new->cmp.symbol->width) {
> > >>          new->cmp.string = xstrdup(new->cmp.string);
> > >> @@ -1858,7 +1848,6 @@ expr_clone_andor(struct expr *expr)
> > >>  static struct expr *
> > >>  expr_clone_condition(struct expr *expr)
> > >>  {
> > >> -    ovs_assert(!expr->as_name);
> > >>      struct expr *new = xmemdup(expr, sizeof *expr);
> > >>      new->cond.string = xstrdup(new->cond.string);
> > >>      return new;
> > >> @@ -1894,8 +1883,6 @@ expr_destroy(struct expr *expr)
> > >>          return;
> > >>      }
> > >>
> > >> -    free(expr->as_name);
> > >> -
> > >>      struct expr *sub;
> > >>
> > >>      switch (expr->type) {
> > >> @@ -2567,7 +2554,6 @@ crush_or_supersets(struct expr *expr, const
> > struct expr_symbol *symbol)
> > >>       * mask sizes. */
> > >>      size_t n = ovs_list_size(&expr->andor);
> > >>      struct expr **subs = xmalloc(n * sizeof *subs);
> > >> -    bool modified = false;
> > >>      /* Linked list over the 'subs' array to quickly skip deleted
> > elements,
> > >>       * i.e. the index of the next potentially non-NULL element. */
> > >>      size_t *next = xmalloc(n * sizeof *next);
> > >> @@ -2596,14 +2582,14 @@ crush_or_supersets(struct expr *expr, const
> > struct expr_symbol *symbol)
> > >>              next[last] = i;
> > >>              last = i;
> > >>          } else {
> > >> +            /* Remove address set reference from the duplicate. */
> > >> +            subs[last]->as_name = NULL;
> > >>              expr_destroy(subs[i]);
> > >>              subs[i] = NULL;
> > >> -            modified = true;
> > >>          }
> > >>      }
> > >>
> > >> -    if (!symbol->width || symbol->level != EXPR_L_ORDINAL
> > >> -        || (!modified && expr->as_name)) {
> > >> +    if (!symbol->width || symbol->level != EXPR_L_ORDINAL) {
> > >>          /* Not a fully maskable field or this expression is
tracking an
> > >>           * address set.  Don't try to optimize to preserve address
set
> > I-P. */
> >
> > The above comment needs to be updated since it obviously doesn't
preserve
> > address set I-P any more. What's the reason it is not considered any
more?
> >
>
> Because of the AS reference per sub expression we can actually allow this
> optimization to happen. In the case of the smaller mask we will lose track
> of only that address instead of the whole set. I'll update the comment.
>
The question is, is it worth losing track of those addresses for the
optimization? I think in most cases address sets shouldn't contain too many
overlapping entries. So the optimization may not make a big difference
anyway, but handling removal of such addresses that are not tracked will
cost much more. I don't have a strong opinion on this though, if
overlapping is rare.

>
> >
> > >>          goto done;
> > >> @@ -2658,10 +2644,11 @@ crush_or_supersets(struct expr *expr, const
> > struct expr_symbol *symbol)
> > >>              if (expr_bitmap_intersect_check(a_value, a_mask,
b_value,
> > b_mask,
> > >>                                              bit_width)
> > >>                  && bitmap_is_superset(b_mask, a_mask, bit_width)) {
> > >> -                /* 'a' is the same expression with a smaller mask.
*/
> > >> +                /* 'a' is the same expression with a smaller mask.
> > >> +                 * Remove address set reference from the duplicate.
*/
> > >> +                a->as_name = NULL;
> > >>                  expr_destroy(subs[j]);
> > >>                  subs[j] = NULL;
> > >> -                modified = true;
> > >>
> > >>                  /* Shorten the path for the next round. */
> > >>                  if (last) {
> > >> @@ -2685,12 +2672,6 @@ done:
> > >>          }
> > >>      }
> > >>
> > >> -    if (modified) {
> > >> -        /* Members modified, so untrack address set. */
> > >> -        free(expr->as_name);
> > >> -        expr->as_name = NULL;
> > >> -    }
> > >> -
> > >>      free(next);
> > >>      free(subs);
> > >>      return expr;
> > >> @@ -3271,10 +3252,10 @@ add_disjunction(const struct expr *or,
> > >>      LIST_FOR_EACH (sub, node, &or->andor) {
> > >>          struct expr_match *match = expr_match_new(m, clause,
n_clauses,
> > >>                                                    conj_id);
> > >> -        if (or->as_name) {
> > >> +        if (sub->as_name) {
> > >>              ovs_assert(sub->type == EXPR_T_CMP);
> > >>              ovs_assert(sub->cmp.symbol->width);
> > >> -            match->as_name = xstrdup(or->as_name);
> > >> +            match->as_name = xstrdup(sub->as_name);
> > >>              match->as_ip = sub->cmp.value.ipv6;
> > >>              match->as_mask = sub->cmp.mask.ipv6;
> > >>          }
> > >> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> > >> index fdcc5aab2..626dd34f6 100644
> > >> --- a/tests/ovn-controller.at
> > >> +++ b/tests/ovn-controller.at
> > >> @@ -1630,7 +1630,9 @@
> > priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.8
> > actions=lo
> > >>  done
> > >>
> > >>  reprocess_count_new=$(read_counter consider_logical_flow)
> > >> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))],
[0],
> > [10
> > >> +# First 2 reprocess are due to change from 1 IP in AS to 2.
> > >> +# Last 5 is due to overlap in IP addresses between as1 and as2.
> > >> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))],
[0],
> > [7
> > >>  ])
> > >>
> > >>  # Remove the IPs from as1 and as2, 1 IP each time.
> > >> @@ -1653,9 +1655,9 @@ for i in $(seq 10); do
> > >>  done
> > >>
> > >>  reprocess_count_new=$(read_counter consider_logical_flow)
> > >> -# In this case the as1 and as2 are merged to a single OR expr, so we
> > lose track of
> > >> -# address set information - can't handle deletion incrementally.
> > >> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))],
[0],
> > [10
> > >> +# First 5 are due to overlap in IP addresses between as1 and as2.
> > >> +# Last 2 are due to change from 2 IP in AS to 1.
> > >> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))],
[0],
> > [7
> > >>  ])
> > >>
> > >>  OVN_CLEANUP([hv1])
> > >> @@ -1822,7 +1824,7 @@
> > priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.5
> > actions=co
> > >>
> >
 priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.10.10.10
> > actions=conjunction,2/2)
> > >>  ])
> > >>  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
> > >>  ])
> > >>
> > >>  # Delete 2 IPs
> > >> @@ -1842,7 +1844,7 @@
> > priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3
> > actions=co
> > >>
> >
 priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.10.10.10
> > actions=conjunction,2/2)
> > >>  ])
> > >>  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])
> > >> --
> > >> 2.44.0
> > >>
> > >
> > > Please consider it as RFC, I forgot to add the tag into subject.
> >
> > Is there a reason why this is RFC? Do you have any TODOs in mind?
> >
>
> Mainly to be sure if this approach is heading the right direction, which
it
> seems it does. There shouldn't be any additional TODO right now.
>
I see. I will take a closer look at this patch and see if I have any more
findings before you send v2.

Thanks,
Han

>
> > I am just not 100% confident about corner cases. The existing test cases
> > seem to be handled correctly. We might need to think about if there are
any
> > other potentially risky scenarios that need to be tested, although I
don't
> > see any obvious problem for now.
> >
>
>
> So things that came to my mind are covered by the tests, let's see if
> others will think about potentially dangerous scenarios.
>
>
> > Thanks,
> > Han
> >
> > > --
> > >
> > > Ales Musil
> > >
> > > Senior Software Engineer - OVN Core
> > >
> > > Red Hat EMEA
> > >
> > > amusil@redhat.com
> >
>
>
> I'll wait a bit for feedback from others before posting v2.
>
>
> Thanks,
> Ales
> --
>
> Ales Musil
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA <https://www.redhat.com>
>
> amusil@redhat.com
> <https://red.ht/sig>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On 3/27/24 08:45, Ales Musil wrote:
> On Wed, Mar 27, 2024 at 7:14 AM Han Zhou <hzhou@ovn.org> wrote:
> 
>>
>>
>> On Tue, Mar 19, 2024 at 9:45 AM Ales Musil <amusil@redhat.com> wrote:
>>>
>>>
>>>
>>> On Tue, Mar 19, 2024 at 5:43 PM Ales Musil <amusil@redhat.com> wrote:
>>>>
>>>> Instead of tracking address set per struct expr_constant_set track it
>>>> per individual struct expr_constant. This allows more fine grained
>>>> control for I-P processing of address sets in controller. It helps with
>>>> scenarios like matching on two address sets in one expression e.g.
>>>> "ip4.src == {$as1, $as2}". This allows any addition or removal of
>>>> individual adress from the set to be incrementally processed instead
>>>> of reprocessing all the flows.
>>>>
>>>> This unfortunately doesn't help with the following flows:
>>>> "ip4.src == $as1 && ip4.dst == $as2"
>>>> "ip4.src == $as1 || ip4.dst == $as2"
>>>>
>>>> The memory impact should be minimal as there is only increase of 8 bytes
>>>> per the struct expr_constant.
>>>>
>>>> Signed-off-by: Ales Musil <amusil@redhat.com>
>>
>> Thanks Ales for the improvement! The approach looks good to me. Please see
>> some comments below:
>>
>>
>>
> Hi Han,
> 
> thank you for the review.
> 
> 
>>
>>>> ---
>>>>  controller/lflow.c      |  4 +-
>>>>  include/ovn/actions.h   |  2 +-
>>>>  include/ovn/expr.h      | 46 ++++++++++----------
>>>>  lib/actions.c           | 20 ++++-----
>>>>  lib/expr.c              | 95 +++++++++++++++++------------------------
>>>>  tests/ovn-controller.at | 14 +++---
>>>>  6 files changed, 83 insertions(+), 98 deletions(-)
>>>>
>>>> diff --git a/controller/lflow.c b/controller/lflow.c
>>>> index 895d17d19..730dc879d 100644
>>>> --- a/controller/lflow.c
>>>> +++ b/controller/lflow.c
>>>> @@ -278,7 +278,7 @@ lflow_handle_changed_flows(struct lflow_ctx_in
>> *l_ctx_in,
>>>>  }
>>>>
>>>>  static bool
>>>> -as_info_from_expr_const(const char *as_name, const union expr_constant
>> *c,
>>>> +as_info_from_expr_const(const char *as_name, const struct
>> expr_constant *c,
>>>>                          struct addrset_info *as_info)
>>>>  {
>>>>      as_info->name = as_name;
>>>> @@ -714,7 +714,7 @@ lflow_handle_addr_set_update(const char *as_name,
>>>>          if (as_diff->deleted) {
>>>>              struct addrset_info as_info;
>>>>              for (size_t i = 0; i < as_diff->deleted->n_values; i++) {
>>>> -                union expr_constant *c = &as_diff->deleted->values[i];
>>>> +                struct expr_constant *c = &as_diff->deleted->values[i];
>>>>                  if (!as_info_from_expr_const(as_name, c, &as_info)) {
>>>>                      continue;
>>>>                  }
>>>> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
>>>> index dcacbb1ff..1e20f9b81 100644
>>>> --- a/include/ovn/actions.h
>>>> +++ b/include/ovn/actions.h
>>>> @@ -238,7 +238,7 @@ struct ovnact_next {
>>>>  struct ovnact_load {
>>>>      struct ovnact ovnact;
>>>>      struct expr_field dst;
>>>> -    union expr_constant imm;
>>>> +    struct expr_constant imm;
>>>>  };
>>>>
>>>>  /* OVNACT_MOVE, OVNACT_EXCHANGE. */
>>>> diff --git a/include/ovn/expr.h b/include/ovn/expr.h
>>>> index c48f82398..e54edb5bf 100644
>>>> --- a/include/ovn/expr.h
>>>> +++ b/include/ovn/expr.h
>>>> @@ -368,7 +368,7 @@ bool expr_relop_from_token(enum lex_type type, enum
>> expr_relop *relop);
>>>>  struct expr {
>>>>      struct ovs_list node;       /* In parent EXPR_T_AND or EXPR_T_OR
>> if any. */
>>>>      enum expr_type type;        /* Expression type. */
>>>> -    char *as_name;              /* Address set name. Null if it is not
>> an
>>>> +    const char *as_name;        /* Address set name. Null if it is not
>> an
>>>>                                     address set. */
>>>>
>>>>      union {
>>>> @@ -505,40 +505,42 @@ enum expr_constant_type {
>>>>  };
>>>>
>>>>  /* A string or integer constant (one must know which from context). */
>>>> -union expr_constant {
>>>> -    /* Integer constant.
>>>> -     *
>>>> -     * The width of a constant isn't always clear, e.g. if you write
>> "1",
>>>> -     * there's no way to tell whether you mean for that to be a 1-bit
>> constant
>>>> -     * or a 128-bit constant or somewhere in between. */
>>>> -    struct {
>>>> -        union mf_subvalue value;
>>>> -        union mf_subvalue mask; /* Only initialized if 'masked'. */
>>>> -        bool masked;
>>>> -
>>>> -        enum lex_format format; /* From the constant's lex_token. */
>>>> -    };
>>>> +struct expr_constant {
>>>> +    const char *as_name;
>>>>
>>>> -    /* Null-terminated string constant. */
>>>> -    char *string;
>>>> +    union {
>>>> +        /* Integer constant.
>>>> +         *
>>>> +         * The width of a constant isn't always clear, e.g. if you
>> write "1",
>>>> +         * there's no way to tell whether you mean for that to be a
>> 1-bit
>>>> +         * constant or a 128-bit constant or somewhere in between. */
>>>> +        struct {
>>>> +            union mf_subvalue value;
>>>> +            union mf_subvalue mask; /* Only initialized if 'masked'. */
>>>> +            bool masked;
>>>> +
>>>> +            enum lex_format format; /* From the constant's lex_token.
>> */
>>>> +        };
>>>> +
>>>> +        /* Null-terminated string constant. */
>>>> +        char *string;
>>>> +    };
>>>>  };
>>>>
>>>>  bool expr_constant_parse(struct lexer *,
>>>>                           const struct expr_field *,
>>>> -                         union expr_constant *);
>>>> -void expr_constant_format(const union expr_constant *,
>>>> +                         struct expr_constant *);
>>>> +void expr_constant_format(const struct expr_constant *,
>>>>                            enum expr_constant_type, struct ds *);
>>>> -void expr_constant_destroy(const union expr_constant *,
>>>> +void expr_constant_destroy(const struct expr_constant *,
>>>>                             enum expr_constant_type);
>>>>
>>>>  /* A collection of "union expr_constant"s of the same type. */
>>>>  struct expr_constant_set {
>>>> -    union expr_constant *values;  /* Constants. */
>>>> +    struct expr_constant *values; /* Constants. */
>>>>      size_t n_values;              /* Number of constants. */
>>>>      enum expr_constant_type type; /* Type of the constants. */
>>>>      bool in_curlies;              /* Whether the constants were in {}.
>> */
>>>> -    char *as_name;                /* Name of an address set. It is
>> NULL if not
>>>> -                                     an address set. */
>>>>  };
>>>>
>>>>  bool expr_constant_set_parse(struct lexer *, struct expr_constant_set
>> *);
>>>> diff --git a/lib/actions.c b/lib/actions.c
>>>> index 71615fc53..6574c4df4 100644
>>>> --- a/lib/actions.c
>>>> +++ b/lib/actions.c
>>>> @@ -450,7 +450,7 @@ encode_LOAD(const struct ovnact_load *load,
>>>>              const struct ovnact_encode_params *ep,
>>>>              struct ofpbuf *ofpacts)
>>>>  {
>>>> -    const union expr_constant *c = &load->imm;
>>>> +    const struct expr_constant *c = &load->imm;
>>>>      struct mf_subfield dst = expr_resolve_field(&load->dst);
>>>>      struct ofpact_set_field *sf = ofpact_put_set_field(ofpacts,
>> dst.field,
>>>>                                                         NULL, NULL);
>>>> @@ -2122,7 +2122,7 @@ encode_event_empty_lb_backends_opts(struct ofpbuf
>> *ofpacts,
>>>>          /* All empty_lb_backends fields are of type 'str' */
>>>>          ovs_assert(!strcmp(o->option->type, "str"));
>>>>
>>>> -        const union expr_constant *c = o->value.values;
>>>> +        const struct expr_constant *c = o->value.values;
>>>>          size_t size = strlen(c->string);
>>>>          struct controller_event_opt_header hdr =
>>>>              (struct controller_event_opt_header) {
>>>> @@ -2598,7 +2598,7 @@ validate_empty_lb_backends(struct action_context
>> *ctx,
>>>>  {
>>>>      for (size_t i = 0; i < n_options; i++) {
>>>>          const struct ovnact_gen_option *o = &options[i];
>>>> -        const union expr_constant *c = o->value.values;
>>>> +        const struct expr_constant *c = o->value.values;
>>>>          struct sockaddr_storage ss;
>>>>          struct uuid uuid;
>>>>
>>>> @@ -2801,7 +2801,7 @@ encode_put_dhcpv4_option(const struct
>> ovnact_gen_option *o,
>>>>      uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2);
>>>>      opt_header[0] = o->option->code;
>>>>
>>>> -    const union expr_constant *c = o->value.values;
>>>> +    const struct expr_constant *c = o->value.values;
>>>>      size_t n_values = o->value.n_values;
>>>>      if (!strcmp(o->option->type, "bool") ||
>>>>          !strcmp(o->option->type, "uint8")) {
>>>> @@ -2967,7 +2967,7 @@ encode_put_dhcpv6_option(const struct
>> ovnact_gen_option *o,
>>>>                           struct ofpbuf *ofpacts)
>>>>  {
>>>>      struct dhcpv6_opt_header *opt = ofpbuf_put_uninit(ofpacts, sizeof
>> *opt);
>>>> -    const union expr_constant *c = o->value.values;
>>>> +    const struct expr_constant *c = o->value.values;
>>>>      size_t n_values = o->value.n_values;
>>>>      size_t size;
>>>>
>>>> @@ -3023,7 +3023,7 @@ encode_PUT_DHCPV4_OPTS(const struct
>> ovnact_put_opts *pdo,
>>>>          find_opt(pdo->options, pdo->n_options, DHCP_OPT_BOOTFILE_CODE);
>>>>      if (boot_opt) {
>>>>          uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2);
>>>> -        const union expr_constant *c = boot_opt->value.values;
>>>> +        const struct expr_constant *c = boot_opt->value.values;
>>>>          opt_header[0] = boot_opt->option->code;
>>>>          opt_header[1] = strlen(c->string);
>>>>          ofpbuf_put(ofpacts, c->string, opt_header[1]);
>>>> @@ -3033,7 +3033,7 @@ encode_PUT_DHCPV4_OPTS(const struct
>> ovnact_put_opts *pdo,
>>>>          find_opt(pdo->options, pdo->n_options,
>> DHCP_OPT_BOOTFILE_ALT_CODE);
>>>>      if (boot_alt_opt) {
>>>>          uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2);
>>>> -        const union expr_constant *c = boot_alt_opt->value.values;
>>>> +        const struct expr_constant *c = boot_alt_opt->value.values;
>>>>          opt_header[0] = boot_alt_opt->option->code;
>>>>          opt_header[1] = strlen(c->string);
>>>>          ofpbuf_put(ofpacts, c->string, opt_header[1]);
>>>> @@ -3043,7 +3043,7 @@ encode_PUT_DHCPV4_OPTS(const struct
>> ovnact_put_opts *pdo,
>>>>          pdo->options, pdo->n_options, DHCP_OPT_NEXT_SERVER_CODE);
>>>>      if (next_server_opt) {
>>>>          uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2);
>>>> -        const union expr_constant *c = next_server_opt->value.values;
>>>> +        const struct expr_constant *c = next_server_opt->value.values;
>>>>          opt_header[0] = next_server_opt->option->code;
>>>>          opt_header[1] = sizeof(ovs_be32);
>>>>          ofpbuf_put(ofpacts, &c->value.ipv4, sizeof(ovs_be32));
>>>> @@ -3247,7 +3247,7 @@ parse_put_nd_ra_opts(struct action_context *ctx,
>> const struct expr_field *dst,
>>>>      /* Let's validate the options. */
>>>>      for (size_t i = 0; i < po->n_options; i++) {
>>>>          const struct ovnact_gen_option *o = &po->options[i];
>>>> -        const union expr_constant *c = o->value.values;
>>>> +        const struct expr_constant *c = o->value.values;
>>>>          if (o->value.n_values > 1) {
>>>>              lexer_error(ctx->lexer, "Invalid value for \"%s\" option",
>>>>                          o->option->name);
>>>> @@ -3430,7 +3430,7 @@ static void
>>>>  encode_put_nd_ra_option(const struct ovnact_gen_option *o,
>>>>                          struct ofpbuf *ofpacts, ptrdiff_t ra_offset)
>>>>  {
>>>> -    const union expr_constant *c = o->value.values;
>>>> +    const struct expr_constant *c = o->value.values;
>>>>
>>>>      switch (o->option->code) {
>>>>      case ND_RA_FLAG_ADDR_MODE:
>>>> diff --git a/lib/expr.c b/lib/expr.c
>>>> index 0cb44e1b6..286782025 100644
>>>> --- a/lib/expr.c
>>>> +++ b/lib/expr.c
>>>> @@ -179,12 +179,10 @@ expr_combine(enum expr_type type, struct expr *a,
>> struct expr *b)
>>>>          } else {
>>>>              ovs_list_push_back(&a->andor, &b->node);
>>>>          }
>>>> -        free(a->as_name);
>>>>          a->as_name = NULL;
>>>>          return a;
>>>>      } else if (b->type == type) {
>>>>          ovs_list_push_front(&b->andor, &a->node);
>>>> -        free(b->as_name);
>>>>          b->as_name = NULL;
>>>>          return b;
>>>>      } else {
>>>> @@ -521,12 +519,13 @@ static bool parse_field(struct expr_context *,
>> struct expr_field *);
>>>>
>>>>  static struct expr *
>>>>  make_cmp__(const struct expr_field *f, enum expr_relop r,
>>>> -             const union expr_constant *c)
>>>> +             const struct expr_constant *c)
>>>>  {
>>>>      struct expr *e = xzalloc(sizeof *e);
>>>>      e->type = EXPR_T_CMP;
>>>>      e->cmp.symbol = f->symbol;
>>>>      e->cmp.relop = r;
>>>> +    e->as_name = c->as_name;
>>>>      if (f->symbol->width) {
>>>>          bitwise_copy(&c->value, sizeof c->value, 0,
>>>>                       &e->cmp.value, sizeof e->cmp.value, f->ofs,
>>>> @@ -547,7 +546,7 @@ make_cmp__(const struct expr_field *f, enum
>> expr_relop r,
>>>>
>>>>  /* Returns the minimum reasonable width for integer constant 'c'. */
>>>>  static int
>>>> -expr_constant_width(const union expr_constant *c)
>>>> +expr_constant_width(const struct expr_constant *c)
>>>>  {
>>>>      if (c->masked) {
>>>>          return mf_subvalue_width(&c->mask);
>>>> @@ -674,10 +673,7 @@ make_cmp(struct expr_context *ctx,
>>>>          e = expr_combine(r == EXPR_R_EQ ? EXPR_T_OR : EXPR_T_AND,
>>>>                           e, make_cmp__(f, r, &cs->values[i]));
>>>>      }
>>>> -    /* Track address set */
>>>> -    if (r == EXPR_R_EQ && e->type == EXPR_T_OR && cs->as_name) {
>>>> -        e->as_name = xstrdup(cs->as_name);
>>>> -    }
>>>> +
>>>>  exit:
>>>>      expr_constant_set_destroy(cs);
>>>>      return e;
>>>> @@ -797,11 +793,10 @@ parse_addr_sets(struct expr_context *ctx, struct
>> expr_constant_set *cs,
>>>>          }
>>>>      }
>>>>
>>>> -    struct expr_constant_set *addr_sets
>>>> -        = (ctx->addr_sets
>>>> -           ? shash_find_data(ctx->addr_sets, ctx->lexer->token.s)
>>>> -           : NULL);
>>>> -    if (!addr_sets) {
>>>> +    struct shash_node *node = ctx->addr_sets
>>>> +                              ? shash_find(ctx->addr_sets,
>> ctx->lexer->token.s)
>>>> +                              : NULL;
>>>> +    if (!node) {
>>>>          lexer_syntax_error(ctx->lexer, "expecting address set name");
>>>>          return false;
>>>>      }
>>>> @@ -810,17 +805,16 @@ parse_addr_sets(struct expr_context *ctx, struct
>> expr_constant_set *cs,
>>>>          return false;
>>>>      }
>>>>
>>>> -    if (!cs->n_values) {
>>>> -        cs->as_name = xstrdup(ctx->lexer->token.s);
>>>> -    }
>>>> -
>>>> +    struct expr_constant_set *addr_sets = node->data;
>>>>      size_t n_values = cs->n_values + addr_sets->n_values;
>>>>      if (n_values >= *allocated_values) {
>>>>          cs->values = xrealloc(cs->values, n_values * sizeof
>> *cs->values);
>>>>          *allocated_values = n_values;
>>>>      }
>>>>      for (size_t i = 0; i < addr_sets->n_values; i++) {
>>>> -        cs->values[cs->n_values++] = addr_sets->values[i];
>>>> +        struct expr_constant *c = &cs->values[cs->n_values++];
>>>> +        *c = addr_sets->values[i];
>>>> +        c->as_name = node->name;
>>
>> I am not sure if it is safe enough to just store the reference. We need to
>> check carefully if there is any chance the pointer becomes dangling when
>> the corresponding address set is destroyed or in any other circumstances.
>>
> 
> Currently it is safe to store as it is only for the duration of the whole
> expression, in the current code base the set outlives the expression.
> However it might be a problem later on, I don't know how viable it would be
> in terms of memory to store clone.
> 
> 
>>
>>>>      }
>>>>
>>>>      return true;
>>>> @@ -859,8 +853,9 @@ parse_port_group(struct expr_context *ctx, struct
>> expr_constant_set *cs,
>>>>          *allocated_values = n_values;
>>>>      }
>>>>      for (size_t i = 0; i < port_group->n_values; i++) {
>>>> -        cs->values[cs->n_values++].string =
>>>> -            xstrdup(port_group->values[i].string);
>>>> +        struct expr_constant *c = &cs->values[cs->n_values++];
>>>> +        c->string = xstrdup(port_group->values[i].string);
>>>> +        c->as_name = NULL;
>>>>      }
>>>>
>>>>      return true;
>>>> @@ -875,11 +870,6 @@ parse_constant(struct expr_context *ctx, struct
>> expr_constant_set *cs,
>>>>                                  sizeof *cs->values);
>>>>      }
>>>>
>>>> -    /* Combining other values to the constant set that is tracking an
>>>> -     * address set, so untrack it. */
>>>> -    free(cs->as_name);
>>>> -    cs->as_name = NULL;
>>>> -
>>>>      if (ctx->lexer->token.type == LEX_T_TEMPLATE) {
>>>>          lexer_error(ctx->lexer, "Unexpanded template.");
>>>>          return false;
>>>> @@ -887,7 +877,9 @@ parse_constant(struct expr_context *ctx, struct
>> expr_constant_set *cs,
>>>>          if (!assign_constant_set_type(ctx, cs, EXPR_C_STRING)) {
>>>>              return false;
>>>>          }
>>>> -        cs->values[cs->n_values++].string =
>> xstrdup(ctx->lexer->token.s);
>>>> +        struct expr_constant *c = &cs->values[cs->n_values++];
>>>> +        c->string = xstrdup(ctx->lexer->token.s);
>>>> +        c->as_name = NULL;
>>>>          lexer_get(ctx->lexer);
>>>>          return true;
>>>>      } else if (ctx->lexer->token.type == LEX_T_INTEGER ||
>>>> @@ -896,13 +888,14 @@ parse_constant(struct expr_context *ctx, struct
>> expr_constant_set *cs,
>>>>              return false;
>>>>          }
>>>>
>>>> -        union expr_constant *c = &cs->values[cs->n_values++];
>>>> +        struct expr_constant *c = &cs->values[cs->n_values++];
>>>>          c->value = ctx->lexer->token.value;
>>>>          c->format = ctx->lexer->token.format;
>>>>          c->masked = ctx->lexer->token.type == LEX_T_MASKED_INTEGER;
>>>>          if (c->masked) {
>>>>              c->mask = ctx->lexer->token.mask;
>>>>          }
>>>> +        c->as_name = NULL;
>>>>          lexer_get(ctx->lexer);
>>>>          return true;
>>>>      } else if (ctx->lexer->token.type == LEX_T_MACRO) {
>>>> @@ -961,7 +954,7 @@ parse_constant_set(struct expr_context *ctx, struct
>> expr_constant_set *cs)
>>>>   * indeterminate. */
>>>>  bool
>>>>  expr_constant_parse(struct lexer *lexer, const struct expr_field *f,
>>>> -                    union expr_constant *c)
>>>> +                    struct expr_constant *c)
>>>>  {
>>>>      if (lexer->error) {
>>>>          return false;
>>>> @@ -987,7 +980,7 @@ expr_constant_parse(struct lexer *lexer, const
>> struct expr_field *f,
>>>>  /* Appends to 's' a re-parseable representation of constant 'c' with
>> the given
>>>>   * 'type'. */
>>>>  void
>>>> -expr_constant_format(const union expr_constant *c,
>>>> +expr_constant_format(const struct expr_constant *c,
>>>>                       enum expr_constant_type type, struct ds *s)
>>>>  {
>>>>      if (type == EXPR_C_STRING) {
>>>> @@ -1010,7 +1003,7 @@ expr_constant_format(const union expr_constant *c,
>>>>   *
>>>>   * Does not free(c). */
>>>>  void
>>>> -expr_constant_destroy(const union expr_constant *c,
>>>> +expr_constant_destroy(const struct expr_constant *c,
>>>>                        enum expr_constant_type type)
>>>>  {
>>>>      if (c && type == EXPR_C_STRING) {
>>>> @@ -1043,7 +1036,7 @@ expr_constant_set_format(const struct
>> expr_constant_set *cs, struct ds *s)
>>>>          ds_put_char(s, '{');
>>>>      }
>>>>
>>>> -    for (const union expr_constant *c = cs->values;
>>>> +    for (const struct expr_constant *c = cs->values;
>>>>           c < &cs->values[cs->n_values]; c++) {
>>>>          if (c != cs->values) {
>>>>              ds_put_cstr(s, ", ");
>>>> @@ -1067,15 +1060,14 @@ expr_constant_set_destroy(struct
>> expr_constant_set *cs)
>>>>              }
>>>>          }
>>>>          free(cs->values);
>>>> -        free(cs->as_name);
>>>>      }
>>>>  }
>>>>
>>>>  static int
>>>>  compare_expr_constant_integer_cb(const void *a_, const void *b_)
>>>>  {
>>>> -    const union expr_constant *a = a_;
>>>> -    const union expr_constant *b = b_;
>>>> +    const struct expr_constant *a = a_;
>>>> +    const struct expr_constant *b = b_;
>>>>
>>>>      int d = memcmp(&a->value, &b->value, sizeof a->value);
>>>>      if (d) {
>>>> @@ -1114,7 +1106,7 @@ expr_constant_set_create_integers(const char
>> *const *values, size_t n_values)
>>>>              VLOG_WARN("Invalid constant set entry: '%s', token type:
>> %d",
>>>>                        values[i], lex.token.type);
>>>>          } else {
>>>> -            union expr_constant *c = &cs->values[cs->n_values++];
>>>> +            struct expr_constant *c = &cs->values[cs->n_values++];
>>>>              c->value = lex.token.value;
>>>>              c->format = lex.token.format;
>>>>              c->masked = lex.token.type == LEX_T_MASKED_INTEGER;
>>>> @@ -1135,7 +1127,7 @@ expr_constant_set_create_integers(const char
>> *const *values, size_t n_values)
>>>>
>>>>  static void
>>>>  expr_constant_set_add_value(struct expr_constant_set **p_cs,
>>>> -                            union expr_constant *c, size_t *allocated)
>>>> +                            struct expr_constant *c, size_t *allocated)
>>>>  {
>>>>      struct expr_constant_set *cs = *p_cs;
>>>>      if (!cs) {
>>>> @@ -1246,7 +1238,7 @@ expr_const_sets_add_strings(struct shash
>> *const_sets, const char *name,
>>>>                          values[i], name);
>>>>              continue;
>>>>          }
>>>> -        union expr_constant *c = &cs->values[cs->n_values++];
>>>> +        struct expr_constant *c = &cs->values[cs->n_values++];
>>>>          c->string = xstrdup(values[i]);
>>>>      }
>>>>
>>>> @@ -1359,7 +1351,7 @@ expr_parse_primary(struct expr_context *ctx, bool
>> *atomic)
>>>>
>>>>              *atomic = true;
>>>>
>>>> -            union expr_constant *cst = xzalloc(sizeof *cst);
>>>> +            struct expr_constant *cst = xzalloc(sizeof *cst);
>>>>              cst->format = LEX_F_HEXADECIMAL;
>>>>              cst->masked = false;
>>>>
>>>> @@ -1367,7 +1359,6 @@ expr_parse_primary(struct expr_context *ctx, bool
>> *atomic)
>>>>              c.values = cst;
>>>>              c.n_values = 1;
>>>>              c.in_curlies = false;
>>>> -            c.as_name = NULL;
>>>>              return make_cmp(ctx, &f, EXPR_R_NE, &c);
>>>>          } else if (parse_relop(ctx, &r) && parse_constant_set(ctx,
>> &c)) {
>>>>              return make_cmp(ctx, &f, r, &c);
>>>> @@ -1834,7 +1825,6 @@ expr_symtab_destroy(struct shash *symtab)
>>>>  static struct expr *
>>>>  expr_clone_cmp(struct expr *expr)
>>>>  {
>>>> -    ovs_assert(!expr->as_name);
>>>>      struct expr *new = xmemdup(expr, sizeof *expr);
>>>>      if (!new->cmp.symbol->width) {
>>>>          new->cmp.string = xstrdup(new->cmp.string);
>>>> @@ -1858,7 +1848,6 @@ expr_clone_andor(struct expr *expr)
>>>>  static struct expr *
>>>>  expr_clone_condition(struct expr *expr)
>>>>  {
>>>> -    ovs_assert(!expr->as_name);
>>>>      struct expr *new = xmemdup(expr, sizeof *expr);
>>>>      new->cond.string = xstrdup(new->cond.string);
>>>>      return new;
>>>> @@ -1894,8 +1883,6 @@ expr_destroy(struct expr *expr)
>>>>          return;
>>>>      }
>>>>
>>>> -    free(expr->as_name);
>>>> -
>>>>      struct expr *sub;
>>>>
>>>>      switch (expr->type) {
>>>> @@ -2567,7 +2554,6 @@ crush_or_supersets(struct expr *expr, const
>> struct expr_symbol *symbol)
>>>>       * mask sizes. */
>>>>      size_t n = ovs_list_size(&expr->andor);
>>>>      struct expr **subs = xmalloc(n * sizeof *subs);
>>>> -    bool modified = false;
>>>>      /* Linked list over the 'subs' array to quickly skip deleted
>> elements,
>>>>       * i.e. the index of the next potentially non-NULL element. */
>>>>      size_t *next = xmalloc(n * sizeof *next);
>>>> @@ -2596,14 +2582,14 @@ crush_or_supersets(struct expr *expr, const
>> struct expr_symbol *symbol)
>>>>              next[last] = i;
>>>>              last = i;
>>>>          } else {
>>>> +            /* Remove address set reference from the duplicate. */
>>>> +            subs[last]->as_name = NULL;
>>>>              expr_destroy(subs[i]);
>>>>              subs[i] = NULL;
>>>> -            modified = true;
>>>>          }
>>>>      }
>>>>
>>>> -    if (!symbol->width || symbol->level != EXPR_L_ORDINAL
>>>> -        || (!modified && expr->as_name)) {
>>>> +    if (!symbol->width || symbol->level != EXPR_L_ORDINAL) {
>>>>          /* Not a fully maskable field or this expression is tracking an
>>>>           * address set.  Don't try to optimize to preserve address set
>> I-P. */
>>
>> The above comment needs to be updated since it obviously doesn't preserve
>> address set I-P any more. What's the reason it is not considered any more?
>>
> 
> Because of the AS reference per sub expression we can actually allow this
> optimization to happen. In the case of the smaller mask we will lose track
> of only that address instead of the whole set. I'll update the comment.
> 
> 
>>
>>>>          goto done;
>>>> @@ -2658,10 +2644,11 @@ crush_or_supersets(struct expr *expr, const
>> struct expr_symbol *symbol)
>>>>              if (expr_bitmap_intersect_check(a_value, a_mask, b_value,
>> b_mask,
>>>>                                              bit_width)
>>>>                  && bitmap_is_superset(b_mask, a_mask, bit_width)) {
>>>> -                /* 'a' is the same expression with a smaller mask. */
>>>> +                /* 'a' is the same expression with a smaller mask.
>>>> +                 * Remove address set reference from the duplicate. */
>>>> +                a->as_name = NULL;
>>>>                  expr_destroy(subs[j]);
>>>>                  subs[j] = NULL;
>>>> -                modified = true;
>>>>
>>>>                  /* Shorten the path for the next round. */
>>>>                  if (last) {
>>>> @@ -2685,12 +2672,6 @@ done:
>>>>          }
>>>>      }
>>>>
>>>> -    if (modified) {
>>>> -        /* Members modified, so untrack address set. */
>>>> -        free(expr->as_name);
>>>> -        expr->as_name = NULL;
>>>> -    }
>>>> -
>>>>      free(next);
>>>>      free(subs);
>>>>      return expr;
>>>> @@ -3271,10 +3252,10 @@ add_disjunction(const struct expr *or,
>>>>      LIST_FOR_EACH (sub, node, &or->andor) {
>>>>          struct expr_match *match = expr_match_new(m, clause, n_clauses,
>>>>                                                    conj_id);
>>>> -        if (or->as_name) {
>>>> +        if (sub->as_name) {
>>>>              ovs_assert(sub->type == EXPR_T_CMP);
>>>>              ovs_assert(sub->cmp.symbol->width);
>>>> -            match->as_name = xstrdup(or->as_name);
>>>> +            match->as_name = xstrdup(sub->as_name);
>>>>              match->as_ip = sub->cmp.value.ipv6;
>>>>              match->as_mask = sub->cmp.mask.ipv6;
>>>>          }
>>>> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
>>>> index fdcc5aab2..626dd34f6 100644
>>>> --- a/tests/ovn-controller.at
>>>> +++ b/tests/ovn-controller.at
>>>> @@ -1630,7 +1630,9 @@
>> priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.8
>> actions=lo
>>>>  done
>>>>
>>>>  reprocess_count_new=$(read_counter consider_logical_flow)
>>>> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
>> [10
>>>> +# First 2 reprocess are due to change from 1 IP in AS to 2.
>>>> +# Last 5 is due to overlap in IP addresses between as1 and as2.
>>>> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
>> [7
>>>>  ])
>>>>
>>>>  # Remove the IPs from as1 and as2, 1 IP each time.
>>>> @@ -1653,9 +1655,9 @@ for i in $(seq 10); do
>>>>  done
>>>>
>>>>  reprocess_count_new=$(read_counter consider_logical_flow)
>>>> -# In this case the as1 and as2 are merged to a single OR expr, so we
>> lose track of
>>>> -# address set information - can't handle deletion incrementally.
>>>> -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
>> [10
>>>> +# First 5 are due to overlap in IP addresses between as1 and as2.
>>>> +# Last 2 are due to change from 2 IP in AS to 1.
>>>> +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0],
>> [7
>>>>  ])
>>>>
>>>>  OVN_CLEANUP([hv1])
>>>> @@ -1822,7 +1824,7 @@
>> priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.5
>> actions=co
>>>>
>>  priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.10.10.10
>> actions=conjunction,2/2)
>>>>  ])
>>>>  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
>>>>  ])
>>>>
>>>>  # Delete 2 IPs
>>>> @@ -1842,7 +1844,7 @@
>> priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3
>> actions=co
>>>>
>>  priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.10.10.10
>> actions=conjunction,2/2)
>>>>  ])
>>>>  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])
>>>> --
>>>> 2.44.0
>>>>
>>>
>>> Please consider it as RFC, I forgot to add the tag into subject.
>>
>> Is there a reason why this is RFC? Do you have any TODOs in mind?
>>
> 
> Mainly to be sure if this approach is heading the right direction, which it
> seems it does. There shouldn't be any additional TODO right now.
> 
> 
>> I am just not 100% confident about corner cases. The existing test cases
>> seem to be handled correctly. We might need to think about if there are any
>> other potentially risky scenarios that need to be tested, although I don't
>> see any obvious problem for now.
>>
> 
> 
> So things that came to my mind are covered by the tests, let's see if
> others will think about potentially dangerous scenarios.
> 
> 
>> Thanks,
>> Han
>>
>>> --
>>>
>>> Ales Musil
>>>
>>> Senior Software Engineer - OVN Core
>>>
>>> Red Hat EMEA
>>>
>>> amusil@redhat.com
>>
> 
> 
> I'll wait a bit for feedback from others before posting v2.
> 

Overall this looks OK to me.  Looking forward to v2.

Regards,
Dumitru