| Message ID | 20171028000937.2631-1-tom@quantonium.net |
|---|---|
| Headers | show |
| Series | gtp: Additional feature support - Part I | expand |
Hi Tom, my apologies for only getting back to reviews now, after return from holidays I was too overloaded with plenty of other tasks. On Fri, Oct 27, 2017 at 05:09:24PM -0700, Tom Herbert wrote: > - Experimental IPv6 support As far as I can tell, my previous comments/concerns regarding an IPv6 support that is in clear violation of how it is specified is not acceptable to me, sorry. The question is - as illustrated quite verbosely before - not one of missing certain bits, but it is simply *different* from what the protocol specification says. This makes absolutely no sense to me. All it will do, is it will raise the impression that IPv6 is supported in a spec-compliant way. Furthermore, it will mean that once somebody does convert this into proper IPv6 support later, they will break the existing use cases of the non-compliant implementation that you're adding in this patch series. To me, I simply don't understand how that makes any sense. There are no known users of GTP outside of 3GPP networks, and particularly none of a different GTP flavour than the one specified in 3GPP. If there are, I would want to hear of it. And if there really are, I would strongly recommend them to use some other tunneling protocol, one that's more reasonable for normal use cases and with better protocol-level security. I wouldn't mind merging *incomplete* IPv6 support. However, I do mind merging *incompatible* or *incompliant* IPv6 support. > - Configurable networking interfaces so that GTP kernel can be > used and tested without needing GSN network emulation (i.e. no user > space daemon needed). > - Addition of a dst_cache in the GTP structure and other cleanup No issues with this from my side. I plan on setting up a test system with your patches over the weekend and give it some more testing from the use cases I know to avoid regressions. > - Common functions to get a route fo for an IP tunnel > - Fix VXLAN gro cells initialization Also no issues from my side, but that's for core networking folks to decide. > For IPv6 support, the mobile subscriber needs to allow IPv6 addresses, > and the remote endpoint can be IPv6. You are raising the impression - again - that this implementation will work with any mobile subscriber. I would bet at lot on the fact that the current IPv6 implementation will in fact *not* work with any existing equipment/devices out there. > Tested: > > Configured the matrix of IPv4/IPv6 mobile subscriber, Please indicate how that testing was done, see my comment above. Regards, Harald
On Sat, Oct 28, 2017 at 1:09 AM, Harald Welte <laforge@gnumonks.org> wrote: > Hi Tom, > > my apologies for only getting back to reviews now, after return from > holidays I was too overloaded with plenty of other tasks. > > On Fri, Oct 27, 2017 at 05:09:24PM -0700, Tom Herbert wrote: >> - Experimental IPv6 support > > As far as I can tell, my previous comments/concerns regarding an IPv6 > support that is in clear violation of how it is specified is not > acceptable to me, sorry. > > The question is - as illustrated quite verbosely before - not one > of missing certain bits, but it is simply *different* from what > the protocol specification says. > > This makes absolutely no sense to me. All it will do, is it will raise > the impression that IPv6 is supported in a spec-compliant way. > Furthermore, it will mean that once somebody does convert this into > proper IPv6 support later, they will break the existing use cases of > the non-compliant implementation that you're adding in this patch > series. > Harald, Here is what the Kconfig for the EXPERIMENTAL option says: "This is an experimental implementation that allows encapsulating IPv6 over GTP and using GTP over IPv6 for testing and development purposes. This is not a standards conformant implementation for IPv6 and GTP. More work is needed to reach that level." I don't see any ambiguity here about it not being standards complete. Nor is there any ambiguity about the its purpose to enable further development and the fact that more work is needed. This a foundation for an IPv6 datapath and is sufficient to do benchmarking and performance to determine the prospects of replacing proprietary HW with commodity servers running Linux kernel. This is a forward step to get IPv6 into GTP, and frankly the _only_ code that has been proposed. There is no reason why someone can't build upon this to make a first rate conformant implementation. In any case, I've invested as much time in this as I can for now. I'll leave it up to DaveM to decide if we wants to take all, none, or some subset of these patches. Tom
Hi Tom, On Sat, Oct 28, 2017 at 09:16:01AM -0700, Tom Herbert wrote: > Here is what the Kconfig for the EXPERIMENTAL option says: > > "This is an experimental implementation that allows encapsulating IPv6 > over GTP and using GTP over IPv6 for testing and development purposes. > This is not a standards conformant implementation for IPv6 and GTP. > More work is needed to reach that level." > > I don't see any ambiguity here about it not being standards complete. > Nor is there any ambiguity about the its purpose to enable further > development and the fact that more work is needed. As stated repeatedly: I have no issue with an *incomplete* implementation, but I have a problem with an *incompatible* one that takes left turns where the spec takes right turns. An *incomplete* implementation could still interoperate with other implementations but is e.g. missing some optional bits. It can later extended with those missing bits and wills stay compatible to users of the incomplete as well as to any later complete implementation. An *incompatible* implementation is what I have issues with. > This a foundation for an IPv6 datapath and is sufficient to do > benchmarking and performance to determine the prospects of replacing > proprietary HW with commodity servers running Linux kernel. I completely agree with that. > This is a forward step to get IPv6 into GTP, and frankly the _only_ code that > has been proposed. There is no reason why someone can't build upon > this to make a first rate conformant implementation. I also agree with this. However, I don't think it makes sense to have it in the kernel given it implements something that's actually not GTP as per the relevant specs. No matter how many disclaimers you put at it, people will still assume it's GTP if it's called GTP. And if it's only useful for benchmarking the poential of a later proper IPv6 implementation, I don't think it should go in. > In any case, I've invested as much time in this as I can for now. I'll > leave it up to DaveM to decide if we wants to take all, none, or some > subset of these patches. Thanks. As indicated, I'm planning some testing later this weekend on the non-IPv6 patches, and am happy to add my Acked-by and/or re-submit those to Dave after that. For sure, the kernel networking maintainer can merge any patches, including the proposed IPv6 patches as-is, and I will accept that. But my vote as the original author and co-maintainer of the kernel GTP code goes politely and respectfully against that - as I have made quite clear by now. Thanks + Regards, Harald
On Sat, Oct 28, 2017 at 9:47 AM, Harald Welte <laforge@gnumonks.org> wrote: > Hi Tom, > > On Sat, Oct 28, 2017 at 09:16:01AM -0700, Tom Herbert wrote: >> Here is what the Kconfig for the EXPERIMENTAL option says: >> >> "This is an experimental implementation that allows encapsulating IPv6 >> over GTP and using GTP over IPv6 for testing and development purposes. >> This is not a standards conformant implementation for IPv6 and GTP. >> More work is needed to reach that level." >> >> I don't see any ambiguity here about it not being standards complete. >> Nor is there any ambiguity about the its purpose to enable further >> development and the fact that more work is needed. > > As stated repeatedly: I have no issue with an *incomplete* implementation, > but I have a problem with an *incompatible* one that takes left turns > where the spec takes right turns. > > An *incomplete* implementation could still interoperate with other > implementations but is e.g. missing some optional bits. It can later > extended with those missing bits and wills stay compatible to users of > the incomplete as well as to any later complete implementation. > > An *incompatible* implementation is what I have issues with. > >> This a foundation for an IPv6 datapath and is sufficient to do >> benchmarking and performance to determine the prospects of replacing >> proprietary HW with commodity servers running Linux kernel. > > I completely agree with that. > >> This is a forward step to get IPv6 into GTP, and frankly the _only_ code that >> has been proposed. There is no reason why someone can't build upon >> this to make a first rate conformant implementation. > > I also agree with this. However, I don't think it makes sense to have it in > the kernel given it implements something that's actually not GTP as per > the relevant specs. No matter how many disclaimers you put at it, > people will still assume it's GTP if it's called GTP. And if it's only > useful for benchmarking the poential of a later proper IPv6 > implementation, I don't think it should go in. > >> In any case, I've invested as much time in this as I can for now. I'll >> leave it up to DaveM to decide if we wants to take all, none, or some >> subset of these patches. > > Thanks. As indicated, I'm planning some testing later this weekend on > the non-IPv6 patches, and am happy to add my Acked-by and/or re-submit > those to Dave after that. > > For sure, the kernel networking maintainer can merge any patches, > including the proposed IPv6 patches as-is, and I will accept that. But > my vote as the original author and co-maintainer of the kernel GTP code > goes politely and respectfully against that - as I have made quite clear > by now. > It's been more than a year since the GTP patches went in and I asked about a IPv6 support-- we haven't heard a peep since then until I posted these patches. IMO, no IPv6 support for something that could support it is a _major_ bug-- we are well past the where it's acceptable to put support in for IPv4 because the "customer uses it" and "we'll get around to IPv6 later". Customers are using IPv6! Failure to support it is doing nothing more than holding the protocol back as well as the kernel, In mobile this especially true, IPv6 is going to eclipse IPv4. We need GTP kernel to support IPv6! So, if you don't like these patches and don't think it's the right direction, that's fine, but as maintainer then please tell us the plan and timeline to get IPv6 supported in GTP. Tom > Thanks + Regards, > Harald > > -- > - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/ > ============================================================================ > "Privacy in residential applications is a desirable marketing option." > (ETSI EN 300 175-7 Ch. A6)
From: Tom Herbert <tom@herbertland.com> Date: Sat, 28 Oct 2017 11:37:51 -0700 > Customers are using IPv6! Customers aren't using GTP with ipv6 the way you have implemented it. Please stop weasel-wording the situation to frame it in a way that purely benefits you and your objectives. Also, just because "nobody has done the work", doesn't mean it's OK to do the work improperly and then force that implementation upon people. Harald is right, calling something GTP/ipv6 sets a certain expectation with people who actually use GTP with ipv6 and have a deployment. No matter if you mark it EXPERIMENTAL or whatever, the effect is the same. Therefore, I fully agree with Harald.
Hi Tom, sorry for the delayed response. But I remain committed in pushing the non-controversial part of your GTP patches forward. On Sat, Oct 28, 2017 at 06:47:59PM +0200, Harald Welte wrote: > Thanks. As indicated, I'm planning some testing later this weekend on > the non-IPv6 patches, and am happy to add my Acked-by and/or re-submit > those to Dave after that. After some more delays and returning from netdev 2.2, I've finally put together a testing setup and successfully (manually) tested with the following patches: 01/13 vxlan: Move gro_cells_init to ndo_init 02/13 iptunnel: Add common functions to get a tunnel route 04/13 gtp: Call common functions to get tunnel routes and add dst_cache 05/13 iptunnel: Generalize tunnel update pmtu 06/13 gtp: Change to use gro_cells 07/13 gtp: Use goto for exceptions in gtp_udp_encap_recv funcs 08/13 gtp: udp recv clean up 09/13 gtp: Call function to update path mtu 10/13 gtp: Eliminate pktinfo and add port configuration I hereby acknowledge those patches. How should we proceed? Should I a) do nothing, you will add Acked-By and re-submit? b) send an individual Acked-By in a reply to each related patch here on netdev and you will re-submit those patches? c) simply create a rebased set from those patches and re-submit them to the list for net-next myself, with the Acked-by? d) be preposterous and provide a gtp git tree for DaveM to pull from? As discussed before, I will not merge/ack IPv6 will until we have an implementation that is interoperable. I have a TODO list of other bugfixes and improvements for Kernel GTP, but I'm hopeful that IPv6 can still be addressed before the end of 2017. Regards, Harald
On Sun, Nov 12, 2017 at 12:56 PM, Harald Welte <laforge@gnumonks.org> wrote: > Hi Tom, > > sorry for the delayed response. But I remain committed in pushing > the non-controversial part of your GTP patches forward. > > On Sat, Oct 28, 2017 at 06:47:59PM +0200, Harald Welte wrote: >> Thanks. As indicated, I'm planning some testing later this weekend on >> the non-IPv6 patches, and am happy to add my Acked-by and/or re-submit >> those to Dave after that. > > After some more delays and returning from netdev 2.2, I've finally put > together a testing setup and successfully (manually) tested with the > following patches: > > 01/13 vxlan: Move gro_cells_init to ndo_init > 02/13 iptunnel: Add common functions to get a tunnel route > 04/13 gtp: Call common functions to get tunnel routes and add dst_cache > 05/13 iptunnel: Generalize tunnel update pmtu > 06/13 gtp: Change to use gro_cells > 07/13 gtp: Use goto for exceptions in gtp_udp_encap_recv funcs > 08/13 gtp: udp recv clean up > 09/13 gtp: Call function to update path mtu > 10/13 gtp: Eliminate pktinfo and add port configuration > The IPv6 related code in patches 4-10 needs to be taken out. It can be restored once there is support for IPv6. > I hereby acknowledge those patches. How should we proceed? Should I > > a) do nothing, you will add Acked-By and re-submit? > > b) send an individual Acked-By in a reply to each related patch here on > netdev and you will re-submit those patches? > > c) simply create a rebased set from those patches and > re-submit them to the list for net-next myself, with the Acked-by? > Feel free to do c). I can Ack and test once the patches are ready. Tom > d) be preposterous and provide a gtp git tree for DaveM to pull from? > > As discussed before, I will not merge/ack IPv6 will until we have an > implementation that is interoperable. I have a TODO list of other > bugfixes and improvements for Kernel GTP, but I'm hopeful that IPv6 can > still be addressed before the end of 2017. > > Regards, > Harald > -- > - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/ > ============================================================================ > "Privacy in residential applications is a desirable marketing option." > (ETSI EN 300 175-7 Ch. A6)
This patch set builds upon the initial GTP implementation to make support closer to that enjoyed by other encapsulation protocols. The major items are: - Experimental IPv6 support - Configurable networking interfaces so that GTP kernel can be used and tested without needing GSN network emulation (i.e. no user space daemon needed). - Addition of a dst_cache in the GTP structure and other cleanup Additionally, this patch set also includes: - Common functions to get a route fo for an IP tunnel - Fix VXLAN gro cells initialization For IPv6 support, the mobile subscriber needs to allow IPv6 addresses, and the remote endpoint can be IPv6. For configurable interfaces, configuration is added to allow an alternate means to configure a GTP and device. This follows the typical UDP encapsulation model of specifying a listener port for receive, and a remote address and port for transmit. Configuration is performed by iproute2/ip. I will post that in a subsequent patch set. Tested: Configured the matrix of IPv4/IPv6 mobile subscriber, IPv4/IPv6 remote peer, and GTP version 0 and 1 (eight combinations). Observed connectivity and functional netperf. Also, tested VXLAN for regression. Test using openggs with ggsn and kernel module on one side and emulated sgsn on the other. Observed connectivity and functional netperf. v2: - Split the original patch to post in parts in order to make review more manageable - Make IPv6 support experimental with a configuration option for it - Prepend hash functions with gtp - Generalize iptunnel update path MTU function and call it from gtp instead using custom code - Split original patch cleaning up udp_recv into several for easier review v3: Properly include netdev on cc v4: - Move __ip6_tnl_get_route to ipv6/route.c to avoid creting dependency on ip6_tunnel - Add "select GRO_CELLS" fo Kconfig for GTP v5: - Rebase to current next-net and repost v6: - Move __ip6_tnl_get_route from net/ipv6/route.c to net/ipv6/ip6_tunnel.c. This addresses the issue pointed out by kbuild that the function is not defined when CONFIG_DST_CACHE is not set v7: - Fixed GRO cell initialization in GTP to be in ndo_init and check return value. Credit to Subash Abhinov Kasiviswanathan for pointing out this issue. - Fixed GRO cell initialization in VXLAN to also be in ndo_init and check return value Tom Herbert (13): vxlan: Move gro_cells_init to ndo_init iptunnel: Add common functions to get a tunnel route vxlan: Call common functions to get tunnel routes gtp: Call common functions to get tunnel routes and add dst_cache iptunnel: Generalize tunnel update pmtu gtp: Change to use gro_cells gtp: Use goto for exceptions in gtp_udp_encap_recv funcs gtp: udp recv clean up gtp: Call function to update path mtu gtp: Eliminate pktinfo and add port configuration gtp: Experimental encapsulation of IPv6 packets gtp: Experimental support encpasulating over IPv6 gtp: Allow configuring GTP interface as standalone drivers/net/Kconfig | 13 +- drivers/net/gtp.c | 1046 ++++++++++++++++++++++++++++++------------ drivers/net/vxlan.c | 95 +--- include/net/ip6_tunnel.h | 31 ++ include/net/ip_tunnels.h | 36 ++ include/uapi/linux/gtp.h | 8 + include/uapi/linux/if_link.h | 3 + net/ipv4/ip_tunnel.c | 70 ++- net/ipv6/ip6_tunnel.c | 42 ++ 9 files changed, 959 insertions(+), 385 deletions(-)