> On May 29, 2015, at 6:40 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> On Mon, May 18, 2015 at 04:10:24PM -0700, Jarno Rajahalme wrote:
>> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
>
> You know, we could make evictions reversible, since after all we have a
> way to mark rules as to-be-deleted and then not delete them. I don't
> know whether it's worthwhile.
I’ll look into this.
> The handling of "conflicts" seems, well, conflicted. I see one comment
> that says we check for conflicts at bundle add time:
>
> /* Bundle conflicts. A new message being added into the bundle is deemed to
> be
> * in conflict, if it would delete or modify a flow or a port that was added or
> * modified by an earlier message in the bundle.
> *
> * We check for this at the bundle add time, and rely on the messages not being
> * in conflict at the bundle commit time. This simplifies the state management
> * requirements for rolling back failing commits.
>
> and another that says we check for them at bundle commit time:
>
> /* Check for conflicts. */
>
> /* The spec says to return OFPBFC_MSG_CONFLICT when processing the
> * BUNDLE_ADD, but with big bundles that may be very expensive. It may be
> * a lot cheaper to check that mods/deletes do not target any of the rules
> * added/modded by the current bundle at the commit time. */
>
> but I couldn't find any actual code that returns OFPBFC_MSG_CONFLICT
> anywhere.
I’m sorry for the confusion. Both of these comments were remnants of an earlier
(incomplete) version. I have removed them now, thanks for spotting them!
>
> Worse, I couldn't find any good definition of a "conflict" in the
> OpenFlow specification. It just says:
>
> If the message in the request is incompatible with another message
> already stored in the bun dle, the switch must refuse to add the
> message to the bundle and send an ofp_error_msg with
> OFPET_BUNDLE_FAILED type and OFPBFC_MSG_CONFLICT code.
>
> I think we might have to ask for clarification.
>
For now I don’t see a case where we’d need to use this.
> I wouldn't use ovs_assert() like this:
> ovs_assert(classifier_remove(&table->cls, &new_rule->cr));
> because defining NDEBUG will then delete this code. Instead:
> if (!classifier_remove(&table->cls, &new_rule->cr)) {
> OVS_NOT_REACHED();
> }
>
OK.
> I guess that the following special case in handle_bundle_control() and
> handle_flow_mod__() could be moved into ofputil_decode_flow_mod(), so
> that we could assume at execution time that the command is a valid one:
> + default:
> + if (fm->command > 0xff) {
> + VLOG_WARN_RL(&rl, "%s: flow_mod has explicit table_id but "
> + "flow_mod_table_id extension is not enabled",
> + ofproto->name);
> + }
> + error = OFPERR_OFPFMFC_BAD_COMMAND;
> + break;
>
Done.
> I think that some code duplication could be eliminated by writing
> handle_flow_mod__() in terms of do_bundle_flow_mod_begin() and
> do_bundle_flow_mod_finish().
>
Thanks for the hint, this ended up removing more code than I initially thought!
> Acked-by: Ben Pfaff <b...@nicira.com>
Thanks for the review!
I also noticed that in the bundle case I did not call ofconn_report_flow_mod(),
I folded that in as well.
Applied to master with the following incremental based on the above,
Jarno
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 17a0c41..0f9a38d 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -1795,11 +1795,19 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
fm->command = command & 0xff;
fm->table_id = command >> 8;
} else {
+ if (command > 0xff) {
+ VLOG_WARN_RL(&bad_ofmsg_rl, "flow_mod has explicit table_id "
+ "but flow_mod_table_id extension is not enabled");
+ }
fm->command = command;
fm->table_id = 0xff;
}
}
+ if (fm->command > OFPFC_DELETE_STRICT) {
+ return OFPERR_OFPFMFC_BAD_COMMAND;
+ }
+
error = ofpacts_pull_openflow_instructions(&b, b.size,
oh->version, ofpacts);
if (error) {
diff --git a/ofproto/bundles.c b/ofproto/bundles.c
index 9f125a5..686d61f 100644
--- a/ofproto/bundles.c
+++ b/ofproto/bundles.c
@@ -59,11 +59,16 @@ ofp_bundle_create(uint32_t id, uint16_t flags)
}
void
-ofp_bundle_remove__(struct ofconn *ofconn, struct ofp_bundle *bundle)
+ofp_bundle_remove__(struct ofconn *ofconn, struct ofp_bundle *bundle,
+ bool success)
{
struct ofp_bundle_entry *msg;
LIST_FOR_EACH_POP (msg, node, &bundle->msg_list) {
+ if (success && msg->type == OFPTYPE_FLOW_MOD) {
+ /* Tell connmgr about successful flow mods. */
+ ofconn_report_flow_mod(ofconn, msg->fm.command);
+ }
ofp_bundle_entry_free(msg);
}
@@ -81,7 +86,7 @@ ofp_bundle_open(struct ofconn *ofconn, uint32_t id, uint16_t
flags)
if (bundle) {
VLOG_INFO("Bundle %x already exists.", id);
- ofp_bundle_remove__(ofconn, bundle);
+ ofp_bundle_remove__(ofconn, bundle, false);
return OFPERR_OFPBFC_BAD_ID;
}
@@ -107,12 +112,12 @@ ofp_bundle_close(struct ofconn *ofconn, uint32_t id,
uint16_t flags)
}
if (bundle->state == BS_CLOSED) {
- ofp_bundle_remove__(ofconn, bundle);
+ ofp_bundle_remove__(ofconn, bundle, false);
return OFPERR_OFPBFC_BUNDLE_CLOSED;
}
if (bundle->flags != flags) {
- ofp_bundle_remove__(ofconn, bundle);
+ ofp_bundle_remove__(ofconn, bundle, false);
return OFPERR_OFPBFC_BAD_FLAGS;
}
@@ -131,19 +136,11 @@ ofp_bundle_discard(struct ofconn *ofconn, uint32_t id)
return OFPERR_OFPBFC_BAD_ID;
}
- ofp_bundle_remove__(ofconn, bundle);
+ ofp_bundle_remove__(ofconn, bundle, false);
return 0;
}
-/* Bundle conflicts. A new message being added into the bundle is deemed to be
- * in conflict, if it would delete or modify a flow or a port that was added or
- * modified by an earlier message in the bundle.
- *
- * We check for this at the bundle add time, and rely on the messages not being
- * in conflict at the bundle commit time. This simplifies the state management
- * requirements for rolling back failing commits.
- */
enum ofperr
ofp_bundle_add_message(struct ofconn *ofconn, uint32_t id, uint16_t flags,
struct ofp_bundle_entry *bmsg)
@@ -162,20 +159,13 @@ ofp_bundle_add_message(struct ofconn *ofconn, uint32_t
id, uint16_t flags,
return error;
}
} else if (bundle->state == BS_CLOSED) {
- ofp_bundle_remove__(ofconn, bundle);
+ ofp_bundle_remove__(ofconn, bundle, false);
return OFPERR_OFPBFC_BUNDLE_CLOSED;
} else if (flags != bundle->flags) {
- ofp_bundle_remove__(ofconn, bundle);
+ ofp_bundle_remove__(ofconn, bundle, false);
return OFPERR_OFPBFC_BAD_FLAGS;
}
- /* Check for conflicts. */
-
- /* The spec says to return OFPBFC_MSG_CONFLICT when processing the
- * BUNDLE_ADD, but with big bundles that may be very expensive. It may be
- * a lot cheaper to check that mods/deletes do not target any of the rules
- * added/modded by the current bundle at the commit time. */
-
list_push_back(&bundle->msg_list, &bmsg->node);
return 0;
}
diff --git a/ofproto/bundles.h b/ofproto/bundles.h
index adc0f89..ab3a137 100644
--- a/ofproto/bundles.h
+++ b/ofproto/bundles.h
@@ -75,7 +75,7 @@ enum ofperr ofp_bundle_discard(struct ofconn *, uint32_t id);
enum ofperr ofp_bundle_add_message(struct ofconn *, uint32_t id,
uint16_t flags, struct ofp_bundle_entry *);
-void ofp_bundle_remove__(struct ofconn *ofconn, struct ofp_bundle *bundle);
+void ofp_bundle_remove__(struct ofconn *, struct ofp_bundle *, bool success);
static inline struct ofp_bundle_entry *
ofp_bundle_entry_alloc(enum ofptype type, const struct ofp_header *oh)
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index 9851997..495364f 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -1215,7 +1215,7 @@ bundle_remove_all(struct ofconn *ofconn)
struct ofp_bundle *b, *next;
HMAP_FOR_EACH_SAFE (b, next, node, &ofconn->bundles) {
- ofp_bundle_remove__(ofconn, b);
+ ofp_bundle_remove__(ofconn, b, false);
}
}
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index ad8ecca..cb91197 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -253,9 +253,6 @@ struct flow_mod_requester {
};
/* OpenFlow. */
-static enum ofperr add_flow(struct ofproto *, struct ofputil_flow_mod *,
- const struct flow_mod_requester *);
-
static enum ofperr modify_flow_check__(struct ofproto *,
struct ofputil_flow_mod *,
const struct rule *)
@@ -281,6 +278,15 @@ static bool ofproto_group_exists(const struct ofproto
*ofproto,
OVS_EXCLUDED(ofproto->groups_rwlock);
static enum ofperr add_group(struct ofproto *, struct ofputil_group_mod *);
static void handle_openflow(struct ofconn *, const struct ofpbuf *);
+static enum ofperr do_bundle_flow_mod_begin(struct ofproto *,
+ struct ofputil_flow_mod *,
+ struct ofp_bundle_entry *)
+ OVS_REQUIRES(ofproto_mutex);
+static void do_bundle_flow_mod_finish(struct ofproto *,
+ struct ofputil_flow_mod *,
+ const struct flow_mod_requester *,
+ struct ofp_bundle_entry *)
+ OVS_REQUIRES(ofproto_mutex);
static enum ofperr handle_flow_mod__(struct ofproto *,
struct ofputil_flow_mod *,
const struct flow_mod_requester *)
@@ -4338,6 +4344,17 @@ set_conjunctions(struct rule *rule, const struct
cls_conjunction *conjs,
cls_rule_set_conjunctions(cr, conjs, n_conjs);
}
+/* Implements OFPFC_ADD and the cases for OFPFC_MODIFY and OFPFC_MODIFY_STRICT
+ * in which no matching flow already exists in the flow table.
+ *
+ * Adds the flow specified by 'fm', to the ofproto's flow table. Returns 0 on
+ * success, or an OpenFlow error code on failure.
+ *
+ * On successful return the caller must complete the operation either by
+ * calling add_flow_finish(), or add_flow_revert() if the operation needs to
+ * be reverted.
+ *
+ * The caller retains ownership of 'fm->ofpacts'. */
static enum ofperr
add_flow_begin(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
struct rule **rulep, bool *modify)
@@ -4463,7 +4480,9 @@ add_flow_revert(struct ofproto *ofproto, struct rule
*new_rule)
if (new_rule) {
struct oftable *table = &ofproto->tables[new_rule->table_id];
- ovs_assert(classifier_remove(&table->cls, &new_rule->cr));
+ if (!classifier_remove(&table->cls, &new_rule->cr)) {
+ OVS_NOT_REACHED();
+ }
classifier_publish(&table->cls);
ofproto_rule_remove__(ofproto, new_rule);
@@ -4511,30 +4530,6 @@ add_flow_finish(struct ofproto *ofproto, struct
ofputil_flow_mod *fm,
send_buffered_packet(req, fm->buffer_id, rule);
}
-
-/* Implements OFPFC_ADD and the cases for OFPFC_MODIFY and OFPFC_MODIFY_STRICT
- * in which no matching flow already exists in the flow table.
- *
- * Adds the flow specified by 'fm', to the ofproto's flow table. Returns 0 on
- * success, or an OpenFlow error code on failure.
- *
- * The caller retains ownership of 'fm->ofpacts'. */
-static enum ofperr
-add_flow(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
- const struct flow_mod_requester *req)
- OVS_REQUIRES(ofproto_mutex)
-{
- struct rule *rule;
- bool modify;
- enum ofperr error;
-
- error = add_flow_begin(ofproto, fm, &rule, &modify);
- if (!error) {
- add_flow_finish(ofproto, fm, req, rule, modify);
- }
-
- return error;
-}
/* OFPFC_MODIFY and OFPFC_MODIFY_STRICT. */
@@ -4789,22 +4784,6 @@ modify_flows_finish(struct ofproto *ofproto, struct
ofputil_flow_mod *fm,
rule_collection_destroy(rules);
}
-static enum ofperr
-modify_flows_loose(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
- const struct flow_mod_requester *req)
- OVS_REQUIRES(ofproto_mutex)
-{
- struct rule_collection rules;
- enum ofperr error;
-
- error = modify_flows_begin_loose(ofproto, fm, &rules);
- if (!error) {
- modify_flows_finish(ofproto, fm, req, &rules);
- }
-
- return error;
-}
-
/* Implements OFPFC_MODIFY_STRICT. Returns 0 on success or an OpenFlow error
* code on failure. */
static enum ofperr
@@ -4832,23 +4811,6 @@ modify_flow_begin_strict(struct ofproto *ofproto, struct
ofputil_flow_mod *fm,
}
return error;
}
-
-static enum ofperr
-modify_flow_strict(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
- const struct flow_mod_requester *req)
- OVS_REQUIRES(ofproto_mutex)
-{
- struct rule_collection rules;
- enum ofperr error;
-
- error = modify_flow_begin_strict(ofproto, fm, &rules);
- if (!error) {
- modify_flows_finish(ofproto, fm, req, &rules);
- }
-
- return error;
-}
-
/* OFPFC_DELETE implementation. */
@@ -4949,23 +4911,6 @@ delete_flows_finish(const struct ofputil_flow_mod *fm,
rule_collection_destroy(rules);
}
-static enum ofperr
-delete_flows_loose(struct ofproto *ofproto,
- const struct ofputil_flow_mod *fm,
- const struct flow_mod_requester *req)
- OVS_REQUIRES(ofproto_mutex)
-{
- struct rule_collection rules;
- enum ofperr error;
-
- error = delete_flows_begin_loose(ofproto, fm, &rules);
- if (!error) {
- delete_flows_finish(fm, req, &rules);
- }
-
- return error;
-}
-
/* Implements OFPFC_DELETE_STRICT. */
static enum ofperr
delete_flow_begin_strict(struct ofproto *ofproto,
@@ -4995,22 +4940,6 @@ delete_flow_begin_strict(struct ofproto *ofproto,
return error;
}
-static enum ofperr
-delete_flow_strict(struct ofproto *ofproto, const struct ofputil_flow_mod *fm,
- const struct flow_mod_requester *req)
- OVS_REQUIRES(ofproto_mutex)
-{
- struct rule_collection rules;
- enum ofperr error;
-
- error = delete_flow_begin_strict(ofproto, fm, &rules);
- if (!error) {
- delete_flows_finish(fm, req, &rules);
- }
-
- return error;
-}
-
static void
ofproto_rule_send_removed(struct rule *rule, uint8_t reason)
OVS_REQUIRES(ofproto_mutex)
@@ -5141,38 +5070,13 @@ handle_flow_mod__(struct ofproto *ofproto, struct
ofputil_flow_mod *fm,
const struct flow_mod_requester *req)
OVS_EXCLUDED(ofproto_mutex)
{
+ struct ofp_bundle_entry be;
enum ofperr error;
ovs_mutex_lock(&ofproto_mutex);
- switch (fm->command) {
- case OFPFC_ADD:
- error = add_flow(ofproto, fm, req);
- break;
-
- case OFPFC_MODIFY:
- error = modify_flows_loose(ofproto, fm, req);
- break;
-
- case OFPFC_MODIFY_STRICT:
- error = modify_flow_strict(ofproto, fm, req);
- break;
-
- case OFPFC_DELETE:
- error = delete_flows_loose(ofproto, fm, req);
- break;
-
- case OFPFC_DELETE_STRICT:
- error = delete_flow_strict(ofproto, fm, req);
- break;
-
- default:
- if (fm->command > 0xff) {
- VLOG_WARN_RL(&rl, "%s: flow_mod has explicit table_id but "
- "flow_mod_table_id extension is not enabled",
- ofproto->name);
- }
- error = OFPERR_OFPFMFC_BAD_COMMAND;
- break;
+ error = do_bundle_flow_mod_begin(ofproto, fm, &be);
+ if (!error) {
+ do_bundle_flow_mod_finish(ofproto, fm, req, &be);
}
ofmonitor_flush(ofproto->connmgr);
ovs_mutex_unlock(&ofproto_mutex);
@@ -6540,40 +6444,24 @@ do_bundle_flow_mod_begin(struct ofproto *ofproto,
struct ofputil_flow_mod *fm,
struct ofp_bundle_entry *be)
OVS_REQUIRES(ofproto_mutex)
{
- enum ofperr error;
-
switch (fm->command) {
case OFPFC_ADD:
- error = add_flow_begin(ofproto, fm, &be->rule, &be->modify);
- break;
+ return add_flow_begin(ofproto, fm, &be->rule, &be->modify);
case OFPFC_MODIFY:
- error = modify_flows_begin_loose(ofproto, fm, &be->rules);
- break;
+ return modify_flows_begin_loose(ofproto, fm, &be->rules);
case OFPFC_MODIFY_STRICT:
- error = modify_flow_begin_strict(ofproto, fm, &be->rules);
- break;
+ return modify_flow_begin_strict(ofproto, fm, &be->rules);
case OFPFC_DELETE:
- error = delete_flows_begin_loose(ofproto, fm, &be->rules);
- break;
+ return delete_flows_begin_loose(ofproto, fm, &be->rules);
case OFPFC_DELETE_STRICT:
- error = delete_flow_begin_strict(ofproto, fm, &be->rules);
- break;
-
- default:
- if (fm->command > 0xff) {
- VLOG_WARN_RL(&rl, "%s: flow_mod has explicit table_id but "
- "flow_mod_table_id extension is not enabled",
- ofproto->name);
- }
- error = OFPERR_OFPFMFC_BAD_COMMAND;
- break;
+ return delete_flow_begin_strict(ofproto, fm, &be->rules);
}
- return error;
+ return OFPERR_OFPFMFC_BAD_COMMAND;
}
static void
@@ -6706,8 +6594,9 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id,
uint16_t flags)
run_rule_executes(ofproto);
}
+
/* The bundle is discarded regardless the outcome. */
- ofp_bundle_remove__(ofconn, bundle);
+ ofp_bundle_remove__(ofconn, bundle, !error);
return error;
}
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
