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

[Jesse Gross]

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.