From: Ben Pfaff Date: Thu, 12 Sep 2013 06:23:00 +0000 (-0700) Subject: ofproto: Add a ref_count to "struct rule" to protect it from being freed. X-Git-Tag: v2.0~44 X-Git-Url: http://git.cascardo.eti.br/?p=cascardo%2Fovs.git;a=commitdiff_plain;h=10dc53e24ee265f1adae4ba951a6010e67193bea ofproto: Add a ref_count to "struct rule" to protect it from being freed. Taking a read-lock on the 'rwlock' member of struct rule prevents members of the rule from changing. This is a short-term use of the 'rwlock': one takes the lock, reads some members, and releases the lock. Taking a read-lock on the 'rwlock' also prevents the rule from being freed. This is often a relatively long-term need. For example, until now flow translation has held the rwlock in xlate_table_action() across the entire recursive translation, which can call into a great deal of different code across multiple files. This commit switches to using a reference count for this second purpose of struct rule's rwlock. This means that all the code that previously held a read-lock on the rwlock across deep stacks of functions can now simply keep a reference. Signed-off-by: Ben Pfaff Acked-by: Ethan Jackson --- diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 54f441b06..ae856a4e5 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -725,7 +725,7 @@ execute_flow_miss(struct flow_miss *miss, struct dpif_op *ops, size_t *n_ops) xlate_actions_for_side_effects(&xin); } } - rule_dpif_release(rule); + rule_dpif_unref(rule); if (miss->xout.odp_actions.size) { LIST_FOR_EACH (packet, list_node, &miss->packets) { diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 1880187b8..92da2381d 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -1669,7 +1669,6 @@ compose_output_action(struct xlate_ctx *ctx, ofp_port_t ofp_port) static void xlate_recursively(struct xlate_ctx *ctx, struct rule_dpif *rule) - OVS_RELEASES(rule) { struct rule_dpif *old_rule = ctx->rule; struct rule_actions *actions; @@ -1685,8 +1684,6 @@ xlate_recursively(struct xlate_ctx *ctx, struct rule_dpif *rule) rule_actions_unref(actions); ctx->rule = old_rule; ctx->recurse--; - - rule_dpif_release(rule); } static void @@ -1697,7 +1694,6 @@ xlate_table_action(struct xlate_ctx *ctx, struct rule_dpif *rule; ofp_port_t old_in_port = ctx->xin->flow.in_port.ofp_port; uint8_t old_table_id = ctx->table_id; - bool got_rule; ctx->table_id = table_id; @@ -1705,18 +1701,16 @@ xlate_table_action(struct xlate_ctx *ctx, * original input port (otherwise OFPP_NORMAL and OFPP_IN_PORT will * have surprising behavior). */ ctx->xin->flow.in_port.ofp_port = in_port; - got_rule = rule_dpif_lookup_in_table(ctx->xbridge->ofproto, - &ctx->xin->flow, &ctx->xout->wc, - table_id, &rule); + rule_dpif_lookup_in_table(ctx->xbridge->ofproto, + &ctx->xin->flow, &ctx->xout->wc, + table_id, &rule); ctx->xin->flow.in_port.ofp_port = old_in_port; if (ctx->xin->resubmit_hook) { ctx->xin->resubmit_hook(ctx->xin, rule, ctx->recurse); } - if (got_rule) { - xlate_recursively(ctx, rule); - } else if (may_packet_in) { + if (!rule && may_packet_in) { struct xport *xport; /* XXX @@ -1729,7 +1723,10 @@ xlate_table_action(struct xlate_ctx *ctx, choose_miss_rule(xport ? xport->config : 0, ctx->xbridge->miss_rule, ctx->xbridge->no_packet_in_rule, &rule); + } + if (rule) { xlate_recursively(ctx, rule); + rule_dpif_unref(rule); } ctx->table_id = old_table_id; diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 1568d6083..bf4fe597b 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -1372,7 +1372,7 @@ add_internal_flow(struct ofproto_dpif *ofproto, int id, if (rule_dpif_lookup_in_table(ofproto, &fm.match.flow, NULL, TBL_INTERNAL, rulep)) { - rule_dpif_release(*rulep); + rule_dpif_unref(*rulep); } else { NOT_REACHED(); } @@ -4172,7 +4172,7 @@ facet_is_controller_flow(struct facet *facet) is_controller = ofpacts_len > 0 && ofpacts->type == OFPACT_CONTROLLER && ofpact_next(ofpacts) >= ofpact_end(ofpacts, ofpacts_len); - rule_dpif_release(rule); + rule_dpif_unref(rule); return is_controller; } @@ -4267,7 +4267,7 @@ facet_check_consistency(struct facet *facet) rule_dpif_lookup(facet->ofproto, &facet->flow, NULL, &rule); xlate_in_init(&xin, facet->ofproto, &facet->flow, rule, 0, NULL); xlate_actions(&xin, &xout); - rule_dpif_release(rule); + rule_dpif_unref(rule); ok = ofpbuf_equal(&facet->xout.odp_actions, &xout.odp_actions) && facet->xout.slow == xout.slow; @@ -4365,7 +4365,7 @@ facet_revalidate(struct facet *facet) || memcmp(&facet->xout.wc, &xout.wc, sizeof xout.wc)) { facet_remove(facet); xlate_out_uninit(&xout); - rule_dpif_release(new_rule); + rule_dpif_unref(new_rule); return false; } @@ -4397,7 +4397,7 @@ facet_revalidate(struct facet *facet) facet->used = MAX(facet->used, new_rule->up.created); xlate_out_uninit(&xout); - rule_dpif_release(new_rule); + rule_dpif_unref(new_rule); return true; } @@ -4430,7 +4430,7 @@ flow_push_stats(struct ofproto_dpif *ofproto, struct flow *flow, xin.resubmit_stats = stats; xin.may_learn = may_learn; xlate_actions_for_side_effects(&xin); - rule_dpif_release(rule); + rule_dpif_unref(rule); } static void @@ -4816,7 +4816,6 @@ 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.rwlock) { struct cls_rule *cls_rule; struct classifier *cls; @@ -4851,11 +4850,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.rwlock)) { - /* The rule is in the process of being removed. Best we can do is - * pretend it isn't there. */ - *rule = NULL; - } + rule_dpif_ref(*rule); ovs_rwlock_unlock(&cls->rwlock); return *rule != NULL; @@ -4867,18 +4862,24 @@ rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto, void choose_miss_rule(enum ofputil_port_config config, struct rule_dpif *miss_rule, struct rule_dpif *no_packet_in_rule, struct rule_dpif **rule) - OVS_NO_THREAD_SAFETY_ANALYSIS { *rule = config & OFPUTIL_PC_NO_PACKET_IN ? no_packet_in_rule : miss_rule; - ovs_rwlock_rdlock(&(*rule)->up.rwlock); + rule_dpif_ref(*rule); +} + +void +rule_dpif_ref(struct rule_dpif *rule) +{ + if (rule) { + ofproto_rule_ref(&rule->up); + } } void -rule_dpif_release(struct rule_dpif *rule) - OVS_NO_THREAD_SAFETY_ANALYSIS +rule_dpif_unref(struct rule_dpif *rule) { if (rule) { - ovs_rwlock_unlock(&rule->up.rwlock); + ofproto_rule_unref(&rule->up); } } @@ -5594,7 +5595,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, const struct flow *flow, xlate_out_uninit(&trace.xout); } - rule_dpif_release(rule); + rule_dpif_unref(rule); } /* Runs a self-check of flow translations in 'ofproto'. Appends a message to diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h index abb2c8971..14a96693b 100644 --- a/ofproto/ofproto-dpif.h +++ b/ofproto/ofproto-dpif.h @@ -61,18 +61,17 @@ struct OVS_LOCKABLE rule_dpif; * actions into datapath actions. */ void rule_dpif_lookup(struct ofproto_dpif *, const struct flow *, - struct flow_wildcards *, struct rule_dpif **rule) - OVS_ACQ_RDLOCK(*rule); + struct flow_wildcards *, struct rule_dpif **rule); bool rule_dpif_lookup_in_table(struct ofproto_dpif *, const struct flow *, struct flow_wildcards *, uint8_t table_id, - struct rule_dpif **rule) - OVS_TRY_RDLOCK(true, *rule); + struct rule_dpif **rule); - void rule_dpif_release(struct rule_dpif *rule) OVS_RELEASES(rule); +void rule_dpif_ref(struct rule_dpif *); +void rule_dpif_unref(struct rule_dpif *); - void rule_dpif_credit_stats(struct rule_dpif *rule , - const struct dpif_flow_stats *); +void rule_dpif_credit_stats(struct rule_dpif *rule , + const struct dpif_flow_stats *); bool rule_dpif_fail_open(const struct rule_dpif *rule); @@ -86,8 +85,7 @@ void rule_dpif_reduce_timeouts(struct rule_dpif *rule, uint16_t idle_timeout, void choose_miss_rule(enum ofputil_port_config, struct rule_dpif *miss_rule, struct rule_dpif *no_packet_in_rule, - struct rule_dpif **rule) - OVS_ACQ_RDLOCK(*rule); + struct rule_dpif **rule); bool ofproto_has_vlan_splinters(const struct ofproto_dpif *); ofp_port_t vsp_realdev_to_vlandev(const struct ofproto_dpif *, diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 8f3261b5a..59ac3dc57 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -27,6 +27,7 @@ #include "ofp-errors.h" #include "ofp-util.h" #include "ofproto/ofproto.h" +#include "ovs-atomic.h" #include "ovs-thread.h" #include "shash.h" #include "simap.h" @@ -213,6 +214,7 @@ struct oftable { struct rule { struct ofproto *ofproto; /* The ofproto that contains this rule. */ struct cls_rule cr; /* In owning ofproto's classifier. */ + atomic_uint ref_count; struct ofoperation *pending; /* Operation now in progress, if nonnull. */ @@ -235,14 +237,16 @@ struct rule { struct eviction_group *eviction_group; /* NULL if not in any group. */ /* 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. */ + * accessed by multiple threads. The main ofproto code is guaranteed not + * to change any of the elements "Guarded by rwlock" without holding the + * writelock. + * + * 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 + * ref_count. + * + * A rule will not be evicted unless its classifier's write lock is + * held. */ struct ovs_rwlock rwlock; /* Guarded by rwlock. */ @@ -260,6 +264,9 @@ struct rule { * is expirable, otherwise empty. */ }; +void ofproto_rule_ref(struct rule *); +void ofproto_rule_unref(struct rule *); + /* A set of actions within a "struct rule". * * diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 02df58897..5473c7d65 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -227,7 +227,6 @@ static int init_ports(struct ofproto *); static void reinit_ports(struct ofproto *); /* rule. */ -static void ofproto_rule_destroy(struct rule *); static void ofproto_rule_destroy__(struct rule *); static void ofproto_rule_send_removed(struct rule *, uint8_t reason); static bool rule_is_modifiable(const struct rule *); @@ -2317,12 +2316,30 @@ update_mtu(struct ofproto *p, struct ofport *port) } } -static void -ofproto_rule_destroy(struct rule *rule) +void +ofproto_rule_ref(struct rule *rule) { if (rule) { - rule->ofproto->ofproto_class->rule_destruct(rule); - ofproto_rule_destroy__(rule); + unsigned int orig; + + atomic_add(&rule->ref_count, 1, &orig); + ovs_assert(orig != 0); + } +} + +void +ofproto_rule_unref(struct rule *rule) +{ + if (rule) { + unsigned int orig; + + atomic_sub(&rule->ref_count, 1, &orig); + if (orig == 1) { + rule->ofproto->ofproto_class->rule_destruct(rule); + ofproto_rule_destroy__(rule); + } else { + ovs_assert(orig != 0); + } } } @@ -3673,6 +3690,7 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn, /* Initialize base state. */ rule->ofproto = ofproto; cls_rule_move(&rule->cr, &cr); + atomic_init(&rule->ref_count, 1); rule->pending = NULL; rule->flow_cookie = fm->new_cookie; rule->created = rule->modified = rule->used = time_msec(); @@ -5193,13 +5211,13 @@ ofopgroup_complete(struct ofopgroup *group) } else { ovs_rwlock_wrlock(&rule->rwlock); oftable_remove_rule(rule); - ofproto_rule_destroy(rule); + ofproto_rule_unref(rule); } break; case OFOPERATION_DELETE: ovs_assert(!op->error); - ofproto_rule_destroy(rule); + ofproto_rule_unref(rule); op->rule = NULL; break;