On Mon, Dec 21, 2015 at 6:21 PM, Ben Pfaff <[email protected]> wrote:
> On Mon, Dec 14, 2015 at 05:23:36PM -0800, Daniele Di Proietto wrote:
>> In the ODP context an empty mask netlink attribute usually means that
>> the flow should be an exact match.
>>
>> odp_flow_key_to_mask() instead returns a struct flow_wildcards
>> with matches only on recirc_id and vlan_tci.
>>
>> A more appropriate behavior is to handle a missing (zero length) netlink
>> mask specially (like we do in userspace and Linux datapath) and create
>> an exact match flow_wildcards from the original flow.
>>
>> This fixes a bug in revalidate_ukey(): every flow created with
>> megaflows disabled would be revalidated away, because the mask would
>> seem too generic. (Another possible fix would be to handle the special
>> case of a missing mask in revalidate_ukey(), but this seems a more
>> generic solution).
>>
>> Signed-off-by: Daniele Di Proietto <[email protected]>
>> Acked-by: Jarno Rajahalme <[email protected]>
>
> I'm a little concerned about introducing this new code here to work with
> fields using Netlink in odp-util.c through the metaflow code. On
> master, we've applied commit 9f861c9182ea42 (dpif-netdev: Don't use
> metaflow to operate on userspace datapath fields.) to get rid of a
> similar issue because, according to Jesse's commit message:
>
> There is a conceptual mismatch here because metaflow is operating on
> OpenFlow fields, not datapath ones. Even though they are generally very
> similar, there are subtle differences, which is why it is necessary to
> fix up the input port mask.
>
> This backport introduces code into odp-util that is similar to the code
> that Jesse deleted from dpif-netdev. Is that OK?
In the context of branch-2.3, this is just moving code that already
existed in dpif-netdev.c, so I'm not sure that it's any worse than
what is already there. The existing code that was there at the time
that I made the commit isn't actually buggy, it's just that
conceptually it wasn't quite right and that required handling a
special case (and I was about to add more cases). For the purposes of
a backport, it seems reasonable to just move the code around.
I think there might be an issue with the patch though: it looks like
that special case code didn't move to odp-util.c. At the bottom of
dpif-netdev.c:dpif_netdev_mask_from_nlattrs(), there is the following
comment and code:
/* Force unwildcard the in_port.
*
* We need to do this even in the case where we unwildcard "everything"
* above because "everything" only includes the 16-bit OpenFlow port number
* mask->in_port.ofp_port, which only covers half of the 32-bit datapath
* port number mask->in_port.odp_port. */
mask->in_port.odp_port = u32_to_odp(UINT32_MAX);
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
