> On Jun 10, 2015, at 5:30 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> 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[*].
I did not understand the ‘[*]’ at the end. Also, I wanted to clarify this last
paragraph a bit more:
* In Open vSwitch 2.4 and later, changes to a flow table (whether to add or
* modify a flow) by a "learn" action are visible only for later flow
* translations, not 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 if the flow translation would resubmit to table 1
* after the processing of the learn action, 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.
>
Applied.
> 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;
>
Applied.
> /* 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.
>
We still need to allocate the space for the rules dynamically, so this is a
matter of taste. IMO keeping the attributes that relate to the actions together
in a struct is a useful abstraction. Maybe we should get rid of the getter
function, however.
> How do you feel about the double defer on rule destruction?
>
IMO it is fine for now. There is no ordering bug possibility here, and both
steps are relatively small, work-wise. Or do you think that postponing form the
ovs-rcu thread itself would be problematic in some cases?
Alternatively, we could have some form of an explicit synchronization between
the handlers and the ofproto where they would “check out a version” and then
explicitly “release it back”.
Maybe we revisit this when we have a solution for the deferred send removal
notification (i’m thinking of maybe adding an ofproto specific deferred work
queue, but still trigger that via RCU callback).
> 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);
>
Right, reverted.
> Similar whitespace changes in handle_flow_stats_request(),
> handle_aggregate_stats_request(), modify_flow_start_strict()?
>
All remnants of an intermediate version… fixed!
> 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);
> }
>
Thanks.
> This goofy incomplete comment of mine has persisted too long, please
> delete it (now it's in replace_rule_finish()):
> /* 'fm' says that */
>
Done!
> Acked-by: Ben Pfaff <b...@nicira.com <mailto:b...@nicira.com>>
Thanks for the review, pushed to master!
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
