diff --git a/CONTRIBUTING b/CONTRIBUTING index bf1355834..f4d2c973c 100644 --- a/CONTRIBUTING +++ b/CONTRIBUTING @@ -193,12 +193,14 @@ Examples of common tags follow. Reported-at: http://openvswitch.org/pipermail/dev/2014-June/040952.html VMware-BZ: #1234567 + ONF-JIRA: EXT-12345 If a patch fixes or is otherwise related to a bug reported in a private bug tracker, you may include some tracking ID for the bug for your own reference. Please include some - identifier to make the origin clear, e.g. "VMware-BZ" in this - case refers to VMware's internal Bugzilla instance. + identifier to make the origin clear, e.g. "VMware-BZ" refers + to VMware's internal Bugzilla instance and "ONF-JIRA" refers + to the Open Networking Foundation's JIRA bug tracker. Bug #1234567. Issue: 1234567 diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h index fe954cdbf..767b98b0a 100644 --- a/include/openflow/nicira-ext.h +++ b/include/openflow/nicira-ext.h @@ -2236,8 +2236,9 @@ OFP_ASSERT(sizeof(struct nx_flow_update_full) == 24); * a flow_mod with type OFPFC_MODIFY affects multiple flows, but only some * of those modifications succeed (e.g. due to hardware limitations). * - * This cannot occur with the current implementation of the Open vSwitch - * software datapath. It could happen with other datapath implementations. + * This cannot occur with the Open vSwitch software datapath. This also + * cannot occur in Open vSwitch 2.4 and later, because these versions only + * execute any flow modifications if all of them will succeed. * * - Changes that race with conflicting changes made by other controllers or * other flow_mods (not separated by barriers) by the same controller. @@ -2246,6 +2247,9 @@ OFP_ASSERT(sizeof(struct nx_flow_update_full) == 24); * (regardless of datapath) because Open vSwitch internally serializes * potentially conflicting changes. * + * - Changes that occur when flow notification is paused (see "Buffer + * Management" above). + * * A flow_mod that does not change the flow table will not trigger any * notification, even an abbreviated one. For example, a "modify" or "delete" * flow_mod that does not match any flows will not trigger a notification. diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 9bb004c4d..69eee4b5c 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -4946,6 +4946,7 @@ const struct ofproto_class ofproto_dpif_class = { rule_dealloc, rule_get_stats, rule_execute, + NULL, /* rule_premodify_actions */ rule_modify_actions, set_frag_handling, packet_out, diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 774104475..c8cd69d9c 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -1160,23 +1160,16 @@ struct ofproto_class { * that the operation will probably succeed: * * - ofproto adds the rule in the flow table before calling - * ->rule_insert(). + * ->rule_insert(). If adding a rule in the flow table fails, ofproto + * removes the new rule in the call to ofoperation_complete(). * * - ofproto updates the rule's actions and other properties before - * calling ->rule_modify_actions(). + * calling ->rule_modify_actions(). Modifying a rule is not allowed to + * fail (but ->rule_premodify_actions() can prevent the modification + * attempt in advance). * - * - ofproto removes the rule before calling ->rule_delete(). - * - * With one exception, when an asynchronous operation completes with an - * error, ofoperation_complete() backs out the already applied changes: - * - * - If adding a rule in the flow table fails, ofproto removes the new - * rule. - * - * - 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. + * - ofproto removes the rule before calling ->rule_delete(). Removing a + * rule is not allowed to fail. It must always succeed. * * The ofproto base code serializes operations: if any operation is in * progress on a given rule, ofproto postpones initiating any new operation @@ -1292,15 +1285,22 @@ struct ofproto_class { enum ofperr (*rule_execute)(struct rule *rule, const struct flow *flow, struct ofpbuf *packet); + /* If the datapath can properly implement changing 'rule''s actions to the + * 'ofpacts_len' bytes in 'ofpacts', returns 0. Otherwise, returns an enum + * ofperr indicating why the new actions wouldn't work. + * + * May be a null pointer if any set of actions is acceptable. */ + enum ofperr (*rule_premodify_actions)(const struct rule *rule, + const struct ofpact *ofpacts, + size_t ofpacts_len) + /* OVS_REQUIRES(ofproto_mutex) */; + /* When ->rule_modify_actions() is called, the caller has already replaced * the OpenFlow actions in 'rule' by a new set. (The original actions are * in rule->pending->actions.) * * ->rule_modify_actions() should set the following in motion: * - * - Validate that the datapath can correctly implement the actions now - * in 'rule'. - * * - Update the datapath flow table with the new actions. * * - Only if 'reset_counters' is true, reset any packet or byte counters @@ -1312,8 +1312,7 @@ struct ofproto_class { * should call ofoperation_complete() later, after the operation does * complete. * - * If the operation fails, then the base ofproto code will restore the - * original 'actions' and 'n_actions' of 'rule'. + * Rule modification must not fail. * * ->rule_modify_actions() should not modify any base members of struct * rule. */ diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index d0129152d..40aa4e851 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -4145,6 +4145,20 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn, enum ofperr error; size_t i; + if (ofproto->ofproto_class->rule_premodify_actions) { + for (i = 0; i < rules->n; i++) { + struct rule *rule = rules->rules[i]; + + if (rule_is_modifiable(rule, fm->flags)) { + error = ofproto->ofproto_class->rule_premodify_actions( + rule, fm->ofpacts, fm->ofpacts_len); + if (error) { + return error; + } + } + } + } + type = fm->command == OFPFC_ADD ? OFOPERATION_REPLACE : OFOPERATION_MODIFY; group = ofopgroup_create(ofproto, ofconn, request, fm->buffer_id); error = OFPERR_OFPBRC_EPERM; @@ -4200,8 +4214,7 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn, ovsrcu_set(&rule->actions, new_actions); - rule->ofproto->ofproto_class->rule_modify_actions(rule, - reset_counters); + ofproto->ofproto_class->rule_modify_actions(rule, reset_counters); } else { ofoperation_complete(op, 0); } @@ -6277,6 +6290,7 @@ ofopgroup_complete(struct ofopgroup *group) rule->pending = NULL; + ovs_assert(!op->error || op->type == OFOPERATION_ADD); switch (op->type) { case OFOPERATION_ADD: if (!op->error) { @@ -6301,42 +6315,22 @@ ofopgroup_complete(struct ofopgroup *group) break; case OFOPERATION_DELETE: - ovs_assert(!op->error); ofproto_rule_unref(rule); op->rule = NULL; break; case OFOPERATION_MODIFY: - case OFOPERATION_REPLACE: - if (!op->error) { - long long int now = time_msec(); + case OFOPERATION_REPLACE: { + long long now = time_msec(); - ovs_mutex_lock(&rule->mutex); - rule->modified = now; - if (op->type == OFOPERATION_REPLACE) { - rule->created = now; - } - ovs_mutex_unlock(&rule->mutex); - } else { - ofproto_rule_change_cookie(ofproto, rule, op->flow_cookie); - ovs_mutex_lock(&rule->mutex); - rule->idle_timeout = op->idle_timeout; - rule->hard_timeout = op->hard_timeout; - ovs_mutex_unlock(&rule->mutex); - if (op->actions) { - const struct rule_actions *old_actions; - - ovs_mutex_lock(&rule->mutex); - old_actions = rule_get_actions(rule); - ovsrcu_set(&rule->actions, op->actions); - ovs_mutex_unlock(&rule->mutex); - - op->actions = NULL; - rule_actions_destroy(old_actions); - } - rule->flags = op->flags; + ovs_mutex_lock(&rule->mutex); + rule->modified = now; + if (op->type == OFOPERATION_REPLACE) { + rule->created = now; } + ovs_mutex_unlock(&rule->mutex); break; + } default: OVS_NOT_REACHED(); @@ -6428,19 +6422,12 @@ ofoperation_destroy(struct ofoperation *op) * If 'error' is 0, indicating success, the operation will be committed * permanently to the flow table. * - * If 'error' is nonzero, then generally the operation will be rolled back: - * - * - If 'op' is an "add flow" operation, ofproto removes the new rule or - * restores the original rule. The caller must have uninitialized any - * derived state in the new rule, as in step 5 of in the "Life Cycle" in - * ofproto/ofproto-provider.h. ofoperation_complete() performs steps 6 and - * and 7 for the new rule, calling its ->rule_dealloc() function. - * - * - If 'op' is a "modify flow" operation, ofproto restores the original - * actions. - * - * - 'op' must not be a "delete flow" operation. Removing a rule is not - * allowed to fail. It must always succeed. + * Flow modifications and deletions must always succeed. Flow additions may + * fail, indicated by nonzero 'error'. If an "add flow" operation fails, this + * function removes the new rule. The caller must have uninitialized any + * derived state in the new rule, as in step 5 of in the "Life Cycle" in + * ofproto/ofproto-provider.h. ofoperation_complete() performs steps 6 and and + * 7 for the new rule, calling its ->rule_dealloc() function. * * Please see the large comment in ofproto/ofproto-provider.h titled * "Asynchronous Operation Support" for more information. */ @@ -6450,7 +6437,7 @@ ofoperation_complete(struct ofoperation *op, enum ofperr error) struct ofopgroup *group = op->group; ovs_assert(group->n_running > 0); - ovs_assert(!error || op->type != OFOPERATION_DELETE); + ovs_assert(!error || op->type == OFOPERATION_ADD); op->error = error; if (!--group->n_running && !list_is_empty(&group->ofproto_node)) {