[ovs-dev] [PATCH 2/4] ofproto-dpif: Handle failed flow 'put's.

Ben Pfaff blp at nicira.com
Thu Jun 20 23:31:11 UTC 2013


On Thu, Jun 20, 2013 at 04:20:16PM -0700, Justin Pettit wrote:
> On Jun 20, 2013, at 4:06 PM, Ben Pfaff <blp at nicira.com> wrote:
> > On Thu, Jun 20, 2013 at 03:16:43PM -0700, Justin Pettit wrote:
> > The following is a little worrisome because EINVAL is very generic and
> > can indicate many kinds of errors.  If we always suppress logging
> > EINVAL, then we could miss important problems.  I am surprised that we
> > do not use EEXIST or another error that is less overloaded:
> 
> Agreed.  Andy's going to change the kernel to return EEXIST instead.
> Are you okay with the patch with EINVAL references replaced with
> EEXIST?

The other thing that worries me is that you mentioned that you saw
multiple installations of a single subfacet in a single batch.  If so,
that worries me because it seems like this could happen:

        handle_flow_miss_with_facet() queues up subfacet sf for
        installation into fast path, and sets sf->path to
        SF_FAST_PATH.

        handle_flow_miss_with_facet() queues up subfacet sf for
        installation into fast path, again, and sets sf->path to
        SF_FAST_PATH.

...

        The first installation succeeds.  OK.

        The second installation fails (EINVAL, EEXIST, whatever), so
        we set sf->path back to SF_NOT_INSTALLED.  But it was
        installed by the first "put".

But I don't understand how we'd get multiple installations to begin
with, since the first handle_flow_miss_with_facet() call would set
sf->path to SF_FAST_PATH and the second one would see that sf->path
was already SF_FAST_PATH and not try to install it again.  Do you have
any idea what was going on there?



More information about the dev mailing list