From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jiri Pirko Subject: Re: [RFC PATCH 5/5] geneve: add initial netdev driver for GENEVE tunnels Date: Fri, 3 Apr 2015 17:20:53 +0200 Message-ID: <20150403152053.GF16847@nanopsycho.orion> 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> <20150403150744.GE31348@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: "John W. Linville" Return-path: Received: from mail-wg0-f46.google.com ([74.125.82.46]:33619 "EHLO mail-wg0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752489AbbDCPU4 (ORCPT ); Fri, 3 Apr 2015 11:20:56 -0400 Received: by wgin8 with SMTP id n8so24036087wgi.0 for ; Fri, 03 Apr 2015 08:20:55 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20150403150744.GE31348@tuxdriver.com> Sender: netdev-owner@vger.kernel.org List-ID: Fri, Apr 03, 2015 at 05:07:44PM CEST, linville@tuxdriver.com wrote: >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? Can be here for sure. I just thought that setup should setup netdev according to type. This is not it. This initializes a part of priv structure. > >> >> > >+ >> > >+ 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.