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