[ovs-dev,3/8] ofp-table: Ignore bits that have to change according to OpenFlow.

Message ID 20180830200056.15484-3-blp@ovn.org
State Accepted
Headers show
Series
  • [ovs-dev,1/8] vconn: Avoid null dereference on error path.
Related show

Commit Message

Ben Pfaff Aug. 30, 2018, 8 p.m.
OpenFlow table feature replies contain a per-table bitmap that indicates
which tables a flow can point to in goto_table actions.  OpenFlow requires
that a table only be able to go to higher-numbered tables.  This means that
a switch that is general as possible will always have different features
for every table, since each one will have a different bitmap.  This makes
the output of "ovs-ofctl dump-table-features" pretty long and ugly because
it has about 250 entries like this:

  table %d:
    metadata: match=0xffffffffffffffff write=0xffffffffffffffff
    max_entries=%d
    instructions (table miss and others):
      next tables: %d-253
      (same instructions)
      (same actions)
    (same matching)

This commit changes the logic that prints table features messages so that
it considers two sequentially numbered tables to be the same if only the
bit that necessarily must be tunred off changes.  This reduces the hundreds
of entries above to just:

   tables 1...253: ditto

which is so much more readable.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 lib/ofp-table.c  | 66 +++++++++++++++++++++++++++++++++++++++++++++-----------
 tests/ofproto.at | 45 ++++++++++++++------------------------
 2 files changed, 70 insertions(+), 41 deletions(-)

Comments

Justin Pettit Oct. 23, 2018, 9:56 p.m. | #1
> On Aug 30, 2018, at 1:00 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> OpenFlow table feature replies contain a per-table bitmap that indicates
> which tables a flow can point to in goto_table actions.  OpenFlow requires
> that a table only be able to go to higher-numbered tables.  This means that
> a switch that is general as possible will always have different features
> for every table, since each one will have a different bitmap.  This makes
> the output of "ovs-ofctl dump-table-features" pretty long and ugly because
> it has about 250 entries like this:
> 
>  table %d:
>    metadata: match=0xffffffffffffffff write=0xffffffffffffffff
>    max_entries=%d
>    instructions (table miss and others):
>      next tables: %d-253
>      (same instructions)
>      (same actions)
>    (same matching)
> 
> This commit changes the logic that prints table features messages so that
> it considers two sequentially numbered tables to be the same if only the
> bit that necessarily must be tunred off changes.  This reduces the hundreds
> of entries above to just:
> 
>   tables 1...253: ditto
> 
> which is so much more readable.
> 
> Signed-off-by: Ben Pfaff <blp@ovn.org>

Acked-by: Justin Pettit <jpettit@ovn.org>

--Justin

Patch

diff --git a/lib/ofp-table.c b/lib/ofp-table.c
index 71197f644250..e5c8247717d7 100644
--- a/lib/ofp-table.c
+++ b/lib/ofp-table.c
@@ -1329,12 +1329,47 @@  print_table_instruction_features(
     }
 }
 
+/* Compares bitmaps of next tables 'a' and 'b', for tables 'a_table_id' and
+ * 'b_table_id', respectively.  Returns true if the bitmaps are equal.
+ *
+ * The bitmaps are considered equal if b_table_id == a_table_id + 1 and the bit
+ * for 'b_table_id' is set in 'a' but not in 'b'.  This is because OpenFlow
+ * requires that a table not be able to do a goto_table back to its own table
+ * or an earlier one.  Without considering these equivalent, every table will
+ * be different from every one in some way, which just isn't useful in printing
+ * table features. */
+static bool
+table_instruction_features_next_equal(const unsigned long *a, int a_table_id,
+                                      const unsigned long *b, int b_table_id)
+{
+    if (b_table_id == a_table_id + 1
+        && bitmap_is_set(a, b_table_id)
+        && !bitmap_is_set(b, b_table_id)) {
+        for (size_t i = 0; i < BITMAP_N_LONGS(255); i++) {
+            unsigned long diff = a[i] ^ b[i];
+            if (i == b_table_id / BITMAP_ULONG_BITS) {
+                diff &= ~bitmap_bit__(b_table_id);
+            }
+            if (diff) {
+                return false;
+            }
+        }
+        return true;
+    } else if (a_table_id == b_table_id + 1) {
+        return table_instruction_features_next_equal(b, b_table_id,
+                                                     a, a_table_id);
+    } else {
+        return bitmap_equal(a, b, 255);
+    }
+}
+
 static bool
 table_instruction_features_equal(
-    const struct ofputil_table_instruction_features *a,
-    const struct ofputil_table_instruction_features *b)
+    const struct ofputil_table_instruction_features *a, int a_table_id,
+    const struct ofputil_table_instruction_features *b, int b_table_id)
 {
-    return (bitmap_equal(a->next, b->next, 255)
+    return (table_instruction_features_next_equal(a->next, a_table_id,
+                                                  b->next, b_table_id)
             && a->instructions == b->instructions
             && table_action_features_equal(&a->write, &b->write)
             && table_action_features_equal(&a->apply, &b->apply));
@@ -1360,8 +1395,10 @@  table_features_equal(const struct ofputil_table_features *a,
             && a->supports_eviction == b->supports_eviction
             && a->supports_vacancy_events == b->supports_vacancy_events
             && a->max_entries == b->max_entries
-            && table_instruction_features_equal(&a->nonmiss, &b->nonmiss)
-            && table_instruction_features_equal(&a->miss, &b->miss)
+            && table_instruction_features_equal(&a->nonmiss, a->table_id,
+                                                &b->nonmiss, b->table_id)
+            && table_instruction_features_equal(&a->miss, a->table_id,
+                                                &b->miss, b->table_id)
             && bitmap_equal(a->match.bm, b->match.bm, MFF_N_IDS));
 }
 
@@ -1398,22 +1435,25 @@  ofputil_table_features_format(
     const struct ofputil_table_map *table_map,
     int *first_ditto, int *last_ditto)
 {
+    int table = features->table_id;
+    int prev_table = prev_features ? prev_features->table_id : 0;
+
     bool same_stats = !stats || (prev_stats
                                  && table_stats_equal(stats, prev_stats));
     bool same_features = prev_features && table_features_equal(features,
                                                                prev_features);
     if (same_stats && same_features && !features->name[0]) {
         if (*first_ditto < 0) {
-            *first_ditto = features->table_id;
+            *first_ditto = table;
         }
-        *last_ditto = features->table_id;
+        *last_ditto = table;
         return;
     }
     ofputil_table_features_format_finish(s, *first_ditto, *last_ditto);
     *first_ditto = -1;
 
     ds_put_format(s, "\n  table ");
-    ofputil_format_table(features->table_id, table_map, s);
+    ofputil_format_table(table, table_map, s);
     if (features->name[0]) {
         ds_put_format(s, " (\"%s\")", features->name);
     }
@@ -1466,13 +1506,15 @@  ofputil_table_features_format(
     const struct ofputil_table_instruction_features *prev_miss
         = prev_features ? &prev_features->miss : NULL;
     if (prev_features
-        && table_instruction_features_equal(&features->nonmiss, prev_nonmiss)
-        && table_instruction_features_equal(&features->miss, prev_miss)) {
+        && table_instruction_features_equal(&features->nonmiss, table,
+                                            prev_nonmiss, prev_table)
+        && table_instruction_features_equal(&features->miss, table,
+                                            prev_miss, prev_table)) {
         if (!table_instruction_features_empty(&features->nonmiss)) {
             ds_put_cstr(s, "    (same instructions)\n");
         }
-    } else if (!table_instruction_features_equal(&features->nonmiss,
-                                                 &features->miss)) {
+    } else if (!table_instruction_features_equal(&features->nonmiss, table,
+                                                 &features->miss, table)) {
         ds_put_cstr(s, "    instructions (other than table miss):\n");
         print_table_instruction_features(s, &features->nonmiss, prev_nonmiss);
         ds_put_cstr(s, "    instructions (table miss):\n");
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 83da32383b68..2291bfb93225 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -2620,29 +2620,8 @@  metadata in_port in_port_oxm pkt_mark ct_mark ct_label reg0 reg1 reg2 reg3 reg4
 
 ' "$1"
 }
-ditto() {
-    printf '  table %d:
-    metadata: match=0xffffffffffffffff write=0xffffffffffffffff
-    max_entries=%d
-    instructions (table miss and others):
-      next tables: %d-253
-      (same instructions)
-      (same actions)
-    (same matching)
-
-' $1 $2 `expr $1 + 1`
-}
 tail_tables() {
-echo '  table 252:
-    metadata: match=0xffffffffffffffff write=0xffffffffffffffff
-    max_entries=1000000
-    instructions (table miss and others):
-      next tables: 253
-      (same instructions)
-      (same actions)
-    (same matching)
-
-  table 253:
+echo '  table 253:
     metadata: match=0xffffffffffffffff write=0xffffffffffffffff
     max_entries=1000000
     instructions (table miss and others):
@@ -2652,9 +2631,7 @@  echo '  table 252:
 '
 }
 (head_table
- for i in `seq 1 251`; do
-     ditto $i 1000000
- done
+ printf '  tables 1...252: ditto\n\n'
  tail_tables) > expout
 AT_CHECK([ovs-ofctl -O OpenFlow13 dump-table-features br0], [0], [expout])
 # Change the configuration.
@@ -2669,10 +2646,20 @@  AT_CHECK(
 ])
 # Check that the configuration was updated.
 (head_table ' ("main")'
- ditto 1 1024
- for i in `seq 2 251`; do
-     ditto $i 1000000
- done
+ echo '  table 1:
+    metadata: match=0xffffffffffffffff write=0xffffffffffffffff
+    max_entries=1024
+    (same instructions)
+    (same matching)
+
+  table 2:
+    metadata: match=0xffffffffffffffff write=0xffffffffffffffff
+    max_entries=1000000
+    (same instructions)
+    (same matching)
+
+  tables 3...252: ditto
+'
  tail_tables) > expout
 AT_CHECK([ovs-ofctl -O OpenFlow13 dump-table-features br0], [0], [expout])
 OVS_VSWITCHD_STOP