[net-next,v2,1/2] vxlan: Notify drivers for listening UDP port changes

From: Joseph Gasparakis <joseph.gasparakis@intel.com>

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 <john.r.fastabend@intel.com>
CC: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Joseph Gasparakis <joseph.gasparakis@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
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(-)

Thu, Aug 29, 2013 at 02:54:37PM CEST, jeffrey.t.kirsher@intel.com wrote:
>From: Joseph Gasparakis <joseph.gasparakis@intel.com>
>
>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 <john.r.fastabend@intel.com>
>CC: Stephen Hemminger <stephen@networkplumber.org>
>Signed-off-by: Joseph Gasparakis <joseph.gasparakis@intel.com>
>Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>---
>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.


>+	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.




> };
> 
> /*
>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
--
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

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 <joseph.gasparakis@intel.com>
> >
> >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 <john.r.fastabend@intel.com>
> >CC: Stephen Hemminger <stephen@networkplumber.org>
> >Signed-off-by: Joseph Gasparakis <joseph.gasparakis@intel.com>
> >Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> >---
> >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
> 
--
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

On Thu, 2013-08-29 at 05:54 -0700, Jeff Kirsher wrote:
> From: Joseph Gasparakis <joseph.gasparakis@intel.com>
> 
> This patch adds two more ndo ops: ndo_add_rx_vxlan_port() and
> ndo_del_rx_vxlan_port().
[...]
> --- 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);
>  };
[...]

I notice these are defined to return int, but the callers don't check
the return value.  Should they return void or do the callers need some
error handling?

Ben.

On Thu, 29 Aug 2013, Ben Hutchings wrote:

> On Thu, 2013-08-29 at 05:54 -0700, Jeff Kirsher wrote:
> > From: Joseph Gasparakis <joseph.gasparakis@intel.com>
> > 
> > This patch adds two more ndo ops: ndo_add_rx_vxlan_port() and
> > ndo_del_rx_vxlan_port().
> [...]
> > --- 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);
> >  };
> [...]
> 
> I notice these are defined to return int, but the callers don't check
> the return value.  Should they return void or do the callers need some
> error handling?
> 
> Ben.
> 
> 
You are right Ben, these should be defined to return void. Will fix that 
asap.

Joseph
--
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