[ovs-dev] [loops 6/6] datapath: Detect and suppress flows that are implicated in loops.

[Ben Pfaff]

On Mon, Jul 26, 2010 at 05:55:54PM -0700, Jesse Gross wrote:
> On Fri, Jul 23, 2010 at 4:44 PM, Ben Pfaff <blp at nicira.com> wrote:
> >
> > +               err = execute_actions(dp, skb, &key, acts->actions,
> > +                                     acts->n_actions, GFP_ATOMIC);
> > +               if (err == -ELOOP)
> > +                       acts->n_actions = 0;
> 
> Can we add an unlikely() on the error condition?  I'm trying to be more
> consistent about doing this, at least on the fast path.

Thanks, I've done that now.  It's good to do that.

> > -       if (unlikely(*loop_count > VPORT_MAX_LOOPS)) {
> > +       if (unlikely(loop->max_count > VPORT_MAX_LOOPS)) {
> >                if (net_ratelimit())
> >                        printk(KERN_WARNING "%s: dropping packet that has
> > looped more than %d times\n",
> >                               dp_name(vport_get_dp_port(vport)->dp),
> > VPORT_MAX_LOOPS);
> 
> 
> Were you intending to clear all the flows that lead to the loop?  If so, I
> don't think this will work because the check of max_count happens before it
> is incremented by later recursions (which will appear to happen during
> vport->ops->send() and the return code won't tell you anything useful).
> 
> In practice this may not matter all that much because in most cases loops
> will repeatedly traverse the same datapath.  In that case there is only one
> flow, so disabling the last flow is the same as disabling the root flow.
>  However, if that's what we want then it isn't necessary to keep track of
> max_count.

Oh gosh, you're right.  If I had a better test case I would have noticed
that.

I've respun the implementation to fix that problem and I'm going to
re-test.

At least as much as review of the implementation (which you did better
than I did on the implementation), I was also looking for an opinion
whether you think that this is a good way to solve the problem.  Do you?