[ovs-dev] [PATCH] ofproto-dpif: Use DPIF_FP_CREATE but not DPIF_FP_MODIFY.

Ben Pfaff blp at nicira.com
Mon Aug 4 17:50:54 UTC 2014

On Mon, Aug 04, 2014 at 10:15:17AM -0700, Jesse Gross wrote:
> On Fri, Aug 1, 2014 at 5:22 PM, Ben Pfaff <blp at nicira.com> wrote:
> > A dpif reports EEXIST if a flow put operation that should create a new flow
> > instead attempts to modify an existing flow, or ENOENT if a flow put would
> > create a flow that overlaps some existing flow.  The latter does not always
> > indicate a bug in OVS userspace, because it can also mean that two
> > userspace OVS packet handler threads received packets that generated
> > the same megaflow for different microflows.  Until now, userspace has
> > logged this, which confuses users by making them suspect a bug.  We could
> > simply not log ENOENT in userspace, but that would suppress logging for
> > genuine bugs too.  Instead, this commit drops DPIF_FP_MODIFY from flow
> > put operations in ofproto-dpif, which transforms this particular
> > problem into EEXIST, which userspace already logs at "debug" level (see
> > flow_message_log_level()), effectively suppressing the logging in normal
> > circumstances.
> >
> > It appears that in practice ofproto-dpif doesn't actually ever need to
> > modify flows in the datapath, only create and delete them, so this
> > shouldn't cause problems.
> >
> > Suggested-by: Jesse Gross <jesse at nicira.com>
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> Acked-by: Jesse Gross <jesse at nicira.com>

Thinking further, I actually have some misgivings about this patch
that I'd like to discuss before applying it to master.  Basically, I
think that the datapath is testing for EEXIST and ENOENT in the wrong
order: it should test for ENOENT before it tests for EEXIST, because
otherwise you can get the following weird results:

        1. Try to create a flow (DPIF_FP_CREATE) -> EEXIST (there's
           already a flow like that).

        2. Try to modify the same flow (DPIF_FP_MODIFY with or without
           DPIF_FP_CREATE) -> ENOENT (there's no flow like that!).

I guess that if one interprets ENOENT as having two different meanings
("there's no flow like that" or "there's a flow overlapping that one")
then it still makes some sense, and maybe that is the intended
interpretation.  If so, I'm happy with this.  It would be even better
if we had separate error codes for those two possibilities, but
perhaps that ship has sailed.

More information about the dev mailing list