[ovs-dev] [threaded-learning 17/25] ofproto: Replace rwlock in struct rule by a mutex.
Ethan Jackson
ethan at nicira.com
Thu Sep 12 02:21:07 UTC 2013
This patch removes a ovs_mutex_destroy(&ofproto_mutex) call from
ofproto_destroy__() perhaps a rebasing error?
Acked-by: Ethan Jackson <ethan at nicira.com>
On Tue, Sep 10, 2013 at 10:27 PM, Ben Pfaff <blp at nicira.com> wrote:
> 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 <blp at nicira.com>
> ---
> ofproto/ofproto-dpif.c | 4 ++--
> ofproto/ofproto-provider.h | 14 +++++++-------
> ofproto/ofproto.c | 45 ++++++++++++++++++++++----------------------
> 3 files changed, 31 insertions(+), 32 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 2e2f571..b0bfb0b 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -4488,10 +4488,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 dd93743..6e77b36 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -230,7 +230,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. */
> @@ -247,10 +247,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
> @@ -258,9 +258,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. */
> @@ -285,7 +285,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 f5bac6a..70aae62 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 *);
> @@ -221,7 +221,7 @@ static enum ofperr modify_flows__(struct ofproto *, struct ofconn *,
> const struct ofp_header *, struct list *);
> 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 *,
> @@ -1119,7 +1119,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);
> @@ -1199,7 +1199,6 @@ ofproto_destroy__(struct ofproto *ofproto)
>
> free(ofproto->vlan_bitmap);
>
> - ovs_mutex_destroy(&ofproto_mutex);
> ofproto->ofproto_class->dealloc(ofproto);
> }
>
> @@ -1768,11 +1767,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;
> }
> @@ -2348,7 +2347,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);
> }
>
> @@ -2964,9 +2963,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);
> @@ -3534,7 +3533,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;
> @@ -3689,7 +3688,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);
> @@ -3786,9 +3785,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);
> @@ -3891,7 +3890,7 @@ delete_flows__(struct ofproto *ofproto, struct ofconn *ofconn,
>
> group = ofopgroup_create(ofproto, ofconn, request, UINT32_MAX);
> LIST_FOR_EACH_SAFE (rule, next, ofproto_node, rules) {
> - ovs_rwlock_wrlock(&rule->rwlock);
> + ovs_mutex_lock(&rule->mutex);
> delete_flow__(rule, group, reason);
> }
> ofopgroup_submit(group);
> @@ -5639,7 +5638,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);
> }
> @@ -5669,10 +5668,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);
> @@ -5861,7 +5860,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;
> }
> @@ -5902,7 +5901,7 @@ ofproto_evict(struct ofproto *ofproto)
> }
>
> if (rule->pending) {
> - ovs_rwlock_unlock(&rule->rwlock);
> + ovs_mutex_unlock(&rule->mutex);
> break;
> }
>
> @@ -6210,7 +6209,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);
>
> @@ -6228,7 +6227,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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list