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. */
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
