From c7c9a7c8ba4bb5b77d1e2f60975317aa8b3d252e Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Thu, 12 Sep 2013 00:28:49 -0700 Subject: [PATCH] ofproto: Replace rwlock in struct rule by a mutex. 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 Acked-by: Ethan Jackson --- 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 c53a744cc..f5f91dd6a 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 bd40fbf80..4e5b0bf1a 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 2b3f09482..24edc623a 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); @@ -1802,11 +1802,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; } @@ -2382,7 +2382,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); } @@ -2998,9 +2998,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); @@ -3626,7 +3626,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; @@ -3784,7 +3784,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); @@ -3880,9 +3880,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); @@ -3997,7 +3997,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); @@ -5769,7 +5769,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); } @@ -5799,10 +5799,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); @@ -5991,7 +5991,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; } @@ -6032,7 +6032,7 @@ ofproto_evict(struct ofproto *ofproto) } if (rule->pending) { - ovs_rwlock_unlock(&rule->rwlock); + ovs_mutex_unlock(&rule->mutex); break; } @@ -6340,7 +6340,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); @@ -6358,7 +6358,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 -- 2.43.0