From mboxrd@z Thu Jan 1 00:00:00 1970 From: "John W. Linville" Subject: Re: [RFC PATCH 5/5] geneve: add initial netdev driver for GENEVE tunnels Date: Fri, 3 Apr 2015 11:07:44 -0400 Message-ID: <20150403150744.GE31348@tuxdriver.com> References: <1428002227-11636-1-git-send-email-linville@tuxdriver.com> <1428002227-11636-6-git-send-email-linville@tuxdriver.com> <20150402202002.GG2613@nanopsycho.orion> <20150403145711.GD31348@tuxdriver.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org, "David S. Miller" , Jesse Gross , Andy Zhou , Stephen Hemminger , Alexander Duyck To: Jiri Pirko Return-path: Received: from charlotte.tuxdriver.com ([70.61.120.58]:60057 "EHLO smtp.tuxdriver.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752559AbbDCPPQ (ORCPT ); Fri, 3 Apr 2015 11:15:16 -0400 Content-Disposition: inline In-Reply-To: <20150403145711.GD31348@tuxdriver.com> Sender: netdev-owner@vger.kernel.org List-ID: 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 > > >--- > > > 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.