[PATCH net-next v3 0/8] net: break dependency of drivers on geneve and vxlan

[PATCH net-next v3 0/8] net: break dependency of drivers on geneve and vxlan

[Hannes Frederic Sowa]

Device drivers which support geneve or vxlan offloading have a dependency
on the correlating tunnel kernel modules. Thus those drivers automatically
load the geneve or vxlan modules. Break this dependency with this
small series.

Additionally this series features a review of the respective ->ndo_open
and other functions around vxlan_get_rx_port and geneve_get_rx_port.

* Result:
$ cd drivers/net/ethernet/
$ find . -name '*.ko' | xargs modinfo | egrep  '^depends:.*(vxlan|geneve)'  | wc -l
0

I also incorporated feedback from Jesse Gross to only use one new
netdevice notifiers type, namely NETDEV_REFRESH_OFFLOADS. Otherwise this
series is very much the same as v1.

Hannes Frederic Sowa (8):
  qlcnic: protect qlcnic_82xx_io_slot_reset with rtnl lock
  mlx4: add rtnl lock protection in mlx4_en_restart
  ixgbe: add rtnl locking in service task around vxlan_get_rx_port
  benet: add rtnl lock protection around be_open in be_resume
  fm10k: add rtnl lock protection in fm10k_io_resume
  netdev: add netdevice notifier type to trigger a reprogramming of
    offloads
  vxlan: break dependency to network drivers
  geneve: break dependency to network drivers

 drivers/net/ethernet/emulex/benet/be_main.c      |  9 +++----
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c     |  2 ++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    |  2 ++
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c   |  2 ++
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 10 ++++++--
 drivers/net/geneve.c                             | 30 +++++++++++++++++++++---
 drivers/net/vxlan.c                              | 17 +++++++++-----
 include/linux/netdevice.h                        |  1 +
 include/net/geneve.h                             |  7 +++---
 include/net/vxlan.h                              |  5 +---
 10 files changed, 62 insertions(+), 23 deletions(-)

-- 
2.5.0

[PATCH net-next v3 1/8] qlcnic: protect qlcnic_82xx_io_slot_reset with rtnl lock

[Hannes Frederic Sowa]

Cc: Dept-GELinuxNICDev@qlogic.com
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
index 1205f6f9c94173..1c29105b6c364f 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
@@ -3952,8 +3952,14 @@ static pci_ers_result_t qlcnic_82xx_io_error_detected(struct pci_dev *pdev,
 
 static pci_ers_result_t qlcnic_82xx_io_slot_reset(struct pci_dev *pdev)
 {
-	return qlcnic_attach_func(pdev) ? PCI_ERS_RESULT_DISCONNECT :
-				PCI_ERS_RESULT_RECOVERED;
+	pci_ers_result_t res;
+
+	rtnl_lock();
+	res = qlcnic_attach_func(pdev) ? PCI_ERS_RESULT_DISCONNECT :
+					 PCI_ERS_RESULT_RECOVERED;
+	rtnl_unlock();
+
+	return res;
 }
 
 static void qlcnic_82xx_io_resume(struct pci_dev *pdev)
-- 
2.5.0

[PATCH net-next v3 2/8] mlx4: add rtnl lock protection in mlx4_en_restart

[Hannes Frederic Sowa]

I don't really understand the use of the state_lock. Can't it be simply
replaced by rtnl_lock? It seems a lot of current users depend on
rtnl_lock anyway.

Anyway, fix this up for the moment.

Cc: Eugenia Emantayev <eugenia@mellanox.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 0c7e3f69a73bb6..94abd0843901cf 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -1846,6 +1846,7 @@ static void mlx4_en_restart(struct work_struct *work)
 
 	en_dbg(DRV, priv, "Watchdog task called for port %d\n", priv->port);
 
+	rtnl_lock();
 	mutex_lock(&mdev->state_lock);
 	if (priv->port_up) {
 		mlx4_en_stop_port(dev, 1);
@@ -1853,6 +1854,7 @@ static void mlx4_en_restart(struct work_struct *work)
 			en_err(priv, "Failed restarting port %d\n", priv->port);
 	}
 	mutex_unlock(&mdev->state_lock);
+	rtnl_unlock();
 }
 
 static void mlx4_en_clear_stats(struct net_device *dev)
-- 
2.5.0

[PATCH net-next v3 3/8] ixgbe: add rtnl locking in service task around vxlan_get_rx_port

[Hannes Frederic Sowa]

Protect IXGBE_FLAG2_VXLAN_REREG_NEEDED flag with rtnl_lock as it seems
it could be concurrently modified by ixgbe_set_features and
ixgbe_del_vxlan_port.

Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index ea9537d0e63a9d..8bef07196c804d 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7115,10 +7115,12 @@ static void ixgbe_service_task(struct work_struct *work)
 		return;
 	}
 #ifdef CONFIG_IXGBE_VXLAN
+	rtnl_lock();
 	if (adapter->flags2 & IXGBE_FLAG2_VXLAN_REREG_NEEDED) {
 		adapter->flags2 &= ~IXGBE_FLAG2_VXLAN_REREG_NEEDED;
 		vxlan_get_rx_port(adapter->netdev);
 	}
+	rtnl_unlock();
 #endif /* CONFIG_IXGBE_VXLAN */
 	ixgbe_reset_subtask(adapter);
 	ixgbe_phy_interrupt_subtask(adapter);
-- 
2.5.0

[PATCH net-next v3 4/8] benet: add rtnl lock protection around be_open in be_resume

[Hannes Frederic Sowa]

Cc: Sathya Perla <sathya.perla@avagotech.com>
Cc: Ajit Khaparde <ajit.khaparde@avagotech.com>
Cc: Padmanabh Ratnakar <padmanabh.ratnakar@avagotech.com>
Cc: Sriharsha Basavapatna <sriharsha.basavapatna@avagotech.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/ethernet/emulex/benet/be_main.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index f99de3657ce3b5..3200f48ddd5a68 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -4846,11 +4846,12 @@ static int be_resume(struct be_adapter *adapter)
 	if (status)
 		return status;
 
-	if (netif_running(netdev)) {
+	rtnl_lock();
+	if (netif_running(netdev))
 		status = be_open(netdev);
-		if (status)
-			return status;
-	}
+	rtnl_unlock();
+	if (status)
+		return status;
 
 	netif_device_attach(netdev);
 
-- 
2.5.0

[PATCH net-next v3 5/8] fm10k: add rtnl lock protection in fm10k_io_resume

[Hannes Frederic Sowa]

Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
index 4eb7a6fa6b0ddc..846d9b731c8c14 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
@@ -2350,8 +2350,10 @@ static void fm10k_io_resume(struct pci_dev *pdev)
 	/* reset clock */
 	fm10k_ts_reset(interface);
 
+	rtnl_lock();
 	if (netif_running(netdev))
 		err = fm10k_open(netdev);
+	rtnl_lock();
 
 	/* final check of hardware state before registering the interface */
 	err = err ? : fm10k_hw_ready(interface);
-- 
2.5.0

[PATCH net-next v3 6/8] netdev: add netdevice notifier type to trigger a reprogramming of offloads

[Hannes Frederic Sowa]

Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 include/linux/netdevice.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 8d8e5ca951b493..9750e46760695d 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2183,6 +2183,7 @@ struct netdev_lag_lower_state_info {
 #define NETDEV_BONDING_INFO	0x0019
 #define NETDEV_PRECHANGEUPPER	0x001A
 #define NETDEV_CHANGELOWERSTATE	0x001B
+#define NETDEV_REFRESH_OFFLOADS	0x001C
 
 int register_netdevice_notifier(struct notifier_block *nb);
 int unregister_netdevice_notifier(struct notifier_block *nb);
-- 
2.5.0

[PATCH net-next v3 7/8] vxlan: break dependency to network drivers

[Hannes Frederic Sowa]

Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/vxlan.c | 17 +++++++++++------
 include/net/vxlan.h |  5 +----
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index fecf7b6c732e96..10f1304d58e1ea 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -2468,7 +2468,7 @@ static struct device_type vxlan_type = {
  * supply the listening VXLAN udp ports. Callers are expected
  * to implement the ndo_add_vxlan_port.
  */
-void vxlan_get_rx_port(struct net_device *dev)
+static void vxlan_notify_refresh_netdev(struct net_device *dev)
 {
 	struct vxlan_sock *vs;
 	struct net *net = dev_net(dev);
@@ -2488,7 +2488,6 @@ void vxlan_get_rx_port(struct net_device *dev)
 	}
 	spin_unlock(&vn->sock_lock);
 }
-EXPORT_SYMBOL_GPL(vxlan_get_rx_port);
 
 /* Initialize the device structure. */
 static void vxlan_setup(struct net_device *dev)
@@ -3164,20 +3163,26 @@ static void vxlan_handle_lowerdev_unregister(struct vxlan_net *vn,
 	unregister_netdevice_many(&list_kill);
 }
 
-static int vxlan_lowerdev_event(struct notifier_block *unused,
-				unsigned long event, void *ptr)
+static int vxlan_notifier(struct notifier_block *unused,
+			  unsigned long event, void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 	struct vxlan_net *vn = net_generic(dev_net(dev), vxlan_net_id);
 
-	if (event == NETDEV_UNREGISTER)
+	switch (event) {
+	case NETDEV_REFRESH_OFFLOADS:
+		vxlan_notify_refresh_netdev(dev);
+		break;
+	case NETDEV_UNREGISTER:
 		vxlan_handle_lowerdev_unregister(vn, dev);
+		break;
+	}
 
 	return NOTIFY_DONE;
 }
 
 static struct notifier_block vxlan_notifier_block __read_mostly = {
-	.notifier_call = vxlan_lowerdev_event,
+	.notifier_call = vxlan_notifier,
 };
 
 static __net_init int vxlan_init_net(struct net *net)
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index 0fb86442544b26..48d0450160c91f 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -242,13 +242,10 @@ 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)
 {
+	call_netdevice_notifiers(NETDEV_REFRESH_OFFLOADS, netdev);
 }
-#endif
 
 static inline unsigned short vxlan_get_sk_family(struct vxlan_sock *vs)
 {
-- 
2.5.0

[PATCH net-next v3 8/8] geneve: break dependency to network drivers

[Hannes Frederic Sowa]

Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/geneve.c | 30 +++++++++++++++++++++++++++---
 include/net/geneve.h |  7 +++----
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 24b077a32c1c9c..5b8d728b1204be 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -1110,7 +1110,7 @@ static struct device_type geneve_type = {
  * supply the listening GENEVE udp ports. Callers are expected
  * to implement the ndo_add_geneve_port.
  */
-void geneve_get_rx_port(struct net_device *dev)
+static void geneve_notify_refresh_netdev(struct net_device *dev)
 {
 	struct net *net = dev_net(dev);
 	struct geneve_net *gn = net_generic(net, geneve_net_id);
@@ -1128,7 +1128,6 @@ void geneve_get_rx_port(struct net_device *dev)
 	}
 	rcu_read_unlock();
 }
-EXPORT_SYMBOL_GPL(geneve_get_rx_port);
 
 /* Initialize the device structure. */
 static void geneve_setup(struct net_device *dev)
@@ -1450,6 +1449,24 @@ struct net_device *geneve_dev_create_fb(struct net *net, const char *name,
 }
 EXPORT_SYMBOL_GPL(geneve_dev_create_fb);
 
+static int geneve_notifier(struct notifier_block *unused,
+			   unsigned long event, void *ptr)
+{
+	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+
+	switch (event) {
+	case NETDEV_REFRESH_OFFLOADS:
+		geneve_notify_refresh_netdev(dev);
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block geneve_notifier_block __read_mostly = {
+	.notifier_call = geneve_notifier,
+};
+
 static __net_init int geneve_init_net(struct net *net)
 {
 	struct geneve_net *gn = net_generic(net, geneve_net_id);
@@ -1502,11 +1519,17 @@ static int __init geneve_init_module(void)
 	if (rc)
 		goto out1;
 
-	rc = rtnl_link_register(&geneve_link_ops);
+	rc = register_netdevice_notifier(&geneve_notifier_block);
 	if (rc)
 		goto out2;
 
+	rc = rtnl_link_register(&geneve_link_ops);
+	if (rc)
+		goto out3;
+
 	return 0;
+out3:
+	unregister_netdevice_notifier(&geneve_notifier_block);
 out2:
 	unregister_pernet_subsys(&geneve_net_ops);
 out1:
@@ -1517,6 +1540,7 @@ late_initcall(geneve_init_module);
 static void __exit geneve_cleanup_module(void)
 {
 	rtnl_link_unregister(&geneve_link_ops);
+	unregister_netdevice_notifier(&geneve_notifier_block);
 	unregister_pernet_subsys(&geneve_net_ops);
 }
 module_exit(geneve_cleanup_module);
diff --git a/include/net/geneve.h b/include/net/geneve.h
index e6c23dc765f7ec..7d52077b72faa3 100644
--- a/include/net/geneve.h
+++ b/include/net/geneve.h
@@ -1,6 +1,8 @@
 #ifndef __NET_GENEVE_H
 #define __NET_GENEVE_H  1
 
+#include <linux/netdevice.h>
+
 #ifdef CONFIG_INET
 #include <net/udp_tunnel.h>
 #endif
@@ -62,13 +64,10 @@ struct genevehdr {
 	struct geneve_opt options[];
 };
 
-#if IS_ENABLED(CONFIG_GENEVE)
-void geneve_get_rx_port(struct net_device *netdev);
-#else
 static inline void geneve_get_rx_port(struct net_device *netdev)
 {
+	call_netdevice_notifiers(NETDEV_REFRESH_OFFLOADS, netdev);
 }
-#endif
 
 #ifdef CONFIG_INET
 struct net_device *geneve_dev_create_fb(struct net *net, const char *name,
-- 
2.5.0

Re: [PATCH net-next v3 8/8] geneve: break dependency to network drivers

[Jesse Gross]

On Wed, Jan 6, 2016 at 3:39 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> ---
>  drivers/net/geneve.c | 30 +++++++++++++++++++++++++++---
>  include/net/geneve.h |  7 +++----
>  2 files changed, 30 insertions(+), 7 deletions(-)

Thanks a lot for going through all the drivers! You found more things
than I thought. I noticed just a couple new things with this version
but overall I think it is a good direction.

> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index 24b077a32c1c9c..5b8d728b1204be 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> @@ -1110,7 +1110,7 @@ static struct device_type geneve_type = {
>   * supply the listening GENEVE udp ports. Callers are expected
>   * to implement the ndo_add_geneve_port.
>   */
> -void geneve_get_rx_port(struct net_device *dev)
> +static void geneve_notify_refresh_netdev(struct net_device *dev)
>  {
>         struct net *net = dev_net(dev);
>         struct geneve_net *gn = net_generic(net, geneve_net_id);

Both this function and the corresponding one in VXLAN assume that the
add port NDO is implemented and blindly dereference the pointer. Now
that all protocols get refreshed at the same time, we should check
first. There's also a comment above the function that says this, which
we can remove now.

I also think we can update the comments in netdevice.h for the
VXLAN/Geneve add/remove NDOs to say that they are protected by RTNL.

> diff --git a/include/net/geneve.h b/include/net/geneve.h
> index e6c23dc765f7ec..7d52077b72faa3 100644
> --- a/include/net/geneve.h
> +++ b/include/net/geneve.h
> -#if IS_ENABLED(CONFIG_GENEVE)
> -void geneve_get_rx_port(struct net_device *netdev);
> -#else
>  static inline void geneve_get_rx_port(struct net_device *netdev)
>  {
> +       call_netdevice_notifiers(NETDEV_REFRESH_OFFLOADS, netdev);
>  }
> -#endif

I think we should merge vxlan_get_rx_port() and geneve_get_rx_port()
into a single generic function. For drivers that support both, they
now have two calls to get notified for all offloaded ports. This
actually can cause problems related to duplicates, similar to what you
feared before.

Re: [PATCH net-next v3 8/8] geneve: break dependency to network drivers

[Hannes Frederic Sowa]

Hi Jesse,

On 07.01.2016 01:18, Jesse Gross wrote:
> On Wed, Jan 6, 2016 at 3:39 PM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
>> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>> ---
>>   drivers/net/geneve.c | 30 +++++++++++++++++++++++++++---
>>   include/net/geneve.h |  7 +++----
>>   2 files changed, 30 insertions(+), 7 deletions(-)
>
> Thanks a lot for going through all the drivers! You found more things
> than I thought. I noticed just a couple new things with this version
> but overall I think it is a good direction.
>
>> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
>> index 24b077a32c1c9c..5b8d728b1204be 100644
>> --- a/drivers/net/geneve.c
>> +++ b/drivers/net/geneve.c
>> @@ -1110,7 +1110,7 @@ static struct device_type geneve_type = {
>>    * supply the listening GENEVE udp ports. Callers are expected
>>    * to implement the ndo_add_geneve_port.
>>    */
>> -void geneve_get_rx_port(struct net_device *dev)
>> +static void geneve_notify_refresh_netdev(struct net_device *dev)
>>   {
>>          struct net *net = dev_net(dev);
>>          struct geneve_net *gn = net_generic(net, geneve_net_id);
>
> Both this function and the corresponding one in VXLAN assume that the
> add port NDO is implemented and blindly dereference the pointer. Now
> that all protocols get refreshed at the same time, we should check
> first. There's also a comment above the function that says this, which
> we can remove now.

Absolutely, will fix this in the next version. With the separate 
offloading types this wouldn't be possible. Thanks for seeing this!

I actually had this in a follow-up patch because I want to call this on 
NETDEV_REGISTER, too, so we don't need to add the geneve/vxlan functions 
to the drivers in ndo_start anymore.

> I also think we can update the comments in netdevice.h for the
> VXLAN/Geneve add/remove NDOs to say that they are protected by RTNL.

Will do. Thanks!

>> diff --git a/include/net/geneve.h b/include/net/geneve.h
>> index e6c23dc765f7ec..7d52077b72faa3 100644
>> --- a/include/net/geneve.h
>> +++ b/include/net/geneve.h
>> -#if IS_ENABLED(CONFIG_GENEVE)
>> -void geneve_get_rx_port(struct net_device *netdev);
>> -#else
>>   static inline void geneve_get_rx_port(struct net_device *netdev)
>>   {
>> +       call_netdevice_notifiers(NETDEV_REFRESH_OFFLOADS, netdev);
>>   }
>> -#endif
>
> I think we should merge vxlan_get_rx_port() and geneve_get_rx_port()
> into a single generic function. For drivers that support both, they
> now have two calls to get notified for all offloaded ports. This
> actually can cause problems related to duplicates, similar to what you
> feared before.

Agreed. I add comments to the offloading functions in netdevice.h so 
they need to be implemented in a idempotent way and review the drivers 
how they deal with it.

Thanks for the review,
Hannes

Re: [PATCH net-next v3 8/8] geneve: break dependency to network drivers

[Hannes Frederic Sowa]

On 07.01.2016 01:18, Jesse Gross wrote:
> I think we should merge vxlan_get_rx_port() and geneve_get_rx_port()
> into a single generic function. For drivers that support both, they
> now have two calls to get notified for all offloaded ports. This
> actually can cause problems related to duplicates, similar to what you
> feared before.

Checking out the ndo_add_vxlan_port callbacks, we cannot change how we 
refresh the ports unfortunately. For some drivers it might be easy to 
update, some look a little bit more complicated.

I guess we should try to keep more complexity in the core and make sure 
we only call those functions if necessary for the particular type of 
offload than to replicate the logic in each driver.

If you agree I prepare this series with the changes to have separate 
callbacks, address your other feedback and send it out.

Thanks,
Hannes

Re: [PATCH net-next v3 8/8] geneve: break dependency to network drivers

[Jesse Gross]

On Fri, Jan 8, 2016 at 12:47 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> On 07.01.2016 01:18, Jesse Gross wrote:
>>
>> I think we should merge vxlan_get_rx_port() and geneve_get_rx_port()
>> into a single generic function. For drivers that support both, they
>> now have two calls to get notified for all offloaded ports. This
>> actually can cause problems related to duplicates, similar to what you
>> feared before.
>
>
> Checking out the ndo_add_vxlan_port callbacks, we cannot change how we
> refresh the ports unfortunately. For some drivers it might be easy to
> update, some look a little bit more complicated.
>
> I guess we should try to keep more complexity in the core and make sure we
> only call those functions if necessary for the particular type of offload
> than to replicate the logic in each driver.
>
> If you agree I prepare this series with the changes to have separate
> callbacks, address your other feedback and send it out.

I'm not sure that I totally understand, do you have an example of a
problematic driver?

I think what I was proposing is fairly simple, so maybe I wasn't
totally clear. Here's what I think we can do:

In your most recent series, vxlan_get_rx_port() and
geneve_get_rx_port() have the same implementation. I think we can
merge them into a single function (i.e. udp_tunnel_get_rx_port()). For
all of the drivers that only implement VXLAN offloads, this is just a
1:1 switch so there should be no functionality change. There's only
one driver than currently implements both (i40e) and it calls both
vxlan_get_rx_port() and geneve_get_rx_port() back to back. We can just
collapse this into a single call and since that will trigger both
offloads to refresh, the behavior should again be the same as we have.

I do know that there are so drivers that try to do clever things as
far as refcounting, etc. However, since the callbacks are the same as
we did before, it shouldn't introduce any issues.

Re: [PATCH net-next v3 8/8] geneve: break dependency to network drivers

[Hannes Frederic Sowa]

On 08.01.2016 22:12, Jesse Gross wrote:
> On Fri, Jan 8, 2016 at 12:47 PM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
>> On 07.01.2016 01:18, Jesse Gross wrote:
>>>
>>> I think we should merge vxlan_get_rx_port() and geneve_get_rx_port()
>>> into a single generic function. For drivers that support both, they
>>> now have two calls to get notified for all offloaded ports. This
>>> actually can cause problems related to duplicates, similar to what you
>>> feared before.
>>
>>
>> Checking out the ndo_add_vxlan_port callbacks, we cannot change how we
>> refresh the ports unfortunately. For some drivers it might be easy to
>> update, some look a little bit more complicated.
>>
>> I guess we should try to keep more complexity in the core and make sure we
>> only call those functions if necessary for the particular type of offload
>> than to replicate the logic in each driver.
>>
>> If you agree I prepare this series with the changes to have separate
>> callbacks, address your other feedback and send it out.
>
> I'm not sure that I totally understand, do you have an example of a
> problematic driver?

bnx2x e.g. does refcounting on the port. So when it will implement 
geneve offloading, it will refcount the vxlan port multiple times with 
the current schema. Currently this is safe, as bnx2x doesn't care about 
geneve and thus no further callbacks will happen. But I fear drivers 
being broken as soon as they implement geneve additionally with the 
current schema.

> I think what I was proposing is fairly simple, so maybe I wasn't
> totally clear. Here's what I think we can do:
>
> In your most recent series, vxlan_get_rx_port() and
> geneve_get_rx_port() have the same implementation. I think we can
> merge them into a single function (i.e. udp_tunnel_get_rx_port()). For
> all of the drivers that only implement VXLAN offloads, this is just a
> 1:1 switch so there should be no functionality change. There's only
> one driver than currently implements both (i40e) and it calls both
> vxlan_get_rx_port() and geneve_get_rx_port() back to back. We can just
> collapse this into a single call and since that will trigger both
> offloads to refresh, the behavior should again be the same as we have.

I totally understood that. I am just fearing future additions of offload 
while not updating the current vxlan offloading then as well.

My current series ATM implements just what you proposed.

> I do know that there are so drivers that try to do clever things as
> far as refcounting, etc. However, since the callbacks are the same as
> we did before, it shouldn't introduce any issues.

Agreed. I can push the series out now as you proposed and we have to 
intercept all upcoming driver changes in regard to geneve offloading and 
review them. :)

Bye,
Hannes

Re: [PATCH net-next v3 8/8] geneve: break dependency to network drivers

[Jesse Gross]

On Fri, Jan 8, 2016 at 1:18 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> On 08.01.2016 22:12, Jesse Gross wrote:
>>
>> On Fri, Jan 8, 2016 at 12:47 PM, Hannes Frederic Sowa
>> <hannes@stressinduktion.org> wrote:
>>>
>>> On 07.01.2016 01:18, Jesse Gross wrote:
>>>>
>>>>
>>>> I think we should merge vxlan_get_rx_port() and geneve_get_rx_port()
>>>> into a single generic function. For drivers that support both, they
>>>> now have two calls to get notified for all offloaded ports. This
>>>> actually can cause problems related to duplicates, similar to what you
>>>> feared before.
>>>
>>>
>>>
>>> Checking out the ndo_add_vxlan_port callbacks, we cannot change how we
>>> refresh the ports unfortunately. For some drivers it might be easy to
>>> update, some look a little bit more complicated.
>>>
>>> I guess we should try to keep more complexity in the core and make sure
>>> we
>>> only call those functions if necessary for the particular type of offload
>>> than to replicate the logic in each driver.
>>>
>>> If you agree I prepare this series with the changes to have separate
>>> callbacks, address your other feedback and send it out.
>>
>>
>> I'm not sure that I totally understand, do you have an example of a
>> problematic driver?
>
>
> bnx2x e.g. does refcounting on the port. So when it will implement geneve
> offloading, it will refcount the vxlan port multiple times with the current
> schema. Currently this is safe, as bnx2x doesn't care about geneve and thus
> no further callbacks will happen. But I fear drivers being broken as soon as
> they implement geneve additionally with the current schema.

OK, I understand what you mean now. I guess having a combined callback
function still seems cleaner to me overall and the exceptions are
relatively few, so I would probably continue to lean towards this
direction. In any case, I think it's hard for the core network stack
to completely handle the complexity as drivers can have various
restrictions on the number of ports, etc. that they support for
offloading.