My only comment is that you might consider using thread safety
annotations to force users do un-reference actions they've taken a
reference of. That might be a pain though, not sure if it's worth it.
Acked-by: Ethan Jackson <et...@nicira.com>
On Tue, Sep 10, 2013 at 10:27 PM, Ben Pfaff <b...@nicira.com> wrote:
> 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 <b...@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
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
