[PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers

[PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers

[Eric Dumazet]

As diagnosed by Song Liu, ndo_poll_controller() can
be very dangerous on loaded hosts, since the cpu
calling ndo_poll_controller() might steal all NAPI
contexts (for all RX/TX queues of the NIC).

This capture, showing one ksoftirqd eating all cycles
can last for unlimited amount of time, since one
cpu is generally not able to drain all the queues under load.

It seems that all networking drivers that do use NAPI
for their TX completions, should not provide a ndo_poll_controller() :

Most NAPI drivers have netpoll support already handled
in core networking stack, since netpoll_poll_dev()
uses poll_napi(dev) to iterate through registered
NAPI contexts for a device.

This patch series take care of the first round, we will
handle other drivers in future rounds.

Eric Dumazet (15):
  netpoll: make ndo_poll_controller() optional
  bonding: use netpoll_poll_dev() helper
  ixgbe: remove ndo_poll_controller
  ixgbevf: remove ndo_poll_controller
  fm10k: remove ndo_poll_controller
  ixgb: remove ndo_poll_controller
  igb: remove ndo_poll_controller
  ice: remove ndo_poll_controller
  i40evf: remove ndo_poll_controller
  mlx4: remove ndo_poll_controller
  mlx5: remove ndo_poll_controller
  bnx2x: remove ndo_poll_controller
  bnxt: remove ndo_poll_controller
  nfp: remove ndo_poll_controller
  tun: remove ndo_poll_controller

 drivers/net/bonding/bond_main.c               | 11 +----
 .../net/ethernet/broadcom/bnx2x/bnx2x_main.c  | 16 -------
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 18 --------
 drivers/net/ethernet/intel/fm10k/fm10k.h      |  3 --
 .../net/ethernet/intel/fm10k/fm10k_netdev.c   |  3 --
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c  | 22 ----------
 .../net/ethernet/intel/i40evf/i40evf_main.c   | 26 -----------
 drivers/net/ethernet/intel/ice/ice_main.c     | 27 ------------
 drivers/net/ethernet/intel/igb/igb_main.c     | 30 -------------
 drivers/net/ethernet/intel/ixgb/ixgb_main.c   | 25 -----------
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 25 -----------
 .../net/ethernet/intel/ixgbevf/ixgbevf_main.c | 21 ---------
 .../net/ethernet/mellanox/mlx4/en_netdev.c    | 20 ---------
 .../net/ethernet/mellanox/mlx5/core/en_main.c | 19 --------
 .../ethernet/netronome/nfp/nfp_net_common.c   | 18 --------
 drivers/net/tun.c                             | 43 -------------------
 include/linux/netpoll.h                       |  5 ++-
 net/core/netpoll.c                            | 19 +++-----
 18 files changed, 12 insertions(+), 339 deletions(-)

-- 
2.19.0.444.g18242da7ef-goog

[PATCH net 01/15] netpoll: make ndo_poll_controller() optional

[Eric Dumazet]

As diagnosed by Song Liu, ndo_poll_controller() can
be very dangerous on loaded hosts, since the cpu
calling ndo_poll_controller() might steal all NAPI
contexts (for all RX/TX queues of the NIC). This capture
can last for unlimited amount of time, since one
cpu is generally not able to drain all the queues under load.

It seems that all networking drivers that do use NAPI
for their TX completions, should not provide a ndo_poll_controller().

NAPI drivers have netpoll support already handled
in core networking stack, since netpoll_poll_dev()
uses poll_napi(dev) to iterate through registered
NAPI contexts for a device.

This patch allows netpoll_poll_dev() to process NAPI
contexts even for drivers not providing ndo_poll_controller(),
allowing for following patches in NAPI drivers.

Also we export netpoll_poll_dev() so that it can be called
by bonding/team drivers in following patches.

Reported-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Tested-by: Song Liu <songliubraving@fb.com>
---
 include/linux/netpoll.h |  5 +++--
 net/core/netpoll.c      | 19 +++++++------------
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
index 67662d01130a7b4f69c8b4ec176f59b11e8ae33d..3ef82d3a78db51d958858470f8158f72cfd74fb3 100644
--- a/include/linux/netpoll.h
+++ b/include/linux/netpoll.h
@@ -49,8 +49,9 @@ struct netpoll_info {
 };
 
 #ifdef CONFIG_NETPOLL
-extern void netpoll_poll_disable(struct net_device *dev);
-extern void netpoll_poll_enable(struct net_device *dev);
+void netpoll_poll_dev(struct net_device *dev);
+void netpoll_poll_disable(struct net_device *dev);
+void netpoll_poll_enable(struct net_device *dev);
 #else
 static inline void netpoll_poll_disable(struct net_device *dev) { return; }
 static inline void netpoll_poll_enable(struct net_device *dev) { return; }
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 57557a6a950cc9cdff959391576a03381d328c1a..3219a2932463096566ce8ff336ecdf699422dd65 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -187,16 +187,16 @@ static void poll_napi(struct net_device *dev)
 	}
 }
 
-static void netpoll_poll_dev(struct net_device *dev)
+void netpoll_poll_dev(struct net_device *dev)
 {
-	const struct net_device_ops *ops;
 	struct netpoll_info *ni = rcu_dereference_bh(dev->npinfo);
+	const struct net_device_ops *ops;
 
 	/* Don't do any rx activity if the dev_lock mutex is held
 	 * the dev_open/close paths use this to block netpoll activity
 	 * while changing device state
 	 */
-	if (down_trylock(&ni->dev_lock))
+	if (!ni || down_trylock(&ni->dev_lock))
 		return;
 
 	if (!netif_running(dev)) {
@@ -205,13 +205,8 @@ static void netpoll_poll_dev(struct net_device *dev)
 	}
 
 	ops = dev->netdev_ops;
-	if (!ops->ndo_poll_controller) {
-		up(&ni->dev_lock);
-		return;
-	}
-
-	/* Process pending work on NIC */
-	ops->ndo_poll_controller(dev);
+	if (ops->ndo_poll_controller)
+		ops->ndo_poll_controller(dev);
 
 	poll_napi(dev);
 
@@ -219,6 +214,7 @@ static void netpoll_poll_dev(struct net_device *dev)
 
 	zap_completion_queue();
 }
+EXPORT_SYMBOL(netpoll_poll_dev);
 
 void netpoll_poll_disable(struct net_device *dev)
 {
@@ -613,8 +609,7 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 	strlcpy(np->dev_name, ndev->name, IFNAMSIZ);
 	INIT_WORK(&np->cleanup_work, netpoll_async_cleanup);
 
-	if ((ndev->priv_flags & IFF_DISABLE_NETPOLL) ||
-	    !ndev->netdev_ops->ndo_poll_controller) {
+	if (ndev->priv_flags & IFF_DISABLE_NETPOLL) {
 		np_err(np, "%s doesn't support polling, aborting\n",
 		       np->dev_name);
 		err = -ENOTSUPP;
-- 
2.19.0.444.g18242da7ef-goog

[PATCH net 02/15] bonding: use netpoll_poll_dev() helper

[Eric Dumazet]

We want to allow NAPI drivers to no longer provide
ndo_poll_controller() method, as it has been proven problematic.

team driver must not look at its presence, but instead call
netpoll_poll_dev() which factorize the needed actions.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Jay Vosburgh <j.vosburgh@gmail.com>
Cc: Veaceslav Falico <vfalico@gmail.com>
Cc: Andy Gospodarek <andy@greyhouse.net>
---
 drivers/net/bonding/bond_main.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index a764a83f99dabe54585dbad7dba40b6601177c03..0d87e11e7f1d84537fe43d95249b1bd3a2ce291d 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -971,16 +971,13 @@ static void bond_poll_controller(struct net_device *bond_dev)
 	struct slave *slave = NULL;
 	struct list_head *iter;
 	struct ad_info ad_info;
-	struct netpoll_info *ni;
-	const struct net_device_ops *ops;
 
 	if (BOND_MODE(bond) == BOND_MODE_8023AD)
 		if (bond_3ad_get_active_agg_info(bond, &ad_info))
 			return;
 
 	bond_for_each_slave_rcu(bond, slave, iter) {
-		ops = slave->dev->netdev_ops;
-		if (!bond_slave_is_up(slave) || !ops->ndo_poll_controller)
+		if (!bond_slave_is_up(slave))
 			continue;
 
 		if (BOND_MODE(bond) == BOND_MODE_8023AD) {
@@ -992,11 +989,7 @@ static void bond_poll_controller(struct net_device *bond_dev)
 				continue;
 		}
 
-		ni = rcu_dereference_bh(slave->dev->npinfo);
-		if (down_trylock(&ni->dev_lock))
-			continue;
-		ops->ndo_poll_controller(slave->dev);
-		up(&ni->dev_lock);
+		netpoll_poll_dev(slave->dev);
 	}
 }
 
-- 
2.19.0.444.g18242da7ef-goog

Re: [PATCH net 02/15] bonding: use netpoll_poll_dev() helper

[Jay Vosburgh]

Eric Dumazet <edumazet@google.com> wrote:

>We want to allow NAPI drivers to no longer provide
>ndo_poll_controller() method, as it has been proven problematic.
>
>team driver must not look at its presence, but instead call
>netpoll_poll_dev() which factorize the needed actions.

	This patch is for bonding, not team; otherwise LGTM.

Acked-by: Jay Vosburgh <jay.vosburgh@canonical.com>

	-J


>Signed-off-by: Eric Dumazet <edumazet@google.com>
>Cc: Jay Vosburgh <j.vosburgh@gmail.com>
>Cc: Veaceslav Falico <vfalico@gmail.com>
>Cc: Andy Gospodarek <andy@greyhouse.net>
>---
> drivers/net/bonding/bond_main.c | 11 ++---------
> 1 file changed, 2 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index a764a83f99dabe54585dbad7dba40b6601177c03..0d87e11e7f1d84537fe43d95249b1bd3a2ce291d 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -971,16 +971,13 @@ static void bond_poll_controller(struct net_device *bond_dev)
> 	struct slave *slave = NULL;
> 	struct list_head *iter;
> 	struct ad_info ad_info;
>-	struct netpoll_info *ni;
>-	const struct net_device_ops *ops;
> 
> 	if (BOND_MODE(bond) == BOND_MODE_8023AD)
> 		if (bond_3ad_get_active_agg_info(bond, &ad_info))
> 			return;
> 
> 	bond_for_each_slave_rcu(bond, slave, iter) {
>-		ops = slave->dev->netdev_ops;
>-		if (!bond_slave_is_up(slave) || !ops->ndo_poll_controller)
>+		if (!bond_slave_is_up(slave))
> 			continue;
> 
> 		if (BOND_MODE(bond) == BOND_MODE_8023AD) {
>@@ -992,11 +989,7 @@ static void bond_poll_controller(struct net_device *bond_dev)
> 				continue;
> 		}
> 
>-		ni = rcu_dereference_bh(slave->dev->npinfo);
>-		if (down_trylock(&ni->dev_lock))
>-			continue;
>-		ops->ndo_poll_controller(slave->dev);
>-		up(&ni->dev_lock);
>+		netpoll_poll_dev(slave->dev);
> 	}
> }
> 
>-- 
>2.19.0.444.g18242da7ef-goog
>

[PATCH net 03/15] ixgbe: remove ndo_poll_controller

[Eric Dumazet]

As diagnosed by Song Liu, ndo_poll_controller() can
be very dangerous on loaded hosts, since the cpu
calling ndo_poll_controller() might steal all NAPI
contexts (for all RX/TX queues of the NIC). This capture
can last for unlimited amount of time, since one
cpu is generally not able to drain all the queues under load.

ixgbe uses NAPI for TX completions, so we better let core
networking stack call the napi->poll() to avoid the capture.

Reported-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Tested-by: Song Liu <songliubraving@fb.com>
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 25 -------------------
 1 file changed, 25 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 9a23d33a47ed52bfeb10d79d970e114ee4702d6e..f27d73a7bf16f084ea8f4ab12d905b031f514cf8 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -8768,28 +8768,6 @@ static int ixgbe_del_sanmac_netdev(struct net_device *dev)
 	return err;
 }
 
-#ifdef CONFIG_NET_POLL_CONTROLLER
-/*
- * Polling 'interrupt' - used by things like netconsole to send skbs
- * without having to re-enable interrupts. It's not called while
- * the interrupt routine is executing.
- */
-static void ixgbe_netpoll(struct net_device *netdev)
-{
-	struct ixgbe_adapter *adapter = netdev_priv(netdev);
-	int i;
-
-	/* if interface is down do nothing */
-	if (test_bit(__IXGBE_DOWN, &adapter->state))
-		return;
-
-	/* loop through and schedule all active queues */
-	for (i = 0; i < adapter->num_q_vectors; i++)
-		ixgbe_msix_clean_rings(0, adapter->q_vector[i]);
-}
-
-#endif
-
 static void ixgbe_get_ring_stats64(struct rtnl_link_stats64 *stats,
 				   struct ixgbe_ring *ring)
 {
@@ -10251,9 +10229,6 @@ static const struct net_device_ops ixgbe_netdev_ops = {
 	.ndo_get_vf_config	= ixgbe_ndo_get_vf_config,
 	.ndo_get_stats64	= ixgbe_get_stats64,
 	.ndo_setup_tc		= __ixgbe_setup_tc,
-#ifdef CONFIG_NET_POLL_CONTROLLER
-	.ndo_poll_controller	= ixgbe_netpoll,
-#endif
 #ifdef IXGBE_FCOE
 	.ndo_select_queue	= ixgbe_select_queue,
 	.ndo_fcoe_ddp_setup = ixgbe_fcoe_ddp_get,
-- 
2.19.0.444.g18242da7ef-goog

[PATCH net 04/15] ixgbevf: remove ndo_poll_controller

[Eric Dumazet]

As diagnosed by Song Liu, ndo_poll_controller() can
be very dangerous on loaded hosts, since the cpu
calling ndo_poll_controller() might steal all NAPI
contexts (for all RX/TX queues of the NIC). This capture
can last for unlimited amount of time, since one
cpu is generally not able to drain all the queues under load.

ixgbevf uses NAPI for TX completions, so we better let core
networking stack call the napi->poll() to avoid the capture.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 .../net/ethernet/intel/ixgbevf/ixgbevf_main.c | 21 -------------------
 1 file changed, 21 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index d86446d202d5ed95826db225139b095d7c7a683c..5a228582423b74726de2bd040a388eb8e59df764 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -4233,24 +4233,6 @@ static int ixgbevf_change_mtu(struct net_device *netdev, int new_mtu)
 	return 0;
 }
 
-#ifdef CONFIG_NET_POLL_CONTROLLER
-/* Polling 'interrupt' - used by things like netconsole to send skbs
- * without having to re-enable interrupts. It's not called while
- * the interrupt routine is executing.
- */
-static void ixgbevf_netpoll(struct net_device *netdev)
-{
-	struct ixgbevf_adapter *adapter = netdev_priv(netdev);
-	int i;
-
-	/* if interface is down do nothing */
-	if (test_bit(__IXGBEVF_DOWN, &adapter->state))
-		return;
-	for (i = 0; i < adapter->num_rx_queues; i++)
-		ixgbevf_msix_clean_rings(0, adapter->q_vector[i]);
-}
-#endif /* CONFIG_NET_POLL_CONTROLLER */
-
 static int ixgbevf_suspend(struct pci_dev *pdev, pm_message_t state)
 {
 	struct net_device *netdev = pci_get_drvdata(pdev);
@@ -4482,9 +4464,6 @@ static const struct net_device_ops ixgbevf_netdev_ops = {
 	.ndo_tx_timeout		= ixgbevf_tx_timeout,
 	.ndo_vlan_rx_add_vid	= ixgbevf_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid	= ixgbevf_vlan_rx_kill_vid,
-#ifdef CONFIG_NET_POLL_CONTROLLER
-	.ndo_poll_controller	= ixgbevf_netpoll,
-#endif
 	.ndo_features_check	= ixgbevf_features_check,
 	.ndo_bpf		= ixgbevf_xdp,
 };
-- 
2.19.0.444.g18242da7ef-goog

[PATCH net 05/15] fm10k: remove ndo_poll_controller

[Eric Dumazet]

As diagnosed by Song Liu, ndo_poll_controller() can
be very dangerous on loaded hosts, since the cpu
calling ndo_poll_controller() might steal all NAPI
contexts (for all RX/TX queues of the NIC). This capture
lasts for unlimited amount of time, since one
cpu is generally not able to drain all the queues under load.

fm10k uses NAPI for TX completions, so we better let core
networking stack call the napi->poll() to avoid the capture.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/fm10k/fm10k.h      |  3 ---
 .../net/ethernet/intel/fm10k/fm10k_netdev.c   |  3 ---
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c  | 22 -------------------
 3 files changed, 28 deletions(-)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k.h b/drivers/net/ethernet/intel/fm10k/fm10k.h
index a903a0ba45e180c39e99c6b6a0726291203b1f91..7d42582ed48dc36f3da8b6b670d1c4f8847ffa9b 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k.h
+++ b/drivers/net/ethernet/intel/fm10k/fm10k.h
@@ -504,9 +504,6 @@ void fm10k_update_stats(struct fm10k_intfc *interface);
 void fm10k_service_event_schedule(struct fm10k_intfc *interface);
 void fm10k_macvlan_schedule(struct fm10k_intfc *interface);
 void fm10k_update_rx_drop_en(struct fm10k_intfc *interface);
-#ifdef CONFIG_NET_POLL_CONTROLLER
-void fm10k_netpoll(struct net_device *netdev);
-#endif
 
 /* Netdev */
 struct net_device *fm10k_alloc_netdev(const struct fm10k_info *info);
diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
index 929f538d28bc03a391340ebf6cc531b03f92b7a3..538a8467f43413f97bbf477053ba8e93560e2bcf 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
@@ -1648,9 +1648,6 @@ static const struct net_device_ops fm10k_netdev_ops = {
 	.ndo_udp_tunnel_del	= fm10k_udp_tunnel_del,
 	.ndo_dfwd_add_station	= fm10k_dfwd_add_station,
 	.ndo_dfwd_del_station	= fm10k_dfwd_del_station,
-#ifdef CONFIG_NET_POLL_CONTROLLER
-	.ndo_poll_controller	= fm10k_netpoll,
-#endif
 	.ndo_features_check	= fm10k_features_check,
 };
 
diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
index 15071e4adb98c9924ab2e9a17c7f616f23104bd0..c859ababeed50e030baf7cb7041fbd20916c2265 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
@@ -1210,28 +1210,6 @@ static irqreturn_t fm10k_msix_mbx_vf(int __always_unused irq, void *data)
 	return IRQ_HANDLED;
 }
 
-#ifdef CONFIG_NET_POLL_CONTROLLER
-/**
- *  fm10k_netpoll - A Polling 'interrupt' handler
- *  @netdev: network interface device structure
- *
- *  This is used by netconsole to send skbs without having to re-enable
- *  interrupts. It's not called while the normal interrupt routine is executing.
- **/
-void fm10k_netpoll(struct net_device *netdev)
-{
-	struct fm10k_intfc *interface = netdev_priv(netdev);
-	int i;
-
-	/* if interface is down do nothing */
-	if (test_bit(__FM10K_DOWN, interface->state))
-		return;
-
-	for (i = 0; i < interface->num_q_vectors; i++)
-		fm10k_msix_clean_rings(0, interface->q_vector[i]);
-}
-
-#endif
 #define FM10K_ERR_MSG(type) case (type): error = #type; break
 static void fm10k_handle_fault(struct fm10k_intfc *interface, int type,
 			       struct fm10k_fault *fault)
-- 
2.19.0.444.g18242da7ef-goog

[PATCH net 06/15] ixgb: remove ndo_poll_controller

[Eric Dumazet]

As diagnosed by Song Liu, ndo_poll_controller() can
be very dangerous on loaded hosts, since the cpu
calling ndo_poll_controller() might steal all NAPI
contexts (for all RX/TX queues of the NIC). This capture
can last for unlimited amount of time, since one
cpu is generally not able to drain all the queues under load.

ixgb uses NAPI for TX completions, so we better let core
networking stack call the napi->poll() to avoid the capture.

This also removes a problematic use of disable_irq() in
a context it is forbidden, as explained in commit
af3e0fcf7887 ("8139too: Use disable_irq_nosync() in
rtl8139_poll_controller()")

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ixgb/ixgb_main.c | 25 ---------------------
 1 file changed, 25 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgb/ixgb_main.c b/drivers/net/ethernet/intel/ixgb/ixgb_main.c
index d3e72d0f66ef428b08e4bd88508e05b734bc43a4..7722153c4ac2fdff29c0afa0627ec26228557d69 100644
--- a/drivers/net/ethernet/intel/ixgb/ixgb_main.c
+++ b/drivers/net/ethernet/intel/ixgb/ixgb_main.c
@@ -81,11 +81,6 @@ static int ixgb_vlan_rx_kill_vid(struct net_device *netdev,
 				 __be16 proto, u16 vid);
 static void ixgb_restore_vlan(struct ixgb_adapter *adapter);
 
-#ifdef CONFIG_NET_POLL_CONTROLLER
-/* for netdump / net console */
-static void ixgb_netpoll(struct net_device *dev);
-#endif
-
 static pci_ers_result_t ixgb_io_error_detected (struct pci_dev *pdev,
                              enum pci_channel_state state);
 static pci_ers_result_t ixgb_io_slot_reset (struct pci_dev *pdev);
@@ -348,9 +343,6 @@ static const struct net_device_ops ixgb_netdev_ops = {
 	.ndo_tx_timeout		= ixgb_tx_timeout,
 	.ndo_vlan_rx_add_vid	= ixgb_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid	= ixgb_vlan_rx_kill_vid,
-#ifdef CONFIG_NET_POLL_CONTROLLER
-	.ndo_poll_controller	= ixgb_netpoll,
-#endif
 	.ndo_fix_features       = ixgb_fix_features,
 	.ndo_set_features       = ixgb_set_features,
 };
@@ -2195,23 +2187,6 @@ ixgb_restore_vlan(struct ixgb_adapter *adapter)
 		ixgb_vlan_rx_add_vid(adapter->netdev, htons(ETH_P_8021Q), vid);
 }
 
-#ifdef CONFIG_NET_POLL_CONTROLLER
-/*
- * Polling 'interrupt' - used by things like netconsole to send skbs
- * without having to re-enable interrupts. It's not called while
- * the interrupt routine is executing.
- */
-
-static void ixgb_netpoll(struct net_device *dev)
-{
-	struct ixgb_adapter *adapter = netdev_priv(dev);
-
-	disable_irq(adapter->pdev->irq);
-	ixgb_intr(adapter->pdev->irq, dev);
-	enable_irq(adapter->pdev->irq);
-}
-#endif
-
 /**
  * ixgb_io_error_detected - called when PCI error is detected
  * @pdev:    pointer to pci device with error
-- 
2.19.0.444.g18242da7ef-goog

[PATCH net 07/15] igb: remove ndo_poll_controller

[Eric Dumazet]

As diagnosed by Song Liu, ndo_poll_controller() can
be very dangerous on loaded hosts, since the cpu
calling ndo_poll_controller() might steal all NAPI
contexts (for all RX/TX queues of the NIC). This capture
can last for unlimited amount of time, since one
cpu is generally not able to drain all the queues under load.

igb uses NAPI for TX completions, so we better let core
networking stack call the napi->poll() to avoid the capture.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 30 -----------------------
 1 file changed, 30 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index a32c576c1e656c0102989413cad114d1d8f03771..0796cef96fa335ee1907d9aa5d4d56f4fd366ca3 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -205,10 +205,6 @@ static struct notifier_block dca_notifier = {
 	.priority	= 0
 };
 #endif
-#ifdef CONFIG_NET_POLL_CONTROLLER
-/* for netdump / net console */
-static void igb_netpoll(struct net_device *);
-#endif
 #ifdef CONFIG_PCI_IOV
 static unsigned int max_vfs;
 module_param(max_vfs, uint, 0);
@@ -2881,9 +2877,6 @@ static const struct net_device_ops igb_netdev_ops = {
 	.ndo_set_vf_spoofchk	= igb_ndo_set_vf_spoofchk,
 	.ndo_set_vf_trust	= igb_ndo_set_vf_trust,
 	.ndo_get_vf_config	= igb_ndo_get_vf_config,
-#ifdef CONFIG_NET_POLL_CONTROLLER
-	.ndo_poll_controller	= igb_netpoll,
-#endif
 	.ndo_fix_features	= igb_fix_features,
 	.ndo_set_features	= igb_set_features,
 	.ndo_fdb_add		= igb_ndo_fdb_add,
@@ -9053,29 +9046,6 @@ static int igb_pci_sriov_configure(struct pci_dev *dev, int num_vfs)
 	return 0;
 }
 
-#ifdef CONFIG_NET_POLL_CONTROLLER
-/* Polling 'interrupt' - used by things like netconsole to send skbs
- * without having to re-enable interrupts. It's not called while
- * the interrupt routine is executing.
- */
-static void igb_netpoll(struct net_device *netdev)
-{
-	struct igb_adapter *adapter = netdev_priv(netdev);
-	struct e1000_hw *hw = &adapter->hw;
-	struct igb_q_vector *q_vector;
-	int i;
-
-	for (i = 0; i < adapter->num_q_vectors; i++) {
-		q_vector = adapter->q_vector[i];
-		if (adapter->flags & IGB_FLAG_HAS_MSIX)
-			wr32(E1000_EIMC, q_vector->eims_value);
-		else
-			igb_irq_disable(adapter);
-		napi_schedule(&q_vector->napi);
-	}
-}
-#endif /* CONFIG_NET_POLL_CONTROLLER */
-
 /**
  *  igb_io_error_detected - called when PCI error is detected
  *  @pdev: Pointer to PCI device
-- 
2.19.0.444.g18242da7ef-goog

[PATCH net 08/15] ice: remove ndo_poll_controller

[Eric Dumazet]

As diagnosed by Song Liu, ndo_poll_controller() can
be very dangerous on loaded hosts, since the cpu
calling ndo_poll_controller() might steal all NAPI
contexts (for all RX/TX queues of the NIC). This capture
can last for unlimited amount of time, since one
cpu is generally not able to drain all the queues under load.

ice uses NAPI for TX completions, so we better let core
networking stack call the napi->poll() to avoid the capture.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c | 27 -----------------------
 1 file changed, 27 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index f1e80eed2fd6d9f94eab163acb4d178d83ba4af2..3f047bb43348881c592adf3b236518ee21bc2fdd 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -4806,30 +4806,6 @@ void ice_get_stats64(struct net_device *netdev, struct rtnl_link_stats64 *stats)
 	stats->rx_length_errors = vsi_stats->rx_length_errors;
 }
 
-#ifdef CONFIG_NET_POLL_CONTROLLER
-/**
- * ice_netpoll - polling "interrupt" handler
- * @netdev: network interface device structure
- *
- * Used by netconsole to send skbs without having to re-enable interrupts.
- * This is not called in the normal interrupt path.
- */
-static void ice_netpoll(struct net_device *netdev)
-{
-	struct ice_netdev_priv *np = netdev_priv(netdev);
-	struct ice_vsi *vsi = np->vsi;
-	struct ice_pf *pf = vsi->back;
-	int i;
-
-	if (test_bit(__ICE_DOWN, vsi->state) ||
-	    !test_bit(ICE_FLAG_MSIX_ENA, pf->flags))
-		return;
-
-	for (i = 0; i < vsi->num_q_vectors; i++)
-		ice_msix_clean_rings(0, vsi->q_vectors[i]);
-}
-#endif /* CONFIG_NET_POLL_CONTROLLER */
-
 /**
  * ice_napi_disable_all - Disable NAPI for all q_vectors in the VSI
  * @vsi: VSI having NAPI disabled
@@ -5497,9 +5473,6 @@ static const struct net_device_ops ice_netdev_ops = {
 	.ndo_validate_addr = eth_validate_addr,
 	.ndo_change_mtu = ice_change_mtu,
 	.ndo_get_stats64 = ice_get_stats64,
-#ifdef CONFIG_NET_POLL_CONTROLLER
-	.ndo_poll_controller = ice_netpoll,
-#endif /* CONFIG_NET_POLL_CONTROLLER */
 	.ndo_vlan_rx_add_vid = ice_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid = ice_vlan_rx_kill_vid,
 	.ndo_set_features = ice_set_features,
-- 
2.19.0.444.g18242da7ef-goog

[PATCH net 09/15] i40evf: remove ndo_poll_controller

[Eric Dumazet]

As diagnosed by Song Liu, ndo_poll_controller() can
be very dangerous on loaded hosts, since the cpu
calling ndo_poll_controller() might steal all NAPI
contexts (for all RX/TX queues of the NIC). This capture
can last for unlimited amount of time, since one
cpu is generally not able to drain all the queues under load.

i40evf uses NAPI for TX completions, so we better let core
networking stack call the napi->poll() to avoid the capture.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 .../net/ethernet/intel/i40evf/i40evf_main.c   | 26 -------------------
 1 file changed, 26 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40evf/i40evf_main.c b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
index 5906c1c1d19d82d7e37b0a891e457fea792b4153..fef6d892ed4cfe5ae293aa2fe01ef38a257c47bf 100644
--- a/drivers/net/ethernet/intel/i40evf/i40evf_main.c
+++ b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
@@ -396,29 +396,6 @@ static void i40evf_map_rings_to_vectors(struct i40evf_adapter *adapter)
 	adapter->aq_required |= I40EVF_FLAG_AQ_MAP_VECTORS;
 }
 
-#ifdef CONFIG_NET_POLL_CONTROLLER
-/**
- * i40evf_netpoll - A Polling 'interrupt' handler
- * @netdev: network interface device structure
- *
- * This is used by netconsole to send skbs without having to re-enable
- * interrupts.  It's not called while the normal interrupt routine is executing.
- **/
-static void i40evf_netpoll(struct net_device *netdev)
-{
-	struct i40evf_adapter *adapter = netdev_priv(netdev);
-	int q_vectors = adapter->num_msix_vectors - NONQ_VECS;
-	int i;
-
-	/* if interface is down do nothing */
-	if (test_bit(__I40E_VSI_DOWN, adapter->vsi.state))
-		return;
-
-	for (i = 0; i < q_vectors; i++)
-		i40evf_msix_clean_rings(0, &adapter->q_vectors[i]);
-}
-
-#endif
 /**
  * i40evf_irq_affinity_notify - Callback for affinity changes
  * @notify: context as to what irq was changed
@@ -3229,9 +3206,6 @@ static const struct net_device_ops i40evf_netdev_ops = {
 	.ndo_features_check	= i40evf_features_check,
 	.ndo_fix_features	= i40evf_fix_features,
 	.ndo_set_features	= i40evf_set_features,
-#ifdef CONFIG_NET_POLL_CONTROLLER
-	.ndo_poll_controller	= i40evf_netpoll,
-#endif
 	.ndo_setup_tc		= i40evf_setup_tc,
 };
 
-- 
2.19.0.444.g18242da7ef-goog

[PATCH net 10/15] mlx4: remove ndo_poll_controller

[Eric Dumazet]

As diagnosed by Song Liu, ndo_poll_controller() can
be very dangerous on loaded hosts, since the cpu
calling ndo_poll_controller() might steal all NAPI
contexts (for all RX/TX queues of the NIC). This capture
can last for unlimited amount of time, since one
cpu is generally not able to drain all the queues under load.

mlx4 uses NAPI for TX completions, so we better let core
networking stack call the napi->poll() to avoid the capture.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Tariq Toukan <tariqt@mellanox.com>
---
 .../net/ethernet/mellanox/mlx4/en_netdev.c    | 20 -------------------
 1 file changed, 20 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 6785661d1a72627d7cc6895359e0ece284577d96..fe49384eba48cb3f75bd33f5bfb9cf1fa15af791 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -1286,20 +1286,6 @@ static void mlx4_en_do_set_rx_mode(struct work_struct *work)
 	mutex_unlock(&mdev->state_lock);
 }
 
-#ifdef CONFIG_NET_POLL_CONTROLLER
-static void mlx4_en_netpoll(struct net_device *dev)
-{
-	struct mlx4_en_priv *priv = netdev_priv(dev);
-	struct mlx4_en_cq *cq;
-	int i;
-
-	for (i = 0; i < priv->tx_ring_num[TX]; i++) {
-		cq = priv->tx_cq[TX][i];
-		napi_schedule(&cq->napi);
-	}
-}
-#endif
-
 static int mlx4_en_set_rss_steer_rules(struct mlx4_en_priv *priv)
 {
 	u64 reg_id;
@@ -2946,9 +2932,6 @@ static const struct net_device_ops mlx4_netdev_ops = {
 	.ndo_tx_timeout		= mlx4_en_tx_timeout,
 	.ndo_vlan_rx_add_vid	= mlx4_en_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid	= mlx4_en_vlan_rx_kill_vid,
-#ifdef CONFIG_NET_POLL_CONTROLLER
-	.ndo_poll_controller	= mlx4_en_netpoll,
-#endif
 	.ndo_set_features	= mlx4_en_set_features,
 	.ndo_fix_features	= mlx4_en_fix_features,
 	.ndo_setup_tc		= __mlx4_en_setup_tc,
@@ -2983,9 +2966,6 @@ static const struct net_device_ops mlx4_netdev_ops_master = {
 	.ndo_set_vf_link_state	= mlx4_en_set_vf_link_state,
 	.ndo_get_vf_stats       = mlx4_en_get_vf_stats,
 	.ndo_get_vf_config	= mlx4_en_get_vf_config,
-#ifdef CONFIG_NET_POLL_CONTROLLER
-	.ndo_poll_controller	= mlx4_en_netpoll,
-#endif
 	.ndo_set_features	= mlx4_en_set_features,
 	.ndo_fix_features	= mlx4_en_fix_features,
 	.ndo_setup_tc		= __mlx4_en_setup_tc,
-- 
2.19.0.444.g18242da7ef-goog

[PATCH net 11/15] mlx5: remove ndo_poll_controller

[Eric Dumazet]

As diagnosed by Song Liu, ndo_poll_controller() can
be very dangerous on loaded hosts, since the cpu
calling ndo_poll_controller() might steal all NAPI
contexts (for all RX/TX queues of the NIC). This capture
can last for unlimited amount of time, since one
cpu is generally not able to drain all the queues under load.

mlx5 uses NAPI for TX completions, so we better let core
networking stack call the napi->poll() to avoid the capture.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Saeed Mahameed <saeedm@mellanox.com>
---
 .../net/ethernet/mellanox/mlx5/core/en_main.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 5a7939e7019026aa9c08d4fba759f95cb0a813c5..54118b77dc1f6d478c5b08e7c01526dd3e5dc740 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -4315,22 +4315,6 @@ static int mlx5e_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 	}
 }
 
-#ifdef CONFIG_NET_POLL_CONTROLLER
-/* Fake "interrupt" called by netpoll (eg netconsole) to send skbs without
- * reenabling interrupts.
- */
-static void mlx5e_netpoll(struct net_device *dev)
-{
-	struct mlx5e_priv *priv = netdev_priv(dev);
-	struct mlx5e_channels *chs = &priv->channels;
-
-	int i;
-
-	for (i = 0; i < chs->num; i++)
-		napi_schedule(&chs->c[i]->napi);
-}
-#endif
-
 static const struct net_device_ops mlx5e_netdev_ops = {
 	.ndo_open                = mlx5e_open,
 	.ndo_stop                = mlx5e_close,
@@ -4356,9 +4340,6 @@ static const struct net_device_ops mlx5e_netdev_ops = {
 #ifdef CONFIG_MLX5_EN_ARFS
 	.ndo_rx_flow_steer	 = mlx5e_rx_flow_steer,
 #endif
-#ifdef CONFIG_NET_POLL_CONTROLLER
-	.ndo_poll_controller     = mlx5e_netpoll,
-#endif
 #ifdef CONFIG_MLX5_ESWITCH
 	/* SRIOV E-Switch NDOs */
 	.ndo_set_vf_mac          = mlx5e_set_vf_mac,
-- 
2.19.0.444.g18242da7ef-goog

[PATCH net 12/15] bnx2x: remove ndo_poll_controller

[Eric Dumazet]

As diagnosed by Song Liu, ndo_poll_controller() can
be very dangerous on loaded hosts, since the cpu
calling ndo_poll_controller() might steal all NAPI
contexts (for all RX/TX queues of the NIC). This capture
can last for unlimited amount of time, since one
cpu is generally not able to drain all the queues under load.

bnx2x uses NAPI for TX completions, so we better let core
networking stack call the napi->poll() to avoid the capture.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Ariel Elior <ariel.elior@cavium.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 71362b7f60402545c3b4aa2bc391a1e2d3cd7f7f..fcc2328bb0d953e797c49699b11e5e5956a0d583 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -12894,19 +12894,6 @@ static int bnx2x_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	}
 }
 
-#ifdef CONFIG_NET_POLL_CONTROLLER
-static void poll_bnx2x(struct net_device *dev)
-{
-	struct bnx2x *bp = netdev_priv(dev);
-	int i;
-
-	for_each_eth_queue(bp, i) {
-		struct bnx2x_fastpath *fp = &bp->fp[i];
-		napi_schedule(&bnx2x_fp(bp, fp->index, napi));
-	}
-}
-#endif
-
 static int bnx2x_validate_addr(struct net_device *dev)
 {
 	struct bnx2x *bp = netdev_priv(dev);
@@ -13113,9 +13100,6 @@ static const struct net_device_ops bnx2x_netdev_ops = {
 	.ndo_tx_timeout		= bnx2x_tx_timeout,
 	.ndo_vlan_rx_add_vid	= bnx2x_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid	= bnx2x_vlan_rx_kill_vid,
-#ifdef CONFIG_NET_POLL_CONTROLLER
-	.ndo_poll_controller	= poll_bnx2x,
-#endif
 	.ndo_setup_tc		= __bnx2x_setup_tc,
 #ifdef CONFIG_BNX2X_SRIOV
 	.ndo_set_vf_mac		= bnx2x_set_vf_mac,
-- 
2.19.0.444.g18242da7ef-goog

[PATCH net 13/15] bnxt: remove ndo_poll_controller

[Eric Dumazet]

As diagnosed by Song Liu, ndo_poll_controller() can
be very dangerous on loaded hosts, since the cpu
calling ndo_poll_controller() might steal all NAPI
contexts (for all RX/TX queues of the NIC). This capture
can last for unlimited amount of time, since one
cpu is generally not able to drain all the queues under load.

bnxt uses NAPI for TX completions, so we better let core
networking stack call the napi->poll() to avoid the capture.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 177587f9c3f1e8560c16e2fc4e05b7edff4e0011..61957b0bbd8c9f46773ff26ac9d14759b96c3960 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -7672,21 +7672,6 @@ static void bnxt_tx_timeout(struct net_device *dev)
 	bnxt_queue_sp_work(bp);
 }
 
-#ifdef CONFIG_NET_POLL_CONTROLLER
-static void bnxt_poll_controller(struct net_device *dev)
-{
-	struct bnxt *bp = netdev_priv(dev);
-	int i;
-
-	/* Only process tx rings/combined rings in netpoll mode. */
-	for (i = 0; i < bp->tx_nr_rings; i++) {
-		struct bnxt_tx_ring_info *txr = &bp->tx_ring[i];
-
-		napi_schedule(&txr->bnapi->napi);
-	}
-}
-#endif
-
 static void bnxt_timer(struct timer_list *t)
 {
 	struct bnxt *bp = from_timer(bp, t, timer);
@@ -8519,9 +8504,6 @@ static const struct net_device_ops bnxt_netdev_ops = {
 	.ndo_set_vf_link_state	= bnxt_set_vf_link_state,
 	.ndo_set_vf_spoofchk	= bnxt_set_vf_spoofchk,
 	.ndo_set_vf_trust	= bnxt_set_vf_trust,
-#endif
-#ifdef CONFIG_NET_POLL_CONTROLLER
-	.ndo_poll_controller	= bnxt_poll_controller,
 #endif
 	.ndo_setup_tc           = bnxt_setup_tc,
 #ifdef CONFIG_RFS_ACCEL
-- 
2.19.0.444.g18242da7ef-goog

[PATCH net 14/15] nfp: remove ndo_poll_controller

[Eric Dumazet]

As diagnosed by Song Liu, ndo_poll_controller() can
be very dangerous on loaded hosts, since the cpu
calling ndo_poll_controller() might steal all NAPI
contexts (for all RX/TX queues of the NIC). This capture
can last for unlimited amount of time, since one
cpu is generally not able to drain all the queues under load.

nfp uses NAPI for TX completions, so we better let core
networking stack call the napi->poll() to avoid the capture.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 .../ethernet/netronome/nfp/nfp_net_common.c    | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index 253bdaef150557a7e20ded3de921021a051a9ca7..8ed38fd5a8520e0e125d96bc2ee7c53d891a5969 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
@@ -3146,21 +3146,6 @@ nfp_net_vlan_rx_kill_vid(struct net_device *netdev, __be16 proto, u16 vid)
 	return nfp_net_reconfig_mbox(nn, NFP_NET_CFG_MBOX_CMD_CTAG_FILTER_KILL);
 }
 
-#ifdef CONFIG_NET_POLL_CONTROLLER
-static void nfp_net_netpoll(struct net_device *netdev)
-{
-	struct nfp_net *nn = netdev_priv(netdev);
-	int i;
-
-	/* nfp_net's NAPIs are statically allocated so even if there is a race
-	 * with reconfig path this will simply try to schedule some disabled
-	 * NAPI instances.
-	 */
-	for (i = 0; i < nn->dp.num_stack_tx_rings; i++)
-		napi_schedule_irqoff(&nn->r_vecs[i].napi);
-}
-#endif
-
 static void nfp_net_stat64(struct net_device *netdev,
 			   struct rtnl_link_stats64 *stats)
 {
@@ -3519,9 +3504,6 @@ const struct net_device_ops nfp_net_netdev_ops = {
 	.ndo_get_stats64	= nfp_net_stat64,
 	.ndo_vlan_rx_add_vid	= nfp_net_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid	= nfp_net_vlan_rx_kill_vid,
-#ifdef CONFIG_NET_POLL_CONTROLLER
-	.ndo_poll_controller	= nfp_net_netpoll,
-#endif
 	.ndo_set_vf_mac         = nfp_app_set_vf_mac,
 	.ndo_set_vf_vlan        = nfp_app_set_vf_vlan,
 	.ndo_set_vf_spoofchk    = nfp_app_set_vf_spoofchk,
-- 
2.19.0.444.g18242da7ef-goog

Re: [PATCH net 14/15] nfp: remove ndo_poll_controller

[Jakub Kicinski]

On Fri, 21 Sep 2018 15:27:51 -0700, Eric Dumazet wrote:
> As diagnosed by Song Liu, ndo_poll_controller() can
> be very dangerous on loaded hosts, since the cpu
> calling ndo_poll_controller() might steal all NAPI
> contexts (for all RX/TX queues of the NIC). This capture
> can last for unlimited amount of time, since one
> cpu is generally not able to drain all the queues under load.
> 
> nfp uses NAPI for TX completions, so we better let core
> networking stack call the napi->poll() to avoid the capture.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Tested-by: Jakub Kicinski <jakub.kicinski@netronome.com>

My testing only detects the pre-existing lockdep splat :)


The test runs multiple netperf sessions and it has threads spewing the
"noise_xyz" lines into /dev/kmsg, then another thread sends series of
the "this_is_line_X_" lines which are counted on the other end to make
sure we don't drop the messages.

And every now and then the test will do something random like reload
the driver or bring the link down or install an XDP program.

[ 1511.417141] BUG: workqueue lockup - pool cpus=4 node=0 flags=0x0 nice=0
[ 1511.417173] noise_KVEQAAHZ
[ 1511.417178]  stuck for 35s!
[ 1511.425191] noise_KVEQAAHZ
[ 1511.428134] Showing busy workqueues and worker pools:
[ 1511.434080] noise_KVEQAAHZ
[ 1511.434744] workqueue events: flags=0x0
[ 1511.440908] noise_KVEQAAHZ
[ 1511.443919]   pwq 8: cpus=4 node=0 flags=0x0 nice=0 active=2/256
[ 1511.458404]     in-flight: 197:free_obj_work
[ 1511.463337]     pending: free_obj_work
[ 1511.467670] 
[ 1511.467671] ======================================================
[ 1511.467672] WARNING: possible circular locking dependency detected
[ 1511.467673] 4.19.0-rc4-debug-00636-g4718a743bc48 #23 Not tainted
[ 1511.467674] ------------------------------------------------------
[ 1511.467674] ksoftirqd/0/10 is trying to acquire lock:
[ 1511.467675] 0000000081e5d4d3 (console_owner){-.-.}, at: vprintk_emit+0x2de/0x480
[ 1511.467678] 
[ 1511.467679] but task is already holding lock:
[ 1511.467680] 00000000ebab6c31 (&(&pool->lock)->rlock){-.-.}, at: show_workqueue_state.cold.34+0x36d/0x13d7
[ 1511.467683] 
[ 1511.467684] which lock already depends on the new lock.
[ 1511.467684] 
[ 1511.467685] 
[ 1511.467686] the existing dependency chain (in reverse order) is:
[ 1511.467686] 
[ 1511.467687] -> #2 (&(&pool->lock)->rlock){-.-.}:
[ 1511.467690]        _raw_spin_lock+0x2f/0x40
[ 1511.467691]        __queue_work+0x2f4/0x11d0
[ 1511.467691]        __queue_delayed_work+0x22d/0x380
[ 1511.467692]        queue_delayed_work_on+0x8d/0xa0
[ 1511.467693]        netpoll_send_skb_on_dev+0x307/0x7e0
[ 1511.467694]        netpoll_send_udp+0x1249/0x1910
[ 1511.467695]        write_msg+0x285/0x2f0 [netconsole]
[ 1511.467695]        console_unlock+0x6a0/0xc90
[ 1511.467696]        vprintk_emit+0x1dc/0x480
[ 1511.467697]        printk_emit+0x88/0xae
[ 1511.467697]        devkmsg_write+0x2e5/0x360
[ 1511.467698]        __vfs_write+0x4e9/0x890
[ 1511.467699]        vfs_write+0x156/0x4a0
[ 1511.467699]        ksys_write+0xd6/0x1d0
[ 1511.467700]        do_syscall_64+0x146/0x5c0
[ 1511.467701]        entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 1511.467701] 
[ 1511.467702] -> #1 (target_list_lock){-.-.}:
[ 1511.467705]        _raw_spin_lock_irqsave+0x46/0x60
[ 1511.467706]        write_msg+0xe1/0x2f0 [netconsole]
[ 1511.467706]        console_unlock+0x6a0/0xc90
[ 1511.467707]        vprintk_emit+0x1dc/0x480
[ 1511.467708]        printk+0x9f/0xc5
[ 1511.467708]        netpoll_print_options+0x77/0x2f0
[ 1511.467709]        enabled_store+0x174/0x320 [netconsole]
[ 1511.467710]        configfs_write_file+0x250/0x400 [configfs]
[ 1511.467711]        __vfs_write+0xde/0x890
[ 1511.467711]        vfs_write+0x156/0x4a0
[ 1511.467712]        ksys_write+0xd6/0x1d0
[ 1511.467713]        do_syscall_64+0x146/0x5c0
[ 1511.467713]        entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 1511.467714] 
[ 1511.467714] -> #0 (console_owner){-.-.}:
[ 1511.467717]        lock_acquire+0x183/0x400
[ 1511.467718]        vprintk_emit+0x339/0x480
[ 1511.467719]        printk+0x9f/0xc5
[ 1511.467720]        show_workqueue_state.cold.34+0x40d/0x13d7
[ 1511.467721]        wq_watchdog_timer_fn+0x598/0x850
[ 1511.467721]        call_timer_fn+0x21b/0x720
[ 1511.467722]        expire_timers+0x34a/0x550
[ 1511.467723]        run_timer_softirq+0x45e/0x6a0
[ 1511.467724]        __do_softirq+0x2ca/0xb1d
[ 1511.467725]        run_ksoftirqd+0x3c/0x60
[ 1511.467725]        smpboot_thread_fn+0x58a/0x9c0
[ 1511.467726]        kthread+0x315/0x3d0
[ 1511.467727]        ret_from_fork+0x24/0x30
[ 1511.467728] 
[ 1511.467729] other info that might help us debug this:
[ 1511.467729] 
[ 1511.467730] Chain exists of:
[ 1511.467730]   console_owner --> target_list_lock --> &(&pool->lock)->rlock
[ 1511.467734] 
[ 1511.467735]  Possible unsafe locking scenario:
[ 1511.467735] 
[ 1511.467736]        CPU0                    CPU1
[ 1511.467737]        ----                    ----
[ 1511.467737]   lock(&(&pool->lock)->rlock);
[ 1511.467739]                                lock(target_list_lock);
[ 1511.467741]                                lock(&(&pool->lock)->rlock);
[ 1511.467743]   lock(console_owner);
[ 1511.467744] 
[ 1511.467745]  *** DEADLOCK ***
[ 1511.467746] 
[ 1511.467746] 3 locks held by ksoftirqd/0/10:
[ 1511.467747]  #0: 0000000007b706eb ((&wq_watchdog_timer)){+.-.}, at: call_timer_fn+0x1c3/0x720
[ 1511.467750]  #1: 00000000470e0b00 (rcu_read_lock_sched){....}, at: show_workqueue_state+0x5/0x1a0
[ 1511.467753]  #2: 00000000ebab6c31 (&(&pool->lock)->rlock){-.-.}, at: show_workqueue_state.cold.34+0x36d/0x13d7
[ 1511.467757] 
[ 1511.467757] stack backtrace:
[ 1511.467758] CPU: 0 PID: 10 Comm: ksoftirqd/0 Not tainted 4.19.0-rc4-debug-00636-g4718a743bc48 #23
[ 1511.467759] Hardware name: Dell Inc. PowerEdge R730/072T6D, BIOS 2.3.4 11/08/2016
[ 1511.467760] Call Trace:
[ 1511.467761]  dump_stack+0xea/0x1b0
[ 1511.467761]  ? dump_stack_print_info.cold.0+0x1b/0x1b
[ 1511.467762]  ? print_stack_trace+0xf3/0x140
[ 1511.467763]  print_circular_bug.isra.13.cold.36+0x2e2/0x417
[ 1511.467764]  ? print_circular_bug_header+0x80/0x80
[ 1511.467764]  check_prev_add.constprop.21+0xccd/0x1600
[ 1511.467765]  ? check_usage+0x7b0/0x7b0
[ 1511.467766]  ? __print_lock_name+0x110/0x110
[ 1511.467767]  ? graph_lock+0x65/0x190
[ 1511.467767]  ? __print_lock_name+0x110/0x110
[ 1511.467768]  __lock_acquire+0x216f/0x35b0
[ 1511.467769]  ? mark_held_locks+0x140/0x140
[ 1511.467770]  ? check_usage+0x7b0/0x7b0
[ 1511.467770]  ? print_irqtrace_events+0x270/0x270
[ 1511.467771]  ? lock_downgrade+0x6b0/0x6b0
[ 1511.467772]  ? do_raw_spin_unlock+0xb1/0x350
[ 1511.467772]  ? __print_lock_name+0x110/0x110
[ 1511.467773]  ? __lock_acquire+0x6a5/0x35b0
[ 1511.467774]  ? mark_held_locks+0x140/0x140
[ 1511.467774]  ? down_trylock+0x4b/0x70
[ 1511.467775]  ? down_trylock+0x13/0x70
[ 1511.467776]  lock_acquire+0x183/0x400
[ 1511.467776]  ? vprintk_emit+0x2de/0x480
[ 1511.467777]  ? lock_release+0x870/0x870
[ 1511.467778]  ? do_raw_spin_trylock+0x1b0/0x1b0
[ 1511.467779]  ? vprintk_emit+0x215/0x480
[ 1511.467779]  ? trace_hardirqs_off+0x95/0x230
[ 1511.467780]  vprintk_emit+0x339/0x480
[ 1511.467781]  ? vprintk_emit+0x2de/0x480
[ 1511.467781]  ? wake_up_klogd+0x1d0/0x1d0
[ 1511.467782]  ? do_raw_spin_unlock+0xb1/0x350
[ 1511.467783]  ? lock_acquire+0x183/0x400
[ 1511.467784]  ? show_workqueue_state.cold.34+0x36d/0x13d7
[ 1511.467784]  ? _raw_spin_unlock_irqrestore+0x59/0x70
[ 1511.467785]  printk+0x9f/0xc5
[ 1511.467786]  ? kmsg_dump_rewind_nolock+0xd9/0xd9
[ 1511.467786]  show_workqueue_state.cold.34+0x40d/0x13d7
[ 1511.467787]  ? print_worker_info+0x4a0/0x4a0
[ 1511.467788]  ? llist_reverse_order+0x60/0x60
[ 1511.467789]  ? radix_tree_iter_resume+0x190/0x190
[ 1511.467789]  ? devkmsg_read+0x840/0x840
[ 1511.467790]  ? radix_tree_next_chunk+0x1b2/0xae0
[ 1511.467791]  ? __radix_tree_next_slot+0x810/0x810
[ 1511.467792]  ? vprintk_emit+0x15f/0x480
[ 1511.467792]  ? wake_up_klogd+0x1d0/0x1d0
[ 1511.467793]  ? print_irqtrace_events+0x270/0x270
[ 1511.467794]  ? mark_held_locks+0x140/0x140
[ 1511.467795]  ? lock_downgrade+0x6b0/0x6b0
[ 1511.467796]  ? _raw_spin_unlock_irqrestore+0x59/0x70
[ 1511.467796]  ? wq_watchdog_timer_fn+0x4ce/0x850
[ 1511.467797]  ? lock_downgrade+0x6b0/0x6b0
[ 1511.467798]  ? idr_get_next_ul+0x290/0x290
[ 1511.467799]  ? rcu_read_unlock_special.part.22+0x1660/0x1660
[ 1511.467800]  ? rcu_dynticks_curr_cpu_in_eqs+0xd9/0x1d0
[ 1511.467801]  wq_watchdog_timer_fn+0x598/0x850
[ 1511.467801]  ? show_workqueue_state+0x1a0/0x1a0
[ 1511.467802]  ? quarantine_put+0xd9/0x300
[ 1511.467803]  ? lockdep_hardirqs_on+0x278/0x580
[ 1511.467804]  ? trace_hardirqs_on+0x74/0x230
[ 1511.467804]  ? lock_acquire+0x183/0x400
[ 1511.467805]  ? call_timer_fn+0x1c3/0x720
[ 1511.467806]  ? rcu_lockdep_current_cpu_online+0x212/0x310
[ 1511.467807]  ? rcu_process_callbacks+0x660/0x660
[ 1511.467807]  call_timer_fn+0x21b/0x720
[ 1511.467808]  ? show_workqueue_state+0x1a0/0x1a0
[ 1511.467809]  ? init_timer_key+0x380/0x380
[ 1511.467810]  ? _raw_spin_unlock_irqrestore+0x3c/0x70
[ 1511.467810]  ? expire_timers+0x33f/0x550
[ 1511.467811]  ? _raw_spin_unlock_irqrestore+0x46/0x70
[ 1511.467812]  ? lock_downgrade+0x6b0/0x6b0
[ 1511.467812]  ? _raw_spin_unlock_irq+0x29/0x50
[ 1511.467813]  ? show_workqueue_state+0x1a0/0x1a0
[ 1511.467814]  ? lockdep_hardirqs_on+0x278/0x580
[ 1511.467815]  ? _raw_spin_unlock_irq+0x29/0x50
[ 1511.467815]  ? trace_hardirqs_on+0x74/0x230
[ 1511.467816]  ? show_workqueue_state+0x1a0/0x1a0
[ 1511.467817]  expire_timers+0x34a/0x550
[ 1511.467817]  ? call_timer_fn+0x720/0x720
[ 1511.467818]  ? lock_acquire+0x183/0x400
[ 1511.467819]  ? run_timer_softirq+0x39d/0x6a0
[ 1511.467819]  ? lock_release+0x870/0x870
[ 1511.467820]  ? trace_hardirqs_off+0x95/0x230
[ 1511.467821]  ? __next_timer_interrupt+0xf0/0xf0
[ 1511.467822]  ? do_raw_spin_trylock+0x1b0/0x1b0
[ 1511.467822]  run_timer_softirq+0x45e/0x6a0
[ 1511.467823]  ? expire_timers+0x550/0x550
[ 1511.467824]  ? tcp_tasklet_func+0x3d6/0x650
[ 1511.467824]  ? tcp_tsq_handler+0x100/0x100
[ 1511.467825]  ? rcu_process_callbacks+0x660/0x660
[ 1511.467826]  ? tasklet_action_common.isra.2+0xcb/0x350
[ 1511.467827]  ? lockdep_hardirqs_on+0x278/0x580
[ 1511.467827]  ? tasklet_action_common.isra.2+0xcb/0x350
[ 1511.467828]  ? trace_hardirqs_on+0x74/0x230
[ 1511.467829]  ? __bpf_trace_preemptirq_template+0x10/0x10
[ 1511.467830]  ? __rcu_process_callbacks+0x1750/0x1750
[ 1511.467830]  ? __do_softirq+0x286/0xb1d
[ 1511.467831]  ? rcu_lockdep_current_cpu_online+0x212/0x310
[ 1511.467832]  ? rcu_process_callbacks+0x660/0x660
[ 1511.467832]  __do_softirq+0x2ca/0xb1d
[ 1511.467833]  ? trace_hardirqs_on_thunk+0x1a/0x1c
[ 1511.467834]  ? __irqentry_text_end+0x1fa0ca/0x1fa0ca
[ 1511.467835]  ? trace_hardirqs_on+0x230/0x230
[ 1511.467835]  ? __schedule+0x6/0x2130
[ 1511.467836]  ? trace_hardirqs_on_thunk+0x1a/0x1c
[ 1511.467837]  ? run_ksoftirqd+0x11/0x60
[ 1511.467837]  ? trace_hardirqs_off+0x95/0x230
[ 1511.467838]  ? trace_hardirqs_on_caller+0x240/0x240
[ 1511.467839]  ? finish_task_switch+0xa20/0xa20
[ 1511.467840]  ? ___preempt_schedule+0x16/0x18
[ 1511.467840]  run_ksoftirqd+0x3c/0x60
[ 1511.467841]  smpboot_thread_fn+0x58a/0x9c0
[ 1511.467842]  ? __smpboot_create_thread+0x410/0x410
[ 1511.467843]  ? _raw_spin_unlock_irqrestore+0x46/0x70
[ 1511.467843]  ? __kthread_parkme+0xb6/0x180
[ 1511.467844]  ? __smpboot_create_thread+0x410/0x410
[ 1511.467845]  kthread+0x315/0x3d0
[ 1511.467845]  ? kthread_park+0x120/0x120
[ 1511.467846]  ret_from_fork+0x24/0x30
[ 1511.468035] this_is_line_0_
[ 1511.469707] workqueue mm_percpu_wq: flags=0x8
[ 1511.475150] noise_KVEQAAHZ
[ 1511.475474] noise_KVEQAAHZ
[ 1511.475740] noise_KVEQAAHZ
[ 1511.476125] noise_KVEQAAHZ
[ 1511.476409] noise_KVEQAAHZ
[ 1511.476700] noise_KVEQAAHZ
[ 1511.476996] noise_KVEQAAHZ
[ 1511.477149] this_is_line_1_
[ 1511.477558] this_is_line_2_
[ 1511.477814] this_is_line_3_
[ 1511.478132] this_is_line_4_
[ 1511.478463] this_is_line_5_
[ 1511.478762] this_is_line_6_
[ 1511.479077] this_is_line_7_
[ 1511.479359] this_is_line_8_
[ 1511.479647] this_is_line_9_
[ 1511.479943] this_is_line_10_
[ 1511.480192] this_is_line_11_
[ 1511.480495] this_is_line_12_
[ 1511.480801] this_is_line_13_
[ 1511.483873]   pwq 8: cpus=4 node=0 flags=0x0 nice=0 active=1/256
[ 1511.491063] noise_KVEQAAHZ
[ 1511.496337] this_is_line_14_
[ 1511.496649] this_is_line_15_
[ 1511.496905] this_is_line_16_
[ 1511.497257] this_is_line_17_
[ 1511.497521] this_is_line_18_
[ 1511.497797]     pending: vmstat_update
[ 1511.497827] this_is_line_19_
[ 1511.497881] pool 8: cpus=4 node=0 flags=0x0 nice=0 hung=35s workers=3 idle: 12260 9068

[PATCH net 15/15] tun: remove ndo_poll_controller

[Eric Dumazet]

As diagnosed by Song Liu, ndo_poll_controller() can
be very dangerous on loaded hosts, since the cpu
calling ndo_poll_controller() might steal all NAPI
contexts (for all RX/TX queues of the NIC). This capture
can last for unlimited amount of time, since one
cpu is generally not able to drain all the queues under load.

tun uses NAPI for TX completions, so we better let core
networking stack call the napi->poll() to avoid the capture.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 drivers/net/tun.c | 43 -------------------------------------------
 1 file changed, 43 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index ebd07ad82431ec78c3801999fdc986188216655a..e2648b5a3861e51dc6c40d19e1198a5f3f7ca7af 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1153,43 +1153,6 @@ static netdev_features_t tun_net_fix_features(struct net_device *dev,
 
 	return (features & tun->set_features) | (features & ~TUN_USER_FEATURES);
 }
-#ifdef CONFIG_NET_POLL_CONTROLLER
-static void tun_poll_controller(struct net_device *dev)
-{
-	/*
-	 * Tun only receives frames when:
-	 * 1) the char device endpoint gets data from user space
-	 * 2) the tun socket gets a sendmsg call from user space
-	 * If NAPI is not enabled, since both of those are synchronous
-	 * operations, we are guaranteed never to have pending data when we poll
-	 * for it so there is nothing to do here but return.
-	 * We need this though so netpoll recognizes us as an interface that
-	 * supports polling, which enables bridge devices in virt setups to
-	 * still use netconsole
-	 * If NAPI is enabled, however, we need to schedule polling for all
-	 * queues unless we are using napi_gro_frags(), which we call in
-	 * process context and not in NAPI context.
-	 */
-	struct tun_struct *tun = netdev_priv(dev);
-
-	if (tun->flags & IFF_NAPI) {
-		struct tun_file *tfile;
-		int i;
-
-		if (tun_napi_frags_enabled(tun))
-			return;
-
-		rcu_read_lock();
-		for (i = 0; i < tun->numqueues; i++) {
-			tfile = rcu_dereference(tun->tfiles[i]);
-			if (tfile->napi_enabled)
-				napi_schedule(&tfile->napi);
-		}
-		rcu_read_unlock();
-	}
-	return;
-}
-#endif
 
 static void tun_set_headroom(struct net_device *dev, int new_hr)
 {
@@ -1283,9 +1246,6 @@ static const struct net_device_ops tun_netdev_ops = {
 	.ndo_start_xmit		= tun_net_xmit,
 	.ndo_fix_features	= tun_net_fix_features,
 	.ndo_select_queue	= tun_select_queue,
-#ifdef CONFIG_NET_POLL_CONTROLLER
-	.ndo_poll_controller	= tun_poll_controller,
-#endif
 	.ndo_set_rx_headroom	= tun_set_headroom,
 	.ndo_get_stats64	= tun_net_get_stats64,
 };
@@ -1365,9 +1325,6 @@ static const struct net_device_ops tap_netdev_ops = {
 	.ndo_set_mac_address	= eth_mac_addr,
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_select_queue	= tun_select_queue,
-#ifdef CONFIG_NET_POLL_CONTROLLER
-	.ndo_poll_controller	= tun_poll_controller,
-#endif
 	.ndo_features_check	= passthru_features_check,
 	.ndo_set_rx_headroom	= tun_set_headroom,
 	.ndo_get_stats64	= tun_net_get_stats64,
-- 
2.19.0.444.g18242da7ef-goog

Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers

[David Miller]

From: Eric Dumazet <edumazet@google.com>
Date: Fri, 21 Sep 2018 15:27:37 -0700

> As diagnosed by Song Liu, ndo_poll_controller() can
> be very dangerous on loaded hosts, since the cpu
> calling ndo_poll_controller() might steal all NAPI
> contexts (for all RX/TX queues of the NIC).
> 
> This capture, showing one ksoftirqd eating all cycles
> can last for unlimited amount of time, since one
> cpu is generally not able to drain all the queues under load.
> 
> It seems that all networking drivers that do use NAPI
> for their TX completions, should not provide a ndo_poll_controller() :
> 
> Most NAPI drivers have netpoll support already handled
> in core networking stack, since netpoll_poll_dev()
> uses poll_napi(dev) to iterate through registered
> NAPI contexts for a device.

I'm having trouble understanding the difference.

If the drivers are processing all of the RX/TX queue draining by hand
in their ndo_poll_controller() method, how is that different from the
generic code walking all of the registererd NAPI instances one by one?

Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers

[Eric Dumazet]

On Sun, Sep 23, 2018 at 12:29 PM David Miller <davem@davemloft.net> wrote:
>
> From: Eric Dumazet <edumazet@google.com>
> Date: Fri, 21 Sep 2018 15:27:37 -0700
>
> > As diagnosed by Song Liu, ndo_poll_controller() can
> > be very dangerous on loaded hosts, since the cpu
> > calling ndo_poll_controller() might steal all NAPI
> > contexts (for all RX/TX queues of the NIC).
> >
> > This capture, showing one ksoftirqd eating all cycles
> > can last for unlimited amount of time, since one
> > cpu is generally not able to drain all the queues under load.
> >
> > It seems that all networking drivers that do use NAPI
> > for their TX completions, should not provide a ndo_poll_controller() :
> >
> > Most NAPI drivers have netpoll support already handled
> > in core networking stack, since netpoll_poll_dev()
> > uses poll_napi(dev) to iterate through registered
> > NAPI contexts for a device.
>
> I'm having trouble understanding the difference.
>
> If the drivers are processing all of the RX/TX queue draining by hand
> in their ndo_poll_controller() method, how is that different from the
> generic code walking all of the registererd NAPI instances one by one?

(resent in plain text mode this time)

Reading poll_napi() and poll_one_napi() I thought that we were using
NAPI_STATE_NPSVC
and cmpxchg(&napi->poll_owner, -1, cpu) to _temporary_ [1] own each
napi at a time.

But I do see we also have this part at the beginning of poll_one_napi() :

if (!test_bit(NAPI_STATE_SCHED, &napi->state))
      return;

So we probably should remove it. (The normal napi->poll() calls would
not proceed since napi->poll_owner would not be -1)

[1]
While if a cpu succeeds into setting NAPI_STATE_SCHED, it means it has
to own it as long as the
napi->poll() does not call napi_complete_done(), and this can be
forever (the capture effect)

Basically calling napi_schedule() is the dangerous part.

I believe busy_polling and netpoll are the same intruders (as they can
run on arbitrary cpus).
But netpoll is far more problematic since it iterates through all RX/TX queues.

Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers

[David Miller]

From: Eric Dumazet <edumazet@google.com>
Date: Fri, 21 Sep 2018 15:27:37 -0700

> As diagnosed by Song Liu, ndo_poll_controller() can
> be very dangerous on loaded hosts, since the cpu
> calling ndo_poll_controller() might steal all NAPI
> contexts (for all RX/TX queues of the NIC).
> 
> This capture, showing one ksoftirqd eating all cycles
> can last for unlimited amount of time, since one
> cpu is generally not able to drain all the queues under load.
> 
> It seems that all networking drivers that do use NAPI
> for their TX completions, should not provide a ndo_poll_controller() :
> 
> Most NAPI drivers have netpoll support already handled
> in core networking stack, since netpoll_poll_dev()
> uses poll_napi(dev) to iterate through registered
> NAPI contexts for a device.
> 
> This patch series take care of the first round, we will
> handle other drivers in future rounds.

Series applied, thanks Eric.

Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers

[Eric Dumazet]

On Sun, Sep 23, 2018 at 10:04 PM David Miller <davem@davemloft.net> wrote:
>
> Series applied, thanks Eric.

Thanks David.

Song, would you please this additional patch ?

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 3219a2932463096566ce8ff336ecdf699422dd65..2ad45babe621b2c979ad5496b7df4342e4efbaa6
100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -150,13 +150,6 @@ static void poll_one_napi(struct napi_struct *napi)
 {
        int work = 0;

-       /* net_rx_action's ->poll() invocations and our's are
-        * synchronized by this test which is only made while
-        * holding the napi->poll_lock.
-        */
-       if (!test_bit(NAPI_STATE_SCHED, &napi->state))
-               return;
-
        /* If we set this bit but see that it has already been set,
         * that indicates that napi has been disabled and we need
         * to abort this operation

Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers

[Song Liu]

> On Sep 24, 2018, at 8:30 AM, Eric Dumazet <edumazet@google.com> wrote:
> 
> On Sun, Sep 23, 2018 at 10:04 PM David Miller <davem@davemloft.net> wrote:
>> 
>> Series applied, thanks Eric.
> 
> Thanks David.
> 
> Song, would you please this additional patch ?
> 
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index 3219a2932463096566ce8ff336ecdf699422dd65..2ad45babe621b2c979ad5496b7df4342e4efbaa6
> 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -150,13 +150,6 @@ static void poll_one_napi(struct napi_struct *napi)
> {
>        int work = 0;
> 
> -       /* net_rx_action's ->poll() invocations and our's are
> -        * synchronized by this test which is only made while
> -        * holding the napi->poll_lock.
> -        */
> -       if (!test_bit(NAPI_STATE_SCHED, &napi->state))
> -               return;
> -
>        /* If we set this bit but see that it has already been set,
>         * that indicates that napi has been disabled and we need
>         * to abort this operation

This triggers dev_watchdog() on a simple netperf TCP_RR on bnxt (I haven't
tested other drivers yet). 

I guess this is because NAPI_STATE_SCHED is set when poll_one_napi() calls
napi->poll(). And then cleared by napi->poll(). So a packet is missed by
napi (set NAPI_STATE_SCHED, but didn't got handled). 

Song

Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers

[Eric Dumazet]

On Mon, Sep 24, 2018 at 12:31 PM Song Liu <songliubraving@fb.com> wrote:

> This triggers dev_watchdog() on a simple netperf TCP_RR on bnxt (I haven't
> tested other drivers yet).
>
> I guess this is because NAPI_STATE_SCHED is set when poll_one_napi() calls
> napi->poll(). And then cleared by napi->poll().

Which part of napi->poll() could possibly clear NAPI_STATE_SCHED when
called by netpoll ?

AFAIK, napi_complete_done() should exit early (before having a chance
to clear NAPI_STATE_SCHED)
because of :

if (unlikely(n->state & (NAPIF_STATE_NPSVC | NAPIF_STATE_IN_BUSY_POLL)))
     return false;

Thanks !

Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers

[Song Liu]

> On Sep 24, 2018, at 12:41 PM, Eric Dumazet <edumazet@google.com> wrote:
> 
> On Mon, Sep 24, 2018 at 12:31 PM Song Liu <songliubraving@fb.com> wrote:
> 
>> This triggers dev_watchdog() on a simple netperf TCP_RR on bnxt (I haven't
>> tested other drivers yet).
>> 
>> I guess this is because NAPI_STATE_SCHED is set when poll_one_napi() calls
>> napi->poll(). And then cleared by napi->poll().
> 
> Which part of napi->poll() could possibly clear NAPI_STATE_SCHED when
> called by netpoll ?
> 
> AFAIK, napi_complete_done() should exit early (before having a chance
> to clear NAPI_STATE_SCHED)
> because of :
> 
> if (unlikely(n->state & (NAPIF_STATE_NPSVC | NAPIF_STATE_IN_BUSY_POLL)))
>     return false;
> 
> Thanks !

You are right on this condition. But this does trigger dev_watchdog() for
some reason. 

Thanks,
Song

Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers

[Eric Dumazet]

On Mon, Sep 24, 2018 at 1:00 PM Song Liu <songliubraving@fb.com> wrote:
>
>
>
> > On Sep 24, 2018, at 12:41 PM, Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Mon, Sep 24, 2018 at 12:31 PM Song Liu <songliubraving@fb.com> wrote:
> >
> >> This triggers dev_watchdog() on a simple netperf TCP_RR on bnxt (I haven't
> >> tested other drivers yet).
> >>
> >> I guess this is because NAPI_STATE_SCHED is set when poll_one_napi() calls
> >> napi->poll(). And then cleared by napi->poll().
> >
> > Which part of napi->poll() could possibly clear NAPI_STATE_SCHED when
> > called by netpoll ?
> >
> > AFAIK, napi_complete_done() should exit early (before having a chance
> > to clear NAPI_STATE_SCHED)
> > because of :
> >
> > if (unlikely(n->state & (NAPIF_STATE_NPSVC | NAPIF_STATE_IN_BUSY_POLL)))
> >     return false;
> >
> > Thanks !
>
> You are right on this condition. But this does trigger dev_watchdog() for
> some reason.

Interesting, maybe a bnxt specific issue.

It seems their model is to process TX/RX notification in the same queue,
they throw away RX events if budget == 0

It means commit e7b9569102995ebc26821789628eef45bd9840d8 is wrong and
must be reverted.

Otherwise, we have a possibility of blocking a queue under netpoll pressure.

Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers

[Eric Dumazet]

>
> Interesting, maybe a bnxt specific issue.
>
> It seems their model is to process TX/RX notification in the same queue,
> they throw away RX events if budget == 0
>
> It means commit e7b9569102995ebc26821789628eef45bd9840d8 is wrong and
> must be reverted.
>
> Otherwise, we have a possibility of blocking a queue under netpoll pressure.

Hmm, actually a revert might not be enough, since code at lines 2030-2031
would fire and we might not call napi_complete_done() anyway.

Unfortunately this driver logic is quite complex.

Could you test on other NIC eventually ?

Thanks.

Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers

[Song Liu]

> On Sep 24, 2018, at 2:05 PM, Eric Dumazet <edumazet@google.com> wrote:
> 
>> 
>> Interesting, maybe a bnxt specific issue.
>> 
>> It seems their model is to process TX/RX notification in the same queue,
>> they throw away RX events if budget == 0
>> 
>> It means commit e7b9569102995ebc26821789628eef45bd9840d8 is wrong and
>> must be reverted.
>> 
>> Otherwise, we have a possibility of blocking a queue under netpoll pressure.
> 
> Hmm, actually a revert might not be enough, since code at lines 2030-2031
> would fire and we might not call napi_complete_done() anyway.
> 
> Unfortunately this driver logic is quite complex.
> 
> Could you test on other NIC eventually ?
> 

It actually runs OK on ixgbe. 

@Michael, could you please help us with this? 

Thanks,
Song

Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers

[Michael Chan]

On Mon, Sep 24, 2018 at 2:18 PM Song Liu <songliubraving@fb.com> wrote:
>
>
>
> > On Sep 24, 2018, at 2:05 PM, Eric Dumazet <edumazet@google.com> wrote:
> >
> >>
> >> Interesting, maybe a bnxt specific issue.
> >>
> >> It seems their model is to process TX/RX notification in the same queue,
> >> they throw away RX events if budget == 0
> >>
> >> It means commit e7b9569102995ebc26821789628eef45bd9840d8 is wrong and
> >> must be reverted.
> >>
> >> Otherwise, we have a possibility of blocking a queue under netpoll pressure.
> >
> > Hmm, actually a revert might not be enough, since code at lines 2030-2031
> > would fire and we might not call napi_complete_done() anyway.
> >
> > Unfortunately this driver logic is quite complex.
> >
> > Could you test on other NIC eventually ?
> >
>
> It actually runs OK on ixgbe.
>
> @Michael, could you please help us with this?
>
I've taken a quick look using today's net tree plus Eric's
poll_one_napi() patch.  The problem I'm seeing is that netpoll calls
bnxt_poll() with budget 0.  And since work_done >= budget of 0, we
return without calling napi_complete_done() and without arming the
interrupt.  netpoll doesn't always call us back until we call
napi_complete_done(), right?  So I think if there are in-flight TX
completions, we'll miss those.

Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers

[Eric Dumazet]

On Tue, Sep 25, 2018 at 7:02 AM Michael Chan <michael.chan@broadcom.com> wrote:
>
> On Mon, Sep 24, 2018 at 2:18 PM Song Liu <songliubraving@fb.com> wrote:
> >
> >
> >
> > > On Sep 24, 2018, at 2:05 PM, Eric Dumazet <edumazet@google.com> wrote:
> > >
> > >>
> > >> Interesting, maybe a bnxt specific issue.
> > >>
> > >> It seems their model is to process TX/RX notification in the same queue,
> > >> they throw away RX events if budget == 0
> > >>
> > >> It means commit e7b9569102995ebc26821789628eef45bd9840d8 is wrong and
> > >> must be reverted.
> > >>
> > >> Otherwise, we have a possibility of blocking a queue under netpoll pressure.
> > >
> > > Hmm, actually a revert might not be enough, since code at lines 2030-2031
> > > would fire and we might not call napi_complete_done() anyway.
> > >
> > > Unfortunately this driver logic is quite complex.
> > >
> > > Could you test on other NIC eventually ?
> > >
> >
> > It actually runs OK on ixgbe.
> >
> > @Michael, could you please help us with this?
> >
> I've taken a quick look using today's net tree plus Eric's
> poll_one_napi() patch.  The problem I'm seeing is that netpoll calls
> bnxt_poll() with budget 0.  And since work_done >= budget of 0, we
> return without calling napi_complete_done() and without arming the
> interrupt.  netpoll doesn't always call us back until we call
> napi_complete_done(), right?  So I think if there are in-flight TX
> completions, we'll miss those.

That's the whole point of netpoll :

 We drain the TX queues, without interrupts being involved at all,
by calling ->napi() with a zero budget.

napi_complete(), even if called from ->napi() while budget was zero,
should do nothing but return early.

budget==0 means that ->napi() should process all TX completions.

So it looks like bnxt has a bug, that is showing up after the latest
poll_one_napi() patch.
This latest patch is needed otherwise the cpu attempting the
netpoll-TX-drain might drain nothing at all,
since it does not anymore call ndo_poll_controller() that was grabbing
SCHED bits on all queues (napi_schedule() like calls)

Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers

[Michael Chan]

On Tue, Sep 25, 2018 at 7:20 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Sep 25, 2018 at 7:02 AM Michael Chan <michael.chan@broadcom.com> wrote:
> >
> > On Mon, Sep 24, 2018 at 2:18 PM Song Liu <songliubraving@fb.com> wrote:
> > >
> > >
> > >
> > > > On Sep 24, 2018, at 2:05 PM, Eric Dumazet <edumazet@google.com> wrote:
> > > >
> > > >>
> > > >> Interesting, maybe a bnxt specific issue.
> > > >>
> > > >> It seems their model is to process TX/RX notification in the same queue,
> > > >> they throw away RX events if budget == 0
> > > >>
> > > >> It means commit e7b9569102995ebc26821789628eef45bd9840d8 is wrong and
> > > >> must be reverted.
> > > >>
> > > >> Otherwise, we have a possibility of blocking a queue under netpoll pressure.
> > > >
> > > > Hmm, actually a revert might not be enough, since code at lines 2030-2031
> > > > would fire and we might not call napi_complete_done() anyway.
> > > >
> > > > Unfortunately this driver logic is quite complex.
> > > >
> > > > Could you test on other NIC eventually ?
> > > >
> > >
> > > It actually runs OK on ixgbe.
> > >
> > > @Michael, could you please help us with this?
> > >
> > I've taken a quick look using today's net tree plus Eric's
> > poll_one_napi() patch.  The problem I'm seeing is that netpoll calls
> > bnxt_poll() with budget 0.  And since work_done >= budget of 0, we
> > return without calling napi_complete_done() and without arming the
> > interrupt.  netpoll doesn't always call us back until we call
> > napi_complete_done(), right?  So I think if there are in-flight TX
> > completions, we'll miss those.
>
> That's the whole point of netpoll :
>
>  We drain the TX queues, without interrupts being involved at all,
> by calling ->napi() with a zero budget.
>
> napi_complete(), even if called from ->napi() while budget was zero,
> should do nothing but return early.
>
> budget==0 means that ->napi() should process all TX completions.

All TX completions that we can see.  We cannot see the in-flight ones.

If budget is exceeded, I think the assumption is that poll will always
be called again.

>
> So it looks like bnxt has a bug, that is showing up after the latest
> poll_one_napi() patch.
> This latest patch is needed otherwise the cpu attempting the
> netpoll-TX-drain might drain nothing at all,
> since it does not anymore call ndo_poll_controller() that was grabbing
> SCHED bits on all queues (napi_schedule() like calls)

I think the latest patch is preventing the normal interrupt -> NAPI
path from coming in and cleaning the remaining TX completions and
arming the interrupt.

Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers

[Song Liu]

> On Sep 25, 2018, at 7:43 AM, Michael Chan <michael.chan@broadcom.com> wrote:
> 
> On Tue, Sep 25, 2018 at 7:20 AM Eric Dumazet <edumazet@google.com> wrote:
>> 
>> On Tue, Sep 25, 2018 at 7:02 AM Michael Chan <michael.chan@broadcom.com> wrote:
>>> 
>>> On Mon, Sep 24, 2018 at 2:18 PM Song Liu <songliubraving@fb.com> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On Sep 24, 2018, at 2:05 PM, Eric Dumazet <edumazet@google.com> wrote:
>>>>> 
>>>>>> 
>>>>>> Interesting, maybe a bnxt specific issue.
>>>>>> 
>>>>>> It seems their model is to process TX/RX notification in the same queue,
>>>>>> they throw away RX events if budget == 0
>>>>>> 
>>>>>> It means commit e7b9569102995ebc26821789628eef45bd9840d8 is wrong and
>>>>>> must be reverted.
>>>>>> 
>>>>>> Otherwise, we have a possibility of blocking a queue under netpoll pressure.
>>>>> 
>>>>> Hmm, actually a revert might not be enough, since code at lines 2030-2031
>>>>> would fire and we might not call napi_complete_done() anyway.
>>>>> 
>>>>> Unfortunately this driver logic is quite complex.
>>>>> 
>>>>> Could you test on other NIC eventually ?
>>>>> 
>>>> 
>>>> It actually runs OK on ixgbe.
>>>> 
>>>> @Michael, could you please help us with this?
>>>> 
>>> I've taken a quick look using today's net tree plus Eric's
>>> poll_one_napi() patch.  The problem I'm seeing is that netpoll calls
>>> bnxt_poll() with budget 0.  And since work_done >= budget of 0, we
>>> return without calling napi_complete_done() and without arming the
>>> interrupt.  netpoll doesn't always call us back until we call
>>> napi_complete_done(), right?  So I think if there are in-flight TX
>>> completions, we'll miss those.
>> 
>> That's the whole point of netpoll :
>> 
>> We drain the TX queues, without interrupts being involved at all,
>> by calling ->napi() with a zero budget.
>> 
>> napi_complete(), even if called from ->napi() while budget was zero,
>> should do nothing but return early.
>> 
>> budget==0 means that ->napi() should process all TX completions.
> 
> All TX completions that we can see.  We cannot see the in-flight ones.
> 
> If budget is exceeded, I think the assumption is that poll will always
> be called again.
> 
>> 
>> So it looks like bnxt has a bug, that is showing up after the latest
>> poll_one_napi() patch.
>> This latest patch is needed otherwise the cpu attempting the
>> netpoll-TX-drain might drain nothing at all,
>> since it does not anymore call ndo_poll_controller() that was grabbing
>> SCHED bits on all queues (napi_schedule() like calls)
> 
> I think the latest patch is preventing the normal interrupt -> NAPI
> path from coming in and cleaning the remaining TX completions and
> arming the interrupt.

Hi Michael, 

This may not be related. But I am looking at this:

bnxt_poll_work() {

	while (1) {
		....
		if (rx_pkts == budget)
			return
	}
}

With budget of 0, the loop will terminate after processing one packet. 
But I think the expectation is to finish all tx packets. So it doesn't
feel right. Could you please confirm?

Thanks,
Song

Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers

[Michael Chan]

On Tue, Sep 25, 2018 at 11:25 AM Song Liu <songliubraving@fb.com> wrote:

>
> Hi Michael,
>
> This may not be related. But I am looking at this:
>
> bnxt_poll_work() {
>
>         while (1) {
>                 ....
>                 if (rx_pkts == budget)
>                         return
>         }
> }
>
> With budget of 0, the loop will terminate after processing one packet.
> But I think the expectation is to finish all tx packets. So it doesn't
> feel right. Could you please confirm?
>

Right, this in effect is processing only 1 TX packet so it will be
inefficient at least.

But I think fixing it here still will not fix all the issues, because
even if we process all the TX packets here, we may still miss some
that are in flight.  When we exit poll, netpoll may not call us back
again and there may be no interrupts because we don't ARM the IRQ when
budget of 0 is reached.  I will send a test patch shortly for review
and testing.  Thanks.

Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers

[Song Liu]

> On Sep 24, 2018, at 8:30 AM, Eric Dumazet <edumazet@google.com> wrote:
> 
> On Sun, Sep 23, 2018 at 10:04 PM David Miller <davem@davemloft.net> wrote:
>> 
>> Series applied, thanks Eric.
> 
> Thanks David.
> 
> Song, would you please this additional patch ?
> 
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index 3219a2932463096566ce8ff336ecdf699422dd65..2ad45babe621b2c979ad5496b7df4342e4efbaa6
> 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -150,13 +150,6 @@ static void poll_one_napi(struct napi_struct *napi)
> {
>        int work = 0;
> 
> -       /* net_rx_action's ->poll() invocations and our's are
> -        * synchronized by this test which is only made while
> -        * holding the napi->poll_lock.
> -        */
> -       if (!test_bit(NAPI_STATE_SCHED, &napi->state))
> -               return;
> -
>        /* If we set this bit but see that it has already been set,
>         * that indicates that napi has been disabled and we need
>         * to abort this operation


Reviewed-and-tested-by: Song Liu <songliubraving@fb.com>