From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joseph Gasparakis Subject: Re: [net-next v2 1/2] vxlan: Notify drivers for listening UDP port changes Date: Thu, 29 Aug 2013 09:49:39 -0700 (PDT) Message-ID: References: <1377780878-32384-1-git-send-email-jeffrey.t.kirsher@intel.com> <20130829141904.GB1450@minipsycho.brq.redhat.com> Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Cc: Jeff Kirsher , davem@davemloft.net, Joseph Gasparakis , netdev@vger.kernel.org, gospo@redhat.com, sassmann@redhat.com, John Fastabend , Stephen Hemminger To: Jiri Pirko Return-path: Received: from mga03.intel.com ([143.182.124.21]:1331 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755511Ab3H2Qer (ORCPT ); Thu, 29 Aug 2013 12:34:47 -0400 In-Reply-To: <20130829141904.GB1450@minipsycho.brq.redhat.com> Sender: netdev-owner@vger.kernel.org List-ID: On Thu, 29 Aug 2013, Jiri Pirko wrote: > Thu, Aug 29, 2013 at 02:54:37PM CEST, jeffrey.t.kirsher@intel.com wrote: > >From: Joseph Gasparakis > > > >This patch adds two more ndo ops: ndo_add_rx_vxlan_port() and > >ndo_del_rx_vxlan_port(). > > > >Drivers can get notifications through the above functions about changes > >of the UDP listening port of VXLAN. Also, when physical ports come up, > >now they can call vxlan_get_rx_port() in order to obtain the port number(s) > >of the existing VXLAN interface in case they already up before them. > > > >This information about the listening UDP port would be used for VXLAN > >related offloads. > > > >A big thank you to John Fastabend (john.r.fastabend@intel.com) for his input > >and his suggestions on this patch set. > > > >CC: John Fastabend > >CC: Stephen Hemminger > >Signed-off-by: Joseph Gasparakis > >Signed-off-by: Jeff Kirsher > >--- > >v2: Now locking on sock_lock around vxlan_get_rx_port() in order to avoid > > races with vxlan removing ports and drivers still keeping resources for > > them. > >--- > > drivers/net/vxlan.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++- > > include/linux/netdevice.h | 16 ++++++++++++++ > > include/net/vxlan.h | 1 + > > 3 files changed, 72 insertions(+), 1 deletion(-) > > > >diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c > >index 3b21aca..65d1df5 100644 > >--- a/drivers/net/vxlan.c > >+++ b/drivers/net/vxlan.c > >@@ -456,6 +456,32 @@ static int vxlan_fdb_append(struct vxlan_fdb *f, > > return 1; > > } > > > >+/* Notify netdevs that UDP port started listening */ > >+static void vxlan_notify_add_rx_port(struct net *net, __be16 port) > >+{ > >+ struct net_device *dev; > >+ > >+ rcu_read_lock(); > >+ for_each_netdev_rcu(net, dev) { > >+ if (dev->netdev_ops->ndo_add_vxlan_port) > >+ dev->netdev_ops->ndo_add_vxlan_port(dev, htons(port)); > >+ } > > > Is it really desirable to notify *all* devices? Even those who are not > involved in vxlan traffic? I think that only affected devices should be > notified. Lower device list being currently introduced by: > "[PATCH net-next v3 02/13] net: add lower_dev_list to net_device and make a full mesh" > Could be used for this. > Just to be clear this notifies the devices that implement ndo_add_vxlan_port, not all (which I think is what you meant anyway). About lower_dev_list, VXLAN implementation is not like VLAN where you know which dev you will be transmitting on in advance, hence we need to notify all the devices that implement the ndo's. > > >+ rcu_read_unlock(); > >+} > >+ > >+/* Notify netdevs that UDP port is no more listening */ > >+static void vxlan_notify_del_rx_port(struct net *net, __be16 port) > >+{ > >+ struct net_device *dev; > >+ > >+ rcu_read_lock(); > >+ for_each_netdev_rcu(net, dev) { > >+ if (dev->netdev_ops->ndo_del_vxlan_port) > >+ dev->netdev_ops->ndo_del_vxlan_port(dev, htons(port)); > >+ } > >+ rcu_read_unlock(); > >+} > >+ > > /* Add new entry to forwarding table -- assumes lock held */ > > static int vxlan_fdb_create(struct vxlan_dev *vxlan, > > const u8 *mac, __be32 ip, > >@@ -797,13 +823,15 @@ static void vxlan_sock_hold(struct vxlan_sock *vs) > > > > void vxlan_sock_release(struct vxlan_sock *vs) > > { > >- struct vxlan_net *vn = net_generic(sock_net(vs->sock->sk), vxlan_net_id); > >+ struct net *net = sock_net(vs->sock->sk); > >+ struct vxlan_net *vn = net_generic(net, vxlan_net_id); > > > > if (!atomic_dec_and_test(&vs->refcnt)) > > return; > > > > spin_lock(&vn->sock_lock); > > hlist_del_rcu(&vs->hlist); > >+ vxlan_notify_del_rx_port(net, inet_sk(vs->sock->sk)->inet_sport); > > spin_unlock(&vn->sock_lock); > > > > queue_work(vxlan_wq, &vs->del_work); > >@@ -1543,6 +1571,31 @@ static struct device_type vxlan_type = { > > .name = "vxlan", > > }; > > > >+/* Calls the ndo_add_vxlan_port of the caller in order to > >+ * supply the listening VXLAN udp ports. > >+ */ > >+void vxlan_get_rx_port(struct net_device *dev) > >+{ > >+ struct vxlan_sock *vs; > >+ struct net *net = dev_net(dev); > >+ struct vxlan_net *vn = net_generic(net, vxlan_net_id); > >+ u16 port; > >+ int i; > >+ > >+ if (!dev || !dev->netdev_ops || !dev->netdev_ops->ndo_add_vxlan_port) > >+ return; > >+ > >+ spin_lock(&vn->sock_lock); > >+ for (i = 0; i < PORT_HASH_SIZE; ++i) { > >+ hlist_for_each_entry_rcu(vs, vs_head(net, i), hlist) { > >+ port = htons(inet_sk(vs->sock->sk)->inet_sport); > >+ dev->netdev_ops->ndo_add_vxlan_port(dev, port); > >+ } > >+ } > >+ spin_unlock(&vn->sock_lock); > >+} > >+EXPORT_SYMBOL_GPL(vxlan_get_rx_port); > >+ > > /* Initialize the device structure. */ > > static void vxlan_setup(struct net_device *dev) > > { > >@@ -1723,6 +1776,7 @@ static struct vxlan_sock *vxlan_socket_create(struct net *net, __be16 port, > > inet_sk(sk)->mc_loop = 0; > > spin_lock(&vn->sock_lock); > > hlist_add_head_rcu(&vs->hlist, vs_head(net, port)); > >+ vxlan_notify_add_rx_port(net, port); > > spin_unlock(&vn->sock_lock); > > > > /* Mark socket as an encapsulation socket. */ > >diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > >index 077363d..f56b141 100644 > >--- a/include/linux/netdevice.h > >+++ b/include/linux/netdevice.h > >@@ -948,6 +948,18 @@ struct netdev_phys_port_id { > > * Called to get ID of physical port of this device. If driver does > > * not implement this, it is assumed that the hw is not able to have > > * multiple net devices on single physical port. > >+ * > >+ * int (*ndo_add_vxlan_port)(struct net_device *dev, > >+ * __u16 port); > >+ * Called by vxlan to notiy a driver about the UDP port that vxlan > >+ * is listnening to. It is called only when a new port starts listening. > >+ * The operation is protected by the vxlan_net->sock_lock. > >+ * > >+ * int (*ndo_del_vxlan_port)(struct net_device *dev, > >+ * __u16 port); > >+ * Called by vxlan to notify the driver about a UDP port of vxlan > >+ * that is not listening anymore. The operation is protected by > >+ * the vxlan_net->sock_lock. > > */ > > struct net_device_ops { > > int (*ndo_init)(struct net_device *dev); > >@@ -1078,6 +1090,10 @@ struct net_device_ops { > > bool new_carrier); > > int (*ndo_get_phys_port_id)(struct net_device *dev, > > struct netdev_phys_port_id *ppid); > >+ int (*ndo_add_vxlan_port)(struct net_device *dev, > >+ __u16 port); > >+ int (*ndo_del_vxlan_port)(struct net_device *dev, > >+ __u16 port); > > > It doesn't look correct to add more specific device ndos like this. I > think they should be more general. How about to rather use netdev notifier > to propagate the change and then driver can use come vxlan exported > function to get ports in use. > Please see comment from Ben Hutchings on previously similar question. > > > > > }; > > > > /* > >diff --git a/include/net/vxlan.h b/include/net/vxlan.h > >index ad342e3..a0dc497 100644 > >--- a/include/net/vxlan.h > >+++ b/include/net/vxlan.h > >@@ -36,4 +36,5 @@ int vxlan_xmit_skb(struct net *net, struct vxlan_sock *vs, > > > > __be16 vxlan_src_port(__u16 port_min, __u16 port_max, struct sk_buff *skb); > > > >+void vxlan_get_rx_port(struct net_device *netdev); > > #endif > >-- > >1.8.3.1 > > > >-- > >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 >