From patchwork Fri Oct 16 03:22:53 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 531004 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from archives.nicira.com (li376-54.members.linode.com [96.126.127.54]) by ozlabs.org (Postfix) with ESMTP id 4386F1401F0 for ; Fri, 16 Oct 2015 14:23:03 +1100 (AEDT) Received: from archives.nicira.com (localhost [127.0.0.1]) by archives.nicira.com (Postfix) with ESMTP id 98E7D10BCC; Thu, 15 Oct 2015 20:23:01 -0700 (PDT) X-Original-To: dev@openvswitch.org Delivered-To: dev@openvswitch.org Received: from mx3v1.cudamail.com (mx3.cudamail.com [64.34.241.5]) by archives.nicira.com (Postfix) with ESMTPS id BB74E10BCA for ; Thu, 15 Oct 2015 20:22:59 -0700 (PDT) Received: from bar3.cudamail.com (bar1 [192.168.15.1]) by mx3v1.cudamail.com (Postfix) with ESMTP id 345FF61814D for ; Thu, 15 Oct 2015 21:22:59 -0600 (MDT) X-ASG-Debug-ID: 1444965778-03dd7b10673b200001-byXFYA Received: from mx3-pf3.cudamail.com ([192.168.14.3]) by bar3.cudamail.com with ESMTP id qkutknglzjHeULb6 (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Thu, 15 Oct 2015 21:22:58 -0600 (MDT) X-Barracuda-Envelope-From: blp@nicira.com X-Barracuda-RBL-Trusted-Forwarder: 192.168.14.3 Received: from unknown (HELO mail-pa0-f41.google.com) (209.85.220.41) by mx3-pf3.cudamail.com with ESMTPS (RC4-SHA encrypted); 16 Oct 2015 03:22:58 -0000 Received-SPF: unknown (mx3-pf3.cudamail.com: Multiple SPF records returned) X-Barracuda-RBL-Trusted-Forwarder: 209.85.220.41 Received: by payp3 with SMTP id p3so58753007pay.1 for ; Thu, 15 Oct 2015 20:22:57 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=7lc6RJ+9Gm61KN9luqjHzjvPCvj4WgZUpPpm3PM3p8I=; b=LeVVvc/EzNBYzGNO5A+52jdHhwQlnmngZ23hPrNc7k+saR8T0I1qBEpxP34GFgRasa QpzMfJvRGtQW/bz61o7UM5hQG6ytjhGaOjWeR/pmqQqf6o6aQuTEkTo5u+MYLdJU7meg sipj5V9PIJRkZP6nPBavnF9oWrjqcag2vPNkKan26OHROGc8cUwdHJuSZm0tMocw1+kf jy0V1rQbqKFAk2GOmAED+s6/bg0iKBLaj1bQFNZZGDsqwONRZw2zOfCy/ruk8bU+DQfR cCKFubcQsGT9Y/9QaQaXFrIhmtAaLN74Aa6rpYDMZ9TuCHAJb2kJ/W6oZB1/Px6i0aWy Ibmg== X-Gm-Message-State: ALoCoQlA9uwJyHTtduKkha2/xXpPUA70oniMFXIthpdic2zvSh/BETYWJIK5oIKkVoPhs6fyc7tx X-Received: by 10.68.234.200 with SMTP id ug8mr13982329pbc.13.1444965777625; Thu, 15 Oct 2015 20:22:57 -0700 (PDT) Received: from nicira.com (173-228-112-197.dsl.dynamic.fusionbroadband.com. [173.228.112.197]) by smtp.gmail.com with ESMTPSA id rc5sm18018372pbc.95.2015.10.15.20.22.55 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 15 Oct 2015 20:22:56 -0700 (PDT) Date: Thu, 15 Oct 2015 20:22:53 -0700 X-Barracuda-Apparent-Source-IP: 173.228.112.197 X-CudaMail-Envelope-Sender: blp@nicira.com From: Ben Pfaff To: Justin Pettit X-CudaMail-Whitelist-To: dev@openvswitch.org X-CudaMail-MID: CM-V3-1014090171 X-CudaMail-DTE: 101515 X-CudaMail-Originating-IP: 209.85.220.41 Message-ID: <20151016032253.GA10217@nicira.com> X-ASG-Orig-Subj: [##CM-V3-1014090171##]Re: [ovs-dev] [PATCH 01/23] ovn: Extend logical "next" action to jump to arbitrary flow tables. References: <1444450544-11845-1-git-send-email-blp@nicira.com> <1444450544-11845-2-git-send-email-blp@nicira.com> <66EA2777-5961-46CF-86C7-587BCE4479FF@nicira.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <66EA2777-5961-46CF-86C7-587BCE4479FF@nicira.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-Barracuda-Connect: UNKNOWN[192.168.14.3] X-Barracuda-Start-Time: 1444965778 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://web.cudamail.com:443/cgi-mod/mark.cgi X-ASG-Whitelist: Header =?UTF-8?B?eFwtY3VkYW1haWxcLXdoaXRlbGlzdFwtdG8=?= X-Virus-Scanned: by bsmtpd at cudamail.com X-Barracuda-BRTS-Status: 1 Cc: dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH 01/23] ovn: Extend logical "next" action to jump to arbitrary flow tables. X-BeenThere: dev@openvswitch.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@openvswitch.org Sender: "dev" On Thu, Oct 15, 2015 at 01:46:44PM -0700, Justin Pettit wrote: > > > On Oct 9, 2015, at 9:15 PM, Ben Pfaff wrote: > > > > @@ -30,8 +30,10 @@ struct action_context { > > /* Input. */ > > struct lexer *lexer; /* Lexer for pulling more tokens. */ > > const struct shash *symtab; /* Symbol table. */ > > - uint8_t next_table_id; /* OpenFlow table for 'next' to resubmit. */ > > - uint8_t output_table_id; /* OpenFlow table for 'output' to resubmit. */ > > + uint8_t first_table; /* First OpenFlow table. */ > > + uint8_t n_tables; /* Number of OpenFlow tables. */ > > I think these two descriptions could be a bit misleading, since they > define the range of tables that a "next" action can jump to, but it > sounds like it's the full range of supported OpenFlow tables. OK, that's a good point, thanks. > > + uint8_t cur_table; /* 0 <= cur_table < n_tables. */ > > This member name seems a little confusing, since all the other > "_table" members refer to an OpenFlow port, but this one is an offset. > What about something like "cur_table_offset"? Another option would be > to relabel them things like "*_of_table" (or "*_phy_table") and > "*_log_table". How about ptable and ltable? > > +static void > > +parse_next_action(struct action_context *ctx) > > +{ > > + if (!ctx->n_tables) { > > + action_error(ctx, "\"next\" action not allowed here."); > > + } else if (lexer_match(ctx->lexer, LEX_T_LPAREN)) { > > + int table; > > Similar to above, this is actually a logical table or an offset to an > OpenFlow table. It might be nice to clarify that for later readers. I renamed it "ltable". > This is a nice addition, but I think some of the older phrasing > related to logical and physical tables was a bit clearer than exposing > that they're offsets. > > Acked-by: Justin Pettit Thanks for the review. I guess given the ack you're more or less happy, so here's an incremental that I folded in, and I'll apply this to master in a minute. diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c index 2bc495c..8afddee 100644 --- a/ovn/lib/actions.c +++ b/ovn/lib/actions.c @@ -27,19 +27,24 @@ /* Context maintained during actions_parse(). */ struct action_context { - /* Input. */ +/* Input. */ + struct lexer *lexer; /* Lexer for pulling more tokens. */ - const struct shash *symtab; /* Symbol table. */ - uint8_t first_table; /* First OpenFlow table. */ - uint8_t n_tables; /* Number of OpenFlow tables. */ - uint8_t cur_table; /* 0 <= cur_table < n_tables. */ - uint8_t output_table; /* OpenFlow table for 'output' to resubmit. */ const struct simap *ports; /* Map from port name to number. */ + const struct shash *symtab; /* Symbol table. */ - /* State. */ + /* OVN maps each logical flow table (ltable), one-to-one, onto a physical + * OpenFlow flow table (ptable). These members describe the mapping and + * data related to flow tables. */ + uint8_t n_tables; /* Number of flow tables. */ + uint8_t first_ptable; /* First OpenFlow table. */ + uint8_t cur_ltable; /* 0 <= cur_ltable < n_tables. */ + uint8_t output_ptable; /* OpenFlow table for 'output' to resubmit. */ + +/* State. */ char *error; /* Error, if any, otherwise NULL. */ - /* Output. */ +/* Output. */ struct ofpbuf *ofpacts; /* Actions. */ struct expr *prereqs; /* Prerequisites to apply to match. */ }; @@ -149,9 +154,9 @@ parse_next_action(struct action_context *ctx) if (!ctx->n_tables) { action_error(ctx, "\"next\" action not allowed here."); } else if (lexer_match(ctx->lexer, LEX_T_LPAREN)) { - int table; + int ltable; - if (!action_get_int(ctx, &table)) { + if (!action_get_int(ctx, <able)) { return; } if (!lexer_match(ctx->lexer, LEX_T_RPAREN)) { @@ -159,16 +164,16 @@ parse_next_action(struct action_context *ctx) return; } - if (table >= ctx->n_tables) { + if (ltable >= ctx->n_tables) { action_error(ctx, "\"next\" argument must be in range 0 to %d.", ctx->n_tables - 1); return; } - emit_resubmit(ctx, ctx->first_table + table); + emit_resubmit(ctx, ctx->first_ptable + ltable); } else { - if (ctx->cur_table < ctx->n_tables) { - emit_resubmit(ctx, ctx->first_table + ctx->cur_table + 1); + if (ctx->cur_ltable < ctx->n_tables) { + emit_resubmit(ctx, ctx->first_ptable + ctx->cur_ltable + 1); } else { action_error(ctx, "\"next\" action not allowed in last table."); } @@ -204,7 +209,7 @@ parse_actions(struct action_context *ctx) } else if (lexer_match_id(ctx->lexer, "next")) { parse_next_action(ctx); } else if (lexer_match_id(ctx->lexer, "output")) { - emit_resubmit(ctx, ctx->output_table); + emit_resubmit(ctx, ctx->output_ptable); } else { action_syntax_error(ctx, "expecting action"); } @@ -228,17 +233,23 @@ parse_actions(struct action_context *ctx) * (as one would provide to expr_to_matches()). Strings used in the actions * that are not in 'ports' are translated to zero. * - * 'first_table' and 'n_tables' define the range of OpenFlow tables that the - * logical "next" action should be able to jump to. Logical table 0 maps to - * OpenFlow table 'first_table', logical table 1 to 'first_table + 1', and so - * on. If 'n_tables' is 0 then "next" is disallowed entirely. + * 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: + * + * - '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. * - * 'cur_table' is an offset from 'first_table' (e.g. 0 <= cur_table < n_tables) - * of the logical flow that contains the actions. If cur_table + 1 < n_tables, - * then this defines the default table that "next" will jump to. + * - '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. * - * 'output_table' should be the OpenFlow table to which the "output" action - * will resubmit + * - 'output_ptable' should be the OpenFlow table to which the logical + * "output" action will resubmit * * Some actions add extra requirements (prerequisites) to the flow's match. If * so, this function sets '*prereqsp' to the actions' prerequisites; otherwise, @@ -253,8 +264,8 @@ parse_actions(struct action_context *ctx) char * OVS_WARN_UNUSED_RESULT actions_parse(struct lexer *lexer, const struct shash *symtab, const struct simap *ports, - uint8_t first_table, uint8_t n_tables, uint8_t cur_table, - uint8_t output_table, struct ofpbuf *ofpacts, + uint8_t first_ptable, uint8_t n_tables, uint8_t cur_ltable, + uint8_t output_ptable, struct ofpbuf *ofpacts, struct expr **prereqsp) { size_t ofpacts_start = ofpacts->size; @@ -263,10 +274,10 @@ actions_parse(struct lexer *lexer, const struct shash *symtab, ctx.lexer = lexer; ctx.symtab = symtab; ctx.ports = ports; - ctx.first_table = first_table; + ctx.first_ptable = first_ptable; ctx.n_tables = n_tables; - ctx.cur_table = cur_table; - ctx.output_table = output_table; + ctx.cur_ltable = cur_ltable; + ctx.output_ptable = output_ptable; ctx.error = NULL; ctx.ofpacts = ofpacts; ctx.prereqs = NULL; diff --git a/ovn/lib/actions.h b/ovn/lib/actions.h index ae897d6..a84c05b 100644 --- a/ovn/lib/actions.h +++ b/ovn/lib/actions.h @@ -27,15 +27,15 @@ struct shash; struct simap; char *actions_parse(struct lexer *, const struct shash *symtab, - const struct simap *ports, uint8_t first_table, - uint8_t n_tables, uint8_t cur_table, - uint8_t output_table, struct ofpbuf *ofpacts, + const struct simap *ports, uint8_t first_ptable, + uint8_t n_tables, uint8_t cur_ltable, + uint8_t output_ptable, struct ofpbuf *ofpacts, struct expr **prereqsp) OVS_WARN_UNUSED_RESULT; char *actions_parse_string(const char *s, const struct shash *symtab, - const struct simap *ports, uint8_t first_table, - uint8_t n_tables, uint8_t cur_table, - uint8_t output_table, struct ofpbuf *ofpacts, + const struct simap *ports, uint8_t first_ptable, + uint8_t n_tables, uint8_t cur_ltable, + uint8_t output_ptable, struct ofpbuf *ofpacts, struct expr **prereqsp) OVS_WARN_UNUSED_RESULT;