Re: [PATCH net-next 1/5] vxlan: implement GPE in L2 mode

[Tom Herbert <tom@herbertland.com>]

On Mon, Feb 29, 2016 at 2:23 AM, Jiri Benc <jbenc@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