From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Herbert Subject: Re: [PATCH net-next 1/5] vxlan: implement GPE in L2 mode Date: Mon, 29 Feb 2016 09:13:50 -0800 Message-ID: References: <6fe53252314d644e994950522d4320bc54ced928.1456472746.git.jbenc@redhat.com> <20160227203142.5b7309f3@griffin> <20160229112322.4da0f0c0@griffin> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: Linux Kernel Network Developers To: Jiri Benc Return-path: Received: from mail-lf0-f49.google.com ([209.85.215.49]:34736 "EHLO mail-lf0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751056AbcB2RNw (ORCPT ); Mon, 29 Feb 2016 12:13:52 -0500 Received: by mail-lf0-f49.google.com with SMTP id j78so95910474lfb.1 for ; Mon, 29 Feb 2016 09:13:51 -0800 (PST) In-Reply-To: <20160229112322.4da0f0c0@griffin> Sender: netdev-owner@vger.kernel.org List-ID: On Mon, Feb 29, 2016 at 2:23 AM, Jiri Benc 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