From patchwork Thu May 2 13:29:09 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ales Musil X-Patchwork-Id: 1930624 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=eY/PzhaY; dkim-atps=neutral Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=2605:bc80:3010::136; helo=smtp3.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=patchwork.ozlabs.org) Received: from smtp3.osuosl.org (smtp3.osuosl.org [IPv6:2605:bc80:3010::136]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (secp384r1) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4VVZXl11n7z1ydX for ; Thu, 2 May 2024 23:29:26 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id 551F560E37; Thu, 2 May 2024 13:29:25 +0000 (UTC) X-Virus-Scanned: amavis at osuosl.org Received: from smtp3.osuosl.org ([127.0.0.1]) by localhost (smtp3.osuosl.org [127.0.0.1]) (amavis, port 10024) with ESMTP id WdZ5KeCRajHw; Thu, 2 May 2024 13:29:22 +0000 (UTC) X-Comment: SPF check N/A for local connections - client-ip=2605:bc80:3010:104::8cd3:938; helo=lists.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver= DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 182C760BC3 Authentication-Results: smtp3.osuosl.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=eY/PzhaY Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [IPv6:2605:bc80:3010:104::8cd3:938]) by smtp3.osuosl.org (Postfix) with ESMTPS id 182C760BC3; Thu, 2 May 2024 13:29:22 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id E5008C007C; Thu, 2 May 2024 13:29:21 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp2.osuosl.org (smtp2.osuosl.org [140.211.166.133]) by lists.linuxfoundation.org (Postfix) with ESMTP id BE02DC007C for ; Thu, 2 May 2024 13:29:19 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id B9A4C402E0 for ; Thu, 2 May 2024 13:29:19 +0000 (UTC) X-Virus-Scanned: amavis at osuosl.org Received: from smtp2.osuosl.org ([127.0.0.1]) by localhost (smtp2.osuosl.org [127.0.0.1]) (amavis, port 10024) with ESMTP id dAvnbEugl3ep for ; Thu, 2 May 2024 13:29:17 +0000 (UTC) Received-SPF: Pass (mailfrom) identity=mailfrom; client-ip=170.10.133.124; helo=us-smtp-delivery-124.mimecast.com; envelope-from=amusil@redhat.com; receiver= DMARC-Filter: OpenDMARC Filter v1.4.2 smtp2.osuosl.org 76D0E40B26 Authentication-Results: smtp2.osuosl.org; dmarc=pass (p=none dis=none) header.from=redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org 76D0E40B26 Authentication-Results: smtp2.osuosl.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=eY/PzhaY Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by smtp2.osuosl.org (Postfix) with ESMTPS id 76D0E40B26 for ; Thu, 2 May 2024 13:29:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1714656556; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=aGEqW6tr6Qu8hLANYxx+qvEr72rCrvNa1Qtz26KvltE=; b=eY/PzhaYoI9jrNPSsopZNAv0q0Se+GKru40YR7C7HVq4que2+dG1PrpD43108IhvPmIF7+ vDfOS3nDK/GjsxDd4rldaoIxKd7L/OvIt3RKON22fzTT+if31vjV36+kowIoIRSDSJgX1F CkOmDedlfUxHnBleQN6RIe/DTToDGtk= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-518-c6E2462-Paa3_zVeWpsYYA-1; Thu, 02 May 2024 09:29:11 -0400 X-MC-Unique: c6E2462-Paa3_zVeWpsYYA-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 5F895188731A; Thu, 2 May 2024 13:29:11 +0000 (UTC) Received: from amusil.redhat.com (unknown [10.45.225.122]) by smtp.corp.redhat.com (Postfix) with ESMTP id F24A240C141D; Thu, 2 May 2024 13:29:09 +0000 (UTC) From: Ales Musil To: dev@openvswitch.org Date: Thu, 2 May 2024 15:29:09 +0200 Message-ID: <20240502132909.1917908-1-amusil@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.2 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Cc: dceara@redhat.com Subject: [ovs-dev] [PATCH ovn v4] controller: Track individual address set constants. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" 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 --- v4: Rebase on top of current main. Update the "lflow_handle_addr_set_update" comment according to suggestion from Han. v3: Rebase on top of current main. Address comments from Han: - Adjust the comment for "lflow_handle_addr_set_update" to include remaning corner cases. - Make sure that the flows are consistent between I-P and recompute. v2: Rebase on top of current main. Adjust the comment for I-P optimization. --- controller/lflow.c | 11 ++--- include/ovn/actions.h | 2 +- include/ovn/expr.h | 46 ++++++++++--------- lib/actions.c | 20 ++++----- lib/expr.c | 99 +++++++++++++++++------------------------ tests/ovn-controller.at | 79 +++++++++++++++++++++++++++++--- 6 files changed, 154 insertions(+), 103 deletions(-) diff --git a/controller/lflow.c b/controller/lflow.c index 760ec0b41..1e05665a1 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -278,7 +278,7 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in, } static bool -as_info_from_expr_const(const char *as_name, const union expr_constant *c, +as_info_from_expr_const(const char *as_name, const struct expr_constant *c, struct addrset_info *as_info) { as_info->name = as_name; @@ -644,14 +644,11 @@ as_update_can_be_handled(const char *as_name, struct addr_set_diff *as_diff, * generated. * * - The sub expression of the address set is combined with other sub- - * expressions/constants, usually because of disjunctions between - * sub-expressions/constants, e.g.: + * expressions/constants on different fields, e.g.: * * ip.src == $as1 || ip.dst == $as2 - * ip.src == {$as1, $as2} - * ip.src == {$as1, ip1} * - * All these could have been split into separate lflows. + * This could have been split into separate lflows. * * - Conjunctions overlapping between lflows, which can be caused by * overlapping address sets or same address set used by multiple lflows @@ -714,7 +711,7 @@ lflow_handle_addr_set_update(const char *as_name, if (as_diff->deleted) { struct addrset_info as_info; for (size_t i = 0; i < as_diff->deleted->n_values; i++) { - union expr_constant *c = &as_diff->deleted->values[i]; + struct expr_constant *c = &as_diff->deleted->values[i]; if (!as_info_from_expr_const(as_name, c, &as_info)) { continue; } diff --git a/include/ovn/actions.h b/include/ovn/actions.h index ae0864fdd..88cf4de79 100644 --- a/include/ovn/actions.h +++ b/include/ovn/actions.h @@ -241,7 +241,7 @@ struct ovnact_next { struct ovnact_load { struct ovnact ovnact; struct expr_field dst; - union expr_constant imm; + struct expr_constant imm; }; /* OVNACT_MOVE, OVNACT_EXCHANGE. */ diff --git a/include/ovn/expr.h b/include/ovn/expr.h index c48f82398..e54edb5bf 100644 --- a/include/ovn/expr.h +++ b/include/ovn/expr.h @@ -368,7 +368,7 @@ bool expr_relop_from_token(enum lex_type type, enum expr_relop *relop); struct expr { struct ovs_list node; /* In parent EXPR_T_AND or EXPR_T_OR if any. */ enum expr_type type; /* Expression type. */ - char *as_name; /* Address set name. Null if it is not an + const char *as_name; /* Address set name. Null if it is not an address set. */ union { @@ -505,40 +505,42 @@ enum expr_constant_type { }; /* A string or integer constant (one must know which from context). */ -union expr_constant { - /* Integer constant. - * - * The width of a constant isn't always clear, e.g. if you write "1", - * there's no way to tell whether you mean for that to be a 1-bit constant - * or a 128-bit constant or somewhere in between. */ - struct { - union mf_subvalue value; - union mf_subvalue mask; /* Only initialized if 'masked'. */ - bool masked; - - enum lex_format format; /* From the constant's lex_token. */ - }; +struct expr_constant { + const char *as_name; - /* Null-terminated string constant. */ - char *string; + union { + /* Integer constant. + * + * The width of a constant isn't always clear, e.g. if you write "1", + * there's no way to tell whether you mean for that to be a 1-bit + * constant or a 128-bit constant or somewhere in between. */ + struct { + union mf_subvalue value; + union mf_subvalue mask; /* Only initialized if 'masked'. */ + bool masked; + + enum lex_format format; /* From the constant's lex_token. */ + }; + + /* Null-terminated string constant. */ + char *string; + }; }; bool expr_constant_parse(struct lexer *, const struct expr_field *, - union expr_constant *); -void expr_constant_format(const union expr_constant *, + struct expr_constant *); +void expr_constant_format(const struct expr_constant *, enum expr_constant_type, struct ds *); -void expr_constant_destroy(const union expr_constant *, +void expr_constant_destroy(const struct expr_constant *, enum expr_constant_type); /* A collection of "union expr_constant"s of the same type. */ struct expr_constant_set { - union expr_constant *values; /* Constants. */ + struct expr_constant *values; /* Constants. */ size_t n_values; /* Number of constants. */ enum expr_constant_type type; /* Type of the constants. */ bool in_curlies; /* Whether the constants were in {}. */ - char *as_name; /* Name of an address set. It is NULL if not - an address set. */ }; bool expr_constant_set_parse(struct lexer *, struct expr_constant_set *); diff --git a/lib/actions.c b/lib/actions.c index 94751d059..cff4f9e3c 100644 --- a/lib/actions.c +++ b/lib/actions.c @@ -450,7 +450,7 @@ encode_LOAD(const struct ovnact_load *load, const struct ovnact_encode_params *ep, struct ofpbuf *ofpacts) { - const union expr_constant *c = &load->imm; + const struct expr_constant *c = &load->imm; struct mf_subfield dst = expr_resolve_field(&load->dst); struct ofpact_set_field *sf = ofpact_put_set_field(ofpacts, dst.field, NULL, NULL); @@ -2077,7 +2077,7 @@ encode_event_empty_lb_backends_opts(struct ofpbuf *ofpacts, /* All empty_lb_backends fields are of type 'str' */ ovs_assert(!strcmp(o->option->type, "str")); - const union expr_constant *c = o->value.values; + const struct expr_constant *c = o->value.values; size_t size = strlen(c->string); struct controller_event_opt_header hdr = (struct controller_event_opt_header) { @@ -2553,7 +2553,7 @@ validate_empty_lb_backends(struct action_context *ctx, { for (size_t i = 0; i < n_options; i++) { const struct ovnact_gen_option *o = &options[i]; - const union expr_constant *c = o->value.values; + const struct expr_constant *c = o->value.values; struct sockaddr_storage ss; struct uuid uuid; @@ -2861,7 +2861,7 @@ encode_put_dhcpv4_option(const struct ovnact_gen_option *o, uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2); opt_header[0] = o->option->code; - const union expr_constant *c = o->value.values; + const struct expr_constant *c = o->value.values; size_t n_values = o->value.n_values; if (!strcmp(o->option->type, "bool") || !strcmp(o->option->type, "uint8")) { @@ -3027,7 +3027,7 @@ encode_put_dhcpv6_option(const struct ovnact_gen_option *o, struct ofpbuf *ofpacts) { struct dhcpv6_opt_header *opt = ofpbuf_put_uninit(ofpacts, sizeof *opt); - const union expr_constant *c = o->value.values; + const struct expr_constant *c = o->value.values; size_t n_values = o->value.n_values; size_t size; @@ -3083,7 +3083,7 @@ encode_PUT_DHCPV4_OPTS(const struct ovnact_put_opts *pdo, find_opt(pdo->options, pdo->n_options, DHCP_OPT_BOOTFILE_CODE); if (boot_opt) { uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2); - const union expr_constant *c = boot_opt->value.values; + const struct expr_constant *c = boot_opt->value.values; opt_header[0] = boot_opt->option->code; opt_header[1] = strlen(c->string); ofpbuf_put(ofpacts, c->string, opt_header[1]); @@ -3093,7 +3093,7 @@ encode_PUT_DHCPV4_OPTS(const struct ovnact_put_opts *pdo, find_opt(pdo->options, pdo->n_options, DHCP_OPT_BOOTFILE_ALT_CODE); if (boot_alt_opt) { uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2); - const union expr_constant *c = boot_alt_opt->value.values; + const struct expr_constant *c = boot_alt_opt->value.values; opt_header[0] = boot_alt_opt->option->code; opt_header[1] = strlen(c->string); ofpbuf_put(ofpacts, c->string, opt_header[1]); @@ -3103,7 +3103,7 @@ encode_PUT_DHCPV4_OPTS(const struct ovnact_put_opts *pdo, pdo->options, pdo->n_options, DHCP_OPT_NEXT_SERVER_CODE); if (next_server_opt) { uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2); - const union expr_constant *c = next_server_opt->value.values; + const struct expr_constant *c = next_server_opt->value.values; opt_header[0] = next_server_opt->option->code; opt_header[1] = sizeof(ovs_be32); ofpbuf_put(ofpacts, &c->value.ipv4, sizeof(ovs_be32)); @@ -3307,7 +3307,7 @@ parse_put_nd_ra_opts(struct action_context *ctx, const struct expr_field *dst, /* Let's validate the options. */ for (size_t i = 0; i < po->n_options; i++) { const struct ovnact_gen_option *o = &po->options[i]; - const union expr_constant *c = o->value.values; + const struct expr_constant *c = o->value.values; if (o->value.n_values > 1) { lexer_error(ctx->lexer, "Invalid value for \"%s\" option", o->option->name); @@ -3490,7 +3490,7 @@ static void encode_put_nd_ra_option(const struct ovnact_gen_option *o, struct ofpbuf *ofpacts, ptrdiff_t ra_offset) { - const union expr_constant *c = o->value.values; + const struct expr_constant *c = o->value.values; switch (o->option->code) { case ND_RA_FLAG_ADDR_MODE: diff --git a/lib/expr.c b/lib/expr.c index 0cb44e1b6..ecf8a6338 100644 --- a/lib/expr.c +++ b/lib/expr.c @@ -179,12 +179,10 @@ expr_combine(enum expr_type type, struct expr *a, struct expr *b) } else { ovs_list_push_back(&a->andor, &b->node); } - free(a->as_name); a->as_name = NULL; return a; } else if (b->type == type) { ovs_list_push_front(&b->andor, &a->node); - free(b->as_name); b->as_name = NULL; return b; } else { @@ -521,12 +519,13 @@ static bool parse_field(struct expr_context *, struct expr_field *); static struct expr * make_cmp__(const struct expr_field *f, enum expr_relop r, - const union expr_constant *c) + const struct expr_constant *c) { struct expr *e = xzalloc(sizeof *e); e->type = EXPR_T_CMP; e->cmp.symbol = f->symbol; e->cmp.relop = r; + e->as_name = c->as_name; if (f->symbol->width) { bitwise_copy(&c->value, sizeof c->value, 0, &e->cmp.value, sizeof e->cmp.value, f->ofs, @@ -547,7 +546,7 @@ make_cmp__(const struct expr_field *f, enum expr_relop r, /* Returns the minimum reasonable width for integer constant 'c'. */ static int -expr_constant_width(const union expr_constant *c) +expr_constant_width(const struct expr_constant *c) { if (c->masked) { return mf_subvalue_width(&c->mask); @@ -674,10 +673,7 @@ make_cmp(struct expr_context *ctx, e = expr_combine(r == EXPR_R_EQ ? EXPR_T_OR : EXPR_T_AND, e, make_cmp__(f, r, &cs->values[i])); } - /* Track address set */ - if (r == EXPR_R_EQ && e->type == EXPR_T_OR && cs->as_name) { - e->as_name = xstrdup(cs->as_name); - } + exit: expr_constant_set_destroy(cs); return e; @@ -797,11 +793,10 @@ parse_addr_sets(struct expr_context *ctx, struct expr_constant_set *cs, } } - struct expr_constant_set *addr_sets - = (ctx->addr_sets - ? shash_find_data(ctx->addr_sets, ctx->lexer->token.s) - : NULL); - if (!addr_sets) { + struct shash_node *node = ctx->addr_sets + ? shash_find(ctx->addr_sets, ctx->lexer->token.s) + : NULL; + if (!node) { lexer_syntax_error(ctx->lexer, "expecting address set name"); return false; } @@ -810,17 +805,16 @@ parse_addr_sets(struct expr_context *ctx, struct expr_constant_set *cs, return false; } - if (!cs->n_values) { - cs->as_name = xstrdup(ctx->lexer->token.s); - } - + struct expr_constant_set *addr_sets = node->data; size_t n_values = cs->n_values + addr_sets->n_values; if (n_values >= *allocated_values) { cs->values = xrealloc(cs->values, n_values * sizeof *cs->values); *allocated_values = n_values; } for (size_t i = 0; i < addr_sets->n_values; i++) { - cs->values[cs->n_values++] = addr_sets->values[i]; + struct expr_constant *c = &cs->values[cs->n_values++]; + *c = addr_sets->values[i]; + c->as_name = node->name; } return true; @@ -859,8 +853,9 @@ parse_port_group(struct expr_context *ctx, struct expr_constant_set *cs, *allocated_values = n_values; } for (size_t i = 0; i < port_group->n_values; i++) { - cs->values[cs->n_values++].string = - xstrdup(port_group->values[i].string); + struct expr_constant *c = &cs->values[cs->n_values++]; + c->string = xstrdup(port_group->values[i].string); + c->as_name = NULL; } return true; @@ -875,11 +870,6 @@ parse_constant(struct expr_context *ctx, struct expr_constant_set *cs, sizeof *cs->values); } - /* Combining other values to the constant set that is tracking an - * address set, so untrack it. */ - free(cs->as_name); - cs->as_name = NULL; - if (ctx->lexer->token.type == LEX_T_TEMPLATE) { lexer_error(ctx->lexer, "Unexpanded template."); return false; @@ -887,7 +877,9 @@ parse_constant(struct expr_context *ctx, struct expr_constant_set *cs, if (!assign_constant_set_type(ctx, cs, EXPR_C_STRING)) { return false; } - cs->values[cs->n_values++].string = xstrdup(ctx->lexer->token.s); + struct expr_constant *c = &cs->values[cs->n_values++]; + c->string = xstrdup(ctx->lexer->token.s); + c->as_name = NULL; lexer_get(ctx->lexer); return true; } else if (ctx->lexer->token.type == LEX_T_INTEGER || @@ -896,13 +888,14 @@ parse_constant(struct expr_context *ctx, struct expr_constant_set *cs, return false; } - union expr_constant *c = &cs->values[cs->n_values++]; + struct expr_constant *c = &cs->values[cs->n_values++]; c->value = ctx->lexer->token.value; c->format = ctx->lexer->token.format; c->masked = ctx->lexer->token.type == LEX_T_MASKED_INTEGER; if (c->masked) { c->mask = ctx->lexer->token.mask; } + c->as_name = NULL; lexer_get(ctx->lexer); return true; } else if (ctx->lexer->token.type == LEX_T_MACRO) { @@ -961,7 +954,7 @@ parse_constant_set(struct expr_context *ctx, struct expr_constant_set *cs) * indeterminate. */ bool expr_constant_parse(struct lexer *lexer, const struct expr_field *f, - union expr_constant *c) + struct expr_constant *c) { if (lexer->error) { return false; @@ -987,7 +980,7 @@ expr_constant_parse(struct lexer *lexer, const struct expr_field *f, /* Appends to 's' a re-parseable representation of constant 'c' with the given * 'type'. */ void -expr_constant_format(const union expr_constant *c, +expr_constant_format(const struct expr_constant *c, enum expr_constant_type type, struct ds *s) { if (type == EXPR_C_STRING) { @@ -1010,7 +1003,7 @@ expr_constant_format(const union expr_constant *c, * * Does not free(c). */ void -expr_constant_destroy(const union expr_constant *c, +expr_constant_destroy(const struct expr_constant *c, enum expr_constant_type type) { if (c && type == EXPR_C_STRING) { @@ -1043,7 +1036,7 @@ expr_constant_set_format(const struct expr_constant_set *cs, struct ds *s) ds_put_char(s, '{'); } - for (const union expr_constant *c = cs->values; + for (const struct expr_constant *c = cs->values; c < &cs->values[cs->n_values]; c++) { if (c != cs->values) { ds_put_cstr(s, ", "); @@ -1067,15 +1060,14 @@ expr_constant_set_destroy(struct expr_constant_set *cs) } } free(cs->values); - free(cs->as_name); } } static int compare_expr_constant_integer_cb(const void *a_, const void *b_) { - const union expr_constant *a = a_; - const union expr_constant *b = b_; + const struct expr_constant *a = a_; + const struct expr_constant *b = b_; int d = memcmp(&a->value, &b->value, sizeof a->value); if (d) { @@ -1114,7 +1106,7 @@ expr_constant_set_create_integers(const char *const *values, size_t n_values) VLOG_WARN("Invalid constant set entry: '%s', token type: %d", values[i], lex.token.type); } else { - union expr_constant *c = &cs->values[cs->n_values++]; + struct expr_constant *c = &cs->values[cs->n_values++]; c->value = lex.token.value; c->format = lex.token.format; c->masked = lex.token.type == LEX_T_MASKED_INTEGER; @@ -1135,7 +1127,7 @@ expr_constant_set_create_integers(const char *const *values, size_t n_values) static void expr_constant_set_add_value(struct expr_constant_set **p_cs, - union expr_constant *c, size_t *allocated) + struct expr_constant *c, size_t *allocated) { struct expr_constant_set *cs = *p_cs; if (!cs) { @@ -1246,7 +1238,7 @@ expr_const_sets_add_strings(struct shash *const_sets, const char *name, values[i], name); continue; } - union expr_constant *c = &cs->values[cs->n_values++]; + struct expr_constant *c = &cs->values[cs->n_values++]; c->string = xstrdup(values[i]); } @@ -1359,7 +1351,7 @@ expr_parse_primary(struct expr_context *ctx, bool *atomic) *atomic = true; - union expr_constant *cst = xzalloc(sizeof *cst); + struct expr_constant *cst = xzalloc(sizeof *cst); cst->format = LEX_F_HEXADECIMAL; cst->masked = false; @@ -1367,7 +1359,6 @@ expr_parse_primary(struct expr_context *ctx, bool *atomic) c.values = cst; c.n_values = 1; c.in_curlies = false; - c.as_name = NULL; return make_cmp(ctx, &f, EXPR_R_NE, &c); } else if (parse_relop(ctx, &r) && parse_constant_set(ctx, &c)) { return make_cmp(ctx, &f, r, &c); @@ -1834,7 +1825,6 @@ expr_symtab_destroy(struct shash *symtab) static struct expr * expr_clone_cmp(struct expr *expr) { - ovs_assert(!expr->as_name); struct expr *new = xmemdup(expr, sizeof *expr); if (!new->cmp.symbol->width) { new->cmp.string = xstrdup(new->cmp.string); @@ -1858,7 +1848,6 @@ expr_clone_andor(struct expr *expr) static struct expr * expr_clone_condition(struct expr *expr) { - ovs_assert(!expr->as_name); struct expr *new = xmemdup(expr, sizeof *expr); new->cond.string = xstrdup(new->cond.string); return new; @@ -1894,8 +1883,6 @@ expr_destroy(struct expr *expr) return; } - free(expr->as_name); - struct expr *sub; switch (expr->type) { @@ -2567,7 +2554,7 @@ crush_or_supersets(struct expr *expr, const struct expr_symbol *symbol) * mask sizes. */ size_t n = ovs_list_size(&expr->andor); struct expr **subs = xmalloc(n * sizeof *subs); - bool modified = false; + bool has_addr_set = false; /* Linked list over the 'subs' array to quickly skip deleted elements, * i.e. the index of the next potentially non-NULL element. */ size_t *next = xmalloc(n * sizeof *next); @@ -2575,6 +2562,9 @@ crush_or_supersets(struct expr *expr, const struct expr_symbol *symbol) size_t i = 0, j, max_n_bits = 0; struct expr *sub; LIST_FOR_EACH (sub, node, &expr->andor) { + if (sub->as_name) { + has_addr_set = true; + } if (symbol->width) { const unsigned long *sub_mask; @@ -2596,14 +2586,14 @@ crush_or_supersets(struct expr *expr, const struct expr_symbol *symbol) next[last] = i; last = i; } else { + /* Remove address set reference from the duplicate. */ + subs[last]->as_name = NULL; expr_destroy(subs[i]); subs[i] = NULL; - modified = true; } } - if (!symbol->width || symbol->level != EXPR_L_ORDINAL - || (!modified && expr->as_name)) { + if (!symbol->width || symbol->level != EXPR_L_ORDINAL || has_addr_set) { /* Not a fully maskable field or this expression is tracking an * address set. Don't try to optimize to preserve address set I-P. */ goto done; @@ -2658,10 +2648,11 @@ crush_or_supersets(struct expr *expr, const struct expr_symbol *symbol) if (expr_bitmap_intersect_check(a_value, a_mask, b_value, b_mask, bit_width) && bitmap_is_superset(b_mask, a_mask, bit_width)) { - /* 'a' is the same expression with a smaller mask. */ + /* 'a' is the same expression with a smaller mask. + * Remove address set reference from the duplicate. */ + a->as_name = NULL; expr_destroy(subs[j]); subs[j] = NULL; - modified = true; /* Shorten the path for the next round. */ if (last) { @@ -2685,12 +2676,6 @@ done: } } - if (modified) { - /* Members modified, so untrack address set. */ - free(expr->as_name); - expr->as_name = NULL; - } - free(next); free(subs); return expr; @@ -3271,10 +3256,10 @@ add_disjunction(const struct expr *or, LIST_FOR_EACH (sub, node, &or->andor) { struct expr_match *match = expr_match_new(m, clause, n_clauses, conj_id); - if (or->as_name) { + if (sub->as_name) { ovs_assert(sub->type == EXPR_T_CMP); ovs_assert(sub->cmp.symbol->width); - match->as_name = xstrdup(or->as_name); + match->as_name = xstrdup(sub->as_name); match->as_ip = sub->cmp.value.ipv6; match->as_mask = sub->cmp.mask.ipv6; } diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index be198e00d..27cec2aec 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -1625,7 +1625,9 @@ priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.8 actions=lo done reprocess_count_new=$(read_counter consider_logical_flow) -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [10 +# First 2 reprocess are due to change from 1 IP in AS to 2. +# Last 5 is due to overlap in IP addresses between as1 and as2. +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [7 ]) # Remove the IPs from as1 and as2, 1 IP each time. @@ -1648,9 +1650,9 @@ for i in $(seq 10); do done reprocess_count_new=$(read_counter consider_logical_flow) -# In this case the as1 and as2 are merged to a single OR expr, so we lose track of -# address set information - can't handle deletion incrementally. -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [10 +# First 5 are due to overlap in IP addresses between as1 and as2. +# Last 2 are due to change from 2 IP in AS to 1. +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [7 ]) OVN_CLEANUP([hv1]) @@ -1817,7 +1819,7 @@ priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.5 actions=co priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.10.10.10 actions=conjunction,2/2) ]) reprocess_count_new=$(read_counter consider_logical_flow) -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1 +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0 ]) # Delete 2 IPs @@ -1837,7 +1839,7 @@ priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3 actions=co priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.10.10.10 actions=conjunction,2/2) ]) reprocess_count_new=$(read_counter consider_logical_flow) -AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [1 +AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [0 ]) OVN_CLEANUP([hv1]) @@ -2906,3 +2908,68 @@ OVN_CLEANUP([hv1],[hv2]) AT_CLEANUP ]) + +AT_SETUP([ovn-controller - AS I-P and recompute consistency]) +AT_KEYWORDS([as-i-p]) + +ovn_start + +net_add n1 +sim_add hv1 +as hv1 +check ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.1 +check ovs-vsctl -- add-port br-int hv1-vif1 -- \ + set interface hv1-vif1 external-ids:iface-id=ls1-lp1 + +check ovn-nbctl ls-add ls1 + +check ovn-nbctl lsp-add ls1 ls1-lp1 \ +-- lsp-set-addresses ls1-lp1 "f0:00:00:00:00:01" + +wait_for_ports_up +ovn-appctl -t ovn-controller vlog/set file:dbg + +# Get the OF table numbers +acl_eval=$(ovn-debug lflow-stage-to-oftable ls_out_acl_eval) +acl_action=$(ovn-debug lflow-stage-to-oftable ls_out_acl_action) + +dp_key=$(printf "%x" $(fetch_column datapath tunnel_key external_ids:name=ls1)) +port_key=$(printf "%x" $(fetch_column port_binding tunnel_key logical_port=ls1-lp1)) + +ovn-nbctl create address_set name=as1 +check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" && ip4.src == $as1' drop +check ovn-nbctl add address_set as1 addresses 10.0.0.0/24 +check ovn-nbctl --wait=hv sync + +AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int table=$acl_eval,reg15=0x$port_key | grep -v reply | awk '{print $7, $8}' | sort], [0], [dnl +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.0/24 actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action) +]) + +check ovn-nbctl add address_set as1 addresses 10.0.0.1 +check ovn-nbctl add address_set as1 addresses 10.0.0.2 +check ovn-nbctl add address_set as1 addresses 10.0.0.3 +check ovn-nbctl add address_set as1 addresses 10.0.0.4 +check ovn-nbctl --wait=hv sync + +AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int table=$acl_eval,reg15=0x$port_key | grep -v reply | awk '{print $7, $8}' | sort], [0], [dnl +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.0/24 actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action) +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.1 actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action) +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.2 actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action) +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.3 actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action) +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.4 actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action) +]) + + +check ovn-appctl inc-engine/recompute + +AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int table=$acl_eval,reg15=0x$port_key | grep -v reply | awk '{print $7, $8}' | sort], [0], [dnl +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.0/24 actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action) +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.1 actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action) +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.2 actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action) +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.3 actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action) +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.4 actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action) +]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP