* [PATCH 0/6] bonding workqueue and lock rework @ 2007-10-11 4:14 Jay Vosburgh 2007-10-11 4:14 ` [PATCH 1/6] Convert bonding timers to workqueues Jay Vosburgh 0 siblings, 1 reply; 17+ messages in thread From: Jay Vosburgh @ 2007-10-11 4:14 UTC (permalink / raw) To: netdev, jgarzik; +Cc: andy Following are patches to update the locking used in the bonding driver. This involves two basic changes: conversion from timers to workqueues for the various periodic monitor functions, and conversion of locking. These patches should resolve the majority of locking and might sleep related warnings that occur during certain operations. The workqueue conversion is fairly straightforward, and substitutes workqueue driven events for the existing timer driven events. The locking changes are of two types: first, changes to resolve deadlocks created by the conversion to workqueues, and second, changes to make the locking more correct, which generally involves holding RTNL and no other locks during specific operations. Some of the RTNL-related changes are fairly extensive, and involve either conditional locking or releasing and reacquiring locks in order to obey lock ordering constraints. These changes were developed and extensively tested by Andy Gospodarek <andy@greyhouse.net> and myself over the last few months. Patches are relative to netdev-2.6#upstream. -J --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/6] Convert bonding timers to workqueues 2007-10-11 4:14 [PATCH 0/6] bonding workqueue and lock rework Jay Vosburgh @ 2007-10-11 4:14 ` Jay Vosburgh 2007-10-11 4:14 ` [PATCH 2/6] Convert balance-rr transmit to new locking Jay Vosburgh 2007-10-15 18:41 ` [PATCH 1/6] Convert bonding timers to workqueues Jeff Garzik 0 siblings, 2 replies; 17+ messages in thread From: Jay Vosburgh @ 2007-10-11 4:14 UTC (permalink / raw) To: netdev, jgarzik; +Cc: andy, Jay Vosburgh Convert bonding timers to workqueues. This converts the various monitor functions to run in periodic work queues instead of timers. This patch introduces the framework and convers the calls, but does not resolve various locking issues, and does not stand alone. Signed-off-by: Andy Gospodarek <andy@greyhouse.net> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com> --- drivers/net/bonding/bond_3ad.c | 6 +- drivers/net/bonding/bond_3ad.h | 2 +- drivers/net/bonding/bond_alb.c | 6 +- drivers/net/bonding/bond_alb.h | 2 +- drivers/net/bonding/bond_main.c | 147 ++++++++++++++++++++----------------- drivers/net/bonding/bond_sysfs.c | 63 +++++----------- drivers/net/bonding/bonding.h | 13 ++-- 7 files changed, 117 insertions(+), 122 deletions(-) diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c index 7a045a3..2471d3b 100644 --- a/drivers/net/bonding/bond_3ad.c +++ b/drivers/net/bonding/bond_3ad.c @@ -2074,8 +2074,10 @@ void bond_3ad_unbind_slave(struct slave *slave) * times out, and it selects an aggregator for the ports that are yet not * related to any aggregator, and selects the active aggregator for a bond. */ -void bond_3ad_state_machine_handler(struct bonding *bond) +void bond_3ad_state_machine_handler(struct work_struct *work) { + struct bonding *bond = container_of(work, struct bonding, + ad_work.work); struct port *port; struct aggregator *aggregator; @@ -2126,7 +2128,7 @@ void bond_3ad_state_machine_handler(struct bonding *bond) } re_arm: - mod_timer(&(BOND_AD_INFO(bond).ad_timer), jiffies + ad_delta_in_ticks); + queue_delayed_work(bond->wq, &bond->ad_work, ad_delta_in_ticks); out: read_unlock(&bond->lock); } diff --git a/drivers/net/bonding/bond_3ad.h b/drivers/net/bonding/bond_3ad.h index 862952f..dde02b0 100644 --- a/drivers/net/bonding/bond_3ad.h +++ b/drivers/net/bonding/bond_3ad.h @@ -276,7 +276,7 @@ struct ad_slave_info { void bond_3ad_initialize(struct bonding *bond, u16 tick_resolution, int lacp_fast); int bond_3ad_bind_slave(struct slave *slave); void bond_3ad_unbind_slave(struct slave *slave); -void bond_3ad_state_machine_handler(struct bonding *bond); +void bond_3ad_state_machine_handler(struct work_struct *); void bond_3ad_adapter_speed_changed(struct slave *slave); void bond_3ad_adapter_duplex_changed(struct slave *slave); void bond_3ad_handle_link_change(struct slave *slave, char link); diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c index aea2217..eb320c3 100644 --- a/drivers/net/bonding/bond_alb.c +++ b/drivers/net/bonding/bond_alb.c @@ -1375,8 +1375,10 @@ out: return 0; } -void bond_alb_monitor(struct bonding *bond) +void bond_alb_monitor(struct work_struct *work) { + struct bonding *bond = container_of(work, struct bonding, + alb_work.work); struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond)); struct slave *slave; int i; @@ -1479,7 +1481,7 @@ void bond_alb_monitor(struct bonding *bond) } re_arm: - mod_timer(&(bond_info->alb_timer), jiffies + alb_delta_in_ticks); + queue_delayed_work(bond->wq, &bond->alb_work, alb_delta_in_ticks); out: read_unlock(&bond->lock); } diff --git a/drivers/net/bonding/bond_alb.h b/drivers/net/bonding/bond_alb.h index fd87264..50968f8 100644 --- a/drivers/net/bonding/bond_alb.h +++ b/drivers/net/bonding/bond_alb.h @@ -125,7 +125,7 @@ void bond_alb_deinit_slave(struct bonding *bond, struct slave *slave); void bond_alb_handle_link_change(struct bonding *bond, struct slave *slave, char link); void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave); int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev); -void bond_alb_monitor(struct bonding *bond); +void bond_alb_monitor(struct work_struct *); int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr); void bond_alb_clear_vlan(struct bonding *bond, unsigned short vlan_id); #endif /* __BOND_ALB_H__ */ diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 64bfec3..28ae961 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -1987,9 +1987,10 @@ static int bond_slave_info_query(struct net_device *bond_dev, struct ifslave *in /*-------------------------------- Monitoring -------------------------------*/ /* this function is called regularly to monitor each slave's link. */ -void bond_mii_monitor(struct net_device *bond_dev) +void bond_mii_monitor(struct work_struct *work) { - struct bonding *bond = bond_dev->priv; + struct bonding *bond = container_of(work, struct bonding, + mii_work.work); struct slave *slave, *oldcurrent; int do_failover = 0; int delta_in_ticks; @@ -2043,7 +2044,7 @@ void bond_mii_monitor(struct net_device *bond_dev) ": %s: link status down for %s " "interface %s, disabling it in " "%d ms.\n", - bond_dev->name, + bond->dev->name, IS_UP(slave_dev) ? ((bond->params.mode == BOND_MODE_ACTIVEBACKUP) ? ((slave == oldcurrent) @@ -2076,7 +2077,7 @@ void bond_mii_monitor(struct net_device *bond_dev) ": %s: link status definitely " "down for interface %s, " "disabling it\n", - bond_dev->name, + bond->dev->name, slave_dev->name); /* notify ad that the link status has changed */ @@ -2102,7 +2103,7 @@ void bond_mii_monitor(struct net_device *bond_dev) printk(KERN_INFO DRV_NAME ": %s: link status up again after %d " "ms for interface %s.\n", - bond_dev->name, + bond->dev->name, (bond->params.downdelay - slave->delay) * bond->params.miimon, slave_dev->name); } @@ -2122,7 +2123,7 @@ void bond_mii_monitor(struct net_device *bond_dev) ": %s: link status up for " "interface %s, enabling it " "in %d ms.\n", - bond_dev->name, + bond->dev->name, slave_dev->name, bond->params.updelay * bond->params.miimon); } @@ -2138,7 +2139,7 @@ void bond_mii_monitor(struct net_device *bond_dev) printk(KERN_INFO DRV_NAME ": %s: link status down again after %d " "ms for interface %s.\n", - bond_dev->name, + bond->dev->name, (bond->params.updelay - slave->delay) * bond->params.miimon, slave_dev->name); } else { @@ -2162,7 +2163,7 @@ void bond_mii_monitor(struct net_device *bond_dev) printk(KERN_INFO DRV_NAME ": %s: link status definitely " "up for interface %s.\n", - bond_dev->name, + bond->dev->name, slave_dev->name); /* notify ad that the link status has changed */ @@ -2188,7 +2189,7 @@ void bond_mii_monitor(struct net_device *bond_dev) /* Should not happen */ printk(KERN_ERR DRV_NAME ": %s: Error: %s Illegal value (link=%d)\n", - bond_dev->name, + bond->dev->name, slave->dev->name, slave->link); goto out; @@ -2218,9 +2219,8 @@ void bond_mii_monitor(struct net_device *bond_dev) bond_set_carrier(bond); re_arm: - if (bond->params.miimon) { - mod_timer(&bond->mii_timer, jiffies + delta_in_ticks); - } + if (bond->params.miimon) + queue_delayed_work(bond->wq, &bond->mii_work, delta_in_ticks); out: read_unlock(&bond->lock); } @@ -2523,9 +2523,10 @@ out: * arp is transmitted to generate traffic. see activebackup_arp_monitor for * arp monitoring in active backup mode. */ -void bond_loadbalance_arp_mon(struct net_device *bond_dev) +void bond_loadbalance_arp_mon(struct work_struct *work) { - struct bonding *bond = bond_dev->priv; + struct bonding *bond = container_of(work, struct bonding, + arp_work.work); struct slave *slave, *oldcurrent; int do_failover = 0; int delta_in_ticks; @@ -2572,13 +2573,13 @@ void bond_loadbalance_arp_mon(struct net_device *bond_dev) printk(KERN_INFO DRV_NAME ": %s: link status definitely " "up for interface %s, ", - bond_dev->name, + bond->dev->name, slave->dev->name); do_failover = 1; } else { printk(KERN_INFO DRV_NAME ": %s: interface %s is now up\n", - bond_dev->name, + bond->dev->name, slave->dev->name); } } @@ -2602,7 +2603,7 @@ void bond_loadbalance_arp_mon(struct net_device *bond_dev) printk(KERN_INFO DRV_NAME ": %s: interface %s is now down.\n", - bond_dev->name, + bond->dev->name, slave->dev->name); if (slave == oldcurrent) { @@ -2632,9 +2633,8 @@ void bond_loadbalance_arp_mon(struct net_device *bond_dev) } re_arm: - if (bond->params.arp_interval) { - mod_timer(&bond->arp_timer, jiffies + delta_in_ticks); - } + if (bond->params.arp_interval) + queue_delayed_work(bond->wq, &bond->arp_work, delta_in_ticks); out: read_unlock(&bond->lock); } @@ -2654,9 +2654,10 @@ out: * may have received. * see loadbalance_arp_monitor for arp monitoring in load balancing mode */ -void bond_activebackup_arp_mon(struct net_device *bond_dev) +void bond_activebackup_arp_mon(struct work_struct *work) { - struct bonding *bond = bond_dev->priv; + struct bonding *bond = container_of(work, struct bonding, + arp_work.work); struct slave *slave; int delta_in_ticks; int i; @@ -2708,14 +2709,14 @@ void bond_activebackup_arp_mon(struct net_device *bond_dev) printk(KERN_INFO DRV_NAME ": %s: %s is up and now the " "active interface\n", - bond_dev->name, + bond->dev->name, slave->dev->name); netif_carrier_on(bond->dev); } else { printk(KERN_INFO DRV_NAME ": %s: backup interface %s is " "now up\n", - bond_dev->name, + bond->dev->name, slave->dev->name); } @@ -2751,7 +2752,7 @@ void bond_activebackup_arp_mon(struct net_device *bond_dev) printk(KERN_INFO DRV_NAME ": %s: backup interface %s is now down\n", - bond_dev->name, + bond->dev->name, slave->dev->name); } else { read_unlock(&bond->curr_slave_lock); @@ -2786,7 +2787,7 @@ void bond_activebackup_arp_mon(struct net_device *bond_dev) printk(KERN_INFO DRV_NAME ": %s: link status down for active interface " "%s, disabling it\n", - bond_dev->name, + bond->dev->name, slave->dev->name); write_lock(&bond->curr_slave_lock); @@ -2808,7 +2809,7 @@ void bond_activebackup_arp_mon(struct net_device *bond_dev) printk(KERN_INFO DRV_NAME ": %s: changing from interface %s to primary " "interface %s\n", - bond_dev->name, + bond->dev->name, slave->dev->name, bond->primary_slave->dev->name); @@ -2872,7 +2873,7 @@ void bond_activebackup_arp_mon(struct net_device *bond_dev) printk(KERN_INFO DRV_NAME ": %s: backup interface %s is " "now down.\n", - bond_dev->name, + bond->dev->name, slave->dev->name); } } @@ -2881,7 +2882,7 @@ void bond_activebackup_arp_mon(struct net_device *bond_dev) re_arm: if (bond->params.arp_interval) { - mod_timer(&bond->arp_timer, jiffies + delta_in_ticks); + queue_delayed_work(bond->wq, &bond->arp_work, delta_in_ticks); } out: read_unlock(&bond->lock); @@ -3460,15 +3461,11 @@ static int bond_xmit_hash_policy_l2(struct sk_buff *skb, static int bond_open(struct net_device *bond_dev) { struct bonding *bond = bond_dev->priv; - struct timer_list *mii_timer = &bond->mii_timer; - struct timer_list *arp_timer = &bond->arp_timer; bond->kill_timers = 0; if ((bond->params.mode == BOND_MODE_TLB) || (bond->params.mode == BOND_MODE_ALB)) { - struct timer_list *alb_timer = &(BOND_ALB_INFO(bond).alb_timer); - /* bond_alb_initialize must be called before the timer * is started. */ @@ -3477,44 +3474,31 @@ static int bond_open(struct net_device *bond_dev) return -1; } - init_timer(alb_timer); - alb_timer->expires = jiffies + 1; - alb_timer->data = (unsigned long)bond; - alb_timer->function = (void *)&bond_alb_monitor; - add_timer(alb_timer); + 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_timer(mii_timer); - mii_timer->expires = jiffies + 1; - mii_timer->data = (unsigned long)bond_dev; - mii_timer->function = (void *)&bond_mii_monitor; - add_timer(mii_timer); + 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. */ - init_timer(arp_timer); - arp_timer->expires = jiffies + 1; - arp_timer->data = (unsigned long)bond_dev; - if (bond->params.mode == BOND_MODE_ACTIVEBACKUP) { - arp_timer->function = (void *)&bond_activebackup_arp_mon; - } else { - arp_timer->function = (void *)&bond_loadbalance_arp_mon; - } + 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.arp_validate) bond_register_arp(bond); - - add_timer(arp_timer); } if (bond->params.mode == BOND_MODE_8023AD) { - struct timer_list *ad_timer = &(BOND_AD_INFO(bond).ad_timer); - init_timer(ad_timer); - ad_timer->expires = jiffies + 1; - ad_timer->data = (unsigned long)bond; - ad_timer->function = (void *)&bond_3ad_state_machine_handler; - add_timer(ad_timer); - + INIT_DELAYED_WORK(&bond->ad_work, bond_alb_monitor); + queue_delayed_work(bond->wq, &bond->ad_work, 0); /* register to receive LACPDUs */ bond_register_lacpdu(bond); } @@ -3542,25 +3526,21 @@ static int bond_close(struct net_device *bond_dev) write_unlock_bh(&bond->lock); - /* del_timer_sync must run without holding the bond->lock - * because a running timer might be trying to hold it too - */ - if (bond->params.miimon) { /* link check interval, in milliseconds. */ - del_timer_sync(&bond->mii_timer); + cancel_delayed_work(&bond->mii_work); } if (bond->params.arp_interval) { /* arp interval, in milliseconds. */ - del_timer_sync(&bond->arp_timer); + cancel_delayed_work(&bond->arp_work); } switch (bond->params.mode) { case BOND_MODE_8023AD: - del_timer_sync(&(BOND_AD_INFO(bond).ad_timer)); + cancel_delayed_work(&bond->ad_work); break; case BOND_MODE_TLB: case BOND_MODE_ALB: - del_timer_sync(&(BOND_ALB_INFO(bond).alb_timer)); + cancel_delayed_work(&bond->alb_work); break; default: break; @@ -4211,6 +4191,10 @@ static int bond_init(struct net_device *bond_dev, struct bond_params *params) bond->params = *params; /* copy params struct */ + bond->wq = create_singlethread_workqueue(bond_dev->name); + if (!bond->wq) + return -ENOMEM; + /* Initialize pointers */ bond->first_slave = NULL; bond->curr_active_slave = NULL; @@ -4690,10 +4674,32 @@ out_rtnl: return res; } +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 && delayed_work_pending(&bond->mii_work)) + 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.mode == BOND_MODE_ALB && + delayed_work_pending(&bond->alb_work)) + cancel_delayed_work(&bond->alb_work); + + if (bond->params.mode == BOND_MODE_8023AD && + delayed_work_pending(&bond->ad_work)) + cancel_delayed_work(&bond->ad_work); +} + static int __init bonding_init(void) { int i; int res; + struct bonding *bond, *nxt; printk(KERN_INFO "%s", version); @@ -4720,6 +4726,11 @@ 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(); diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c index 6f49ca7..cf102e6 100644 --- a/drivers/net/bonding/bond_sysfs.c +++ b/drivers/net/bonding/bond_sysfs.c @@ -609,12 +609,9 @@ static ssize_t bonding_store_arp_interval(struct device *d, "%s Disabling MII monitoring.\n", bond->dev->name, bond->dev->name); bond->params.miimon = 0; - /* Kill MII timer, else it brings bond's link down */ - if (bond->arp_timer.function) { - printk(KERN_INFO DRV_NAME - ": %s: Kill MII timer, else it brings bond's link down...\n", - bond->dev->name); - del_timer_sync(&bond->mii_timer); + if (delayed_work_pending(&bond->mii_work)) { + cancel_delayed_work(&bond->mii_work); + flush_workqueue(bond->wq); } } if (!bond->params.arp_targets[0]) { @@ -629,25 +626,15 @@ static ssize_t bonding_store_arp_interval(struct device *d, * timer will get fired off when the open function * is called. */ - if (bond->arp_timer.function) { - /* The timer's already set up, so fire it off */ - mod_timer(&bond->arp_timer, jiffies + 1); - } else { - /* Set up the timer. */ - init_timer(&bond->arp_timer); - bond->arp_timer.expires = jiffies + 1; - bond->arp_timer.data = - (unsigned long) bond->dev; - if (bond->params.mode == BOND_MODE_ACTIVEBACKUP) { - bond->arp_timer.function = - (void *) - &bond_activebackup_arp_mon; - } else { - bond->arp_timer.function = - (void *) - &bond_loadbalance_arp_mon; - } - add_timer(&bond->arp_timer); + 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); } } @@ -1003,12 +990,9 @@ static ssize_t bonding_store_miimon(struct device *d, bond->params.arp_validate = BOND_ARP_VALIDATE_NONE; } - /* Kill ARP timer, else it brings bond's link down */ - if (bond->mii_timer.function) { - printk(KERN_INFO DRV_NAME - ": %s: Kill ARP timer, else it brings bond's link down...\n", - bond->dev->name); - del_timer_sync(&bond->arp_timer); + if (delayed_work_pending(&bond->arp_work)) { + cancel_delayed_work(&bond->arp_work); + flush_workqueue(bond->wq); } } @@ -1018,18 +1002,11 @@ static ssize_t bonding_store_miimon(struct device *d, * timer will get fired off when the open function * is called. */ - if (bond->mii_timer.function) { - /* The timer's already set up, so fire it off */ - mod_timer(&bond->mii_timer, jiffies + 1); - } else { - /* Set up the timer. */ - init_timer(&bond->mii_timer); - bond->mii_timer.expires = jiffies + 1; - bond->mii_timer.data = - (unsigned long) bond->dev; - bond->mii_timer.function = - (void *) &bond_mii_monitor; - add_timer(&bond->mii_timer); + 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); } } } diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h index 2a6af7d..4986d0f 100644 --- a/drivers/net/bonding/bonding.h +++ b/drivers/net/bonding/bonding.h @@ -182,8 +182,6 @@ struct bonding { s32 slave_cnt; /* never change this value outside the attach/detach wrappers */ rwlock_t lock; rwlock_t curr_slave_lock; - struct timer_list mii_timer; - struct timer_list arp_timer; s8 kill_timers; struct net_device_stats stats; #ifdef CONFIG_PROC_FS @@ -201,6 +199,11 @@ struct bonding { struct list_head vlan_list; struct vlan_group *vlgrp; struct packet_type arp_mon_pt; + struct workqueue_struct *wq; + struct delayed_work mii_work; + struct delayed_work arp_work; + struct delayed_work alb_work; + struct delayed_work ad_work; }; /** @@ -301,9 +304,9 @@ int bond_create_slave_symlinks(struct net_device *master, struct net_device *sla void bond_destroy_slave_symlinks(struct net_device *master, struct net_device *slave); int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev); int bond_release(struct net_device *bond_dev, struct net_device *slave_dev); -void bond_mii_monitor(struct net_device *bond_dev); -void bond_loadbalance_arp_mon(struct net_device *bond_dev); -void bond_activebackup_arp_mon(struct net_device *bond_dev); +void bond_mii_monitor(struct work_struct *); +void bond_loadbalance_arp_mon(struct work_struct *); +void bond_activebackup_arp_mon(struct work_struct *); void bond_set_mode_ops(struct bonding *bond, int mode); int bond_parse_parm(char *mode_arg, struct bond_parm_tbl *tbl); void bond_select_active_slave(struct bonding *bond); -- 1.5.3.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/6] Convert balance-rr transmit to new locking 2007-10-11 4:14 ` [PATCH 1/6] Convert bonding timers to workqueues Jay Vosburgh @ 2007-10-11 4:14 ` Jay Vosburgh 2007-10-11 4:14 ` [PATCH 3/6] Convert miimon " Jay Vosburgh 2007-10-15 18:41 ` [PATCH 1/6] Convert bonding timers to workqueues Jeff Garzik 1 sibling, 1 reply; 17+ messages in thread From: Jay Vosburgh @ 2007-10-11 4:14 UTC (permalink / raw) To: netdev, jgarzik; +Cc: andy, Jay Vosburgh Change locking in balance-rr transmit processing to use a free running counter to determine which slave to transmit on. Instead, a free-running counter is maintained, and modulo arithmetic used to select a slave for transmit. This removes lock operations from the TX path, and eliminates a deadlock introduced by the conversion to work queues. Signed-off-by: Andy Gospodarek <andy@greyhouse.net> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com> --- drivers/net/bonding/bond_main.c | 25 ++++++++++++------------- drivers/net/bonding/bonding.h | 1 + 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 28ae961..4541a1b 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -3928,8 +3928,7 @@ static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev { struct bonding *bond = bond_dev->priv; struct slave *slave, *start_at; - int i; - int res = 1; + int i, slave_no, res = 1; read_lock(&bond->lock); @@ -3937,29 +3936,29 @@ static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev goto out; } - read_lock(&bond->curr_slave_lock); - slave = start_at = bond->curr_active_slave; - read_unlock(&bond->curr_slave_lock); + /* + * Concurrent TX may collide on rr_tx_counter; we accept that + * as being rare enough not to justify using an atomic op here + */ + slave_no = bond->rr_tx_counter++ % bond->slave_cnt; - if (!slave) { - goto out; + bond_for_each_slave(bond, slave, i) { + slave_no--; + if (slave_no < 0) { + break; + } } + start_at = slave; bond_for_each_slave_from(bond, slave, i, start_at) { if (IS_UP(slave->dev) && (slave->link == BOND_LINK_UP) && (slave->state == BOND_STATE_ACTIVE)) { res = bond_dev_queue_xmit(bond, skb, slave->dev); - - write_lock(&bond->curr_slave_lock); - bond->curr_active_slave = slave->next; - write_unlock(&bond->curr_slave_lock); - break; } } - out: if (res) { /* no suitable interface, frame not sent */ diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h index 4986d0f..e7c3671 100644 --- a/drivers/net/bonding/bonding.h +++ b/drivers/net/bonding/bonding.h @@ -183,6 +183,7 @@ struct bonding { rwlock_t lock; rwlock_t curr_slave_lock; s8 kill_timers; + u16 rr_tx_counter; struct net_device_stats stats; #ifdef CONFIG_PROC_FS struct proc_dir_entry *proc_entry; -- 1.5.3.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 3/6] Convert miimon to new locking 2007-10-11 4:14 ` [PATCH 2/6] Convert balance-rr transmit to new locking Jay Vosburgh @ 2007-10-11 4:14 ` Jay Vosburgh 2007-10-11 4:14 ` [PATCH 4/6] Convert locks to _bh, rework alb locking for " Jay Vosburgh 0 siblings, 1 reply; 17+ messages in thread From: Jay Vosburgh @ 2007-10-11 4:14 UTC (permalink / raw) To: netdev, jgarzik; +Cc: andy, Jay Vosburgh Convert mii (link state) monitor to acquire correct locks for failover events. In particular, failovers generally require RTNL at a low level (when manipulating device MAC addresses, for example) and no other locks. The high level monitor is responsible for acquiring a known set of locks, RTNL, the bond->lock for read and the slave_lock for write, and the low level failover processing can then release appropriate locks as needed. This patch provides the high level portion. As it is undesirable to acquire RTNL for every monitor pass (which may occur as often as every 10 ms), the miimon has been converted to do conditional locking. A first pass inspects all slaves to determine if any action is required, and if so, a second pass (after acquring RTNL) is done to perform any actions (doing a complete rescan, as the situation may have changed when all locks were released). Signed-off-by: Andy Gospodarek <andy@greyhouse.net> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com> --- drivers/net/bonding/bond_main.c | 70 ++++++++++++++++++++++++++++---------- 1 files changed, 51 insertions(+), 19 deletions(-) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 4541a1b..d7f98c2 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -1986,27 +1986,25 @@ static int bond_slave_info_query(struct net_device *bond_dev, struct ifslave *in /*-------------------------------- Monitoring -------------------------------*/ -/* this function is called regularly to monitor each slave's link. */ -void bond_mii_monitor(struct work_struct *work) +/* + * if !have_locks, return nonzero if a failover is necessary. if + * have_locks, do whatever failover activities are needed. + * + * This is to separate the inspection and failover steps for locking + * purposes; failover requires rtnl, but acquiring it for every + * inspection is undesirable, so a wrapper first does inspection, and + * the acquires the necessary locks and calls again to perform + * failover if needed. Since all locks are dropped, a complete + * restart is needed between calls. + */ +static int __bond_mii_monitor(struct bonding *bond, int have_locks) { - struct bonding *bond = container_of(work, struct bonding, - mii_work.work); struct slave *slave, *oldcurrent; int do_failover = 0; - int delta_in_ticks; int i; - read_lock(&bond->lock); - - delta_in_ticks = (bond->params.miimon * HZ) / 1000; - - if (bond->kill_timers) { + if (bond->slave_cnt == 0) goto out; - } - - if (bond->slave_cnt == 0) { - goto re_arm; - } /* we will try to read the link status of each of our slaves, and * set their IFF_RUNNING flag appropriately. For each slave not @@ -2062,6 +2060,9 @@ void bond_mii_monitor(struct work_struct *work) if (link_state != BMSR_LSTATUS) { /* link stays down */ if (slave->delay <= 0) { + if (!have_locks) + return 1; + /* link down for too long time */ slave->link = BOND_LINK_DOWN; @@ -2145,6 +2146,9 @@ void bond_mii_monitor(struct work_struct *work) } else { /* link stays up */ if (slave->delay == 0) { + if (!have_locks) + return 1; + /* now the link has been up for long time enough */ slave->link = BOND_LINK_UP; slave->jiffies = jiffies; @@ -2218,13 +2222,41 @@ void bond_mii_monitor(struct work_struct *work) } else bond_set_carrier(bond); -re_arm: - if (bond->params.miimon) - queue_delayed_work(bond->wq, &bond->mii_work, delta_in_ticks); out: - read_unlock(&bond->lock); + return 0; } +/* + * bond_mii_monitor + * + * Really a wrapper that splits the mii monitor into two phases: an + * inspection, then (if inspection indicates something needs to be + * done) an acquisition of appropriate locks followed by another pass + * to implement whatever link state changes are indicated. + */ +void bond_mii_monitor(struct work_struct *work) +{ + struct bonding *bond = container_of(work, struct bonding, + mii_work.work); + unsigned long delay; + + read_lock(&bond->lock); + if (bond->kill_timers) { + read_unlock(&bond->lock); + return; + } + if (__bond_mii_monitor(bond, 0)) { + read_unlock(&bond->lock); + rtnl_lock(); + read_lock(&bond->lock); + __bond_mii_monitor(bond, 1); + rtnl_unlock(); + } + + delay = ((bond->params.miimon * HZ) / 1000) ? : 1; + read_unlock(&bond->lock); + queue_delayed_work(bond->wq, &bond->mii_work, delay); +} static __be32 bond_glean_dev_ip(struct net_device *dev) { -- 1.5.3.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4/6] Convert locks to _bh, rework alb locking for new locking 2007-10-11 4:14 ` [PATCH 3/6] Convert miimon " Jay Vosburgh @ 2007-10-11 4:14 ` Jay Vosburgh 2007-10-11 4:14 ` [PATCH 5/6] Convert more locks to _bh, acquire rtnl, " Jay Vosburgh 0 siblings, 1 reply; 17+ messages in thread From: Jay Vosburgh @ 2007-10-11 4:14 UTC (permalink / raw) To: netdev, jgarzik; +Cc: andy, Jay Vosburgh Convert locking-related activity to new & improved system. Convert some lock acquisitions to _bh and rework parts of ALB mode, both to avoid deadlocks with workqueue activity. Signed-off-by: Andy Gospodarek <andy@greyhouse.net> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com> --- drivers/net/bonding/bond_alb.c | 69 +++++++++++++++++++++++++++++++--- drivers/net/bonding/bond_main.c | 78 +++++++++++++++++++++++++++------------ 2 files changed, 117 insertions(+), 30 deletions(-) diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c index eb320c3..f2e2872 100644 --- a/drivers/net/bonding/bond_alb.c +++ b/drivers/net/bonding/bond_alb.c @@ -959,19 +959,34 @@ static int alb_set_slave_mac_addr(struct slave *slave, u8 addr[], int hw) return 0; } -/* Caller must hold bond lock for write or curr_slave_lock for write*/ +/* + * Swap MAC addresses between two slaves. + * + * Called with RTNL held, and no other locks. + * + */ + static void alb_swap_mac_addr(struct bonding *bond, struct slave *slave1, struct slave *slave2) { - struct slave *disabled_slave = NULL; u8 tmp_mac_addr[ETH_ALEN]; - int slaves_state_differ; - - slaves_state_differ = (SLAVE_IS_OK(slave1) != SLAVE_IS_OK(slave2)); memcpy(tmp_mac_addr, slave1->dev->dev_addr, ETH_ALEN); alb_set_slave_mac_addr(slave1, slave2->dev->dev_addr, bond->alb_info.rlb_enabled); alb_set_slave_mac_addr(slave2, tmp_mac_addr, bond->alb_info.rlb_enabled); +} + +/* + * Send learning packets after MAC address swap. + * + * Called with RTNL and bond->lock held for read. + */ +static void alb_fasten_mac_swap(struct bonding *bond, struct slave *slave1, + struct slave *slave2) +{ + int slaves_state_differ = (SLAVE_IS_OK(slave1) != SLAVE_IS_OK(slave2)); + struct slave *disabled_slave = NULL; + /* fasten the change in the switch */ if (SLAVE_IS_OK(slave1)) { alb_send_learning_packets(slave1, slave1->dev->dev_addr); @@ -1044,7 +1059,9 @@ static void alb_change_hw_addr_on_detach(struct bonding *bond, struct slave *sla } if (found) { + /* locking: needs RTNL and nothing else */ alb_swap_mac_addr(bond, slave, tmp_slave); + alb_fasten_mac_swap(bond, slave, tmp_slave); } } } @@ -1571,13 +1588,21 @@ void bond_alb_handle_link_change(struct bonding *bond, struct slave *slave, char * Set the bond->curr_active_slave to @new_slave and handle * mac address swapping and promiscuity changes as needed. * - * Caller must hold bond curr_slave_lock for write (or bond lock for write) + * If new_slave is NULL, caller must hold curr_slave_lock or + * bond->lock for write. + * + * If new_slave is not NULL, caller must hold RTNL, bond->lock for + * read and curr_slave_lock for write. Processing here may sleep, so + * no other locks may be held. */ void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave) { struct slave *swap_slave; int i; + if (new_slave) + ASSERT_RTNL(); + if (bond->curr_active_slave == new_slave) { return; } @@ -1610,6 +1635,19 @@ void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave } } + /* + * Arrange for swap_slave and new_slave to temporarily be + * ignored so we can mess with their MAC addresses without + * fear of interference from transmit activity. + */ + if (swap_slave) { + tlb_clear_slave(bond, swap_slave, 1); + } + tlb_clear_slave(bond, new_slave, 1); + + write_unlock_bh(&bond->curr_slave_lock); + read_unlock(&bond->lock); + /* curr_active_slave must be set before calling alb_swap_mac_addr */ if (swap_slave) { /* swap mac address */ @@ -1618,11 +1656,23 @@ void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave /* set the new_slave to the bond mac address */ alb_set_slave_mac_addr(new_slave, bond->dev->dev_addr, bond->alb_info.rlb_enabled); + } + + read_lock(&bond->lock); + + if (swap_slave) { + alb_fasten_mac_swap(bond, swap_slave, new_slave); + } else { /* fasten bond mac on new current slave */ alb_send_learning_packets(new_slave, bond->dev->dev_addr); } + + write_lock_bh(&bond->curr_slave_lock); } +/* + * Called with RTNL + */ int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr) { struct bonding *bond = bond_dev->priv; @@ -1659,8 +1709,12 @@ int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr) } } + write_unlock_bh(&bond->curr_slave_lock); + read_unlock(&bond->lock); + if (swap_slave) { alb_swap_mac_addr(bond, swap_slave, bond->curr_active_slave); + alb_fasten_mac_swap(bond, swap_slave, bond->curr_active_slave); } else { alb_set_slave_mac_addr(bond->curr_active_slave, bond_dev->dev_addr, bond->alb_info.rlb_enabled); @@ -1672,6 +1726,9 @@ int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr) } } + read_lock(&bond->lock); + write_lock_bh(&bond->curr_slave_lock); + return 0; } diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index d7f98c2..2bb5b50 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -1523,15 +1523,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) case BOND_MODE_TLB: case BOND_MODE_ALB: new_slave->state = BOND_STATE_ACTIVE; - if ((!bond->curr_active_slave) && - (new_slave->link != BOND_LINK_DOWN)) { - /* first slave or no active slave yet, and this link - * is OK, so make this interface the active one - */ - bond_change_active_slave(bond, new_slave); - } else { - bond_set_slave_inactive_flags(new_slave); - } + bond_set_slave_inactive_flags(new_slave); break; default: dprintk("This slave is always active in trunk mode\n"); @@ -1685,9 +1677,23 @@ int bond_release(struct net_device *bond_dev, struct net_device *slave_dev) bond_alb_deinit_slave(bond, slave); } - if (oldcurrent == slave) + if (oldcurrent == slave) { + /* + * Note that we hold RTNL over this sequence, so there + * is no concern that another slave add/remove event + * will interfere. + */ + write_unlock_bh(&bond->lock); + read_lock(&bond->lock); + write_lock_bh(&bond->curr_slave_lock); + bond_select_active_slave(bond); + write_unlock_bh(&bond->curr_slave_lock); + read_unlock(&bond->lock); + write_lock_bh(&bond->lock); + } + if (bond->slave_cnt == 0) { bond_set_carrier(bond); @@ -1910,16 +1916,19 @@ static int bond_ioctl_change_active(struct net_device *bond_dev, struct net_devi return -EINVAL; } - write_lock_bh(&bond->lock); + read_lock(&bond->lock); + read_lock(&bond->curr_slave_lock); old_active = bond->curr_active_slave; + read_unlock(&bond->curr_slave_lock); + new_active = bond_get_slave_by_dev(bond, slave_dev); /* * Changing to the current active: do nothing; return success. */ if (new_active && (new_active == old_active)) { - write_unlock_bh(&bond->lock); + read_unlock(&bond->lock); return 0; } @@ -1927,12 +1936,14 @@ static int bond_ioctl_change_active(struct net_device *bond_dev, struct net_devi (old_active) && (new_active->link == BOND_LINK_UP) && IS_UP(new_active->dev)) { + write_lock_bh(&bond->curr_slave_lock); bond_change_active_slave(bond, new_active); + write_unlock_bh(&bond->curr_slave_lock); } else { res = -EINVAL; } - write_unlock_bh(&bond->lock); + read_unlock(&bond->lock); return res; } @@ -2027,7 +2038,11 @@ static int __bond_mii_monitor(struct bonding *bond, int have_locks) switch (slave->link) { case BOND_LINK_UP: /* the link was up */ if (link_state == BMSR_LSTATUS) { - /* link stays up, nothing more to do */ + if (!oldcurrent) { + if (!have_locks) + return 1; + do_failover = 1; + } break; } else { /* link going down */ slave->link = BOND_LINK_FAIL; @@ -2214,11 +2229,14 @@ static int __bond_mii_monitor(struct bonding *bond, int have_locks) } /* end of for */ if (do_failover) { - write_lock(&bond->curr_slave_lock); + ASSERT_RTNL(); + + write_lock_bh(&bond->curr_slave_lock); bond_select_active_slave(bond); - write_unlock(&bond->curr_slave_lock); + write_unlock_bh(&bond->curr_slave_lock); + } else bond_set_carrier(bond); @@ -2657,11 +2675,14 @@ void bond_loadbalance_arp_mon(struct work_struct *work) } if (do_failover) { - write_lock(&bond->curr_slave_lock); + rtnl_lock(); + write_lock_bh(&bond->curr_slave_lock); bond_select_active_slave(bond); - write_unlock(&bond->curr_slave_lock); + write_unlock_bh(&bond->curr_slave_lock); + rtnl_unlock(); + } re_arm: @@ -2718,7 +2739,9 @@ void bond_activebackup_arp_mon(struct work_struct *work) slave->link = BOND_LINK_UP; - write_lock(&bond->curr_slave_lock); + rtnl_lock(); + + write_lock_bh(&bond->curr_slave_lock); if ((!bond->curr_active_slave) && ((jiffies - slave->dev->trans_start) <= delta_in_ticks)) { @@ -2752,7 +2775,8 @@ void bond_activebackup_arp_mon(struct work_struct *work) slave->dev->name); } - write_unlock(&bond->curr_slave_lock); + write_unlock_bh(&bond->curr_slave_lock); + rtnl_unlock(); } } else { read_lock(&bond->curr_slave_lock); @@ -2822,12 +2846,15 @@ void bond_activebackup_arp_mon(struct work_struct *work) bond->dev->name, slave->dev->name); - write_lock(&bond->curr_slave_lock); + rtnl_lock(); + write_lock_bh(&bond->curr_slave_lock); bond_select_active_slave(bond); slave = bond->curr_active_slave; - write_unlock(&bond->curr_slave_lock); + write_unlock_bh(&bond->curr_slave_lock); + + rtnl_unlock(); bond->current_arp_slave = slave; @@ -2846,9 +2873,12 @@ void bond_activebackup_arp_mon(struct work_struct *work) bond->primary_slave->dev->name); /* primary is up so switch to it */ - write_lock(&bond->curr_slave_lock); + rtnl_lock(); + write_lock_bh(&bond->curr_slave_lock); bond_change_active_slave(bond, bond->primary_slave); - write_unlock(&bond->curr_slave_lock); + write_unlock_bh(&bond->curr_slave_lock); + + rtnl_unlock(); slave = bond->primary_slave; slave->jiffies = jiffies; -- 1.5.3.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 5/6] Convert more locks to _bh, acquire rtnl, for new locking 2007-10-11 4:14 ` [PATCH 4/6] Convert locks to _bh, rework alb locking for " Jay Vosburgh @ 2007-10-11 4:14 ` Jay Vosburgh 2007-10-11 4:14 ` [PATCH 6/6] Acquire correct locks in alb for promisc change Jay Vosburgh 0 siblings, 1 reply; 17+ messages in thread From: Jay Vosburgh @ 2007-10-11 4:14 UTC (permalink / raw) To: netdev, jgarzik; +Cc: andy, Jay Vosburgh Convert more lock acquisitions to _bh flavor to avoid deadlock with workqueue activity and add acquisition of RTNL in appropriate places. Affects ALB mode, as well as core bonding functions and sysfs. Signed-off-by: Andy Gospodarek <andy@greyhouse.net> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com> --- drivers/net/bonding/bond_alb.c | 16 ++++++++-------- drivers/net/bonding/bond_main.c | 18 +++++++++--------- drivers/net/bonding/bond_sysfs.c | 18 ++++++++++++------ 3 files changed, 29 insertions(+), 23 deletions(-) diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c index f2e2872..6db5d76 100644 --- a/drivers/net/bonding/bond_alb.c +++ b/drivers/net/bonding/bond_alb.c @@ -128,12 +128,12 @@ static inline u8 _simple_hash(const u8 *hash_start, int hash_size) static inline void _lock_tx_hashtbl(struct bonding *bond) { - spin_lock(&(BOND_ALB_INFO(bond).tx_hashtbl_lock)); + spin_lock_bh(&(BOND_ALB_INFO(bond).tx_hashtbl_lock)); } static inline void _unlock_tx_hashtbl(struct bonding *bond) { - spin_unlock(&(BOND_ALB_INFO(bond).tx_hashtbl_lock)); + spin_unlock_bh(&(BOND_ALB_INFO(bond).tx_hashtbl_lock)); } /* Caller must hold tx_hashtbl lock */ @@ -305,12 +305,12 @@ static struct slave *tlb_choose_channel(struct bonding *bond, u32 hash_index, u3 /*********************** rlb specific functions ***************************/ static inline void _lock_rx_hashtbl(struct bonding *bond) { - spin_lock(&(BOND_ALB_INFO(bond).rx_hashtbl_lock)); + spin_lock_bh(&(BOND_ALB_INFO(bond).rx_hashtbl_lock)); } static inline void _unlock_rx_hashtbl(struct bonding *bond) { - spin_unlock(&(BOND_ALB_INFO(bond).rx_hashtbl_lock)); + spin_unlock_bh(&(BOND_ALB_INFO(bond).rx_hashtbl_lock)); } /* when an ARP REPLY is received from a client update its info @@ -472,13 +472,13 @@ static void rlb_clear_slave(struct bonding *bond, struct slave *slave) _unlock_rx_hashtbl(bond); - write_lock(&bond->curr_slave_lock); + write_lock_bh(&bond->curr_slave_lock); if (slave != bond->curr_active_slave) { rlb_teach_disabled_mac_on_primary(bond, slave->dev->dev_addr); } - write_unlock(&bond->curr_slave_lock); + write_unlock_bh(&bond->curr_slave_lock); } static void rlb_update_client(struct rlb_client_info *client_info) @@ -1519,11 +1519,11 @@ int bond_alb_init_slave(struct bonding *bond, struct slave *slave) /* caller must hold the bond lock for write since the mac addresses * are compared and may be swapped. */ - write_lock_bh(&bond->lock); + read_lock(&bond->lock); res = alb_handle_addr_collision_on_attach(bond, slave); - write_unlock_bh(&bond->lock); + read_unlock(&bond->lock); if (res) { return res; diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 2bb5b50..5bd1e2e 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -1955,9 +1955,9 @@ static int bond_info_query(struct net_device *bond_dev, struct ifbond *info) info->bond_mode = bond->params.mode; info->miimon = bond->params.miimon; - read_lock_bh(&bond->lock); + read_lock(&bond->lock); info->num_slaves = bond->slave_cnt; - read_unlock_bh(&bond->lock); + read_unlock(&bond->lock); return 0; } @@ -1972,7 +1972,7 @@ static int bond_slave_info_query(struct net_device *bond_dev, struct ifslave *in return -ENODEV; } - read_lock_bh(&bond->lock); + read_lock(&bond->lock); bond_for_each_slave(bond, slave, i) { if (i == (int)info->slave_id) { @@ -1981,7 +1981,7 @@ static int bond_slave_info_query(struct net_device *bond_dev, struct ifslave *in } } - read_unlock_bh(&bond->lock); + read_unlock(&bond->lock); if (found) { strcpy(info->slave_name, slave->dev->name); @@ -2965,7 +2965,7 @@ static void *bond_info_seq_start(struct seq_file *seq, loff_t *pos) /* make sure the bond won't be taken away */ read_lock(&dev_base_lock); - read_lock_bh(&bond->lock); + read_lock(&bond->lock); if (*pos == 0) { return SEQ_START_TOKEN; @@ -2999,7 +2999,7 @@ static void bond_info_seq_stop(struct seq_file *seq, void *v) { struct bonding *bond = seq->private; - read_unlock_bh(&bond->lock); + read_unlock(&bond->lock); read_unlock(&dev_base_lock); } @@ -3699,13 +3699,13 @@ static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd if (mii->reg_num == 1) { struct bonding *bond = bond_dev->priv; mii->val_out = 0; - read_lock_bh(&bond->lock); + read_lock(&bond->lock); read_lock(&bond->curr_slave_lock); if (netif_carrier_ok(bond->dev)) { mii->val_out = BMSR_LSTATUS; } read_unlock(&bond->curr_slave_lock); - read_unlock_bh(&bond->lock); + read_unlock(&bond->lock); } return 0; @@ -4343,8 +4343,8 @@ static void bond_free_all(void) bond_mc_list_destroy(bond); /* Release the bonded slaves */ bond_release_all(bond_dev); - bond_deinit(bond_dev); unregister_netdevice(bond_dev); + bond_deinit(bond_dev); } #ifdef CONFIG_PROC_FS diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c index cf102e6..ac507d9 100644 --- a/drivers/net/bonding/bond_sysfs.c +++ b/drivers/net/bonding/bond_sysfs.c @@ -164,9 +164,9 @@ static ssize_t bonding_store_bonds(struct class *cls, const char *buffer, size_t printk(KERN_INFO DRV_NAME ": %s is being deleted...\n", bond->dev->name); + unregister_netdevice(bond->dev); bond_deinit(bond->dev); bond_destroy_sysfs_entry(bond); - unregister_netdevice(bond->dev); rtnl_unlock(); goto out; } @@ -231,7 +231,7 @@ static ssize_t bonding_show_slaves(struct device *d, int i, res = 0; struct bonding *bond = to_bond(d); - read_lock_bh(&bond->lock); + read_lock(&bond->lock); bond_for_each_slave(bond, slave, i) { if (res > (PAGE_SIZE - IFNAMSIZ)) { /* not enough space for another interface name */ @@ -242,7 +242,7 @@ static ssize_t bonding_show_slaves(struct device *d, } res += sprintf(buf + res, "%s ", slave->dev->name); } - read_unlock_bh(&bond->lock); + read_unlock(&bond->lock); res += sprintf(buf + res, "\n"); res++; return res; @@ -285,18 +285,18 @@ static ssize_t bonding_store_slaves(struct device *d, /* Got a slave name in ifname. Is it already in the list? */ found = 0; - read_lock_bh(&bond->lock); + read_lock(&bond->lock); bond_for_each_slave(bond, slave, i) if (strnicmp(slave->dev->name, ifname, IFNAMSIZ) == 0) { printk(KERN_ERR DRV_NAME ": %s: Interface %s is already enslaved!\n", bond->dev->name, ifname); ret = -EPERM; - read_unlock_bh(&bond->lock); + read_unlock(&bond->lock); goto out; } - read_unlock_bh(&bond->lock); + read_unlock(&bond->lock); printk(KERN_INFO DRV_NAME ": %s: Adding slave %s.\n", bond->dev->name, ifname); dev = dev_get_by_name(&init_net, ifname); @@ -1080,6 +1080,9 @@ static ssize_t bonding_store_primary(struct device *d, } out: write_unlock_bh(&bond->lock); + + rtnl_unlock(); + return count; } static DEVICE_ATTR(primary, S_IRUGO | S_IWUSR, bonding_show_primary, bonding_store_primary); @@ -1137,6 +1140,7 @@ static ssize_t bonding_show_active_slave(struct device *d, struct bonding *bond = to_bond(d); int count; + rtnl_lock(); read_lock(&bond->curr_slave_lock); curr = bond->curr_active_slave; @@ -1216,6 +1220,8 @@ static ssize_t bonding_store_active_slave(struct device *d, } out: write_unlock_bh(&bond->lock); + rtnl_unlock(); + return count; } -- 1.5.3.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 6/6] Acquire correct locks in alb for promisc change 2007-10-11 4:14 ` [PATCH 5/6] Convert more locks to _bh, acquire rtnl, " Jay Vosburgh @ 2007-10-11 4:14 ` Jay Vosburgh 0 siblings, 0 replies; 17+ messages in thread From: Jay Vosburgh @ 2007-10-11 4:14 UTC (permalink / raw) To: netdev, jgarzik; +Cc: andy, Jay Vosburgh Update ALB mode monitor to hold correct locks (RTNL and nothing else) when calling dev_set_promiscuity. Signed-off-by: Andy Gospodarek <andy@greyhouse.net> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com> --- drivers/net/bonding/bond_alb.c | 19 ++++++++++--------- 1 files changed, 10 insertions(+), 9 deletions(-) diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c index 6db5d76..25b8dbf 100644 --- a/drivers/net/bonding/bond_alb.c +++ b/drivers/net/bonding/bond_alb.c @@ -1455,16 +1455,16 @@ void bond_alb_monitor(struct work_struct *work) /* handle rlb stuff */ if (bond_info->rlb_enabled) { - /* the following code changes the promiscuity of the - * the curr_active_slave. It needs to be locked with a - * write lock to protect from other code that also - * sets the promiscuity. - */ - write_lock_bh(&bond->curr_slave_lock); - if (bond_info->primary_is_promisc && (++bond_info->rlb_promisc_timeout_counter >= RLB_PROMISC_TIMEOUT)) { + /* + * dev_set_promiscuity requires rtnl and + * nothing else. + */ + read_unlock(&bond->lock); + rtnl_lock(); + bond_info->rlb_promisc_timeout_counter = 0; /* If the primary was set to promiscuous mode @@ -1473,9 +1473,10 @@ void bond_alb_monitor(struct work_struct *work) */ dev_set_promiscuity(bond->curr_active_slave->dev, -1); bond_info->primary_is_promisc = 0; - } - write_unlock_bh(&bond->curr_slave_lock); + rtnl_unlock(); + read_lock(&bond->lock); + } if (bond_info->rlb_rebalance) { bond_info->rlb_rebalance = 0; -- 1.5.3.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/6] Convert bonding timers to workqueues 2007-10-11 4:14 ` [PATCH 1/6] Convert bonding timers to workqueues Jay Vosburgh 2007-10-11 4:14 ` [PATCH 2/6] Convert balance-rr transmit to new locking Jay Vosburgh @ 2007-10-15 18:41 ` Jeff Garzik 2007-10-15 19:08 ` Jay Vosburgh 1 sibling, 1 reply; 17+ messages in thread From: Jeff Garzik @ 2007-10-15 18:41 UTC (permalink / raw) To: Jay Vosburgh; +Cc: netdev, andy Jay Vosburgh wrote: > Convert bonding timers to workqueues. This converts the various > monitor functions to run in periodic work queues instead of timers. This > patch introduces the framework and convers the calls, but does not resolve > various locking issues, and does not stand alone. > > Signed-off-by: Andy Gospodarek <andy@greyhouse.net> > Signed-off-by: Jay Vosburgh <fubar@us.ibm.com> "does not stand alone" == it is not bisectable? That's a problem. Anyway, we have more fun fish to fry: after applying the IPoIB bonding patchset, this no longer applies... ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/6] Convert bonding timers to workqueues 2007-10-15 18:41 ` [PATCH 1/6] Convert bonding timers to workqueues Jeff Garzik @ 2007-10-15 19:08 ` Jay Vosburgh 2007-10-22 14:55 ` Mike Snitzer 0 siblings, 1 reply; 17+ messages in thread From: Jay Vosburgh @ 2007-10-15 19:08 UTC (permalink / raw) To: Jeff Garzik; +Cc: netdev, andy Jeff Garzik <jgarzik@pobox.com> wrote: >Jay Vosburgh wrote: >> Convert bonding timers to workqueues. This converts the various >> monitor functions to run in periodic work queues instead of timers. This >> patch introduces the framework and convers the calls, but does not resolve >> various locking issues, and does not stand alone. >> >> Signed-off-by: Andy Gospodarek <andy@greyhouse.net> >> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com> > >"does not stand alone" == it is not bisectable? That's a problem. The patch will compile fine (this is true for any point in the series), but it's possible for bonding to deadlock or misbehave when doing certain operations. Those problems are addressed in the later patches. I didn't want to just vomit out one big mega-patch that has the whole set of changes, because that's harder to understand, and the later patches generally address discrete issues. Does that make you more or less nervous about its bisectability? I can repost the whole thing as a big blob if that's what you'd prefer. >Anyway, we have more fun fish to fry: after applying the IPoIB bonding >patchset, this no longer applies... Yah, I'll rebase it (once I know your blob vs. not-blob preference). I wasn't sure how long the IPoIB mystery bug hunt was going to take. -J --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/6] Convert bonding timers to workqueues 2007-10-15 19:08 ` Jay Vosburgh @ 2007-10-22 14:55 ` Mike Snitzer 2007-10-22 18:34 ` Jeff Garzik 0 siblings, 1 reply; 17+ messages in thread From: Mike Snitzer @ 2007-10-22 14:55 UTC (permalink / raw) To: Jeff Garzik; +Cc: Jay Vosburgh, netdev, andy On 10/15/07, Jay Vosburgh <fubar@us.ibm.com> wrote: > Jeff Garzik <jgarzik@pobox.com> wrote: > > >Jay Vosburgh wrote: > >> Convert bonding timers to workqueues. This converts the various > >> monitor functions to run in periodic work queues instead of timers. This > >> patch introduces the framework and convers the calls, but does not resolve > >> various locking issues, and does not stand alone. > >> > >> Signed-off-by: Andy Gospodarek <andy@greyhouse.net> > >> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com> > > > >"does not stand alone" == it is not bisectable? That's a problem. > > The patch will compile fine (this is true for any point in the > series), but it's possible for bonding to deadlock or misbehave when > doing certain operations. Those problems are addressed in the later > patches. > > I didn't want to just vomit out one big mega-patch that has the > whole set of changes, because that's harder to understand, and the later > patches generally address discrete issues. > > Does that make you more or less nervous about its bisectability? > I can repost the whole thing as a big blob if that's what you'd prefer. > > >Anyway, we have more fun fish to fry: after applying the IPoIB bonding > >patchset, this no longer applies... > > Yah, I'll rebase it (once I know your blob vs. not-blob > preference). I wasn't sure how long the IPoIB mystery bug hunt was > going to take. Jeff, Given that Jay rebased and posted updated bonding locking patches on Oct 17 will you be pushing them for inclusion in 2.6.24-rc1? or 2.6.24-rcX? I don't see them queued in your netdev-2.6 tree. These bonding locking issues have been a problem for some time and it would be very good to get them fixed upstream sooner rather than later. thanks, Mike ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/6] Convert bonding timers to workqueues 2007-10-22 14:55 ` Mike Snitzer @ 2007-10-22 18:34 ` Jeff Garzik 2007-10-22 18:57 ` Jay Vosburgh 0 siblings, 1 reply; 17+ messages in thread From: Jeff Garzik @ 2007-10-22 18:34 UTC (permalink / raw) To: Mike Snitzer, Jay Vosburgh; +Cc: netdev, andy Mike Snitzer wrote: > Given that Jay rebased and posted updated bonding locking patches on > Oct 17 will you be pushing them for inclusion in 2.6.24-rc1? or > 2.6.24-rcX? I don't see them queued in your netdev-2.6 tree. These > bonding locking issues have been a problem for some time and it would > be very good to get them fixed upstream sooner rather than later. I wasn't aware this brand-new code is a bug fix? Normally brand new code needs to be posted and reviewed and committed to a tree -before- the merge window opens. Jeff ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/6] Convert bonding timers to workqueues 2007-10-22 18:34 ` Jeff Garzik @ 2007-10-22 18:57 ` Jay Vosburgh 0 siblings, 0 replies; 17+ messages in thread From: Jay Vosburgh @ 2007-10-22 18:57 UTC (permalink / raw) To: Jeff Garzik; +Cc: Mike Snitzer, netdev, andy Jeff Garzik <jeff@garzik.org> wrote: [...] >I wasn't aware this brand-new code is a bug fix? > >Normally brand new code needs to be posted and reviewed and committed to a >tree -before- the merge window opens. I had thought this was going in ok, as you'd attempted to apply it after the IPoIB stuff a week or so ago, but it needed to be rebased and you'd had concerns about bisect. In any event, I'd argue it's changes to fix locking bugs (various "sleep with lock held" and ASSERT_RTNLs, e.g., bug 7974 on bugzilla.kernel.org); this isn't a feature add or a refactor just for efficiency. -J --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 0/6] bonding workqueue and lock rework @ 2007-10-18 0:37 Jay Vosburgh 2007-10-18 0:37 ` [PATCH 1/6] Convert bonding timers to workqueues Jay Vosburgh 0 siblings, 1 reply; 17+ messages in thread From: Jay Vosburgh @ 2007-10-18 0:37 UTC (permalink / raw) To: netdev, jgarzik; +Cc: andy Following are patches to update the locking used in the bonding driver. This involves two basic changes: conversion from timers to workqueues for the various periodic monitor functions, and conversion of locking. These patches should resolve the majority of locking and might sleep related warnings that occur during certain operations. The workqueue conversion is fairly straightforward, and substitutes workqueue driven events for the existing timer driven events. The locking changes are of two types: first, changes to resolve deadlocks created by the conversion to workqueues, and second, changes to make the locking more correct, which generally involves holding RTNL and no other locks during specific operations. Some of the RTNL-related changes are fairly extensive, and involve either conditional locking or releasing and reacquiring locks in order to obey lock ordering constraints. These changes were developed and extensively tested by Andy Gospodarek <andy@greyhouse.net> and myself over the last few months. Patches are relative to linux-2.6. -J --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/6] Convert bonding timers to workqueues 2007-10-18 0:37 [PATCH 0/6] bonding workqueue and lock rework Jay Vosburgh @ 2007-10-18 0:37 ` Jay Vosburgh 2007-10-24 0:42 ` Jeff Garzik 0 siblings, 1 reply; 17+ messages in thread From: Jay Vosburgh @ 2007-10-18 0:37 UTC (permalink / raw) To: netdev, jgarzik; +Cc: andy, Jay Vosburgh Convert bonding timers to workqueues. This converts the various monitor functions to run in periodic work queues instead of timers. This patch introduces the framework and convers the calls, but does not resolve various locking issues, and does not stand alone. Signed-off-by: Andy Gospodarek <andy@greyhouse.net> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com> --- drivers/net/bonding/bond_3ad.c | 6 +- drivers/net/bonding/bond_3ad.h | 2 +- drivers/net/bonding/bond_alb.c | 6 +- drivers/net/bonding/bond_alb.h | 2 +- drivers/net/bonding/bond_main.c | 147 ++++++++++++++++++++----------------- drivers/net/bonding/bond_sysfs.c | 63 +++++----------- drivers/net/bonding/bonding.h | 13 ++-- 7 files changed, 117 insertions(+), 122 deletions(-) diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c index 7a045a3..2471d3b 100644 --- a/drivers/net/bonding/bond_3ad.c +++ b/drivers/net/bonding/bond_3ad.c @@ -2074,8 +2074,10 @@ void bond_3ad_unbind_slave(struct slave *slave) * times out, and it selects an aggregator for the ports that are yet not * related to any aggregator, and selects the active aggregator for a bond. */ -void bond_3ad_state_machine_handler(struct bonding *bond) +void bond_3ad_state_machine_handler(struct work_struct *work) { + struct bonding *bond = container_of(work, struct bonding, + ad_work.work); struct port *port; struct aggregator *aggregator; @@ -2126,7 +2128,7 @@ void bond_3ad_state_machine_handler(struct bonding *bond) } re_arm: - mod_timer(&(BOND_AD_INFO(bond).ad_timer), jiffies + ad_delta_in_ticks); + queue_delayed_work(bond->wq, &bond->ad_work, ad_delta_in_ticks); out: read_unlock(&bond->lock); } diff --git a/drivers/net/bonding/bond_3ad.h b/drivers/net/bonding/bond_3ad.h index 862952f..dde02b0 100644 --- a/drivers/net/bonding/bond_3ad.h +++ b/drivers/net/bonding/bond_3ad.h @@ -276,7 +276,7 @@ struct ad_slave_info { void bond_3ad_initialize(struct bonding *bond, u16 tick_resolution, int lacp_fast); int bond_3ad_bind_slave(struct slave *slave); void bond_3ad_unbind_slave(struct slave *slave); -void bond_3ad_state_machine_handler(struct bonding *bond); +void bond_3ad_state_machine_handler(struct work_struct *); void bond_3ad_adapter_speed_changed(struct slave *slave); void bond_3ad_adapter_duplex_changed(struct slave *slave); void bond_3ad_handle_link_change(struct slave *slave, char link); diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c index aea2217..eb320c3 100644 --- a/drivers/net/bonding/bond_alb.c +++ b/drivers/net/bonding/bond_alb.c @@ -1375,8 +1375,10 @@ out: return 0; } -void bond_alb_monitor(struct bonding *bond) +void bond_alb_monitor(struct work_struct *work) { + struct bonding *bond = container_of(work, struct bonding, + alb_work.work); struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond)); struct slave *slave; int i; @@ -1479,7 +1481,7 @@ void bond_alb_monitor(struct bonding *bond) } re_arm: - mod_timer(&(bond_info->alb_timer), jiffies + alb_delta_in_ticks); + queue_delayed_work(bond->wq, &bond->alb_work, alb_delta_in_ticks); out: read_unlock(&bond->lock); } diff --git a/drivers/net/bonding/bond_alb.h b/drivers/net/bonding/bond_alb.h index fd87264..50968f8 100644 --- a/drivers/net/bonding/bond_alb.h +++ b/drivers/net/bonding/bond_alb.h @@ -125,7 +125,7 @@ void bond_alb_deinit_slave(struct bonding *bond, struct slave *slave); void bond_alb_handle_link_change(struct bonding *bond, struct slave *slave, char link); void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave); int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev); -void bond_alb_monitor(struct bonding *bond); +void bond_alb_monitor(struct work_struct *); int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr); void bond_alb_clear_vlan(struct bonding *bond, unsigned short vlan_id); #endif /* __BOND_ALB_H__ */ diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 6f85cc3..ed361d6 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -2089,9 +2089,10 @@ static int bond_slave_info_query(struct net_device *bond_dev, struct ifslave *in /*-------------------------------- Monitoring -------------------------------*/ /* this function is called regularly to monitor each slave's link. */ -void bond_mii_monitor(struct net_device *bond_dev) +void bond_mii_monitor(struct work_struct *work) { - struct bonding *bond = bond_dev->priv; + struct bonding *bond = container_of(work, struct bonding, + mii_work.work); struct slave *slave, *oldcurrent; int do_failover = 0; int delta_in_ticks; @@ -2156,7 +2157,7 @@ void bond_mii_monitor(struct net_device *bond_dev) ": %s: link status down for %s " "interface %s, disabling it in " "%d ms.\n", - bond_dev->name, + bond->dev->name, IS_UP(slave_dev) ? ((bond->params.mode == BOND_MODE_ACTIVEBACKUP) ? ((slave == oldcurrent) @@ -2189,7 +2190,7 @@ void bond_mii_monitor(struct net_device *bond_dev) ": %s: link status definitely " "down for interface %s, " "disabling it\n", - bond_dev->name, + bond->dev->name, slave_dev->name); /* notify ad that the link status has changed */ @@ -2215,7 +2216,7 @@ void bond_mii_monitor(struct net_device *bond_dev) printk(KERN_INFO DRV_NAME ": %s: link status up again after %d " "ms for interface %s.\n", - bond_dev->name, + bond->dev->name, (bond->params.downdelay - slave->delay) * bond->params.miimon, slave_dev->name); } @@ -2235,7 +2236,7 @@ void bond_mii_monitor(struct net_device *bond_dev) ": %s: link status up for " "interface %s, enabling it " "in %d ms.\n", - bond_dev->name, + bond->dev->name, slave_dev->name, bond->params.updelay * bond->params.miimon); } @@ -2251,7 +2252,7 @@ void bond_mii_monitor(struct net_device *bond_dev) printk(KERN_INFO DRV_NAME ": %s: link status down again after %d " "ms for interface %s.\n", - bond_dev->name, + bond->dev->name, (bond->params.updelay - slave->delay) * bond->params.miimon, slave_dev->name); } else { @@ -2275,7 +2276,7 @@ void bond_mii_monitor(struct net_device *bond_dev) printk(KERN_INFO DRV_NAME ": %s: link status definitely " "up for interface %s.\n", - bond_dev->name, + bond->dev->name, slave_dev->name); /* notify ad that the link status has changed */ @@ -2301,7 +2302,7 @@ void bond_mii_monitor(struct net_device *bond_dev) /* Should not happen */ printk(KERN_ERR DRV_NAME ": %s: Error: %s Illegal value (link=%d)\n", - bond_dev->name, + bond->dev->name, slave->dev->name, slave->link); goto out; @@ -2331,9 +2332,8 @@ void bond_mii_monitor(struct net_device *bond_dev) bond_set_carrier(bond); re_arm: - if (bond->params.miimon) { - mod_timer(&bond->mii_timer, jiffies + delta_in_ticks); - } + if (bond->params.miimon) + queue_delayed_work(bond->wq, &bond->mii_work, delta_in_ticks); out: read_unlock(&bond->lock); } @@ -2636,9 +2636,10 @@ out: * arp is transmitted to generate traffic. see activebackup_arp_monitor for * arp monitoring in active backup mode. */ -void bond_loadbalance_arp_mon(struct net_device *bond_dev) +void bond_loadbalance_arp_mon(struct work_struct *work) { - struct bonding *bond = bond_dev->priv; + struct bonding *bond = container_of(work, struct bonding, + arp_work.work); struct slave *slave, *oldcurrent; int do_failover = 0; int delta_in_ticks; @@ -2685,13 +2686,13 @@ void bond_loadbalance_arp_mon(struct net_device *bond_dev) printk(KERN_INFO DRV_NAME ": %s: link status definitely " "up for interface %s, ", - bond_dev->name, + bond->dev->name, slave->dev->name); do_failover = 1; } else { printk(KERN_INFO DRV_NAME ": %s: interface %s is now up\n", - bond_dev->name, + bond->dev->name, slave->dev->name); } } @@ -2715,7 +2716,7 @@ void bond_loadbalance_arp_mon(struct net_device *bond_dev) printk(KERN_INFO DRV_NAME ": %s: interface %s is now down.\n", - bond_dev->name, + bond->dev->name, slave->dev->name); if (slave == oldcurrent) { @@ -2745,9 +2746,8 @@ void bond_loadbalance_arp_mon(struct net_device *bond_dev) } re_arm: - if (bond->params.arp_interval) { - mod_timer(&bond->arp_timer, jiffies + delta_in_ticks); - } + if (bond->params.arp_interval) + queue_delayed_work(bond->wq, &bond->arp_work, delta_in_ticks); out: read_unlock(&bond->lock); } @@ -2767,9 +2767,10 @@ out: * may have received. * see loadbalance_arp_monitor for arp monitoring in load balancing mode */ -void bond_activebackup_arp_mon(struct net_device *bond_dev) +void bond_activebackup_arp_mon(struct work_struct *work) { - struct bonding *bond = bond_dev->priv; + struct bonding *bond = container_of(work, struct bonding, + arp_work.work); struct slave *slave; int delta_in_ticks; int i; @@ -2821,14 +2822,14 @@ void bond_activebackup_arp_mon(struct net_device *bond_dev) printk(KERN_INFO DRV_NAME ": %s: %s is up and now the " "active interface\n", - bond_dev->name, + bond->dev->name, slave->dev->name); netif_carrier_on(bond->dev); } else { printk(KERN_INFO DRV_NAME ": %s: backup interface %s is " "now up\n", - bond_dev->name, + bond->dev->name, slave->dev->name); } @@ -2864,7 +2865,7 @@ void bond_activebackup_arp_mon(struct net_device *bond_dev) printk(KERN_INFO DRV_NAME ": %s: backup interface %s is now down\n", - bond_dev->name, + bond->dev->name, slave->dev->name); } else { read_unlock(&bond->curr_slave_lock); @@ -2899,7 +2900,7 @@ void bond_activebackup_arp_mon(struct net_device *bond_dev) printk(KERN_INFO DRV_NAME ": %s: link status down for active interface " "%s, disabling it\n", - bond_dev->name, + bond->dev->name, slave->dev->name); write_lock(&bond->curr_slave_lock); @@ -2921,7 +2922,7 @@ void bond_activebackup_arp_mon(struct net_device *bond_dev) printk(KERN_INFO DRV_NAME ": %s: changing from interface %s to primary " "interface %s\n", - bond_dev->name, + bond->dev->name, slave->dev->name, bond->primary_slave->dev->name); @@ -2985,7 +2986,7 @@ void bond_activebackup_arp_mon(struct net_device *bond_dev) printk(KERN_INFO DRV_NAME ": %s: backup interface %s is " "now down.\n", - bond_dev->name, + bond->dev->name, slave->dev->name); } } @@ -2994,7 +2995,7 @@ void bond_activebackup_arp_mon(struct net_device *bond_dev) re_arm: if (bond->params.arp_interval) { - mod_timer(&bond->arp_timer, jiffies + delta_in_ticks); + queue_delayed_work(bond->wq, &bond->arp_work, delta_in_ticks); } out: read_unlock(&bond->lock); @@ -3582,15 +3583,11 @@ static int bond_xmit_hash_policy_l2(struct sk_buff *skb, static int bond_open(struct net_device *bond_dev) { struct bonding *bond = bond_dev->priv; - struct timer_list *mii_timer = &bond->mii_timer; - struct timer_list *arp_timer = &bond->arp_timer; bond->kill_timers = 0; if ((bond->params.mode == BOND_MODE_TLB) || (bond->params.mode == BOND_MODE_ALB)) { - struct timer_list *alb_timer = &(BOND_ALB_INFO(bond).alb_timer); - /* bond_alb_initialize must be called before the timer * is started. */ @@ -3599,44 +3596,31 @@ static int bond_open(struct net_device *bond_dev) return -1; } - init_timer(alb_timer); - alb_timer->expires = jiffies + 1; - alb_timer->data = (unsigned long)bond; - alb_timer->function = (void *)&bond_alb_monitor; - add_timer(alb_timer); + 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_timer(mii_timer); - mii_timer->expires = jiffies + 1; - mii_timer->data = (unsigned long)bond_dev; - mii_timer->function = (void *)&bond_mii_monitor; - add_timer(mii_timer); + 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. */ - init_timer(arp_timer); - arp_timer->expires = jiffies + 1; - arp_timer->data = (unsigned long)bond_dev; - if (bond->params.mode == BOND_MODE_ACTIVEBACKUP) { - arp_timer->function = (void *)&bond_activebackup_arp_mon; - } else { - arp_timer->function = (void *)&bond_loadbalance_arp_mon; - } + 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.arp_validate) bond_register_arp(bond); - - add_timer(arp_timer); } if (bond->params.mode == BOND_MODE_8023AD) { - struct timer_list *ad_timer = &(BOND_AD_INFO(bond).ad_timer); - init_timer(ad_timer); - ad_timer->expires = jiffies + 1; - ad_timer->data = (unsigned long)bond; - ad_timer->function = (void *)&bond_3ad_state_machine_handler; - add_timer(ad_timer); - + INIT_DELAYED_WORK(&bond->ad_work, bond_alb_monitor); + queue_delayed_work(bond->wq, &bond->ad_work, 0); /* register to receive LACPDUs */ bond_register_lacpdu(bond); } @@ -3664,25 +3648,21 @@ static int bond_close(struct net_device *bond_dev) write_unlock_bh(&bond->lock); - /* del_timer_sync must run without holding the bond->lock - * because a running timer might be trying to hold it too - */ - if (bond->params.miimon) { /* link check interval, in milliseconds. */ - del_timer_sync(&bond->mii_timer); + cancel_delayed_work(&bond->mii_work); } if (bond->params.arp_interval) { /* arp interval, in milliseconds. */ - del_timer_sync(&bond->arp_timer); + cancel_delayed_work(&bond->arp_work); } switch (bond->params.mode) { case BOND_MODE_8023AD: - del_timer_sync(&(BOND_AD_INFO(bond).ad_timer)); + cancel_delayed_work(&bond->ad_work); break; case BOND_MODE_TLB: case BOND_MODE_ALB: - del_timer_sync(&(BOND_ALB_INFO(bond).alb_timer)); + cancel_delayed_work(&bond->alb_work); break; default: break; @@ -4340,6 +4320,10 @@ static int bond_init(struct net_device *bond_dev, struct bond_params *params) bond->params = *params; /* copy params struct */ + bond->wq = create_singlethread_workqueue(bond_dev->name); + if (!bond->wq) + return -ENOMEM; + /* Initialize pointers */ bond->first_slave = NULL; bond->curr_active_slave = NULL; @@ -4826,10 +4810,32 @@ out_rtnl: return res; } +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 && delayed_work_pending(&bond->mii_work)) + 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.mode == BOND_MODE_ALB && + delayed_work_pending(&bond->alb_work)) + cancel_delayed_work(&bond->alb_work); + + if (bond->params.mode == BOND_MODE_8023AD && + delayed_work_pending(&bond->ad_work)) + cancel_delayed_work(&bond->ad_work); +} + static int __init bonding_init(void) { int i; int res; + struct bonding *bond, *nxt; printk(KERN_INFO "%s", version); @@ -4856,6 +4862,11 @@ 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(); diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c index 80c0c8c..ffa62a7 100644 --- a/drivers/net/bonding/bond_sysfs.c +++ b/drivers/net/bonding/bond_sysfs.c @@ -662,12 +662,9 @@ static ssize_t bonding_store_arp_interval(struct device *d, "%s Disabling MII monitoring.\n", bond->dev->name, bond->dev->name); bond->params.miimon = 0; - /* Kill MII timer, else it brings bond's link down */ - if (bond->arp_timer.function) { - printk(KERN_INFO DRV_NAME - ": %s: Kill MII timer, else it brings bond's link down...\n", - bond->dev->name); - del_timer_sync(&bond->mii_timer); + if (delayed_work_pending(&bond->mii_work)) { + cancel_delayed_work(&bond->mii_work); + flush_workqueue(bond->wq); } } if (!bond->params.arp_targets[0]) { @@ -682,25 +679,15 @@ static ssize_t bonding_store_arp_interval(struct device *d, * timer will get fired off when the open function * is called. */ - if (bond->arp_timer.function) { - /* The timer's already set up, so fire it off */ - mod_timer(&bond->arp_timer, jiffies + 1); - } else { - /* Set up the timer. */ - init_timer(&bond->arp_timer); - bond->arp_timer.expires = jiffies + 1; - bond->arp_timer.data = - (unsigned long) bond->dev; - if (bond->params.mode == BOND_MODE_ACTIVEBACKUP) { - bond->arp_timer.function = - (void *) - &bond_activebackup_arp_mon; - } else { - bond->arp_timer.function = - (void *) - &bond_loadbalance_arp_mon; - } - add_timer(&bond->arp_timer); + 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); } } @@ -1056,12 +1043,9 @@ static ssize_t bonding_store_miimon(struct device *d, bond->params.arp_validate = BOND_ARP_VALIDATE_NONE; } - /* Kill ARP timer, else it brings bond's link down */ - if (bond->mii_timer.function) { - printk(KERN_INFO DRV_NAME - ": %s: Kill ARP timer, else it brings bond's link down...\n", - bond->dev->name); - del_timer_sync(&bond->arp_timer); + if (delayed_work_pending(&bond->arp_work)) { + cancel_delayed_work(&bond->arp_work); + flush_workqueue(bond->wq); } } @@ -1071,18 +1055,11 @@ static ssize_t bonding_store_miimon(struct device *d, * timer will get fired off when the open function * is called. */ - if (bond->mii_timer.function) { - /* The timer's already set up, so fire it off */ - mod_timer(&bond->mii_timer, jiffies + 1); - } else { - /* Set up the timer. */ - init_timer(&bond->mii_timer); - bond->mii_timer.expires = jiffies + 1; - bond->mii_timer.data = - (unsigned long) bond->dev; - bond->mii_timer.function = - (void *) &bond_mii_monitor; - add_timer(&bond->mii_timer); + 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); } } } diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h index b818060..a8f2384 100644 --- a/drivers/net/bonding/bonding.h +++ b/drivers/net/bonding/bonding.h @@ -184,8 +184,6 @@ struct bonding { s32 slave_cnt; /* never change this value outside the attach/detach wrappers */ rwlock_t lock; rwlock_t curr_slave_lock; - struct timer_list mii_timer; - struct timer_list arp_timer; s8 kill_timers; s8 send_grat_arp; s8 setup_by_slave; @@ -205,6 +203,11 @@ struct bonding { struct list_head vlan_list; struct vlan_group *vlgrp; struct packet_type arp_mon_pt; + struct workqueue_struct *wq; + struct delayed_work mii_work; + struct delayed_work arp_work; + struct delayed_work alb_work; + struct delayed_work ad_work; }; /** @@ -307,9 +310,9 @@ int bond_create_slave_symlinks(struct net_device *master, struct net_device *sla void bond_destroy_slave_symlinks(struct net_device *master, struct net_device *slave); int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev); int bond_release(struct net_device *bond_dev, struct net_device *slave_dev); -void bond_mii_monitor(struct net_device *bond_dev); -void bond_loadbalance_arp_mon(struct net_device *bond_dev); -void bond_activebackup_arp_mon(struct net_device *bond_dev); +void bond_mii_monitor(struct work_struct *); +void bond_loadbalance_arp_mon(struct work_struct *); +void bond_activebackup_arp_mon(struct work_struct *); void bond_set_mode_ops(struct bonding *bond, int mode); int bond_parse_parm(char *mode_arg, struct bond_parm_tbl *tbl); void bond_select_active_slave(struct bonding *bond); -- 1.5.3.4.206.g58ba4-dirty ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/6] Convert bonding timers to workqueues 2007-10-18 0:37 ` [PATCH 1/6] Convert bonding timers to workqueues Jay Vosburgh @ 2007-10-24 0:42 ` Jeff Garzik 2007-10-24 1:00 ` Jay Vosburgh 0 siblings, 1 reply; 17+ messages in thread From: Jeff Garzik @ 2007-10-24 0:42 UTC (permalink / raw) To: Jay Vosburgh; +Cc: netdev, andy Jay Vosburgh wrote: > Convert bonding timers to workqueues. This converts the various > monitor functions to run in periodic work queues instead of timers. This > patch introduces the framework and convers the calls, but does not resolve > various locking issues, and does not stand alone. > > Signed-off-by: Andy Gospodarek <andy@greyhouse.net> > Signed-off-by: Jay Vosburgh <fubar@us.ibm.com> > --- > drivers/net/bonding/bond_3ad.c | 6 +- > drivers/net/bonding/bond_3ad.h | 2 +- > drivers/net/bonding/bond_alb.c | 6 +- > drivers/net/bonding/bond_alb.h | 2 +- > drivers/net/bonding/bond_main.c | 147 ++++++++++++++++++++----------------- > drivers/net/bonding/bond_sysfs.c | 63 +++++----------- > drivers/net/bonding/bonding.h | 13 ++-- > 7 files changed, 117 insertions(+), 122 deletions(-) applied patches 1-6 However, two issues: 1) Credit. Did Andy write these, as the Signed-off-by lines indicate? If so, you should reflect that with a From: Andy Gospodarek <andy@greyhouse.net> as the first line of your email body. Linus's git tool will automatically notice this special first-line header, and appropriately credit the right person. 2) vague subject line. _Every_ subject line is copied verbatim into the kernel changelog, preserved as a permanent one-line summary of the changeset. As such, with considered in the context of the entire kernel, Convert more locks to _bh, acquire rtnl, for new locking tells us nothing at all about the portion of the kernel being modified. In the future, make sure to include "bonding" somewhere in the subject line. Generally a "bonding: " prefix would probably be best, but it can fall elsewhere as your patch #1 subject line indicates. I hand-edited these subject lines to add "bonding: " prefix before I applied the patchset. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/6] Convert bonding timers to workqueues 2007-10-24 0:42 ` Jeff Garzik @ 2007-10-24 1:00 ` Jay Vosburgh 2007-10-24 1:13 ` Andy Gospodarek 0 siblings, 1 reply; 17+ messages in thread From: Jay Vosburgh @ 2007-10-24 1:00 UTC (permalink / raw) To: Jeff Garzik; +Cc: netdev, andy Jeff Garzik <jeff@garzik.org> wrote: >applied patches 1-6 Thanks. >However, two issues: > >1) Credit. Did Andy write these, as the Signed-off-by lines indicate? Andy did some of it and I did some of it, so I presume that dual Signed-off-by is correct (vs. one Signed-off and one Acked). >2) vague subject line. [...] Sorry about that; will keep it in mind next time. -J --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/6] Convert bonding timers to workqueues 2007-10-24 1:00 ` Jay Vosburgh @ 2007-10-24 1:13 ` Andy Gospodarek 2007-10-24 1:19 ` Jeff Garzik 0 siblings, 1 reply; 17+ messages in thread From: Andy Gospodarek @ 2007-10-24 1:13 UTC (permalink / raw) To: Jay Vosburgh; +Cc: Jeff Garzik, netdev, andy On Tue, Oct 23, 2007 at 06:00:01PM -0700, Jay Vosburgh wrote: > Jeff Garzik <jeff@garzik.org> wrote: > > >applied patches 1-6 > > Thanks. > > >However, two issues: > > > >1) Credit. Did Andy write these, as the Signed-off-by lines indicate? > > Andy did some of it and I did some of it, so I presume that dual I was just getting ready to post the same thing. Jay beat me to it! > Signed-off-by is correct (vs. one Signed-off and one Acked). > I figured the dual 'Signed-off-by:' was the best way too since I didn't think Linus had dual 'From:' lines. > >2) vague subject line. [...] > > Sorry about that; will keep it in mind next time. > > -J > > --- > -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com > - > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/6] Convert bonding timers to workqueues 2007-10-24 1:13 ` Andy Gospodarek @ 2007-10-24 1:19 ` Jeff Garzik 0 siblings, 0 replies; 17+ messages in thread From: Jeff Garzik @ 2007-10-24 1:19 UTC (permalink / raw) To: Andy Gospodarek, Jay Vosburgh; +Cc: netdev Andy Gospodarek wrote: > On Tue, Oct 23, 2007 at 06:00:01PM -0700, Jay Vosburgh wrote: >> Jeff Garzik <jeff@garzik.org> wrote: >> >>> applied patches 1-6 >> Thanks. >> >>> However, two issues: >>> >>> 1) Credit. Did Andy write these, as the Signed-off-by lines indicate? >> Andy did some of it and I did some of it, so I presume that dual > > I was just getting ready to post the same thing. Jay beat me to it! Cool, just checking. Given the situation, it's pretty much "maintainer's best judgement" at that point... Thanks, Jeff ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2007-10-24 1:19 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-10-11 4:14 [PATCH 0/6] bonding workqueue and lock rework Jay Vosburgh 2007-10-11 4:14 ` [PATCH 1/6] Convert bonding timers to workqueues Jay Vosburgh 2007-10-11 4:14 ` [PATCH 2/6] Convert balance-rr transmit to new locking Jay Vosburgh 2007-10-11 4:14 ` [PATCH 3/6] Convert miimon " Jay Vosburgh 2007-10-11 4:14 ` [PATCH 4/6] Convert locks to _bh, rework alb locking for " Jay Vosburgh 2007-10-11 4:14 ` [PATCH 5/6] Convert more locks to _bh, acquire rtnl, " Jay Vosburgh 2007-10-11 4:14 ` [PATCH 6/6] Acquire correct locks in alb for promisc change Jay Vosburgh 2007-10-15 18:41 ` [PATCH 1/6] Convert bonding timers to workqueues Jeff Garzik 2007-10-15 19:08 ` Jay Vosburgh 2007-10-22 14:55 ` Mike Snitzer 2007-10-22 18:34 ` Jeff Garzik 2007-10-22 18:57 ` Jay Vosburgh -- strict thread matches above, loose matches on Subject: below -- 2007-10-18 0:37 [PATCH 0/6] bonding workqueue and lock rework Jay Vosburgh 2007-10-18 0:37 ` [PATCH 1/6] Convert bonding timers to workqueues Jay Vosburgh 2007-10-24 0:42 ` Jeff Garzik 2007-10-24 1:00 ` Jay Vosburgh 2007-10-24 1:13 ` Andy Gospodarek 2007-10-24 1:19 ` Jeff Garzik
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).