Hi Jiannan,
please keep in mind I have zero clue about OVS, so I cannot really
comment on what is customary in that context and what not. However,
some general review from the GTP point of view below:
On Wed, Jul 12, 2017 at 05:44:54PM -0700, Jiannan Ouyang wrote:
quoted hunk
Add the gtp_create_flow_based_dev() interface to create flow-based gtp
net_device, which sets gtp->collect_md. Under flow-based mode, UDP sockets are
created and maintained in kernel.
Signed-off-by: Jiannan Ouyang <redacted>
---
drivers/net/gtp.c | 213 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
include/net/gtp.h | 8 ++
2 files changed, 217 insertions(+), 4 deletions(-)
diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 5a7b504..09712c9 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -642,9 +642,94 @@ static netdev_tx_t gtp_dev_xmit(struct sk_buff *skb, struct net_device *dev)
return NETDEV_TX_OK;
}
+static int gtp_hashtable_new(struct gtp_dev *gtp, int hsize);
+static void gtp_hashtable_free(struct gtp_dev *gtp);
+static int gtp_encap_enable(struct gtp_dev *gtp, struct nlattr *data[]);
+
+static int gtp_change_mtu(struct net_device *dev, int new_mtu, bool strict)
+{
+ int max_mtu = IP_MAX_MTU - dev->hard_header_len - sizeof(struct iphdr)
+ - sizeof(struct udphdr) - sizeof(struct gtp1_header);
+
+ if (new_mtu < ETH_MIN_MTU)
+ return -EINVAL;
+
+ if (new_mtu > max_mtu) {
+ if (strict)
+ return -EINVAL;
+
+ new_mtu = max_mtu;
+ }
+
+ dev->mtu = new_mtu;
+ return 0;
+}
+
+static int gtp_dev_open(struct net_device *dev)
+{
+ struct gtp_dev *gtp = netdev_priv(dev);
+ struct net *net = gtp->net;
+ struct socket *sock1u;
+ struct socket *sock0;
+ struct udp_tunnel_sock_cfg tunnel_cfg;
+ struct udp_port_cfg udp_conf;
+ int err;
+
+ memset(&udp_conf, 0, sizeof(udp_conf));
+
+ udp_conf.family = AF_INET;
+ udp_conf.local_ip.s_addr = htonl(INADDR_ANY);
+ udp_conf.local_udp_port = htons(GTP1U_PORT);
+
+ err = udp_sock_create(gtp->net, &udp_conf, &sock1u);
+ if (err < 0)
+ return err;
+
+ udp_conf.local_udp_port = htons(GTP0_PORT);
+ err = udp_sock_create(gtp->net, &udp_conf, &sock0);
+ if (err < 0)
+ return err;
you're unconditionally binding to both GTP0 and GTP1 UDP ports. This is
done selectively based on netlink attributes in the existing "normal"
non-OVS kernel code, i.e. the control is left to the user.
Is this function is only called/used in the context of OVS? If so,
since you explicitly implement only GTPv1, why bind to GTPv0 port?
+ setup_udp_tunnel_sock(net, sock1u, &tunnel_cfg);
even here, you're only setting up the v1 and not v0.
+ /* Assume largest header, ie. GTPv0. */
+ dev->needed_headroom = LL_MAX_HEADER +
+ sizeof(struct iphdr) +
+ sizeof(struct udphdr) +
+ sizeof(struct gtp0_header);
... and here you're using headroom for a GTPv0 header, despite (I think)
only supporting GTPv1 from this configuration?
+ err = gtp_hashtable_new(gtp, GTP_PDP_HASHSIZE); // JO: when to free??
I think that question about when to free needs to be resolved before any
merge. Did you check that it persists even after the device is
closed/removed?
--
- Harald Welte [off-list ref] http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
Hi Harald,
On 7/13/17, 12:35 AM, "Harald Welte" [off-list ref] wrote:
quoted
+static int gtp_dev_open(struct net_device *dev)
+{
+ struct gtp_dev *gtp = netdev_priv(dev);
+ struct net *net = gtp->net;
+ struct socket *sock1u;
+ struct socket *sock0;
+ struct udp_tunnel_sock_cfg tunnel_cfg;
+ struct udp_port_cfg udp_conf;
+ int err;
+
+ memset(&udp_conf, 0, sizeof(udp_conf));
+
+ udp_conf.family = AF_INET;
+ udp_conf.local_ip.s_addr = htonl(INADDR_ANY);
+ udp_conf.local_udp_port = htons(GTP1U_PORT);
+
+ err = udp_sock_create(gtp->net, &udp_conf, &sock1u);
+ if (err < 0)
+ return err;
+
+ udp_conf.local_udp_port = htons(GTP0_PORT);
+ err = udp_sock_create(gtp->net, &udp_conf, &sock0);
+ if (err < 0)
+ return err;
you're unconditionally binding to both GTP0 and GTP1 UDP ports. This is
done selectively based on netlink attributes in the existing "normal"
non-OVS kernel code, i.e. the control is left to the user.
Is this function is only called/used in the context of OVS? If so,
since you explicitly implement only GTPv1, why bind to GTPv0 port?
I had doubts on how this flow-based GTPv1 code path should fit in, which is why
the GTPv0 and the GTPv1 code pieces are mixed in my changes. Should I
explicitly claim that the flow-based change is for GTPv1 only?
quoted
+ setup_udp_tunnel_sock(net, sock1u, &tunnel_cfg);
even here, you're only setting up the v1 and not v0.
same reason as above.
quoted
+ /* Assume largest header, ie. GTPv0. */
+ dev->needed_headroom = LL_MAX_HEADER +
+ sizeof(struct iphdr) +
+ sizeof(struct udphdr) +
+ sizeof(struct gtp0_header);
... and here you're using headroom for a GTPv0 header, despite (I think)
only supporting GTPv1 from this configuration?
Yes, only GTPv1 is supported.
quoted
+ err = gtp_hashtable_new(gtp, GTP_PDP_HASHSIZE); // JO: when to free??
I think that question about when to free needs to be resolved before any
merge. Did you check that it persists even after the device is
closed/removed?
I didn't investigate it. What do you mean by persist?
Thanks
-Jiannan
Hi Jiannan,
On Fri, Jul 14, 2017 at 01:01:34AM +0000, Jiannan Ouyang wrote:
quoted
you're unconditionally binding to both GTP0 and GTP1 UDP ports. This is
done selectively based on netlink attributes in the existing "normal"
non-OVS kernel code, i.e. the control is left to the user.
Is this function is only called/used in the context of OVS? If so,
since you explicitly implement only GTPv1, why bind to GTPv0 port?
I had doubts on how this flow-based GTPv1 code path should fit in, which is why
the GTPv0 and the GTPv1 code pieces are mixed in my changes.
Well, I know nothing about flow-based paths and OVS, so if you are the
one proposing related changes to me as the maintainer, you need to make
sure that your changes are consistend and useful within your use
case/scenario while making sure that the existing features don't break.
If you refactor generic code (used by "classic" GTP tunneling + your new
flow based tunneling), and that old code worked with GTPv0 and GTPv1,
then your modifications must make sure that they continue to support
GTPv0 and v1 in the "classic tunnel" use case.
If your new code for flow-based tunneling simply only implements v0, it
is fine to me - but then those restrictions must be in the flow-based
part only, and things must be consistent. I.e. in this case, for the
flow-based tunnel approach you must not bind the v0 port, if you don't
handle related packets.
Should I explicitly claim that the flow-based change is for GTPv1
only?
That's definitely important, too - but is not the point I raised (see
above).
quoted
quoted
+ setup_udp_tunnel_sock(net, sock1u, &tunnel_cfg);
even here, you're only setting up the v1 and not v0.
same reason as above.
yes, but if I read your code correctly, this is generic/shared code that
will break the existing GTPv0 support in the "classic tunnel" case!
quoted
quoted
+ /* Assume largest header, ie. GTPv0. */
+ dev->needed_headroom = LL_MAX_HEADER +
+ sizeof(struct iphdr) +
+ sizeof(struct udphdr) +
+ sizeof(struct gtp0_header);
... and here you're using headroom for a GTPv0 header, despite (I think)
only supporting GTPv1 from this configuration?
Yes, only GTPv1 is supported.
well, then I suggest you don't generate headroom for a v0 header (which
is larger) in a v1-only code path :)
quoted
quoted
+ err = gtp_hashtable_new(gtp, GTP_PDP_HASHSIZE); // JO: when to free??
I think that question about when to free needs to be resolved before any
merge. Did you check that it persists even after the device is
closed/removed?
I didn't investigate it. What do you mean by persist?
"persist" means "remains allocated after the release of the network
device". Whatever you allocate during device creation you must
de-allocate on device release. I cannot tell you when exactly (as I'm
not familiar with OVS or flow-based tunneling, as indicateD). However,
I know for sure we cannot introduce code that looks like it introduces
memory leaks to the kernel :)
Regards,
Harald
--
- Harald Welte [off-list ref] http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)