On 22/12/2015 12:23, "Jesse Gross" <je...@kernel.org> wrote:
>On Mon, Dec 21, 2015 at 6:21 PM, Ben Pfaff <b...@ovn.org> 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 <diproiet...@vmware.com> >>> Acked-by: Jarno Rajahalme <jrajaha...@nicira.com> >> >> 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? I definitely share your concerns, that's why I posted the backport to the list. Given Jesse's detailed analysis I'm happier moving forward with this. >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); Oops, you're right, I didn't consider this. I guess that's why using meta-flow fields wasn't appropriate. Thanks for your reviews Ben and Jesse, I posted a v2 with the extended unwildcarding of in_port. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
