On Oct 21, 2013, at 3:52 PM, Ben Pfaff <b...@nicira.com> wrote:
> Commit e3b5693319c (Fix table checking for goto table instruction.) moved
> action checking into modify_flows__(), for good reason, but as a side
> effect made modify_flows__() abandon and never commit the ofopgroup that it
> started, if action checking failed. This commit fixes the problem.
>
> The following commands, run under "make sandbox", illustrate the problem.
> Without this change, the final command hangs because the barrier request
> that ovs-ofctl sends never gets a response (because barriers wait for all
> ofopgroups to complete, which never happens). With this commit, the
> commands complete quickly:
>
> ovs-vsctl add-br br0
> ovs-vsctl set bridge br0 protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13
> ovs-ofctl add-flow -O OpenFlow11 br0 table=1,action=mod_tp_dst:79,goto_table:2
> ovs-ofctl add-flow -O OpenFlow11 br0 table=1,action=mod_tp_dst:79,goto_table:1
>
> Reported-by: Jarno Rajahalme <jrajaha...@vmware.com>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> ofproto/ofproto.c | 19 ++++++++++++-------
> tests/ofproto.at | 26 ++++++++++++++++++++++++++
> 2 files changed, 38 insertions(+), 7 deletions(-)
>
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index f67e1fb..8dba732 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -4041,6 +4041,18 @@ modify_flows__(struct ofproto *ofproto, struct ofconn
> *ofconn,
> enum ofperr error;
> size_t i;
>
> + /* Verify actions before we start to modify any rules, to avoid partial
> + * flow table modifications. */
> + for (i = 0; i < rules->n; i++) {
> + struct rule *rule = rules->rules[i];
> +
> + error = ofpacts_check(fm->ofpacts, fm->ofpacts_len, &fm->match.flow,
> + u16_to_ofp(ofproto->max_ports),
> rule->table_id);
> + if (error) {
> + return error;
> + }
> + }
> +
This fixes the problem I had, thank you!
While we are at this, we should use ofproto_check_ofpacts() instead and maybe
avoid repeating the same check over and over again. How about this incremental:
commit f61dcc149af6b132959eefb8a210ea6e99d191cc
Author: Jarno Rajahalme <jrajaha...@nicira.com>
Date: Thu Oct 24 13:56:14 2013 -0700
Fixup.
Use ofproto_check_ofpacts() instead of ofpacts_check() directly.
Avoid repeating the same check.
---
ofproto/ofproto.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 87d9f7a..63d9684 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -4040,16 +4040,23 @@ modify_flows__(struct ofproto *ofproto, struct ofconn
*ofconn,
struct ofopgroup *group;
enum ofperr error;
size_t i;
+ uint8_t last_table = 255; /* Invalid table value. */
/* Verify actions before we start to modify any rules, to avoid partial
- * flow table modifications. */
+ * flow table modifications.
+ *
+ * Rules are collected one table at a time, avoid checking with the same
+ * table_id multiple times. */
for (i = 0; i < rules->n; i++) {
struct rule *rule = rules->rules[i];
-
- error = ofpacts_check(fm->ofpacts, fm->ofpacts_len, &fm->match.flow,
- u16_to_ofp(ofproto->max_ports), rule->table_id);
- if (error) {
- return error;
+ if (rule->table_id != last_table) {
+ error = ofproto_check_ofpacts(ofproto, fm->ofpacts,
+ fm->ofpacts_len, &fm->match.flow,
+ rule->table_id);
+ if (error) {
+ return error;
+ }
+ last_table = rule->table_id;
}
}
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
