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:
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 <ouyangj(a)fb.com>
---
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 <laforge(a)gnumonks.org>
http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)