[PATCH] bonding cleanup 2.6 - Consolidate timer handling

Consolidate timers initialization, error checking and re-queuing.

drivers/net/bonding/bond_3ad.c

@@ -2122,16 +2122,17 @@ void bond_3ad_state_machine_handler(struct bonding *bond)
 {
 	struct port *port;
 	struct aggregator *aggregator;
+	int delta_in_ticks = AD_TIMER_INTERVAL * HZ / 1000;
 
 	read_lock(&bond->lock);
 
-	//check if there are any slaves
-	if (bond->next == (struct slave *)bond) {
-		goto end;
+	if (bond->kill_timers) {
+		goto out;
 	}
 
-	if ((bond->device->flags & IFF_UP) != IFF_UP) {
-		goto end;
+	//check if there are any slaves
+	if (bond->next == (struct slave *)bond) {
+		goto re_arm;
 	}
 
 	// check if agg_select_timer timer after initialize is timed out
@@ -2140,7 +2141,7 @@ void bond_3ad_state_machine_handler(struct bonding *bond)
 		if ((port = __get_first_port(bond))) {
 			if (!port->slave) {
 				printk(KERN_WARNING DRV_NAME ": Warning: bond's first port is uninitialized\n");
-				goto end;
+				goto re_arm;
 			}
 
 			aggregator = __get_first_agg(port);
@@ -2152,7 +2153,7 @@ void bond_3ad_state_machine_handler(struct bonding *bond)
 	for (port = __get_first_port(bond); port; port = __get_next_port(port)) {
 		if (!port->slave) {
 			printk(KERN_WARNING DRV_NAME ": Warning: Found an uninitialized port\n");
-			goto end;
+			goto re_arm;
 		}
 
 		ad_rx_machine(NULL, port);
@@ -2167,14 +2168,10 @@ void bond_3ad_state_machine_handler(struct bonding *bond)
 		}
 	}
 
-end:
+re_arm:
+	mod_timer(&(BOND_AD_INFO(bond).ad_timer), jiffies + delta_in_ticks);
+out:
 	read_unlock(&bond->lock);
-
-
-	if ((bond->device->flags & IFF_UP) == IFF_UP) {
-		/* re-arm the timer */
-		mod_timer(&(BOND_AD_INFO(bond).ad_timer), jiffies + (AD_TIMER_INTERVAL * HZ / 1000));
-	}
 }
 
 /**

drivers/net/bonding/bond_alb.c

@@ -1322,13 +1322,18 @@ bond_alb_monitor(struct bonding *bond)
 {
 	struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
 	struct slave *slave = NULL;
+	int delta_in_ticks = HZ / ALB_TIMER_TICKS_PER_SEC;
 
 	read_lock(&bond->lock);
 
-	if ((bond->slave_cnt == 0) || !(bond->device->flags & IFF_UP)) {
+	if (bond->kill_timers) {
+		goto out;
+	}
+
+	if (bond->slave_cnt == 0) {
 		bond_info->tx_rebalance_counter = 0;
 		bond_info->lp_counter = 0;
-		goto out;
+		goto re_arm;
 	}
 
 	bond_info->tx_rebalance_counter++;
@@ -1413,14 +1418,10 @@ bond_alb_monitor(struct bonding *bond)
 		}
 	}
 
+re_arm:
+	mod_timer(&(bond_info->alb_timer), jiffies + delta_in_ticks);
 out:
 	read_unlock(&bond->lock);
-
-	if (bond->device->flags & IFF_UP) {
-		/* re-arm the timer */
-		mod_timer(&(bond_info->alb_timer),
-			jiffies + (HZ/ALB_TIMER_TICKS_PER_SEC));
-	}
 }
 
 /* assumption: called before the slave is attched to the bond

drivers/net/bonding/bond_main.c

@@ -934,6 +934,8 @@ static int bond_open(struct net_device *dev)
 	struct timer_list *timer = &bond->mii_timer;
 	struct timer_list *arp_timer = &bond->arp_timer;
 
+	bond->kill_timers = 0;
+
 	if ((bond_mode == BOND_MODE_TLB) ||
 	    (bond_mode == BOND_MODE_ALB)) {
 		struct timer_list *alb_timer = &(BOND_ALB_INFO(bond).alb_timer);
@@ -955,7 +957,7 @@ static int bond_open(struct net_device *dev)
 
 	if (miimon > 0) {  /* link check interval, in milliseconds. */
 		init_timer(timer);
-		timer->expires  = jiffies + (miimon * HZ / 1000);
+		timer->expires  = jiffies + 1;
 		timer->data     = (unsigned long)dev;
 		timer->function = (void *)&bond_mii_monitor;
 		add_timer(timer);
@@ -963,7 +965,7 @@ static int bond_open(struct net_device *dev)
 
 	if (arp_interval> 0) {  /* arp interval, in milliseconds. */
 		init_timer(arp_timer);
-		arp_timer->expires  = jiffies + (arp_interval * HZ / 1000);
+		arp_timer->expires  = jiffies + 1;
 		arp_timer->data     = (unsigned long)dev;
 		if (bond_mode == BOND_MODE_ACTIVEBACKUP) {
 			arp_timer->function = (void *)&activebackup_arp_monitor;
@@ -976,7 +978,7 @@ static int bond_open(struct net_device *dev)
 	if (bond_mode == BOND_MODE_8023AD) {
 		struct timer_list *ad_timer = &(BOND_AD_INFO(bond).ad_timer);
 		init_timer(ad_timer);
-		ad_timer->expires  = jiffies + (AD_TIMER_INTERVAL * HZ / 1000);
+		ad_timer->expires  = jiffies + 1;
 		ad_timer->data     = (unsigned long)bond;
 		ad_timer->function = (void *)&bond_3ad_state_machine_handler;
 		add_timer(ad_timer);
@@ -994,31 +996,50 @@ static int bond_close(struct net_device *master)
 
 	write_lock_bh(&bond->lock);
 
-	if (miimon > 0) {  /* link check interval, in milliseconds. */
-		del_timer(&bond->mii_timer);
-	}
-	if (arp_interval> 0) {  /* arp interval, in milliseconds. */
-		del_timer(&bond->arp_timer);
-	}
+	bond_mc_list_destroy (bond);
 
 	if (bond_mode == BOND_MODE_8023AD) {
-		del_timer_sync(&(BOND_AD_INFO(bond).ad_timer));
-
 		/* Unregister the receive of LACPDUs */
 		bond_unregister_lacpdu(bond);
 	}
 
-	bond_mc_list_destroy (bond);
+	/* signal timers not to re-arm */
+	bond->kill_timers = 1;
 
 	write_unlock_bh(&bond->lock);
 
+	/* del_timer_sync must run without holding the bond->lock
+	 * because a running timer might be trying to hold it too
+	 */
+
+	if (miimon > 0) {  /* link check interval, in milliseconds. */
+		del_timer_sync(&bond->mii_timer);
+	}
+
+	if (arp_interval> 0) {  /* arp interval, in milliseconds. */
+		del_timer_sync(&bond->arp_timer);
+	}
+
+	switch (bond_mode) {
+	case BOND_MODE_8023AD:
+		del_timer_sync(&(BOND_AD_INFO(bond).ad_timer));
+		break;
+	case BOND_MODE_TLB:
+	case BOND_MODE_ALB:
+		del_timer_sync(&(BOND_ALB_INFO(bond).alb_timer));
+		break;
+	default:
+		break;
+	}
+
 	/* Release the bonded slaves */
 	bond_release_all(master);
 
 	if ((bond_mode == BOND_MODE_TLB) ||
 	    (bond_mode == BOND_MODE_ALB)) {
-		del_timer_sync(&(BOND_ALB_INFO(bond).alb_timer));
-
+		/* Must be called only after all
+		 * slaves have been released
+		 */
 		bond_alb_deinitialize(bond);
 	}
 
@@ -2150,9 +2171,18 @@ static void bond_mii_monitor(struct net_device *master)
 	struct slave *slave, *oldcurrent;
 	int slave_died = 0;
 	int do_failover = 0;
+	int delta_in_ticks = miimon * HZ / 1000;
 
 	read_lock(&bond->lock);
 
+	if (bond->kill_timers) {
+		goto out;
+	}
+
+	if (bond->slave_cnt == 0) {
+		goto re_arm;
+	}
+
 	/* 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
@@ -2361,9 +2391,10 @@ static void bond_mii_monitor(struct net_device *master)
 		write_unlock(&bond->ptrlock);
 	}
 
+re_arm:
+	mod_timer(&bond->mii_timer, jiffies + delta_in_ticks);
+out:
 	read_unlock(&bond->lock);
-	/* re-arm the timer */
-	mod_timer(&bond->mii_timer, jiffies + (miimon * HZ / 1000));
 }
 
 /* 
@@ -2375,34 +2406,21 @@ static void bond_mii_monitor(struct net_device *master)
  */
 static void loadbalance_arp_monitor(struct net_device *master)
 {
-	struct bonding *bond;
+	struct bonding *bond = (struct bonding *)master->priv;
 	struct slave *slave, *oldcurrent;
-	int the_delta_in_ticks =  arp_interval * HZ / 1000;
-	int next_timer = jiffies + (arp_interval * HZ / 1000);
 	int do_failover = 0;
+	int the_delta_in_ticks =  arp_interval * HZ / 1000;
 
-	bond = (struct bonding *)master->priv; 
-	if (master->priv == NULL) {
-		mod_timer(&bond->arp_timer, next_timer);
-		return;
-	}
+	read_lock(&bond->lock);
 
-	/* TODO: investigate why rtnl_shlock_nowait and rtnl_exlock_nowait
-	 * are called below and add comment why they are required... 
-	 */
-	if ((!IS_UP(master)) || rtnl_shlock_nowait()) {
-		mod_timer(&bond->arp_timer, next_timer);
-		return;
+	if (bond->kill_timers) {
+		goto out;
 	}
 
-	if (rtnl_exlock_nowait()) {
-		rtnl_shunlock();
-		mod_timer(&bond->arp_timer, next_timer);
-		return;
+	if (bond->slave_cnt == 0) {
+		goto re_arm;
 	}
 
-	read_lock(&bond->lock);
-
 	read_lock(&bond->ptrlock);
 	oldcurrent = bond->current_slave;
 	read_unlock(&bond->ptrlock);
@@ -2500,12 +2518,10 @@ static void loadbalance_arp_monitor(struct net_device *master)
 		write_unlock(&bond->ptrlock);
 	}
 
+re_arm:
+	mod_timer(&bond->arp_timer, jiffies + the_delta_in_ticks);
+out:
 	read_unlock(&bond->lock);
-	rtnl_exunlock();
-	rtnl_shunlock();
-
-	/* re-arm the timer */
-	mod_timer(&bond->arp_timer, next_timer);
 }
 
 /* 
@@ -2525,23 +2541,19 @@ static void loadbalance_arp_monitor(struct net_device *master)
  */
 static void activebackup_arp_monitor(struct net_device *master)
 {
-	struct bonding *bond;
+	struct bonding *bond = (struct bonding *)master->priv;
 	struct slave *slave;
 	int the_delta_in_ticks =  arp_interval * HZ / 1000;
-	int next_timer = jiffies + (arp_interval * HZ / 1000);
 
-	bond = (struct bonding *)master->priv; 
-	if (master->priv == NULL) {
-		mod_timer(&bond->arp_timer, next_timer);
-		return;
-	}
+	read_lock(&bond->lock);
 
-	if (!IS_UP(master)) {
-		mod_timer(&bond->arp_timer, next_timer);
-		return;
+	if (bond->kill_timers) {
+		goto out;
 	}
 
-	read_lock(&bond->lock);
+	if (bond->slave_cnt == 0) {
+		goto re_arm;
+	}
 
 	/* determine if any slave has come up or any backup slave has 
 	 * gone down 
@@ -2740,8 +2752,10 @@ static void activebackup_arp_monitor(struct net_device *master)
 		}
 	}
 
+re_arm:
+	mod_timer(&bond->arp_timer, jiffies + the_delta_in_ticks);
+out:
 	read_unlock(&bond->lock);
-	mod_timer(&bond->arp_timer, next_timer);
 }
 
 static int bond_sethwaddr(struct net_device *master, struct net_device *slave)

drivers/net/bonding/bonding.h

@@ -98,6 +98,7 @@ struct bonding {
 	rwlock_t ptrlock;
 	struct timer_list mii_timer;
 	struct timer_list arp_timer;
+	int kill_timers;
 	struct net_device_stats stats;
 #ifdef CONFIG_PROC_FS
 	struct proc_dir_entry *bond_proc_file;