[ovs-dev] [PATCH] ofproto: Avoid abandoning an ofopgroup without committing it.

Ben Pfaff blp at nicira.com
Fri Oct 25 21:25:10 UTC 2013


On Fri, Oct 25, 2013 at 02:04:44PM -0700, Jarno Rajahalme wrote:
> 
> On Oct 25, 2013, at 1:56 PM, Ben Pfaff <blp at 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 <blp at 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 <blp at 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 <jrajahalme at vmware.com>
> >>>>> Signed-off-by: Ben Pfaff <blp at 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).



More information about the dev mailing list