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