Almost all classifier users already exclude concurrent modifications,
or are single-threaded, hence the classifier internal mutex can be
removed. Due to this change, ovs-router.c needs a new mutex, which is
added.
Suggested-by: Ben Pfaff <b...@nicira.com>
Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
---
lib/classifier-private.h | 13 ++++++-------
lib/classifier.c | 46 +++++-----------------------------------------
lib/classifier.h | 23 ++++++++++-------------
lib/ovs-router.c | 11 ++++++++++-
tests/test-classifier.c | 11 ++---------
5 files changed, 33 insertions(+), 71 deletions(-)
diff --git a/lib/classifier-private.h b/lib/classifier-private.h
index 4716f2f..c00fdc1 100644
--- a/lib/classifier-private.h
+++ b/lib/classifier-private.h
@@ -28,10 +28,9 @@
/* A set of rules that all have the same fields wildcarded. */
struct cls_subtable {
/* These fields are only used by writers. */
- struct cmap_node cmap_node OVS_GUARDED; /* Within struct classifier's
- * 'subtables_map'. */
- int max_priority OVS_GUARDED; /* Max priority of any rule in subtable. */
- unsigned int max_count OVS_GUARDED; /* Count of max_priority rules. */
+ struct cmap_node cmap_node; /* Within classifier's 'subtables_map'. */
+ int max_priority; /* Max priority of any rule in subtable. */
+ unsigned int max_count; /* Count of max_priority rules. */
/* Accessed by iterators. */
struct rculist rules_list; /* Unordered. */
@@ -62,16 +61,16 @@ struct cls_partition {
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'. */
+ struct tag_tracker tracker; /* Tracks the bits in 'tags'. */
};
/* Internal representation of a rule in a "struct cls_subtable". */
struct cls_match {
/* Accessed by everybody. */
- struct rculist list OVS_GUARDED; /* Identical, lower-priority rules. */
+ struct rculist list; /* Identical, lower-priority rules. */
/* Accessed only by writers. */
- struct cls_partition *partition OVS_GUARDED;
+ struct cls_partition *partition;
/* Accessed by readers interested in wildcarding. */
const int priority; /* Larger numbers are higher priorities. */
diff --git a/lib/classifier.c b/lib/classifier.c
index 5c5ed8a..ff54246 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -56,10 +56,8 @@ cls_match_alloc(struct cls_rule *rule)
static struct cls_subtable *find_subtable(const struct classifier *cls,
const struct minimask *);
static struct cls_subtable *insert_subtable(struct classifier *cls,
- const struct minimask *)
- OVS_REQUIRES(cls->mutex);
-static void destroy_subtable(struct classifier *cls, struct cls_subtable *)
- OVS_REQUIRES(cls->mutex);
+ const struct minimask *);
+static void destroy_subtable(struct classifier *cls, struct cls_subtable *);
static const struct cls_match *find_match_wc(const struct cls_subtable *,
const struct flow *,
@@ -99,9 +97,7 @@ next_rule_in_list_protected(struct cls_match *rule)
return next->priority < rule->priority ? next : NULL;
}
-/* Iterates RULE over HEAD and all of the cls_rules on HEAD->list.
- * Classifier's mutex must be held while iterating, as the list is
- * protoceted by it. */
+/* Iterates RULE over HEAD and all of the cls_rules on HEAD->list. */
#define FOR_EACH_RULE_IN_LIST(RULE, HEAD) \
for ((RULE) = (HEAD); (RULE) != NULL; (RULE) = next_rule_in_list(RULE))
#define FOR_EACH_RULE_IN_LIST_PROTECTED(RULE, HEAD) \
@@ -111,8 +107,7 @@ next_rule_in_list_protected(struct cls_match *rule)
static unsigned int minimask_get_prefix_len(const struct minimask *,
const struct mf_field *);
static void trie_init(struct classifier *cls, int trie_idx,
- const struct mf_field *)
- OVS_REQUIRES(cls->mutex);
+ const struct mf_field *);
static unsigned int trie_lookup(const struct cls_trie *, const struct flow *,
union mf_value *plens);
static unsigned int trie_lookup_value(const rcu_trie_ptr *,
@@ -134,7 +129,6 @@ static bool mask_prefix_bits_set(const struct
flow_wildcards *,
static inline void
cls_rule_init__(struct cls_rule *rule, unsigned int priority)
- OVS_NO_THREAD_SAFETY_ANALYSIS
{
rculist_init(&rule->node);
rule->priority = priority;
@@ -181,7 +175,6 @@ cls_rule_clone(struct cls_rule *dst, const struct cls_rule
*src)
* The caller must eventually destroy 'dst' with cls_rule_destroy(). */
void
cls_rule_move(struct cls_rule *dst, struct cls_rule *src)
- OVS_NO_THREAD_SAFETY_ANALYSIS
{
ovs_assert(!src->cls_match); /* Must not be in a classifier. */
cls_rule_init__(dst, src->priority);
@@ -194,7 +187,6 @@ cls_rule_move(struct cls_rule *dst, struct cls_rule *src)
* ('rule' must not currently be in a classifier.) */
void
cls_rule_destroy(struct cls_rule *rule)
- OVS_NO_THREAD_SAFETY_ANALYSIS
{
ovs_assert(!rule->cls_match); /* Must not be in a classifier. */
@@ -240,10 +232,7 @@ cls_rule_is_catchall(const struct cls_rule *rule)
* rules. */
void
classifier_init(struct classifier *cls, const uint8_t *flow_segments)
- OVS_EXCLUDED(cls->mutex)
{
- ovs_mutex_init(&cls->mutex);
- ovs_mutex_lock(&cls->mutex);
cls->n_rules = 0;
cmap_init(&cls->subtables_map);
pvector_init(&cls->subtables);
@@ -259,7 +248,6 @@ classifier_init(struct classifier *cls, const uint8_t
*flow_segments)
for (int i = 0; i < CLS_MAX_TRIES; i++) {
trie_init(cls, i, NULL);
}
- ovs_mutex_unlock(&cls->mutex);
}
/* Destroys 'cls'. Rules within 'cls', if any, are not freed; this is the
@@ -267,14 +255,12 @@ classifier_init(struct classifier *cls, const uint8_t
*flow_segments)
* May only be called after all the readers have been terminated. */
void
classifier_destroy(struct classifier *cls)
- OVS_EXCLUDED(cls->mutex)
{
if (cls) {
struct cls_partition *partition;
struct cls_subtable *subtable;
int i;
- ovs_mutex_lock(&cls->mutex);
for (i = 0; i < cls->n_tries; i++) {
trie_destroy(&cls->tries[i].root);
}
@@ -290,8 +276,6 @@ classifier_destroy(struct classifier *cls)
cmap_destroy(&cls->partitions);
pvector_destroy(&cls->subtables);
- ovs_mutex_unlock(&cls->mutex);
- ovs_mutex_destroy(&cls->mutex);
}
}
@@ -300,14 +284,12 @@ bool
classifier_set_prefix_fields(struct classifier *cls,
const enum mf_field_id *trie_fields,
unsigned int n_fields)
- OVS_EXCLUDED(cls->mutex)
{
const struct mf_field * new_fields[CLS_MAX_TRIES];
struct mf_bitmap fields = MF_BITMAP_INITIALIZER;
int i, n_tries = 0;
bool changed = false;
- ovs_mutex_lock(&cls->mutex);
for (i = 0; i < n_fields && n_tries < CLS_MAX_TRIES; i++) {
const struct mf_field *field = mf_from_id(trie_fields[i]);
if (field->flow_be32ofs < 0 || field->n_bits % 32) {
@@ -370,17 +352,14 @@ classifier_set_prefix_fields(struct classifier *cls,
}
cls->n_tries = n_tries;
- ovs_mutex_unlock(&cls->mutex);
return true;
}
- ovs_mutex_unlock(&cls->mutex);
return false; /* No change. */
}
static void
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];
struct cls_subtable *subtable;
@@ -422,7 +401,6 @@ classifier_is_empty(const struct classifier *cls)
/* Returns the number of rules in 'cls'. */
int
classifier_count(const struct classifier *cls)
- OVS_NO_THREAD_SAFETY_ANALYSIS
{
/* n_rules is an int, so in the presence of concurrent writers this will
* return either the old or a new value. */
@@ -453,7 +431,6 @@ find_partition(const struct classifier *cls, ovs_be64
metadata, uint32_t hash)
static struct cls_partition *
create_partition(struct classifier *cls, struct cls_subtable *subtable,
ovs_be64 metadata)
- OVS_REQUIRES(cls->mutex)
{
uint32_t hash = hash_metadata(metadata);
struct cls_partition *partition = find_partition(cls, metadata, hash);
@@ -480,7 +457,6 @@ subtable_replace_head_rule(struct classifier *cls
OVS_UNUSED,
struct cls_subtable *subtable,
struct cls_match *head, struct cls_match *new,
uint32_t hash, uint32_t ihash[CLS_MAX_INDICES])
- OVS_REQUIRES(cls->mutex)
{
/* Rule's data is already in the tries. */
@@ -524,7 +500,6 @@ subtable_replace_head_rule(struct classifier *cls
OVS_UNUSED,
* updated. */
const struct cls_rule *
classifier_replace(struct classifier *cls, struct cls_rule *rule)
- OVS_EXCLUDED(cls->mutex)
{
struct cls_subtable *subtable;
struct cls_match *head;
@@ -534,7 +509,6 @@ classifier_replace(struct classifier *cls, struct cls_rule
*rule)
size_t n_rules = 0;
struct cls_match *new = cls_match_alloc(rule);
- ovs_mutex_lock(&cls->mutex);
rule->cls_match = new;
subtable = find_subtable(cls, &rule->match.mask);
@@ -628,7 +602,6 @@ classifier_replace(struct classifier *cls, struct cls_rule
*rule)
/* Return displaced rule. Caller is responsible for keeping it
* around until all threads quiesce. */
- ovs_mutex_unlock(&cls->mutex);
return old;
}
}
@@ -651,7 +624,6 @@ classifier_replace(struct classifier *cls, struct cls_rule
*rule)
/* Nothing was replaced. */
cls->n_rules++;
- ovs_mutex_unlock(&cls->mutex);
return NULL;
}
@@ -678,7 +650,6 @@ classifier_insert(struct classifier *cls, struct cls_rule
*rule)
*/
const struct cls_rule *
classifier_remove(struct classifier *cls, const struct cls_rule *rule)
- OVS_EXCLUDED(cls->mutex)
{
struct cls_partition *partition;
struct cls_match *cls_match;
@@ -690,11 +661,9 @@ classifier_remove(struct classifier *cls, const struct
cls_rule *rule)
uint8_t prev_be32ofs = 0;
size_t n_rules;
- ovs_mutex_lock(&cls->mutex);
cls_match = rule->cls_match;
if (!cls_match) {
- rule = NULL;
- goto unlock; /* Already removed. */
+ return NULL;
}
/* Mark as removed. */
CONST_CAST(struct cls_rule *, rule)->cls_match = NULL;
@@ -791,8 +760,6 @@ check_priority:
free:
ovsrcu_postpone(free, cls_match);
cls->n_rules--;
-unlock:
- ovs_mutex_unlock(&cls->mutex);
return rule;
}
@@ -1126,7 +1093,6 @@ find_subtable(const struct 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 classifier *cls, const struct minimask *mask)
- OVS_REQUIRES(cls->mutex)
{
uint32_t hash = minimask_hash(mask, 0);
struct cls_subtable *subtable;
@@ -1196,7 +1162,6 @@ insert_subtable(struct classifier *cls, const struct
minimask *mask)
/* RCU readers may still access the subtable before it is actually freed. */
static void
destroy_subtable(struct classifier *cls, struct cls_subtable *subtable)
- OVS_REQUIRES(cls->mutex)
{
int i;
@@ -1597,7 +1562,6 @@ trie_node_rcu_realloc(const struct trie_node *node)
return new_node;
}
-/* 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 9a5ca4e..f7ba46c 100644
--- a/lib/classifier.h
+++ b/lib/classifier.h
@@ -216,7 +216,6 @@
#include "cmap.h"
#include "match.h"
#include "meta-flow.h"
-#include "ovs-thread.h"
#include "pvector.h"
#include "rculist.h"
@@ -244,8 +243,7 @@ enum {
/* A flow classifier. */
struct classifier {
- struct ovs_mutex mutex;
- int n_rules OVS_GUARDED; /* Total number of rules. */
+ int n_rules; /* Total number of rules. */
uint8_t n_flow_segments;
uint8_t flow_segments[CLS_MAX_INDICES]; /* Flow segment boundaries to use
* for staged lookup. */
@@ -260,7 +258,7 @@ struct classifier {
struct cls_rule {
struct rculist node; /* In struct cls_subtable 'rules_list'. */
int priority; /* Larger numbers are higher priorities. */
- struct cls_match *cls_match OVS_GUARDED; /* NULL if not in a classifier. */
+ struct cls_match *cls_match; /* NULL if not in a classifier. */
struct minimatch match; /* Matching rule. */
};
@@ -270,42 +268,41 @@ void cls_rule_init_from_minimatch(struct cls_rule *,
const struct minimatch *,
void cls_rule_clone(struct cls_rule *, const struct cls_rule *);
void cls_rule_move(struct cls_rule *dst, struct cls_rule *src);
void cls_rule_destroy(struct cls_rule *);
-
bool cls_rule_equal(const struct cls_rule *, const struct cls_rule *);
uint32_t cls_rule_hash(const struct cls_rule *, uint32_t basis);
-
void cls_rule_format(const struct cls_rule *, struct ds *);
-
bool cls_rule_is_catchall(const struct cls_rule *);
-
bool cls_rule_is_loose_match(const struct cls_rule *rule,
const struct minimatch *criteria);
+/* Constructor/destructor. Must run single-threaded. */
void classifier_init(struct classifier *, const uint8_t *flow_segments);
void classifier_destroy(struct classifier *);
+
+/* Modifiers. Caller MUST exclude concurrent calls from other threads. */
bool classifier_set_prefix_fields(struct classifier *,
const enum mf_field_id *trie_fields,
unsigned int n_trie_fields);
-
-bool classifier_is_empty(const struct classifier *);
-int classifier_count(const struct classifier *);
void classifier_insert(struct classifier *, struct cls_rule *);
const struct cls_rule *classifier_replace(struct classifier *,
struct cls_rule *);
const struct cls_rule *classifier_remove(struct classifier *,
const struct cls_rule *);
+
+/* Lookups. These are RCU protected and may run concurrently with modifiers
+ * and each other. */
const struct cls_rule *classifier_lookup(const struct classifier *,
const struct flow *,
struct flow_wildcards *);
bool classifier_rule_overlaps(const struct classifier *,
const struct cls_rule *);
-
const struct cls_rule *classifier_find_rule_exactly(const struct classifier *,
const struct cls_rule *);
-
const struct cls_rule *classifier_find_match_exactly(const struct classifier *,
const struct match *,
int priority);
+bool classifier_is_empty(const struct classifier *);
+int classifier_count(const struct classifier *);
/* Iteration.
*
diff --git a/lib/ovs-router.c b/lib/ovs-router.c
index 381cdde..0ce68ba 100644
--- a/lib/ovs-router.c
+++ b/lib/ovs-router.c
@@ -35,9 +35,11 @@
#include "packets.h"
#include "ovs-router.h"
#include "ovs-router-linux.h"
+#include "ovs-thread.h"
#include "unixctl.h"
#include "util.h"
+static struct ovs_mutex mutex = OVS_MUTEX_INITIALIZER;
static struct classifier cls;
struct ovs_router_entry {
@@ -114,7 +116,10 @@ ovs_router_insert__(uint8_t priority, ovs_be32 ip_dst,
uint8_t plen,
p->priority = priority;
cls_rule_init(&p->cr, &match, priority); /* Longest prefix matches first.
*/
+ ovs_mutex_lock(&mutex);
cr = classifier_replace(&cls, &p->cr);
+ ovs_mutex_unlock(&mutex);
+
if (cr) {
/* An old rule with the same match was displaced. */
ovsrcu_postpone(rt_entry_free, ovs_router_entry_cast(cr));
@@ -143,9 +148,11 @@ rt_entry_delete(uint8_t priority, ovs_be32 ip_dst, uint8_t
plen)
cr = classifier_find_rule_exactly(&cls, &rule);
if (cr) {
/* Remove it. */
+ ovs_mutex_lock(&mutex);
cr = classifier_remove(&cls, cr);
- if (cr) {
+ ovs_mutex_unlock(&mutex);
+ if (cr) {
ovsrcu_postpone(rt_entry_free, ovs_router_entry_cast(cr));
return true;
}
@@ -257,7 +264,9 @@ ovs_router_flush(void)
CLS_FOR_EACH(rt, cr, &cls) {
if (rt->priority == rt->plen) {
+ ovs_mutex_lock(&mutex);
classifier_remove(&cls, &rt->cr);
+ ovs_mutex_unlock(&mutex);
}
}
}
diff --git a/tests/test-classifier.c b/tests/test-classifier.c
index 5bdb523..273b0b9 100644
--- a/tests/test-classifier.c
+++ b/tests/test-classifier.c
@@ -496,6 +496,7 @@ trie_verify(const rcu_trie_ptr *trie, unsigned int ofs,
unsigned int n_bits)
static void
verify_tries(struct classifier *cls)
+ OVS_NO_THREAD_SAFETY_ANALYSIS
{
unsigned int n_rules = 0;
int i;
@@ -504,14 +505,13 @@ verify_tries(struct classifier *cls)
n_rules += trie_verify(&cls->tries[i].root, 0,
cls->tries[i].field->n_bits);
}
- ovs_mutex_lock(&cls->mutex);
assert(n_rules <= cls->n_rules);
- ovs_mutex_unlock(&cls->mutex);
}
static void
check_tables(const struct classifier *cls, int n_tables, int n_rules,
int n_dups)
+ OVS_NO_THREAD_SAFETY_ANALYSIS
{
const struct cls_subtable *table;
struct test_rule *test_rule;
@@ -543,11 +543,8 @@ check_tables(const struct classifier *cls, int n_tables,
int n_rules,
}
assert(!cmap_is_empty(&table->rules));
-
- ovs_mutex_lock(&cls->mutex);
assert(trie_verify(&table->ports_trie, 0, table->ports_mask_len)
== (table->ports_mask_len ? cmap_count(&table->rules) : 0));
- ovs_mutex_unlock(&cls->mutex);
found_tables++;
CMAP_FOR_EACH (head, cmap_node, &table->rules) {
@@ -564,9 +561,7 @@ check_tables(const struct classifier *cls, int n_tables,
int n_rules,
found_rules++;
RCULIST_FOR_EACH (rule, list, &head->list) {
assert(rule->priority < prev_priority);
- ovs_mutex_lock(&cls->mutex);
assert(rule->priority <= table->max_priority);
- ovs_mutex_unlock(&cls->mutex);
prev_priority = rule->priority;
found_rules++;
@@ -575,10 +570,8 @@ check_tables(const struct classifier *cls, int n_tables,
int n_rules,
== rule->cls_rule);
}
}
- ovs_mutex_lock(&cls->mutex);
assert(table->max_priority == max_priority);
assert(table->max_count == max_count);
- ovs_mutex_unlock(&cls->mutex);
}
assert(found_tables == cmap_count(&cls->subtables_map));
--
1.7.10.4
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
