On Thu, May 2, 2024 at 6:29 AM Ales Musil <[email protected]> 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.
>
> Reported-at: https://issues.redhat.com/browse/FDP-509
> Signed-off-by: Ales Musil <[email protected]>
> ---
> v4: Rebase on top of current main.
> Update the "lflow_handle_addr_set_update" comment according to
suggestion from Han.
Thanks Ales. I updated the commit message for the same, and applied to main
branch.
Regards,
Han
> v3: Rebase on top of current main.
> Address comments from Han:
> - Adjust the comment for "lflow_handle_addr_set_update" to include
remaning corner cases.
> - Make sure that the flows are consistent between I-P and recompute.
> v2: Rebase on top of current main.
> Adjust the comment for I-P optimization.
> ---
> controller/lflow.c | 11 ++---
> include/ovn/actions.h | 2 +-
> include/ovn/expr.h | 46 ++++++++++---------
> lib/actions.c | 20 ++++-----
> lib/expr.c | 99 +++++++++++++++++------------------------
> tests/ovn-controller.at | 79 +++++++++++++++++++++++++++++---
> 6 files changed, 154 insertions(+), 103 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 760ec0b41..1e05665a1 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;
> @@ -644,14 +644,11 @@ as_update_can_be_handled(const char *as_name,
struct addr_set_diff *as_diff,
> * generated.
> *
> * - The sub expression of the address set is combined with other
sub-
> - * expressions/constants, usually because of disjunctions between
> - * sub-expressions/constants, e.g.:
> + * expressions/constants on different fields, e.g.:
> *
> * ip.src == $as1 || ip.dst == $as2
> - * ip.src == {$as1, $as2}
> - * ip.src == {$as1, ip1}
> *
> - * All these could have been split into separate lflows.
> + * This could have been split into separate lflows.
> *
> * - Conjunctions overlapping between lflows, which can be caused by
> * overlapping address sets or same address set used by multiple
lflows
> @@ -714,7 +711,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 ae0864fdd..88cf4de79 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -241,7 +241,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 94751d059..cff4f9e3c 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);
> @@ -2077,7 +2077,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) {
> @@ -2553,7 +2553,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;
>
> @@ -2861,7 +2861,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")) {
> @@ -3027,7 +3027,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;
>
> @@ -3083,7 +3083,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]);
> @@ -3093,7 +3093,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]);
> @@ -3103,7 +3103,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));
> @@ -3307,7 +3307,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);
> @@ -3490,7 +3490,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..ecf8a6338 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,7 @@ 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;
> + bool has_addr_set = 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);
> @@ -2575,6 +2562,9 @@ crush_or_supersets(struct expr *expr, const struct
expr_symbol *symbol)
> size_t i = 0, j, max_n_bits = 0;
> struct expr *sub;
> LIST_FOR_EACH (sub, node, &expr->andor) {
> + if (sub->as_name) {
> + has_addr_set = true;
> + }
> if (symbol->width) {
> const unsigned long *sub_mask;
>
> @@ -2596,14 +2586,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 ||
has_addr_set) {
> /* 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 +2648,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 +2676,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 +3256,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 be198e00d..27cec2aec 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -1625,7 +1625,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.
> @@ -1648,9 +1650,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])
> @@ -1817,7 +1819,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
> @@ -1837,7 +1839,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])
> @@ -2906,3 +2908,68 @@ OVN_CLEANUP([hv1],[hv2])
>
> AT_CLEANUP
> ])
> +
> +AT_SETUP([ovn-controller - AS I-P and recompute consistency])
> +AT_KEYWORDS([as-i-p])
> +
> +ovn_start
> +
> +net_add n1
> +sim_add hv1
> +as hv1
> +check ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +check ovs-vsctl -- add-port br-int hv1-vif1 -- \
> + set interface hv1-vif1 external-ids:iface-id=ls1-lp1
> +
> +check ovn-nbctl ls-add ls1
> +
> +check ovn-nbctl lsp-add ls1 ls1-lp1 \
> +-- lsp-set-addresses ls1-lp1 "f0:00:00:00:00:01"
> +
> +wait_for_ports_up
> +ovn-appctl -t ovn-controller vlog/set file:dbg
> +
> +# Get the OF table numbers
> +acl_eval=$(ovn-debug lflow-stage-to-oftable ls_out_acl_eval)
> +acl_action=$(ovn-debug lflow-stage-to-oftable ls_out_acl_action)
> +
> +dp_key=$(printf "%x" $(fetch_column datapath tunnel_key
external_ids:name=ls1))
> +port_key=$(printf "%x" $(fetch_column port_binding tunnel_key
logical_port=ls1-lp1))
> +
> +ovn-nbctl create address_set name=as1
> +check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" &&
ip4.src == $as1' drop
> +check ovn-nbctl add address_set as1 addresses 10.0.0.0/24
> +check ovn-nbctl --wait=hv sync
> +
> +AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int
table=$acl_eval,reg15=0x$port_key | grep -v reply | awk '{print $7, $8}' |
sort], [0], [dnl
> +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.0/24
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action)
> +])
> +
> +check ovn-nbctl add address_set as1 addresses 10.0.0.1
> +check ovn-nbctl add address_set as1 addresses 10.0.0.2
> +check ovn-nbctl add address_set as1 addresses 10.0.0.3
> +check ovn-nbctl add address_set as1 addresses 10.0.0.4
> +check ovn-nbctl --wait=hv sync
> +
> +AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int
table=$acl_eval,reg15=0x$port_key | grep -v reply | awk '{print $7, $8}' |
sort], [0], [dnl
> +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.0/24
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action)
> +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.1
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action)
> +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.2
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action)
> +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.3
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action)
> +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.4
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action)
> +])
> +
> +
> +check ovn-appctl inc-engine/recompute
> +
> +AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int
table=$acl_eval,reg15=0x$port_key | grep -v reply | awk '{print $7, $8}' |
sort], [0], [dnl
> +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.0/24
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action)
> +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.1
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action)
> +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.2
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action)
> +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.3
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action)
> +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.4
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action)
> +])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> --
> 2.44.0
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
