On Thu, Oct 30, 2014 at 11:39:44AM -0700, Jarno Rajahalme wrote:
>
> On Oct 29, 2014, at 11:46 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> > A "conjunctive match" allows higher-level matches in the flow table, such
> > as set membership matches, without causing a cross-product explosion for
> > multidimensional matches. Please refer to the documentation that this
> > commit adds to ovs-ofctl(8) for a better explanation, including an example.
...
> > @@ -4379,6 +4419,7 @@ modify_flows__(struct ofproto *ofproto, struct
> > ofputil_flow_mod *fm,
> > if (change_actions) {
> > ovsrcu_set(&rule->actions, rule_actions_create(fm->ofpacts,
> >
> > fm->ofpacts_len));
> > + set_conjunction(rule);
>
> Does the above need to be atomic for the lookup? If not, does it matter which
> is visible first? As it is now, it seems possible that the lookup is
> successful and the new actions are used when the lookup was done with the old
> conjunctions.
>
> If this is removing conjunctions, then old conjunctions could be used in the
> lookup and the new actions ignored. This is fine, I think.
>
> If this adds conjunctions, then a rule with conjunction action could be
> returned as a hard match, if the lookup is done before the conjunctions have
> been set, but the actions are set before the thread doing the lookup gets
> them.
>
> Setting the conjunctions first would result in the following:
>
> Removing conjunctions: the rule is changed to a hard match, then the
> hard_match could be returning the conjunction action.
>
> Adding conjunctions: This should be OK, new conjunctions are used, while the
> actions still point to the old actions. Since (incomplete) soft matches are
> never returned as a lookup result, the old actions would not be used any more.
>
> Maybe solve this by setting the conjunctions first, and then have the thread
> doing the lookup repeat the lookup if the actions of the returned rule
> contain the conjunction action? This should be pretty rare incidence, but
> this could solve the race here.
>
> Another possibility is to treat a modify which only changes actions, and
> where the conjunction is changed, added, or removed, with a
> classifier_replace, and give the conjunctions as a parameter to it?
I'm working on getting a v3 of this patch ready. This issue has been
the major sticking point. I think I've found a simple solution,
though. Please allow me to run it by you before I post a full v3.
Basically, one ordering is broken in the "add conjunctions" case, and
the other ordering is broken in the "remove conjunctions" case. So:
use one order if we're adding conjunctions and the other if we're
removing them. Thus:
if (change_actions) {
/* We have to change the actions. The rule's conjunctive match set
* is a function of its actions, so we need to update that too. We
* update them in a carefully chosen order:
*
* - If we're adding a conjunctive match set where there wasn't one
* before, we set the conjunctive match set before the rule's
* actions.
*
* - To clear some nonempty conjunctive set, we set the rule's
* actions first.
*
* In either case, with the ordering reversed, a classifier lookup
* could find the conjunctive match action, which should never
* happen.
*
* Order doesn't matter for changing one nonempty conjunctive match
* set to some other nonempty set, since the actions don't matter
* either before or after the change. */
bool conjunctive = is_conjunction(fm->ofpacts, fm->ofpacts_len);
if (conjunctive) {
set_conjunction(rule);
}
ovsrcu_set(&rule->actions, rule_actions_create(fm->ofpacts,
fm->ofpacts_len));
if (!conjunctive) {
set_conjunction(rule);
}
}
Thoughts?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
