| Message ID | 20240430165642.1230867-1-amusil@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v3] controller: Track individual address set constants. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Tue, Apr 30, 2024 at 9:56 AM 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. > > Reported-at: https://issues.redhat.com/browse/FDP-509 > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > 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 | 7 ++- > 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, 153 insertions(+), 100 deletions(-) > > diff --git a/controller/lflow.c b/controller/lflow.c > index 760ec0b41..06e839cbe 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; > @@ -647,9 +647,8 @@ as_update_can_be_handled(const char *as_name, struct addr_set_diff *as_diff, > * expressions/constants, usually because of disjunctions between > * sub-expressions/constants, e.g.: > * > + * ip.src == $as1 && ip.dst == $as2 > * ip.src == $as1 || ip.dst == $as2 > - * ip.src == {$as1, $as2} > - * ip.src == {$as1, ip1} > * > * All these could have been split into separate lflows. Hi Ales, thanks for v3. I checked again and wondered why you mentioned that "ip.src == $as1 && ip.dst == $as2" is not supported. This expression would generate conjunctions, which works with I-P before your change and still works. Did I miss anything? In addition, since the constraints are relaxed after your change, I'd also update the above comments a little more, something like: * - The sub expression of the address set is combined with other sub- * expressions/constants on different fields, e.g.: * * ip.src == $as1 || ip.dst == $as2 * * This could have been split into separate lflows. What do you think? Thanks, Han > * > @@ -714,7 +713,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 >
On Wed, May 1, 2024 at 6:38 PM Han Zhou <hzhou@ovn.org> wrote: > > > On Tue, Apr 30, 2024 at 9:56 AM 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. > > > > Reported-at: https://issues.redhat.com/browse/FDP-509 > > Signed-off-by: Ales Musil <amusil@redhat.com> > > --- > > 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 | 7 ++- > > 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, 153 insertions(+), 100 deletions(-) > > > > diff --git a/controller/lflow.c b/controller/lflow.c > > index 760ec0b41..06e839cbe 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; > > @@ -647,9 +647,8 @@ as_update_can_be_handled(const char *as_name, struct > addr_set_diff *as_diff, > > * expressions/constants, usually because of disjunctions between > > * sub-expressions/constants, e.g.: > > * > > + * ip.src == $as1 && ip.dst == $as2 > > * ip.src == $as1 || ip.dst == $as2 > > - * ip.src == {$as1, $as2} > > - * ip.src == {$as1, ip1} > > * > > * All these could have been split into separate lflows. > > Hi Ales, thanks for v3. > Hi Han, > I checked again and wondered why you mentioned that "ip.src == $as1 && > ip.dst == $as2" is not supported. This expression would generate > conjunctions, which works with I-P before your change and still works. Did > I miss anything? > yeah my bad, I was focused on this patch rather than what is supported overall. > > In addition, since the constraints are relaxed after your change, I'd also > update the above comments a little more, something like: > > * - The sub expression of the address set is combined with other > sub- > * expressions/constants on different fields, e.g.: > > > * > > > > > * ip.src == $as1 || ip.dst == $as2 > > * > > * This could have been split into separate lflows. > > > What do you think? > Sounds good, I'll post v4 with this update. > > Thanks, > Han > > > * > > @@ -714,7 +713,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 > > > Thanks, Ales
diff --git a/controller/lflow.c b/controller/lflow.c index 760ec0b41..06e839cbe 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; @@ -647,9 +647,8 @@ as_update_can_be_handled(const char *as_name, struct addr_set_diff *as_diff, * expressions/constants, usually because of disjunctions between * sub-expressions/constants, e.g.: * + * ip.src == $as1 && ip.dst == $as2 * ip.src == $as1 || ip.dst == $as2 - * ip.src == {$as1, $as2} - * ip.src == {$as1, ip1} * * All these could have been split into separate lflows. * @@ -714,7 +713,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
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 <amusil@redhat.com> --- 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 | 7 ++- 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, 153 insertions(+), 100 deletions(-)