[ovs-dev] [PATCH 2/2] [RFC] classifier: Add support for conjunctive matches.

Ben Pfaff blp at nicira.com
Mon Jan 5 21:55:36 UTC 2015


On Mon, Jan 05, 2015 at 01:49:18PM -0800, Jarno Rajahalme wrote:
> 
> On Dec 30, 2014, at 5:16 PM, Ben Pfaff <blp at 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 <blp at 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!



More information about the dev mailing list