[PATCH 0/4] bonding: Fix workqueue manipulation and lock taking

[PATCH 0/4] bonding: Fix workqueue manipulation and lock taking

[Makito SHIOKAWA]

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.

-- 
Makito SHIOKAWA
MIRACLE LINUX CORPORATION 

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

[Makito SHIOKAWA]

[-- Attachment #1: bonding-fix-work-manipulation-in-sysfs.patch --]
[-- Type: text/plain, Size: 7400 bytes --]

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.

Signed-off-by: Makito SHIOKAWA <mshiokawa@miraclelinux.com>
---

 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(-)

--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -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);
 }
 
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -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:
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -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;
 };

-- 
Makito SHIOKAWA
MIRACLE LINUX CORPORATION 

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

[Makito SHIOKAWA]

[-- Attachment #1: bonding-guarantee-destroy-work.patch --]
[-- Type: text/plain, Size: 6186 bytes --]

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.

Signed-off-by: Makito SHIOKAWA <mshiokawa@miraclelinux.com>
---

 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(-)

--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -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();
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -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;
 			}
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -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);

-- 
Makito SHIOKAWA
MIRACLE LINUX CORPORATION 

[PATCH 3/4] bonding: Fix work rearming

[Makito SHIOKAWA]

[-- Attachment #1: bonding-fix-monitor-rearming.patch --]
[-- Type: text/plain, Size: 706 bytes --]

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

Signed-off-by: Makito SHIOKAWA <mshiokawa@miraclelinux.com>
---
 drivers/net/bonding/bond_main.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -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)

-- 
Makito SHIOKAWA
MIRACLE LINUX CORPORATION 

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

[Makito SHIOKAWA]

[-- Attachment #1: bonding-fix-some-rtnl-taking.patch --]
[-- Type: text/plain, Size: 3810 bytes --]

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 <mshiokawa@miraclelinux.com>
---
 drivers/net/bonding/bond_main.c |   24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -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-------------------------------*/

-- 
Makito SHIOKAWA
MIRACLE LINUX CORPORATION 

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

[Jarek Poplawski]

On 15-01-2008 07:36, Makito SHIOKAWA wrote:
> Change code not to rearm bond_mii_monitor() when value 0 is set for miimon.
> 
> Signed-off-by: Makito SHIOKAWA <mshiokawa@miraclelinux.com>
> ---
>  drivers/net/bonding/bond_main.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -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.

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

[Jarek Poplawski]

On 15-01-2008 07:36, Makito SHIOKAWA wrote:
...
> --- a/drivers/net/bonding/bond_sysfs.c
> +++ b/drivers/net/bonding/bond_sysfs.c
> @@ -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.

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

[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.

>> Change code not to rearm bond_mii_monitor() when value 0 is set for miimon.
>>
>> Signed-off-by: Makito SHIOKAWA <mshiokawa@miraclelinux.com>
>> ---
>>  drivers/net/bonding/bond_main.c |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -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.

--
Makito SHIOKAWA
MIRACLE LINUX CORPORATION

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

[Makito SHIOKAWA]

 > 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

-- 
Makito SHIOKAWA
MIRACLE LINUX CORPORATION

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

[Makito SHIOKAWA]

> 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(-)

--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -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);
  }

--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -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:
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -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;
  };

-- 
Makito SHIOKAWA
MIRACLE LINUX CORPORATION

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

[Jarek Poplawski]

On 15-01-2008 07:36, Makito SHIOKAWA wrote:
> 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 <mshiokawa@miraclelinux.com>
> ---
>  drivers/net/bonding/bond_main.c |   24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
> 
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -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;

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.

>  	}
>  
>  	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.

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

Regards,
Jarek P.

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

[Jarek Poplawski]

On Wed, Jan 16, 2008 at 12:19:51PM +0900, Makito SHIOKAWA wrote:
> 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.

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

[Makito SHIOKAWA]

> 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.

-- 
Makito SHIOKAWA
MIRACLE LINUX CORPORATION

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

[Makito SHIOKAWA]

> 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.

> 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.)

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

-- 
Makito SHIOKAWA
MIRACLE LINUX CORPORATION

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

[Jarek Poplawski]

On Thu, Jan 17, 2008 at 02:30:36PM +0900, Makito SHIOKAWA wrote:
>> 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.

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

[Jarek Poplawski]

On Thu, Jan 17, 2008 at 02:30:37PM +0900, Makito SHIOKAWA wrote:
>> 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.
>
>> 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.

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

[Makito SHIOKAWA]

> 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

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

[Makito SHIOKAWA]

> 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(-)

--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -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


-- 
Makito SHIOKAWA
MIRACLE LINUX CORPORATION

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

[Jarek Poplawski]

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

>> 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.

> 
> ---
>   drivers/net/bonding/bond_sysfs.c |    4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> --- a/drivers/net/bonding/bond_sysfs.c
> +++ b/drivers/net/bonding/bond_sysfs.c
> @@ -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
> 
> 

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

[Jarek Poplawski]

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

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

...

> @@ -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)) {


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

Jarek P.

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

[Makito SHIOKAWA]

 > (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?)

 > - 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.


Signed-off-by: Makito SHIOKAWA <mshiokawa@miraclelinux.com>
---
  drivers/net/bonding/bond_main.c  |   11 ++++-------
  drivers/net/bonding/bond_sysfs.c |   19 +++++++++++++++++--
  2 files changed, 21 insertions(+), 9 deletions(-)

--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -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);
  }
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -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


-- 
Makito SHIOKAWA
MIRACLE LINUX CORPORATION

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

[Jarek Poplawski]

On Mon, Jan 21, 2008 at 01:04:06PM +0900, Makito SHIOKAWA wrote:
> > (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...

>
> > - 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.

>
>
> Signed-off-by: Makito SHIOKAWA <mshiokawa@miraclelinux.com>
> ---
>  drivers/net/bonding/bond_main.c  |   11 ++++-------
>  drivers/net/bonding/bond_sysfs.c |   19 +++++++++++++++++--
>  2 files changed, 21 insertions(+), 9 deletions(-)
>
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -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);
>  }
> --- a/drivers/net/bonding/bond_sysfs.c
> +++ b/drivers/net/bonding/bond_sysfs.c
> @@ -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
>
>
> -- 
> Makito SHIOKAWA
> MIRACLE LINUX CORPORATION

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

[Makito SHIOKAWA]

> 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.)

> Alas I don't understand the reason of this change in bond_main()...
> Some comment?
It was unnecessary any more..., thanks.


Signed-off-by: Makito SHIOKAWA <mshiokawa@miraclelinux.com>
---
  drivers/net/bonding/bond_main.c  |    7 ++-----
  drivers/net/bonding/bond_sysfs.c |   19 +++++++++++++++++--
  2 files changed, 19 insertions(+), 7 deletions(-)

--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -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);
  }
@@ -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);
  }
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -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


-- 
Makito SHIOKAWA
MIRACLE LINUX CORPORATION