A rwlock is suitable when one expects long hold times so there is a need
for some parallelism for readers. But when the lock is held briefly, it
makes more sense to use a mutex for two reasons. First, a rwlock is more
complex than a mutex so one would expect it to be more expensive to
acquire. Second, a rwlock is less fair than a mutex: as long as there are
any readers this blocks out writers.
At least, that's the behavior I intuitively expect, and a few looks around
the web suggest that I'm not the only one.
Previously, struct rule's rwlock was held for long periods, so using a
rwlock made sense. Now it is held only briefly, so this commit replaces it
by a mutex.
Signed-off-by: Ben Pfaff <b...@nicira.com>
Acked-by: Ethan Jackson <et...@nicira.com>
---
ofproto/ofproto-dpif.c | 4 ++--
ofproto/ofproto-provider.h | 14 +++++++-------
ofproto/ofproto.c | 44 ++++++++++++++++++++++----------------------
3 files changed, 31 insertions(+), 31 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index c53a744..f5f91dd 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4493,10 +4493,10 @@ rule_dpif_get_actions(const struct rule_dpif *rule)
{
struct rule_actions *actions;
- ovs_rwlock_rdlock(&rule->up.rwlock);
+ ovs_mutex_lock(&rule->up.mutex);
actions = rule->up.actions;
rule_actions_ref(actions);
- ovs_rwlock_unlock(&rule->up.rwlock);
+ ovs_mutex_unlock(&rule->up.mutex);
return actions;
}
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index bd40fbf..4e5b0bf 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -234,7 +234,7 @@ struct rule {
struct ofoperation *pending; /* Operation now in progress, if nonnull. */
ovs_be64 flow_cookie; /* Controller-issued identifier. Guarded by
- rwlock. */
+ mutex. */
struct hindex_node cookie_node OVS_GUARDED_BY(ofproto_mutex);
long long int created; /* Creation time. */
@@ -251,10 +251,10 @@ struct rule {
struct heap_node evg_node; /* In eviction_group's "rules" heap. */
struct eviction_group *eviction_group; /* NULL if not in any group. */
- /* The rwlock is used to protect those elements in struct rule which are
+ /* The mutex is used to protect those elements in struct rule which are
* accessed by multiple threads. The main ofproto code is guaranteed not
- * to change any of the elements "Guarded by rwlock" without holding the
- * writelock.
+ * to change any of the elements "Guarded by mutex" without holding the
+ * lock.
*
* While maintaining a pointer to struct rule, threads are required to hold
* a readlock on the classifier that holds the rule or increment the rule's
@@ -262,9 +262,9 @@ struct rule {
*
* A rule will not be evicted unless its classifier's write lock is
* held. */
- struct ovs_rwlock rwlock;
+ struct ovs_mutex mutex;
- /* Guarded by rwlock. */
+ /* Guarded by mutex. */
struct rule_actions *actions;
struct list meter_list_node; /* In owning meter's 'rules' list. */
@@ -289,7 +289,7 @@ void ofproto_rule_unref(struct rule *);
* =============
*
* A struct rule_actions 'actions' may be accessed without a risk of being
- * freed by code that holds a read-lock or write-lock on 'rule->rwlock' (where
+ * freed by code that holds a read-lock or write-lock on 'rule->mutex' (where
* 'rule' is the rule for which 'rule->actions == actions') or that owns a
* reference to 'actions->ref_count' (or both). */
struct rule_actions {
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 97a2c07..8473559 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -153,10 +153,10 @@ static void oftable_enable_eviction(struct oftable *,
const struct mf_subfield *fields,
size_t n_fields);
-static void oftable_remove_rule(struct rule *rule) OVS_RELEASES(rule->rwlock);
+static void oftable_remove_rule(struct rule *rule) OVS_RELEASES(rule->mutex);
static void oftable_remove_rule__(struct ofproto *ofproto,
struct classifier *cls, struct rule *rule)
- OVS_REQ_WRLOCK(cls->rwlock) OVS_RELEASES(rule->rwlock);
+ OVS_REQ_WRLOCK(cls->rwlock) OVS_RELEASES(rule->mutex);
static void oftable_insert_rule(struct rule *);
/* A set of rules within a single OpenFlow table (oftable) that have the same
@@ -182,7 +182,7 @@ struct eviction_group {
};
static bool choose_rule_to_evict(struct oftable *table, struct rule **rulep)
- OVS_TRY_WRLOCK(true, (*rulep)->rwlock);
+ OVS_TRY_WRLOCK(true, (*rulep)->mutex);
static void ofproto_evict(struct ofproto *);
static uint32_t rule_eviction_priority(struct rule *);
static void eviction_group_add_rule(struct rule *);
@@ -254,7 +254,7 @@ static enum ofperr modify_flows__(struct ofproto *, struct
ofconn *,
const struct rule_collection *);
static void delete_flow__(struct rule *rule, struct ofopgroup *,
enum ofp_flow_removed_reason)
- OVS_RELEASES(rule->rwlock);
+ OVS_RELEASES(rule->mutex);
static enum ofperr add_group(struct ofproto *, struct ofputil_group_mod *);
static bool handle_openflow(struct ofconn *, const struct ofpbuf *);
static enum ofperr handle_flow_mod__(struct ofproto *, struct ofconn *,
@@ -1153,7 +1153,7 @@ ofproto_rule_delete(struct ofproto *ofproto, struct
classifier *cls,
group = ofopgroup_create_unattached(ofproto);
ofoperation_create(group, rule, OFOPERATION_DELETE, OFPRR_DELETE);
- ovs_rwlock_wrlock(&rule->rwlock);
+ ovs_mutex_lock(&rule->mutex);
oftable_remove_rule__(ofproto, cls, rule);
ofproto->ofproto_class->rule_delete(rule);
ofopgroup_submit(group);
@@ -1800,11 +1800,11 @@ ofproto_add_flow(struct ofproto *ofproto, const struct
match *match,
rule = rule_from_cls_rule(classifier_find_match_exactly(
&ofproto->tables[0].cls, match, priority));
if (rule) {
- ovs_rwlock_rdlock(&rule->rwlock);
+ ovs_mutex_lock(&rule->mutex);
must_add = !ofpacts_equal(rule->actions->ofpacts,
rule->actions->ofpacts_len,
ofpacts, ofpacts_len);
- ovs_rwlock_unlock(&rule->rwlock);
+ ovs_mutex_unlock(&rule->mutex);
} else {
must_add = true;
}
@@ -2380,7 +2380,7 @@ ofproto_rule_destroy__(struct rule *rule)
cls_rule_destroy(&rule->cr);
rule_actions_unref(rule->actions);
ovs_mutex_destroy(&rule->timeout_mutex);
- ovs_rwlock_destroy(&rule->rwlock);
+ ovs_mutex_destroy(&rule->mutex);
rule->ofproto->ofproto_class->rule_dealloc(rule);
}
@@ -2996,9 +2996,9 @@ ofproto_rule_change_cookie(struct ofproto *ofproto,
struct rule *rule,
ovs_mutex_lock(&ofproto_mutex);
cookies_remove(ofproto, rule);
- ovs_rwlock_wrlock(&rule->rwlock);
+ ovs_mutex_lock(&rule->mutex);
rule->flow_cookie = new_cookie;
- ovs_rwlock_unlock(&rule->rwlock);
+ ovs_mutex_unlock(&rule->mutex);
cookies_insert(ofproto, rule);
ovs_mutex_unlock(&ofproto_mutex);
@@ -3624,7 +3624,7 @@ evict_rule_from_table(struct ofproto *ofproto, struct
oftable *table)
} else if (!choose_rule_to_evict(table, &rule)) {
return OFPERR_OFPFMFC_TABLE_FULL;
} else if (rule->pending) {
- ovs_rwlock_unlock(&rule->rwlock);
+ ovs_mutex_unlock(&rule->mutex);
return OFPROTO_POSTPONE;
} else {
struct ofopgroup *group;
@@ -3782,7 +3782,7 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
rule->monitor_flags = 0;
rule->add_seqno = 0;
rule->modify_seqno = 0;
- ovs_rwlock_init(&rule->rwlock);
+ ovs_mutex_init(&rule->mutex);
/* Construct rule, initializing derived state. */
error = ofproto->ofproto_class->rule_construct(rule);
@@ -3878,9 +3878,9 @@ modify_flows__(struct ofproto *ofproto, struct ofconn
*ofconn,
op->actions = rule->actions;
new_actions = rule_actions_create(fm->ofpacts, fm->ofpacts_len);
- ovs_rwlock_wrlock(&rule->rwlock);
+ ovs_mutex_lock(&rule->mutex);
rule->actions = new_actions;
- ovs_rwlock_unlock(&rule->rwlock);
+ ovs_mutex_unlock(&rule->mutex);
rule->ofproto->ofproto_class->rule_modify_actions(rule,
reset_counters);
@@ -3995,7 +3995,7 @@ delete_flows__(struct ofproto *ofproto, struct ofconn
*ofconn,
group = ofopgroup_create(ofproto, ofconn, request, UINT32_MAX);
for (i = 0; i < rules->n; i++) {
struct rule *rule = rules->rules[i];
- ovs_rwlock_wrlock(&rule->rwlock);
+ ovs_mutex_lock(&rule->mutex);
delete_flow__(rule, group, reason);
}
ofopgroup_submit(group);
@@ -5764,7 +5764,7 @@ ofopgroup_complete(struct ofopgroup *group)
}
}
} else {
- ovs_rwlock_wrlock(&rule->rwlock);
+ ovs_mutex_lock(&rule->mutex);
oftable_remove_rule(rule);
ofproto_rule_unref(rule);
}
@@ -5794,10 +5794,10 @@ ofopgroup_complete(struct ofopgroup *group)
if (op->actions) {
struct rule_actions *old_actions;
- ovs_rwlock_wrlock(&rule->rwlock);
+ ovs_mutex_lock(&rule->mutex);
old_actions = rule->actions;
rule->actions = op->actions;
- ovs_rwlock_unlock(&rule->rwlock);
+ ovs_mutex_unlock(&rule->mutex);
op->actions = NULL;
rule_actions_unref(old_actions);
@@ -5986,7 +5986,7 @@ choose_rule_to_evict(struct oftable *table, struct rule
**rulep)
struct rule *rule;
HEAP_FOR_EACH (rule, evg_node, &evg->rules) {
- if (!ovs_rwlock_trywrlock(&rule->rwlock)) {
+ if (!ovs_mutex_trylock(&rule->mutex)) {
*rulep = rule;
return true;
}
@@ -6027,7 +6027,7 @@ ofproto_evict(struct ofproto *ofproto)
}
if (rule->pending) {
- ovs_rwlock_unlock(&rule->rwlock);
+ ovs_mutex_unlock(&rule->mutex);
break;
}
@@ -6335,7 +6335,7 @@ oftable_enable_eviction(struct oftable *table,
static void
oftable_remove_rule__(struct ofproto *ofproto, struct classifier *cls,
struct rule *rule)
- OVS_REQ_WRLOCK(cls->rwlock) OVS_RELEASES(rule->rwlock)
+ OVS_REQ_WRLOCK(cls->rwlock) OVS_RELEASES(rule->mutex)
{
classifier_remove(cls, &rule->cr);
@@ -6353,7 +6353,7 @@ oftable_remove_rule__(struct ofproto *ofproto, struct
classifier *cls,
list_remove(&rule->meter_list_node);
list_init(&rule->meter_list_node);
}
- ovs_rwlock_unlock(&rule->rwlock);
+ ovs_mutex_unlock(&rule->mutex);
}
static void
--
1.7.10.4
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
