Acked-by: Andy Zhou <az...@nicira.com>
On Tue, Mar 11, 2014 at 1:56 PM, Ben Pfaff <b...@nicira.com> wrote:
> Signed-off-by: Ben Pfaff <b...@nicira.com>
>
> Conflicts:
> lib/ovs-rcu.h
> ---
> 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 033ab7d..0058309 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 eb4931e..5201b93 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -1836,7 +1836,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--;
> }
> @@ -3171,7 +3170,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 8c43ee9..1c779ee 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -3587,8 +3587,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 2c72fbc..956e593 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 d61e618..d0daea4 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"
> @@ -1897,11 +1898,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;
> }
> @@ -1945,14 +1944,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();
> @@ -2574,33 +2575,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);
> }
> @@ -2617,7 +2597,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
> @@ -2627,23 +2606,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);
> }
> }
>
> @@ -2653,9 +2629,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. */
> @@ -2663,9 +2643,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
> @@ -3593,7 +3577,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);
>
> @@ -3611,8 +3595,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);
> @@ -3634,7 +3616,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);
>
> @@ -3651,8 +3633,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
> @@ -4057,7 +4037,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);
> @@ -4108,6 +4089,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;
> @@ -4121,9 +4103,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);
>
> @@ -4150,13 +4133,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);
> @@ -4737,7 +4718,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. */
> @@ -4747,11 +4728,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:
> @@ -6241,12 +6222,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;
> }
> @@ -6332,7 +6313,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);
> }
>
> @@ -6814,6 +6795,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);
> @@ -6826,9 +6808,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);
> }
> --
> 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
