Re: [PATCH v1 2/6] net: Add a generic udp_offload_get_port function
From: Alexander Duyck <hidden>
Date: 2015-11-24 06:08:49
On 11/23/2015 01:02 PM, Anjali Singhai Jain wrote:
quoted hunk ↗ jump to hunk
The new function udp_offload_get_port replaces vxlan_get_rx_port(). This is a generic function that will help replay all udp tunnel ports irrespective of tunnel type. This way when new udp tunnels get added this function need not change. Note: Drivers besides i40e are compile tested with this change. Signed-off-by: Anjali Singhai Jain <redacted> Signed-off-by: Kiran Patil <redacted> --- drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 5 ++-- drivers/net/ethernet/broadcom/bnxt/bnxt.c | 4 +++- drivers/net/ethernet/emulex/benet/be_main.c | 4 +++- drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 3 ++- drivers/net/ethernet/intel/i40e/i40e_main.c | 5 ++-- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 ++-- drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 3 ++- drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 3 ++- drivers/net/vxlan.c | 29 ++---------------------- include/linux/netdevice.h | 2 ++ include/net/protocol.h | 2 ++ include/net/vxlan.h | 8 ------- net/ipv4/udp_offload.c | 27 ++++++++++++++++++++++ 13 files changed, 53 insertions(+), 47 deletions(-)diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c index ad2782f..56777c8 100644 --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c@@ -60,6 +60,7 @@ #include <linux/semaphore.h> #include <linux/stringify.h> #include <linux/vmalloc.h> +#include <net/protocol.h> #include "bnx2x.h" #include "bnx2x_init.h"@@ -10293,7 +10294,7 @@ sp_rtnl_not_reset: netdev_info(bp->dev, "Deleted vxlan dest port %d", port); bp->vxlan_dst_port = 0; - vxlan_get_rx_port(bp->dev); + udp_offload_get_port(bp->dev); } } #endif@@ -12499,7 +12500,7 @@ static int bnx2x_open(struct net_device *dev) #ifdef CONFIG_BNX2X_VXLAN if (IS_PF(bp)) - vxlan_get_rx_port(dev); + udp_offload_get_port(dev); #endif return 0;diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index 5b96ddf..f49ca38 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c@@ -39,6 +39,7 @@ #include <net/ip6_checksum.h> #if defined(CONFIG_VXLAN) || defined(CONFIG_VXLAN_MODULE) #include <net/vxlan.h> +#include <net/protocol.h> #endif #ifdef CONFIG_NET_RX_BUSY_POLL #include <net/busy_poll.h>@@ -4589,7 +4590,7 @@ static int __bnxt_open_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init) if (irq_re_init) { #if defined(CONFIG_VXLAN) || defined(CONFIG_VXLAN_MODULE) - vxlan_get_rx_port(bp->dev); + udp_offload_get_port(bp->dev); #endif if (!bnxt_hwrm_tunnel_dst_port_alloc( bp, htons(0x17c1),@@ -5458,6 +5459,7 @@ static void bnxt_del_vxlan_port(struct net_device *dev, sa_family_t sa_family, if (type != UDP_TUNNEL_VXLAN) return; + if (bp->vxlan_port_cnt && bp->vxlan_port == port) { bp->vxlan_port_cnt--;
This looks like a bit of white-space bleed-through from your first patch. You probably need to place it there.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c index e699deca..a4da753 100644 --- a/drivers/net/ethernet/emulex/benet/be_main.c +++ b/drivers/net/ethernet/emulex/benet/be_main.c@@ -25,6 +25,7 @@ #include <net/busy_poll.h> #include <net/udp_tunnel.h> #include <net/vxlan.h> +#include <net/protocol.h> MODULE_VERSION(DRV_VER); MODULE_DESCRIPTION(DRV_DESC " " DRV_VER);@@ -3604,7 +3605,7 @@ static int be_open(struct net_device *netdev) #ifdef CONFIG_BE2NET_VXLAN if (skyhawk_chip(adapter)) - vxlan_get_rx_port(netdev); + udp_offload_get_port(netdev); #endif return 0;@@ -5239,6 +5240,7 @@ static void be_del_vxlan_port(struct net_device *netdev, sa_family_t sa_family, if (type != UDP_TUNNEL_VXLAN) return; + if (lancer_chip(adapter) || BEx_chip(adapter) || be_is_mc(adapter)) return;
Same here. We don't need white space changes in these patches as they just clutter things up.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c index 447d5e6..1564a13 100644 --- a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c +++ b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c@@ -23,6 +23,7 @@ #if IS_ENABLED(CONFIG_FM10K_VXLAN) #include <net/udp_tunnel.h> #include <net/vxlan.h> +#include <net/protocol.h> #endif /* CONFIG_FM10K_VXLAN */ /**@@ -573,7 +574,7 @@ int fm10k_open(struct net_device *netdev) #if IS_ENABLED(CONFIG_FM10K_VXLAN) /* update VXLAN port configuration */ - vxlan_get_rx_port(netdev); + udp_offload_get_port(netdev); #endif fm10k_up(interface);diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c index 520e34e..4be0a26 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_main.c +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c@@ -31,6 +31,7 @@ #include <net/vxlan.h> #endif #include <net/udp_tunnel.h> +#include <net/protocol.h> const char i40e_driver_name[] = "i40e"; static const char i40e_driver_string[] =@@ -5303,9 +5304,7 @@ int i40e_open(struct net_device *netdev) TCP_FLAG_CWR) >> 16); wr32(&pf->hw, I40E_GLLAN_TSOMSK_L, be32_to_cpu(TCP_FLAG_CWR) >> 16); -#ifdef CONFIG_I40E_VXLAN - vxlan_get_rx_port(netdev); -#endif + udp_offload_get_port(netdev); return 0; }diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c index 76ccc77..ba92c7a 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c@@ -52,6 +52,7 @@ #include <scsi/fc/fc_fcoe.h> #include <net/udp_tunnel.h> #include <net/vxlan.h> +#include <net/protocol.h> #ifdef CONFIG_OF #include <linux/of_net.h>@@ -5823,7 +5824,7 @@ static int ixgbe_open(struct net_device *netdev) ixgbe_clear_vxlan_port(adapter); #ifdef CONFIG_IXGBE_VXLAN - vxlan_get_rx_port(netdev); + udp_offload_get_port(netdev); #endif return 0;@@ -6913,7 +6914,7 @@ static void ixgbe_service_task(struct work_struct *work) #ifdef CONFIG_IXGBE_VXLAN if (adapter->flags2 & IXGBE_FLAG2_VXLAN_REREG_NEEDED) { adapter->flags2 &= ~IXGBE_FLAG2_VXLAN_REREG_NEEDED; - vxlan_get_rx_port(adapter->netdev); + udp_offload_get_port(adapter->netdev); } #endif /* CONFIG_IXGBE_VXLAN */ ixgbe_reset_subtask(adapter);diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c index 2cb19c7..b91b8f1 100644 --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c@@ -41,6 +41,7 @@ #include <net/busy_poll.h> #include <net/udp_tunnel.h> #include <net/vxlan.h> +#include <net/protocol.h> #include <linux/mlx4/driver.h> #include <linux/mlx4/device.h>@@ -1684,7 +1685,7 @@ int mlx4_en_start_port(struct net_device *dev) #ifdef CONFIG_MLX4_EN_VXLAN if (priv->mdev->dev->caps.tunnel_offload_mode == MLX4_TUNNEL_OFFLOAD_MODE_VXLAN) - vxlan_get_rx_port(dev); + udp_offload_get_port(dev); #endif priv->port_up = true; netif_tx_start_all_queues(dev);diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c index aa38dbb..a640872 100644 --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c@@ -19,6 +19,7 @@ #ifdef CONFIG_QLCNIC_VXLAN #include <net/udp_tunnel.h> #include <net/vxlan.h> +#include <net/protocol.h> #endif #include "qlcnic.h"@@ -2026,7 +2027,7 @@ qlcnic_attach(struct qlcnic_adapter *adapter) #ifdef CONFIG_QLCNIC_VXLAN if (qlcnic_encap_rx_offload(adapter)) - vxlan_get_rx_port(netdev); + udp_offload_get_port(netdev); #endif adapter->is_up = QLCNIC_ADAPTER_UP_MAGIC;diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c index 5490629..702f9be 100644 --- a/drivers/net/vxlan.c +++ b/drivers/net/vxlan.c@@ -2424,33 +2424,6 @@ static struct device_type vxlan_type = { .name = "vxlan", }; -/* Calls the ndo_add_udp_tunnel_port of the caller in order to - * supply the listening VXLAN udp ports. Callers are expected - * to implement the ndo_add_tunnel_port. - */ -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); - sa_family_t sa_family; - __be16 port; - unsigned int i; - - spin_lock(&vn->sock_lock); - for (i = 0; i < PORT_HASH_SIZE; ++i) { - hlist_for_each_entry_rcu(vs, &vn->sock_list[i], hlist) { - port = inet_sk(vs->sock->sk)->inet_sport; - sa_family = vxlan_get_sk_family(vs); - dev->netdev_ops->ndo_add_udp_tunnel_port(dev, sa_family, - port, - UDP_TUNNEL_VXLAN); - } - } - spin_unlock(&vn->sock_lock); -} -EXPORT_SYMBOL_GPL(vxlan_get_rx_port); - /* Initialize the device structure. */ static void vxlan_setup(struct net_device *dev) {@@ -2639,6 +2612,8 @@ static struct vxlan_sock *vxlan_socket_create(struct net *net, bool ipv6, /* Initialize the vxlan udp offloads structure */ vs->udp_offloads.port = port; + vs->udp_offloads.tunnel_type = UDP_TUNNEL_VXLAN; + vs->udp_offloads.family = ipv6 ? AF_INET6 : AF_INET; vs->udp_offloads.callbacks.gro_receive = vxlan_gro_receive; vs->udp_offloads.callbacks.gro_complete = vxlan_gro_complete;
It seems like you are losing functionality here. The function vxlan_get_sk_family was what was being used to obtain the socket family before and it retrieved it from the socket. If you are going to drop use of the function vxlan_get_sk_family like this you should probably just to through the code and drop all of the references to it since you have duplicated the functionality with the tunnel_type field. It might make sense to make that a seperate patch and to take care of it before you make this change.
quoted hunk ↗ jump to hunk
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index eaecc42..0073009 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h@@ -2075,6 +2075,8 @@ struct udp_offload_callbacks { struct udp_offload { __be16 port; + u8 tunnel_type; + u8 family; u8 ipproto; struct udp_offload_callbacks callbacks; };
You cast family as a u8, but skc_family is an unsigned short. You should probably use either that or sa_family_t.
quoted hunk ↗ jump to hunk
diff --git a/include/net/protocol.h b/include/net/protocol.h index d6fcc1f..738bfc6 100644 --- a/include/net/protocol.h +++ b/include/net/protocol.h@@ -110,6 +110,8 @@ void inet_unregister_protosw(struct inet_protosw *p); int udp_add_offload(struct udp_offload *prot); void udp_del_offload(struct udp_offload *prot); +void udp_offload_get_port(struct net_device *dev); + #if IS_ENABLED(CONFIG_IPV6) int inet6_add_protocol(const struct inet6_protocol *prot, unsigned char num); int inet6_del_protocol(const struct inet6_protocol *prot, unsigned char num);diff --git a/include/net/vxlan.h b/include/net/vxlan.h index c1c899c..926455e 100644 --- a/include/net/vxlan.h +++ b/include/net/vxlan.h@@ -242,14 +242,6 @@ static inline netdev_features_t vxlan_features_check(struct sk_buff *skb, /* IPv6 header + UDP + VXLAN + Ethernet header */ #define VXLAN6_HEADROOM (40 + 8 + 8 + 14) -#if IS_ENABLED(CONFIG_VXLAN) -void vxlan_get_rx_port(struct net_device *netdev); -#else -static inline void vxlan_get_rx_port(struct net_device *netdev) -{ -} -#endif - static inline unsigned short vxlan_get_sk_family(struct vxlan_sock *vs) { return vs->sock->sk->sk_family;diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c index f938616..8597020 100644 --- a/net/ipv4/udp_offload.c +++ b/net/ipv4/udp_offload.c@@ -290,6 +290,33 @@ unlock: } EXPORT_SYMBOL(udp_del_offload); +void udp_offload_get_port(struct net_device *dev) +{ + struct udp_offload_priv __rcu **head; + struct udp_offload_priv *uo_priv; + struct udp_offload *uo; + + if (udp_offload_base) + head = &udp_offload_base; + else + return; + + spin_lock(&udp_offload_lock); + uo_priv = udp_deref_protected(*head); + for (; uo_priv != NULL; uo_priv = udp_deref_protected(*head)) {
You can really simplify all of this by using a while loop here instead of a for loop. Just check for "while (uo_priv)".
+ /* call the right add port */ + uo = uo_priv->offload; + if (uo && dev->netdev_ops->ndo_add_udp_tunnel_port) + dev->netdev_ops->ndo_add_udp_tunnel_port(dev, + uo->family, + uo->port, + uo->tunnel_type); + head = &uo_priv->next;
No need to carry head, it is just dead weight. At this point you could do: uo_priv = udp_deref_protected(uo_priv->next);
+ }
+ spin_unlock(&udp_offload_lock);
+}
+EXPORT_SYMBOL(udp_offload_get_port);
+
struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *skb,
struct udphdr *uh)
{