From a07dfd5d746ad984f7541f84628f884419314c4b Mon Sep 17 00:00:00 2001 From: Ethan Jackson Date: Tue, 27 Aug 2013 13:17:11 -0700 Subject: [PATCH] ofproto-dpif: Hide struct rule_dpif internally. By hiding struct rule_dpif inside ofproto-dpif, it becomes very clear which attributes are accessed by multiple threads and need to be protected by locks. Signed-off-by: Ethan Jackson Acked-by: Ben Pfaff --- ofproto/ofproto-dpif-upcall.c | 6 +-- ofproto/ofproto-dpif-xlate.c | 27 ++++++----- ofproto/ofproto-dpif-xlate.h | 2 + ofproto/ofproto-dpif.c | 90 ++++++++++++++++++++++++++++------- ofproto/ofproto-dpif.h | 58 ++++++++++------------ 5 files changed, 116 insertions(+), 67 deletions(-) diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 3294d3b48..54f441b06 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -684,7 +684,7 @@ execute_flow_miss(struct flow_miss *miss, struct dpif_op *ops, size_t *n_ops) flow_wildcards_init_catchall(&wc); rule_dpif_lookup(ofproto, &miss->flow, &wc, &rule); - rule_credit_stats(rule, &miss->stats); + rule_dpif_credit_stats(rule, &miss->stats); xlate_in_init(&xin, ofproto, &miss->flow, rule, miss->stats.tcp_flags, NULL); xin.may_learn = true; @@ -692,7 +692,7 @@ execute_flow_miss(struct flow_miss *miss, struct dpif_op *ops, size_t *n_ops) xlate_actions(&xin, &miss->xout); flow_wildcards_or(&miss->xout.wc, &miss->xout.wc, &wc); - if (rule->up.cr.priority == FAIL_OPEN_PRIORITY) { + if (rule_dpif_fail_open(rule)) { LIST_FOR_EACH (packet, list_node, &miss->packets) { struct ofputil_packet_in *pin; @@ -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_release(rule); + rule_dpif_release(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 feee71c39..6a78c6d4a 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -1668,21 +1668,24 @@ 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->up.evict) + OVS_RELEASES(rule) { struct rule_dpif *old_rule = ctx->rule; + const struct ofpact *ofpacts; + size_t ofpacts_len; if (ctx->xin->resubmit_stats) { - rule_credit_stats(rule, ctx->xin->resubmit_stats); + rule_dpif_credit_stats(rule, ctx->xin->resubmit_stats); } ctx->recurse++; ctx->rule = rule; - do_xlate_actions(rule->up.ofpacts, rule->up.ofpacts_len, ctx); + rule_dpif_get_actions(rule, &ofpacts, &ofpacts_len); + do_xlate_actions(ofpacts, ofpacts_len, ctx); ctx->rule = old_rule; ctx->recurse--; - rule_release(rule); + rule_dpif_release(rule); } static void @@ -1722,10 +1725,9 @@ xlate_table_action(struct xlate_ctx *ctx, * OFPTC_TABLE_MISS_DROP * When OF1.0, OFPTC_TABLE_MISS_CONTINUE is used. What to do? */ xport = get_ofp_port(ctx->xbridge, ctx->xin->flow.in_port.ofp_port); - rule = choose_miss_rule(xport ? xport->config : 0, - ctx->xbridge->miss_rule, - ctx->xbridge->no_packet_in_rule); - ovs_rwlock_rdlock(&rule->up.evict); + choose_miss_rule(xport ? xport->config : 0, + ctx->xbridge->miss_rule, + ctx->xbridge->no_packet_in_rule, &rule); xlate_recursively(ctx, rule); } @@ -1811,7 +1813,7 @@ execute_controller_action(struct xlate_ctx *ctx, int len, pin->reason = reason; pin->controller_id = controller_id; pin->table_id = ctx->table_id; - pin->cookie = ctx->rule ? ctx->rule->up.flow_cookie : 0; + pin->cookie = ctx->rule ? rule_dpif_get_flow_cookie(ctx->rule) : 0; pin->send_len = len; flow_get_metadata(&ctx->xin->flow, &pin->fmd); @@ -2119,8 +2121,8 @@ xlate_fin_timeout(struct xlate_ctx *ctx, const struct ofpact_fin_timeout *oft) { if (ctx->xin->tcp_flags & (TCP_FIN | TCP_RST) && ctx->rule) { - ofproto_rule_reduce_timeouts(&ctx->rule->up, oft->fin_idle_timeout, - oft->fin_hard_timeout); + rule_dpif_reduce_timeouts(ctx->rule, oft->fin_idle_timeout, + oft->fin_hard_timeout); } } @@ -2598,8 +2600,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout) ofpacts = xin->ofpacts; ofpacts_len = xin->ofpacts_len; } else if (xin->rule) { - ofpacts = xin->rule->up.ofpacts; - ofpacts_len = xin->rule->up.ofpacts_len; + rule_dpif_get_actions(xin->rule, &ofpacts, &ofpacts_len); } else { NOT_REACHED(); } diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h index ba24e926f..a54a9e4a2 100644 --- a/ofproto/ofproto-dpif-xlate.h +++ b/ofproto/ofproto-dpif-xlate.h @@ -21,6 +21,8 @@ #include "ofpbuf.h" #include "ofproto-dpif-mirror.h" #include "ofproto-dpif.h" +#include "ofproto.h" +#include "stp.h" struct bfd; struct bond; diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index ed14600af..cab7bc3d5 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -83,7 +83,29 @@ BUILD_ASSERT_DECL(N_TABLES >= 2 && N_TABLES <= 255); struct flow_miss; struct facet; +struct rule_dpif { + struct rule up; + + /* These statistics: + * + * - Do include packets and bytes from facets that have been deleted or + * whose own statistics have been folded into the rule. + * + * - Do include packets and bytes sent "by hand" that were accounted to + * the rule without any facet being involved (this is a rare corner + * case in rule_execute()). + * + * - Do not include packet or bytes that can be obtained from any facet's + * packet_count or byte_count member or that can be obtained from the + * datapath by, e.g., dpif_flow_get() for any subfacet. + */ + struct ovs_mutex stats_mutex; + uint64_t packet_count OVS_GUARDED; /* Number of packets received. */ + uint64_t byte_count OVS_GUARDED; /* Number of bytes received. */ +}; + static void rule_get_stats(struct rule *, uint64_t *packets, uint64_t *bytes); +static struct rule_dpif *rule_dpif_cast(const struct rule *); struct ofbundle { struct hmap_node hmap_node; /* In struct ofproto's "bundles" hmap. */ @@ -1359,7 +1381,7 @@ add_internal_flow(struct ofproto_dpif *ofproto, int id, if (rule_dpif_lookup_in_table(ofproto, &fm.match.flow, NULL, TBL_INTERNAL, rulep)) { - ovs_rwlock_unlock(&(*rulep)->up.evict); + rule_dpif_release(*rulep); } else { NOT_REACHED(); } @@ -4190,7 +4212,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_release(rule); + rule_dpif_release(rule); return is_controller; } return false; @@ -4284,7 +4306,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_release(rule); + rule_dpif_release(rule); ok = ofpbuf_equal(&facet->xout.odp_actions, &xout.odp_actions) && facet->xout.slow == xout.slow; @@ -4382,7 +4404,7 @@ facet_revalidate(struct facet *facet) || memcmp(&facet->xout.wc, &xout.wc, sizeof xout.wc)) { facet_remove(facet); xlate_out_uninit(&xout); - rule_release(new_rule); + rule_dpif_release(new_rule); return false; } @@ -4414,7 +4436,7 @@ facet_revalidate(struct facet *facet) facet->used = MAX(facet->used, new_rule->up.created); xlate_out_uninit(&xout); - rule_release(new_rule); + rule_dpif_release(new_rule); return true; } @@ -4442,12 +4464,12 @@ flow_push_stats(struct ofproto_dpif *ofproto, struct flow *flow, } rule_dpif_lookup(ofproto, flow, NULL, &rule); - rule_credit_stats(rule, stats); + rule_dpif_credit_stats(rule, stats); xlate_in_init(&xin, ofproto, flow, rule, stats->tcp_flags, NULL); xin.resubmit_stats = stats; xin.may_learn = may_learn; xlate_actions_for_side_effects(&xin); - rule_release(rule); + rule_dpif_release(rule); } static void @@ -4513,7 +4535,8 @@ push_all_stats(void) } void -rule_credit_stats(struct rule_dpif *rule, const struct dpif_flow_stats *stats) +rule_dpif_credit_stats(struct rule_dpif *rule, + const struct dpif_flow_stats *stats) { ovs_mutex_lock(&rule->stats_mutex); rule->packet_count += stats->n_packets; @@ -4521,6 +4544,33 @@ rule_credit_stats(struct rule_dpif *rule, const struct dpif_flow_stats *stats) ofproto_rule_update_used(&rule->up, stats->used); ovs_mutex_unlock(&rule->stats_mutex); } + +bool +rule_dpif_fail_open(const struct rule_dpif *rule) +{ + return rule->up.cr.priority == FAIL_OPEN_PRIORITY; +} + +ovs_be64 +rule_dpif_get_flow_cookie(const struct rule_dpif *rule) +{ + return rule->up.flow_cookie; +} + +void +rule_dpif_reduce_timeouts(struct rule_dpif *rule, uint16_t idle_timeout, + uint16_t hard_timeout) +{ + ofproto_rule_reduce_timeouts(&rule->up, idle_timeout, hard_timeout); +} + +void +rule_dpif_get_actions(const struct rule_dpif *rule, + const struct ofpact **ofpacts, size_t *ofpacts_len) +{ + *ofpacts = rule->up.ofpacts; + *ofpacts_len = rule->up.ofpacts_len; +} /* Subfacets. */ @@ -4789,9 +4839,8 @@ rule_dpif_lookup(struct ofproto_dpif *ofproto, const struct flow *flow, flow->in_port.ofp_port); } - *rule = choose_miss_rule(port ? port->up.pp.config : 0, ofproto->miss_rule, - ofproto->no_packet_in_rule); - ovs_rwlock_rdlock(&(*rule)->up.evict); + choose_miss_rule(port ? port->up.pp.config : 0, ofproto->miss_rule, + ofproto->no_packet_in_rule, rule); } bool @@ -4846,15 +4895,17 @@ rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto, /* Given a port configuration (specified as zero if there's no port), chooses * which of 'miss_rule' and 'no_packet_in_rule' should be used in case of a * flow table miss. */ -struct rule_dpif * +void choose_miss_rule(enum ofputil_port_config config, struct rule_dpif *miss_rule, - struct rule_dpif *no_packet_in_rule) + struct rule_dpif *no_packet_in_rule, struct rule_dpif **rule) + OVS_NO_THREAD_SAFETY_ANALYSIS { - return config & OFPUTIL_PC_NO_PACKET_IN ? no_packet_in_rule : miss_rule; + *rule = config & OFPUTIL_PC_NO_PACKET_IN ? no_packet_in_rule : miss_rule; + ovs_rwlock_rdlock(&(*rule)->up.evict); } void -rule_release(struct rule_dpif *rule) +rule_dpif_release(struct rule_dpif *rule) OVS_NO_THREAD_SAFETY_ANALYSIS { if (rule) { @@ -4877,6 +4928,11 @@ complete_operation(struct rule_dpif *rule) } } +static struct rule_dpif *rule_dpif_cast(const struct rule *rule) +{ + return rule ? CONTAINER_OF(rule, struct rule_dpif, up) : NULL; +} + static struct rule * rule_alloc(void) { @@ -4953,7 +5009,7 @@ rule_dpif_execute(struct rule_dpif *rule, const struct flow *flow, struct xlate_in xin; dpif_flow_stats_extract(flow, packet, time_msec(), &stats); - rule_credit_stats(rule, &stats); + rule_dpif_credit_stats(rule, &stats); xlate_in_init(&xin, ofproto, flow, rule, stats.tcp_flags, packet); xin.resubmit_stats = &stats; @@ -5574,7 +5630,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, const struct flow *flow, xlate_out_uninit(&trace.xout); } - rule_release(rule); + rule_dpif_release(rule); } static void diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h index 15e58e957..eb724d5ee 100644 --- a/ofproto/ofproto-dpif.h +++ b/ofproto/ofproto-dpif.h @@ -19,16 +19,18 @@ #include "hmapx.h" #include "odp-util.h" -#include "ofproto/ofproto-provider.h" +#include "ofp-util.h" #include "ovs-thread.h" #include "timer.h" #include "util.h" #include "ovs-thread.h" union user_action_cookie; +struct dpif_flow_stats; struct ofproto_dpif; struct ofport_dpif; struct dpif_backer; +struct OVS_LOCKABLE rule_dpif; /* Ofproto-dpif -- DPIF based ofproto implementation. * @@ -58,48 +60,36 @@ struct dpif_backer; * Ofproto-dpif-xlate is responsible for translating translating OpenFlow * actions into datapath actions. */ -struct rule_dpif { - struct rule up; - - /* These statistics: - * - * - Do include packets and bytes from facets that have been deleted or - * whose own statistics have been folded into the rule. - * - * - Do include packets and bytes sent "by hand" that were accounted to - * the rule without any facet being involved (this is a rare corner - * case in rule_execute()). - * - * - Do not include packet or bytes that can be obtained from any facet's - * packet_count or byte_count member or that can be obtained from the - * datapath by, e.g., dpif_flow_get() for any subfacet. - */ - struct ovs_mutex stats_mutex; - uint64_t packet_count OVS_GUARDED; /* Number of packets received. */ - uint64_t byte_count OVS_GUARDED; /* Number of bytes received. */ -}; - -static inline struct rule_dpif *rule_dpif_cast(const struct rule *rule) -{ - return rule ? CONTAINER_OF(rule, struct rule_dpif, up) : NULL; -} - void rule_dpif_lookup(struct ofproto_dpif *, const struct flow *, struct flow_wildcards *, struct rule_dpif **rule) - OVS_ACQ_RDLOCK((*rule)->up.evict); + OVS_ACQ_RDLOCK(*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)->up.evict); + OVS_TRY_RDLOCK(true, *rule); + + void rule_dpif_release(struct rule_dpif *rule) OVS_RELEASES(rule); + + void rule_dpif_credit_stats(struct rule_dpif *rule , + const struct dpif_flow_stats *); + +bool rule_dpif_fail_open(const struct rule_dpif *rule); + +void rule_dpif_get_actions(const struct rule_dpif *rule, + const struct ofpact **ofpacts, + size_t *ofpacts_len); -void rule_release(struct rule_dpif *rule) OVS_RELEASES(rule->up.evict); +ovs_be64 rule_dpif_get_flow_cookie(const struct rule_dpif *rule); -void rule_credit_stats(struct rule_dpif *, const struct dpif_flow_stats *); +void rule_dpif_reduce_timeouts(struct rule_dpif *rule, uint16_t idle_timeout, + uint16_t hard_timeout); -struct rule_dpif *choose_miss_rule(enum ofputil_port_config, - struct rule_dpif *miss_rule, - struct rule_dpif *no_packet_in_rule); +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); bool ofproto_has_vlan_splinters(const struct ofproto_dpif *); ofp_port_t vsp_realdev_to_vlandev(const struct ofproto_dpif *, -- 2.20.1