Acked-by: Ethan Jackson <et...@nicira.com>
On Tue, Sep 10, 2013 at 10:27 PM, Ben Pfaff <b...@nicira.com> wrote:
> I think that ->timeout_mutex and ->mutex were separate because the latter
> (which was actually a rwlock named ->rwlock until recently) was held for
> long periods of time, which meant that having a separate ->timeout_mutex
> reduced lock contention. But ->mutex is now only held briefly, so it seems
> reasonable to just use it.
>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> ofproto/connmgr.c | 4 ++--
> ofproto/ofproto-dpif.c | 4 ++--
> ofproto/ofproto-provider.h | 3 +--
> ofproto/ofproto.c | 50
> +++++++++++++++++++++-----------------------
> 4 files changed, 29 insertions(+), 32 deletions(-)
>
> diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
> index 1edbd59..1700fe6 100644
> --- a/ofproto/connmgr.c
> +++ b/ofproto/connmgr.c
> @@ -1897,10 +1897,10 @@ ofmonitor_report(struct connmgr *mgr, struct rule
> *rule,
> fu.match = &match;
> fu.priority = rule->cr.priority;
>
> - ovs_mutex_lock(&rule->timeout_mutex);
> + ovs_mutex_lock(&rule->mutex);
> fu.idle_timeout = rule->idle_timeout;
> fu.hard_timeout = rule->hard_timeout;
> - ovs_mutex_unlock(&rule->timeout_mutex);
> + ovs_mutex_unlock(&rule->mutex);
>
> if (flags & NXFMF_ACTIONS) {
> fu.ofpacts = rule->actions->ofpacts;
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index b0bfb0b..f661035 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -3909,10 +3909,10 @@ rule_expire(struct rule_dpif *rule)
>
> ovs_assert(!rule->up.pending);
>
> - ovs_mutex_lock(&rule->up.timeout_mutex);
> + ovs_mutex_lock(&rule->up.mutex);
> hard_timeout = rule->up.hard_timeout;
> idle_timeout = rule->up.idle_timeout;
> - ovs_mutex_unlock(&rule->up.timeout_mutex);
> + ovs_mutex_unlock(&rule->up.mutex);
>
> /* Has 'rule' expired? */
> now = time_msec();
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 6e77b36..3684b0c 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -239,7 +239,6 @@ struct rule {
> uint8_t table_id; /* Index in ofproto's 'tables' array. */
> bool send_flow_removed; /* Send a flow removed message? */
>
> - struct ovs_mutex timeout_mutex;
> uint16_t hard_timeout OVS_GUARDED; /* In seconds from ->modified. */
> uint16_t idle_timeout OVS_GUARDED; /* In seconds from ->used. */
>
> @@ -325,7 +324,7 @@ void ofproto_rule_delete(struct ofproto *, struct
> classifier *cls,
> struct rule *) OVS_REQ_WRLOCK(cls->rwlock);
> void ofproto_rule_reduce_timeouts(struct rule *rule, uint16_t idle_timeout,
> uint16_t hard_timeout)
> - OVS_EXCLUDED(ofproto_mutex, rule->timeout_mutex);
> + OVS_EXCLUDED(ofproto_mutex, rule->mutex);
>
> bool ofproto_rule_has_out_port(const struct rule *, ofp_port_t out_port);
>
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 70aae62..c325cef 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -2346,7 +2346,6 @@ ofproto_rule_destroy__(struct rule *rule)
> {
> cls_rule_destroy(&rule->cr);
> rule_actions_unref(rule->actions);
> - ovs_mutex_destroy(&rule->timeout_mutex);
> ovs_mutex_destroy(&rule->mutex);
> rule->ofproto->ofproto_class->rule_dealloc(rule);
> }
> @@ -3253,10 +3252,10 @@ handle_flow_stats_request(struct ofconn *ofconn,
> fs.ofpacts = rule->actions->ofpacts;
> fs.ofpacts_len = rule->actions->ofpacts_len;
>
> - ovs_mutex_lock(&rule->timeout_mutex);
> + ovs_mutex_lock(&rule->mutex);
> fs.idle_timeout = rule->idle_timeout;
> fs.hard_timeout = rule->hard_timeout;
> - ovs_mutex_unlock(&rule->timeout_mutex);
> + ovs_mutex_unlock(&rule->mutex);
>
> fs.flags = 0;
> if (rule->send_flow_removed) {
> @@ -3673,11 +3672,11 @@ add_flow(struct ofproto *ofproto, struct ofconn
> *ofconn,
> rule->flow_cookie = fm->new_cookie;
> rule->created = rule->modified = rule->used = time_msec();
>
> - ovs_mutex_init(&rule->timeout_mutex);
> - ovs_mutex_lock(&rule->timeout_mutex);
> + ovs_mutex_init(&rule->mutex);
> + ovs_mutex_lock(&rule->mutex);
> rule->idle_timeout = fm->idle_timeout;
> rule->hard_timeout = fm->hard_timeout;
> - ovs_mutex_unlock(&rule->timeout_mutex);
> + ovs_mutex_unlock(&rule->mutex);
>
> rule->table_id = table - ofproto->tables;
> rule->send_flow_removed = (fm->flags & OFPUTIL_FF_SEND_FLOW_REM) != 0;
> @@ -3688,7 +3687,6 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
> rule->monitor_flags = 0;
> rule->add_seqno = 0;
> rule->modify_seqno = 0;
> - ovs_mutex_init(&rule->mutex);
>
> /* Construct rule, initializing derived state. */
> error = ofproto->ofproto_class->rule_construct(rule);
> @@ -3761,10 +3759,10 @@ modify_flows__(struct ofproto *ofproto, struct ofconn
> *ofconn,
> ofproto_rule_change_cookie(ofproto, rule, fm->new_cookie);
> }
> if (type == OFOPERATION_REPLACE) {
> - ovs_mutex_lock(&rule->timeout_mutex);
> + ovs_mutex_lock(&rule->mutex);
> rule->idle_timeout = fm->idle_timeout;
> rule->hard_timeout = fm->hard_timeout;
> - ovs_mutex_unlock(&rule->timeout_mutex);
> + ovs_mutex_unlock(&rule->mutex);
>
> rule->send_flow_removed = (fm->flags
> & OFPUTIL_FF_SEND_FLOW_REM) != 0;
> @@ -3951,10 +3949,10 @@ ofproto_rule_send_removed(struct rule *rule, uint8_t
> reason)
> fr.table_id = rule->table_id;
> calc_duration(rule->created, time_msec(),
> &fr.duration_sec, &fr.duration_nsec);
> - ovs_mutex_lock(&rule->timeout_mutex);
> + ovs_mutex_lock(&rule->mutex);
> fr.idle_timeout = rule->idle_timeout;
> fr.hard_timeout = rule->hard_timeout;
> - ovs_mutex_unlock(&rule->timeout_mutex);
> + ovs_mutex_unlock(&rule->mutex);
> rule->ofproto->ofproto_class->rule_get_stats(rule, &fr.packet_count,
> &fr.byte_count);
>
> @@ -4003,7 +4001,7 @@ reduce_timeout(uint16_t max, uint16_t *timeout)
> void
> ofproto_rule_reduce_timeouts(struct rule *rule,
> uint16_t idle_timeout, uint16_t hard_timeout)
> - OVS_EXCLUDED(ofproto_mutex, rule->timeout_mutex)
> + OVS_EXCLUDED(ofproto_mutex, rule->mutex)
> {
> if (!idle_timeout && !hard_timeout) {
> return;
> @@ -4015,10 +4013,10 @@ ofproto_rule_reduce_timeouts(struct rule *rule,
> }
> ovs_mutex_unlock(&ofproto_mutex);
>
> - ovs_mutex_lock(&rule->timeout_mutex);
> + ovs_mutex_lock(&rule->mutex);
> reduce_timeout(idle_timeout, &rule->idle_timeout);
> reduce_timeout(hard_timeout, &rule->hard_timeout);
> - ovs_mutex_unlock(&rule->timeout_mutex);
> + ovs_mutex_unlock(&rule->mutex);
> }
>
> static enum ofperr
> @@ -4317,10 +4315,10 @@ ofproto_compose_flow_refresh_update(const struct rule
> *rule,
> fu.event = (flags & (NXFMF_INITIAL | NXFMF_ADD)
> ? NXFME_ADDED : NXFME_MODIFIED);
> fu.reason = 0;
> - ovs_mutex_lock(&rule->timeout_mutex);
> + ovs_mutex_lock(&rule->mutex);
> fu.idle_timeout = rule->idle_timeout;
> fu.hard_timeout = rule->hard_timeout;
> - ovs_mutex_unlock(&rule->timeout_mutex);
> + ovs_mutex_unlock(&rule->mutex);
> fu.table_id = rule->table_id;
> fu.cookie = rule->flow_cookie;
> minimatch_expand(&rule->cr.match, &match);
> @@ -5661,10 +5659,10 @@ ofopgroup_complete(struct ofopgroup *group)
> }
> } else {
> ofproto_rule_change_cookie(ofproto, rule, op->flow_cookie);
> - ovs_mutex_lock(&rule->timeout_mutex);
> + ovs_mutex_lock(&rule->mutex);
> rule->idle_timeout = op->idle_timeout;
> rule->hard_timeout = op->hard_timeout;
> - ovs_mutex_unlock(&rule->timeout_mutex);
> + ovs_mutex_unlock(&rule->mutex);
> if (op->actions) {
> struct rule_actions *old_actions;
>
> @@ -5730,10 +5728,10 @@ ofoperation_create(struct ofopgroup *group, struct
> rule *rule,
> op->type = type;
> op->reason = reason;
> op->flow_cookie = rule->flow_cookie;
> - ovs_mutex_lock(&rule->timeout_mutex);
> + ovs_mutex_lock(&rule->mutex);
> op->idle_timeout = rule->idle_timeout;
> op->hard_timeout = rule->hard_timeout;
> - ovs_mutex_unlock(&rule->timeout_mutex);
> + ovs_mutex_unlock(&rule->mutex);
> op->send_flow_removed = rule->send_flow_removed;
>
> group->n_running++;
> @@ -6045,7 +6043,7 @@ rule_eviction_priority(struct rule *rule)
> uint32_t expiration_offset;
>
> /* Calculate time of expiration. */
> - ovs_mutex_lock(&rule->timeout_mutex);
> + ovs_mutex_lock(&rule->mutex);
> hard_expiration = (rule->hard_timeout
> ? rule->modified + rule->hard_timeout * 1000
> : LLONG_MAX);
> @@ -6053,7 +6051,7 @@ rule_eviction_priority(struct rule *rule)
> ? rule->used + rule->idle_timeout * 1000
> : LLONG_MAX);
> expiration = MIN(hard_expiration, idle_expiration);
> - ovs_mutex_unlock(&rule->timeout_mutex);
> + ovs_mutex_unlock(&rule->mutex);
> if (expiration == LLONG_MAX) {
> return 0;
> }
> @@ -6082,9 +6080,9 @@ eviction_group_add_rule(struct rule *rule)
> struct oftable *table = &ofproto->tables[rule->table_id];
> bool has_timeout;
>
> - ovs_mutex_lock(&rule->timeout_mutex);
> + ovs_mutex_lock(&rule->mutex);
> has_timeout = rule->hard_timeout || rule->idle_timeout;
> - ovs_mutex_unlock(&rule->timeout_mutex);
> + ovs_mutex_unlock(&rule->mutex);
>
> if (table->eviction_fields && has_timeout) {
> struct eviction_group *evg;
> @@ -6250,9 +6248,9 @@ oftable_insert_rule(struct rule *rule)
> struct oftable *table = &ofproto->tables[rule->table_id];
> bool may_expire;
>
> - ovs_mutex_lock(&rule->timeout_mutex);
> + ovs_mutex_lock(&rule->mutex);
> may_expire = rule->hard_timeout || rule->idle_timeout;
> - ovs_mutex_unlock(&rule->timeout_mutex);
> + ovs_mutex_unlock(&rule->mutex);
>
> if (may_expire) {
> ovs_mutex_lock(&ofproto_mutex);
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
