lib/classifier: Clarify trie_lookup_value().
[cascardo/ovs.git] / lib / classifier.c
index 2646996..140d9e3 100644 (file)
@@ -212,26 +212,25 @@ cls_subtable_cache_push_back(struct cls_subtable_cache *array,
     array->subtables[array->size++] = a;
 }
 
-/* Only for rearranging entries in the same cache. */
+/* Move subtable entry at 'from' to 'to', shifting the elements in between
+ * (including the one at 'to') accordingly. */
 static inline void
-cls_subtable_cache_splice(struct cls_subtable_entry *to,
-                          struct cls_subtable_entry *start,
-                          struct cls_subtable_entry *end)
-{
-    if (to > end) {
-        /* Same as splicing entries to (start) from [end, to). */
-        struct cls_subtable_entry *temp = to;
-        to = start; start = end; end = temp;
-    }
-    if (to < start) {
-        while (start != end) {
-            struct cls_subtable_entry temp = *start;
-
-            memmove(to + 1, to, (start - to) * sizeof *to);
-            *to = temp;
-            start++;
+cls_subtable_cache_move(struct cls_subtable_entry *to,
+                        struct cls_subtable_entry *from)
+{
+    if (to != from) {
+        struct cls_subtable_entry temp = *from;
+
+        if (to > from) {
+            /* Shift entries (from,to] backwards to make space at 'to'. */
+            memmove(from, from + 1, (to - from) * sizeof *to);
+        } else {
+            /* Shift entries [to,from) forward to make space at 'to'. */
+            memmove(to + 1, to, (from - to) * sizeof *to);
         }
-    } /* Else nothing to be done. */
+
+        *to = temp;
+    }
 }
 
 /* Array removal. */
@@ -262,6 +261,125 @@ cls_subtable_cache_remove(struct cls_subtable_cache *array,
          ITER > (ARRAY)->subtables                                  \
              && OVS_LIKELY(SUBTABLE = (--ITER)->subtable);)
 
+static void
+cls_subtable_cache_verify(struct cls_subtable_cache *array)
+{
+    struct cls_subtable *table;
+    struct cls_subtable_entry *iter;
+    unsigned int priority = 0;
+
+    CLS_SUBTABLE_CACHE_FOR_EACH_REVERSE (table, iter, array) {
+        if (iter->max_priority != table->max_priority) {
+            VLOG_WARN("Subtable %p has mismatching priority in cache (%u != %u)",
+                      table, iter->max_priority, table->max_priority);
+        }
+        if (iter->max_priority < priority) {
+            VLOG_WARN("Subtable cache is out of order (%u < %u)",
+                      iter->max_priority, priority);
+        }
+        priority = iter->max_priority;
+    }
+}
+
+static void
+cls_subtable_cache_reset(struct cls_classifier *cls)
+{
+    struct cls_subtable_cache old = cls->subtables_priority;
+    struct cls_subtable *subtable;
+
+    VLOG_WARN("Resetting subtable cache.");
+
+    cls_subtable_cache_verify(&cls->subtables_priority);
+
+    cls_subtable_cache_init(&cls->subtables_priority);
+
+    HMAP_FOR_EACH (subtable, hmap_node, &cls->subtables) {
+        struct cls_match *head;
+        struct cls_subtable_entry elem;
+        struct cls_subtable *table;
+        struct cls_subtable_entry *iter, *from = NULL;
+        unsigned int new_max = 0;
+        unsigned int max_count = 0;
+        bool found;
+
+        /* Verify max_priority. */
+        HMAP_FOR_EACH (head, hmap_node, &subtable->rules) {
+            if (head->priority > new_max) {
+                new_max = head->priority;
+                max_count = 1;
+            } else if (head->priority == new_max) {
+                max_count++;
+            }
+        }
+        if (new_max != subtable->max_priority ||
+            max_count != subtable->max_count) {
+            VLOG_WARN("subtable %p (%u rules) has mismatching max_priority "
+                      "(%u) or max_count (%u). Highest priority found was %u, "
+                      "count: %u",
+                      subtable, subtable->n_rules, subtable->max_priority,
+                      subtable->max_count, new_max, max_count);
+            subtable->max_priority = new_max;
+            subtable->max_count = max_count;
+        }
+
+        /* Locate the subtable from the old cache. */
+        found = false;
+        CLS_SUBTABLE_CACHE_FOR_EACH (table, iter, &old) {
+            if (table == subtable) {
+                if (iter->max_priority != new_max) {
+                    VLOG_WARN("Subtable %p has wrong max priority (%u != %u) "
+                              "in the old cache.",
+                              subtable, iter->max_priority, new_max);
+                }
+                if (found) {
+                    VLOG_WARN("Subtable %p duplicated in the old cache.",
+                              subtable);
+                }
+                found = true;
+            }
+        }
+        if (!found) {
+            VLOG_WARN("Subtable %p not found from the old cache.", subtable);
+        }
+
+        elem.subtable = subtable;
+        elem.tag = subtable->tag;
+        elem.max_priority = subtable->max_priority;
+        cls_subtable_cache_push_back(&cls->subtables_priority, elem);
+
+        /* Possibly move 'subtable' earlier in the priority array.  If
+         * we break out of the loop, then the subtable (at 'from')
+         * should be moved to the position right after the current
+         * element.  If the loop terminates normally, then 'iter' will
+         * be at the first array element and we'll move the subtable
+         * to the front of the array. */
+        CLS_SUBTABLE_CACHE_FOR_EACH_REVERSE (table, iter,
+                                             &cls->subtables_priority) {
+            if (table == subtable) {
+                from = iter; /* Locate the subtable as we go. */
+            } else if (table->max_priority >= new_max) {
+                ovs_assert(from != NULL);
+                iter++; /* After this. */
+                break;
+            }
+        }
+
+        /* Move subtable at 'from' to 'iter'. */
+        cls_subtable_cache_move(iter, from);
+    }
+
+    /* Verify that the old and the new have the same size. */
+    if (old.size != cls->subtables_priority.size) {
+        VLOG_WARN("subtables cache sizes differ: old (%"PRIuSIZE
+                  ") != new (%"PRIuSIZE").",
+                  old.size, cls->subtables_priority.size);
+    }
+
+    cls_subtable_cache_destroy(&old);
+
+    cls_subtable_cache_verify(&cls->subtables_priority);
+}
+
 \f
 /* flow/miniflow/minimask/minimatch utilities.
  * These are only used by the classifier, so place them here to allow
@@ -1431,31 +1549,36 @@ update_subtables_after_insertion(struct cls_classifier *cls,
         ++subtable->max_count;
     } else if (new_priority > subtable->max_priority) {
         struct cls_subtable *table;
-        struct cls_subtable_entry *iter, *subtable_iter = NULL;
+        struct cls_subtable_entry *iter, *from = NULL;
 
         subtable->max_priority = new_priority;
         subtable->max_count = 1;
 
-        /* Possibly move 'subtable' earlier in the priority list.  If we break
-         * out of the loop, then 'subtable_iter' should be moved just before
-         * 'iter'.  If the loop terminates normally, then 'iter' will be the
-         * first list element and we'll move subtable just before that
-         * (e.g. to the front of the list). */
-        CLS_SUBTABLE_CACHE_FOR_EACH_REVERSE (table, iter, &cls->subtables_priority) {
+        /* Possibly move 'subtable' earlier in the priority array.  If
+         * we break out of the loop, then the subtable (at 'from')
+         * should be moved to the position right after the current
+         * element.  If the loop terminates normally, then 'iter' will
+         * be at the first array element and we'll move the subtable
+         * to the front of the array. */
+        CLS_SUBTABLE_CACHE_FOR_EACH_REVERSE (table, iter,
+                                             &cls->subtables_priority) {
             if (table == subtable) {
-                subtable_iter = iter; /* Locate the subtable as we go. */
+                from = iter; /* Locate the subtable as we go. */
                 iter->max_priority = new_priority;
             } else if (table->max_priority >= new_priority) {
-                ovs_assert(subtable_iter != NULL);
-                iter++;
+                if (from == NULL) {
+                    /* Corrupted cache? */
+                    cls_subtable_cache_reset(cls);
+                    VLOG_ABORT("update_subtables_after_insertion(): Subtable priority list corrupted.");
+                    OVS_NOT_REACHED();
+                }
+                iter++; /* After this. */
                 break;
             }
         }
 
-        /* Move 'subtable' just before 'iter' (unless it's already there). */
-        if (iter != subtable_iter) {
-            cls_subtable_cache_splice(iter, subtable_iter, subtable_iter + 1);
-        }
+        /* Move subtable at 'from' to 'iter'. */
+        cls_subtable_cache_move(iter, from);
     }
 }
 
@@ -1477,7 +1600,7 @@ update_subtables_after_removal(struct cls_classifier *cls,
     if (del_priority == subtable->max_priority && --subtable->max_count == 0) {
         struct cls_match *head;
         struct cls_subtable *table;
-        struct cls_subtable_entry *iter, *subtable_iter = NULL;
+        struct cls_subtable_entry *iter, *from = NULL;
 
         subtable->max_priority = 0;
         HMAP_FOR_EACH (head, hmap_node, &subtable->rules) {
@@ -1489,25 +1612,30 @@ update_subtables_after_removal(struct cls_classifier *cls,
             }
         }
 
-        /* Possibly move 'subtable' later in the priority list.  If we break
-         * out of the loop, then 'subtable' should be moved just before that
-         * 'iter'.  If the loop terminates normally, then 'iter' will be the
-         * list head and we'll move subtable just before that (e.g. to the back
-         * of the list). */
+        /* Possibly move 'subtable' later in the priority array.
+         * After the loop the 'iter' will point right after the position
+         * at which the subtable should be moved (either at a subtable
+         * with an equal or lower priority, or just past the array),
+         * so it is decremented once. */
         CLS_SUBTABLE_CACHE_FOR_EACH (table, iter, &cls->subtables_priority) {
             if (table == subtable) {
-                subtable_iter = iter; /* Locate the subtable as we go. */
+                from = iter; /* Locate the subtable as we go. */
                 iter->max_priority = subtable->max_priority;
             } else if (table->max_priority <= subtable->max_priority) {
-                ovs_assert(subtable_iter != NULL);
+                if (from == NULL) {
+                    /* Corrupted cache? */
+                    cls_subtable_cache_reset(cls);
+                    VLOG_ABORT("update_subtables_after_removal(): Subtable priority list corrupted.");
+                    OVS_NOT_REACHED();
+                }
                 break;
             }
         }
+        /* Now at one past the destination. */
+        iter--;
 
-        /* Move 'subtable' just before 'iter' (unless it's already there). */
-        if (iter != subtable_iter) {
-            cls_subtable_cache_splice(iter, subtable_iter, subtable_iter + 1);
-        }
+        /* Move subtable at 'from' to 'iter'. */
+        cls_subtable_cache_move(iter, from);
     }
 }
 
@@ -2006,26 +2134,26 @@ static unsigned int
 trie_lookup_value(const struct trie_node *node, const ovs_be32 value[],
                   unsigned int *checkbits)
 {
-    unsigned int plen = 0, match_len = 0;
+    unsigned int ofs = 0, match_len = 0;
     const struct trie_node *prev = NULL;
 
-    for (; node; prev = node, node = trie_next_node(node, value, plen)) {
+    for (; node; prev = node, node = trie_next_node(node, value, ofs)) {
         unsigned int eqbits;
         /* Check if this edge can be followed. */
-        eqbits = prefix_equal_bits(node->prefix, node->nbits, value, plen);
-        plen += eqbits;
+        eqbits = prefix_equal_bits(node->prefix, node->nbits, value, ofs);
+        ofs += eqbits;
         if (eqbits < node->nbits) { /* Mismatch, nothing more to be found. */
-            /* Bit at offset 'plen' differed. */
-            *checkbits = plen + 1; /* Includes the first mismatching bit. */
+            /* Bit at offset 'ofs' differed. */
+            *checkbits = ofs + 1; /* Includes the first mismatching bit. */
             return match_len;
         }
         /* Full match, check if rules exist at this prefix length. */
         if (node->n_rules > 0) {
-            match_len = plen;
+            match_len = ofs;
         }
     }
     /* Dead end, exclude the other branch if it exists. */
-    *checkbits = !prev || trie_is_leaf(prev) ? plen : plen + 1;
+    *checkbits = !prev || trie_is_leaf(prev) ? ofs : ofs + 1;
     return match_len;
 }