[ovs-dev] [RFC urcu 2/2] ofproto: Protect rule_actions with RCU.
Ben Pfaff
blp at nicira.com
Tue Oct 15 22:47:54 UTC 2013
---
ofproto/connmgr.c | 1 +
ofproto/ofproto-dpif-xlate.c | 9 +++--
ofproto/ofproto-dpif.c | 7 ++--
ofproto/ofproto-provider.h | 14 +++-----
ofproto/ofproto.c | 81 ++++++++++++++++--------------------------
5 files changed, 48 insertions(+), 64 deletions(-)
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index 02da1f6..9f69c3f 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -20,6 +20,7 @@
#include <errno.h>
#include <stdlib.h>
+#include <urcu-qsbr.h>
#include "coverage.h"
#include "fail-open.h"
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 9313a7f..9b24bdb 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -17,6 +17,7 @@
#include "ofproto/ofproto-dpif-xlate.h"
#include <errno.h>
+#include <urcu-qsbr.h>
#include "bfd.h"
#include "bitmap.h"
@@ -1707,9 +1708,10 @@ xlate_recursively(struct xlate_ctx *ctx, struct rule_dpif *rule)
ctx->resubmits++;
ctx->recurse++;
ctx->rule = rule;
+ rcu_read_lock();
actions = rule_dpif_get_actions(rule);
do_xlate_actions(actions->ofpacts, actions->ofpacts_len, ctx);
- rule_actions_unref(actions);
+ rcu_read_unlock();
ctx->rule = old_rule;
ctx->recurse--;
}
@@ -2780,6 +2782,7 @@ xlate_actions__(struct xlate_in *xin, struct xlate_out *xout)
ofpacts = xin->ofpacts;
ofpacts_len = xin->ofpacts_len;
} else if (ctx.rule) {
+ rcu_read_lock();
actions = rule_dpif_get_actions(ctx.rule);
ofpacts = actions->ofpacts;
ofpacts_len = actions->ofpacts_len;
@@ -2883,7 +2886,9 @@ xlate_actions__(struct xlate_in *xin, struct xlate_out *xout)
memset(&wc->masks.regs, 0, sizeof wc->masks.regs);
out:
- rule_actions_unref(actions);
+ if (actions) {
+ rcu_read_unlock();
+ }
rule_dpif_unref(rule);
}
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index e53bb25..dcf6184 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -20,6 +20,7 @@
#include "ofproto/ofproto-provider.h"
#include <errno.h>
+#include <urcu-qsbr.h>
#include "bfd.h"
#include "bond.h"
@@ -3971,6 +3972,7 @@ facet_is_controller_flow(struct facet *facet)
bool is_controller;
rule_dpif_lookup(ofproto, &facet->flow, NULL, &rule);
+ rcu_read_lock();
actions = rule_dpif_get_actions(rule);
rule_dpif_unref(rule);
@@ -3979,7 +3981,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_actions_unref(actions);
+ rcu_read_unlock();
return is_controller;
}
@@ -5052,6 +5054,7 @@ trace_format_rule(struct ds *result, int level, const struct rule_dpif *rule)
cls_rule_format(&rule->up.cr, result);
ds_put_char(result, '\n');
+ rcu_read_lock();
actions = rule_dpif_get_actions(rule);
ds_put_char_multiple(result, '\t', level);
@@ -5059,7 +5062,7 @@ trace_format_rule(struct ds *result, int level, const struct rule_dpif *rule)
ofpacts_format(actions->ofpacts, actions->ofpacts_len, result);
ds_put_char(result, '\n');
- rule_actions_unref(actions);
+ rcu_read_unlock();
}
static void
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index de566e3..c00edfc 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -33,6 +33,7 @@
* structures for details.
*/
+#include <urcu-call-rcu.h>
#include "cfm.h"
#include "classifier.h"
#include "guarded-list.h"
@@ -371,7 +372,7 @@ struct rule {
/* OpenFlow actions. See struct rule_actions for more thread-safety
* notes. */
- struct rule_actions *actions OVS_GUARDED;
+ 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);
@@ -393,10 +394,7 @@ 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);
+struct rule_actions *rule_get_actions(const struct rule *);
/* A set of actions within a "struct rule".
*
@@ -409,10 +407,9 @@ struct rule_actions *rule_get_actions__(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 {
- atomic_uint ref_count;
-
/* These members are immutable: they do not change during the struct's
* lifetime. */
+ struct rcu_head rcu_head;
struct ofpact *ofpacts; /* Sequence of "struct ofpacts". */
unsigned int ofpacts_len; /* Size of 'ofpacts', in bytes. */
uint32_t provider_meter_id; /* Datapath meter_id, or UINT32_MAX. */
@@ -420,8 +417,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 fe004b5..3185586 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -22,6 +22,7 @@
#include <stdbool.h>
#include <stdlib.h>
#include <unistd.h>
+#include <urcu-qsbr.h>
#include "bitmap.h"
#include "byte-order.h"
#include "classifier.h"
@@ -1850,11 +1851,13 @@ 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;
+
+ rcu_read_lock();
+ actions = rule_get_actions(rule);
+ must_add = !ofpacts_equal(actions->ofpacts, actions->ofpacts_len,
ofpacts, ofpacts_len);
- ovs_mutex_unlock(&rule->mutex);
+ rcu_read_unlock();
} else {
must_add = true;
}
@@ -2474,23 +2477,8 @@ 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;
+ return rcu_dereference(rule->actions);
}
static void
@@ -2498,7 +2486,7 @@ 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->actions);
ovs_mutex_destroy(&rule->mutex);
rule->ofproto->ofproto_class->rule_dealloc(rule);
}
@@ -2515,7 +2503,6 @@ rule_actions_create(const struct ofproto *ofproto,
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->provider_meter_id
@@ -2525,33 +2512,23 @@ 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 rcu_head *rcu_head)
{
- if (actions) {
- unsigned int orig;
+ struct rule_actions *actions
+ = CONTAINER_OF(rcu_head, struct rule_actions, rcu_head);
- atomic_add(&actions->ref_count, 1, &orig);
- ovs_assert(orig != 0);
- }
+ 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) {
- unsigned int orig;
-
- atomic_sub(&actions->ref_count, 1, &orig);
- if (orig == 1) {
- free(actions->ofpacts);
- free(actions);
- } else {
- ovs_assert(orig != 0);
- }
+ call_rcu(&actions->rcu_head, rule_actions_destroy_cb);
}
}
@@ -3502,7 +3479,8 @@ handle_flow_stats_request(struct ofconn *ofconn,
created = rule->created;
used = rule->used;
modified = rule->modified;
- actions = rule_get_actions__(rule);
+ rcu_read_lock();
+ actions = rule_get_actions(rule);
flags = rule->flags;
ovs_mutex_unlock(&rule->mutex);
@@ -3520,7 +3498,7 @@ handle_flow_stats_request(struct ofconn *ofconn,
fs.flags = flags;
ofputil_append_flow_stats_reply(&fs, &replies);
- rule_actions_unref(actions);
+ rcu_read_unlock();
}
rule_collection_unref(&rules);
@@ -3542,7 +3520,8 @@ flow_stats_ds(struct rule *rule, struct ds *results)
&packet_count, &byte_count);
ovs_mutex_lock(&rule->mutex);
- actions = rule_get_actions__(rule);
+ rcu_read_lock();
+ actions = rule_get_actions(rule);
created = rule->created;
ovs_mutex_unlock(&rule->mutex);
@@ -3560,7 +3539,7 @@ flow_stats_ds(struct rule *rule, struct ds *results)
ds_put_cstr(results, "\n");
- rule_actions_unref(actions);
+ rcu_read_unlock();
}
/* Adds a pretty-printed description of all flows to 'results', including
@@ -3972,7 +3951,9 @@ 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);
+ rcu_assign_pointer(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);
@@ -4076,9 +4057,7 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn,
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);
+ rcu_assign_pointer(rule->actions, new_actions);
rule->ofproto->ofproto_class->rule_modify_actions(rule,
reset_counters);
@@ -6032,11 +6011,11 @@ ofopgroup_complete(struct ofopgroup *group)
ovs_mutex_lock(&rule->mutex);
old_actions = rule->actions;
- rule->actions = op->actions;
+ rcu_assign_pointer(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;
}
@@ -6122,7 +6101,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);
}
--
1.7.10.4
More information about the dev
mailing list