In add_flow, should we take the evict lock on the rule while we're
still holding the classifier readlock? It doesn't matter much if
add_flow will only be called by the main thread, however if that isn't
the case, we don't want the rule we're working on to be evicted from
under us.
There's a serious problem with this code which I think pre-dated this
patch. Specifically, modify_flows__() can change a rule's actions
without holding a lock. If some child thread is using those actions
to do xlation, I think we'll run into trouble.
I'll put some thought into this.
Acked-by: Ethan Jackson <et...@nicira.com>
On Mon, Aug 26, 2013 at 5:10 PM, Ben Pfaff <b...@nicira.com> wrote:
> add_flow() in ofproto.c has a race: it adds a new flow to the flow table
> before calling ->rule_construct(). That means that (in ofproto-dpif) the
> new flow is visible to the forwarder threads before gets properly
> initialized.
>
> One solution would be to lock the flow table across the entire operation,
> but this is not desirable:
>
> * We would need a write-lock but this would be expensive for
> implementing "learn" flow_mods that often don't really modify anything
> at all.
>
> * The code would need to keep the lock across a couple of differen calls
> into the client, which seems undesirable if it can be avoided.
>
> * The code in add_flow() is difficult to understand already.
>
> Instead, I've decided to refactor add_flow() to simplify it and make it
> easier to understand. I think this will also improve the potential for
> concurrency.
>
> This commit starts off by collapsing two different cases together into
> (almost) one. In particular, OpenFlow has two ways to modify a flow with a
> "flow_mod" command. You can use an "add" flow_mod to replace the flow, or
> you can use a "modify" flow_mod to change it. The differences in semantics
> are minor, but until now Open vSwitch has treated them quite differently.
> This commit merges both cases, treating them as variants of what was
> previously a "modify". The advantage is that add_flow() no longer has to
> deal with two flows at a time in the "add" case (the old flow being
> deleted, the new flow replacing that one). This does not fix the race, but
> it makes it easier to deal with in a later commit.
>
> Transforming "add" into a form of "modify" requires that "modify" be able
> to reset flow statistic counters. OpenFlow 1.2 and later make this an
> optional flag in a "flow_mod". Until now, we haven't implemented that
> feature of OF1.2+, but we get it pretty much for free with this
> refactoring, so this commit also adds that OF1.2+ feature too.
>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> OPENFLOW-1.1+ | 3 -
> ofproto/ofproto-dpif.c | 9 +-
> ofproto/ofproto-provider.h | 47 +++-----
> ofproto/ofproto.c | 271
> ++++++++++++++++++++++++--------------------
> 4 files changed, 174 insertions(+), 156 deletions(-)
>
> diff --git a/OPENFLOW-1.1+ b/OPENFLOW-1.1+
> index 190dd29..753dbba 100644
> --- a/OPENFLOW-1.1+
> +++ b/OPENFLOW-1.1+
> @@ -119,9 +119,6 @@ end of the OF1.2 spec. I didn't compare the specs
> carefully yet.)
>
> * OFPT_FLOW_MOD:
>
> - * New flag OFPFF_RESET_COUNTS.
> - [required for OF1.2+]
> -
> * Add ability to delete flow in all tables.
> [required for OF1.2+]
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 80c7c4c..8d82512 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -4955,10 +4955,17 @@ rule_execute(struct rule *rule, const struct flow
> *flow,
> }
>
> static void
> -rule_modify_actions(struct rule *rule_)
> +rule_modify_actions(struct rule *rule_, bool reset_counters)
> {
> struct rule_dpif *rule = rule_dpif_cast(rule_);
>
> + if (reset_counters) {
> + ovs_mutex_lock(&rule->stats_mutex);
> + rule->packet_count = 0;
> + rule->byte_count = 0;
> + ovs_mutex_unlock(&rule->stats_mutex);
> + }
> +
> complete_operation(rule);
> }
>
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 72ba3be..43273ec 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -288,7 +288,6 @@ void ofproto_rule_reduce_timeouts(struct rule *rule,
> uint16_t idle_timeout,
> bool ofproto_rule_has_out_port(const struct rule *, ofp_port_t out_port);
>
> void ofoperation_complete(struct ofoperation *, enum ofperr);
> -struct rule *ofoperation_get_victim(struct ofoperation *);
>
> bool ofoperation_has_out_port(const struct ofoperation *, ofp_port_t
> out_port);
>
> @@ -890,7 +889,7 @@ struct ofproto_class {
> * An ofproto implementation reports the success or failure of an
> * asynchronous operation on a rule using the rule's 'pending' member,
> * which points to a opaque "struct ofoperation" that represents the
> - * ongoing opreation. When the operation completes, the ofproto
> + * ongoing operation. When the operation completes, the ofproto
> * implementation calls ofoperation_complete(), passing the ofoperation
> and
> * an error indication.
> *
> @@ -907,22 +906,22 @@ struct ofproto_class {
> * The ofproto base code updates the flow table optimistically, assuming
> * that the operation will probably succeed:
> *
> - * - ofproto adds or replaces the rule in the flow table before calling
> + * - ofproto adds the rule in the flow table before calling
> * ->rule_construct().
> *
> - * - ofproto updates the rule's actions before calling
> - * ->rule_modify_actions().
> + * - ofproto updates the rule's actions and other properties before
> + * calling ->rule_modify_actions().
> *
> * - ofproto removes the rule before calling ->rule_destruct().
> *
> * With one exception, when an asynchronous operation completes with an
> * error, ofoperation_complete() backs out the already applied changes:
> *
> - * - If adding or replacing a rule in the flow table fails, ofproto
> - * removes the new rule or restores the original rule.
> + * - If adding a rule in the flow table fails, ofproto removes the new
> + * rule.
> *
> - * - If modifying a rule's actions fails, ofproto restores the original
> - * actions.
> + * - If modifying a rule fails, ofproto restores the original actions
> + * (and other properties).
> *
> * - Removing a rule is not allowed to fail. It must always succeed.
> *
> @@ -938,17 +937,9 @@ struct ofproto_class {
> * Construction
> * ============
> *
> - * When ->rule_construct() is called, the caller has already inserted
> - * 'rule' into 'rule->ofproto''s flow table numbered 'rule->table_id'.
> - * There are two cases:
> - *
> - * - 'rule' is a new rule in its flow table. In this case,
> - * ofoperation_get_victim(rule) returns NULL.
> - *
> - * - 'rule' is replacing an existing rule in its flow table that had
> the
> - * same matching criteria and priority. In this case,
> - * ofoperation_get_victim(rule) returns the rule being replaced (the
> - * "victim" rule).
> + * When ->rule_construct() is called, 'rule' is a new rule in its flow
> + * table. The caller has already inserted 'rule' into 'rule->ofproto''s
> + * flow table numbered 'rule->table_id'.
> *
> * ->rule_construct() should set the following in motion:
> *
> @@ -959,16 +950,10 @@ struct ofproto_class {
> *
> * - Validate that the datapath can correctly implement
> 'rule->ofpacts'.
> *
> - * - If the rule is valid, update the datapath flow table, adding the
> new
> - * rule or replacing the existing one.
> - *
> - * - If 'rule' is replacing an existing rule, uninitialize any derived
> - * state for the victim rule, as in step 5 in the "Life Cycle"
> - * described above.
> + * - If the rule is valid, add the new rule to the datapath flow table.
> *
> * (On failure, the ofproto code will roll back the insertion from the
> flow
> - * table, either removing 'rule' or replacing it by the victim rule if
> - * there is one.)
> + * table by removing 'rule'.)
> *
> * ->rule_construct() must act in one of the following ways:
> *
> @@ -1044,6 +1029,10 @@ struct ofproto_class {
> *
> * - Update the datapath flow table with the new actions.
> *
> + * - Only if 'reset_counters' is true, reset any packet or byte
> counters
> + * associated with the rule to zero, so that rule_get_stats() will
> not
> + * longer count those packets or bytes.
> + *
> * If the operation synchronously completes, ->rule_modify_actions() may
> * call ofoperation_complete() before it returns. Otherwise, ->run()
> * should call ofoperation_complete() later, after the operation does
> @@ -1054,7 +1043,7 @@ struct ofproto_class {
> *
> * ->rule_modify_actions() should not modify any base members of struct
> * rule. */
> - void (*rule_modify_actions)(struct rule *rule);
> + void (*rule_modify_actions)(struct rule *rule, bool reset_counters);
>
> /* Changes the OpenFlow IP fragment handling policy to 'frag_handling',
> * which takes one of the following values, with the corresponding
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 080592e..623039b 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -76,7 +76,8 @@ enum ofproto_state {
> enum ofoperation_type {
> OFOPERATION_ADD,
> OFOPERATION_DELETE,
> - OFOPERATION_MODIFY
> + OFOPERATION_MODIFY,
> + OFOPERATION_REPLACE
> };
>
> /* A single OpenFlow request can execute any number of operations. The
> @@ -121,10 +122,8 @@ struct ofoperation {
> struct rule *rule; /* Rule being operated upon. */
> enum ofoperation_type type; /* Type of operation. */
>
> - /* OFOPERATION_ADD. */
> - struct rule *victim; /* Rule being replaced, if any.. */
> -
> - /* OFOPERATION_MODIFY: The old actions, if the actions are changing. */
> + /* OFOPERATION_MODIFY, OFOPERATION_REPLACE: The old actions, if the
> actions
> + * are changing. */
> struct ofpact *ofpacts;
> size_t ofpacts_len;
> uint32_t meter_id;
> @@ -133,6 +132,9 @@ struct ofoperation {
> enum ofp_flow_removed_reason reason; /* Reason flow was removed. */
>
> ovs_be64 flow_cookie; /* Rule's old flow cookie. */
> + uint16_t idle_timeout; /* Rule's old idle timeout. */
> + uint16_t hard_timeout; /* Rule's old hard timeout. */
> + bool send_flow_removed; /* Rule's old 'send_flow_removed'. */
> enum ofperr error; /* 0 if no error. */
> };
>
> @@ -157,8 +159,7 @@ static void oftable_remove_rule(struct rule *rule)
> OVS_RELEASES(rule->evict);
> static void oftable_remove_rule__(struct ofproto *ofproto,
> struct classifier *cls, struct rule *rule)
> OVS_REQ_WRLOCK(cls->rwlock) OVS_RELEASES(rule->evict);
> -static struct rule *oftable_replace_rule(struct rule *);
> -static void oftable_substitute_rule(struct rule *old, struct rule *new);
> +static void oftable_insert_rule(struct rule *);
>
> /* A set of rules within a single OpenFlow table (oftable) that have the same
> * values for the oftable's eviction_fields. A rule to be evicted, when one
> is
> @@ -186,7 +187,8 @@ static bool choose_rule_to_evict(struct oftable *table,
> struct rule **rulep)
> OVS_TRY_WRLOCK(true, (*rulep)->evict);
> static void ofproto_evict(struct ofproto *);
> static uint32_t rule_eviction_priority(struct rule *);
> -static void eviction_group_add_rule(struct rule *rule);
> +static void eviction_group_add_rule(struct rule *);
> +static void eviction_group_remove_rule(struct rule *);
>
> /* ofport. */
> static void ofport_destroy__(struct ofport *);
> @@ -205,6 +207,9 @@ static bool rule_is_modifiable(const struct rule *);
> static enum ofperr add_flow(struct ofproto *, struct ofconn *,
> struct ofputil_flow_mod *,
> const struct ofp_header *);
> +static enum ofperr modify_flows__(struct ofproto *, struct ofconn *,
> + struct ofputil_flow_mod *,
> + const struct ofp_header *, struct list *);
> static void delete_flow__(struct rule *rule, struct ofopgroup *,
> enum ofp_flow_removed_reason)
> OVS_RELEASES(rule->evict);
> @@ -2246,12 +2251,11 @@ ofoperation_has_out_port(const struct ofoperation
> *op, ofp_port_t out_port)
>
> switch (op->type) {
> case OFOPERATION_ADD:
> - return op->victim && ofproto_rule_has_out_port(op->victim, out_port);
> -
> case OFOPERATION_DELETE:
> return false;
>
> case OFOPERATION_MODIFY:
> + case OFOPERATION_REPLACE:
> return ofpacts_output_to_port(op->ofpacts, op->ofpacts_len,
> out_port);
> }
>
> @@ -3357,8 +3361,10 @@ add_flow(struct ofproto *ofproto, struct ofconn
> *ofconn,
> {
> struct oftable *table;
> struct ofopgroup *group;
> - struct rule *victim;
> + struct ofoperation *op;
> + struct rule *evict;
> struct rule *rule;
> + size_t n_rules;
> uint8_t table_id;
> int error;
>
> @@ -3392,6 +3398,27 @@ add_flow(struct ofproto *ofproto, struct ofconn
> *ofconn,
> return OFPERR_OFPBRC_EPERM;
> }
>
> + /* Transform "add" into "modify" if there's an existing identical flow.
> */
> + ovs_rwlock_rdlock(&table->cls.rwlock);
> + rule = rule_from_cls_rule(classifier_find_match_exactly(&table->cls,
> + &fm->match,
> + fm->priority));
> + ovs_rwlock_unlock(&table->cls.rwlock);
> + if (rule) {
> + if (!rule_is_modifiable(rule)) {
> + return OFPERR_OFPBRC_EPERM;
> + } else if (rule->pending) {
> + return OFPROTO_POSTPONE;
> + } else {
> + struct list rules;
> +
> + list_init(&rules);
> + list_push_back(&rules, &rule->ofproto_node);
> + fm->modify_cookie = true;
> + return modify_flows__(ofproto, ofconn, fm, request, &rules);
> + }
> + }
> +
> /* Verify actions. */
> error = ofproto_check_ofpacts(ofproto, fm->ofpacts, fm->ofpacts_len,
> &fm->match.flow, table_id);
> @@ -3457,62 +3484,53 @@ add_flow(struct ofproto *ofproto, struct ofconn
> *ofconn,
> ovs_rwlock_init(&rule->evict);
>
> /* Insert new rule. */
> - victim = oftable_replace_rule(rule);
> - if (victim && !rule_is_modifiable(victim)) {
> - error = OFPERR_OFPBRC_EPERM;
> - } else if (victim && victim->pending) {
> - error = OFPROTO_POSTPONE;
> - } else {
> - struct ofoperation *op;
> - struct rule *evict;
> - size_t n_rules;
> + oftable_insert_rule(rule);
>
> - ovs_rwlock_rdlock(&table->cls.rwlock);
> - n_rules = classifier_count(&table->cls);
> - ovs_rwlock_unlock(&table->cls.rwlock);
> - if (n_rules > table->max_flows) {
> - ovs_rwlock_rdlock(&rule->evict);
> - if (choose_rule_to_evict(table, &evict)) {
> - ovs_rwlock_unlock(&rule->evict);
> - ovs_rwlock_unlock(&evict->evict);
> - if (evict->pending) {
> - error = OFPROTO_POSTPONE;
> - goto exit;
> - }
> - } else {
> - ovs_rwlock_unlock(&rule->evict);
> - error = OFPERR_OFPFMFC_TABLE_FULL;
> + ovs_rwlock_rdlock(&table->cls.rwlock);
> + n_rules = classifier_count(&table->cls);
> + ovs_rwlock_unlock(&table->cls.rwlock);
> + if (n_rules > table->max_flows) {
> + ovs_rwlock_rdlock(&rule->evict);
> + if (choose_rule_to_evict(table, &evict)) {
> + ovs_rwlock_unlock(&rule->evict);
> + ovs_rwlock_unlock(&evict->evict);
> + if (evict->pending) {
> + error = OFPROTO_POSTPONE;
> goto exit;
> }
> } else {
> - evict = NULL;
> + ovs_rwlock_unlock(&rule->evict);
> + error = OFPERR_OFPFMFC_TABLE_FULL;
> + goto exit;
> }
> + } else {
> + evict = NULL;
> + }
>
> - group = ofopgroup_create(ofproto, ofconn, request, fm->buffer_id);
> - op = ofoperation_create(group, rule, OFOPERATION_ADD, 0);
> - op->victim = victim;
> + group = ofopgroup_create(ofproto, ofconn, request, fm->buffer_id);
> + op = ofoperation_create(group, rule, OFOPERATION_ADD, 0);
>
> - error = ofproto->ofproto_class->rule_construct(rule);
> - if (error) {
> - op->group->n_running--;
> - ofoperation_destroy(rule->pending);
> - } else if (evict) {
> - /* It would be better if we maintained the lock we took in
> - * choose_rule_to_evict() earlier, but that confuses the thread
> - * safety analysis, and this code is fragile enough that we
> really
> - * need it. In the worst case, we'll have to block a little
> while
> - * before we perform the eviction, which doesn't seem like a big
> - * problem. */
> - ovs_rwlock_wrlock(&evict->evict);
> - delete_flow__(evict, group, OFPRR_EVICTION);
> - }
> - ofopgroup_submit(group);
> + error = ofproto->ofproto_class->rule_construct(rule);
> + if (error) {
> + op->group->n_running--;
> + ofoperation_destroy(rule->pending);
> + } else if (evict) {
> + /* It would be better if we maintained the lock we took in
> + * choose_rule_to_evict() earlier, but that confuses the thread
> + * safety analysis, and this code is fragile enough that we really
> + * need it. In the worst case, we'll have to block a little while
> + * before we perform the eviction, which doesn't seem like a big
> + * problem. */
> + ovs_rwlock_wrlock(&evict->evict);
> + delete_flow__(evict, group, OFPRR_EVICTION);
> }
> + ofopgroup_submit(group);
>
> exit:
> /* Back out if an error occurred. */
> if (error) {
> - oftable_substitute_rule(rule, victim);
> + ovs_rwlock_wrlock(&rule->evict);
> + oftable_remove_rule(rule);
> ofproto_rule_destroy__(rule);
> }
> return error;
> @@ -3532,15 +3550,18 @@ modify_flows__(struct ofproto *ofproto, struct ofconn
> *ofconn,
> struct ofputil_flow_mod *fm, const struct ofp_header *request,
> struct list *rules)
> {
> + enum ofoperation_type type;
> struct ofopgroup *group;
> struct rule *rule;
> enum ofperr error;
>
> + type = fm->command == OFPFC_ADD ? OFOPERATION_REPLACE :
> OFOPERATION_MODIFY;
> group = ofopgroup_create(ofproto, ofconn, request, fm->buffer_id);
> error = OFPERR_OFPBRC_EPERM;
> LIST_FOR_EACH (rule, ofproto_node, rules) {
> struct ofoperation *op;
> bool actions_changed;
> + bool reset_counters;
>
> /* FIXME: Implement OFPFUTIL_FF_RESET_COUNTS */
>
> @@ -3561,19 +3582,39 @@ modify_flows__(struct ofproto *ofproto, struct ofconn
> *ofconn,
> actions_changed = !ofpacts_equal(fm->ofpacts, fm->ofpacts_len,
> rule->ofpacts, rule->ofpacts_len);
>
> - op = ofoperation_create(group, rule, OFOPERATION_MODIFY, 0);
> + op = ofoperation_create(group, rule, type, 0);
>
> if (fm->modify_cookie && fm->new_cookie != htonll(UINT64_MAX)) {
> ofproto_rule_change_cookie(ofproto, rule, fm->new_cookie);
> }
> - if (actions_changed) {
> + if (type == OFOPERATION_REPLACE) {
> + ovs_mutex_lock(&rule->timeout_mutex);
> + rule->idle_timeout = fm->idle_timeout;
> + rule->hard_timeout = fm->hard_timeout;
> + ovs_mutex_unlock(&rule->timeout_mutex);
> +
> + rule->send_flow_removed = (fm->flags
> + & OFPUTIL_FF_SEND_FLOW_REM) != 0;
> +
> + if (fm->idle_timeout || fm->hard_timeout) {
> + if (!rule->eviction_group) {
> + eviction_group_add_rule(rule);
> + }
> + } else {
> + eviction_group_remove_rule(rule);
> + }
> + }
> +
> + 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;
> rule->ofpacts = xmemdup(fm->ofpacts, fm->ofpacts_len);
> rule->ofpacts_len = fm->ofpacts_len;
> rule->meter_id = find_meter(rule->ofpacts, rule->ofpacts_len);
> - rule->ofproto->ofproto_class->rule_modify_actions(rule);
> + rule->ofproto->ofproto_class->rule_modify_actions(rule,
> +
> reset_counters);
> } else {
> ofoperation_complete(op, 0);
> }
> @@ -4075,7 +4116,7 @@ ofproto_compose_flow_refresh_update(const struct rule
> *rule,
> struct ofputil_flow_update fu;
> struct match match;
>
> - if (op && op->type == OFOPERATION_ADD && !op->victim) {
> + if (op && op->type == OFOPERATION_ADD) {
> /* We'll report the final flow when the operation completes.
> Reporting
> * it now would cause a duplicate report later. */
> return;
> @@ -4104,12 +4145,10 @@ ofproto_compose_flow_refresh_update(const struct rule
> *rule,
> * actions, so that when the operation commits we report the change.
> */
> switch (op->type) {
> case OFOPERATION_ADD:
> - /* We already verified that there was a victim. */
> - fu.ofpacts = op->victim->ofpacts;
> - fu.ofpacts_len = op->victim->ofpacts_len;
> - break;
> + NOT_REACHED();
>
> case OFOPERATION_MODIFY:
> + case OFOPERATION_REPLACE:
> if (op->ofpacts) {
> fu.ofpacts = op->ofpacts;
> fu.ofpacts_len = op->ofpacts_len;
> @@ -4920,15 +4959,27 @@ ofopgroup_complete(struct ofopgroup *group)
> && rule->flow_cookie == op->flow_cookie))) {
> /* Check that we can just cast from ofoperation_type to
> * nx_flow_update_event. */
> - BUILD_ASSERT_DECL((enum nx_flow_update_event) OFOPERATION_ADD
> - == NXFME_ADDED);
> - BUILD_ASSERT_DECL((enum nx_flow_update_event) OFOPERATION_DELETE
> - == NXFME_DELETED);
> - BUILD_ASSERT_DECL((enum nx_flow_update_event) OFOPERATION_MODIFY
> - == NXFME_MODIFIED);
> -
> - ofmonitor_report(ofproto->connmgr, rule,
> - (enum nx_flow_update_event) op->type,
> + enum nx_flow_update_event event_type;
> +
> + switch (op->type) {
> + case OFOPERATION_ADD:
> + case OFOPERATION_REPLACE:
> + event_type = NXFME_ADDED;
> + break;
> +
> + case OFOPERATION_DELETE:
> + event_type = NXFME_DELETED;
> + break;
> +
> + case OFOPERATION_MODIFY:
> + event_type = NXFME_MODIFIED;
> + break;
> +
> + default:
> + NOT_REACHED();
> + }
> +
> + ofmonitor_report(ofproto->connmgr, rule, event_type,
> op->reason, abbrev_ofconn, abbrev_xid);
> }
>
> @@ -4939,7 +4990,6 @@ ofopgroup_complete(struct ofopgroup *group)
> if (!op->error) {
> uint16_t vid_mask;
>
> - ofproto_rule_destroy__(op->victim);
> vid_mask = minimask_get_vid_mask(&rule->cr.match.mask);
> if (vid_mask == VLAN_VID_MASK) {
> if (ofproto->vlan_bitmap) {
> @@ -4953,7 +5003,8 @@ ofopgroup_complete(struct ofopgroup *group)
> }
> }
> } else {
> - oftable_substitute_rule(rule, op->victim);
> + ovs_rwlock_wrlock(&rule->evict);
> + oftable_remove_rule(rule);
> ofproto_rule_destroy__(rule);
> }
> break;
> @@ -4965,10 +5016,20 @@ ofopgroup_complete(struct ofopgroup *group)
> break;
>
> case OFOPERATION_MODIFY:
> + case OFOPERATION_REPLACE:
> if (!op->error) {
> - rule->modified = time_msec();
> + long long int now = time_msec();
> +
> + rule->modified = now;
> + if (op->type == OFOPERATION_REPLACE) {
> + rule->created = rule->used = now;
> + }
> } else {
> ofproto_rule_change_cookie(ofproto, rule, op->flow_cookie);
> + ovs_mutex_lock(&rule->timeout_mutex);
> + rule->idle_timeout = op->idle_timeout;
> + rule->hard_timeout = op->hard_timeout;
> + ovs_mutex_unlock(&rule->timeout_mutex);
> if (op->ofpacts) {
> free(rule->ofpacts);
> rule->ofpacts = op->ofpacts;
> @@ -4976,6 +5037,7 @@ ofopgroup_complete(struct ofopgroup *group)
> op->ofpacts = NULL;
> op->ofpacts_len = 0;
> }
> + rule->send_flow_removed = op->send_flow_removed;
> }
> break;
>
> @@ -5029,6 +5091,11 @@ ofoperation_create(struct ofopgroup *group, struct
> rule *rule,
> op->type = type;
> op->reason = reason;
> op->flow_cookie = rule->flow_cookie;
> + ovs_mutex_lock(&rule->timeout_mutex);
> + op->idle_timeout = rule->idle_timeout;
> + op->hard_timeout = rule->hard_timeout;
> + ovs_mutex_unlock(&rule->timeout_mutex);
> + op->send_flow_removed = rule->send_flow_removed;
>
> group->n_running++;
>
> @@ -5060,14 +5127,7 @@ ofoperation_destroy(struct ofoperation *op)
> * indicate success or an OpenFlow error code on failure.
> *
> * If 'error' is 0, indicating success, the operation will be committed
> - * permanently to the flow table. There is one interesting subcase:
> - *
> - * - If 'op' is an "add flow" operation that is replacing an existing rule
> in
> - * the flow table (the "victim" rule) by a new one, then the caller must
> - * have uninitialized any derived state in the victim rule, as in step 5
> in
> - * the "Life Cycle" in ofproto/ofproto-provider.h.
> ofoperation_complete()
> - * performs steps 6 and 7 for the victim rule, most notably by calling
> its
> - * ->rule_dealloc() function.
> + * permanently to the flow table.
> *
> * If 'error' is nonzero, then generally the operation will be rolled back:
> *
> @@ -5099,13 +5159,6 @@ ofoperation_complete(struct ofoperation *op, enum
> ofperr error)
> ofopgroup_complete(group);
> }
> }
> -
> -struct rule *
> -ofoperation_get_victim(struct ofoperation *op)
> -{
> - ovs_assert(op->type == OFOPERATION_ADD);
> - return op->victim;
> -}
>
> static uint64_t
> pick_datapath_id(const struct ofproto *ofproto)
> @@ -5547,15 +5600,13 @@ oftable_remove_rule(struct rule *rule)
> ovs_rwlock_unlock(&table->cls.rwlock);
> }
>
> -/* Inserts 'rule' into its oftable. Removes any existing rule from 'rule''s
> - * oftable that has an identical cls_rule. Returns the rule that was
> removed,
> - * if any, and otherwise NULL. */
> -static struct rule *
> -oftable_replace_rule(struct rule *rule)
> +/* Inserts 'rule' into its oftable, which must not already contain any rule
> for
> + * the same cls_rule. */
> +static void
> +oftable_insert_rule(struct rule *rule)
> {
> struct ofproto *ofproto = rule->ofproto;
> struct oftable *table = &ofproto->tables[rule->table_id];
> - struct rule *victim;
> bool may_expire;
>
> ovs_mutex_lock(&rule->timeout_mutex);
> @@ -5573,35 +5624,9 @@ oftable_replace_rule(struct rule *rule)
> list_insert(&meter->rules, &rule->meter_list_node);
> }
> ovs_rwlock_wrlock(&table->cls.rwlock);
> - victim = rule_from_cls_rule(classifier_replace(&table->cls, &rule->cr));
> + classifier_insert(&table->cls, &rule->cr);
> ovs_rwlock_unlock(&table->cls.rwlock);
> - if (victim) {
> - if (victim->meter_id) {
> - list_remove(&victim->meter_list_node);
> - }
> - cookies_remove(ofproto, victim);
> -
> - ovs_mutex_lock(&ofproto->expirable_mutex);
> - if (!list_is_empty(&victim->expirable)) {
> - list_remove(&victim->expirable);
> - }
> - ovs_mutex_unlock(&ofproto->expirable_mutex);
> - eviction_group_remove_rule(victim);
> - }
> eviction_group_add_rule(rule);
> - return victim;
> -}
> -
> -/* Removes 'old' from its oftable then, if 'new' is nonnull, inserts 'new'.
> */
> -static void
> -oftable_substitute_rule(struct rule *old, struct rule *new)
> -{
> - if (new) {
> - oftable_replace_rule(new);
> - } else {
> - ovs_rwlock_wrlock(&old->evict);
> - oftable_remove_rule(old);
> - }
> }
>
> /* unixctl commands. */
> --
> 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
