[ovs-dev] [PATCH 08/16] ofproto: Only initiate flow table modifications if they will succeed.
Ben Pfaff
blp at nicira.com
Fri Jun 6 05:02:02 UTC 2014
In OpenFlow, a single "flow_mod" operation can change the actions (and
some other properties) of an arbitrary number of flows. Until now,
Open vSwitch has assumed that any subset of these operations could
fail. However, it has come out in discussion in the OpenFlow extensibility
working group that "partial failure" of a flow table operation is
undesirable, and furthermore that it should be possible to avoid it on
hardware implementations. (The latter is the reason that Open vSwitch
permitted it to be with.)
This commit changes Open vSwitch to check whether all of a set of flow
table modifications will succeed before it initiates any of them.
This will not change visible behavior of the Open vSwitch software
switch, which never failed flow table modifications anyway. It might
change behavior of some hardware implementation, but I don't actually know
of any.
ONF-JIRA: EXT-362.
Signed-off-by: Ben Pfaff <blp at nicira.com>
---
CONTRIBUTING | 6 ++--
include/openflow/nicira-ext.h | 8 +++--
ofproto/ofproto-dpif.c | 1 +
ofproto/ofproto-provider.h | 37 +++++++++++----------
ofproto/ofproto.c | 75 ++++++++++++++++++-------------------------
5 files changed, 60 insertions(+), 67 deletions(-)
diff --git a/CONTRIBUTING b/CONTRIBUTING
index bf13558..f4d2c97 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 fe954cd..767b98b 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 45c1393..2ca883f 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4897,6 +4897,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 7741044..c8cd69d 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 80c7126..27f5a46 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -4139,6 +4139,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;
@@ -4194,8 +4208,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);
}
@@ -6268,6 +6281,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) {
@@ -6292,42 +6306,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();
@@ -6419,19 +6413,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. */
@@ -6441,7 +6428,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)) {
--
1.9.1
More information about the dev
mailing list