Re: [RFC PATCH 5/5] geneve: add initial netdev driver for GENEVE tunnels

["John W. Linville" <linville@tuxdriver.com>]

On Thu, Apr 02, 2015 at 10:20:02PM +0200, Jiri Pirko wrote:
> Thu, Apr 02, 2015 at 09:17:06PM CEST, linville@tuxdriver.com wrote:
> >This is an initial implementation of a netdev driver for GENEVE
> >tunnels.  This implementation uses a fixed UDP port, and only supports
> >a single tunnel (and therefore only a single VNI) per net namespace.
> >Only IPv4 links are supported at this time.
> 
> 
> Thanks for doing this John!
> 
> 
> >
> >Signed-off-by: John W. Linville <linville@tuxdriver.com>
> >---
> > drivers/net/Kconfig          |  14 ++
> > drivers/net/Makefile         |   1 +
> > drivers/net/geneve.c         | 451 +++++++++++++++++++++++++++++++++++++++++++
> > include/uapi/linux/if_link.h |   9 +
> > 4 files changed, 475 insertions(+)
> > create mode 100644 drivers/net/geneve.c
> >
> 
> ...
> 
> >+/* Initialize the device structure. */
> >+static void geneve_setup(struct net_device *dev)
> >+{
> >+	struct geneve_dev *geneve = netdev_priv(dev);
> >+
> >+	ether_setup(dev);
> >+
> >+	dev->netdev_ops = &geneve_netdev_ops;
> >+	dev->destructor = free_netdev;
> >+	SET_NETDEV_DEVTYPE(dev, &geneve_type);
> >+
> >+	INIT_WORK(&geneve->sock_work, geneve_sock_work);
> 
> I would push work initialization into geneve_newlink. Seems odd to have
> it here in setup.

Yes, that will probably work a lot better for multiple tunnels on a
host... :-)

> >+
> >+	dev->tx_queue_len = 0;
> >+	dev->features = 0;
> >+
> >+	dev->vlan_features = dev->features;
> >+	dev->hw_features = 0;
> >+
> >+	geneve->dev = dev;
> >+}
> >+
> >+static const struct nla_policy geneve_policy[IFLA_GENEVE_MAX + 1] = {
> >+	[IFLA_GENEVE_ID]		= { .type = NLA_U32 },
> >+	[IFLA_GENEVE_REMOTE]		= { .len = FIELD_SIZEOF(struct iphdr, daddr) },
> >+};
> >+
> >+static int geneve_validate(struct nlattr *tb[], struct nlattr *data[])
> >+{
> >+	if (tb[IFLA_ADDRESS]) {
> >+		if (nla_len(tb[IFLA_ADDRESS]) != ETH_ALEN)
> >+			return -EINVAL;
> >+
> >+		if (!is_valid_ether_addr(nla_data(tb[IFLA_ADDRESS])))
> >+			return -EADDRNOTAVAIL;
> >+	}
> >+
> >+	if (!data)
> >+		return -EINVAL;
> >+
> >+	if (data[IFLA_GENEVE_ID]) {
> >+		__u32 vni =  nla_get_u32(data[IFLA_GENEVE_ID]);
> 
> missing newline

Sure.

> >+		if (vni >= GENEVE_VID_MASK)
> >+			return -ERANGE;
> >+	}
> >+
> >+	return 0;
> >+}
> >+
> >+static void geneve_get_drvinfo(struct net_device *dev,
> >+			       struct ethtool_drvinfo *drvinfo)
> >+{
> >+	strlcpy(drvinfo->version, GENEVE_NETDEV_VER, sizeof(drvinfo->version));
> >+	strlcpy(drvinfo->driver, "geneve", sizeof(drvinfo->driver));
> >+}
> >+
> >+static const struct ethtool_ops geneve_ethtool_ops = {
> >+	.get_drvinfo	= geneve_get_drvinfo,
> >+	.get_link	= ethtool_op_get_link,
> >+};
> >+
> >+static int geneve_newlink(struct net *net, struct net_device *dev,
> >+			 struct nlattr *tb[], struct nlattr *data[])
> >+{
> >+	struct geneve_net *gn = net_generic(net, geneve_net_id);
> >+	struct geneve_dev *geneve = netdev_priv(dev);
> >+	__u32 vni;
> 
> why not "u32" ?

I think I copied that from vxlan.c.  In fact, I'm not really sure I
understand why both exist?
 
> >+	int err;
> >+
> >+	/* TODO: need to support multiple tunnels in a namespace */
> >+	if (!list_empty(&gn->geneve_list))
> >+		return -EBUSY;
> 
> Interesting limitation :)

That should disappear, of course. :-)

> ...
> 
> >+static void __net_exit geneve_exit_net(struct net *net)
> >+{
> >+	struct geneve_net *gn = net_generic(net, geneve_net_id);
> >+	struct geneve_dev *geneve, *next;
> >+	struct net_device *dev, *aux;
> >+	LIST_HEAD(list);
> >+
> >+	rtnl_lock();
> >+	for_each_netdev_safe(net, dev, aux)
> >+		if (dev->rtnl_link_ops == &geneve_link_ops)
> >+			unregister_netdevice_queue(dev, &list);
> >+
> >+	list_for_each_entry_safe(geneve, next, &gn->geneve_list, next) {
> >+		/* If geneve->dev is in the same netns, it was already added
> >+		 * to the list by the previous loop.
> >+		 */
> >+		if (!net_eq(dev_net(geneve->dev), net))
> >+			unregister_netdevice_queue(dev, &list);
> >+	}
> 
> I know this is c&p of vxlan, but I do not understand why the first loop
> is there. The second loop will take care of all since all devs are
> listed in ->geneve_list, right?
> 
> Also you do not need _safe variant since you traverse through
> ->geneve_list, which is not modified.

Yes, it is boilerplate from vxlan.  Maybe Stephen can explain it?

Maybe it relates to the the ordering of the unregister queue?
I'll try to figure it out...

> >+
> >+	unregister_netdevice_many(&list);
> >+	rtnl_unlock();
> >+}

Thanks for the review and suggestions!

John

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.