[PATCH net-next-2.6 0/5] bonding: Refactor, fix, and updates

[PATCH net-next-2.6 0/5] bonding: Refactor, fix, and updates

[Jay Vosburgh]

	Five patches for bonding; these apply to net-next-2.6.

	Patch 1 is a refactor of the MII monitor, similar to the previous
refactor of the ARP active-backup monitor.  It replaces the monolithic
monitor function that uses conditional locking with a two phase (inspect
and commit) approach with strict locking (RTNL) required only for the
commit phase (which is only called when things actually change).  The long
term goal here is to ultimately consolidate all monitors within a generic
framework.

	Patch 2 makes a change to the Infiniband slave removal processing
to avoid a system crash when removing the final slave via sysfs.

	Patches 3 - 5 provide support for allowing slaves to receive
traffic independently from the master, and require some explanation.

	The goal of the last three patches is to permit slaves to receive
incoming traffic independently from the master; there are legitimate
reasons for wanting to do so, e.g., LLDP.  There are two ways to implement
this: a special case within bonding (skb_bond_should_drop) that would
require a hard-coded list of protocols to pass through, or a generic
method, that modifies the packet receive logic within netif_receive_skb.
The latter method is what is presented here.

	Please apply patches 1 - 2, and review and apply or provide
feedback for patches 3 - 5.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

[PATCH 1/5] bonding: refactor mii monitor

[Jay Vosburgh]

	Refactor mii monitor.  As with the previous ARP monitor refactor,
the motivation for this is to handle locking rationally (in this case,
removing conditional locking) and generally clean up the code.

	This patch breaks up the monolithic mii monitor into two phases:
an inspection phase, followed by an optional commit phase.  The commit phase
is the only portion that requires RTNL or makes changes to state, and is
only called when inspection finds something to change.

Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
---
 drivers/net/bonding/bond_3ad.c  |    1 +
 drivers/net/bonding/bond_main.c |  394 +++++++++++++++++----------------------
 2 files changed, 173 insertions(+), 222 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index ebb539e..6106660 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -2107,6 +2107,7 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
 			aggregator = __get_first_agg(port);
 			ad_agg_selection_logic(aggregator);
 		}
+		bond_3ad_set_carrier(bond);
 	}
 
 	// for each port run the state machines
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index d57b65d..6ee6db0 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2208,272 +2208,217 @@ static int bond_slave_info_query(struct net_device *bond_dev, struct ifslave *in
 
 /*-------------------------------- Monitoring -------------------------------*/
 
-/*
- * if !have_locks, return nonzero if a failover is necessary.  if
- * have_locks, do whatever failover activities are needed.
- *
- * This is to separate the inspection and failover steps for locking
- * purposes; failover requires rtnl, but acquiring it for every
- * inspection is undesirable, so a wrapper first does inspection, and
- * the acquires the necessary locks and calls again to perform
- * failover if needed.  Since all locks are dropped, a complete
- * restart is needed between calls.
- */
-static int __bond_mii_monitor(struct bonding *bond, int have_locks)
-{
-	struct slave *slave, *oldcurrent;
-	int do_failover = 0;
-	int i;
-
-	if (bond->slave_cnt == 0)
-		goto out;
 
-	/* we will try to read the link status of each of our slaves, and
-	 * set their IFF_RUNNING flag appropriately. For each slave not
-	 * supporting MII status, we won't do anything so that a user-space
-	 * program could monitor the link itself if needed.
-	 */
-
-	read_lock(&bond->curr_slave_lock);
-	oldcurrent = bond->curr_active_slave;
-	read_unlock(&bond->curr_slave_lock);
+static int bond_miimon_inspect(struct bonding *bond)
+{
+	struct slave *slave;
+	int i, link_state, commit = 0;
 
 	bond_for_each_slave(bond, slave, i) {
-		struct net_device *slave_dev = slave->dev;
-		int link_state;
-		u16 old_speed = slave->speed;
-		u8 old_duplex = slave->duplex;
+		slave->new_link = BOND_LINK_NOCHANGE;
 
-		link_state = bond_check_dev_link(bond, slave_dev, 0);
+		link_state = bond_check_dev_link(bond, slave->dev, 0);
 
 		switch (slave->link) {
-		case BOND_LINK_UP:	/* the link was up */
-			if (link_state == BMSR_LSTATUS) {
-				if (!oldcurrent) {
-					if (!have_locks)
-						return 1;
-					do_failover = 1;
-				}
-				break;
-			} else { /* link going down */
-				slave->link  = BOND_LINK_FAIL;
-				slave->delay = bond->params.downdelay;
-
-				if (slave->link_failure_count < UINT_MAX) {
-					slave->link_failure_count++;
-				}
+		case BOND_LINK_UP:
+			if (link_state)
+				continue;
 
-				if (bond->params.downdelay) {
-					printk(KERN_INFO DRV_NAME
-					       ": %s: link status down for %s "
-					       "interface %s, disabling it in "
-					       "%d ms.\n",
-					       bond->dev->name,
-					       IS_UP(slave_dev)
-					       ? ((bond->params.mode == BOND_MODE_ACTIVEBACKUP)
-						  ? ((slave == oldcurrent)
-						     ? "active " : "backup ")
-						  : "")
-					       : "idle ",
-					       slave_dev->name,
-					       bond->params.downdelay * bond->params.miimon);
-				}
+			slave->link = BOND_LINK_FAIL;
+			slave->delay = bond->params.downdelay;
+			if (slave->delay) {
+				printk(KERN_INFO DRV_NAME
+				       ": %s: link status down for %s"
+				       "interface %s, disabling it in %d ms.\n",
+				       bond->dev->name,
+				       (bond->params.mode ==
+					BOND_MODE_ACTIVEBACKUP) ?
+				       ((slave->state == BOND_STATE_ACTIVE) ?
+					"active " : "backup ") : "",
+				       slave->dev->name,
+				       bond->params.downdelay * bond->params.miimon);
 			}
-			/* no break ! fall through the BOND_LINK_FAIL test to
-			   ensure proper action to be taken
-			*/
-		case BOND_LINK_FAIL:	/* the link has just gone down */
-			if (link_state != BMSR_LSTATUS) {
-				/* link stays down */
-				if (slave->delay <= 0) {
-					if (!have_locks)
-						return 1;
-
-					/* link down for too long time */
-					slave->link = BOND_LINK_DOWN;
-
-					/* in active/backup mode, we must
-					 * completely disable this interface
-					 */
-					if ((bond->params.mode == BOND_MODE_ACTIVEBACKUP) ||
-					    (bond->params.mode == BOND_MODE_8023AD)) {
-						bond_set_slave_inactive_flags(slave);
-					}
-
-					printk(KERN_INFO DRV_NAME
-					       ": %s: link status definitely "
-					       "down for interface %s, "
-					       "disabling it\n",
-					       bond->dev->name,
-					       slave_dev->name);
-
-					/* notify ad that the link status has changed */
-					if (bond->params.mode == BOND_MODE_8023AD) {
-						bond_3ad_handle_link_change(slave, BOND_LINK_DOWN);
-					}
-
-					if ((bond->params.mode == BOND_MODE_TLB) ||
-					    (bond->params.mode == BOND_MODE_ALB)) {
-						bond_alb_handle_link_change(bond, slave, BOND_LINK_DOWN);
-					}
-
-					if (slave == oldcurrent) {
-						do_failover = 1;
-					}
-				} else {
-					slave->delay--;
-				}
-			} else {
-				/* link up again */
-				slave->link  = BOND_LINK_UP;
+			/*FALLTHRU*/
+		case BOND_LINK_FAIL:
+			if (link_state) {
+				/*
+				 * recovered before downdelay expired
+				 */
+				slave->link = BOND_LINK_UP;
 				slave->jiffies = jiffies;
 				printk(KERN_INFO DRV_NAME
 				       ": %s: link status up again after %d "
 				       "ms for interface %s.\n",
 				       bond->dev->name,
-				       (bond->params.downdelay - slave->delay) * bond->params.miimon,
-				       slave_dev->name);
+				       (bond->params.downdelay - slave->delay) *
+				       bond->params.miimon,
+				       slave->dev->name);
+				continue;
 			}
-			break;
-		case BOND_LINK_DOWN:	/* the link was down */
-			if (link_state != BMSR_LSTATUS) {
-				/* the link stays down, nothing more to do */
-				break;
-			} else {	/* link going up */
-				slave->link  = BOND_LINK_BACK;
-				slave->delay = bond->params.updelay;
 
-				if (bond->params.updelay) {
-					/* if updelay == 0, no need to
-					   advertise about a 0 ms delay */
-					printk(KERN_INFO DRV_NAME
-					       ": %s: link status up for "
-					       "interface %s, enabling it "
-					       "in %d ms.\n",
-					       bond->dev->name,
-					       slave_dev->name,
-					       bond->params.updelay * bond->params.miimon);
-				}
+			if (slave->delay <= 0) {
+				slave->new_link = BOND_LINK_DOWN;
+				commit++;
+				continue;
 			}
-			/* no break ! fall through the BOND_LINK_BACK state in
-			   case there's something to do.
-			*/
-		case BOND_LINK_BACK:	/* the link has just come back */
-			if (link_state != BMSR_LSTATUS) {
-				/* link down again */
-				slave->link  = BOND_LINK_DOWN;
 
+			slave->delay--;
+			break;
+
+		case BOND_LINK_DOWN:
+			if (!link_state)
+				continue;
+
+			slave->link = BOND_LINK_BACK;
+			slave->delay = bond->params.updelay;
+
+			if (slave->delay) {
+				printk(KERN_INFO DRV_NAME
+				       ": %s: link status up for "
+				       "interface %s, enabling it in %d ms.\n",
+				       bond->dev->name, slave->dev->name,
+				       bond->params.updelay *
+				       bond->params.miimon);
+			}
+			/*FALLTHRU*/
+		case BOND_LINK_BACK:
+			if (!link_state) {
+				slave->link = BOND_LINK_DOWN;
 				printk(KERN_INFO DRV_NAME
 				       ": %s: link status down again after %d "
 				       "ms for interface %s.\n",
 				       bond->dev->name,
-				       (bond->params.updelay - slave->delay) * bond->params.miimon,
-				       slave_dev->name);
-			} else {
-				/* link stays up */
-				if (slave->delay == 0) {
-					if (!have_locks)
-						return 1;
-
-					/* now the link has been up for long time enough */
-					slave->link = BOND_LINK_UP;
-					slave->jiffies = jiffies;
-
-					if (bond->params.mode == BOND_MODE_8023AD) {
-						/* prevent it from being the active one */
-						slave->state = BOND_STATE_BACKUP;
-					} else if (bond->params.mode != BOND_MODE_ACTIVEBACKUP) {
-						/* make it immediately active */
-						slave->state = BOND_STATE_ACTIVE;
-					} else if (slave != bond->primary_slave) {
-						/* prevent it from being the active one */
-						slave->state = BOND_STATE_BACKUP;
-					}
+				       (bond->params.updelay - slave->delay) *
+				       bond->params.miimon,
+				       slave->dev->name);
 
-					printk(KERN_INFO DRV_NAME
-					       ": %s: link status definitely "
-					       "up for interface %s.\n",
-					       bond->dev->name,
-					       slave_dev->name);
-
-					/* notify ad that the link status has changed */
-					if (bond->params.mode == BOND_MODE_8023AD) {
-						bond_3ad_handle_link_change(slave, BOND_LINK_UP);
-					}
-
-					if ((bond->params.mode == BOND_MODE_TLB) ||
-					    (bond->params.mode == BOND_MODE_ALB)) {
-						bond_alb_handle_link_change(bond, slave, BOND_LINK_UP);
-					}
-
-					if ((!oldcurrent) ||
-					    (slave == bond->primary_slave)) {
-						do_failover = 1;
-					}
-				} else {
-					slave->delay--;
-				}
+				continue;
 			}
+
+			if (slave->delay <= 0) {
+				slave->new_link = BOND_LINK_UP;
+				commit++;
+				continue;
+			}
+
+			slave->delay--;
 			break;
-		default:
-			/* Should not happen */
-			printk(KERN_ERR DRV_NAME
-			       ": %s: Error: %s Illegal value (link=%d)\n",
-			       bond->dev->name,
-			       slave->dev->name,
-			       slave->link);
-			goto out;
-		} /* end of switch (slave->link) */
+		}
+	}
 
-		bond_update_speed_duplex(slave);
+	return commit;
+}
 
-		if (bond->params.mode == BOND_MODE_8023AD) {
-			if (old_speed != slave->speed) {
-				bond_3ad_adapter_speed_changed(slave);
-			}
+static void bond_miimon_commit(struct bonding *bond)
+{
+	struct slave *slave;
+	int i;
+
+	bond_for_each_slave(bond, slave, i) {
+		switch (slave->new_link) {
+		case BOND_LINK_NOCHANGE:
+			continue;
+
+		case BOND_LINK_UP:
+			slave->link = BOND_LINK_UP;
+			slave->jiffies = jiffies;
 
-			if (old_duplex != slave->duplex) {
-				bond_3ad_adapter_duplex_changed(slave);
+			if (bond->params.mode == BOND_MODE_8023AD) {
+				/* prevent it from being the active one */
+				slave->state = BOND_STATE_BACKUP;
+			} else if (bond->params.mode != BOND_MODE_ACTIVEBACKUP) {
+				/* make it immediately active */
+				slave->state = BOND_STATE_ACTIVE;
+			} else if (slave != bond->primary_slave) {
+				/* prevent it from being the active one */
+				slave->state = BOND_STATE_BACKUP;
 			}
-		}
 
-	} /* end of for */
+			printk(KERN_INFO DRV_NAME
+			       ": %s: link status definitely "
+			       "up for interface %s.\n",
+			       bond->dev->name, slave->dev->name);
 
-	if (do_failover) {
-		ASSERT_RTNL();
+			/* notify ad that the link status has changed */
+			if (bond->params.mode == BOND_MODE_8023AD)
+				bond_3ad_handle_link_change(slave, BOND_LINK_UP);
 
-		write_lock_bh(&bond->curr_slave_lock);
+			if ((bond->params.mode == BOND_MODE_TLB) ||
+			    (bond->params.mode == BOND_MODE_ALB))
+				bond_alb_handle_link_change(bond, slave,
+							    BOND_LINK_UP);
 
-		bond_select_active_slave(bond);
+			if (!bond->curr_active_slave ||
+			    (slave == bond->primary_slave))
+				goto do_failover;
 
-		write_unlock_bh(&bond->curr_slave_lock);
+			continue;
 
-	} else
-		bond_set_carrier(bond);
+		case BOND_LINK_DOWN:
+			slave->link = BOND_LINK_DOWN;
 
-out:
-	return 0;
+			if (bond->params.mode == BOND_MODE_ACTIVEBACKUP ||
+			    bond->params.mode == BOND_MODE_8023AD)
+				bond_set_slave_inactive_flags(slave);
+
+			printk(KERN_INFO DRV_NAME
+			       ": %s: link status definitely down for "
+			       "interface %s, disabling it\n",
+			       bond->dev->name, slave->dev->name);
+
+			if (bond->params.mode == BOND_MODE_8023AD)
+				bond_3ad_handle_link_change(slave,
+							    BOND_LINK_DOWN);
+
+			if (bond->params.mode == BOND_MODE_TLB ||
+			    bond->params.mode == BOND_MODE_ALB)
+				bond_alb_handle_link_change(bond, slave,
+							    BOND_LINK_DOWN);
+
+			if (slave == bond->curr_active_slave)
+				goto do_failover;
+
+			continue;
+
+		default:
+			printk(KERN_ERR DRV_NAME
+			       ": %s: invalid new link %d on slave %s\n",
+			       bond->dev->name, slave->new_link,
+			       slave->dev->name);
+			slave->new_link = BOND_LINK_NOCHANGE;
+
+			continue;
+		}
+
+do_failover:
+		ASSERT_RTNL();
+		write_lock_bh(&bond->curr_slave_lock);
+		bond_select_active_slave(bond);
+		write_unlock_bh(&bond->curr_slave_lock);
+	}
+
+	bond_set_carrier(bond);
 }
 
 /*
  * bond_mii_monitor
  *
  * Really a wrapper that splits the mii monitor into two phases: an
- * inspection, then (if inspection indicates something needs to be
- * done) an acquisition of appropriate locks followed by another pass
- * to implement whatever link state changes are indicated.
+ * inspection, then (if inspection indicates something needs to be done)
+ * an acquisition of appropriate locks followed by a commit phase to
+ * implement whatever link state changes are indicated.
  */
 void bond_mii_monitor(struct work_struct *work)
 {
 	struct bonding *bond = container_of(work, struct bonding,
 					    mii_work.work);
-	unsigned long delay;
 
 	read_lock(&bond->lock);
-	if (bond->kill_timers) {
-		read_unlock(&bond->lock);
-		return;
-	}
+	if (bond->kill_timers)
+		goto out;
+
+	if (bond->slave_cnt == 0)
+		goto re_arm;
 
 	if (bond->send_grat_arp) {
 		read_lock(&bond->curr_slave_lock);
@@ -2481,19 +2426,24 @@ void bond_mii_monitor(struct work_struct *work)
 		read_unlock(&bond->curr_slave_lock);
 	}
 
-	if (__bond_mii_monitor(bond, 0)) {
+	if (bond_miimon_inspect(bond)) {
 		read_unlock(&bond->lock);
 		rtnl_lock();
 		read_lock(&bond->lock);
-		__bond_mii_monitor(bond, 1);
+
+		bond_miimon_commit(bond);
+
 		read_unlock(&bond->lock);
 		rtnl_unlock();	/* might sleep, hold no other locks */
 		read_lock(&bond->lock);
 	}
 
-	delay = msecs_to_jiffies(bond->params.miimon);
+re_arm:
+	if (bond->params.miimon)
+		queue_delayed_work(bond->wq, &bond->mii_work,
+				   msecs_to_jiffies(bond->params.miimon));
+out:
 	read_unlock(&bond->lock);
-	queue_delayed_work(bond->wq, &bond->mii_work, delay);
 }
 
 static __be32 bond_glean_dev_ip(struct net_device *dev)
-- 
1.5.2.4

[PATCH 2/5] bonding: Don't destroy bonding master when removing slave via sysfs

[Jay Vosburgh]

From: Moni Shoua <monis@voltaire.com>

It is wrong to destroy a bonding master from a context that uses the sysfs
of that bond. When last IPoIB slave is unenslaved from by writing to a
sysfs file (for bond0 this would be /sys/class/net/bond0/bonding/slaves)
the driver tries to destroy the bond. This is wrong and can lead to a
lockup or a crash.  This fix lets the bonding master stay and relies on
the user to destroy the bonding master if necessary (i.e. before module
ib_ipoib is unloaded)

This patch affects only bonds of IPoIB slaves. Ethernet slaves stay
unaffected.

Signed-off-by: Moni Shoua <monis@voltaire.com>
Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
---
 drivers/net/bonding/bond_sysfs.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 6caac0f..3bdb473 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -350,9 +350,6 @@ static ssize_t bonding_store_slaves(struct device *d,
 		if (dev) {
 			printk(KERN_INFO DRV_NAME ": %s: Removing slave %s\n",
 				bond->dev->name, dev->name);
-			if (bond->setup_by_slave)
-				res = bond_release_and_destroy(bond->dev, dev);
-			else
 				res = bond_release(bond->dev, dev);
 			if (res) {
 				ret = res;
-- 
1.5.2.4

[PATCH 3/5] net/core: Uninline skb_bond().

[Jay Vosburgh]

From: Joe Eykholt <jre@nuovasystems.com>

Otherwise subsequent changes need multiple return values.

Signed-off-by: Joe Eykholt <jre@nuovasystems.com>
Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
---
 net/core/dev.c |   28 ++++++++--------------------
 1 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index f6944ec..78114f6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1872,22 +1872,6 @@ int netif_rx_ni(struct sk_buff *skb)
 
 EXPORT_SYMBOL(netif_rx_ni);
 
-static inline struct net_device *skb_bond(struct sk_buff *skb)
-{
-	struct net_device *dev = skb->dev;
-
-	if (dev->master) {
-		if (skb_bond_should_drop(skb)) {
-			kfree_skb(skb);
-			return NULL;
-		}
-		skb->dev = dev->master;
-	}
-
-	return dev;
-}
-
-
 static void net_tx_action(struct softirq_action *h)
 {
 	struct softnet_data *sd = &__get_cpu_var(softnet_data);
@@ -2092,10 +2076,14 @@ int netif_receive_skb(struct sk_buff *skb)
 	if (!skb->iif)
 		skb->iif = skb->dev->ifindex;
 
-	orig_dev = skb_bond(skb);
-
-	if (!orig_dev)
-		return NET_RX_DROP;
+	orig_dev = skb->dev;
+	if (orig_dev->master) {
+		if (skb_bond_should_drop(skb)) {
+			kfree_skb(skb);
+			return NET_RX_DROP;
+		}
+		skb->dev = orig_dev->master;
+	}
 
 	__get_cpu_var(netdev_rx_stat).total++;
 
-- 
1.5.2.4

[PATCH 4/5] net/core: Allow certain receives on inactive slave.

[Jay Vosburgh]

From: Joe Eykholt <jre@nuovasystems.com>

Allow a packet_type that specifies the exact device to receive
even on an inactive bonding slave devices.  This is important for some
L2 protocols such as LLDP and FCoE.  This can eventually be used
for the bonding special cases as well.

Signed-off-by: Joe Eykholt <jre@nuovasystems.com>
Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
---
 net/core/dev.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 78114f6..a949664 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2063,6 +2063,7 @@ int netif_receive_skb(struct sk_buff *skb)
 {
 	struct packet_type *ptype, *pt_prev;
 	struct net_device *orig_dev;
+	struct net_device *null_or_orig;
 	int ret = NET_RX_DROP;
 	__be16 type;
 
@@ -2076,13 +2077,13 @@ int netif_receive_skb(struct sk_buff *skb)
 	if (!skb->iif)
 		skb->iif = skb->dev->ifindex;
 
+	null_or_orig = NULL;
 	orig_dev = skb->dev;
 	if (orig_dev->master) {
-		if (skb_bond_should_drop(skb)) {
-			kfree_skb(skb);
-			return NET_RX_DROP;
-		}
-		skb->dev = orig_dev->master;
+		if (skb_bond_should_drop(skb))
+			null_or_orig = orig_dev; /* deliver only exact match */
+		else
+			skb->dev = orig_dev->master;
 	}
 
 	__get_cpu_var(netdev_rx_stat).total++;
@@ -2103,7 +2104,7 @@ int netif_receive_skb(struct sk_buff *skb)
 #endif
 
 	list_for_each_entry_rcu(ptype, &ptype_all, list) {
-		if (!ptype->dev || ptype->dev == skb->dev) {
+		if (ptype->dev == null_or_orig || ptype->dev == skb->dev) {
 			if (pt_prev)
 				ret = deliver_skb(skb, pt_prev, orig_dev);
 			pt_prev = ptype;
@@ -2128,7 +2129,7 @@ ncls:
 	list_for_each_entry_rcu(ptype,
 			&ptype_base[ntohs(type) & PTYPE_HASH_MASK], list) {
 		if (ptype->type == type &&
-		    (!ptype->dev || ptype->dev == skb->dev)) {
+		    (ptype->dev == null_or_orig || ptype->dev == skb->dev)) {
 			if (pt_prev)
 				ret = deliver_skb(skb, pt_prev, orig_dev);
 			pt_prev = ptype;
-- 
1.5.2.4

[PATCH 5/5] net/core: Allow receive on active slaves.

[Jay Vosburgh]

From: Joe Eykholt <jre@nuovasystems.com>

If a packet_type specifies an active slave to bonding and not just any
interface, allow it to receive frames that came in on that interface.

Signed-off-by: Joe Eykholt <jre@nuovasystems.com>
Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
---
 net/core/dev.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index a949664..d68f662 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2104,7 +2104,8 @@ int netif_receive_skb(struct sk_buff *skb)
 #endif
 
 	list_for_each_entry_rcu(ptype, &ptype_all, list) {
-		if (ptype->dev == null_or_orig || ptype->dev == skb->dev) {
+		if (ptype->dev == null_or_orig || ptype->dev == skb->dev ||
+		    ptype->dev == orig_dev) {
 			if (pt_prev)
 				ret = deliver_skb(skb, pt_prev, orig_dev);
 			pt_prev = ptype;
@@ -2129,7 +2130,8 @@ ncls:
 	list_for_each_entry_rcu(ptype,
 			&ptype_base[ntohs(type) & PTYPE_HASH_MASK], list) {
 		if (ptype->type == type &&
-		    (ptype->dev == null_or_orig || ptype->dev == skb->dev)) {
+		    (ptype->dev == null_or_orig || ptype->dev == skb->dev ||
+		     ptype->dev == orig_dev)) {
 			if (pt_prev)
 				ret = deliver_skb(skb, pt_prev, orig_dev);
 			pt_prev = ptype;
-- 
1.5.2.4

Re: [PATCH 5/5] net/core: Allow receive on active slaves.

[Jeff Garzik]

Jay Vosburgh wrote:
> From: Joe Eykholt <jre@nuovasystems.com>
> 
> If a packet_type specifies an active slave to bonding and not just any
> interface, allow it to receive frames that came in on that interface.
> 
> Signed-off-by: Joe Eykholt <jre@nuovasystems.com>
> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
> ---
>  net/core/dev.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index a949664..d68f662 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2104,7 +2104,8 @@ int netif_receive_skb(struct sk_buff *skb)
>  #endif
>  
>  	list_for_each_entry_rcu(ptype, &ptype_all, list) {
> -		if (ptype->dev == null_or_orig || ptype->dev == skb->dev) {
> +		if (ptype->dev == null_or_orig || ptype->dev == skb->dev ||
> +		    ptype->dev == orig_dev) {
>  			if (pt_prev)
>  				ret = deliver_skb(skb, pt_prev, orig_dev);
>  			pt_prev = ptype;
> @@ -2129,7 +2130,8 @@ ncls:
>  	list_for_each_entry_rcu(ptype,
>  			&ptype_base[ntohs(type) & PTYPE_HASH_MASK], list) {
>  		if (ptype->type == type &&
> -		    (ptype->dev == null_or_orig || ptype->dev == skb->dev)) {
> +		    (ptype->dev == null_or_orig || ptype->dev == skb->dev ||
> +		     ptype->dev == orig_dev)) {
>  			if (pt_prev)

This needs a review&ack from David, then OK...

Re: [PATCH 5/5] net/core: Allow receive on active slaves.

[David Miller]

From: Jeff Garzik <jgarzik@pobox.com>
Date: Fri, 04 Jul 2008 08:45:52 -0400

> Jay Vosburgh wrote:
> > @@ -2104,7 +2104,8 @@ int netif_receive_skb(struct sk_buff *skb)
> >  #endif
> >  
> >  	list_for_each_entry_rcu(ptype, &ptype_all, list) {
> > -		if (ptype->dev == null_or_orig || ptype->dev == skb->dev) {
> > +		if (ptype->dev == null_or_orig || ptype->dev == skb->dev ||
> > +		    ptype->dev == orig_dev) {
> >  			if (pt_prev)
> >  				ret = deliver_skb(skb, pt_prev, orig_dev);
> >  			pt_prev = ptype;
> > @@ -2129,7 +2130,8 @@ ncls:
> >  	list_for_each_entry_rcu(ptype,
> >  			&ptype_base[ntohs(type) & PTYPE_HASH_MASK], list) {
> >  		if (ptype->type == type &&
> > -		    (ptype->dev == null_or_orig || ptype->dev == skb->dev)) {
> > +		    (ptype->dev == null_or_orig || ptype->dev == skb->dev ||
> > +		     ptype->dev == orig_dev)) {
> >  			if (pt_prev)
> 
> This needs a review&ack from David, then OK...

I fear this bit of the changes will break AF_PACKET device binding.
So for example, if a tap registers it wants to hear eth0, it will now
hear bond0 as well as eth0 if eth0 is a part of bond0.

Re: [PATCH 5/5] net/core: Allow receive on active slaves.

[Joe Eykholt]

David Miller wrote:
> From: Jeff Garzik <jgarzik@pobox.com>
> Date: Fri, 04 Jul 2008 08:45:52 -0400
> 
>> Jay Vosburgh wrote:
>>> @@ -2104,7 +2104,8 @@ int netif_receive_skb(struct sk_buff *skb)
>>>  #endif
>>>  
>>>  	list_for_each_entry_rcu(ptype, &ptype_all, list) {
>>> -		if (ptype->dev == null_or_orig || ptype->dev == skb->dev) {
>>> +		if (ptype->dev == null_or_orig || ptype->dev == skb->dev ||
>>> +		    ptype->dev == orig_dev) {
>>>  			if (pt_prev)
>>>  				ret = deliver_skb(skb, pt_prev, orig_dev);
>>>  			pt_prev = ptype;
>>> @@ -2129,7 +2130,8 @@ ncls:
>>>  	list_for_each_entry_rcu(ptype,
>>>  			&ptype_base[ntohs(type) & PTYPE_HASH_MASK], list) {
>>>  		if (ptype->type == type &&
>>> -		    (ptype->dev == null_or_orig || ptype->dev == skb->dev)) {
>>> +		    (ptype->dev == null_or_orig || ptype->dev == skb->dev ||
>>> +		     ptype->dev == orig_dev)) {
>>>  			if (pt_prev)
>> This needs a review&ack from David, then OK...
> 
> I fear this bit of the changes will break AF_PACKET device binding.
> So for example, if a tap registers it wants to hear eth0, it will now
> hear bond0 as well as eth0 if eth0 is a part of bond0.

Each ptype will get matched and delivered to at most once.  So if 
someone binds AF_PACKET to only eth0, they'll only see the packet once.
Whereas before patch 4, the frame wouldn't be delivered to them
at all, since skb->dev has already been changed to the bond.

       Regards,
       Joe Eykholt

Re: [PATCH 5/5] net/core: Allow receive on active slaves.

[David Miller]

From: Joe Eykholt <jre@nuovasystems.com>
Date: Sat, 05 Jul 2008 21:36:42 -0700

> David Miller wrote:
> > From: Jeff Garzik <jgarzik@pobox.com>
> > Date: Fri, 04 Jul 2008 08:45:52 -0400
> > 
> >> Jay Vosburgh wrote:
> >>> @@ -2104,7 +2104,8 @@ int netif_receive_skb(struct sk_buff *skb)
> >>>  #endif
> >>>  
> >>>  	list_for_each_entry_rcu(ptype, &ptype_all, list) {
> >>> -		if (ptype->dev == null_or_orig || ptype->dev == skb->dev) {
> >>> +		if (ptype->dev == null_or_orig || ptype->dev == skb->dev ||
> >>> +		    ptype->dev == orig_dev) {
> >>>  			if (pt_prev)
> >>>  				ret = deliver_skb(skb, pt_prev, orig_dev);
> >>>  			pt_prev = ptype;
> >>> @@ -2129,7 +2130,8 @@ ncls:
> >>>  	list_for_each_entry_rcu(ptype,
> >>>  			&ptype_base[ntohs(type) & PTYPE_HASH_MASK], list) {
> >>>  		if (ptype->type == type &&
> >>> -		    (ptype->dev == null_or_orig || ptype->dev == skb->dev)) {
> >>> +		    (ptype->dev == null_or_orig || ptype->dev == skb->dev ||
> >>> +		     ptype->dev == orig_dev)) {
> >>>  			if (pt_prev)
> >> This needs a review&ack from David, then OK...
> > 
> > I fear this bit of the changes will break AF_PACKET device binding.
> > So for example, if a tap registers it wants to hear eth0, it will now
> > hear bond0 as well as eth0 if eth0 is a part of bond0.
> 
> Each ptype will get matched and delivered to at most once.  So if 
> someone binds AF_PACKET to only eth0, they'll only see the packet once.
> Whereas before patch 4, the frame wouldn't be delivered to them
> at all, since skb->dev has already been changed to the bond.

Ok, that works.

Jeff, I ACK this patch and the others that go with it:

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH 5/5] net/core: Allow receive on active slaves.

[Joe Eykholt]

David Miller wrote:
> From: Joe Eykholt <jre@nuovasystems.com>
> Date: Sat, 05 Jul 2008 21:36:42 -0700
> 
>> David Miller wrote:
>>> From: Jeff Garzik <jgarzik@pobox.com>
>>> Date: Fri, 04 Jul 2008 08:45:52 -0400
>>>
>>>> Jay Vosburgh wrote:
>>>>> @@ -2104,7 +2104,8 @@ int netif_receive_skb(struct sk_buff *skb)
>>>>>  #endif
>>>>>  
>>>>>  	list_for_each_entry_rcu(ptype, &ptype_all, list) {
>>>>> -		if (ptype->dev == null_or_orig || ptype->dev == skb->dev) {
>>>>> +		if (ptype->dev == null_or_orig || ptype->dev == skb->dev ||
>>>>> +		    ptype->dev == orig_dev) {
>>>>>  			if (pt_prev)
>>>>>  				ret = deliver_skb(skb, pt_prev, orig_dev);
>>>>>  			pt_prev = ptype;
>>>>> @@ -2129,7 +2130,8 @@ ncls:
>>>>>  	list_for_each_entry_rcu(ptype,
>>>>>  			&ptype_base[ntohs(type) & PTYPE_HASH_MASK], list) {
>>>>>  		if (ptype->type == type &&
>>>>> -		    (ptype->dev == null_or_orig || ptype->dev == skb->dev)) {
>>>>> +		    (ptype->dev == null_or_orig || ptype->dev == skb->dev ||
>>>>> +		     ptype->dev == orig_dev)) {
>>>>>  			if (pt_prev)
>>>> This needs a review&ack from David, then OK...
>>> I fear this bit of the changes will break AF_PACKET device binding.
>>> So for example, if a tap registers it wants to hear eth0, it will now
>>> hear bond0 as well as eth0 if eth0 is a part of bond0.
>> Each ptype will get matched and delivered to at most once.  So if 
>> someone binds AF_PACKET to only eth0, they'll only see the packet once.
>> Whereas before patch 4, the frame wouldn't be delivered to them
>> at all, since skb->dev has already been changed to the bond.
> 
> Ok, that works.
> 
> Jeff, I ACK this patch and the others that go with it:
> 
> Acked-by: David S. Miller <davem@davemloft.net>

Is there something more that I need to do to get this integrated?
Do I need to refresh the patches or something?
Will it go in during the next merge window?

	Thanks,
	Joe

Re: [PATCH 1/5] bonding: refactor mii monitor

[Jeff Garzik]

Jay Vosburgh wrote:
> 	Refactor mii monitor.  As with the previous ARP monitor refactor,
> the motivation for this is to handle locking rationally (in this case,
> removing conditional locking) and generally clean up the code.
> 
> 	This patch breaks up the monolithic mii monitor into two phases:
> an inspection phase, followed by an optional commit phase.  The commit phase
> is the only portion that requires RTNL or makes changes to state, and is
> only called when inspection finds something to change.
> 
> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
> ---
>  drivers/net/bonding/bond_3ad.c  |    1 +
>  drivers/net/bonding/bond_main.c |  394 +++++++++++++++++----------------------
>  2 files changed, 173 insertions(+), 222 deletions(-)

applied 1-5

Re: [PATCH net-next-2.6 0/5] bonding: Refactor, fix, and updates

[Jay Vosburgh]

	Jeff, are you still waiting for net-next-2.6 to settle before
forwarding patches?  It's been a couple of weeks since your last status
update, and I want check to see if I should resend the patches from this
series.

	Davem did ack the relevant patches.

From: Jay Vosburgh <fubar@us.ibm.com>
To: netdev@vger.kernel.org
Cc: Jeff Garzik <jgarzik@pobox.com>, David Miller <davem@davemloft.net>
Subject: [PATCH net-next-2.6 0/5] bonding: Refactor, fix, and updates
Date: 	Wed,  2 Jul 2008 18:21:57 -0700

	Five patches for bonding; these apply to net-next-2.6.

	Patch 1 is a refactor of the MII monitor, similar to the previous
refactor of the ARP active-backup monitor.  It replaces the monolithic
monitor function that uses conditional locking with a two phase (inspect
and commit) approach with strict locking (RTNL) required only for the
commit phase (which is only called when things actually change).  The long
term goal here is to ultimately consolidate all monitors within a generic
framework.

	Patch 2 makes a change to the Infiniband slave removal processing
to avoid a system crash when removing the final slave via sysfs.

	Patches 3 - 5 provide support for allowing slaves to receive
traffic independently from the master, and require some explanation.

	The goal of the last three patches is to permit slaves to receive
incoming traffic independently from the master; there are legitimate
reasons for wanting to do so, e.g., LLDP.  There are two ways to implement
this: a special case within bonding (skb_bond_should_drop) that would
require a hard-coded list of protocols to pass through, or a generic
method, that modifies the packet receive logic within netif_receive_skb.
The latter method is what is presented here.

	Please apply patches 1 - 2, and review and apply or provide
feedback for patches 3 - 5.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com