On Thu, May 2, 2024 at 10:35 PM Ales Musil <amu...@redhat.com> wrote: > > On Thu, May 2, 2024 at 6:23 PM Han Zhou <hz...@ovn.org> wrote: > > > > > > > > On Thu, May 2, 2024 at 6:29 AM Ales Musil <amu...@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. > > > > > > Reported-at: https://issues.redhat.com/browse/FDP-509 > > > Signed-off-by: Ales Musil <amu...@redhat.com> > > > --- > > > 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 > > > > > > Thank you Han, > > I would like to start the discussion about the possibility of > backporting this change. The reason for the backport being that this > change might be beneficial to ovn-kubernetes users, as this was > actually observed in ovn-kubernetes with network policies. I know that > strictly speaking this is not a bug fix, because this limitation was > well documented, however I would like to hear the opinion of others.
I have no objections if this improvement is strongly required for released versions, even though it is not a bug. May I ask for which releases do you want this to be backported? Thanks, Han > > Thanks, > Ales > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA > > amu...@redhat.com > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev