[ovs-dev] [PATCH 1/4] lib/classifier: Unify struct classifier and cls_classifier.

Jarno Rajahalme jrajahalme at nicira.com
Fri Jul 11 11:55:54 UTC 2014


Now that it is clear that struct cls_classifier itself does not
need RCU indirection and pvector is defined in its own header, it
is possible get rid of the indirection from struct classifier to
struct cls_classifier.

Suggested-by: YAMAMOTO Takashi <yamamoto at valinux.co.jp>
Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
---
 lib/classifier.c        |  120 ++++++++++++++---------------------------------
 lib/classifier.h        |   29 ++++++++++--
 tests/test-classifier.c |   35 +++++++-------
 3 files changed, 76 insertions(+), 108 deletions(-)

diff --git a/lib/classifier.c b/lib/classifier.c
index fe38a55..723749a 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -26,52 +26,23 @@
 #include "list.h"
 #include "odp-util.h"
 #include "ofp-util.h"
-#include "ovs-thread.h"
 #include "packets.h"
-#include "pvector.h"
 #include "tag.h"
 #include "util.h"
 #include "vlog.h"
 
 VLOG_DEFINE_THIS_MODULE(classifier);
 
-struct trie_node;
 struct trie_ctx;
 
 /* Ports trie depends on both ports sharing the same ovs_be32. */
 #define TP_PORTS_OFS32 (offsetof(struct flow, tp_src) / 4)
 BUILD_ASSERT_DECL(TP_PORTS_OFS32 == offsetof(struct flow, tp_dst) / 4);
 
-typedef OVSRCU_TYPE(struct trie_node *) rcu_trie_ptr;
-
-/* Prefix trie for a 'field' */
-struct cls_trie {
-    const struct mf_field *field; /* Trie field, or NULL. */
-    rcu_trie_ptr root;            /* NULL if none. */
-};
-
-enum {
-    CLS_MAX_INDICES = 3   /* Maximum number of lookup indices per subtable. */
-};
-
-struct cls_classifier {
-    struct ovs_mutex mutex;
-    int n_rules OVS_GUARDED;        /* Total number of rules. */
-    uint8_t n_flow_segments;
-    uint8_t flow_segments[CLS_MAX_INDICES]; /* Flow segment boundaries to use
-                                             * for staged lookup. */
-    struct cmap subtables_map;      /* Contains "struct cls_subtable"s.  */
-    struct pvector subtables;
-    struct cmap partitions;         /* Contains "struct cls_partition"s. */
-    struct cls_trie tries[CLS_MAX_TRIES]; /* Prefix tries. */
-    unsigned int n_tries;
-};
-
 /* A set of rules that all have the same fields wildcarded. */
 struct cls_subtable {
     /* The fields are only used by writers and iterators. */
-    struct cmap_node cmap_node; /* Within struct cls_classifier
-                                 * 'subtables_map'. */
+    struct cmap_node cmap_node; /* Within struct classifier 'subtables_map'. */
 
     /* The fields are only used by writers. */
     int n_rules OVS_GUARDED;                /* Number of rules, including
@@ -100,8 +71,7 @@ struct cls_subtable {
  * field) with tags for the "cls_subtable"s that contain rules that match that
  * metadata value.  */
 struct cls_partition {
-    struct cmap_node cmap_node; /* In struct cls_classifier's 'partitions'
-                                 * map. */
+    struct cmap_node cmap_node; /* In struct classifier's 'partitions' map. */
     ovs_be64 metadata;          /* metadata value for this partition. */
     tag_type tags;              /* OR of each flow's cls_subtable tag. */
     struct tag_tracker tracker OVS_GUARDED; /* Tracks the bits in 'tags'. */
@@ -143,15 +113,15 @@ cls_match_alloc(struct cls_rule *rule)
     return cls_match;
 }
 
-static struct cls_subtable *find_subtable(const struct cls_classifier *cls,
+static struct cls_subtable *find_subtable(const struct classifier *cls,
                                           const struct minimask *)
     OVS_REQUIRES(cls->mutex);
-static struct cls_subtable *insert_subtable(struct cls_classifier *cls,
+static struct cls_subtable *insert_subtable(struct classifier *cls,
                                             const struct minimask *)
     OVS_REQUIRES(cls->mutex);
-static void destroy_subtable(struct cls_classifier *cls, struct cls_subtable *)
+static void destroy_subtable(struct classifier *cls, struct cls_subtable *)
     OVS_REQUIRES(cls->mutex);
-static struct cls_match *insert_rule(struct cls_classifier *cls,
+static struct cls_match *insert_rule(struct classifier *cls,
                                      struct cls_subtable *, struct cls_rule *)
     OVS_REQUIRES(cls->mutex);
 
@@ -177,7 +147,7 @@ static struct cls_match *next_rule_in_list(struct cls_match *);
 
 static unsigned int minimask_get_prefix_len(const struct minimask *,
                                             const struct mf_field *);
-static void trie_init(struct cls_classifier *cls, int trie_idx,
+static void trie_init(struct classifier *cls, int trie_idx,
                       const struct mf_field *)
     OVS_REQUIRES(cls->mutex);
 static unsigned int trie_lookup(const struct cls_trie *, const struct flow *,
@@ -476,16 +446,11 @@ cls_rule_is_catchall(const struct cls_rule *rule)
 /* Initializes 'cls' as a classifier that initially contains no classification
  * rules. */
 void
-classifier_init(struct classifier *cls_, const uint8_t *flow_segments)
-    OVS_EXCLUDED(cls_->cls->mutex)
+classifier_init(struct classifier *cls, const uint8_t *flow_segments)
+    OVS_EXCLUDED(cls->mutex)
 {
-    struct cls_classifier *cls = xmalloc(sizeof *cls);
-
     ovs_mutex_init(&cls->mutex);
-
     ovs_mutex_lock(&cls->mutex);
-    cls_->cls = cls;
-
     cls->n_rules = 0;
     cmap_init(&cls->subtables_map);
     pvector_init(&cls->subtables);
@@ -508,19 +473,14 @@ classifier_init(struct classifier *cls_, const uint8_t *flow_segments)
  * caller's responsibility.
  * May only be called after all the readers have been terminated. */
 void
-classifier_destroy(struct classifier *cls_)
-    OVS_EXCLUDED(cls_->cls->mutex)
+classifier_destroy(struct classifier *cls)
+    OVS_EXCLUDED(cls->mutex)
 {
-    if (cls_) {
-        struct cls_classifier *cls = cls_->cls;
+    if (cls) {
         struct cls_partition *partition, *next_partition;
         struct cls_subtable *subtable, *next_subtable;
         int i;
 
-        if (!cls) {
-            return;
-        }
-
         ovs_mutex_lock(&cls->mutex);
         for (i = 0; i < cls->n_tries; i++) {
             trie_destroy(&cls->tries[i].root);
@@ -541,7 +501,6 @@ classifier_destroy(struct classifier *cls_)
         pvector_destroy(&cls->subtables);
         ovs_mutex_unlock(&cls->mutex);
         ovs_mutex_destroy(&cls->mutex);
-        free(cls);
     }
 }
 
@@ -550,12 +509,11 @@ BUILD_ASSERT_DECL(MFF_N_IDS <= 64);
 
 /* Set the fields for which prefix lookup should be performed. */
 bool
-classifier_set_prefix_fields(struct classifier *cls_,
+classifier_set_prefix_fields(struct classifier *cls,
                              const enum mf_field_id *trie_fields,
                              unsigned int n_fields)
-    OVS_EXCLUDED(cls_->cls->mutex)
+    OVS_EXCLUDED(cls->mutex)
 {
-    struct cls_classifier *cls = cls_->cls;
     uint64_t fields = 0;
     const struct mf_field * new_fields[CLS_MAX_TRIES];
     int i, n_tries = 0;
@@ -633,8 +591,7 @@ classifier_set_prefix_fields(struct classifier *cls_,
 }
 
 static void
-trie_init(struct cls_classifier *cls, int trie_idx,
-          const struct mf_field *field)
+trie_init(struct classifier *cls, int trie_idx, const struct mf_field *field)
     OVS_REQUIRES(cls->mutex)
 {
     struct cls_trie *trie = &cls->tries[trie_idx];
@@ -675,7 +632,7 @@ trie_init(struct cls_classifier *cls, int trie_idx,
 bool
 classifier_is_empty(const struct classifier *cls)
 {
-    return cmap_is_empty(&cls->cls->subtables_map);
+    return cmap_is_empty(&cls->subtables_map);
 }
 
 /* Returns the number of rules in 'cls'. */
@@ -685,7 +642,7 @@ classifier_count(const struct classifier *cls)
 {
     /* n_rules is an int, so in the presence of concurrent writers this will
      * return either the old or a new value. */
-    return cls->cls->n_rules;
+    return cls->n_rules;
 }
 
 static uint32_t
@@ -696,8 +653,7 @@ hash_metadata(ovs_be64 metadata_)
 }
 
 static struct cls_partition *
-find_partition(const struct cls_classifier *cls, ovs_be64 metadata,
-               uint32_t hash)
+find_partition(const struct classifier *cls, ovs_be64 metadata, uint32_t hash)
 {
     struct cls_partition *partition;
 
@@ -711,7 +667,7 @@ find_partition(const struct cls_classifier *cls, ovs_be64 metadata,
 }
 
 static struct cls_partition *
-create_partition(struct cls_classifier *cls, struct cls_subtable *subtable,
+create_partition(struct classifier *cls, struct cls_subtable *subtable,
                  ovs_be64 metadata)
     OVS_REQUIRES(cls->mutex)
 {
@@ -749,10 +705,9 @@ static inline ovs_be32 minimatch_get_ports(const struct minimatch *match)
  * rule, even rules that cannot have any effect because the new rule matches a
  * superset of their flows and has higher priority. */
 struct cls_rule *
-classifier_replace(struct classifier *cls_, struct cls_rule *rule)
-    OVS_EXCLUDED(cls_->cls->mutex)
+classifier_replace(struct classifier *cls, struct cls_rule *rule)
+    OVS_EXCLUDED(cls->mutex)
 {
-    struct cls_classifier *cls = cls_->cls;
     struct cls_match *old_rule;
     struct cls_subtable *subtable;
     struct cls_rule *old_cls_rule = NULL;
@@ -823,10 +778,9 @@ classifier_insert(struct classifier *cls, struct cls_rule *rule)
  * 'rule' with cls_rule_destroy(), freeing the memory block in which 'rule'
  * resides, etc., as necessary. */
 void
-classifier_remove(struct classifier *cls_, struct cls_rule *rule)
-    OVS_EXCLUDED(cls_->cls->mutex)
+classifier_remove(struct classifier *cls, struct cls_rule *rule)
+    OVS_EXCLUDED(cls->mutex)
 {
-    struct cls_classifier *cls = cls_->cls;
     struct cls_partition *partition;
     struct cls_match *cls_match = rule->cls_match;
     struct cls_match *head;
@@ -944,10 +898,9 @@ trie_ctx_init(struct trie_ctx *ctx, const struct cls_trie *trie)
  * earlier, 'wc' should have been initialized (e.g., by
  * flow_wildcards_init_catchall()). */
 struct cls_rule *
-classifier_lookup(const struct classifier *cls_, const struct flow *flow,
+classifier_lookup(const struct classifier *cls, const struct flow *flow,
                   struct flow_wildcards *wc)
 {
-    struct cls_classifier *cls = cls_->cls;
     const struct cls_partition *partition;
     tag_type tags;
     int64_t best_priority = -1;
@@ -1056,11 +1009,10 @@ find_match_miniflow(const struct cls_subtable *subtable,
  * classifier_lookup() function.  Specifically, it does not implement
  * priorities, instead returning any rule which matches the flow. */
 void
-classifier_lookup_miniflow_batch(const struct classifier *cls_,
+classifier_lookup_miniflow_batch(const struct classifier *cls,
                                  const struct miniflow **flows,
                                  struct cls_rule **rules, size_t len)
 {
-    struct cls_classifier *cls = cls_->cls;
     struct cls_subtable *subtable;
     size_t i, begin = 0;
 
@@ -1094,11 +1046,10 @@ classifier_lookup_miniflow_batch(const struct classifier *cls_,
  * matching criteria as 'target'.  Returns a null pointer if 'cls' doesn't
  * contain an exact match. */
 struct cls_rule *
-classifier_find_rule_exactly(const struct classifier *cls_,
+classifier_find_rule_exactly(const struct classifier *cls,
                              const struct cls_rule *target)
-    OVS_EXCLUDED(cls_->cls->mutex)
+    OVS_EXCLUDED(cls->mutex)
 {
-    struct cls_classifier *cls = cls_->cls;
     struct cls_match *head, *rule;
     struct cls_subtable *subtable;
 
@@ -1149,11 +1100,10 @@ classifier_find_match_exactly(const struct classifier *cls,
  * considered to overlap if both rules have the same priority and a packet
  * could match both. */
 bool
-classifier_rule_overlaps(const struct classifier *cls_,
+classifier_rule_overlaps(const struct classifier *cls,
                          const struct cls_rule *target)
-    OVS_EXCLUDED(cls_->cls->mutex)
+    OVS_EXCLUDED(cls->mutex)
 {
-    struct cls_classifier *cls = cls_->cls;
     struct cls_subtable *subtable;
     int64_t stop_at_priority = (int64_t)target->priority - 1;
 
@@ -1277,7 +1227,7 @@ struct cls_cursor cls_cursor_init(const struct classifier *cls,
     struct cls_rule *cls_rule = NULL;
 
     cursor.safe = safe;
-    cursor.cls = cls->cls;
+    cursor.cls = cls;
     cursor.target = target && !cls_rule_is_catchall(target) ? target : NULL;
 
     /* Find first rule. */
@@ -1358,7 +1308,7 @@ cls_cursor_next(struct cls_cursor *cursor, const struct cls_rule *rule_)
 }
 
 static struct cls_subtable *
-find_subtable(const struct cls_classifier *cls, const struct minimask *mask)
+find_subtable(const struct classifier *cls, const struct minimask *mask)
     OVS_REQUIRES(cls->mutex)
 {
     struct cls_subtable *subtable;
@@ -1374,7 +1324,7 @@ find_subtable(const struct cls_classifier *cls, const struct minimask *mask)
 
 /* The new subtable will be visible to the readers only after this. */
 static struct cls_subtable *
-insert_subtable(struct cls_classifier *cls, const struct minimask *mask)
+insert_subtable(struct classifier *cls, const struct minimask *mask)
     OVS_REQUIRES(cls->mutex)
 {
     uint32_t hash = minimask_hash(mask, 0);
@@ -1437,7 +1387,7 @@ insert_subtable(struct cls_classifier *cls, const struct minimask *mask)
 }
 
 static void
-destroy_subtable(struct cls_classifier *cls, struct cls_subtable *subtable)
+destroy_subtable(struct classifier *cls, struct cls_subtable *subtable)
     OVS_REQUIRES(cls->mutex)
 {
     int i;
@@ -1741,7 +1691,7 @@ find_equal(struct cls_subtable *subtable, const struct miniflow *flow,
  * the subtable until they see the updated partitions.
  */
 static struct cls_match *
-insert_rule(struct cls_classifier *cls, struct cls_subtable *subtable,
+insert_rule(struct classifier *cls, struct cls_subtable *subtable,
             struct cls_rule *new_rule)
     OVS_REQUIRES(cls->mutex)
 {
@@ -1967,7 +1917,7 @@ trie_node_rcu_realloc(const struct trie_node *node)
     return new_node;
 }
 
-/* May only be called while holding the cls_classifier mutex. */
+/* May only be called while holding the classifier mutex. */
 static void
 trie_destroy(rcu_trie_ptr *trie)
 {
diff --git a/lib/classifier.h b/lib/classifier.h
index d0a408b..1159610 100644
--- a/lib/classifier.h
+++ b/lib/classifier.h
@@ -216,23 +216,43 @@
 #include "cmap.h"
 #include "match.h"
 #include "meta-flow.h"
+#include "ovs-thread.h"
+#include "pvector.h"
 
 #ifdef __cplusplus
 extern "C" {
 #endif
 
 /* Classifier internal data structures. */
-struct cls_classifier;
 struct cls_subtable;
 struct cls_match;
 
+struct trie_node;
+typedef OVSRCU_TYPE(struct trie_node *) rcu_trie_ptr;
+
+/* Prefix trie for a 'field' */
+struct cls_trie {
+    const struct mf_field *field; /* Trie field, or NULL. */
+    rcu_trie_ptr root;            /* NULL if none. */
+};
+
 enum {
-    CLS_MAX_TRIES = 3    /* Maximum number of prefix trees per classifier. */
+    CLS_MAX_INDICES = 3,   /* Maximum number of lookup indices per subtable. */
+    CLS_MAX_TRIES = 3      /* Maximum number of prefix trees per classifier. */
 };
 
 /* A flow classifier. */
 struct classifier {
-    struct cls_classifier *cls;
+    struct ovs_mutex mutex;
+    int n_rules OVS_GUARDED;        /* Total number of rules. */
+    uint8_t n_flow_segments;
+    uint8_t flow_segments[CLS_MAX_INDICES]; /* Flow segment boundaries to use
+                                             * for staged lookup. */
+    struct cmap subtables_map;      /* Contains "struct cls_subtable"s.  */
+    struct pvector subtables;
+    struct cmap partitions;         /* Contains "struct cls_partition"s. */
+    struct cls_trie tries[CLS_MAX_TRIES]; /* Prefix tries. */
+    unsigned int n_tries;
 };
 
 /* A rule to be inserted to the classifier. */
@@ -291,7 +311,7 @@ struct cls_rule *classifier_find_match_exactly(const struct classifier *,
 /* Iteration. */
 
 struct cls_cursor {
-    const struct cls_classifier *cls;
+    const struct classifier *cls;
     const struct cls_subtable *subtable;
     const struct cls_rule *target;
     struct cmap_cursor subtables;
@@ -299,7 +319,6 @@ struct cls_cursor {
     bool safe;
 };
 
-
 /* Iteration requires mutual exclusion of writers.  We do this by taking
  * a mutex for the duration of the iteration, except for the
  * 'SAFE' variant, where we release the mutex for the body of the loop. */
diff --git a/tests/test-classifier.c b/tests/test-classifier.c
index 1d5515c..253e9b3 100644
--- a/tests/test-classifier.c
+++ b/tests/test-classifier.c
@@ -432,7 +432,7 @@ compare_classifiers(struct classifier *cls, struct tcls *tcls)
         flow.nw_tos = nw_dscp_values[get_value(&x, N_NW_DSCP_VALUES)];
 
         /* This assertion is here to suppress a GCC 4.9 array-bounds warning */
-        ovs_assert(cls->cls->n_tries <= CLS_MAX_TRIES);
+        ovs_assert(cls->n_tries <= CLS_MAX_TRIES);
 
         cr0 = classifier_lookup(cls, &flow, &wc);
         cr1 = tcls_lookup(tcls, &flow);
@@ -462,7 +462,7 @@ destroy_classifier(struct classifier *cls)
 }
 
 static void
-pvector_verify(struct pvector *pvec)
+pvector_verify(const struct pvector *pvec)
 {
     void *ptr OVS_UNUSED;
     unsigned int priority, prev_priority = UINT_MAX;
@@ -495,9 +495,8 @@ trie_verify(const rcu_trie_ptr *trie, unsigned int ofs, unsigned int n_bits)
 }
 
 static void
-verify_tries(struct classifier *cls_)
+verify_tries(struct classifier *cls)
 {
-    struct cls_classifier *cls = cls_->cls;
     unsigned int n_rules = 0;
     int i;
 
@@ -521,8 +520,8 @@ check_tables(const struct classifier *cls, int n_tables, int n_rules,
     int found_dups = 0;
     int found_rules2 = 0;
 
-    pvector_verify(&cls->cls->subtables);
-    CMAP_FOR_EACH (table, cmap_node, &cls->cls->subtables_map) {
+    pvector_verify(&cls->subtables);
+    CMAP_FOR_EACH (table, cmap_node, &cls->subtables_map) {
         const struct cls_match *head;
         unsigned int max_priority = 0;
         unsigned int max_count = 0;
@@ -530,7 +529,7 @@ check_tables(const struct classifier *cls, int n_tables, int n_rules,
         const struct cls_subtable *iter;
 
         /* Locate the subtable from 'subtables'. */
-        PVECTOR_FOR_EACH (iter, &cls->cls->subtables) {
+        PVECTOR_FOR_EACH (iter, &cls->subtables) {
             if (iter == table) {
                 if (found) {
                     VLOG_ABORT("Subtable %p duplicated in 'subtables'.",
@@ -545,10 +544,10 @@ check_tables(const struct classifier *cls, int n_tables, int n_rules,
 
         assert(!cmap_is_empty(&table->rules));
 
-        ovs_mutex_lock(&cls->cls->mutex);
+        ovs_mutex_lock(&cls->mutex);
         assert(trie_verify(&table->ports_trie, 0, table->ports_mask_len)
                == (table->ports_mask_len ? table->n_rules : 0));
-        ovs_mutex_unlock(&cls->cls->mutex);
+        ovs_mutex_unlock(&cls->mutex);
 
         found_tables++;
         CMAP_FOR_EACH (head, cmap_node, &table->rules) {
@@ -563,7 +562,7 @@ check_tables(const struct classifier *cls, int n_tables, int n_rules,
             }
 
             found_rules++;
-            ovs_mutex_lock(&cls->cls->mutex);
+            ovs_mutex_lock(&cls->mutex);
             LIST_FOR_EACH (rule, list, &head->list) {
                 assert(rule->priority < prev_priority);
                 assert(rule->priority <= table->max_priority);
@@ -571,22 +570,22 @@ check_tables(const struct classifier *cls, int n_tables, int n_rules,
                 prev_priority = rule->priority;
                 found_rules++;
                 found_dups++;
-                ovs_mutex_unlock(&cls->cls->mutex);
+                ovs_mutex_unlock(&cls->mutex);
                 assert(classifier_find_rule_exactly(cls, rule->cls_rule)
                        == rule->cls_rule);
-                ovs_mutex_lock(&cls->cls->mutex);
+                ovs_mutex_lock(&cls->mutex);
             }
-            ovs_mutex_unlock(&cls->cls->mutex);
+            ovs_mutex_unlock(&cls->mutex);
         }
-        ovs_mutex_lock(&cls->cls->mutex);
+        ovs_mutex_lock(&cls->mutex);
         assert(table->max_priority == max_priority);
         assert(table->max_count == max_count);
-        ovs_mutex_unlock(&cls->cls->mutex);
+        ovs_mutex_unlock(&cls->mutex);
     }
 
-    assert(found_tables == cmap_count(&cls->cls->subtables_map));
-    assert(found_tables == pvector_count(&cls->cls->subtables));
-    assert(n_tables == -1 || n_tables == cmap_count(&cls->cls->subtables_map));
+    assert(found_tables == cmap_count(&cls->subtables_map));
+    assert(found_tables == pvector_count(&cls->subtables));
+    assert(n_tables == -1 || n_tables == cmap_count(&cls->subtables_map));
     assert(n_rules == -1 || found_rules == n_rules);
     assert(n_dups == -1 || found_dups == n_dups);
 
-- 
1.7.10.4




More information about the dev mailing list