On Fri, Oct 25, 2013 at 02:25:10PM -0700, Ben Pfaff wrote:
> On Fri, Oct 25, 2013 at 02:04:44PM -0700, Jarno Rajahalme wrote:
> >
> > On Oct 25, 2013, at 1:56 PM, Ben Pfaff <b...@nicira.com> wrote:
> >
> > > On Fri, Oct 25, 2013 at 01:44:17PM -0700, Jarno Rajahalme wrote:
> > >>
> > >> On Oct 25, 2013, at 1:32 PM, Ben Pfaff <b...@nicira.com> wrote:
> > >>
> > >>> On Thu, Oct 24, 2013 at 02:08:24PM -0700, Jarno Rajahalme wrote:
> > >>>>
> > >>>>
> > >>>> On Oct 21, 2013, at 3:52 PM, Ben Pfaff <b...@nicira.com> wrote:
> > >>>>
> > >>>>> Commit e3b5693319c (Fix table checking for goto table instruction.)
> > >>>>> moved
> > >>>>> action checking into modify_flows__(), for good reason, but as a side
> > >>>>> effect made modify_flows__() abandon and never commit the ofopgroup
> > >>>>> that it
> > >>>>> started, if action checking failed. This commit fixes the problem.
> > >>>>>
> > >>>>> The following commands, run under "make sandbox", illustrate the
> > >>>>> problem.
> > >>>>> Without this change, the final command hangs because the barrier
> > >>>>> request
> > >>>>> that ovs-ofctl sends never gets a response (because barriers wait for
> > >>>>> all
> > >>>>> ofopgroups to complete, which never happens). With this commit, the
> > >>>>> commands complete quickly:
> > >>>>>
> > >>>>> ovs-vsctl add-br br0
> > >>>>> ovs-vsctl set bridge br0
> > >>>>> protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13
> > >>>>> ovs-ofctl add-flow -O OpenFlow11 br0
> > >>>>> table=1,action=mod_tp_dst:79,goto_table:2
> > >>>>> ovs-ofctl add-flow -O OpenFlow11 br0
> > >>>>> table=1,action=mod_tp_dst:79,goto_table:1
> > >>>>>
> > >>>>> Reported-by: Jarno Rajahalme <jrajaha...@vmware.com>
> > >>>>> Signed-off-by: Ben Pfaff <b...@nicira.com>
> > >>>>> ---
> > >>>>> ofproto/ofproto.c | 19 ++++++++++++-------
> > >>>>> tests/ofproto.at | 26 ++++++++++++++++++++++++++
> > >>>>> 2 files changed, 38 insertions(+), 7 deletions(-)
> > >>>>>
> > >>>>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > >>>>> index f67e1fb..8dba732 100644
> > >>>>> --- a/ofproto/ofproto.c
> > >>>>> +++ b/ofproto/ofproto.c
> > >>>>> @@ -4041,6 +4041,18 @@ modify_flows__(struct ofproto *ofproto, struct
> > >>>>> ofconn *ofconn,
> > >>>>> enum ofperr error;
> > >>>>> size_t i;
> > >>>>>
> > >>>>> + /* Verify actions before we start to modify any rules, to avoid
> > >>>>> partial
> > >>>>> + * flow table modifications. */
> > >>>>> + for (i = 0; i < rules->n; i++) {
> > >>>>> + struct rule *rule = rules->rules[i];
> > >>>>> +
> > >>>>> + error = ofpacts_check(fm->ofpacts, fm->ofpacts_len,
> > >>>>> &fm->match.flow,
> > >>>>> + u16_to_ofp(ofproto->max_ports),
> > >>>>> rule->table_id);
> > >>>>> + if (error) {
> > >>>>> + return error;
> > >>>>> + }
> > >>>>> + }
> > >>>>> +
> > >>>>
> > >>>> This fixes the problem I had, thank you!
> > >>>>
> > >>>> While we are at this, we should use ofproto_check_ofpacts() instead
> > >>>> and maybe avoid repeating the same check over and over again. How
> > >>>> about this incremental:
> > >>>
> > >>> Can we really avoid repeating the check? Since I proposed this
> > >>> change, ofpacts_check() now checks consistency of the flow and the
> > >>> actions, and since the flows vary among the rules that we are
> > >>> checking, I imagine that some of them could be inconsistent within a
> > >>> single table, even if others are not.
> > >>>
> > >>
> > >> It seems to me that we are checking the new actions against the new flow
> > >> (both from the new flow mod) in the context of the old rule's table_id,
> > >> i.e.
> > >> the check calls do not really vary by the rule (other than rule's table
> > >> id) at all.
> > >
> > > I don't understand yet.
> > >
> > > Let me provide an example. Suppose we do a flow_mod that changes all
> > > of the actions in table 0 from whatever they were previously to
> > > "mod_tp_src:80". If the first rule whose change we validate in that
> > > table satisfies the prerequisites for mod_tp_src, but other rules in
> > > the table do not satisfy the prerequisites, then I think that we would
> > > allow the flow_mod to go through without noticing the problem.
> >
> > But the old rule is never passed for the check, see:
> >
> > error = ofpacts_check(fm->ofpacts, fm->ofpacts_len, &fm->match.flow,
> > u16_to_ofp(ofproto->max_ports),
> > rule->table_id);
> >
> > The flow mod comes in with the flow (&fm->match.flow) so the exact
> > same validation is being repeated over and over again, if the
> > rule->table_id remains the same.
>
> OK, I understand now and agree.
>
> I have further thoughts here that I'm going to investigate before I
> apply your incremental (as a new patch).
I finally finished polishing my alternative version. Please take a look
at this series when you get a chance:
http://openvswitch.org/pipermail/dev/2013-November/033478.html
http://openvswitch.org/pipermail/dev/2013-November/033479.html
http://openvswitch.org/pipermail/dev/2013-November/033480.html
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
