[ovs-dev] [PATCH v4 1/5] classifier: Do not insert duplicate rules in indices.

Ben Pfaff blp at nicira.com
Thu Nov 13 23:50:57 UTC 2014


On Thu, Nov 13, 2014 at 11:56:13AM -0800, Jarno Rajahalme wrote:
> There is no point in adding duplicate information into prefix tries.
> 
> Also, since the lower-priority duplicate rules are not visible to
> lookups, they do not need to be in staged lookup indices directly
> either (the head rule is).
> 
> Finally, now that cmap operations return the number of elements in the
> cmap, subtable's 'n_rules' member is not needed any more.
> 
> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>

Acked-by: Ben Pfaff <blp at nicira.com>

I played with the code a little to improve my understanding of it, and
ended up with the following incremental change.  You're welcome to use
it or ignore it, as you like.

diff --git a/lib/classifier.c b/lib/classifier.c
index 5b01a69..37ac880 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -571,50 +571,49 @@ classifier_replace(struct classifier *cls, struct cls_rule *rule)
             cmap_insert(&subtable->indices[i], &new->index_nodes[i], ihash[i]);
         }
         n_rules = cmap_insert(&subtable->rules, &new->cmap_node, hash);
-
     } else {   /* Equal rules exist in the classifier already. */
         struct cls_match *iter;
-        struct cls_rule *old = NULL;
 
         /* Scan the list for the insertion point that will keep the list in
          * order of decreasing priority. */
         FOR_EACH_RULE_IN_LIST_PROTECTED (iter, head) {
             if (rule->priority >= iter->priority) {
-                if (rule->priority == iter->priority) {
-                    old = CONST_CAST(struct cls_rule *, iter->cls_rule);
-                }
                 break;
             }
         }
 
         /* 'iter' now at the insertion point or NULL it at end. */
-        if (old) {
-            rculist_replace(&new->list, &iter->list);
-        } else {
-            if (!iter) {
-                rculist_push_back(&head->list, &new->list);
+        if (iter) {
+            struct cls_rule *old;
+
+            if (rule->priority == iter->priority) {
+                rculist_replace(&new->list, &iter->list);
+                old = CONST_CAST(struct cls_rule *, iter->cls_rule);
             } else {
-                /* Insert 'rule' right before 'iter'. */
                 rculist_insert(&iter->list, &new->list);
+                old = NULL;
             }
-        }
 
-        /* Replace the existing head in data structures, if rule is the new
-         * head. */
-        if (iter == head) {
-            subtable_replace_head_rule(cls, subtable, head, new, hash, ihash);
-        }
+            /* Replace the existing head in data structures, if rule is the new
+             * head. */
+            if (iter == head) {
+                subtable_replace_head_rule(cls, subtable, head,
+                                           new, hash, ihash);
+            }
 
-        if (old) {
-            ovsrcu_postpone(free, iter);
-            old->cls_match = NULL;
+            if (old) {
+                ovsrcu_postpone(free, iter);
+                old->cls_match = NULL;
 
-            /* No change in subtable's max priority or max count. */
+                /* No change in subtable's max priority or max count. */
 
-            /* Return displaced rule.  Caller is responsible for keeping it
-             * around until all threads quiesce. */
-            ovs_mutex_unlock(&cls->mutex);
-            return old;
+                /* Return displaced rule.  Caller is responsible for keeping it
+                 * around until all threads quiesce. */
+                ovs_mutex_unlock(&cls->mutex);
+                return old;
+            }
+        } else {
+            rculist_push_back(&head->list, &new->list);
         }
     }
 




More information about the dev mailing list