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

Ben Pfaff blp at nicira.com
Mon Aug 4 19:23:05 UTC 2014


On Mon, Aug 04, 2014 at 11:32:17AM -0700, Jesse Gross wrote:
> On Mon, Aug 4, 2014 at 10:50 AM, Ben Pfaff <blp at nicira.com> wrote:
> > 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.
> 
> I think ENOENT is probably not the most descriptive error code because
> we really would have liked to use EEXIST but we needed to distinguish
> these two cases. In the context of OVS_FLOW_CMD_NEW, ENOENT really
> just means "there's an overlapping flow" and it will never be returned
> for a non-existent flow. In this case, it seems that the ordering is
> correct to me  However, in the context of OVS_FLOW_CMD_SET, ENOENT is
> returned for a non-existent flow. This is somewhat unfortunate but
> it's not actually ambiguous since it's a different command.

OK.

I applied your ack and pushed this to master.



More information about the dev mailing list