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
* 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.
* (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.
rule_dealloc,
rule_get_stats,
rule_execute,
+ NULL, /* rule_premodify_actions */
rule_modify_actions,
set_frag_handling,
packet_out,
* 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
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
* 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. */
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;
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);
}
rule->pending = NULL;
+ ovs_assert(!op->error || op->type == OFOPERATION_ADD);
switch (op->type) {
case OFOPERATION_ADD:
if (!op->error) {
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();
* 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. */
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)) {