On Sat, Jan 21, 2017 at 11:13 AM, Ben Pfaff <b...@ovn.org> wrote:
> This feature is useful for centralized gateways.
>
> Signed-off-by: Ben Pfaff <b...@ovn.org>
> Acked-by: Mickey Spiegel <mickeys....@gmail.com>
>
The ovn-trace.c changes look good to me. No more comments.
Thank you very much for this patch set!
Mickey
---
> include/ovn/actions.h | 63 ++++++++++++++++++++++++------------------
> ovn/controller/lflow.c | 7 +++--
> ovn/lib/actions.c | 70 ++++++++++++++++++++++++++++++
> ++++++++++-------
> ovn/ovn-sb.xml | 12 ++++++--
> ovn/utilities/ovn-trace.c | 35 ++++++++++++++++++++++--
> tests/ovn.at | 22 ++++++++++++++-
> tests/test-ovn.c | 6 ++--
> 7 files changed, 167 insertions(+), 48 deletions(-)
>
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index f392d03..6691116 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -151,13 +151,15 @@ struct ovnact_next {
> struct ovnact ovnact;
>
> /* Arguments. */
> - uint8_t ltable; /* Logical table ID of next table. */
> + uint8_t ltable; /* Logical table ID of next table. */
> + enum ovnact_pipeline pipeline; /* Pipeline of next table. */
>
> /* Information about the flow that the action is in. This does not
> affect
> * behavior, since the implementation of "next" doesn't depend on the
> * source table or pipeline. It does affect how ovnacts_format()
> prints
> * the action. */
> - uint8_t src_ltable; /* Logical table ID of source table. */
> + uint8_t src_ltable; /* Logical table ID of source
> table. */
> + enum ovnact_pipeline src_pipeline; /* Pipeline of source table. */
> };
>
> /* OVNACT_LOAD. */
> @@ -402,22 +404,26 @@ struct ovnact_parse_params {
> /* hmap of 'struct dhcp_opts_map' to support 'put_dhcpv6_opts'
> action */
> const struct hmap *dhcpv6_opts;
>
> - /* OVN maps each logical flow table (ltable), one-to-one, onto a
> physical
> - * OpenFlow flow table (ptable). A number of parameters describe this
> - * mapping and data related to flow tables:
> + /* Each OVN flow exists in a logical table within a logical pipeline.
> + * These parameters express this context for a set of OVN actions
> being
> + * parsed:
> *
> - * - 'first_ptable' and 'n_tables' define the range of OpenFlow
> tables
> - * to which the logical "next" action should be able to jump.
> - * Logical table 0 maps to OpenFlow table 'first_ptable',
> logical
> - * table 1 to 'first_ptable + 1', and so on. If 'n_tables' is
> 0
> - * then "next" is disallowed entirely.
> + * - 'n_tables' is the number of tables in the logical ingress and
> + * egress pipelines, that is, "next" may specify a table less
> than
> + * or equal to 'n_tables'. If 'n_tables' is 0 then "next" is
> + * disallowed entirely.
> *
> - * - 'cur_ltable' is an offset from 'first_ptable' (e.g. 0 <=
> - * cur_ltable < n_tables) of the logical flow that contains the
> - * actions. If cur_ltable + 1 < n_tables, then this defines the
> - * default table that "next" will jump to. */
> - uint8_t n_tables; /* Number of flow tables. */
> - uint8_t cur_ltable; /* 0 <= cur_ltable < n_tables. */
> + * - 'cur_ltable' is the logical table of the current flow, within
> + * 'pipeline'. If cur_ltable + 1 < n_tables, then this defines
> the
> + * default table that "next" will jump to.
> + *
> + * - 'pipeline' is the logical pipeline. It is the default
> pipeline to
> + * which 'next' will jump. If 'pipeline' is OVNACT_P_EGRESS,
> then
> + * 'next' will also be able to jump into the ingress pipeline,
> but
> + * the reverse is not true. */
> + enum ovnact_pipeline pipeline; /* Logical pipeline. */
> + uint8_t n_tables; /* Number of logical flow tables. */
> + uint8_t cur_ltable; /* 0 <= cur_ltable < n_tables. */
> };
>
> bool ovnacts_parse(struct lexer *, const struct ovnact_parse_params *,
> @@ -448,20 +454,23 @@ struct ovnact_encode_params {
> * OpenFlow flow table (ptable). A number of parameters describe this
> * mapping and data related to flow tables:
> *
> - * - 'first_ptable' and 'n_tables' define the range of OpenFlow
> tables
> - * to which the logical "next" action should be able to jump.
> - * Logical table 0 maps to OpenFlow table 'first_ptable',
> logical
> - * table 1 to 'first_ptable + 1', and so on. If 'n_tables' is
> 0
> - * then "next" is disallowed entirely.
> + * - 'pipeline' is the logical pipeline in which the actions are
> + * executing.
> *
> - * - 'cur_ltable' is an offset from 'first_ptable' (e.g. 0 <=
> - * cur_ltable < n_ptables) of the logical flow that contains the
> - * actions. If cur_ltable + 1 < n_tables, then this defines the
> - * default table that "next" will jump to.
> + * - 'ingress_ptable' is the OpenFlow table that corresponds to
> OVN
> + * ingress table 0.
> + *
> + * - 'egress_ptable' is the OpenFlow table that corresponds to OVN
> + * egress table 0.
> *
> * - 'output_ptable' should be the OpenFlow table to which the
> logical
> - * "output" action will resubmit. */
> - uint8_t first_ptable; /* First OpenFlow table. */
> + * "output" action will resubmit.
> + *
> + * - 'mac_bind_ptable' should be the OpenFlow table used to track
> MAC
> + * bindings. */
> + enum ovnact_pipeline pipeline; /* Logical pipeline. */
> + uint8_t ingress_ptable; /* First OpenFlow ingress table. */
> + uint8_t egress_ptable; /* First OpenFlow egress table. */
> uint8_t output_ptable; /* OpenFlow table for 'output' to
> resubmit. */
> uint8_t mac_bind_ptable; /* OpenFlow table for 'get_arp'/'get_nd'
> to
> resubmit. */
> diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> index 3d7633e..2d9213b 100644
> --- a/ovn/controller/lflow.c
> +++ b/ovn/controller/lflow.c
> @@ -1,4 +1,4 @@
> -/* Copyright (c) 2015, 2016 Nicira, Inc.
> +/* Copyright (c) 2015, 2016, 2017 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -191,6 +191,7 @@ consider_logical_flow(const struct lport_index *lports,
> .dhcp_opts = dhcp_opts,
> .dhcpv6_opts = dhcpv6_opts,
>
> + .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS,
> .n_tables = LOG_PIPELINE_LEN,
> .cur_ltable = lflow->table_id,
> };
> @@ -223,7 +224,9 @@ consider_logical_flow(const struct lport_index *lports,
> .ct_zones = ct_zones,
> .group_table = group_table,
>
> - .first_ptable = first_ptable,
> + .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS,
> + .ingress_ptable = OFTABLE_LOG_INGRESS_PIPELINE,
> + .egress_ptable = OFTABLE_LOG_EGRESS_PIPELINE,
> .output_ptable = output_ptable,
> .mac_bind_ptable = OFTABLE_MAC_BINDING,
> };
> diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
> index a139ff4..186552f 100644
> --- a/ovn/lib/actions.c
> +++ b/ovn/lib/actions.c
> @@ -170,6 +170,15 @@ put_load(uint64_t value, enum mf_field_id dst, int
> ofs, int n_bits,
> bitwise_copy(&n_value, 8, 0, sf->value, sf->field->n_bytes, ofs,
> n_bits);
> bitwise_one(ofpact_set_field_mask(sf), sf->field->n_bytes, ofs,
> n_bits);
> }
> +
> +static uint8_t
> +first_ptable(const struct ovnact_encode_params *ep,
> + enum ovnact_pipeline pipeline)
> +{
> + return (pipeline == OVNACT_P_INGRESS
> + ? ep->ingress_ptable
> + : ep->egress_ptable);
> +}
>
> /* Context maintained during ovnacts_parse(). */
> struct action_context {
> @@ -262,14 +271,48 @@ parse_NEXT(struct action_context *ctx)
> return;
> }
>
> + int pipeline = ctx->pp->pipeline;
> int table = ctx->pp->cur_ltable + 1;
> - if (lexer_match(ctx->lexer, LEX_T_LPAREN)
> - && (!lexer_force_int(ctx->lexer, &table) ||
> - !lexer_force_match(ctx->lexer, LEX_T_RPAREN))) {
> - return;
> + if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {
> + if (lexer_is_int(ctx->lexer)) {
> + lexer_get_int(ctx->lexer, &table);
> + } else {
> + do {
> + if (lexer_match_id(ctx->lexer, "pipeline")) {
> + if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
> + return;
> + }
> + if (lexer_match_id(ctx->lexer, "ingress")) {
> + pipeline = OVNACT_P_INGRESS;
> + } else if (lexer_match_id(ctx->lexer, "egress")) {
> + pipeline = OVNACT_P_EGRESS;
> + } else {
> + lexer_syntax_error(
> + ctx->lexer, "expecting \"ingress\" or
> \"egress\"");
> + return;
> + }
> + } else if (lexer_match_id(ctx->lexer, "table")) {
> + if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS) ||
> + !lexer_force_int(ctx->lexer, &table)) {
> + return;
> + }
> + } else {
> + lexer_syntax_error(ctx->lexer,
> + "expecting \"pipeline\" or
> \"table\"");
> + return;
> + }
> + } while (lexer_match(ctx->lexer, LEX_T_COMMA));
> + }
> + if (!lexer_force_match(ctx->lexer, LEX_T_RPAREN)) {
> + return;
> + }
> }
>
> - if (table >= ctx->pp->n_tables) {
> + if (pipeline == OVNACT_P_EGRESS && ctx->pp->pipeline ==
> OVNACT_P_INGRESS) {
> + lexer_error(ctx->lexer,
> + "\"next\" action cannot advance from ingress to
> egress "
> + "pipeline (use \"output\" action instead)");
> + } else if (table >= ctx->pp->n_tables) {
> lexer_error(ctx->lexer,
> "\"next\" action cannot advance beyond table %d.",
> ctx->pp->n_tables - 1);
> @@ -277,14 +320,21 @@ parse_NEXT(struct action_context *ctx)
> }
>
> struct ovnact_next *next = ovnact_put_NEXT(ctx->ovnacts);
> + next->pipeline = pipeline;
> next->ltable = table;
> + next->src_pipeline = ctx->pp->pipeline;
> next->src_ltable = ctx->pp->cur_ltable;
> }
>
> static void
> format_NEXT(const struct ovnact_next *next, struct ds *s)
> {
> - if (next->ltable != next->src_ltable + 1) {
> + if (next->pipeline != next->src_pipeline) {
> + ds_put_format(s, "next(pipeline=%s, table=%d);",
> + (next->pipeline == OVNACT_P_INGRESS
> + ? "ingress" : "egress"),
> + next->ltable);
> + } else if (next->ltable != next->src_ltable + 1) {
> ds_put_format(s, "next(%d);", next->ltable);
> } else {
> ds_put_cstr(s, "next;");
> @@ -296,7 +346,7 @@ encode_NEXT(const struct ovnact_next *next,
> const struct ovnact_encode_params *ep,
> struct ofpbuf *ofpacts)
> {
> - emit_resubmit(ofpacts, ep->first_ptable + next->ltable);
> + emit_resubmit(ofpacts, first_ptable(ep, next->pipeline) +
> next->ltable);
> }
>
> static void
> @@ -541,7 +591,7 @@ encode_CT_NEXT(const struct ovnact_ct_next *ct_next,
> struct ofpbuf *ofpacts)
> {
> struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
> - ct->recirc_table = ep->first_ptable + ct_next->ltable;
> + ct->recirc_table = first_ptable(ep, ep->pipeline) + ct_next->ltable;
> ct->zone_src.field = ep->is_switch ? mf_from_id(MFF_LOG_CT_ZONE)
> : mf_from_id(MFF_LOG_DNAT_ZONE);
> ct->zone_src.ofs = 0;
> @@ -745,7 +795,7 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
> ofpbuf_pull(ofpacts, ct_offset);
>
> struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
> - ct->recirc_table = cn->ltable + ep->first_ptable;
> + ct->recirc_table = cn->ltable + first_ptable(ep, ep->pipeline);
> if (snat) {
> ct->zone_src.field = mf_from_id(MFF_LOG_SNAT_ZONE);
> } else {
> @@ -889,7 +939,7 @@ encode_CT_LB(const struct ovnact_ct_lb *cl,
> const struct ovnact_encode_params *ep,
> struct ofpbuf *ofpacts)
> {
> - uint8_t recirc_table = cl->ltable + ep->first_ptable;
> + uint8_t recirc_table = cl->ltable + first_ptable(ep, ep->pipeline);
> if (!cl->n_dsts) {
> /* ct_lb without any destinations means that this is an
> established
> * connection and we just need to do a NAT. */
> diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
> index a724ae6..6171f9d 100644
> --- a/ovn/ovn-sb.xml
> +++ b/ovn/ovn-sb.xml
> @@ -956,10 +956,16 @@
>
> <dt><code>next;</code></dt>
> <dt><code>next(<var>table</var>);</code></dt>
> + <dt><code>next(pipeline=<var>pipeline</var>,
> table=<var>table</var>);</code></dt>
> <dd>
> - Executes another logical datapath table as a subroutine. By
> default,
> - the table after the current one is executed. Specify
> - <var>table</var> to jump to a specific table in the same
> pipeline.
> + Executes the given logical datapath <var>table</var> in
> + <var>pipeline</var> as a subroutine. The default
> <var>table</var> is
> + just after the current one. If <var>pipeline</var> is
> specified, it
> + may be <code>ingress</code> or <code>egress</code>; the default
> + <var>pipeline</var> is the one currently executing. Actions in
> the
> + ingress pipeline may not use <code>next</code> to jump into the
> + egress pipeline (use the <code>output</code> instead), but
> + transitions in the opposite direction are allowed.
> </dd>
>
> <dt><code><var>field</var> = <var>constant</var>;</code></dt>
> diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
> index c201775..3212982 100644
> --- a/ovn/utilities/ovn-trace.c
> +++ b/ovn/utilities/ovn-trace.c
> @@ -409,6 +409,16 @@ ovntrace_port_find_by_key(const struct
> ovntrace_datapath *dp,
> return NULL;
> }
>
> +static const char *
> +ovntrace_port_key_to_name(const struct ovntrace_datapath *dp,
> + uint16_t key)
> +{
> + const struct ovntrace_port *port = ovntrace_port_find_by_key(dp, key);
> + return (port ? port->name
> + : !key ? ""
> + : "(unnamed)");
> +}
> +
> static const struct ovntrace_mcgroup *
> ovntrace_mcgroup_find_by_key(const struct ovntrace_datapath *dp,
> uint16_t tunnel_key)
> @@ -673,6 +683,9 @@ read_flows(void)
> .symtab = &symtab,
> .dhcp_opts = &dhcp_opts,
> .dhcpv6_opts = &dhcpv6_opts,
> + .pipeline = (!strcmp(sblf->pipeline, "ingress")
> + ? OVNACT_P_INGRESS
> + : OVNACT_P_EGRESS),
> .n_tables = 16,
> .cur_ltable = sblf->table_id,
> };
> @@ -1162,8 +1175,7 @@ execute_output(const struct ovntrace_datapath *dp,
> struct flow *uflow,
> }
>
> uint16_t in_key = uflow->regs[MFF_LOG_INPORT - MFF_REG0];
> - const struct ovntrace_port *inport = ovntrace_port_find_by_key(dp,
> in_key);
> - const char *inport_name = !in_key ? "" : inport ? inport->name :
> "(unnamed)";
> + const char *inport_name = ovntrace_port_key_to_name(dp, in_key);
> uint32_t flags = uflow->regs[MFF_LOG_FLAGS - MFF_REG0];
> bool allow_loopback = (flags & MLF_ALLOW_LOOPBACK) != 0;
>
> @@ -1365,6 +1377,23 @@ execute_put_dhcp_opts(const struct
> ovnact_put_dhcp_opts *pdo,
> }
>
> static void
> +execute_next(const struct ovnact_next *next,
> + const struct ovntrace_datapath *dp, struct flow *uflow,
> + enum ovnact_pipeline pipeline, struct ovs_list *super)
> +{
> + if (pipeline != next->pipeline) {
> + ovs_assert(next->pipeline == OVNACT_P_INGRESS);
> +
> + uint16_t in_key = uflow->regs[MFF_LOG_INPORT - MFF_REG0];
> + struct ovntrace_node *node = ovntrace_node_append(
> + super, OVNTRACE_NODE_PIPELINE, "ingress(dp=\"%s\",
> inport=\"%s\")",
> + dp->name, ovntrace_port_key_to_name(dp, in_key));
> + super = &node->subs;
> + }
> + trace__(dp, uflow, next->ltable, next->pipeline, super);
> +}
> +
> +static void
> trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
> const struct ovntrace_datapath *dp, struct flow *uflow,
> uint8_t table_id, enum ovnact_pipeline pipeline,
> @@ -1388,7 +1417,7 @@ trace_actions(const struct ovnact *ovnacts, size_t
> ovnacts_len,
> break;
>
> case OVNACT_NEXT:
> - trace__(dp, uflow, ovnact_get_NEXT(a)->ltable, pipeline,
> super);
> + execute_next(ovnact_get_NEXT(a), dp, uflow, pipeline, super);
> break;
>
> case OVNACT_LOAD:
> diff --git a/tests/ovn.at b/tests/ovn.at
> index f71a4af..ecdb2be 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -653,12 +653,32 @@ next(15);
> encodes as resubmit(,31)
>
> next();
> - Syntax error at `)' expecting small integer.
> + Syntax error at `)' expecting "pipeline" or "table".
> next(10;
> Syntax error at `;' expecting `)'.
> next(16);
> "next" action cannot advance beyond table 15.
>
> +next(table=11);
> + formats as next;
> + encodes as resubmit(,27)
> +next(pipeline=ingress);
> + formats as next;
> + encodes as resubmit(,27)
> +next(table=11, pipeline=ingress);
> + formats as next;
> + encodes as resubmit(,27)
> +next(pipeline=ingress, table=11);
> + formats as next;
> + encodes as resubmit(,27)
> +
> +next(pipeline=egress);
> + "next" action cannot advance from ingress to egress pipeline (use
> "output" action instead)
> +
> +next(table=10);
> + formats as next(10);
> + encodes as resubmit(,26)
> +
> # Loading a constant value.
> tcp.dst=80;
> formats as tcp.dst = 80;
> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> index e64f12f..ea7c45a 100644
> --- a/tests/test-ovn.c
> +++ b/tests/test-ovn.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2015, 2016 Nicira, Inc.
> + * Copyright (c) 2015, 2016, 2017 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -1246,7 +1246,9 @@ test_parse_actions(struct ovs_cmdl_context *ctx
> OVS_UNUSED)
> .ct_zones = &ct_zones,
> .group_table = &group_table,
>
> - .first_ptable = 16,
> + .pipeline = OVNACT_P_INGRESS,
> + .ingress_ptable = 16,
> + .egress_ptable = 48,
> .output_ptable = 64,
> .mac_bind_ptable = 65,
> };
> --
> 2.10.2
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
