bonding: sync netpoll code with bridge

From: Amerigo Wang <[email protected]>
Date: Thu, 2 Dec 2010 21:34:44 +0800
Subject: [v3 PATCH 2/2] netpoll: remove IFF_IN_NETPOLL flag

This patch removes the flag IFF_IN_NETPOLL, we don't need it any more since we have netpoll_tx_running() now.

Signed-off-by WANG Cong <[email protected]>

Cc Neil Horman <[email protected]>

Cc Herbert Xu <[email protected]>

Cc Jay Vosburgh <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc Stephen Hemminger <[email protected]>

Cc Jiri Pirko <[email protected]>

Cc "Eric W. Biederman" <[email protected]>

---

 drivers/net/bonding/bond_main.c |    6 ++---- drivers/net/bonding/bonding.h   |    2 +- include/linux/if.h              |    9 ++++----- net/core/netpoll.c              |    2 --

 4 files changed, 7 insertions(+), 12 deletions(-)

drivers/net/bonding/bond_main.c +2 -4

index 7fafe06..21ac08b 100644
@@ -448,11 +448,9 @@ int bond_dev_queue_xmit(struct bonding *bond, struct sk_buff *skb,
        }
 
        skb->priority = 1;
-       if (unlikely(netpoll_tx_running(slave_dev))) {
-               slave_dev->priv_flags |= IFF_IN_NETPOLL;
+       if (unlikely(netpoll_tx_running(slave_dev)))
                bond_netpoll_send_skb(bond_get_slave_by_dev(bond, slave_dev), skb);
-               slave_dev->priv_flags &= ~IFF_IN_NETPOLL;
-       } else
+       else
                dev_queue_xmit(skb);
 
        return 0;

drivers/net/bonding/bonding.h +1 -1

index c4f6a94..493e645 100644
@@ -138,7 +138,7 @@ static inline void unblock_netpoll_tx(void)
 
  static inline int is_netpoll_tx_blocked(struct net_device *dev)
  {
-       if (unlikely(dev->priv_flags & IFF_IN_NETPOLL))
+       if (unlikely(netpoll_tx_running(dev)))
                return cpumask_test_cpu(smp_processor_id(), netpoll_block_tx);
        return 0;
  }

include/linux/if.h +0 -1

index 1239599..3bc63e6 100644
@@ -71,11 +71,10 @@
                                         * release skb->dst
                                         */
  #define IFF_DONT_BRIDGE 0x800          /* disallow bridging this ether dev */
-#define IFF_IN_NETPOLL 0x1000          /* whether we are processing netpoll */

You can't change the values of these macros or delete some of them, because they are exported to user space and are parts of ABI.

A few lines above that code said:

/* Private (from user) interface flags (netdevice->priv_flags). */

unless this comment is wrong, we don't need to worry.

Quoted 7 lines · Cong Wang

A few lines above that code said:

/* Private (from user) interface flags (netdevice->priv_flags). */

unless this comment is wrong, we don't need to worry.

It seems these macros are exported to user space wrongly.

From: Amerigo Wang <[email protected]>
Date: Thu, 2 Dec 2010 21:31:19 +0800
Subject: [v3 PATCH 1/2] bonding: sync netpoll code with bridge

V3: remove an useless #ifdef.

This patch unifies the netpoll code in bonding with netpoll code in bridge, thanks to Herbert that code is much cleaner now.

Signed-off-by WANG Cong <[email protected]>

Cc Neil Horman <[email protected]>

Cc Herbert Xu <[email protected]>

Cc Jay Vosburgh <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc Stephen Hemminger <[email protected]>

Cc Jiri Pirko <[email protected]>

Cc "Eric W. Biederman" <[email protected]>

---

 drivers/net/bonding/bond_main.c |  155 ++++++++++++++++++++++++--------------
 drivers/net/bonding/bonding.h   |   20 +++++
 2 files changed, 118 insertions(+), 57 deletions(-)

drivers/net/bonding/bond_main.c +5 -16

index 0273ad0..7fafe06 100644
@@ -59,7 +59,6 @@
 #include <linux/uaccess.h>
 #include <linux/errno.h>
 #include <linux/netdevice.h>
-#include <linux/netpoll.h>
 #include <linux/inetdevice.h>
 #include <linux/igmp.h>
 #include <linux/etherdevice.h>

@@ -449,15 +448,11 @@ int bond_dev_queue_xmit(struct bonding *bond, struct sk_buff *skb,
 	}
 
 	skb->priority = 1;
-#ifdef CONFIG_NET_POLL_CONTROLLER
-	if (unlikely(bond->dev->priv_flags & IFF_IN_NETPOLL)) {
-		struct netpoll *np = bond->dev->npinfo->netpoll;
-		slave_dev->npinfo = bond->dev->npinfo;
+	if (unlikely(netpoll_tx_running(slave_dev))) {
 		slave_dev->priv_flags |= IFF_IN_NETPOLL;
-		netpoll_send_skb_on_dev(np, skb, slave_dev);
+		bond_netpoll_send_skb(bond_get_slave_by_dev(bond, slave_dev), skb);
 		slave_dev->priv_flags &= ~IFF_IN_NETPOLL;
 	} else
-#endif
 		dev_queue_xmit(skb);
 
 	return 0;

@@ -1310,63 +1305,113 @@ static void bond_detach_slave(struct bonding *bond, struct slave *slave)
 }
 
 #ifdef CONFIG_NET_POLL_CONTROLLER
-/*
- * You must hold read lock on bond->lock before calling this.
- */
-static bool slaves_support_netpoll(struct net_device *bond_dev)
+static inline int slave_enable_netpoll(struct slave *slave)
 {
-	struct bonding *bond = netdev_priv(bond_dev);
-	struct slave *slave;
-	int i = 0;
-	bool ret = true;
+	struct netpoll *np;
+	int err = 0;
 
-	bond_for_each_slave(bond, slave, i) {

drivers/net/bonding/bond_main.c +29 -2

+	if (err) {
+		kfree(np);
+		goto out;
 	}
-	return i != 0 && ret;
+	slave->np = np;
+out:
+	return err;
+}
+static inline void slave_disable_netpoll(struct slave *slave)
+{
+	struct netpoll *np = slave->np;
+
+	if (!np)
+		return;
+
+	slave->np = NULL;
+	synchronize_rcu_bh();
+	__netpoll_cleanup(np);
+	kfree(np);
+}
+static inline bool slave_dev_support_netpoll(struct net_device *slave_dev)
+{
+	if (slave_dev->priv_flags & IFF_DISABLE_NETPOLL)
+		return false;
+	if (!slave_dev->netdev_ops->ndo_poll_controller)
+		return false;
+	return true;
 }
 
 static void bond_poll_controller(struct net_device *bond_dev)
 {
-	struct bonding *bond = netdev_priv(bond_dev);
+}
+
+static void __bond_netpoll_cleanup(struct bonding *bond)

drivers/net/bonding/bond_main.c +50 -32

+			slave_disable_netpoll(slave);
 }
-
 static void bond_netpoll_cleanup(struct net_device *bond_dev)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
+
+	read_lock(&bond->lock);
+	__bond_netpoll_cleanup(bond);
+	read_unlock(&bond->lock);
+}
+
+static int bond_netpoll_setup(struct net_device *dev, struct netpoll_info *ni)
+{
+	struct bonding *bond = netdev_priv(dev);
 	struct slave *slave;
-	const struct net_device_ops *ops;
-	int i;
+	int i, err = 0;
 
 	read_lock(&bond->lock);
-	bond_dev->npinfo = NULL;
 	bond_for_each_slave(bond, slave, i) {
-		if (slave->dev) {
-			ops = slave->dev->netdev_ops;
-			if (ops->ndo_netpoll_cleanup)
-				ops->ndo_netpoll_cleanup(slave->dev);
-			else
-				slave->dev->npinfo = NULL;
+		if (!slave->dev)
+			continue;
+		err = slave_enable_netpoll(slave);
+		if (err) {
+			__bond_netpoll_cleanup(bond);
+			break;
 		}
 	}
 	read_unlock(&bond->lock);
+	return err;
 }
 
-#else
+static struct netpoll_info *bond_netpoll_info(struct bonding *bond)
+{
+	return bond->dev->npinfo;
+}
 
+#else
+static inline int slave_enable_netpoll(struct slave *slave)
+{
+	return 0;
+}
+static inline void slave_disable_netpoll(struct slave *slave)
+{
+}
 static void bond_netpoll_cleanup(struct net_device *bond_dev)
 {
 }
-
+static int bond_netpoll_setup(struct net_device *dev, struct netpoll_info *ni)
+{
+	return 0;
+}
+static struct netpoll_info *bond_netpoll_info(struct bonding *bond)
+{
+	return NULL;
+}
 #endif
 
 /*---------------------------------- IOCTL ----------------------------------*/

@@ -1804,17 +1849,19 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 	bond_set_carrier(bond);
 
 #ifdef CONFIG_NET_POLL_CONTROLLER
-	if (slaves_support_netpoll(bond_dev)) {
-		bond_dev->priv_flags &= ~IFF_DISABLE_NETPOLL;
-		if (bond_dev->npinfo)
-			slave_dev->npinfo = bond_dev->npinfo;
-	} else if (!(bond_dev->priv_flags & IFF_DISABLE_NETPOLL)) {
-		bond_dev->priv_flags |= IFF_DISABLE_NETPOLL;
-		pr_info("New slave device %s does not support netpoll\n",
-			slave_dev->name);
-		pr_info("Disabling netpoll support for %s\n", bond_dev->name);
+	slave_dev->npinfo = bond_netpoll_info(bond);
+	if (slave_dev->npinfo) {
+		if (slave_enable_netpoll(new_slave)) {
+			read_unlock(&bond->lock);
+			pr_info("Error, %s: master_dev is using netpoll, "
+				 "but new slave device does not support netpoll.\n",
+				 bond_dev->name);
+			res = -EBUSY;
+			goto err_close;
+		}
 	}
 #endif
+
 	read_unlock(&bond->lock);
 
 	res = bond_create_slave_symlinks(bond_dev, slave_dev);

@@ -2016,17 +2063,7 @@ int bond_release(struct net_device *bond_dev, struct net_device *slave_dev)
 
 	netdev_set_master(slave_dev, NULL);
 
-#ifdef CONFIG_NET_POLL_CONTROLLER
-	read_lock_bh(&bond->lock);
-
-	if (slaves_support_netpoll(bond_dev))
-		bond_dev->priv_flags &= ~IFF_DISABLE_NETPOLL;
-	read_unlock_bh(&bond->lock);
-	if (slave_dev->netdev_ops->ndo_netpoll_cleanup)
-		slave_dev->netdev_ops->ndo_netpoll_cleanup(slave_dev);
-	else
-		slave_dev->npinfo = NULL;
-#endif
+	slave_disable_netpoll(slave);
 
 	/* close slave before restoring its mac address */
 	dev_close(slave_dev);

drivers/net/bonding/bond_main.c +3 -0

 		pr_info("%s: destroying bond %s.\n",
 			bond_dev->name, bond_dev->name);
 		unregister_netdevice(bond_dev);

@@ -2138,6 +2176,8 @@ static int bond_release_all(struct net_device *bond_dev)
 
 		netdev_set_master(slave_dev, NULL);
 
+		slave_disable_netpoll(slave);
+
 		/* close slave before restoring its mac address */
 		dev_close(slave_dev);
 

@@ -4670,6 +4710,7 @@ static const struct net_device_ops bond_netdev_ops = {
 	.ndo_vlan_rx_add_vid 	= bond_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid	= bond_vlan_rx_kill_vid,
 #ifdef CONFIG_NET_POLL_CONTROLLER
+	.ndo_netpoll_setup	= bond_netpoll_setup,
 	.ndo_netpoll_cleanup	= bond_netpoll_cleanup,
 	.ndo_poll_controller	= bond_poll_controller,
 #endif

drivers/net/bonding/bonding.h +20 -0

index ad3ae46..c4f6a94 100644
@@ -21,6 +21,7 @@
 #include <linux/kobject.h>
 #include <linux/cpumask.h>
 #include <linux/in6.h>
+#include <linux/netpoll.h>
 #include "bond_3ad.h"
 #include "bond_alb.h"
 

@@ -203,6 +204,9 @@ struct slave {
 	u16    queue_id;
 	struct ad_slave_info ad_info; /* HUGE - better to dynamically alloc */
 	struct tlb_slave_info tlb_info;
+#ifdef CONFIG_NET_POLL_CONTROLLER
+	struct netpoll *np;
+#endif
 };
 
 /*

@@ -324,6 +328,22 @@ static inline unsigned long slave_last_rx(struct bonding *bond,
 	return slave->dev->last_rx;
 }
 
+#ifdef CONFIG_NET_POLL_CONTROLLER
+static inline void bond_netpoll_send_skb(const struct slave *slave,
+					 struct sk_buff *skb)
+{
+	struct netpoll *np = slave->np;
+
+	if (np)
+		netpoll_send_skb(np, skb);
+}
+#else
+static inline void bond_netpoll_send_skb(const struct slave *slave,
+					 struct sk_buff *skb)
+{
+}
+#endif
+
 static inline void bond_set_slave_inactive_flags(struct slave *slave)
 {
 	struct bonding *bond = netdev_priv(slave->dev->master);

Signature · 1 line

1.7.1

drivers/net/bonding/bond_main.c static void bond_detach_slave(struct bonding *bond, struct slave *slave) · line 1319

Quoted 11 lines · Amerigo Wang

drivers/net/bonding/bond_main.c +7 -4

-	bond_for_each_slave(bond, slave, i) {
-		if ((slave->dev->priv_flags&  IFF_DISABLE_NETPOLL) ||
-		    !slave->dev->netdev_ops->ndo_poll_controller)
-			ret = false;
+	np = kmalloc(sizeof(*np), GFP_KERNEL);
+	err = -ENOMEM;
+	if (!np)
+		goto out;
+
+	np->dev = slave->dev;
+	err = __netpoll_setup(np);

Setting up our own netpoll instance on each slave worries me a bit. The

implication here is that, by doing so, some frames will get entirely processed by the slave. Most notably arp frames. That means anything that gets queued up to the arp_tx queue in __netpoll_rx will get processed during that poll event, and responded to with the mac of the slave device, rather than with the mac of the bond device, which isn't always what you want. I think if you go with this route, you'll need to add code to netpoll_poll_dev, right before the call to service_arp_queue, to check if IFF_SLAVE is set in priv_flags, and move the list to the master device, or some such.

It also seems like you'll want to zero out the other fields in the netpoll structure. Leaving garbage in them will be bad. Most notably here I'm looking at the rx_hook field. If its non-null we're going to add a bogus pointer to the rx_np list and call off into space at some point.

Quoted 5 lines

drivers/net/bonding/bond_main.c +2 -0

+static void __bond_netpoll_cleanup(struct bonding *bond)
+{
  	struct slave *slave;
  	int i;

drivers/net/bonding/bond_main.c static void bond_detach_slave(struct bonding *bond, struct slave *slave) · line 1359

Quoted 6 lines

drivers/net/bonding/bond_main.c +2 -4

-	bond_for_each_slave(bond, slave, i) {
-		if (slave->dev&&  IS_UP(slave->dev))
-			netpoll_poll_dev(slave->dev);
-	}
+	bond_for_each_slave(bond, slave, i)
+		if (slave->dev)

Why are you checking slave->dev here? If the dev pointer has been set to NULL here it would seem we're not holding on to dev long enough. If we enabled netpoll with a dev pointer and lost it somewhere along the way, we're going to leak that struct netpoll memory that we allocated.

drivers/net/bonding/bond_main.c static int bond_release_and_destroy(struct net_device *bond_dev, · line 2101

Quoted 8 lines

/* close slave before restoring its mac address */ dev_close(slave_dev);

drivers/net/bonding/bond_main.c +1 -0

@@ -2061,6 +2098,7 @@ static int  bond_release_and_destroy(struct net_device *bond_dev,
 
  	ret = bond_release(bond_dev, slave_dev);
  	if ((ret == 0)&&  (bond->slave_cnt == 0)) {
+		bond_dev->priv_flags |= IFF_DISABLE_NETPOLL;

Why are you setting IFF_DISABLE_NETPOLL here? That seems unnecessecary

Quoted 6 lines

Quoted 6 lines

      /* close slave before restoring its mac address */
      dev_close(slave_dev);

(quoted diff) +0 -0

@@ -2061,6 +2098,7 @@ static int  bond_release_and_destroy(struct
 net_device *bond_dev,
 

Quoted 5 lines

drivers/net/bonding/bond_main.c static int bond_release_and_destroy(struct net_device *bond_dev, · line 2101

Quoted 3 lines

drivers/net/bonding/bond_main.c +1 -0

      ret = bond_release(bond_dev, slave_dev);
      if ((ret == 0)&&  (bond->slave_cnt == 0)) {
+        bond_dev->priv_flags |= IFF_DISABLE_NETPOLL;

Why are you setting IFF_DISABLE_NETPOLL here? That seems unnecessecary

It gets removed in patch 2/2. :)

Quoted 12 lines · Neil Horman

drivers/net/bonding/bond_main.c static void bond_detach_slave(struct bonding *bond, struct slave *slave) · line 1319

Quoted 11 lines · Amerigo Wang

drivers/net/bonding/bond_main.c +7 -4

-    bond_for_each_slave(bond, slave, i) {
-        if ((slave->dev->priv_flags&  IFF_DISABLE_NETPOLL) ||
-            !slave->dev->netdev_ops->ndo_poll_controller)
-            ret = false;
+    np = kmalloc(sizeof(*np), GFP_KERNEL);
+    err = -ENOMEM;
+    if (!np)
+        goto out;
+
+    np->dev = slave->dev;
+    err = __netpoll_setup(np);

Quoted 9 lines

Setting up our own netpoll instance on each slave worries me a bit. The implication here is that, by doing so, some frames will get entirely processed by the slave. Most notably arp frames. That means anything that gets queued up to the arp_tx queue in __netpoll_rx will get processed during that poll event, and responded to with the mac of the slave device, rather than with the mac of the bond device, which isn't always what you want. I think if you go with this route, you'll need to add code to netpoll_poll_dev, right before the call to service_arp_queue, to check if IFF_SLAVE is set in priv_flags, and move the list to the master device, or some such.

Good point! Will fix i

Quoted 24 lines · Cong Wang

Quoted 21 lines · Neil Horman

drivers/net/bonding/bond_main.c static void bond_detach_slave(struct bonding *bond, struct slave *slave) · line 1319

Quoted 11 lines · Amerigo Wang

drivers/net/bonding/bond_main.c +7 -4

-    bond_for_each_slave(bond, slave, i) {
-        if ((slave->dev->priv_flags&  IFF_DISABLE_NETPOLL) ||
-            !slave->dev->netdev_ops->ndo_poll_controller)
-            ret = false;
+    np = kmalloc(sizeof(*np), GFP_KERNEL);
+    err = -ENOMEM;
+    if (!np)
+        goto out;
+
+    np->dev = slave->dev;
+    err = __netpoll_setup(np);

Setting up our own netpoll instance on each slave worries me a bit. The implication here is that, by doing so, some frames will get entirely processed by the slave. Most notably arp frames. That means anything that gets queued up to the arp_tx queue in __netpoll_rx will get processed during that poll event, and responded to with the mac of the slave device, rather than with the mac of the bond device, which isn't always what you want. I think if you go with this route, you'll need to add code to netpoll_poll_dev, right before the call to service_arp_queue, to check if IFF_SLAVE is set in priv_flags, and move the list to the master device, or some such.

Quoted 1 line

Good point! Will fix i

Hi, Neil,

I think we should do that in bond_poll_controller() rather than netpoll_poll_dev(), right? Since this is bond-specific. Does moving all arp_tx of slaves to their bond address your concern?

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/