On Wed, Jan 29, 2014 at 12:53:03PM +0900, Simon Horman wrote:
> This reworks lookup of rules for both table 0 and table action translation.
> The result is that Table Mod settings, which can alter the miss-behaviour
> of tables, including table 0, on a per-table basis may be honoured.
>
> Previous patches proposed by myself which build on earlier merged patches
> by Andy Zhou implement the ofproto side of Table Mod. So with this patch
> the feature should be complete.
>
> Neither this patch, nor any other patches it builds on, alter the default
> behaviour of Open vSwitch. And in particular the OpenFlow1.1 behaviour is
> the default regardless of which OpenFlow version is negotiated between the
> switch and the controller.
>
> An implementation detail, which lends itself to future work, is the
> handling of OFPTC_TABLE_MISS_CONTINUE. If a table has this behaviour set by
> Table Mod and a miss occurs then a loop is created, skipping to the next
> table. It is quite easy to create a situation where this loop covers ~255
> tables which is very expensive as the lookup for each table involves taking
> locks, amongst other things.
>
> Cc: Andy Zhou <az...@nicira.com>
> Signed-off-by: Simon Horman <ho...@verge.net.au>
This commit reads more naturally to me when we fold in the following.
(I haven't tested it yet to make sure the tests still pass, but I
don't intend this to change behavior.)
Reading through, what really bothers me here is the significant
apparent code duplication between xlate_table_action() and
rule_dpif_lookup(). Do you see a way to avoid that?
I don't think that "resubmit" should honor OFPTC11_TABLE_MISS_*. It
would be pretty unexpected behavior, I think.
I think that we need to work on stats. OpenFlow specifies per-table
lookup and match counts, but it looks like we squash all of them into
a single pair of stats and attribute those to table 0. I guess we
need one miss_rule and one packet_in_rule per table, not just one of
each globally. I don't think that has to happen in this patch,
though.
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 8ecf4ca..fe0ca79 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2972,7 +2972,7 @@ rule_dpif_get_actions(const struct rule_dpif *rule)
}
/* Lookup 'flow' in table 0 of 'ofproto''s classifier.
- * If 'wc' is non-null, sets * the fields that were relevant as part of
+ * If 'wc' is non-null, sets the fields that were relevant as part of
* the lookup. Returns the table_id where a match or miss occurred.
*
* The return value will be zero unless there was a miss and
@@ -3080,16 +3080,13 @@ rule_dpif_lookup_from_table(struct ofproto_dpif
*ofproto,
const struct flow *flow, struct flow_wildcards *wc,
uint8_t *table_id, struct rule_dpif **rule)
{
- while (1) {
+ while (*table_id < ofproto->up.n_tables) {
enum ofp_table_config config;
if (rule_dpif_lookup_in_table(ofproto, flow, wc, *table_id, rule)) {
return RULE_DPIF_LOOKUP_VERDICT_MATCH;
}
- config = table_get_config(&ofproto->up, *table_id);
- config &= OFPTC11_TABLE_MISS_MASK;
-
/* XXX
* This does not take into account different
* behaviour for different OpenFlow versions
@@ -3100,36 +3097,25 @@ rule_dpif_lookup_from_table(struct ofproto_dpif
*ofproto,
*
* Instead the global default is OFPTC_TABLE_MISS_CONTROLLER
* which may be configured globally using Table Mod. */
+ config = table_get_config(&ofproto->up, *table_id);
switch (config & OFPTC11_TABLE_MISS_MASK) {
- case OFPTC11_TABLE_MISS_CONTINUE: {
- uint8_t next_id = *table_id + 1;
-
- if (next_id == TBL_INTERNAL) {
- next_id++;
- }
-
- if (next_id < ofproto->up.n_tables) {
- /* XXX
- * This may get very expensive as each call to
- * rule_dpif_lookup_in_table() and table_get_config() is
- * expensive and this may occur up to approximately
- * ofproto->up.n_tables times. */
- *table_id = next_id;
- continue;
- }
- /* Fall through to OFPTC_TABLE_MISS_CONTROLLER
- * as this is the last table in the pipeline */
- }
+ case OFPTC11_TABLE_MISS_CONTINUE:
+ break;
case OFPTC11_TABLE_MISS_CONTROLLER:
return RULE_DPIF_LOOKUP_VERDICT_CONTROLLER;
case OFPTC11_TABLE_MISS_DROP:
return RULE_DPIF_LOOKUP_VERDICT_DROP;
- default:
- OVS_NOT_REACHED();
+ }
+
+ /* Go on to next table. */
+ ++*table_id;
+ if (*table_id == TBL_INTERNAL) {
+ ++*table_id;
}
}
- OVS_NOT_REACHED();
+ /* We fell off the */
+ return RULE_DPIF_LOOKUP_VERDICT_CONTROLLER;
}
/* Given a port configuration (specified as zero if there's no port), chooses
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
