| Message ID | 20240412152017.137024-1-amusil@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [ovs-dev,v2] controller: Track individual address set constants. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On Fri, Apr 12, 2024 at 8:20 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> > --- > v2: Rebase on top of current main. > Adjust the comment for I-P optimization. Thanks Ales for v2, and sorry for taking so long to review. Please see my comments below. > --- > controller/lflow.c | 4 +- > include/ovn/actions.h | 2 +- > include/ovn/expr.h | 46 ++++++++++--------- > lib/actions.c | 20 ++++----- > lib/expr.c | 98 ++++++++++++++++------------------------- > tests/ovn-controller.at | 14 +++--- > 6 files changed, 84 insertions(+), 100 deletions(-) > > diff --git a/controller/lflow.c b/controller/lflow.c In lflow.c, I noticed that the comments for the function lflow_handle_addr_set_update needs to be updated, because these are not true any more, and also better to mention what's still not supported such as the examples in your commit message. -------- * - The sub expression of the address set is combined with other sub- * expressions/constants, usually because of disjunctions between * sub-expressions/constants, 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. -------- > index 895d17d19..730dc879d 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c > @@ -278,7 +278,7 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in, > } > > static bool > -as_info_from_expr_const(const char *as_name, const union expr_constant *c, > +as_info_from_expr_const(const char *as_name, const struct expr_constant *c, > struct addrset_info *as_info) > { > as_info->name = as_name; > @@ -714,7 +714,7 @@ lflow_handle_addr_set_update(const char *as_name, > if (as_diff->deleted) { > struct addrset_info as_info; > for (size_t i = 0; i < as_diff->deleted->n_values; i++) { > - union expr_constant *c = &as_diff->deleted->values[i]; > + struct expr_constant *c = &as_diff->deleted->values[i]; > if (!as_info_from_expr_const(as_name, c, &as_info)) { > continue; > } > diff --git a/include/ovn/actions.h b/include/ovn/actions.h > index 8e794450c..39c62bb66 100644 > --- a/include/ovn/actions.h > +++ b/include/ovn/actions.h > @@ -238,7 +238,7 @@ struct ovnact_next { > struct ovnact_load { > struct ovnact ovnact; > struct expr_field dst; > - union expr_constant imm; > + struct expr_constant imm; > }; > > /* OVNACT_MOVE, OVNACT_EXCHANGE. */ > diff --git a/include/ovn/expr.h b/include/ovn/expr.h > index c48f82398..e54edb5bf 100644 > --- a/include/ovn/expr.h > +++ b/include/ovn/expr.h > @@ -368,7 +368,7 @@ bool expr_relop_from_token(enum lex_type type, enum expr_relop *relop); > struct expr { > struct ovs_list node; /* In parent EXPR_T_AND or EXPR_T_OR if any. */ > enum expr_type type; /* Expression type. */ > - char *as_name; /* Address set name. Null if it is not an > + const char *as_name; /* Address set name. Null if it is not an > address set. */ > > union { > @@ -505,40 +505,42 @@ enum expr_constant_type { > }; > > /* A string or integer constant (one must know which from context). */ > -union expr_constant { > - /* Integer constant. > - * > - * The width of a constant isn't always clear, e.g. if you write "1", > - * there's no way to tell whether you mean for that to be a 1-bit constant > - * or a 128-bit constant or somewhere in between. */ > - struct { > - union mf_subvalue value; > - union mf_subvalue mask; /* Only initialized if 'masked'. */ > - bool masked; > - > - enum lex_format format; /* From the constant's lex_token. */ > - }; > +struct expr_constant { > + const char *as_name; > > - /* Null-terminated string constant. */ > - char *string; > + union { > + /* Integer constant. > + * > + * The width of a constant isn't always clear, e.g. if you write "1", > + * there's no way to tell whether you mean for that to be a 1-bit > + * constant or a 128-bit constant or somewhere in between. */ > + struct { > + union mf_subvalue value; > + union mf_subvalue mask; /* Only initialized if 'masked'. */ > + bool masked; > + > + enum lex_format format; /* From the constant's lex_token. */ > + }; > + > + /* Null-terminated string constant. */ > + char *string; > + }; > }; > > bool expr_constant_parse(struct lexer *, > const struct expr_field *, > - union expr_constant *); > -void expr_constant_format(const union expr_constant *, > + struct expr_constant *); > +void expr_constant_format(const struct expr_constant *, > enum expr_constant_type, struct ds *); > -void expr_constant_destroy(const union expr_constant *, > +void expr_constant_destroy(const struct expr_constant *, > enum expr_constant_type); > > /* A collection of "union expr_constant"s of the same type. */ > struct expr_constant_set { > - union expr_constant *values; /* Constants. */ > + struct expr_constant *values; /* Constants. */ > size_t n_values; /* Number of constants. */ > enum expr_constant_type type; /* Type of the constants. */ > bool in_curlies; /* Whether the constants were in {}. */ > - char *as_name; /* Name of an address set. It is NULL if not > - an address set. */ > }; > > bool expr_constant_set_parse(struct lexer *, struct expr_constant_set *); > diff --git a/lib/actions.c b/lib/actions.c > index 39bb5bc8a..e43f81684 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); > @@ -1981,7 +1981,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) { > @@ -2457,7 +2457,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; > > @@ -2660,7 +2660,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")) { > @@ -2826,7 +2826,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; > > @@ -2882,7 +2882,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]); > @@ -2892,7 +2892,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]); > @@ -2902,7 +2902,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)); > @@ -3106,7 +3106,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); > @@ -3289,7 +3289,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..56f6220b3 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); > - } > + I understand that this condition is removed because we don't need to assign as_name to expr any more. However, since this is also relaxing the constraints and completely depending on the later expr parsing to determine if address-set information needs to be attached to a sub-expr, I am not 100% sure about the correctness and the test coverage. Do you have a theory that proves this is correct? (I don't think my concern should block the patch since I don't have a negative case yet. I just want to know if you have more thoughts/tests on this already) > exit: > expr_constant_set_destroy(cs); > return e; > @@ -797,11 +793,10 @@ parse_addr_sets(struct expr_context *ctx, struct expr_constant_set *cs, > } > } > > - struct expr_constant_set *addr_sets > - = (ctx->addr_sets > - ? shash_find_data(ctx->addr_sets, ctx->lexer->token.s) > - : NULL); > - if (!addr_sets) { > + struct shash_node *node = ctx->addr_sets > + ? shash_find(ctx->addr_sets, ctx->lexer->token.s) > + : NULL; > + if (!node) { > lexer_syntax_error(ctx->lexer, "expecting address set name"); > return false; > } > @@ -810,17 +805,16 @@ parse_addr_sets(struct expr_context *ctx, struct expr_constant_set *cs, > return false; > } > > - if (!cs->n_values) { > - cs->as_name = xstrdup(ctx->lexer->token.s); > - } > - > + struct expr_constant_set *addr_sets = node->data; > size_t n_values = cs->n_values + addr_sets->n_values; > if (n_values >= *allocated_values) { > cs->values = xrealloc(cs->values, n_values * sizeof *cs->values); > *allocated_values = n_values; > } > for (size_t i = 0; i < addr_sets->n_values; i++) { > - cs->values[cs->n_values++] = addr_sets->values[i]; > + struct expr_constant *c = &cs->values[cs->n_values++]; > + *c = addr_sets->values[i]; > + c->as_name = node->name; > } > > return true; > @@ -859,8 +853,9 @@ parse_port_group(struct expr_context *ctx, struct expr_constant_set *cs, > *allocated_values = n_values; > } > for (size_t i = 0; i < port_group->n_values; i++) { > - cs->values[cs->n_values++].string = > - xstrdup(port_group->values[i].string); > + struct expr_constant *c = &cs->values[cs->n_values++]; > + c->string = xstrdup(port_group->values[i].string); > + c->as_name = NULL; > } > > return true; > @@ -875,11 +870,6 @@ parse_constant(struct expr_context *ctx, struct expr_constant_set *cs, > sizeof *cs->values); > } > > - /* Combining other values to the constant set that is tracking an > - * address set, so untrack it. */ > - free(cs->as_name); > - cs->as_name = NULL; > - > if (ctx->lexer->token.type == LEX_T_TEMPLATE) { > lexer_error(ctx->lexer, "Unexpanded template."); > return false; > @@ -887,7 +877,9 @@ parse_constant(struct expr_context *ctx, struct expr_constant_set *cs, > if (!assign_constant_set_type(ctx, cs, EXPR_C_STRING)) { > return false; > } > - cs->values[cs->n_values++].string = xstrdup(ctx->lexer->token.s); > + struct expr_constant *c = &cs->values[cs->n_values++]; > + c->string = xstrdup(ctx->lexer->token.s); > + c->as_name = NULL; > lexer_get(ctx->lexer); > return true; > } else if (ctx->lexer->token.type == LEX_T_INTEGER || > @@ -896,13 +888,14 @@ parse_constant(struct expr_context *ctx, struct expr_constant_set *cs, > return false; > } > > - union expr_constant *c = &cs->values[cs->n_values++]; > + struct expr_constant *c = &cs->values[cs->n_values++]; > c->value = ctx->lexer->token.value; > c->format = ctx->lexer->token.format; > c->masked = ctx->lexer->token.type == LEX_T_MASKED_INTEGER; > if (c->masked) { > c->mask = ctx->lexer->token.mask; > } > + c->as_name = NULL; > lexer_get(ctx->lexer); > return true; > } else if (ctx->lexer->token.type == LEX_T_MACRO) { > @@ -961,7 +954,7 @@ parse_constant_set(struct expr_context *ctx, struct expr_constant_set *cs) > * indeterminate. */ > bool > expr_constant_parse(struct lexer *lexer, const struct expr_field *f, > - union expr_constant *c) > + struct expr_constant *c) > { > if (lexer->error) { > return false; > @@ -987,7 +980,7 @@ expr_constant_parse(struct lexer *lexer, const struct expr_field *f, > /* Appends to 's' a re-parseable representation of constant 'c' with the given > * 'type'. */ > void > -expr_constant_format(const union expr_constant *c, > +expr_constant_format(const struct expr_constant *c, > enum expr_constant_type type, struct ds *s) > { > if (type == EXPR_C_STRING) { > @@ -1010,7 +1003,7 @@ expr_constant_format(const union expr_constant *c, > * > * Does not free(c). */ > void > -expr_constant_destroy(const union expr_constant *c, > +expr_constant_destroy(const struct expr_constant *c, > enum expr_constant_type type) > { > if (c && type == EXPR_C_STRING) { > @@ -1043,7 +1036,7 @@ expr_constant_set_format(const struct expr_constant_set *cs, struct ds *s) > ds_put_char(s, '{'); > } > > - for (const union expr_constant *c = cs->values; > + for (const struct expr_constant *c = cs->values; > c < &cs->values[cs->n_values]; c++) { > if (c != cs->values) { > ds_put_cstr(s, ", "); > @@ -1067,15 +1060,14 @@ expr_constant_set_destroy(struct expr_constant_set *cs) > } > } > free(cs->values); > - free(cs->as_name); > } > } > > static int > compare_expr_constant_integer_cb(const void *a_, const void *b_) > { > - const union expr_constant *a = a_; > - const union expr_constant *b = b_; > + const struct expr_constant *a = a_; > + const struct expr_constant *b = b_; > > int d = memcmp(&a->value, &b->value, sizeof a->value); > if (d) { > @@ -1114,7 +1106,7 @@ expr_constant_set_create_integers(const char *const *values, size_t n_values) > VLOG_WARN("Invalid constant set entry: '%s', token type: %d", > values[i], lex.token.type); > } else { > - union expr_constant *c = &cs->values[cs->n_values++]; > + struct expr_constant *c = &cs->values[cs->n_values++]; > c->value = lex.token.value; > c->format = lex.token.format; > c->masked = lex.token.type == LEX_T_MASKED_INTEGER; > @@ -1135,7 +1127,7 @@ expr_constant_set_create_integers(const char *const *values, size_t n_values) > > static void > expr_constant_set_add_value(struct expr_constant_set **p_cs, > - union expr_constant *c, size_t *allocated) > + struct expr_constant *c, size_t *allocated) > { > struct expr_constant_set *cs = *p_cs; > if (!cs) { > @@ -1246,7 +1238,7 @@ expr_const_sets_add_strings(struct shash *const_sets, const char *name, > values[i], name); > continue; > } > - union expr_constant *c = &cs->values[cs->n_values++]; > + struct expr_constant *c = &cs->values[cs->n_values++]; > c->string = xstrdup(values[i]); > } > > @@ -1359,7 +1351,7 @@ expr_parse_primary(struct expr_context *ctx, bool *atomic) > > *atomic = true; > > - union expr_constant *cst = xzalloc(sizeof *cst); > + struct expr_constant *cst = xzalloc(sizeof *cst); > cst->format = LEX_F_HEXADECIMAL; > cst->masked = false; > > @@ -1367,7 +1359,6 @@ expr_parse_primary(struct expr_context *ctx, bool *atomic) > c.values = cst; > c.n_values = 1; > c.in_curlies = false; > - c.as_name = NULL; > return make_cmp(ctx, &f, EXPR_R_NE, &c); > } else if (parse_relop(ctx, &r) && parse_constant_set(ctx, &c)) { > return make_cmp(ctx, &f, r, &c); > @@ -1834,7 +1825,6 @@ expr_symtab_destroy(struct shash *symtab) > static struct expr * > expr_clone_cmp(struct expr *expr) > { > - ovs_assert(!expr->as_name); > struct expr *new = xmemdup(expr, sizeof *expr); > if (!new->cmp.symbol->width) { > new->cmp.string = xstrdup(new->cmp.string); > @@ -1858,7 +1848,6 @@ expr_clone_andor(struct expr *expr) > static struct expr * > expr_clone_condition(struct expr *expr) > { > - ovs_assert(!expr->as_name); > struct expr *new = xmemdup(expr, sizeof *expr); > new->cond.string = xstrdup(new->cond.string); > return new; > @@ -1894,8 +1883,6 @@ expr_destroy(struct expr *expr) > return; > } > > - free(expr->as_name); > - > struct expr *sub; > > switch (expr->type) { > @@ -2567,7 +2554,6 @@ crush_or_supersets(struct expr *expr, const struct expr_symbol *symbol) > * mask sizes. */ > size_t n = ovs_list_size(&expr->andor); > struct expr **subs = xmalloc(n * sizeof *subs); > - bool modified = false; > /* Linked list over the 'subs' array to quickly skip deleted elements, > * i.e. the index of the next potentially non-NULL element. */ > size_t *next = xmalloc(n * sizeof *next); > @@ -2596,16 +2582,15 @@ 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)) { > - /* Not a fully maskable field or this expression is tracking an > - * address set. Don't try to optimize to preserve address set I-P. */ I did some more tests and realized that this change would make I-P and recompute result in different flows, which is not very good, even though they both are correct. The problem is, when adding an IP to an address set, if it is a subset of an existing IP (CIDR) of the address set, the crush_or_supersets logic wouldn't be triggered, so a new flow will be added. But if we trigger a recompute, the flow would be removed because it is a subset. E.g. [hanzhou@hanz tutorial]$ for i in `seq 1 10`; do ovn-nbctl add address_set as0 addresses 1.2.3.${i}; done [hanzhou@hanz tutorial]$ ovs-ofctl dump-flows br-int | grep -F 1200 cookie=0x81580570, duration=40.978s, table=46, n_packets=0, n_bytes=0, idle_age=40, priority=1200,ip,reg15=0x2,metadata=0x2,nw_src=1.2.3.0/24 actions=load:0x1->OXM_OF_PKT_REG4[48],resubmit(,47) cookie=0x81580570, duration=3.980s, table=46, n_packets=0, n_bytes=0, idle_age=3, priority=1200,ip,reg15=0x2,metadata=0x2,nw_src=1.2.3.2 actions=load:0x1->OXM_OF_PKT_REG4[48],resubmit(,47) cookie=0x81580570, duration=3.970s, table=46, n_packets=0, n_bytes=0, idle_age=3, priority=1200,ip,reg15=0x2,metadata=0x2,nw_src=1.2.3.3 actions=load:0x1->OXM_OF_PKT_REG4[48],resubmit(,47) cookie=0x81580570, duration=3.959s, table=46, n_packets=0, n_bytes=0, idle_age=3, priority=1200,ip,reg15=0x2,metadata=0x2,nw_src=1.2.3.4 actions=load:0x1->OXM_OF_PKT_REG4[48],resubmit(,47) cookie=0x81580570, duration=3.949s, table=46, n_packets=0, n_bytes=0, idle_age=3, priority=1200,ip,reg15=0x2,metadata=0x2,nw_src=1.2.3.5 actions=load:0x1->OXM_OF_PKT_REG4[48],resubmit(,47) cookie=0x81580570, duration=3.939s, table=46, n_packets=0, n_bytes=0, idle_age=3, priority=1200,ip,reg15=0x2,metadata=0x2,nw_src=1.2.3.6 actions=load:0x1->OXM_OF_PKT_REG4[48],resubmit(,47) cookie=0x81580570, duration=3.930s, table=46, n_packets=0, n_bytes=0, idle_age=3, priority=1200,ip,reg15=0x2,metadata=0x2,nw_src=1.2.3.7 actions=load:0x1->OXM_OF_PKT_REG4[48],resubmit(,47) cookie=0x81580570, duration=3.921s, table=46, n_packets=0, n_bytes=0, idle_age=3, priority=1200,ip,reg15=0x2,metadata=0x2,nw_src=1.2.3.8 actions=load:0x1->OXM_OF_PKT_REG4[48],resubmit(,47) cookie=0x81580570, duration=3.911s, table=46, n_packets=0, n_bytes=0, idle_age=3, priority=1200,ip,reg15=0x2,metadata=0x2,nw_src=1.2.3.9 actions=load:0x1->OXM_OF_PKT_REG4[48],resubmit(,47) cookie=0x81580570, duration=3.901s, table=46, n_packets=0, n_bytes=0, idle_age=3, priority=1200,ip,reg15=0x2,metadata=0x2,nw_src=1.2.3.10 actions=load:0x1->OXM_OF_PKT_REG4[48],resubmit(,47) [hanzhou@hanz tutorial]$ ovn-appctl -t ovn-controller recompute [hanzhou@hanz tutorial]$ ovs-ofctl dump-flows br-int | grep -F 1200 cookie=0x81580570, duration=56.474s, table=46, n_packets=0, n_bytes=0, idle_age=56, priority=1200,ip,reg15=0x2,metadata=0x2,nw_src=1.2.3.0/24 actions=load:0x1->OXM_OF_PKT_REG4[48],resubmit(,47) So I think it is better to keep address sets untouched to preserve I-P and at the same time be consistent with recompute output. I don't have any other problems found so far. I am still thinking about corner cases which may still need some time, but I realized that I'd better share the above feedback to move this forward. Thanks, Han > + if (!symbol->width || symbol->level != EXPR_L_ORDINAL) { > + /* Not a fully maskable field. */ > goto done; > } > > @@ -2658,10 +2643,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 +2671,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 +3251,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 f2c792c9c..0cee006a1 100644 > --- a/tests/ovn-controller.at > +++ b/tests/ovn-controller.at > @@ -1630,7 +1630,9 @@ priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.8 actions=lo > done > > reprocess_count_new=$(read_counter consider_logical_flow) > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [10 > +# First 2 reprocess are due to change from 1 IP in AS to 2. > +# Last 5 is due to overlap in IP addresses between as1 and as2. > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [7 > ]) > > # Remove the IPs from as1 and as2, 1 IP each time. > @@ -1653,9 +1655,9 @@ for i in $(seq 10); do > done > > reprocess_count_new=$(read_counter consider_logical_flow) > -# In this case the as1 and as2 are merged to a single OR expr, so we lose track of > -# address set information - can't handle deletion incrementally. > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [10 > +# First 5 are due to overlap in IP addresses between as1 and as2. > +# Last 2 are due to change from 2 IP in AS to 1. > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [7 > ]) > > OVN_CLEANUP([hv1]) > @@ -1822,7 +1824,7 @@ priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.5 actions=co > priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.10.10.10 actions=conjunction,2/2) > ]) > reprocess_count_new=$(read_counter consider_logical_flow) > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1 > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0 > ]) > > # Delete 2 IPs > @@ -1842,7 +1844,7 @@ priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3 actions=co > priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.10.10.10 actions=conjunction,2/2) > ]) > reprocess_count_new=$(read_counter consider_logical_flow) > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1 > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0 > ]) > > OVN_CLEANUP([hv1]) > -- > 2.44.0 >
On Mon, Apr 29, 2024 at 11:01 PM Han Zhou <hzhou@ovn.org> wrote: > On Fri, Apr 12, 2024 at 8:20 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> > > --- > > v2: Rebase on top of current main. > > Adjust the comment for I-P optimization. > > Thanks Ales for v2, and sorry for taking so long to review. Please see my > comments below. > > Hi Han, thank you for the review. > > > --- > > controller/lflow.c | 4 +- > > include/ovn/actions.h | 2 +- > > include/ovn/expr.h | 46 ++++++++++--------- > > lib/actions.c | 20 ++++----- > > lib/expr.c | 98 ++++++++++++++++------------------------- > > tests/ovn-controller.at | 14 +++--- > > 6 files changed, 84 insertions(+), 100 deletions(-) > > > > diff --git a/controller/lflow.c b/controller/lflow.c > > In lflow.c, I noticed that the comments for the function > lflow_handle_addr_set_update needs to be updated, because these are not > true any more, and also better to mention what's still not supported such > as the examples in your commit message. > -------- > * - The sub expression of the address set is combined with other > sub- > * expressions/constants, usually because of disjunctions between > > * sub-expressions/constants, 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. > -------- > Good point, I'll update that comment in v3 to reflect this. > > > index 895d17d19..730dc879d 100644 > > --- a/controller/lflow.c > > +++ b/controller/lflow.c > > @@ -278,7 +278,7 @@ lflow_handle_changed_flows(struct lflow_ctx_in > *l_ctx_in, > > } > > > > static bool > > -as_info_from_expr_const(const char *as_name, const union expr_constant > *c, > > +as_info_from_expr_const(const char *as_name, const struct expr_constant > *c, > > struct addrset_info *as_info) > > { > > as_info->name = as_name; > > @@ -714,7 +714,7 @@ lflow_handle_addr_set_update(const char *as_name, > > if (as_diff->deleted) { > > struct addrset_info as_info; > > for (size_t i = 0; i < as_diff->deleted->n_values; i++) { > > - union expr_constant *c = &as_diff->deleted->values[i]; > > + struct expr_constant *c = &as_diff->deleted->values[i]; > > if (!as_info_from_expr_const(as_name, c, &as_info)) { > > continue; > > } > > diff --git a/include/ovn/actions.h b/include/ovn/actions.h > > index 8e794450c..39c62bb66 100644 > > --- a/include/ovn/actions.h > > +++ b/include/ovn/actions.h > > @@ -238,7 +238,7 @@ struct ovnact_next { > > struct ovnact_load { > > struct ovnact ovnact; > > struct expr_field dst; > > - union expr_constant imm; > > + struct expr_constant imm; > > }; > > > > /* OVNACT_MOVE, OVNACT_EXCHANGE. */ > > diff --git a/include/ovn/expr.h b/include/ovn/expr.h > > index c48f82398..e54edb5bf 100644 > > --- a/include/ovn/expr.h > > +++ b/include/ovn/expr.h > > @@ -368,7 +368,7 @@ bool expr_relop_from_token(enum lex_type type, enum > expr_relop *relop); > > struct expr { > > struct ovs_list node; /* In parent EXPR_T_AND or EXPR_T_OR if > any. */ > > enum expr_type type; /* Expression type. */ > > - char *as_name; /* Address set name. Null if it is not > an > > + const char *as_name; /* Address set name. Null if it is not > an > > address set. */ > > > > union { > > @@ -505,40 +505,42 @@ enum expr_constant_type { > > }; > > > > /* A string or integer constant (one must know which from context). */ > > -union expr_constant { > > - /* Integer constant. > > - * > > - * The width of a constant isn't always clear, e.g. if you write > "1", > > - * there's no way to tell whether you mean for that to be a 1-bit > constant > > - * or a 128-bit constant or somewhere in between. */ > > - struct { > > - union mf_subvalue value; > > - union mf_subvalue mask; /* Only initialized if 'masked'. */ > > - bool masked; > > - > > - enum lex_format format; /* From the constant's lex_token. */ > > - }; > > +struct expr_constant { > > + const char *as_name; > > > > - /* Null-terminated string constant. */ > > - char *string; > > + union { > > + /* Integer constant. > > + * > > + * The width of a constant isn't always clear, e.g. if you > write "1", > > + * there's no way to tell whether you mean for that to be a > 1-bit > > + * constant or a 128-bit constant or somewhere in between. */ > > + struct { > > + union mf_subvalue value; > > + union mf_subvalue mask; /* Only initialized if 'masked'. */ > > + bool masked; > > + > > + enum lex_format format; /* From the constant's lex_token. */ > > + }; > > + > > + /* Null-terminated string constant. */ > > + char *string; > > + }; > > }; > > > > bool expr_constant_parse(struct lexer *, > > const struct expr_field *, > > - union expr_constant *); > > -void expr_constant_format(const union expr_constant *, > > + struct expr_constant *); > > +void expr_constant_format(const struct expr_constant *, > > enum expr_constant_type, struct ds *); > > -void expr_constant_destroy(const union expr_constant *, > > +void expr_constant_destroy(const struct expr_constant *, > > enum expr_constant_type); > > > > /* A collection of "union expr_constant"s of the same type. */ > > struct expr_constant_set { > > - union expr_constant *values; /* Constants. */ > > + struct expr_constant *values; /* Constants. */ > > size_t n_values; /* Number of constants. */ > > enum expr_constant_type type; /* Type of the constants. */ > > bool in_curlies; /* Whether the constants were in {}. > */ > > - char *as_name; /* Name of an address set. It is NULL > if not > > - an address set. */ > > }; > > > > bool expr_constant_set_parse(struct lexer *, struct expr_constant_set > *); > > diff --git a/lib/actions.c b/lib/actions.c > > index 39bb5bc8a..e43f81684 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); > > @@ -1981,7 +1981,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) { > > @@ -2457,7 +2457,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; > > > > @@ -2660,7 +2660,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")) { > > @@ -2826,7 +2826,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; > > > > @@ -2882,7 +2882,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]); > > @@ -2892,7 +2892,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]); > > @@ -2902,7 +2902,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)); > > @@ -3106,7 +3106,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); > > @@ -3289,7 +3289,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..56f6220b3 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); > > - } > > + > > I understand that this condition is removed because we don't need to > assign as_name to expr any more. However, since this is also relaxing the > constraints and completely depending on the later expr parsing to determine > if address-set information needs to be attached to a sub-expr, I am not > 100% sure about the correctness and the test coverage. Do you have a theory > that proves this is correct? (I don't think my concern should block the > patch since I don't have a negative case yet. I just want to know if you > have more thoughts/tests on this already) > It is still assigned to the expression but as part of the "make_cmp__()" instead, because the whole expression set doesn't track the AS now. The relaxation should be fine from my point of view because if it's OR it will still result in multiple lflows marked as AS individually. If it's AND the whole expression is actually a combination of multiple expressions so it won't have the AS reference in the end. The test coverage should be fine, AFAIR the "ovn-controller - I-P for address set update: multiple ASes used by same lflow" leads into the AND scenario. > > > exit: > > expr_constant_set_destroy(cs); > > return e; > > @@ -797,11 +793,10 @@ parse_addr_sets(struct expr_context *ctx, struct > expr_constant_set *cs, > > } > > } > > > > - struct expr_constant_set *addr_sets > > - = (ctx->addr_sets > > - ? shash_find_data(ctx->addr_sets, ctx->lexer->token.s) > > - : NULL); > > - if (!addr_sets) { > > + struct shash_node *node = ctx->addr_sets > > + ? shash_find(ctx->addr_sets, > ctx->lexer->token.s) > > + : NULL; > > + if (!node) { > > lexer_syntax_error(ctx->lexer, "expecting address set name"); > > return false; > > } > > @@ -810,17 +805,16 @@ parse_addr_sets(struct expr_context *ctx, struct > expr_constant_set *cs, > > return false; > > } > > > > - if (!cs->n_values) { > > - cs->as_name = xstrdup(ctx->lexer->token.s); > > - } > > - > > + struct expr_constant_set *addr_sets = node->data; > > size_t n_values = cs->n_values + addr_sets->n_values; > > if (n_values >= *allocated_values) { > > cs->values = xrealloc(cs->values, n_values * sizeof > *cs->values); > > *allocated_values = n_values; > > } > > for (size_t i = 0; i < addr_sets->n_values; i++) { > > - cs->values[cs->n_values++] = addr_sets->values[i]; > > + struct expr_constant *c = &cs->values[cs->n_values++]; > > + *c = addr_sets->values[i]; > > + c->as_name = node->name; > > } > > > > return true; > > @@ -859,8 +853,9 @@ parse_port_group(struct expr_context *ctx, struct > expr_constant_set *cs, > > *allocated_values = n_values; > > } > > for (size_t i = 0; i < port_group->n_values; i++) { > > - cs->values[cs->n_values++].string = > > - xstrdup(port_group->values[i].string); > > + struct expr_constant *c = &cs->values[cs->n_values++]; > > + c->string = xstrdup(port_group->values[i].string); > > + c->as_name = NULL; > > } > > > > return true; > > @@ -875,11 +870,6 @@ parse_constant(struct expr_context *ctx, struct > expr_constant_set *cs, > > sizeof *cs->values); > > } > > > > - /* Combining other values to the constant set that is tracking an > > - * address set, so untrack it. */ > > - free(cs->as_name); > > - cs->as_name = NULL; > > - > > if (ctx->lexer->token.type == LEX_T_TEMPLATE) { > > lexer_error(ctx->lexer, "Unexpanded template."); > > return false; > > @@ -887,7 +877,9 @@ parse_constant(struct expr_context *ctx, struct > expr_constant_set *cs, > > if (!assign_constant_set_type(ctx, cs, EXPR_C_STRING)) { > > return false; > > } > > - cs->values[cs->n_values++].string = > xstrdup(ctx->lexer->token.s); > > + struct expr_constant *c = &cs->values[cs->n_values++]; > > + c->string = xstrdup(ctx->lexer->token.s); > > + c->as_name = NULL; > > lexer_get(ctx->lexer); > > return true; > > } else if (ctx->lexer->token.type == LEX_T_INTEGER || > > @@ -896,13 +888,14 @@ parse_constant(struct expr_context *ctx, struct > expr_constant_set *cs, > > return false; > > } > > > > - union expr_constant *c = &cs->values[cs->n_values++]; > > + struct expr_constant *c = &cs->values[cs->n_values++]; > > c->value = ctx->lexer->token.value; > > c->format = ctx->lexer->token.format; > > c->masked = ctx->lexer->token.type == LEX_T_MASKED_INTEGER; > > if (c->masked) { > > c->mask = ctx->lexer->token.mask; > > } > > + c->as_name = NULL; > > lexer_get(ctx->lexer); > > return true; > > } else if (ctx->lexer->token.type == LEX_T_MACRO) { > > @@ -961,7 +954,7 @@ parse_constant_set(struct expr_context *ctx, struct > expr_constant_set *cs) > > * indeterminate. */ > > bool > > expr_constant_parse(struct lexer *lexer, const struct expr_field *f, > > - union expr_constant *c) > > + struct expr_constant *c) > > { > > if (lexer->error) { > > return false; > > @@ -987,7 +980,7 @@ expr_constant_parse(struct lexer *lexer, const > struct expr_field *f, > > /* Appends to 's' a re-parseable representation of constant 'c' with > the given > > * 'type'. */ > > void > > -expr_constant_format(const union expr_constant *c, > > +expr_constant_format(const struct expr_constant *c, > > enum expr_constant_type type, struct ds *s) > > { > > if (type == EXPR_C_STRING) { > > @@ -1010,7 +1003,7 @@ expr_constant_format(const union expr_constant *c, > > * > > * Does not free(c). */ > > void > > -expr_constant_destroy(const union expr_constant *c, > > +expr_constant_destroy(const struct expr_constant *c, > > enum expr_constant_type type) > > { > > if (c && type == EXPR_C_STRING) { > > @@ -1043,7 +1036,7 @@ expr_constant_set_format(const struct > expr_constant_set *cs, struct ds *s) > > ds_put_char(s, '{'); > > } > > > > - for (const union expr_constant *c = cs->values; > > + for (const struct expr_constant *c = cs->values; > > c < &cs->values[cs->n_values]; c++) { > > if (c != cs->values) { > > ds_put_cstr(s, ", "); > > @@ -1067,15 +1060,14 @@ expr_constant_set_destroy(struct > expr_constant_set *cs) > > } > > } > > free(cs->values); > > - free(cs->as_name); > > } > > } > > > > static int > > compare_expr_constant_integer_cb(const void *a_, const void *b_) > > { > > - const union expr_constant *a = a_; > > - const union expr_constant *b = b_; > > + const struct expr_constant *a = a_; > > + const struct expr_constant *b = b_; > > > > int d = memcmp(&a->value, &b->value, sizeof a->value); > > if (d) { > > @@ -1114,7 +1106,7 @@ expr_constant_set_create_integers(const char > *const *values, size_t n_values) > > VLOG_WARN("Invalid constant set entry: '%s', token type: > %d", > > values[i], lex.token.type); > > } else { > > - union expr_constant *c = &cs->values[cs->n_values++]; > > + struct expr_constant *c = &cs->values[cs->n_values++]; > > c->value = lex.token.value; > > c->format = lex.token.format; > > c->masked = lex.token.type == LEX_T_MASKED_INTEGER; > > @@ -1135,7 +1127,7 @@ expr_constant_set_create_integers(const char > *const *values, size_t n_values) > > > > static void > > expr_constant_set_add_value(struct expr_constant_set **p_cs, > > - union expr_constant *c, size_t *allocated) > > + struct expr_constant *c, size_t *allocated) > > { > > struct expr_constant_set *cs = *p_cs; > > if (!cs) { > > @@ -1246,7 +1238,7 @@ expr_const_sets_add_strings(struct shash > *const_sets, const char *name, > > values[i], name); > > continue; > > } > > - union expr_constant *c = &cs->values[cs->n_values++]; > > + struct expr_constant *c = &cs->values[cs->n_values++]; > > c->string = xstrdup(values[i]); > > } > > > > @@ -1359,7 +1351,7 @@ expr_parse_primary(struct expr_context *ctx, bool > *atomic) > > > > *atomic = true; > > > > - union expr_constant *cst = xzalloc(sizeof *cst); > > + struct expr_constant *cst = xzalloc(sizeof *cst); > > cst->format = LEX_F_HEXADECIMAL; > > cst->masked = false; > > > > @@ -1367,7 +1359,6 @@ expr_parse_primary(struct expr_context *ctx, bool > *atomic) > > c.values = cst; > > c.n_values = 1; > > c.in_curlies = false; > > - c.as_name = NULL; > > return make_cmp(ctx, &f, EXPR_R_NE, &c); > > } else if (parse_relop(ctx, &r) && parse_constant_set(ctx, &c)) > { > > return make_cmp(ctx, &f, r, &c); > > @@ -1834,7 +1825,6 @@ expr_symtab_destroy(struct shash *symtab) > > static struct expr * > > expr_clone_cmp(struct expr *expr) > > { > > - ovs_assert(!expr->as_name); > > struct expr *new = xmemdup(expr, sizeof *expr); > > if (!new->cmp.symbol->width) { > > new->cmp.string = xstrdup(new->cmp.string); > > @@ -1858,7 +1848,6 @@ expr_clone_andor(struct expr *expr) > > static struct expr * > > expr_clone_condition(struct expr *expr) > > { > > - ovs_assert(!expr->as_name); > > struct expr *new = xmemdup(expr, sizeof *expr); > > new->cond.string = xstrdup(new->cond.string); > > return new; > > @@ -1894,8 +1883,6 @@ expr_destroy(struct expr *expr) > > return; > > } > > > > - free(expr->as_name); > > - > > struct expr *sub; > > > > switch (expr->type) { > > @@ -2567,7 +2554,6 @@ crush_or_supersets(struct expr *expr, const struct > expr_symbol *symbol) > > * mask sizes. */ > > size_t n = ovs_list_size(&expr->andor); > > struct expr **subs = xmalloc(n * sizeof *subs); > > - bool modified = false; > > /* Linked list over the 'subs' array to quickly skip deleted > elements, > > * i.e. the index of the next potentially non-NULL element. */ > > size_t *next = xmalloc(n * sizeof *next); > > @@ -2596,16 +2582,15 @@ 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)) { > > - /* Not a fully maskable field or this expression is tracking an > > - * address set. Don't try to optimize to preserve address set > I-P. */ > > I did some more tests and realized that this change would make I-P and > recompute result in different flows, which is not very good, even though > they both are correct. > The problem is, when adding an IP to an address set, if it is a subset of > an existing IP (CIDR) of the address set, the crush_or_supersets logic > wouldn't be triggered, so a new flow will be added. But if we trigger a > recompute, the flow would be removed because it is a subset. > E.g. > > [hanzhou@hanz tutorial]$ for i in `seq 1 10`; do ovn-nbctl add > address_set as0 addresses 1.2.3.${i}; done > [hanzhou@hanz tutorial]$ ovs-ofctl dump-flows br-int | grep -F 1200 > cookie=0x81580570, duration=40.978s, table=46, n_packets=0, n_bytes=0, > idle_age=40, priority=1200,ip,reg15=0x2,metadata=0x2,nw_src=1.2.3.0/24 > actions=load:0x1->OXM_OF_PKT_REG4[48],resubmit(,47) > cookie=0x81580570, duration=3.980s, table=46, n_packets=0, n_bytes=0, > idle_age=3, priority=1200,ip,reg15=0x2,metadata=0x2,nw_src=1.2.3.2 > actions=load:0x1->OXM_OF_PKT_REG4[48],resubmit(,47) > cookie=0x81580570, duration=3.970s, table=46, n_packets=0, n_bytes=0, > idle_age=3, priority=1200,ip,reg15=0x2,metadata=0x2,nw_src=1.2.3.3 > actions=load:0x1->OXM_OF_PKT_REG4[48],resubmit(,47) > cookie=0x81580570, duration=3.959s, table=46, n_packets=0, n_bytes=0, > idle_age=3, priority=1200,ip,reg15=0x2,metadata=0x2,nw_src=1.2.3.4 > actions=load:0x1->OXM_OF_PKT_REG4[48],resubmit(,47) > cookie=0x81580570, duration=3.949s, table=46, n_packets=0, n_bytes=0, > idle_age=3, priority=1200,ip,reg15=0x2,metadata=0x2,nw_src=1.2.3.5 > actions=load:0x1->OXM_OF_PKT_REG4[48],resubmit(,47) > cookie=0x81580570, duration=3.939s, table=46, n_packets=0, n_bytes=0, > idle_age=3, priority=1200,ip,reg15=0x2,metadata=0x2,nw_src=1.2.3.6 > actions=load:0x1->OXM_OF_PKT_REG4[48],resubmit(,47) > cookie=0x81580570, duration=3.930s, table=46, n_packets=0, n_bytes=0, > idle_age=3, priority=1200,ip,reg15=0x2,metadata=0x2,nw_src=1.2.3.7 > actions=load:0x1->OXM_OF_PKT_REG4[48],resubmit(,47) > cookie=0x81580570, duration=3.921s, table=46, n_packets=0, n_bytes=0, > idle_age=3, priority=1200,ip,reg15=0x2,metadata=0x2,nw_src=1.2.3.8 > actions=load:0x1->OXM_OF_PKT_REG4[48],resubmit(,47) > cookie=0x81580570, duration=3.911s, table=46, n_packets=0, n_bytes=0, > idle_age=3, priority=1200,ip,reg15=0x2,metadata=0x2,nw_src=1.2.3.9 > actions=load:0x1->OXM_OF_PKT_REG4[48],resubmit(,47) > cookie=0x81580570, duration=3.901s, table=46, n_packets=0, n_bytes=0, > idle_age=3, priority=1200,ip,reg15=0x2,metadata=0x2,nw_src=1.2.3.10 > actions=load:0x1->OXM_OF_PKT_REG4[48],resubmit(,47) > [hanzhou@hanz tutorial]$ ovn-appctl -t ovn-controller recompute > [hanzhou@hanz tutorial]$ ovs-ofctl dump-flows br-int | grep -F 1200 > cookie=0x81580570, duration=56.474s, table=46, n_packets=0, n_bytes=0, > idle_age=56, priority=1200,ip,reg15=0x2,metadata=0x2,nw_src=1.2.3.0/24 > actions=load:0x1->OXM_OF_PKT_REG4[48],resubmit(,47) > > So I think it is better to keep address sets untouched to preserve I-P and > at the same time be consistent with recompute output. > > Nice catch! You are right it's not wrong however I agree that we should aim for consistency, I'll fix that in v3 and add test case for this. > > I don't have any other problems found so far. I am still thinking about > corner cases which may still need some time, but I realized that I'd better > share the above feedback to move this forward. > > Thanks, > Han > > > + if (!symbol->width || symbol->level != EXPR_L_ORDINAL) { > > + /* Not a fully maskable field. */ > > goto done; > > } > > > > @@ -2658,10 +2643,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 +2671,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 +3251,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 f2c792c9c..0cee006a1 100644 > > --- a/tests/ovn-controller.at > > +++ b/tests/ovn-controller.at > > @@ -1630,7 +1630,9 @@ > priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.8 > actions=lo > > done > > > > reprocess_count_new=$(read_counter consider_logical_flow) > > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], > [10 > > +# First 2 reprocess are due to change from 1 IP in AS to 2. > > +# Last 5 is due to overlap in IP addresses between as1 and as2. > > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], > [7 > > ]) > > > > # Remove the IPs from as1 and as2, 1 IP each time. > > @@ -1653,9 +1655,9 @@ for i in $(seq 10); do > > done > > > > reprocess_count_new=$(read_counter consider_logical_flow) > > -# In this case the as1 and as2 are merged to a single OR expr, so we > lose track of > > -# address set information - can't handle deletion incrementally. > > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], > [10 > > +# First 5 are due to overlap in IP addresses between as1 and as2. > > +# Last 2 are due to change from 2 IP in AS to 1. > > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], > [7 > > ]) > > > > OVN_CLEANUP([hv1]) > > @@ -1822,7 +1824,7 @@ > priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.5 > actions=co > > > priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.10.10.10 > actions=conjunction,2/2) > > ]) > > reprocess_count_new=$(read_counter consider_logical_flow) > > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], > [1 > > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], > [0 > > ]) > > > > # Delete 2 IPs > > @@ -1842,7 +1844,7 @@ > priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3 > actions=co > > > priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.10.10.10 > actions=conjunction,2/2) > > ]) > > reprocess_count_new=$(read_counter consider_logical_flow) > > -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], > [1 > > +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], > [0 > > ]) > > > > OVN_CLEANUP([hv1]) > > -- > > 2.44.0 > > > Thanks, Ales
diff --git a/controller/lflow.c b/controller/lflow.c index 895d17d19..730dc879d 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -278,7 +278,7 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in, } static bool -as_info_from_expr_const(const char *as_name, const union expr_constant *c, +as_info_from_expr_const(const char *as_name, const struct expr_constant *c, struct addrset_info *as_info) { as_info->name = as_name; @@ -714,7 +714,7 @@ lflow_handle_addr_set_update(const char *as_name, if (as_diff->deleted) { struct addrset_info as_info; for (size_t i = 0; i < as_diff->deleted->n_values; i++) { - union expr_constant *c = &as_diff->deleted->values[i]; + struct expr_constant *c = &as_diff->deleted->values[i]; if (!as_info_from_expr_const(as_name, c, &as_info)) { continue; } diff --git a/include/ovn/actions.h b/include/ovn/actions.h index 8e794450c..39c62bb66 100644 --- a/include/ovn/actions.h +++ b/include/ovn/actions.h @@ -238,7 +238,7 @@ struct ovnact_next { struct ovnact_load { struct ovnact ovnact; struct expr_field dst; - union expr_constant imm; + struct expr_constant imm; }; /* OVNACT_MOVE, OVNACT_EXCHANGE. */ diff --git a/include/ovn/expr.h b/include/ovn/expr.h index c48f82398..e54edb5bf 100644 --- a/include/ovn/expr.h +++ b/include/ovn/expr.h @@ -368,7 +368,7 @@ bool expr_relop_from_token(enum lex_type type, enum expr_relop *relop); struct expr { struct ovs_list node; /* In parent EXPR_T_AND or EXPR_T_OR if any. */ enum expr_type type; /* Expression type. */ - char *as_name; /* Address set name. Null if it is not an + const char *as_name; /* Address set name. Null if it is not an address set. */ union { @@ -505,40 +505,42 @@ enum expr_constant_type { }; /* A string or integer constant (one must know which from context). */ -union expr_constant { - /* Integer constant. - * - * The width of a constant isn't always clear, e.g. if you write "1", - * there's no way to tell whether you mean for that to be a 1-bit constant - * or a 128-bit constant or somewhere in between. */ - struct { - union mf_subvalue value; - union mf_subvalue mask; /* Only initialized if 'masked'. */ - bool masked; - - enum lex_format format; /* From the constant's lex_token. */ - }; +struct expr_constant { + const char *as_name; - /* Null-terminated string constant. */ - char *string; + union { + /* Integer constant. + * + * The width of a constant isn't always clear, e.g. if you write "1", + * there's no way to tell whether you mean for that to be a 1-bit + * constant or a 128-bit constant or somewhere in between. */ + struct { + union mf_subvalue value; + union mf_subvalue mask; /* Only initialized if 'masked'. */ + bool masked; + + enum lex_format format; /* From the constant's lex_token. */ + }; + + /* Null-terminated string constant. */ + char *string; + }; }; bool expr_constant_parse(struct lexer *, const struct expr_field *, - union expr_constant *); -void expr_constant_format(const union expr_constant *, + struct expr_constant *); +void expr_constant_format(const struct expr_constant *, enum expr_constant_type, struct ds *); -void expr_constant_destroy(const union expr_constant *, +void expr_constant_destroy(const struct expr_constant *, enum expr_constant_type); /* A collection of "union expr_constant"s of the same type. */ struct expr_constant_set { - union expr_constant *values; /* Constants. */ + struct expr_constant *values; /* Constants. */ size_t n_values; /* Number of constants. */ enum expr_constant_type type; /* Type of the constants. */ bool in_curlies; /* Whether the constants were in {}. */ - char *as_name; /* Name of an address set. It is NULL if not - an address set. */ }; bool expr_constant_set_parse(struct lexer *, struct expr_constant_set *); diff --git a/lib/actions.c b/lib/actions.c index 39bb5bc8a..e43f81684 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); @@ -1981,7 +1981,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) { @@ -2457,7 +2457,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; @@ -2660,7 +2660,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")) { @@ -2826,7 +2826,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; @@ -2882,7 +2882,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]); @@ -2892,7 +2892,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]); @@ -2902,7 +2902,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)); @@ -3106,7 +3106,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); @@ -3289,7 +3289,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..56f6220b3 100644 --- a/lib/expr.c +++ b/lib/expr.c @@ -179,12 +179,10 @@ expr_combine(enum expr_type type, struct expr *a, struct expr *b) } else { ovs_list_push_back(&a->andor, &b->node); } - free(a->as_name); a->as_name = NULL; return a; } else if (b->type == type) { ovs_list_push_front(&b->andor, &a->node); - free(b->as_name); b->as_name = NULL; return b; } else { @@ -521,12 +519,13 @@ static bool parse_field(struct expr_context *, struct expr_field *); static struct expr * make_cmp__(const struct expr_field *f, enum expr_relop r, - const union expr_constant *c) + const struct expr_constant *c) { struct expr *e = xzalloc(sizeof *e); e->type = EXPR_T_CMP; e->cmp.symbol = f->symbol; e->cmp.relop = r; + e->as_name = c->as_name; if (f->symbol->width) { bitwise_copy(&c->value, sizeof c->value, 0, &e->cmp.value, sizeof e->cmp.value, f->ofs, @@ -547,7 +546,7 @@ make_cmp__(const struct expr_field *f, enum expr_relop r, /* Returns the minimum reasonable width for integer constant 'c'. */ static int -expr_constant_width(const union expr_constant *c) +expr_constant_width(const struct expr_constant *c) { if (c->masked) { return mf_subvalue_width(&c->mask); @@ -674,10 +673,7 @@ make_cmp(struct expr_context *ctx, e = expr_combine(r == EXPR_R_EQ ? EXPR_T_OR : EXPR_T_AND, e, make_cmp__(f, r, &cs->values[i])); } - /* Track address set */ - if (r == EXPR_R_EQ && e->type == EXPR_T_OR && cs->as_name) { - e->as_name = xstrdup(cs->as_name); - } + exit: expr_constant_set_destroy(cs); return e; @@ -797,11 +793,10 @@ parse_addr_sets(struct expr_context *ctx, struct expr_constant_set *cs, } } - struct expr_constant_set *addr_sets - = (ctx->addr_sets - ? shash_find_data(ctx->addr_sets, ctx->lexer->token.s) - : NULL); - if (!addr_sets) { + struct shash_node *node = ctx->addr_sets + ? shash_find(ctx->addr_sets, ctx->lexer->token.s) + : NULL; + if (!node) { lexer_syntax_error(ctx->lexer, "expecting address set name"); return false; } @@ -810,17 +805,16 @@ parse_addr_sets(struct expr_context *ctx, struct expr_constant_set *cs, return false; } - if (!cs->n_values) { - cs->as_name = xstrdup(ctx->lexer->token.s); - } - + struct expr_constant_set *addr_sets = node->data; size_t n_values = cs->n_values + addr_sets->n_values; if (n_values >= *allocated_values) { cs->values = xrealloc(cs->values, n_values * sizeof *cs->values); *allocated_values = n_values; } for (size_t i = 0; i < addr_sets->n_values; i++) { - cs->values[cs->n_values++] = addr_sets->values[i]; + struct expr_constant *c = &cs->values[cs->n_values++]; + *c = addr_sets->values[i]; + c->as_name = node->name; } return true; @@ -859,8 +853,9 @@ parse_port_group(struct expr_context *ctx, struct expr_constant_set *cs, *allocated_values = n_values; } for (size_t i = 0; i < port_group->n_values; i++) { - cs->values[cs->n_values++].string = - xstrdup(port_group->values[i].string); + struct expr_constant *c = &cs->values[cs->n_values++]; + c->string = xstrdup(port_group->values[i].string); + c->as_name = NULL; } return true; @@ -875,11 +870,6 @@ parse_constant(struct expr_context *ctx, struct expr_constant_set *cs, sizeof *cs->values); } - /* Combining other values to the constant set that is tracking an - * address set, so untrack it. */ - free(cs->as_name); - cs->as_name = NULL; - if (ctx->lexer->token.type == LEX_T_TEMPLATE) { lexer_error(ctx->lexer, "Unexpanded template."); return false; @@ -887,7 +877,9 @@ parse_constant(struct expr_context *ctx, struct expr_constant_set *cs, if (!assign_constant_set_type(ctx, cs, EXPR_C_STRING)) { return false; } - cs->values[cs->n_values++].string = xstrdup(ctx->lexer->token.s); + struct expr_constant *c = &cs->values[cs->n_values++]; + c->string = xstrdup(ctx->lexer->token.s); + c->as_name = NULL; lexer_get(ctx->lexer); return true; } else if (ctx->lexer->token.type == LEX_T_INTEGER || @@ -896,13 +888,14 @@ parse_constant(struct expr_context *ctx, struct expr_constant_set *cs, return false; } - union expr_constant *c = &cs->values[cs->n_values++]; + struct expr_constant *c = &cs->values[cs->n_values++]; c->value = ctx->lexer->token.value; c->format = ctx->lexer->token.format; c->masked = ctx->lexer->token.type == LEX_T_MASKED_INTEGER; if (c->masked) { c->mask = ctx->lexer->token.mask; } + c->as_name = NULL; lexer_get(ctx->lexer); return true; } else if (ctx->lexer->token.type == LEX_T_MACRO) { @@ -961,7 +954,7 @@ parse_constant_set(struct expr_context *ctx, struct expr_constant_set *cs) * indeterminate. */ bool expr_constant_parse(struct lexer *lexer, const struct expr_field *f, - union expr_constant *c) + struct expr_constant *c) { if (lexer->error) { return false; @@ -987,7 +980,7 @@ expr_constant_parse(struct lexer *lexer, const struct expr_field *f, /* Appends to 's' a re-parseable representation of constant 'c' with the given * 'type'. */ void -expr_constant_format(const union expr_constant *c, +expr_constant_format(const struct expr_constant *c, enum expr_constant_type type, struct ds *s) { if (type == EXPR_C_STRING) { @@ -1010,7 +1003,7 @@ expr_constant_format(const union expr_constant *c, * * Does not free(c). */ void -expr_constant_destroy(const union expr_constant *c, +expr_constant_destroy(const struct expr_constant *c, enum expr_constant_type type) { if (c && type == EXPR_C_STRING) { @@ -1043,7 +1036,7 @@ expr_constant_set_format(const struct expr_constant_set *cs, struct ds *s) ds_put_char(s, '{'); } - for (const union expr_constant *c = cs->values; + for (const struct expr_constant *c = cs->values; c < &cs->values[cs->n_values]; c++) { if (c != cs->values) { ds_put_cstr(s, ", "); @@ -1067,15 +1060,14 @@ expr_constant_set_destroy(struct expr_constant_set *cs) } } free(cs->values); - free(cs->as_name); } } static int compare_expr_constant_integer_cb(const void *a_, const void *b_) { - const union expr_constant *a = a_; - const union expr_constant *b = b_; + const struct expr_constant *a = a_; + const struct expr_constant *b = b_; int d = memcmp(&a->value, &b->value, sizeof a->value); if (d) { @@ -1114,7 +1106,7 @@ expr_constant_set_create_integers(const char *const *values, size_t n_values) VLOG_WARN("Invalid constant set entry: '%s', token type: %d", values[i], lex.token.type); } else { - union expr_constant *c = &cs->values[cs->n_values++]; + struct expr_constant *c = &cs->values[cs->n_values++]; c->value = lex.token.value; c->format = lex.token.format; c->masked = lex.token.type == LEX_T_MASKED_INTEGER; @@ -1135,7 +1127,7 @@ expr_constant_set_create_integers(const char *const *values, size_t n_values) static void expr_constant_set_add_value(struct expr_constant_set **p_cs, - union expr_constant *c, size_t *allocated) + struct expr_constant *c, size_t *allocated) { struct expr_constant_set *cs = *p_cs; if (!cs) { @@ -1246,7 +1238,7 @@ expr_const_sets_add_strings(struct shash *const_sets, const char *name, values[i], name); continue; } - union expr_constant *c = &cs->values[cs->n_values++]; + struct expr_constant *c = &cs->values[cs->n_values++]; c->string = xstrdup(values[i]); } @@ -1359,7 +1351,7 @@ expr_parse_primary(struct expr_context *ctx, bool *atomic) *atomic = true; - union expr_constant *cst = xzalloc(sizeof *cst); + struct expr_constant *cst = xzalloc(sizeof *cst); cst->format = LEX_F_HEXADECIMAL; cst->masked = false; @@ -1367,7 +1359,6 @@ expr_parse_primary(struct expr_context *ctx, bool *atomic) c.values = cst; c.n_values = 1; c.in_curlies = false; - c.as_name = NULL; return make_cmp(ctx, &f, EXPR_R_NE, &c); } else if (parse_relop(ctx, &r) && parse_constant_set(ctx, &c)) { return make_cmp(ctx, &f, r, &c); @@ -1834,7 +1825,6 @@ expr_symtab_destroy(struct shash *symtab) static struct expr * expr_clone_cmp(struct expr *expr) { - ovs_assert(!expr->as_name); struct expr *new = xmemdup(expr, sizeof *expr); if (!new->cmp.symbol->width) { new->cmp.string = xstrdup(new->cmp.string); @@ -1858,7 +1848,6 @@ expr_clone_andor(struct expr *expr) static struct expr * expr_clone_condition(struct expr *expr) { - ovs_assert(!expr->as_name); struct expr *new = xmemdup(expr, sizeof *expr); new->cond.string = xstrdup(new->cond.string); return new; @@ -1894,8 +1883,6 @@ expr_destroy(struct expr *expr) return; } - free(expr->as_name); - struct expr *sub; switch (expr->type) { @@ -2567,7 +2554,6 @@ crush_or_supersets(struct expr *expr, const struct expr_symbol *symbol) * mask sizes. */ size_t n = ovs_list_size(&expr->andor); struct expr **subs = xmalloc(n * sizeof *subs); - bool modified = false; /* Linked list over the 'subs' array to quickly skip deleted elements, * i.e. the index of the next potentially non-NULL element. */ size_t *next = xmalloc(n * sizeof *next); @@ -2596,16 +2582,15 @@ 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)) { - /* Not a fully maskable field or this expression is tracking an - * address set. Don't try to optimize to preserve address set I-P. */ + if (!symbol->width || symbol->level != EXPR_L_ORDINAL) { + /* Not a fully maskable field. */ goto done; } @@ -2658,10 +2643,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 +2671,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 +3251,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 f2c792c9c..0cee006a1 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -1630,7 +1630,9 @@ priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.8 actions=lo done reprocess_count_new=$(read_counter consider_logical_flow) -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [10 +# First 2 reprocess are due to change from 1 IP in AS to 2. +# Last 5 is due to overlap in IP addresses between as1 and as2. +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [7 ]) # Remove the IPs from as1 and as2, 1 IP each time. @@ -1653,9 +1655,9 @@ for i in $(seq 10); do done reprocess_count_new=$(read_counter consider_logical_flow) -# In this case the as1 and as2 are merged to a single OR expr, so we lose track of -# address set information - can't handle deletion incrementally. -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [10 +# First 5 are due to overlap in IP addresses between as1 and as2. +# Last 2 are due to change from 2 IP in AS to 1. +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [7 ]) OVN_CLEANUP([hv1]) @@ -1822,7 +1824,7 @@ priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.5 actions=co priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.10.10.10 actions=conjunction,2/2) ]) reprocess_count_new=$(read_counter consider_logical_flow) -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1 +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0 ]) # Delete 2 IPs @@ -1842,7 +1844,7 @@ priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3 actions=co priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.10.10.10 actions=conjunction,2/2) ]) reprocess_count_new=$(read_counter consider_logical_flow) -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1 +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0 ]) OVN_CLEANUP([hv1])
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> --- v2: Rebase on top of current main. Adjust the comment for I-P optimization. --- controller/lflow.c | 4 +- include/ovn/actions.h | 2 +- include/ovn/expr.h | 46 ++++++++++--------- lib/actions.c | 20 ++++----- lib/expr.c | 98 ++++++++++++++++------------------------- tests/ovn-controller.at | 14 +++--- 6 files changed, 84 insertions(+), 100 deletions(-)