[ovs-dev] [PATCH v5 REPOST] ofproto: Honour Table Mod settings for table-miss handling

Ben Pfaff blp at nicira.com
Tue Feb 4 01:12:12 UTC 2014


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 <azhou at nicira.com>
> Signed-off-by: Simon Horman <horms at 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



More information about the dev mailing list