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

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

On Fri, Apr 03, 2015 at 10:57:12AM -0400, John W. Linville wrote:
> 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... :-)

Ignore that comment -- I was thinking...something else...need coffee... ;-)

What makes newlink better for INIT_WORK than setup?

> 
> > >+
> > >+	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.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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