On Mon, Jan 05, 2015 at 01:49:18PM -0800, Jarno Rajahalme wrote:
>
> On Dec 30, 2014, at 5:16 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> > 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?
>
> Reading my comments again, this seems the right thing to do. The comment
> block above is not very clear to me, however. How about:
>
> /* 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.
> The
> * conjunctive match set is used in the lookup process to figure
> which
> * (if any) collection of conjunctive sets the packet matches with.
> * However, a rule with conjunction actions is never to be
> returned as
> * a classifier lookup result. To make sure a rule with
> conjunction action
> * is not returned as a lookup result, we update them in a
> carefully
> * chosen order:
> *
> * - If we're adding a conjunctive match set where there wasn't one
> * before, we have to make the conjunctive match set available to
> * lookups before the rule?s actions are changed, as otherwise
> rule
> * with a conjunction action could be returned as a lookup
> result.
> *
> * - To clear some nonempty conjunctive set, we set the rule's
> * actions first, so that a lookup can?t return a rule with
> conjunction
> * actions.
> *
> * - Otherwise, order doesn't matter for changing one nonempty
> * conjunctive match set to some other nonempty set, since the
> * rule?s actions are not seen by the classifier, and hence
> don?t
> * matter either before or after the change. */
That's better. I made that change. Thank you!
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
