From 06a0f3e21b2cf2b772a01cc0779d4c2e01389095 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Fri, 14 Mar 2014 10:47:50 -0700 Subject: [PATCH] ofproto: Use RCU to protect rule_actions. Signed-off-by: Ben Pfaff Acked-by: Andy Zhou --- ofproto/connmgr.c | 5 +- ofproto/ofproto-dpif-xlate.c | 2 - ofproto/ofproto-dpif.c | 2 - ofproto/ofproto-provider.h | 19 +++--- ofproto/ofproto.c | 119 +++++++++++++++-------------------- 5 files changed, 63 insertions(+), 84 deletions(-) diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c index 033ab7d5c..0058309b4 100644 --- a/ofproto/connmgr.c +++ b/ofproto/connmgr.c @@ -2043,8 +2043,9 @@ ofmonitor_report(struct connmgr *mgr, struct rule *rule, ovs_mutex_unlock(&rule->mutex); if (flags & NXFMF_ACTIONS) { - fu.ofpacts = rule->actions->ofpacts; - fu.ofpacts_len = rule->actions->ofpacts_len; + struct rule_actions *actions = rule_get_actions(rule); + fu.ofpacts = actions->ofpacts; + fu.ofpacts_len = actions->ofpacts_len; } else { fu.ofpacts = NULL; fu.ofpacts_len = 0; diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 97c4e59ce..1ff80d946 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -1852,7 +1852,6 @@ xlate_recursively(struct xlate_ctx *ctx, struct rule_dpif *rule) ctx->rule = rule; actions = rule_dpif_get_actions(rule); do_xlate_actions(actions->ofpacts, actions->ofpacts_len, ctx); - rule_actions_unref(actions); ctx->rule = old_rule; ctx->recurse--; } @@ -3187,7 +3186,6 @@ xlate_actions__(struct xlate_in *xin, struct xlate_out *xout) } out: - rule_actions_unref(actions); rule_dpif_unref(rule); } diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index bda1d33a4..2cdf8568f 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -3593,8 +3593,6 @@ trace_format_rule(struct ds *result, int level, const struct rule_dpif *rule) ds_put_cstr(result, "OpenFlow actions="); ofpacts_format(actions->ofpacts, actions->ofpacts_len, result); ds_put_char(result, '\n'); - - rule_actions_unref(actions); } static void diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index d116451bd..28223fea7 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc. + * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -43,6 +43,7 @@ #include "ofp-util.h" #include "ofproto/ofproto.h" #include "ovs-atomic.h" +#include "ovs-rcu.h" #include "ovs-thread.h" #include "shash.h" #include "simap.h" @@ -374,7 +375,7 @@ struct rule { /* OpenFlow actions. See struct rule_actions for more thread-safety * notes. */ - struct rule_actions *actions OVS_GUARDED; + OVSRCU_TYPE(struct rule_actions *) actions; /* In owning meter's 'rules' list. An empty list if there is no meter. */ struct list meter_list_node OVS_GUARDED_BY(ofproto_mutex); @@ -403,10 +404,11 @@ struct rule { void ofproto_rule_ref(struct rule *); void ofproto_rule_unref(struct rule *); -struct rule_actions *rule_get_actions(const struct rule *rule) - OVS_EXCLUDED(rule->mutex); -struct rule_actions *rule_get_actions__(const struct rule *rule) - OVS_REQUIRES(rule->mutex); +static inline struct rule_actions * +rule_get_actions(const struct rule *rule) +{ + return ovsrcu_get(struct rule_actions *, &rule->actions); +} /* Returns true if 'rule' is an OpenFlow 1.3 "table-miss" rule, false * otherwise. @@ -431,8 +433,6 @@ rule_is_table_miss(const struct rule *rule) * 'rule' is the rule for which 'rule->actions == actions') or that owns a * reference to 'actions->ref_count' (or both). */ struct rule_actions { - struct ovs_refcount ref_count; - /* These members are immutable: they do not change during the struct's * lifetime. */ struct ofpact *ofpacts; /* Sequence of "struct ofpacts". */ @@ -442,8 +442,7 @@ struct rule_actions { struct rule_actions *rule_actions_create(const struct ofproto *, const struct ofpact *, size_t); -void rule_actions_ref(struct rule_actions *); -void rule_actions_unref(struct rule_actions *); +void rule_actions_destroy(struct rule_actions *); /* A set of rules to which an OpenFlow operation applies. */ struct rule_collection { diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 884e63ef8..c2f9c8b1d 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -43,6 +43,7 @@ #include "ofproto-provider.h" #include "openflow/nicira-ext.h" #include "openflow/openflow.h" +#include "ovs-rcu.h" #include "packets.h" #include "pinsched.h" #include "pktbuf.h" @@ -1910,11 +1911,9 @@ 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_mutex_lock(&rule->mutex); - must_add = !ofpacts_equal(rule->actions->ofpacts, - rule->actions->ofpacts_len, + struct rule_actions *actions = rule_get_actions(rule); + must_add = !ofpacts_equal(actions->ofpacts, actions->ofpacts_len, ofpacts, ofpacts_len); - ovs_mutex_unlock(&rule->mutex); } else { must_add = true; } @@ -1958,14 +1957,16 @@ ofproto_flow_mod(struct ofproto *ofproto, struct ofputil_flow_mod *fm) /* Reading many of the rule fields and writing on 'modified' * requires the rule->mutex. Also, rule->actions may change * if rule->mutex is not held. */ + const struct rule_actions *actions; + ovs_mutex_lock(&rule->mutex); + actions = rule_get_actions(rule); if (rule->idle_timeout == fm->idle_timeout && rule->hard_timeout == fm->hard_timeout && rule->flags == (fm->flags & OFPUTIL_FF_STATE) && (!fm->modify_cookie || (fm->new_cookie == rule->flow_cookie)) && ofpacts_equal(fm->ofpacts, fm->ofpacts_len, - rule->actions->ofpacts, - rule->actions->ofpacts_len)) { + actions->ofpacts, actions->ofpacts_len)) { /* Rule already exists and need not change, only update the modified timestamp. */ rule->modified = time_msec(); @@ -2587,33 +2588,12 @@ ofproto_rule_unref(struct rule *rule) } } -struct rule_actions * -rule_get_actions(const struct rule *rule) - OVS_EXCLUDED(rule->mutex) -{ - struct rule_actions *actions; - - ovs_mutex_lock(&rule->mutex); - actions = rule_get_actions__(rule); - ovs_mutex_unlock(&rule->mutex); - - return actions; -} - -struct rule_actions * -rule_get_actions__(const struct rule *rule) - OVS_REQUIRES(rule->mutex) -{ - rule_actions_ref(rule->actions); - return rule->actions; -} - static void ofproto_rule_destroy__(struct rule *rule) OVS_NO_THREAD_SAFETY_ANALYSIS { cls_rule_destroy(CONST_CAST(struct cls_rule *, &rule->cr)); - rule_actions_unref(rule->actions); + rule_actions_destroy(rule_get_actions(rule)); ovs_mutex_destroy(&rule->mutex); rule->ofproto->ofproto_class->rule_dealloc(rule); } @@ -2630,7 +2610,6 @@ rule_actions_create(const struct ofproto *ofproto, struct rule_actions *actions; actions = xmalloc(sizeof *actions); - ovs_refcount_init(&actions->ref_count); actions->ofpacts = xmemdup(ofpacts, ofpacts_len); actions->ofpacts_len = ofpacts_len; actions->provider_meter_id @@ -2640,23 +2619,20 @@ rule_actions_create(const struct ofproto *ofproto, return actions; } -/* Increments 'actions''s ref_count. */ -void -rule_actions_ref(struct rule_actions *actions) +static void +rule_actions_destroy_cb(struct rule_actions *actions) { - if (actions) { - ovs_refcount_ref(&actions->ref_count); - } + free(actions->ofpacts); + free(actions); } /* Decrements 'actions''s ref_count and frees 'actions' if the ref_count * reaches 0. */ void -rule_actions_unref(struct rule_actions *actions) +rule_actions_destroy(struct rule_actions *actions) { - if (actions && ovs_refcount_unref(&actions->ref_count) == 1) { - free(actions->ofpacts); - free(actions); + if (actions) { + ovsrcu_postpone(rule_actions_destroy_cb, actions); } } @@ -2666,9 +2642,13 @@ static bool ofproto_rule_has_out_port(const struct rule *rule, ofp_port_t port) OVS_REQUIRES(ofproto_mutex) { - return (port == OFPP_ANY - || ofpacts_output_to_port(rule->actions->ofpacts, - rule->actions->ofpacts_len, port)); + if (port == OFPP_ANY) { + return true; + } else { + const struct rule_actions *actions = rule_get_actions(rule); + return ofpacts_output_to_port(actions->ofpacts, + actions->ofpacts_len, port); + } } /* Returns true if 'rule' has group and equals group_id. */ @@ -2676,9 +2656,13 @@ static bool ofproto_rule_has_out_group(const struct rule *rule, uint32_t group_id) OVS_REQUIRES(ofproto_mutex) { - return (group_id == OFPG11_ANY - || ofpacts_output_to_group(rule->actions->ofpacts, - rule->actions->ofpacts_len, group_id)); + if (group_id == OFPG_ANY) { + return true; + } else { + const struct rule_actions *actions = rule_get_actions(rule); + return ofpacts_output_to_group(actions->ofpacts, + actions->ofpacts_len, group_id); + } } /* Returns true if a rule related to 'op' has an OpenFlow OFPAT_OUTPUT or @@ -3606,7 +3590,7 @@ handle_flow_stats_request(struct ofconn *ofconn, fs.hard_timeout = rule->hard_timeout; created = rule->created; modified = rule->modified; - actions = rule_get_actions__(rule); + actions = rule_get_actions(rule); flags = rule->flags; ovs_mutex_unlock(&rule->mutex); @@ -3624,8 +3608,6 @@ handle_flow_stats_request(struct ofconn *ofconn, fs.flags = flags; ofputil_append_flow_stats_reply(&fs, &replies); - - rule_actions_unref(actions); } rule_collection_unref(&rules); @@ -3647,7 +3629,7 @@ flow_stats_ds(struct rule *rule, struct ds *results) &byte_count, &used); ovs_mutex_lock(&rule->mutex); - actions = rule_get_actions__(rule); + actions = rule_get_actions(rule); created = rule->created; ovs_mutex_unlock(&rule->mutex); @@ -3664,8 +3646,6 @@ flow_stats_ds(struct rule *rule, struct ds *results) ofpacts_format(actions->ofpacts, actions->ofpacts_len, results); ds_put_cstr(results, "\n"); - - rule_actions_unref(actions); } /* Adds a pretty-printed description of all flows to 'results', including @@ -4070,7 +4050,8 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn, *CONST_CAST(uint8_t *, &rule->table_id) = table - ofproto->tables; rule->flags = fm->flags & OFPUTIL_FF_STATE; - rule->actions = rule_actions_create(ofproto, fm->ofpacts, fm->ofpacts_len); + ovsrcu_set(&rule->actions, + rule_actions_create(ofproto, fm->ofpacts, fm->ofpacts_len)); list_init(&rule->meter_list_node); rule->eviction_group = NULL; list_init(&rule->expirable); @@ -4121,6 +4102,7 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn, error = OFPERR_OFPBRC_EPERM; for (i = 0; i < rules->n; i++) { struct rule *rule = rules->rules[i]; + const struct rule_actions *actions; struct ofoperation *op; bool actions_changed; bool reset_counters; @@ -4134,9 +4116,10 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn, continue; } + actions = rule_get_actions(rule); actions_changed = !ofpacts_equal(fm->ofpacts, fm->ofpacts_len, - rule->actions->ofpacts, - rule->actions->ofpacts_len); + actions->ofpacts, + actions->ofpacts_len); op = ofoperation_create(group, rule, type, 0); @@ -4163,13 +4146,11 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn, if (actions_changed || reset_counters) { struct rule_actions *new_actions; - op->actions = rule->actions; + op->actions = rule_get_actions(rule); new_actions = rule_actions_create(ofproto, fm->ofpacts, fm->ofpacts_len); - ovs_mutex_lock(&rule->mutex); - rule->actions = new_actions; - ovs_mutex_unlock(&rule->mutex); + ovsrcu_set(&rule->actions, new_actions); rule->ofproto->ofproto_class->rule_modify_actions(rule, reset_counters); @@ -4750,7 +4731,7 @@ ofproto_compose_flow_refresh_update(const struct rule *rule, if (!(flags & NXFMF_ACTIONS)) { actions = NULL; } else if (!op) { - actions = rule->actions; + actions = rule_get_actions(rule); } else { /* An operation is in progress. Use the previous version of the flow's * actions, so that when the operation commits we report the change. */ @@ -4760,11 +4741,11 @@ ofproto_compose_flow_refresh_update(const struct rule *rule, case OFOPERATION_MODIFY: case OFOPERATION_REPLACE: - actions = op->actions ? op->actions : rule->actions; + actions = op->actions ? op->actions : rule_get_actions(rule); break; case OFOPERATION_DELETE: - actions = rule->actions; + actions = rule_get_actions(rule); break; default: @@ -6254,12 +6235,12 @@ ofopgroup_complete(struct ofopgroup *group) struct rule_actions *old_actions; ovs_mutex_lock(&rule->mutex); - old_actions = rule->actions; - rule->actions = op->actions; + old_actions = rule_get_actions(rule); + ovsrcu_set(&rule->actions, op->actions); ovs_mutex_unlock(&rule->mutex); op->actions = NULL; - rule_actions_unref(old_actions); + rule_actions_destroy(old_actions); } rule->flags = op->flags; } @@ -6345,7 +6326,7 @@ ofoperation_destroy(struct ofoperation *op) hmap_remove(&group->ofproto->deletions, &op->hmap_node); } list_remove(&op->group_node); - rule_actions_unref(op->actions); + rule_actions_destroy(op->actions); free(op); } @@ -6827,6 +6808,7 @@ oftable_insert_rule(struct rule *rule) { struct ofproto *ofproto = rule->ofproto; struct oftable *table = &ofproto->tables[rule->table_id]; + struct rule_actions *actions; bool may_expire; ovs_mutex_lock(&rule->mutex); @@ -6839,9 +6821,10 @@ oftable_insert_rule(struct rule *rule) cookies_insert(ofproto, rule); - if (rule->actions->provider_meter_id != UINT32_MAX) { - uint32_t meter_id = ofpacts_get_meter(rule->actions->ofpacts, - rule->actions->ofpacts_len); + actions = rule_get_actions(rule); + if (actions->provider_meter_id != UINT32_MAX) { + uint32_t meter_id = ofpacts_get_meter(actions->ofpacts, + actions->ofpacts_len); struct meter *meter = ofproto->meters[meter_id]; list_insert(&meter->rules, &rule->meter_list_node); } -- 2.43.0