On Mon, Feb 29, 2016 at 2:23 AM, Jiri Benc <jb...@redhat.com> wrote: > On Sat, 27 Feb 2016 12:54:52 -0800, Tom Herbert wrote: >> Yes, but RCO has not been specified for VXLAN-GPE either > > As far as I can see, RCO will just work with VXLAN-GPE. But I have no > problem disallowing them to be set together, if you prefer that. > >> so the patch >> does not correctly refuse setting those two together. Inevitably >> though, those and other extensions will defined for VXLAN-GPE and new >> ones for VXLAN. Again, the protocols are fundamentally incompatible, >> so instead of trying to enforce each valid combination at >> configuration > > We need to do the checking in either case. If we accepted unsupported > combinations and then just silently ignored them, we'd be in troubles > later when such combination becomes defined/supported. There would be > no way for the userspace tools to detect whether a particular kernel > supports the combination or not. > > So, we need to check for supported combination of options during > configuration anyway. > > And when we have that, I don't really see the reason for doing that > kind of code duplication that you suggest. > >> or performing multiple checks for flavor each time we >> look at a packet, it seems easier to split the parsing with at most >> one check for the protocol variant. For instance in >> vxlan_udp_encap_recv just do: >> >> if (vs->flags & VXLAN_F_GPE) >> if (!vxlan_parse_gpe_hdr(&unparsed, skb, vs->flags)) >> goto drop; >> else >> if (!vxlan_parse_gpe(&unparsed, skb, vs->flags)) >> goto drop; > > Most of the code of these two functions will be identical. To > consolidate that as much as possible, you'll end up with what I have or > something very similar. > >> And then move REMCSUM and GPB and other protocol specific checks to >> the right function. > > And when RCO is defined for GPE, we copy the code? Doesn't make sense, > sorry. > > If you look at the code in the current net-next (and the code after > this patchset), the extension handling has been made generic and each > extension gets its own handler function, leading to clean separation in > the code. There's no reason to split the vxlan_rcv into two functions > doing the same things but with slightly different calls to extensions. > They may or may not be "slightly different"; if they are the same (like RCO for VXLAN-GPE uses the low order bits in VNI) then a common backend function can be called.
As defined now, GPB can't be used with VXLAN-GPE at all, but when I read your patch it looks very much like GPB is being checked and allowed in the VXLAN-GPE path. The fact that "if (vs->flags & VXLAN_F_GBP)" always fails for VXLAN-GPE packets because of configuration constraints is not at all obvious, and really this just results in an unnecessary conditional that gives the same answer for every single VXLAN-GPE packet which we've already checked for just a few lines above. At least the check for GPB could be moved to an else block of " if (vs->flags & VXLAN_F_GPE)", this alone improves clarity and eliminates an unnecessary conditional in the VXLAN-GPE path. > Jiri
