From: Ethan Jackson Date: Wed, 4 Sep 2013 00:23:44 +0000 (-0700) Subject: ofproto: Rename struct rule's evict lock and use it more widely. X-Git-Tag: sliver-openvswitch-2.0.90-1~16^2~22 X-Git-Url: http://git.onelab.eu/?p=sliver-openvswitch.git;a=commitdiff_plain;h=884e1dc43275683c9a2bf23ce4e46d8fb43a3d41 ofproto: Rename struct rule's evict lock and use it more widely. There are a few fields in struct rule which are accessible by functions in ofproto-dpif and therefore need to accessed in a thread safe manner. This patch achieves this by generalizing the rules evict rwlock and requiring a writelock to be held to edit them. Signed-off-by: Ethan Jackson Acked-by: Ben Pfaff --- diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 4df12c8bc..c7d357365 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -4836,7 +4836,7 @@ bool rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto, const struct flow *flow, struct flow_wildcards *wc, uint8_t table_id, struct rule_dpif **rule) - OVS_TRY_RDLOCK(true, (*rule)->up.evict) + OVS_TRY_RDLOCK(true, (*rule)->up.rwlock) { struct cls_rule *cls_rule; struct classifier *cls; @@ -4871,7 +4871,7 @@ rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto, } *rule = rule_dpif_cast(rule_from_cls_rule(cls_rule)); - if (*rule && ovs_rwlock_tryrdlock(&(*rule)->up.evict)) { + if (*rule && ovs_rwlock_tryrdlock(&(*rule)->up.rwlock)) { /* The rule is in the process of being removed. Best we can do is * pretend it isn't there. */ *rule = NULL; @@ -4890,7 +4890,7 @@ choose_miss_rule(enum ofputil_port_config config, struct rule_dpif *miss_rule, OVS_NO_THREAD_SAFETY_ANALYSIS { *rule = config & OFPUTIL_PC_NO_PACKET_IN ? no_packet_in_rule : miss_rule; - ovs_rwlock_rdlock(&(*rule)->up.evict); + ovs_rwlock_rdlock(&(*rule)->up.rwlock); } void @@ -4898,7 +4898,7 @@ rule_dpif_release(struct rule_dpif *rule) OVS_NO_THREAD_SAFETY_ANALYSIS { if (rule) { - ovs_rwlock_unlock(&rule->up.evict); + ovs_rwlock_unlock(&rule->up.rwlock); } } diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 52a701409..433e03055 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -223,7 +223,8 @@ struct rule { struct ofoperation *pending; /* Operation now in progress, if nonnull. */ - ovs_be64 flow_cookie; /* Controller-issued identifier. */ + ovs_be64 flow_cookie; /* Controller-issued identifier. Guarded by + rwlock. */ struct hindex_node cookie_node; /* In owning ofproto's 'cookies' index. */ long long int created; /* Creation time. */ @@ -240,15 +241,18 @@ 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 evict lock is used to prevent rules from being evicted while child - * threads are using them to xlate flows. A read lock means the rule is - * currently being used. A write lock means the rule is in the process of - * being evicted and should be considered gone. A rule will not be evicted - * unless both its own and its classifiers write locks are held. - * Therefore, while holding a classifier readlock, one can be assured that - * even write locked rules are safe. */ - struct ovs_rwlock evict; + /* The rwlock is used to protect those elements in struct rule which are + * accessed by multiple threads. While maintaining a pointer to struct + * rule, threads are required to hold a readlock. The main ofproto code is + * guaranteed not to evict the rule, or change any of the elements "Guarded + * by rwlock" without holding the writelock. + * + * A rule will not be evicted unless both its own and its classifier's + * write locks are held. Therefore, while holding a classifier readlock, + * one can be assured that write locked rules are safe to reference. */ + struct ovs_rwlock rwlock; + /* Guarded by rwlock. */ struct ofpact *ofpacts; /* Sequence of "struct ofpacts". */ unsigned int ofpacts_len; /* Size of 'ofpacts', in bytes. */ diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 3747eef79..dacbb71b4 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -155,10 +155,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->evict); +static void oftable_remove_rule(struct rule *rule) OVS_RELEASES(rule->rwlock); static void oftable_remove_rule__(struct ofproto *ofproto, struct classifier *cls, struct rule *rule) - OVS_REQ_WRLOCK(cls->rwlock) OVS_RELEASES(rule->evict); + OVS_REQ_WRLOCK(cls->rwlock) OVS_RELEASES(rule->rwlock); static void oftable_insert_rule(struct rule *); /* A set of rules within a single OpenFlow table (oftable) that have the same @@ -184,7 +184,7 @@ struct eviction_group { }; static bool choose_rule_to_evict(struct oftable *table, struct rule **rulep) - OVS_TRY_WRLOCK(true, (*rulep)->evict); + OVS_TRY_WRLOCK(true, (*rulep)->rwlock); static void ofproto_evict(struct ofproto *); static uint32_t rule_eviction_priority(struct rule *); static void eviction_group_add_rule(struct rule *); @@ -212,7 +212,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->evict); + OVS_RELEASES(rule->rwlock); 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 *, @@ -1094,7 +1094,7 @@ ofproto_delete_rule(struct ofproto *ofproto, struct classifier *cls, group = ofopgroup_create_unattached(ofproto); ofoperation_create(group, rule, OFOPERATION_DELETE, OFPRR_DELETE); - ovs_rwlock_wrlock(&rule->evict); + ovs_rwlock_wrlock(&rule->rwlock); oftable_remove_rule__(ofproto, cls, rule); ofproto->ofproto_class->rule_delete(rule); ofopgroup_submit(group); @@ -2263,7 +2263,7 @@ ofproto_rule_destroy__(struct rule *rule) cls_rule_destroy(&rule->cr); free(rule->ofpacts); ovs_mutex_destroy(&rule->timeout_mutex); - ovs_rwlock_destroy(&rule->evict); + ovs_rwlock_destroy(&rule->rwlock); rule->ofproto->ofproto_class->rule_dealloc(rule); } } @@ -2843,7 +2843,9 @@ ofproto_rule_change_cookie(struct ofproto *ofproto, struct rule *rule, if (new_cookie != rule->flow_cookie) { cookies_remove(ofproto, rule); + ovs_rwlock_wrlock(&rule->rwlock); rule->flow_cookie = new_cookie; + ovs_rwlock_unlock(&rule->rwlock); cookies_insert(ofproto, rule); } @@ -3418,7 +3420,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->evict); + ovs_rwlock_unlock(&rule->rwlock); return OFPROTO_POSTPONE; } else { struct ofopgroup *group; @@ -3575,7 +3577,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->evict); + ovs_rwlock_init(&rule->rwlock); /* Construct rule, initializing derived state. */ error = ofproto->ofproto_class->rule_construct(rule); @@ -3669,8 +3671,12 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn, op->ofpacts = rule->ofpacts; op->ofpacts_len = rule->ofpacts_len; op->meter_id = rule->meter_id; + + ovs_rwlock_wrlock(&rule->rwlock); rule->ofpacts = xmemdup(fm->ofpacts, fm->ofpacts_len); rule->ofpacts_len = fm->ofpacts_len; + ovs_rwlock_unlock(&rule->rwlock); + rule->meter_id = find_meter(rule->ofpacts, rule->ofpacts_len); rule->ofproto->ofproto_class->rule_modify_actions(rule, reset_counters); @@ -3773,7 +3779,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->evict); + ovs_rwlock_wrlock(&rule->rwlock); delete_flow__(rule, group, reason); } ofopgroup_submit(group); @@ -5446,7 +5452,7 @@ ofopgroup_complete(struct ofopgroup *group) } } } else { - ovs_rwlock_wrlock(&rule->evict); + ovs_rwlock_wrlock(&rule->rwlock); oftable_remove_rule(rule); ofproto_rule_destroy__(rule); } @@ -5475,8 +5481,12 @@ ofopgroup_complete(struct ofopgroup *group) ovs_mutex_unlock(&rule->timeout_mutex); if (op->ofpacts) { free(rule->ofpacts); + + ovs_rwlock_wrlock(&rule->rwlock); rule->ofpacts = op->ofpacts; rule->ofpacts_len = op->ofpacts_len; + ovs_rwlock_unlock(&rule->rwlock); + op->ofpacts = NULL; op->ofpacts_len = 0; } @@ -5664,7 +5674,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->evict)) { + if (!ovs_rwlock_trywrlock(&rule->rwlock)) { *rulep = rule; return true; } @@ -5705,7 +5715,7 @@ ofproto_evict(struct ofproto *ofproto) } if (rule->pending) { - ovs_rwlock_unlock(&rule->evict); + ovs_rwlock_unlock(&rule->rwlock); break; } @@ -6013,7 +6023,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->evict) + OVS_REQ_WRLOCK(cls->rwlock) OVS_RELEASES(rule->rwlock) { classifier_remove(cls, &rule->cr); cookies_remove(ofproto, rule); @@ -6027,7 +6037,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->evict); + ovs_rwlock_unlock(&rule->rwlock); } static void