[ovs-dev] [PATCH 10/11] ofproto: Break actions out of rule into new rule_actions structure.
Ben Pfaff
blp at nicira.com
Mon Sep 9 21:23:03 UTC 2013
This permits code to ensure long-term access to a rule's actions
without holding a long-term lock on the rule's rwlock.
Signed-off-by: Ben Pfaff <blp at nicira.com>
---
ofproto/connmgr.c | 4 +-
ofproto/ofproto-dpif-xlate.c | 16 +++--
ofproto/ofproto-dpif.c | 26 +++++---
ofproto/ofproto-dpif.h | 4 +-
ofproto/ofproto-provider.h | 28 ++++++++-
ofproto/ofproto.c | 140 ++++++++++++++++++++++++++++--------------
6 files changed, 151 insertions(+), 67 deletions(-)
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index 2f315e6..1edbd59 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -1903,8 +1903,8 @@ ofmonitor_report(struct connmgr *mgr, struct rule *rule,
ovs_mutex_unlock(&rule->timeout_mutex);
if (flags & NXFMF_ACTIONS) {
- fu.ofpacts = rule->ofpacts;
- fu.ofpacts_len = rule->ofpacts_len;
+ fu.ofpacts = rule->actions->ofpacts;
+ fu.ofpacts_len = rule->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 e7cec14..eb6a1f9 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -44,6 +44,7 @@
#include "ofproto/ofproto-dpif-mirror.h"
#include "ofproto/ofproto-dpif-sflow.h"
#include "ofproto/ofproto-dpif.h"
+#include "ofproto/ofproto-provider.h"
#include "tunnel.h"
#include "vlog.h"
@@ -1671,8 +1672,7 @@ xlate_recursively(struct xlate_ctx *ctx, struct rule_dpif *rule)
OVS_RELEASES(rule)
{
struct rule_dpif *old_rule = ctx->rule;
- const struct ofpact *ofpacts;
- size_t ofpacts_len;
+ struct rule_actions *actions;
if (ctx->xin->resubmit_stats) {
rule_dpif_credit_stats(rule, ctx->xin->resubmit_stats);
@@ -1680,8 +1680,9 @@ xlate_recursively(struct xlate_ctx *ctx, struct rule_dpif *rule)
ctx->recurse++;
ctx->rule = rule;
- rule_dpif_get_actions(rule, &ofpacts, &ofpacts_len);
- do_xlate_actions(ofpacts, ofpacts_len, ctx);
+ 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--;
@@ -2528,6 +2529,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
struct flow_wildcards *wc = &xout->wc;
struct flow *flow = &xin->flow;
+ struct rule_actions *actions = NULL;
enum slow_path_reason special;
const struct ofpact *ofpacts;
struct xport *in_port;
@@ -2604,7 +2606,9 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
ofpacts = xin->ofpacts;
ofpacts_len = xin->ofpacts_len;
} else if (xin->rule) {
- rule_dpif_get_actions(xin->rule, &ofpacts, &ofpacts_len);
+ actions = rule_dpif_get_actions(xin->rule);
+ ofpacts = actions->ofpacts;
+ ofpacts_len = actions->ofpacts_len;
} else {
NOT_REACHED();
}
@@ -2690,4 +2694,6 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
out:
ovs_rwlock_unlock(&xlate_rwlock);
+
+ rule_actions_unref(actions);
}
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 97735e2..cbaae5a 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4162,12 +4162,13 @@ facet_is_controller_flow(struct facet *facet)
bool is_controller;
rule_dpif_lookup(ofproto, &facet->flow, NULL, &rule);
- ofpacts_len = rule->up.ofpacts_len;
- ofpacts = rule->up.ofpacts;
+ ofpacts_len = rule->up.actions->ofpacts_len;
+ ofpacts = rule->up.actions->ofpacts;
is_controller = ofpacts_len > 0
&& ofpacts->type == OFPACT_CONTROLLER
&& ofpact_next(ofpacts) >= ofpact_end(ofpacts, ofpacts_len);
rule_dpif_release(rule);
+
return is_controller;
}
return false;
@@ -4519,12 +4520,20 @@ rule_dpif_reduce_timeouts(struct rule_dpif *rule, uint16_t idle_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)
+/* Returns 'rule''s actions. The caller owns a reference on the returned
+ * actions and must eventually release it (with rule_actions_unref()) to avoid
+ * a memory leak. */
+struct rule_actions *
+rule_dpif_get_actions(const struct rule_dpif *rule)
{
- *ofpacts = rule->up.ofpacts;
- *ofpacts_len = rule->up.ofpacts_len;
+ struct rule_actions *actions;
+
+ ovs_rwlock_rdlock(&rule->up.rwlock);
+ actions = rule->up.actions;
+ rule_actions_ref(actions);
+ ovs_rwlock_unlock(&rule->up.rwlock);
+
+ return actions;
}
/* Subfacets. */
@@ -5308,7 +5317,8 @@ trace_format_rule(struct ds *result, int level, const struct rule_dpif *rule)
ds_put_char_multiple(result, '\t', level);
ds_put_cstr(result, "OpenFlow ");
- ofpacts_format(rule->up.ofpacts, rule->up.ofpacts_len, result);
+ ofpacts_format(rule->up.actions->ofpacts, rule->up.actions->ofpacts_len,
+ result);
ds_put_char(result, '\n');
}
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index 7efc8d7..befd458 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -76,9 +76,7 @@ void rule_dpif_credit_stats(struct rule_dpif *rule ,
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);
+struct rule_actions *rule_dpif_get_actions(const struct rule_dpif *);
ovs_be64 rule_dpif_get_flow_cookie(const struct rule_dpif *rule);
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 0b8a5e5..f8b856e 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -253,10 +253,8 @@ struct rule {
struct ovs_rwlock rwlock;
/* Guarded by rwlock. */
- struct ofpact *ofpacts; /* Sequence of "struct ofpacts". */
- unsigned int ofpacts_len; /* Size of 'ofpacts', in bytes. */
+ struct rule_actions *actions;
- uint32_t meter_id; /* Non-zero OF meter_id, or zero. */
struct list meter_list_node; /* In owning meter's 'rules' list. */
/* Flow monitors. */
@@ -269,6 +267,30 @@ struct rule {
* is expirable, otherwise empty. */
};
+/* A set of actions within a "struct rule".
+ *
+ *
+ * Thread-safety
+ * =============
+ *
+ * A struct rule_actions 'actions' may be accessed without a risk of being
+ * freed by code that holds a read-lock or write-lock on 'rule->rwlock' (where
+ * 'rule' is the rule for which 'rule->actions == actions') or that owns a
+ * reference to 'actions->ref_count' (or both). */
+struct rule_actions {
+ atomic_uint ref_count;
+
+ /* These members are immutable: they do not change during the struct's
+ * lifetime. */
+ struct ofpact *ofpacts; /* Sequence of "struct ofpacts". */
+ unsigned int ofpacts_len; /* Size of 'ofpacts', in bytes. */
+ uint32_t meter_id; /* Non-zero OF meter_id, or zero. */
+};
+
+struct rule_actions *rule_actions_create(const struct ofpact *, size_t);
+void rule_actions_ref(struct rule_actions *);
+void rule_actions_unref(struct rule_actions *);
+
/* Threshold at which to begin flow table eviction. Only affects the
* ofproto-dpif implementation */
extern unsigned flow_eviction_threshold;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 458ff04..9399d41 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -124,9 +124,7 @@ struct ofoperation {
/* OFOPERATION_MODIFY, OFOPERATION_REPLACE: The old actions, if the actions
* are changing. */
- struct ofpact *ofpacts;
- size_t ofpacts_len;
- uint32_t meter_id;
+ struct rule_actions *actions;
/* OFOPERATION_DELETE. */
enum ofp_flow_removed_reason reason; /* Reason flow was removed. */
@@ -1741,20 +1739,29 @@ ofproto_add_flow(struct ofproto *ofproto, const struct match *match,
const struct ofpact *ofpacts, size_t ofpacts_len)
{
const struct rule *rule;
+ bool must_add;
/* First do a cheap check whether the rule we're looking for already exists
* with the actions that we want. If it does, then we're done. */
ovs_rwlock_rdlock(&ofproto->tables[0].cls.rwlock);
rule = rule_from_cls_rule(classifier_find_match_exactly(
&ofproto->tables[0].cls, match, priority));
+ if (rule) {
+ ovs_rwlock_rdlock(&rule->rwlock);
+ must_add = !ofpacts_equal(rule->actions->ofpacts,
+ rule->actions->ofpacts_len,
+ ofpacts, ofpacts_len);
+ ovs_rwlock_unlock(&rule->rwlock);
+ } else {
+ must_add = true;
+ }
ovs_rwlock_unlock(&ofproto->tables[0].cls.rwlock);
/* If there's no such rule or the rule doesn't have the actions we want,
* fall back to a executing a full flow mod. We can't optimize this at
* all because we didn't take enough locks above to ensure that the flow
* table didn't already change beneath us. */
- if (!rule || !ofpacts_equal(rule->ofpacts, rule->ofpacts_len,
- ofpacts, ofpacts_len)) {
+ if (must_add) {
simple_flow_mod(ofproto, match, priority, ofpacts, ofpacts_len,
OFPFC_MODIFY_STRICT);
}
@@ -2300,19 +2307,64 @@ static void
ofproto_rule_destroy__(struct rule *rule)
{
cls_rule_destroy(&rule->cr);
- free(rule->ofpacts);
+ rule_actions_unref(rule->actions);
ovs_mutex_destroy(&rule->timeout_mutex);
ovs_rwlock_destroy(&rule->rwlock);
rule->ofproto->ofproto_class->rule_dealloc(rule);
}
+/* Creates and returns a new 'struct rule_actions', with a ref_count of 1,
+ * whose actions are a copy of from the 'ofpacts_len' bytes of 'ofpacts'. */
+struct rule_actions *
+rule_actions_create(const struct ofpact *ofpacts, size_t ofpacts_len)
+{
+ struct rule_actions *actions;
+
+ actions = xmalloc(sizeof *actions);
+ atomic_init(&actions->ref_count, 1);
+ actions->ofpacts = xmemdup(ofpacts, ofpacts_len);
+ actions->ofpacts_len = ofpacts_len;
+ actions->meter_id = ofpacts_get_meter(ofpacts, ofpacts_len);
+ return actions;
+}
+
+/* Increments 'actions''s ref_count. */
+void
+rule_actions_ref(struct rule_actions *actions)
+{
+ if (actions) {
+ unsigned int orig;
+
+ atomic_add(&actions->ref_count, 1, &orig);
+ ovs_assert(orig != 0);
+ }
+}
+
+/* Decrements 'actions''s ref_count and frees 'actions' if the ref_count
+ * reaches 0. */
+void
+rule_actions_unref(struct rule_actions *actions)
+{
+ if (actions) {
+ unsigned int orig;
+
+ atomic_sub(&actions->ref_count, 1, &orig);
+ if (orig == 1) {
+ free(actions);
+ } else {
+ ovs_assert(orig != 0);
+ }
+ }
+}
+
/* Returns true if 'rule' has an OpenFlow OFPAT_OUTPUT or OFPAT_ENQUEUE action
* that outputs to 'port' (output to OFPP_FLOOD and OFPP_ALL doesn't count). */
bool
ofproto_rule_has_out_port(const struct rule *rule, ofp_port_t port)
{
return (port == OFPP_ANY
- || ofpacts_output_to_port(rule->ofpacts, rule->ofpacts_len, port));
+ || ofpacts_output_to_port(rule->actions->ofpacts,
+ rule->actions->ofpacts_len, port));
}
/* Returns true if 'rule' has group and equals group_id. */
@@ -2320,7 +2372,8 @@ bool
ofproto_rule_has_out_group(const struct rule *rule, uint32_t group_id)
{
return (group_id == OFPG11_ANY
- || ofpacts_output_to_group(rule->ofpacts, rule->ofpacts_len, group_id));
+ || ofpacts_output_to_group(rule->actions->ofpacts,
+ rule->actions->ofpacts_len, group_id));
}
/* Returns true if a rule related to 'op' has an OpenFlow OFPAT_OUTPUT or
@@ -2339,7 +2392,8 @@ ofoperation_has_out_port(const struct ofoperation *op, ofp_port_t out_port)
case OFOPERATION_MODIFY:
case OFOPERATION_REPLACE:
- return ofpacts_output_to_port(op->ofpacts, op->ofpacts_len, out_port);
+ return ofpacts_output_to_port(op->actions->ofpacts,
+ op->actions->ofpacts_len, out_port);
}
NOT_REACHED();
@@ -3124,8 +3178,8 @@ handle_flow_stats_request(struct ofconn *ofconn,
fs.hard_age = age_secs(now - rule->modified);
ofproto->ofproto_class->rule_get_stats(rule, &fs.packet_count,
&fs.byte_count);
- fs.ofpacts = rule->ofpacts;
- fs.ofpacts_len = rule->ofpacts_len;
+ fs.ofpacts = rule->actions->ofpacts;
+ fs.ofpacts_len = rule->actions->ofpacts_len;
ovs_mutex_lock(&rule->timeout_mutex);
fs.idle_timeout = rule->idle_timeout;
@@ -3163,7 +3217,8 @@ flow_stats_ds(struct rule *rule, struct ds *results)
ds_put_format(results, "n_bytes=%"PRIu64", ", byte_count);
cls_rule_format(&rule->cr, results);
ds_put_char(results, ',');
- ofpacts_format(rule->ofpacts, rule->ofpacts_len, results);
+ ofpacts_format(rule->actions->ofpacts, rule->actions->ofpacts_len,
+ results);
ds_put_cstr(results, "\n");
}
@@ -3553,9 +3608,7 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
rule->table_id = table - ofproto->tables;
rule->send_flow_removed = (fm->flags & OFPUTIL_FF_SEND_FLOW_REM) != 0;
- rule->ofpacts = xmemdup(fm->ofpacts, fm->ofpacts_len);
- rule->ofpacts_len = fm->ofpacts_len;
- rule->meter_id = ofpacts_get_meter(rule->ofpacts, rule->ofpacts_len);
+ rule->actions = rule_actions_create(fm->ofpacts, fm->ofpacts_len);
list_init(&rule->meter_list_node);
rule->eviction_group = NULL;
list_init(&rule->expirable);
@@ -3626,7 +3679,8 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn,
}
actions_changed = !ofpacts_equal(fm->ofpacts, fm->ofpacts_len,
- rule->ofpacts, rule->ofpacts_len);
+ rule->actions->ofpacts,
+ rule->actions->ofpacts_len);
op = ofoperation_create(group, rule, type, 0);
@@ -3653,17 +3707,15 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn,
reset_counters = (fm->flags & OFPUTIL_FF_RESET_COUNTS) != 0;
if (actions_changed || reset_counters) {
- op->ofpacts = rule->ofpacts;
- op->ofpacts_len = rule->ofpacts_len;
- op->meter_id = rule->meter_id;
+ struct rule_actions *new_actions;
+
+ op->actions = rule->actions;
+ new_actions = rule_actions_create(fm->ofpacts, fm->ofpacts_len);
ovs_rwlock_wrlock(&rule->rwlock);
- rule->ofpacts = xmemdup(fm->ofpacts, fm->ofpacts_len);
- rule->ofpacts_len = fm->ofpacts_len;
+ rule->actions = new_actions;
ovs_rwlock_unlock(&rule->rwlock);
- rule->meter_id = ofpacts_get_meter(rule->ofpacts,
- rule->ofpacts_len);
rule->ofproto->ofproto_class->rule_modify_actions(rule,
reset_counters);
} else {
@@ -4168,6 +4220,7 @@ ofproto_compose_flow_refresh_update(const struct rule *rule,
struct list *msgs)
{
struct ofoperation *op = rule->pending;
+ const struct rule_actions *actions;
struct ofputil_flow_update fu;
struct match match;
@@ -4189,12 +4242,11 @@ ofproto_compose_flow_refresh_update(const struct rule *rule,
minimatch_expand(&rule->cr.match, &match);
fu.match = &match;
fu.priority = rule->cr.priority;
+
if (!(flags & NXFMF_ACTIONS)) {
- fu.ofpacts = NULL;
- fu.ofpacts_len = 0;
+ actions = NULL;
} else if (!op) {
- fu.ofpacts = rule->ofpacts;
- fu.ofpacts_len = rule->ofpacts_len;
+ actions = rule->actions;
} 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. */
@@ -4204,24 +4256,19 @@ ofproto_compose_flow_refresh_update(const struct rule *rule,
case OFOPERATION_MODIFY:
case OFOPERATION_REPLACE:
- if (op->ofpacts) {
- fu.ofpacts = op->ofpacts;
- fu.ofpacts_len = op->ofpacts_len;
- } else {
- fu.ofpacts = rule->ofpacts;
- fu.ofpacts_len = rule->ofpacts_len;
- }
+ actions = op->actions ? op->actions : rule->actions;
break;
case OFOPERATION_DELETE:
- fu.ofpacts = rule->ofpacts;
- fu.ofpacts_len = rule->ofpacts_len;
+ actions = rule->actions;
break;
default:
NOT_REACHED();
}
}
+ fu.ofpacts = actions ? actions->ofpacts : NULL;
+ fu.ofpacts_len = actions ? actions->ofpacts_len : 0;
if (list_is_empty(msgs)) {
ofputil_start_flow_update(msgs);
@@ -5444,7 +5491,7 @@ ofopgroup_complete(struct ofopgroup *group)
if (!(op->error
|| ofproto_rule_is_hidden(rule)
|| (op->type == OFOPERATION_MODIFY
- && op->ofpacts
+ && op->actions
&& rule->flow_cookie == op->flow_cookie))) {
/* Check that we can just cast from ofoperation_type to
* nx_flow_update_event. */
@@ -5519,16 +5566,16 @@ ofopgroup_complete(struct ofopgroup *group)
rule->idle_timeout = op->idle_timeout;
rule->hard_timeout = op->hard_timeout;
ovs_mutex_unlock(&rule->timeout_mutex);
- if (op->ofpacts) {
- free(rule->ofpacts);
+ if (op->actions) {
+ struct rule_actions *old_actions;
ovs_rwlock_wrlock(&rule->rwlock);
- rule->ofpacts = op->ofpacts;
- rule->ofpacts_len = op->ofpacts_len;
+ old_actions = rule->actions;
+ rule->actions = op->actions;
ovs_rwlock_unlock(&rule->rwlock);
- op->ofpacts = NULL;
- op->ofpacts_len = 0;
+ op->actions = NULL;
+ rule_actions_unref(old_actions);
}
rule->send_flow_removed = op->send_flow_removed;
}
@@ -5612,7 +5659,7 @@ ofoperation_destroy(struct ofoperation *op)
hmap_remove(&group->ofproto->deletions, &op->hmap_node);
}
list_remove(&op->group_node);
- free(op->ofpacts);
+ rule_actions_unref(op->actions);
free(op);
}
@@ -6110,8 +6157,9 @@ oftable_insert_rule(struct rule *rule)
ovs_mutex_unlock(&ofproto->expirable_mutex);
}
cookies_insert(ofproto, rule);
- if (rule->meter_id) {
- struct meter *meter = ofproto->meters[rule->meter_id];
+
+ if (rule->actions->meter_id) {
+ struct meter *meter = ofproto->meters[rule->actions->meter_id];
list_insert(&meter->rules, &rule->meter_list_node);
}
ovs_rwlock_wrlock(&table->cls.rwlock);
--
1.7.10.4
More information about the dev
mailing list