bonding: Fix workqueue manipulation and lock taking

Change code not to rearm bond_mii_monitor() when value 0 is set for miimon.

---
 drivers/net/bonding/bond_main.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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

@@ -2388,7 +2388,8 @@ void bond_mii_monitor(struct work_struct
 
 	delay = ((bond->params.miimon * HZ) / 1000) ? : 1;
 	read_unlock(&bond->lock);
-	queue_delayed_work(bond->wq, &bond->mii_work, delay);
+	if (bond->params.miimon)
+		queue_delayed_work(bond->wq, &bond->mii_work, delay);
 }
 
 static __be32 bond_glean_dev_ip(struct net_device *dev)

Signature · 2 lines

Makito SHIOKAWA
MIRACLE LINUX CORPORATION

Makito SHIOKAWA

author

2008-01-15 06:34 UTC #0 lore raw

cover bonding: Fix workqueue manipulation and lock taking

These patches fix some workqueue manipulation and lock taking in bonding driver.

PATCH 1/4 through PATCH 3/4 are on workqueue manipulation, PATCH 4/4 is on lock taking, and each patch are logically independent. Patches are for latest netdev-2.6 git.

Signature · 2 lines

Makito SHIOKAWA
MIRACLE LINUX CORPORATION

Makito SHIOKAWA

author

2008-01-15 06:34 UTC #1 lore raw

PATCH 1/4 bonding: Fix work initing and cancelling

delayed_work_pending() is used to check whether work is being queued or not, when queueing or cancelling corresponding work. However, delayed_work_pending() returns 0 when work is just running, and in that case, work_struct will be destroyed or work won't be cancelled. So remove all delayed_work_pending(), and move all INIT_DELAYED_WORK to bonding init process, just queue or cancel work. For this purpose, arp_work is divided into two works.

---

 drivers/net/bonding/bond_main.c  |   46 ++++++++++++++++++++------------------
 drivers/net/bonding/bond_sysfs.c |   36 +++++++++--------------------
 drivers/net/bonding/bonding.h    |    3 +-
 3 files changed, 39 insertions(+), 46 deletions(-)

drivers/net/bonding/bond_main.c +25 ~~-21~~

@@ -2692,7 +2692,7 @@ out:
 void bond_loadbalance_arp_mon(struct work_struct *work)
 {
 	struct bonding *bond = container_of(work, struct bonding,
-					    arp_work.work);
+					    lb_arp_work.work);
 	struct slave *slave, *oldcurrent;
 	int do_failover = 0;
 	int delta_in_ticks;

@@ -2803,7 +2803,7 @@ void bond_loadbalance_arp_mon(struct wor
 
 re_arm:
 	if (bond->params.arp_interval)
-		queue_delayed_work(bond->wq, &bond->arp_work, delta_in_ticks);
+		queue_delayed_work(bond->wq, &bond->lb_arp_work, delta_in_ticks);
 out:
 	read_unlock(&bond->lock);
 }

@@ -2826,7 +2826,7 @@ out:
 void bond_activebackup_arp_mon(struct work_struct *work)
 {
 	struct bonding *bond = container_of(work, struct bonding,
-					    arp_work.work);
+					    ab_arp_work.work);
 	struct slave *slave;
 	int delta_in_ticks;
 	int i;

@@ -3060,7 +3060,7 @@ void bond_activebackup_arp_mon(struct wo
 
 re_arm:
 	if (bond->params.arp_interval) {
-		queue_delayed_work(bond->wq, &bond->arp_work, delta_in_ticks);
+		queue_delayed_work(bond->wq, &bond->ab_arp_work, delta_in_ticks);
 	}
 out:
 	read_unlock(&bond->lock);

@@ -3679,30 +3679,23 @@ static int bond_open(struct net_device *
 			return -1;
 		}
 
-		INIT_DELAYED_WORK(&bond->alb_work, bond_alb_monitor);
 		queue_delayed_work(bond->wq, &bond->alb_work, 0);
 	}
 
 	if (bond->params.miimon) {  /* link check interval, in milliseconds. */
-		INIT_DELAYED_WORK(&bond->mii_work, bond_mii_monitor);
 		queue_delayed_work(bond->wq, &bond->mii_work, 0);
 	}
 
 	if (bond->params.arp_interval) {  /* arp interval, in milliseconds. */
 		if (bond->params.mode == BOND_MODE_ACTIVEBACKUP)
-			INIT_DELAYED_WORK(&bond->arp_work,
-					  bond_activebackup_arp_mon);
+			queue_delayed_work(bond->wq, &bond->ab_arp_work, 0);
 		else
-			INIT_DELAYED_WORK(&bond->arp_work,
-					  bond_loadbalance_arp_mon);
-
-		queue_delayed_work(bond->wq, &bond->arp_work, 0);
+			queue_delayed_work(bond->wq, &bond->lb_arp_work, 0);
 		if (bond->params.arp_validate)
 			bond_register_arp(bond);
 	}
 
 	if (bond->params.mode == BOND_MODE_8023AD) {
-		INIT_DELAYED_WORK(&bond->ad_work, bond_3ad_state_machine_handler);
 		queue_delayed_work(bond->wq, &bond->ad_work, 0);
 		/* register to receive LACPDUs */
 		bond_register_lacpdu(bond);

@@ -3736,7 +3729,10 @@ static int bond_close(struct net_device 
 	}
 
 	if (bond->params.arp_interval) {  /* arp interval, in milliseconds. */
-		cancel_delayed_work(&bond->arp_work);
+		if (bond->params.mode == BOND_MODE_ACTIVEBACKUP)
+			cancel_delayed_work(&bond->ab_arp_work);
+		else
+			cancel_delayed_work(&bond->lb_arp_work);
 	}
 
 	switch (bond->params.mode) {

@@ -4416,6 +4412,12 @@ static int bond_init(struct net_device *
 	if (!bond->wq)
 		return -ENOMEM;
 
+	INIT_DELAYED_WORK(&bond->alb_work, bond_alb_monitor);
+	INIT_DELAYED_WORK(&bond->mii_work, bond_mii_monitor);
+	INIT_DELAYED_WORK(&bond->ab_arp_work, bond_activebackup_arp_mon);
+	INIT_DELAYED_WORK(&bond->lb_arp_work, bond_loadbalance_arp_mon);
+	INIT_DELAYED_WORK(&bond->ad_work, bond_3ad_state_machine_handler);
+
 	/* Initialize pointers */
 	bond->first_slave = NULL;
 	bond->curr_active_slave = NULL;

@@ -4498,18 +4500,20 @@ static void bond_work_cancel_all(struct 
 	bond->kill_timers = 1;
 	write_unlock_bh(&bond->lock);
 
-	if (bond->params.miimon && delayed_work_pending(&bond->mii_work))
+	if (bond->params.miimon)
 		cancel_delayed_work(&bond->mii_work);
 
-	if (bond->params.arp_interval && delayed_work_pending(&bond->arp_work))
-		cancel_delayed_work(&bond->arp_work);
+	if (bond->params.arp_interval) {
+		if (bond->params.mode == BOND_MODE_ACTIVEBACKUP)
+			cancel_delayed_work(&bond->ab_arp_work);
+		else
+			cancel_delayed_work(&bond->lb_arp_work);
+	}
 
-	if (bond->params.mode == BOND_MODE_ALB &&
-	    delayed_work_pending(&bond->alb_work))
+	if (bond->params.mode == BOND_MODE_ALB)
 		cancel_delayed_work(&bond->alb_work);
 
-	if (bond->params.mode == BOND_MODE_8023AD &&
-	    delayed_work_pending(&bond->ad_work))
+	if (bond->params.mode == BOND_MODE_8023AD)
 		cancel_delayed_work(&bond->ad_work);
 }

drivers/net/bonding/bond_sysfs.c +12 ~~-24~~

@@ -643,10 +643,8 @@ static ssize_t bonding_store_arp_interva
 		       "%s Disabling MII monitoring.\n",
 		       bond->dev->name, bond->dev->name);
 		bond->params.miimon = 0;
-		if (delayed_work_pending(&bond->mii_work)) {
-			cancel_delayed_work(&bond->mii_work);
-			flush_workqueue(bond->wq);
-		}
+		cancel_delayed_work(&bond->mii_work);
+		flush_workqueue(bond->wq);
 	}
 	if (!bond->params.arp_targets[0]) {
 		printk(KERN_INFO DRV_NAME

@@ -660,16 +658,10 @@ static ssize_t bonding_store_arp_interva
 		 * timer will get fired off when the open function
 		 * is called.
 		 */
-		if (!delayed_work_pending(&bond->arp_work)) {
-			if (bond->params.mode == BOND_MODE_ACTIVEBACKUP)
-				INIT_DELAYED_WORK(&bond->arp_work,
-						  bond_activebackup_arp_mon);
-			else
-				INIT_DELAYED_WORK(&bond->arp_work,
-						  bond_loadbalance_arp_mon);
-
-			queue_delayed_work(bond->wq, &bond->arp_work, 0);
-		}
+		if (bond->params.mode == BOND_MODE_ACTIVEBACKUP)
+			queue_delayed_work(bond->wq, &bond->ab_arp_work, 0);
+		else
+			queue_delayed_work(bond->wq, &bond->lb_arp_work, 0);
 	}
 
 out:

@@ -1022,10 +1014,11 @@ static ssize_t bonding_store_miimon(stru
 				bond->params.arp_validate =
 					BOND_ARP_VALIDATE_NONE;
 			}
-			if (delayed_work_pending(&bond->arp_work)) {
-				cancel_delayed_work(&bond->arp_work);
-				flush_workqueue(bond->wq);
-			}
+			if (bond->params.mode == BOND_MODE_ACTIVEBACKUP)
+				cancel_delayed_work(&bond->ab_arp_work);
+			else
+				cancel_delayed_work(&bond->lb_arp_work);
+			flush_workqueue(bond->wq);
 		}
 
 		if (bond->dev->flags & IFF_UP) {

@@ -1034,12 +1027,7 @@ static ssize_t bonding_store_miimon(stru
 			 * timer will get fired off when the open function
 			 * is called.
 			 */
-			if (!delayed_work_pending(&bond->mii_work)) {
-				INIT_DELAYED_WORK(&bond->mii_work,
-						  bond_mii_monitor);
-				queue_delayed_work(bond->wq,
-						   &bond->mii_work, 0);
-			}
+			queue_delayed_work(bond->wq, &bond->mii_work, 0);
 		}
 	}
 out:

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

@@ -206,7 +206,8 @@ struct bonding {
 	struct   packet_type arp_mon_pt;
 	struct   workqueue_struct *wq;
 	struct   delayed_work mii_work;
-	struct   delayed_work arp_work;
+	struct   delayed_work ab_arp_work;
+	struct   delayed_work lb_arp_work;
 	struct   delayed_work alb_work;
 	struct   delayed_work ad_work;
 };

Signature · 2 lines

Makito SHIOKAWA
MIRACLE LINUX CORPORATION

Makito SHIOKAWA

author

2008-01-15 06:34 UTC #4 lore raw

PATCH 2/4 bonding: Add destroy_workqueue() to bond device destroying process

There is no destroy_workqueue() in bond device destroying process, therefore worker thread will remain even if bond device is destroyed. So add destroy_workqueue(), and also, ensure all works are canceled before destroy_workqueue() is called.

---

 drivers/net/bonding/bond_main.c  |   72 +++++++++++++++++++-------------------
 drivers/net/bonding/bond_sysfs.c |    9 ++++
 drivers/net/bonding/bonding.h    |    5 ++
 3 files changed, 48 insertions(+), 38 deletions(-)

drivers/net/bonding/bond_main.c +36 ~~-36~~

@@ -190,7 +190,6 @@ struct bond_parm_tbl arp_validate_tbl[] 
 /*-------------------------- Forward declarations ---------------------------*/
 
 static void bond_send_gratuitous_arp(struct bonding *bond);
-static void bond_deinit(struct net_device *bond_dev);
 
 /*---------------------------- General routines -----------------------------*/

@@ -862,7 +861,7 @@ static void bond_resend_igmp_join_reques
 /*
  * Totally destroys the mc_list in bond
  */
-static void bond_mc_list_destroy(struct bonding *bond)
+void bond_mc_list_destroy(struct bonding *bond)
 {
 	struct dev_mc_list *dmi;

@@ -1875,7 +1874,7 @@ int  bond_release_and_destroy(struct net
 /*
  * This function releases all slaves.
  */
-static int bond_release_all(struct net_device *bond_dev)
+int bond_release_all(struct net_device *bond_dev)
 {
 	struct bonding *bond = bond_dev->priv;
 	struct slave *slave;

@@ -4392,6 +4391,25 @@ static const struct ethtool_ops bond_eth
 	.get_drvinfo		= bond_ethtool_get_drvinfo,
 };
 
+/* Caller must not hold rtnl_lock */
+void bond_work_cancel_all(struct bonding *bond)
+{
+	/* ensure all works are stopped */
+	cancel_delayed_work_sync(&bond->alb_work);
+	cancel_delayed_work_sync(&bond->mii_work);
+	cancel_delayed_work_sync(&bond->ab_arp_work);
+	cancel_delayed_work_sync(&bond->lb_arp_work);
+	cancel_delayed_work_sync(&bond->ad_work);
+}
+
+static void bond_free_netdev(struct net_device *dev)
+{
+	struct bonding *bond = netdev_priv(dev);
+
+	destroy_workqueue(bond->wq);
+	free_netdev(dev);
+}
+
 /*
  * Does not allocate but creates a /proc entry.
  * Allowed to fail.

@@ -4441,7 +4459,7 @@ static int bond_init(struct net_device *
 
 	bond_set_mode_ops(bond, bond->params.mode);
 
-	bond_dev->destructor = free_netdev;
+	bond_dev->destructor = bond_free_netdev;
 
 	/* Initialize the device options */
 	bond_dev->tx_queue_len = 0;

@@ -4483,7 +4501,7 @@ static int bond_init(struct net_device *
 /* De-initialize device specific data.
  * Caller must hold rtnl_lock.
  */
-static void bond_deinit(struct net_device *bond_dev)
+void bond_deinit(struct net_device *bond_dev)
 {
 	struct bonding *bond = bond_dev->priv;

@@ -4494,29 +4512,6 @@ static void bond_deinit(struct net_devic
 #endif
 }
 
-static void bond_work_cancel_all(struct bonding *bond)
-{
-	write_lock_bh(&bond->lock);
-	bond->kill_timers = 1;
-	write_unlock_bh(&bond->lock);
-
-	if (bond->params.miimon)
-		cancel_delayed_work(&bond->mii_work);
-
-	if (bond->params.arp_interval) {
-		if (bond->params.mode == BOND_MODE_ACTIVEBACKUP)
-			cancel_delayed_work(&bond->ab_arp_work);
-		else
-			cancel_delayed_work(&bond->lb_arp_work);
-	}
-
-	if (bond->params.mode == BOND_MODE_ALB)
-		cancel_delayed_work(&bond->alb_work);
-
-	if (bond->params.mode == BOND_MODE_8023AD)
-		cancel_delayed_work(&bond->ad_work);
-}
-
 /* Unregister and free all bond devices.
  * Caller must hold rtnl_lock.
  */

@@ -4527,11 +4522,14 @@ static void bond_free_all(void)
 	list_for_each_entry_safe(bond, nxt, &bond_dev_list, bond_list) {
 		struct net_device *bond_dev = bond->dev;
 
+		bond_deinit(bond_dev);
+		rtnl_unlock();
 		bond_work_cancel_all(bond);
+		rtnl_lock();
 		bond_mc_list_destroy(bond);
 		/* Release the bonded slaves */
 		bond_release_all(bond_dev);
-		bond_deinit(bond_dev);
+		bond_destroy_sysfs_entry(bond);
 		unregister_netdevice(bond_dev);
 	}

@@ -4921,8 +4919,16 @@ int bond_create(char *name, struct bond_
 
 out_bond:
 	bond_deinit(bond_dev);
+	if (*newbond)
+		unregister_netdevice(bond_dev);
+	else {
+		rtnl_unlock();
+		bond_work_cancel_all(netdev_priv(bond_dev));
+		rtnl_lock();
+		destroy_workqueue(((struct bonding *)netdev_priv(bond_dev))->wq);
 out_netdev:
-	free_netdev(bond_dev);
+		free_netdev(bond_dev);
+	}
 out_rtnl:
 	rtnl_unlock();
 	return res;

@@ -4932,7 +4938,6 @@ static int __init bonding_init(void)
 {
 	int i;
 	int res;
-	struct bonding *bond, *nxt;
 
 	printk(KERN_INFO "%s", version);

@@ -4959,11 +4964,6 @@ static int __init bonding_init(void)
 
 	goto out;
 err:
-	list_for_each_entry_safe(bond, nxt, &bond_dev_list, bond_list) {
-		bond_work_cancel_all(bond);
-		destroy_workqueue(bond->wq);
-	}
-
 	rtnl_lock();
 	bond_free_all();
 	bond_destroy_sysfs();

drivers/net/bonding/bond_sysfs.c +8 -1

@@ -163,7 +163,14 @@ static ssize_t bonding_store_bonds(struc
 				printk(KERN_INFO DRV_NAME
 					": %s is being deleted...\n",
 					bond->dev->name);
-				bond_destroy(bond);
+ 				bond_deinit(bond->dev);
+				rtnl_unlock();
+				bond_work_cancel_all(bond);
+				rtnl_lock();
+				bond_mc_list_destroy(bond);
+				bond_release_all(bond->dev);
+ 		        	bond_destroy_sysfs_entry(bond);
+ 				unregister_netdevice(bond->dev);
 				rtnl_unlock();
 				goto out;
 			}

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

@@ -301,7 +301,10 @@ static inline void bond_unset_master_alb
 struct vlan_entry *bond_next_vlan(struct bonding *bond, struct vlan_entry *curr);
 int bond_dev_queue_xmit(struct bonding *bond, struct sk_buff *skb, struct net_device *slave_dev);
 int bond_create(char *name, struct bond_params *params, struct bonding **newbond);
-void bond_destroy(struct bonding *bond);
+void bond_deinit(struct net_device *bond_dev);
+void bond_work_cancel_all(struct bonding *bond);
+void bond_mc_list_destroy(struct bonding *bond);
+int bond_release_all(struct net_device *bond_dev);
 int  bond_release_and_destroy(struct net_device *bond_dev, struct net_device *slave_dev);
 int bond_create_sysfs(void);
 void bond_destroy_sysfs(void);

Signature · 2 lines

Makito SHIOKAWA
MIRACLE LINUX CORPORATION

Makito SHIOKAWA

author

2008-01-15 06:34 UTC #18 lore raw

PATCH 4/4 bonding: Fix some RTNL taking

Fix some RTNL lock taking:

* RTNL (mutex; may sleep) must not be taken under read_lock (spinlock; must be atomic). However, RTNL is taken under read_lock in bond_loadbalance_arp_mon() and bond_activebackup_arp_mon(). So change code to take RTNL outside of read_lock.

* rtnl_unlock() calls netdev_run_todo() which takes net_todo_run_mutex, and rtnl_unlock() is called under read_lock in bond_mii_monitor(). So for the same reason as above, change code to call rtnl_unlock() outside of read_lock.

---
 drivers/net/bonding/bond_main.c |   24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

drivers/net/bonding/bond_main.c +10 ~~-14~~

@@ -2372,6 +2372,7 @@ void bond_mii_monitor(struct work_struct
 	struct bonding *bond = container_of(work, struct bonding,
 					    mii_work.work);
 	unsigned long delay;
+	int need_unlock = 0;
 
 	read_lock(&bond->lock);
 	if (bond->kill_timers) {

@@ -2383,13 +2384,16 @@ void bond_mii_monitor(struct work_struct
 		rtnl_lock();
 		read_lock(&bond->lock);
 		__bond_mii_monitor(bond, 1);
-		rtnl_unlock();
+		need_unlock = 1;
 	}
 
 	delay = ((bond->params.miimon * HZ) / 1000) ? : 1;
-	read_unlock(&bond->lock);
 	if (bond->params.miimon)
 		queue_delayed_work(bond->wq, &bond->mii_work, delay);
+	read_unlock(&bond->lock);
+	/* rtnl_unlock() may sleep, so call it after read_unlock() */
+	if (need_unlock)
+		rtnl_unlock();
 }
 
 static __be32 bond_glean_dev_ip(struct net_device *dev)

@@ -2698,6 +2702,7 @@ void bond_loadbalance_arp_mon(struct wor
 	int delta_in_ticks;
 	int i;
 
+	rtnl_lock();
 	read_lock(&bond->lock);
 
 	delta_in_ticks = (bond->params.arp_interval * HZ) / 1000;

@@ -2791,14 +2796,11 @@ void bond_loadbalance_arp_mon(struct wor
 	}
 
 	if (do_failover) {
-		rtnl_lock();
 		write_lock_bh(&bond->curr_slave_lock);
 
 		bond_select_active_slave(bond);
 
 		write_unlock_bh(&bond->curr_slave_lock);
-		rtnl_unlock();
-
 	}
 
 re_arm:

@@ -2806,6 +2808,7 @@ re_arm:
 		queue_delayed_work(bond->wq, &bond->lb_arp_work, delta_in_ticks);
 out:
 	read_unlock(&bond->lock);
+	rtnl_unlock();
 }
 
 /*

@@ -2831,6 +2834,7 @@ void bond_activebackup_arp_mon(struct wo
 	int delta_in_ticks;
 	int i;
 
+	rtnl_lock();
 	read_lock(&bond->lock);
 
 	delta_in_ticks = (bond->params.arp_interval * HZ) / 1000;

@@ -2855,8 +2859,6 @@ void bond_activebackup_arp_mon(struct wo
 
 				slave->link = BOND_LINK_UP;
 
-				rtnl_lock();
-
 				write_lock_bh(&bond->curr_slave_lock);
 
 				if ((!bond->curr_active_slave) &&

@@ -2892,7 +2894,6 @@ void bond_activebackup_arp_mon(struct wo
 				}
 
 				write_unlock_bh(&bond->curr_slave_lock);
-				rtnl_unlock();
 			}
 		} else {
 			read_lock(&bond->curr_slave_lock);

@@ -2962,7 +2963,6 @@ void bond_activebackup_arp_mon(struct wo
 			       bond->dev->name,
 			       slave->dev->name);
 
-			rtnl_lock();
 			write_lock_bh(&bond->curr_slave_lock);
 
 			bond_select_active_slave(bond);

@@ -2970,8 +2970,6 @@ void bond_activebackup_arp_mon(struct wo
 
 			write_unlock_bh(&bond->curr_slave_lock);
 
-			rtnl_unlock();
-
 			bond->current_arp_slave = slave;
 
 			if (slave) {

@@ -2989,13 +2987,10 @@ void bond_activebackup_arp_mon(struct wo
 			       bond->primary_slave->dev->name);
 
 			/* primary is up so switch to it */
-			rtnl_lock();
 			write_lock_bh(&bond->curr_slave_lock);
 			bond_change_active_slave(bond, bond->primary_slave);
 			write_unlock_bh(&bond->curr_slave_lock);
 
-			rtnl_unlock();
-
 			slave = bond->primary_slave;
 			slave->jiffies = jiffies;
 		} else {

@@ -3064,6 +3059,7 @@ re_arm:
 	}
 out:
 	read_unlock(&bond->lock);
+	rtnl_unlock();
 }
 
 /*------------------------------ proc/seq_file-------------------------------*/

Signature · 2 lines

Makito SHIOKAWA
MIRACLE LINUX CORPORATION

Jarek Poplawski

2008-01-15 08:59 UTC #6 lore raw

Re: [PATCH 3/4] bonding: Fix work rearming

Quoted 10 lines · Makito SHIOKAWA

Change code not to rearm bond_mii_monitor() when value 0 is set for miimon.
Signed-off-by Makito SHIOKAWA <[email protected]>
---
 drivers/net/bonding/bond_main.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/net/bonding/bond_main.c void bond_mii_monitor(struct work_struct · line 2393

Quoted 7 lines

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

@@ -2388,7 +2388,8 @@ void bond_mii_monitor(struct work_struct
 
 	delay = ((bond->params.miimon * HZ) / 1000) ? : 1;
 	read_unlock(&bond->lock);
-	queue_delayed_work(bond->wq, &bond->mii_work, delay);
+	if (bond->params.miimon)
+		queue_delayed_work(bond->wq, &bond->mii_work, delay);
 }

Maybe I miss something, but is this bond_mii_monitor() function supposed to be ever started if (!bond->params.miimon)? (IOW: isn't it enough to control this where the parameter is changed only?)

Regards,
Jarek P.

Jarek Poplawski

2008-01-15 10:49 UTC #2 lore raw

Re: [PATCH 1/4] bonding: Fix work initing and cancelling

On 15-01-2008 07:36, Makito SHIOKAWA wrote:
...

drivers/net/bonding/bond_sysfs.c static ssize_t bonding_store_arp_interva · line 647

Quoted 12 lines

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

@@ -643,10 +643,8 @@ static ssize_t bonding_store_arp_interva
 		       "%s Disabling MII monitoring.\n",
 		       bond->dev->name, bond->dev->name);
 		bond->params.miimon = 0;
-		if (delayed_work_pending(&bond->mii_work)) {
-			cancel_delayed_work(&bond->mii_work);
-			flush_workqueue(bond->wq);
-		}
+		cancel_delayed_work(&bond->mii_work);
+		flush_workqueue(bond->wq);

I wonder why don't you use cancel_delayed_work_sync() here (and in a few other places), like in bond_work_cancel_all() from patch 2/4?

Regards,
Jarek P.

Makito SHIOKAWA

author

2008-01-16 03:21 UTC #7 lore raw

Re: [PATCH 3/4] bonding: Fix work rearming

This patch is supposing a case that bond_mii_monitor() is invoked in bond_open(), and after that, 0 is set to miimon via sysfs (see same place on other monitors). Though message in bonding_store_miimon() says miimon value 1-INT_MAX rejected, but it looks like 0 can be accepted and monitor must be stopped in that case.

Quoted 22 lines

Quoted 10 lines
Change code not to rearm bond_mii_monitor() when value 0 is set for miimon.
Signed-off-by Makito SHIOKAWA <[email protected]>
---
 drivers/net/bonding/bond_main.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/bonding/bond_main.c void bond_mii_monitor(struct work_struct · line 2393
Quoted 7 lines
drivers/net/bonding/bond_main.c +2 -1
@@ -2388,7 +2388,8 @@ void bond_mii_monitor(struct work_struct
 
 	delay = ((bond->params.miimon * HZ) / 1000) ? : 1;
 	read_unlock(&bond->lock);
-	queue_delayed_work(bond->wq, &bond->mii_work, delay);
+	if (bond->params.miimon)
+		queue_delayed_work(bond->wq, &bond->mii_work, delay);
 }
Maybe I miss something, but is this bond_mii_monitor() function supposed to be ever started if (!bond->params.miimon)? (IOW: isn't it enough to control this where the parameter is changed only?)

Quoted 2 lines

Regards,
Jarek P.

--
Makito SHIOKAWA
MIRACLE LINUX CORPORATION

Makito SHIOKAWA

author

2008-01-16 03:30 UTC #17 lore raw

Re: [PATCH 3/4] bonding: Fix work rearming

Quoted 3 lines

Though message in bonding_store_miimon() says miimon value 1-INT_MAX rejected, but it looks like 0 can be accepted and monitor must be stopped in that case.

=> miimon value *not in range* 1-INT_MAX

Signature · 2 lines

Makito SHIOKAWA
MIRACLE LINUX CORPORATION

Makito SHIOKAWA

author

2008-01-16 05:18 UTC #3 lore raw

Re: [PATCH 1/4] bonding: Fix work initing and cancelling

Quoted 2 lines

I wonder why don't you use cancel_delayed_work_sync() here (and in a few other places), like in bond_work_cancel_all() from patch 2/4?

In bond_close(), you can't use cancel_delayed_work_sync() because you can't unlock RTNL in it. (So, on current implementation, it becomes work cancel is not ensured on bond_close().)
In sysfs, I think you are right, thanks. So, I will modify the patch as below (other patches won't be affected).

---

 drivers/net/bonding/bond_main.c  |   46 ++++++++++++++++++++------------------
 drivers/net/bonding/bond_sysfs.c |   34 ++++++++--------------------
 drivers/net/bonding/bonding.h    |    3 +-
 3 files changed, 37 insertions(+), 46 deletions(-)

drivers/net/bonding/bond_main.c +25 ~~-21~~

@@ -2692,7 +2692,7 @@ out:
 void bond_loadbalance_arp_mon(struct work_struct *work)
 {
 	struct bonding *bond = container_of(work, struct bonding,
-					    arp_work.work);
+					    lb_arp_work.work);
 	struct slave *slave, *oldcurrent;
 	int do_failover = 0;
 	int delta_in_ticks;

@@ -2803,7 +2803,7 @@ void bond_loadbalance_arp_mon(struct wor
 
 re_arm:
 	if (bond->params.arp_interval)
-		queue_delayed_work(bond->wq, &bond->arp_work, delta_in_ticks);
+		queue_delayed_work(bond->wq, &bond->lb_arp_work, delta_in_ticks);
 out:
 	read_unlock(&bond->lock);
 }

@@ -2826,7 +2826,7 @@ out:
 void bond_activebackup_arp_mon(struct work_struct *work)
 {
 	struct bonding *bond = container_of(work, struct bonding,
-					    arp_work.work);
+					    ab_arp_work.work);
 	struct slave *slave;
 	int delta_in_ticks;
 	int i;

@@ -3060,7 +3060,7 @@ void bond_activebackup_arp_mon(struct wo
 
 re_arm:
 	if (bond->params.arp_interval) {
-		queue_delayed_work(bond->wq, &bond->arp_work, delta_in_ticks);
+		queue_delayed_work(bond->wq, &bond->ab_arp_work, delta_in_ticks);
 	}
 out:
 	read_unlock(&bond->lock);

@@ -3679,30 +3679,23 @@ static int bond_open(struct net_device *
 			return -1;
 		}
 
-		INIT_DELAYED_WORK(&bond->alb_work, bond_alb_monitor);
 		queue_delayed_work(bond->wq, &bond->alb_work, 0);
 	}
 
 	if (bond->params.miimon) {  /* link check interval, in milliseconds. */
-		INIT_DELAYED_WORK(&bond->mii_work, bond_mii_monitor);
 		queue_delayed_work(bond->wq, &bond->mii_work, 0);
 	}
 
 	if (bond->params.arp_interval) {  /* arp interval, in milliseconds. */
 		if (bond->params.mode == BOND_MODE_ACTIVEBACKUP)
-			INIT_DELAYED_WORK(&bond->arp_work,
-					  bond_activebackup_arp_mon);
+			queue_delayed_work(bond->wq, &bond->ab_arp_work, 0);
 		else
-			INIT_DELAYED_WORK(&bond->arp_work,
-					  bond_loadbalance_arp_mon);
-
-		queue_delayed_work(bond->wq, &bond->arp_work, 0);
+			queue_delayed_work(bond->wq, &bond->lb_arp_work, 0);
 		if (bond->params.arp_validate)
 			bond_register_arp(bond);
 	}
 
 	if (bond->params.mode == BOND_MODE_8023AD) {
-		INIT_DELAYED_WORK(&bond->ad_work, bond_3ad_state_machine_handler);
 		queue_delayed_work(bond->wq, &bond->ad_work, 0);
 		/* register to receive LACPDUs */
 		bond_register_lacpdu(bond);

@@ -3736,7 +3729,10 @@ static int bond_close(struct net_device
 	}
 
 	if (bond->params.arp_interval) {  /* arp interval, in milliseconds. */
-		cancel_delayed_work(&bond->arp_work);
+		if (bond->params.mode == BOND_MODE_ACTIVEBACKUP)
+			cancel_delayed_work(&bond->ab_arp_work);
+		else
+			cancel_delayed_work(&bond->lb_arp_work);
 	}
 
 	switch (bond->params.mode) {

@@ -4416,6 +4412,12 @@ static int bond_init(struct net_device *
 	if (!bond->wq)
 		return -ENOMEM;
 
+	INIT_DELAYED_WORK(&bond->alb_work, bond_alb_monitor);
+	INIT_DELAYED_WORK(&bond->mii_work, bond_mii_monitor);
+	INIT_DELAYED_WORK(&bond->ab_arp_work, bond_activebackup_arp_mon);
+	INIT_DELAYED_WORK(&bond->lb_arp_work, bond_loadbalance_arp_mon);
+	INIT_DELAYED_WORK(&bond->ad_work, bond_3ad_state_machine_handler);
+
 	/* Initialize pointers */
 	bond->first_slave = NULL;
 	bond->curr_active_slave = NULL;

@@ -4498,18 +4500,20 @@ static void bond_work_cancel_all(struct
 	bond->kill_timers = 1;
 	write_unlock_bh(&bond->lock);
 
-	if (bond->params.miimon && delayed_work_pending(&bond->mii_work))
+	if (bond->params.miimon)
 		cancel_delayed_work(&bond->mii_work);
 
-	if (bond->params.arp_interval && delayed_work_pending(&bond->arp_work))
-		cancel_delayed_work(&bond->arp_work);
+	if (bond->params.arp_interval) {
+		if (bond->params.mode == BOND_MODE_ACTIVEBACKUP)
+			cancel_delayed_work(&bond->ab_arp_work);
+		else
+			cancel_delayed_work(&bond->lb_arp_work);
+	}
 
-	if (bond->params.mode == BOND_MODE_ALB &&
-	    delayed_work_pending(&bond->alb_work))
+	if (bond->params.mode == BOND_MODE_ALB)
 		cancel_delayed_work(&bond->alb_work);
 
-	if (bond->params.mode == BOND_MODE_8023AD &&
-	    delayed_work_pending(&bond->ad_work))
+	if (bond->params.mode == BOND_MODE_8023AD)
 		cancel_delayed_work(&bond->ad_work);
 }

drivers/net/bonding/bond_sysfs.c +10 ~~-24~~

@@ -643,10 +643,7 @@ static ssize_t bonding_store_arp_interva
 		       "%s Disabling MII monitoring.\n",
 		       bond->dev->name, bond->dev->name);
 		bond->params.miimon = 0;
-		if (delayed_work_pending(&bond->mii_work)) {
-			cancel_delayed_work(&bond->mii_work);
-			flush_workqueue(bond->wq);
-		}
+		cancel_delayed_work_sync(&bond->mii_work);
 	}
 	if (!bond->params.arp_targets[0]) {
 		printk(KERN_INFO DRV_NAME

@@ -660,16 +657,10 @@ static ssize_t bonding_store_arp_interva
 		 * timer will get fired off when the open function
 		 * is called.
 		 */
-		if (!delayed_work_pending(&bond->arp_work)) {
-			if (bond->params.mode == BOND_MODE_ACTIVEBACKUP)
-				INIT_DELAYED_WORK(&bond->arp_work,
-						  bond_activebackup_arp_mon);
-			else
-				INIT_DELAYED_WORK(&bond->arp_work,
-						  bond_loadbalance_arp_mon);
-
-			queue_delayed_work(bond->wq, &bond->arp_work, 0);
-		}
+		if (bond->params.mode == BOND_MODE_ACTIVEBACKUP)
+			queue_delayed_work(bond->wq, &bond->ab_arp_work, 0);
+		else
+			queue_delayed_work(bond->wq, &bond->lb_arp_work, 0);
 	}
 
 out:

@@ -1022,10 +1013,10 @@ static ssize_t bonding_store_miimon(stru
 				bond->params.arp_validate =
 					BOND_ARP_VALIDATE_NONE;
 			}
-			if (delayed_work_pending(&bond->arp_work)) {
-				cancel_delayed_work(&bond->arp_work);
-				flush_workqueue(bond->wq);
-			}
+			if (bond->params.mode == BOND_MODE_ACTIVEBACKUP)
+				cancel_delayed_work_sync(&bond->ab_arp_work);
+			else
+				cancel_delayed_work_sync(&bond->lb_arp_work);
 		}
 
 		if (bond->dev->flags & IFF_UP) {

@@ -1034,12 +1025,7 @@ static ssize_t bonding_store_miimon(stru
 			 * timer will get fired off when the open function
 			 * is called.
 			 */
-			if (!delayed_work_pending(&bond->mii_work)) {
-				INIT_DELAYED_WORK(&bond->mii_work,
-						  bond_mii_monitor);
-				queue_delayed_work(bond->wq,
-						   &bond->mii_work, 0);
-			}
+			queue_delayed_work(bond->wq, &bond->mii_work, 0);
 		}
 	}
 out:

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

@@ -206,7 +206,8 @@ struct bonding {
 	struct   packet_type arp_mon_pt;
 	struct   workqueue_struct *wq;
 	struct   delayed_work mii_work;
-	struct   delayed_work arp_work;
+	struct   delayed_work ab_arp_work;
+	struct   delayed_work lb_arp_work;
 	struct   delayed_work alb_work;
 	struct   delayed_work ad_work;
 };

Signature · 2 lines

Makito SHIOKAWA
MIRACLE LINUX CORPORATION

Jarek Poplawski

2008-01-16 12:38 UTC #19 lore raw

Re: [PATCH 4/4] bonding: Fix some RTNL taking

Quoted 26 lines · Makito SHIOKAWA

Fix some RTNL lock taking:
* RTNL (mutex; may sleep) must not be taken under read_lock (spinlock; must be atomic). However, RTNL is taken under read_lock in bond_loadbalance_arp_mon() and bond_activebackup_arp_mon(). So change code to take RTNL outside of read_lock.
* rtnl_unlock() calls netdev_run_todo() which takes net_todo_run_mutex, and rtnl_unlock() is called under read_lock in bond_mii_monitor(). So for the same reason as above, change code to call rtnl_unlock() outside of read_lock.
Signed-off-by Makito SHIOKAWA <[email protected]>
---
 drivers/net/bonding/bond_main.c |   24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)
drivers/net/bonding/bond_main.c +1 -0
@@ -2372,6 +2372,7 @@ void bond_mii_monitor(struct work_struct
 	struct bonding *bond = container_of(work, struct bonding,
 					    mii_work.work);
 	unsigned long delay;
+	int need_unlock = 0;
 
 	read_lock(&bond->lock);
 	if (bond->kill_timers) {

drivers/net/bonding/bond_main.c void bond_mii_monitor(struct work_struct · line 2387

Quoted 5 lines

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

@@ -2383,13 +2384,16 @@ void bond_mii_monitor(struct work_struct
 		rtnl_lock();
 		read_lock(&bond->lock);
 		__bond_mii_monitor(bond, 1);
-		rtnl_unlock();
+		need_unlock = 1;

Maybe I'm wrong, but since this read_lock() is given and taken anyway, it seems this looks a bit better to me (why hold this rtnl longer than needed?):

		read_unlock(&bond->lock);
		rtnl_unlock();
		read_lock(&bond->lock);

On the other hand, probably 'if (bond->kill_timers)' could be repeated after this read_lock() retaking.

drivers/net/bonding/bond_main.c void bond_mii_monitor(struct work_struct · line 2392

Quoted 6 lines

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

 	}
 
 	delay = ((bond->params.miimon * HZ) / 1000) ? : 1;
-	read_unlock(&bond->lock);
 	if (bond->params.miimon)
 		queue_delayed_work(bond->wq, &bond->mii_work, delay);

If this if () is really necessary here, then this should be better before "delay = ..." with a block.

drivers/net/bonding/bond_main.c void bond_mii_monitor(struct work_struct · line 2397

Quoted 5 lines

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

+	read_unlock(&bond->lock);
+	/* rtnl_unlock() may sleep, so call it after read_unlock() */
+	if (need_unlock)
+		rtnl_unlock();
 }

Regards,
Jarek P.

Jarek Poplawski

2008-01-16 13:30 UTC #8 lore raw

Re: [PATCH 3/4] bonding: Fix work rearming

Quoted 6 lines · Makito SHIOKAWA

This patch is supposing a case that bond_mii_monitor() is invoked in bond_open(), and after that, 0 is set to miimon via sysfs (see same place on other monitors).
Though message in bonding_store_miimon() says miimon value 1-INT_MAX rejected, but it looks like 0 can be accepted and monitor must be stopped in that case.

But, since during this change from sysfs cancel_delayed_work_sync() could be probably used, and it's rather efficient with killing rearming works, it seems this check could be unnecessary yet.

Jarek P.

Makito SHIOKAWA

author

2008-01-17 05:32 UTC #9 lore raw

Re: [PATCH 3/4] bonding: Fix work rearming

Quoted 3 lines

But, since during this change from sysfs cancel_delayed_work_sync() could be probably used, and it's rather efficient with killing rearming works, it seems this check could be unnecessary yet.

What going to be cancelled in bonding_store_miimon() when setting miimon to 0 is arp monitor, not mii monitor. So, this check will be needed to stop rearming mii monitor when value 0 is set to miimon.

Signature · 2 lines

Makito SHIOKAWA
MIRACLE LINUX CORPORATION

Makito SHIOKAWA

author

2008-01-17 05:32 UTC #20 lore raw

Re: [PATCH 4/4] bonding: Fix some RTNL taking

Quoted 6 lines

Maybe I'm wrong, but since this read_lock() is given and taken anyway, it seems this looks a bit better to me (why hold this rtnl longer than needed?):
		read_unlock(&bond->lock);
		rtnl_unlock();
		read_lock(&bond->lock);

Seems better.

Quoted 2 lines

On the other hand, probably 'if (bond->kill_timers)' could be repeated after this read_lock() retaking.

Sorry, what do you mean? (A case that bond->kill_timers = 1 is done during lock retaking, and work being queued only to do 'if (bond->kill_timers)'? If so, I think that won't differ much.)

Quoted 2 lines

If this if () is really necessary here, then this should be better before "delay = ..." with a block.

I agree.

Signature · 2 lines

Makito SHIOKAWA
MIRACLE LINUX CORPORATION

Jarek Poplawski

2008-01-17 11:12 UTC #10 lore raw

Re: [PATCH 3/4] bonding: Fix work rearming

Quoted 6 lines · Makito SHIOKAWA

Quoted 3 lines
But, since during this change from sysfs cancel_delayed_work_sync() could be probably used, and it's rather efficient with killing rearming works, it seems this check could be unnecessary yet.
What going to be cancelled in bonding_store_miimon() when setting miimon to 0 is arp monitor, not mii monitor. So, this check will be needed to stop rearming mii monitor when value 0 is set to miimon.

Hmm... I'm not sure I understand your point, but it seems both bonding_store_arp_interval() and bonding_store_miimon() where this field could be changed, currently use cancel_delayed_work() with flush_workqueue(), so I presume, there is no rtnl_lock() nor write_lock(&bond->lock) held, so cancel_delayed_work_sync() could be used, which doesn't require this additional check.

...Unless you mean that despite miimon value is changed there, mii_work for some reason can't be cancelled at the same time?

Of course, if there is such a reason for doing this check each time a work runs instead of controlling where the value changes, then OK!

Regards,
Jarek P.

Jarek Poplawski

2008-01-17 11:40 UTC #21 lore raw

Re: [PATCH 4/4] bonding: Fix some RTNL taking

Quoted 6 lines · Makito SHIOKAWA

Quoted 6 lines
Maybe I'm wrong, but since this read_lock() is given and taken anyway, it seems this looks a bit better to me (why hold this rtnl longer than needed?):
		read_unlock(&bond->lock);
		rtnl_unlock();
		read_lock(&bond->lock);

Quoted 7 lines

Seems better.
Quoted 2 lines
On the other hand, probably 'if (bond->kill_timers)' could be repeated after this read_lock() retaking.
Sorry, what do you mean? (A case that bond->kill_timers = 1 is done during lock retaking, and work being queued only to do 'if (bond->kill_timers)'? If so, I think that won't differ much.)

Probably the difference is not much, but since this all double locking, unlocking and something between could take a while, and such a check looks cheaper than re-queueing... But I don't persist in this.

Jarek P.

Makito SHIOKAWA

author

2008-01-18 09:08 UTC #22 lore raw

Re: [PATCH 4/4] bonding: Fix some RTNL taking

Quoted 3 lines

Probably the difference is not much, but since this all double locking, unlocking and something between could take a while, and such a check looks cheaper than re-queueing... But I don't persist in this.

Looks like this one is settled, thank you for your advice!

--
Makito SHIOKAWA
MIRACLE LINUX CORPORATION

Makito SHIOKAWA

author

2008-01-18 13:44 UTC #11 lore raw

Re: [PATCH 3/4] bonding: Fix work rearming

Quoted 6 lines

Hmm... I'm not sure I understand your point, but it seems both bonding_store_arp_interval() and bonding_store_miimon() where this field could be changed, currently use cancel_delayed_work() with flush_workqueue(), so I presume, there is no rtnl_lock() nor write_lock(&bond->lock) held, so cancel_delayed_work_sync() could be used, which doesn't require this additional check.

I see. I rewrited the patch as below. How about this?
(But, it may be just a matter to change 'if (new_value < 0)' to 'if (new_value <= 0)' in bonding_store_miimon() and bonding_store_arp_interval()...)

---
 drivers/net/bonding/bond_sysfs.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/net/bonding/bond_sysfs.c +3 -1

@@ -997,6 +997,8 @@ static ssize_t bonding_store_miimon(stru
 		       ": %s: Setting MII monitoring interval to %d.\n",
 		       bond->dev->name, new_value);
 		bond->params.miimon = new_value;
+		if (bond->params.miimon == 0)
+			cancel_delayed_work_sync(&bond->mii_work);
 		if(bond->params.updelay)
 			printk(KERN_INFO DRV_NAME
 			      ": %s: Note: Updating updelay (to %d) "

@@ -1026,7 +1028,7 @@ static ssize_t bonding_store_miimon(stru
 				cancel_delayed_work_sync(&bond->lb_arp_work);
 		}
 
-		if (bond->dev->flags & IFF_UP) {
+		if (bond->params.miimon && (bond->dev->flags & IFF_UP)) {
 			/* If the interface is up, we may need to fire off
 			 * the MII timer. If the interface is down, the
 			 * timer will get fired off when the open function

Signature · 2 lines

Makito SHIOKAWA
MIRACLE LINUX CORPORATION

Jarek Poplawski

2008-01-18 22:23 UTC #12 lore raw

Re: [PATCH 3/4] bonding: Fix work rearming

Makito SHIOKAWA wrote, On 01/18/2008 02:43 PM:

Quoted 9 lines

Quoted 6 lines
Hmm... I'm not sure I understand your point, but it seems both bonding_store_arp_interval() and bonding_store_miimon() where this field could be changed, currently use cancel_delayed_work() with flush_workqueue(), so I presume, there is no rtnl_lock() nor write_lock(&bond->lock) held, so cancel_delayed_work_sync() could be used, which doesn't require this additional check.
I see. I rewrited the patch as below. How about this?
(But, it may be just a matter to change 'if (new_value < 0)' to 'if (new_value <= 0)' in bonding_store_miimon() and bonding_store_arp_interval()...)

Yes, looks fine to me.

(But new_value = 0 seems needed - just like from module_param()?)

Maybe only a few slight doubts, yet:
- maybe before this cancel IFF_UP test would be useful as well,
- maybe to test if the value has changed at all,
- maybe it's OK, I don't know, but it seems both monitors could be turned off now.

Thanks,
Jarek P.

Quoted 27 lines

---
  drivers/net/bonding/bond_sysfs.c |    4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

drivers/net/bonding/bond_sysfs.c +3 -1

@@ -997,6 +997,8 @@ static ssize_t bonding_store_miimon(stru
  		       ": %s: Setting MII monitoring interval to %d.\n",
  		       bond->dev->name, new_value);
  		bond->params.miimon = new_value;
+		if (bond->params.miimon == 0)
+			cancel_delayed_work_sync(&bond->mii_work);
  		if(bond->params.updelay)
  			printk(KERN_INFO DRV_NAME
  			      ": %s: Note: Updating updelay (to %d) "

@@ -1026,7 +1028,7 @@ static ssize_t bonding_store_miimon(stru
  				cancel_delayed_work_sync(&bond->lb_arp_work);
  		}
 
-		if (bond->dev->flags & IFF_UP) {
+		if (bond->params.miimon && (bond->dev->flags & IFF_UP)) {
  			/* If the interface is up, we may need to fire off
  			 * the MII timer. If the interface is down, the
  			 * timer will get fired off when the open function

Jarek Poplawski

2008-01-18 22:39 UTC #13 lore raw

Re: [PATCH 3/4] bonding: Fix work rearming

Jarek Poplawski wrote, On 01/18/2008 11:27 PM:

Quoted 1 line

Makito SHIOKAWA wrote, On 01/18/2008 02:43 PM:

...

Quoted 6 lines

(quoted diff) +0 -0
@@ -1026,7 +1028,7 @@ static ssize_t bonding_store_miimon(stru
 >   				cancel_delayed_work_sync(&bond->lb_arp_work);
 >   		}
drivers/net/bonding/bond_sysfs.c static ssize_t bonding_store_miimon(stru · line 1031
Quoted 3 lines
drivers/net/bonding/bond_sysfs.c +1 -1
-		if (bond->dev->flags & IFF_UP) {
+		if (bond->params.miimon && (bond->dev->flags & IFF_UP)) {

...similar change in bonding_store_arp_interval(), I guess?

Jarek P.

Makito SHIOKAWA

author

2008-01-21 04:03 UTC #14 lore raw

Re: [PATCH 3/4] bonding: Fix work rearming

Quoted 1 line

(But new_value = 0 seems needed - just like from module_param()?)

Do you mean to initialize new_value before sscanf()? (There is a check 'if (sscanf(buf, "%d", &new_value) != 1)', so is it necesarry?)

Quoted 1 line

- maybe to test if the value has changed at all,

Tested.

For now, patch will be like below. Any slight comment for this will be helpful, regards.

---
 drivers/net/bonding/bond_main.c  |   11 ++++-------
 drivers/net/bonding/bond_sysfs.c |   19 +++++++++++++++++--
 2 files changed, 21 insertions(+), 9 deletions(-)

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

@@ -2699,7 +2699,7 @@ void bond_loadbalance_arp_mon(struct wor
 
 	read_lock(&bond->lock);
 
-	delta_in_ticks = (bond->params.arp_interval * HZ) / 1000;
+	delta_in_ticks = ((bond->params.arp_interval * HZ) / 1000) ? : 1;
 
 	if (bond->kill_timers) {
 		goto out;

@@ -2801,8 +2801,7 @@ void bond_loadbalance_arp_mon(struct wor
 	}
 
 re_arm:
-	if (bond->params.arp_interval)
-		queue_delayed_work(bond->wq, &bond->lb_arp_work, delta_in_ticks);
+	queue_delayed_work(bond->wq, &bond->lb_arp_work, delta_in_ticks);
 out:
 	read_unlock(&bond->lock);
 }

@@ -2832,7 +2831,7 @@ void bond_activebackup_arp_mon(struct wo
 
 	read_lock(&bond->lock);
 
-	delta_in_ticks = (bond->params.arp_interval * HZ) / 1000;
+	delta_in_ticks = ((bond->params.arp_interval * HZ) / 1000) ? : 1;
 
 	if (bond->kill_timers) {
 		goto out;

@@ -3058,9 +3057,7 @@ void bond_activebackup_arp_mon(struct wo
 	}
 
 re_arm:
-	if (bond->params.arp_interval) {
-		queue_delayed_work(bond->wq, &bond->ab_arp_work, delta_in_ticks);
-	}
+	queue_delayed_work(bond->wq, &bond->ab_arp_work, delta_in_ticks);
 out:
 	read_unlock(&bond->lock);
 }

drivers/net/bonding/bond_sysfs.c +17 -2

@@ -644,6 +644,15 @@ static ssize_t bonding_store_arp_interva
 	       ": %s: Setting ARP monitoring interval to %d.\n",
 	       bond->dev->name, new_value);
 	bond->params.arp_interval = new_value;
+	if (bond->params.arp_interval == 0 && (bond->dev->flags & IFF_UP)) {
+		printk(KERN_INFO DRV_NAME
+		       ": %s: Disabling ARP monitoring.\n",
+		       bond->dev->name);
+		if (bond->params.mode == BOND_MODE_ACTIVEBACKUP)
+			cancel_delayed_work_sync(&bond->ab_arp_work);
+		else
+			cancel_delayed_work_sync(&bond->lb_arp_work);
+	}
 	if (bond->params.miimon) {
 		printk(KERN_INFO DRV_NAME
 		       ": %s: ARP monitoring cannot be used with MII monitoring. "

@@ -658,7 +667,7 @@ static ssize_t bonding_store_arp_interva
 		       "but no ARP targets have been specified.\n",
 		       bond->dev->name);
 	}
-	if (bond->dev->flags & IFF_UP) {
+	if (bond->params.arp_interval && (bond->dev->flags & IFF_UP)) {
 		/* If the interface is up, we may need to fire off
 		 * the ARP timer.  If the interface is down, the
 		 * timer will get fired off when the open function

@@ -997,6 +1006,12 @@ static ssize_t bonding_store_miimon(stru
 		       ": %s: Setting MII monitoring interval to %d.\n",
 		       bond->dev->name, new_value);
 		bond->params.miimon = new_value;
+		if (bond->params.miimon == 0 && (bond->dev->flags & IFF_UP)) {
+			printk(KERN_INFO DRV_NAME
+			       ": %s: Disabling MII monitoring...\n",
+			       bond->dev->name);
+			cancel_delayed_work_sync(&bond->mii_work);
+		}
 		if(bond->params.updelay)
 			printk(KERN_INFO DRV_NAME
 			      ": %s: Note: Updating updelay (to %d) "

@@ -1026,7 +1041,7 @@ static ssize_t bonding_store_miimon(stru
 				cancel_delayed_work_sync(&bond->lb_arp_work);
 		}
 
-		if (bond->dev->flags & IFF_UP) {
+		if (bond->params.miimon && (bond->dev->flags & IFF_UP)) {
 			/* If the interface is up, we may need to fire off
 			 * the MII timer. If the interface is down, the
 			 * timer will get fired off when the open function

Signature · 2 lines

Makito SHIOKAWA
MIRACLE LINUX CORPORATION

Jarek Poplawski

2008-01-21 13:27 UTC #15 lore raw

Re: [PATCH 3/4] bonding: Fix work rearming

Quoted 2 lines · Makito SHIOKAWA

Quoted 1 line
(But new_value = 0 seems needed - just like from module_param()?)
Do you mean to initialize new_value before sscanf()? (There is a check 'if (sscanf(buf, "%d", &new_value) != 1)', so is it necesarry?)

No: you mentioned about treating new_value == 0 like new_value < 0 with 'if (new_value <= 0)', and I didn't understand this idea...

Quoted 5 lines

Quoted 2 lines
- maybe to test if the value has changed at all,
Tested.
For now, patch will be like below. Any slight comment for this will be helpful, regards.

I think cancelling of delayed works in bond_sysfs is OK now.

Alas I don't understand the reason of this change in bond_main()... Some comment?

Thanks,
Jarek P.

Quoted 103 lines

---
 drivers/net/bonding/bond_main.c  |   11 ++++-------
 drivers/net/bonding/bond_sysfs.c |   19 +++++++++++++++++--
 2 files changed, 21 insertions(+), 9 deletions(-)

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

@@ -2699,7 +2699,7 @@ void bond_loadbalance_arp_mon(struct wor
 
 	read_lock(&bond->lock);
 
-	delta_in_ticks = (bond->params.arp_interval * HZ) / 1000;
+	delta_in_ticks = ((bond->params.arp_interval * HZ) / 1000) ? : 1;
 
 	if (bond->kill_timers) {
 		goto out;

@@ -2801,8 +2801,7 @@ void bond_loadbalance_arp_mon(struct wor
 	}
 
 re_arm:
-	if (bond->params.arp_interval)
-		queue_delayed_work(bond->wq, &bond->lb_arp_work, delta_in_ticks);
+	queue_delayed_work(bond->wq, &bond->lb_arp_work, delta_in_ticks);
 out:
 	read_unlock(&bond->lock);
 }

@@ -2832,7 +2831,7 @@ void bond_activebackup_arp_mon(struct wo
 
 	read_lock(&bond->lock);
 
-	delta_in_ticks = (bond->params.arp_interval * HZ) / 1000;
+	delta_in_ticks = ((bond->params.arp_interval * HZ) / 1000) ? : 1;
 
 	if (bond->kill_timers) {
 		goto out;

@@ -3058,9 +3057,7 @@ void bond_activebackup_arp_mon(struct wo
 	}
 
 re_arm:
-	if (bond->params.arp_interval) {
-		queue_delayed_work(bond->wq, &bond->ab_arp_work, delta_in_ticks);
-	}
+	queue_delayed_work(bond->wq, &bond->ab_arp_work, delta_in_ticks);
 out:
 	read_unlock(&bond->lock);
 }

drivers/net/bonding/bond_sysfs.c +17 -2

@@ -644,6 +644,15 @@ static ssize_t bonding_store_arp_interva
 	       ": %s: Setting ARP monitoring interval to %d.\n",
 	       bond->dev->name, new_value);
 	bond->params.arp_interval = new_value;
+	if (bond->params.arp_interval == 0 && (bond->dev->flags & IFF_UP)) {
+		printk(KERN_INFO DRV_NAME
+		       ": %s: Disabling ARP monitoring.\n",
+		       bond->dev->name);
+		if (bond->params.mode == BOND_MODE_ACTIVEBACKUP)
+			cancel_delayed_work_sync(&bond->ab_arp_work);
+		else
+			cancel_delayed_work_sync(&bond->lb_arp_work);
+	}
 	if (bond->params.miimon) {
 		printk(KERN_INFO DRV_NAME
 		       ": %s: ARP monitoring cannot be used with MII monitoring. "

@@ -658,7 +667,7 @@ static ssize_t bonding_store_arp_interva
 		       "but no ARP targets have been specified.\n",
 		       bond->dev->name);
 	}
-	if (bond->dev->flags & IFF_UP) {
+	if (bond->params.arp_interval && (bond->dev->flags & IFF_UP)) {
 		/* If the interface is up, we may need to fire off
 		 * the ARP timer.  If the interface is down, the
 		 * timer will get fired off when the open function

@@ -997,6 +1006,12 @@ static ssize_t bonding_store_miimon(stru
 		       ": %s: Setting MII monitoring interval to %d.\n",
 		       bond->dev->name, new_value);
 		bond->params.miimon = new_value;
+		if (bond->params.miimon == 0 && (bond->dev->flags & IFF_UP)) {
+			printk(KERN_INFO DRV_NAME
+			       ": %s: Disabling MII monitoring...\n",
+			       bond->dev->name);
+			cancel_delayed_work_sync(&bond->mii_work);
+		}
 		if(bond->params.updelay)
 			printk(KERN_INFO DRV_NAME
 			      ": %s: Note: Updating updelay (to %d) "

@@ -1026,7 +1041,7 @@ static ssize_t bonding_store_miimon(stru
 				cancel_delayed_work_sync(&bond->lb_arp_work);
 		}
 
-		if (bond->dev->flags & IFF_UP) {
+		if (bond->params.miimon && (bond->dev->flags & IFF_UP)) {
 			/* If the interface is up, we may need to fire off
 			 * the MII timer. If the interface is down, the
 			 * timer will get fired off when the open function

Signature · 3 lines

-- 
Makito SHIOKAWA
MIRACLE LINUX CORPORATION

Makito SHIOKAWA

author

2008-01-22 03:33 UTC #16 lore raw

Re: [PATCH 3/4] bonding: Fix work rearming

Quoted 2 lines

No: you mentioned about treating new_value == 0 like new_value < 0 with 'if (new_value <= 0)', and I didn't understand this idea...

I'm sorry to have misunderstood you. I wanted to say that there is a way just to fix 'if (new_value < 0)' to 'if (new_value <= 0)' and reject miimon == 0, instead of PATCH 3/4. (Of course, you won't be able to stop mii monitor only in that case.)

Quoted 2 lines

Alas I don't understand the reason of this change in bond_main()... Some comment?

It was unnecessary any more..., thanks.