On Tue, Jun 09, 2015 at 05:24:14PM -0700, Jarno Rajahalme wrote:
> Each rule is now added or deleted in a specific tables version. Flow
> tables are versioned with a monotonically increasing 64-bit integer,
> where positive values are valid version numbers.
>
> Rule modifications are implemented as an insertion of a new rule and a
> deletion of the old rule, both taking place in the same tables
> version. Since concurrent lookups may use different versions, both
> the old and new rule must be available for lookups at the same time.
>
> The ofproto provider interface is changed to accomodate the above. As
> rule's actions need not be modified any more, we no longer need
> 'rule_premodify_actions', nor 'rule_modify_actions'. 'rule_insert'
> now takes a pointer to the old rule and adds a flag that tells whether
> the old stats should be forwarded to the new rule or not (this
> replaces the 'reset_counters' flag of the now removed
> 'rule_modify_actions').
>
> Versioning all flow table changes has the side effect of making
> learned flows visible for future lookups only. I.e., the upcall that
> executes the learn action, will not see the newly learned action in
> it's classifier lookups. Only upcalls that start executing after the
> new flow was added will match on it.
>
> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
I think that this change to the "learn" action is only documented in the
commit message. How about this:
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index e18229d..521e599 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -3660,6 +3660,22 @@ format_RESUBMIT(const struct ofpact_resubmit *a, struct
ds *s)
* address. This is not usually the intent in MAC learning; instead, we want
* the MAC learn entry to expire when no traffic has been sent *from* the
* learned address. Use a hard timeout for that.
+ *
+ *
+ * Visibility of Changes
+ * ---------------------
+ *
+ * Prior to Open vSwitch 2.4, any changes made by a "learn" action in a given
+ * flow translation are visible to flow table lookups made later in the flow
+ * translation. This means that, in the example above, a MAC learned by the
+ * learn action in table 0 would be found in table 1 (if the packet being
+ * processed had the same source and destination MAC address).
+ *
+ * In Open vSwitch 2.4 and later, changes to a flow table (whether to add or
+ * modify a flow) by a "learn" action are not visible for later lookups within
+ * the same flow translation. In the MAC learning example, a MAC learned by
+ * the learn action in table 0 would not be found in table 1, meaning that if
+ * this MAC had not been learned before then the packet would be flooded[*].
*/
struct nx_action_learn {
ovs_be16 type; /* OFPAT_VENDOR. */
Here's a suggested clarification for NEWS:
diff --git a/NEWS b/NEWS
index f171cfc..a3eeed5 100644
--- a/NEWS
+++ b/NEWS
@@ -2,9 +2,9 @@ Post-v2.3.0
---------------------
- Flow table modifications are now atomic, meaning that each packet
now sees a coherent version of the OpenFlow pipeline. For
- example, if a controller removes all flows, no packet sees an
- intermediate version of the OpenFlow pipeline where only some of
- the flows have been deleted.
+ example, if a controller removes all flows with a single OpenFlow
+ "flow_mod", no packet sees an intermediate version of the OpenFlow
+ pipeline where only some of the flows have been deleted.
- Added support for SFQ, FQ_CoDel and CoDel qdiscs.
- Add bash command-line completion support for ovs-vsctl Please check
utilities/ovs-command-compgen.INSTALL.md for how to use.
Suggested new comment for 'new_rule':
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 7a903fa..55fea0f 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -91,7 +91,11 @@ struct rule_dpif {
/* In non-NULL, will point to a new rule (for which a reference is held) to
* which all the stats updates should be forwarded. This exists only
- * transitionally when flows are replaced. */
+ * transitionally when flows are replaced.
+ *
+ * Protected by stats_mutex. If both 'rule->stats_mutex' and
+ * 'rule->new_rule->stats_mutex' must be held together, acquire them in that
+ * order, */
struct rule_dpif *new_rule OVS_GUARDED;
/* If non-zero then the recirculation id that has
I wonder whether rule_actions should go away now. It appears that it
never changes anymore once a rule is created.
How do you feel about the double defer on rule destruction?
In learned_cookies_flush(), I think this change is just whitespace:
- rule_criteria_init(&criteria, c->table_id, &match, 0, CLS_MAX_VERSION,
+ rule_criteria_init(&criteria, c->table_id, &match, 0,
+ CLS_MAX_VERSION,
c->cookie, OVS_BE64_MAX, OFPP_ANY, OFPG_ANY);
Similar whitespace changes in handle_flow_stats_request(),
handle_aggregate_stats_request(), modify_flow_start_strict()?
In rule_collection_detach(), I think that this:
if (rules->rules == rules->stub) {
size_t size = rules->n * sizeof *rules->rules;
rules->rules = xmalloc(size);
memcpy(rules->rules, rules->stub, size);
}
could be simplified to:
if (rules->rules == rules->stub) {
rules->rules = xmemdup(rules->rules, rules->n * sizeof *rules->rules);
}
This goofy incomplete comment of mine has persisted too long, please
delete it (now it's in replace_rule_finish()):
/* 'fm' says that */
Acked-by: Ben Pfaff <b...@nicira.com>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
